Fix K8S executor override config using pod_override_object - #35185

Merged
hussein-awala merged 13 commits into
apache:mainfrom
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged

Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom
hussein-awala:fix/k8s_executor_ns_override

Conversation

@hussein-awala

@hussein-awalahussein-awala commented Oct 25, 2023

Copy link
Copy Markdown
Member

related: #22298

Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).

This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.

@boring-cyborgboring-cyborgBot added area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues labels Oct 25, 2023
eladkal
eladkal approved these changes Oct 25, 2023
View reviewed changes

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!

@eladkal

eladkal commented Oct 25, 2023

Copy link
Copy Markdown
Contributor

I am not sure why helm chart tests are skipped here cc @potiuk
In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK

@eladkaleladkal added the full tests needed We need to run full set of tests for this PR to merge label Oct 25, 2023
@eladkaleladkal closed this Oct 25, 2023
@eladkaleladkal reopened this Oct 25, 2023
@eladkal

eladkal commented Oct 26, 2023

Copy link
Copy Markdown
Contributor

Helm tests are failing :(

@hussein-awala

hussein-awala commented Oct 26, 2023

Copy link
Copy Markdown
MemberAuthor

I'm still trying to reproduce the tests failure locally:

test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED

@hussein-awalahussein-awala mentioned this pull request Oct 27, 2023
Closed
@hussein-awala

hussein-awala commented Oct 27, 2023

Copy link
Copy Markdown
MemberAuthor

@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.

@potiuk
potiukforce-pushed the fix/k8s_executor_ns_override branch from 5ebc0c9 to d98b7ceCompareOctober 27, 2023 21:38
@potiuk

potiuk commented Oct 27, 2023

Copy link
Copy Markdown
Member

This branch is 4 commits ahead, 18 commits behind apache:main.

I just rebased it. I think #35191 should fix it.

@potiuk

potiuk commented Oct 27, 2023

Copy link
Copy Markdown
Member

But let's see.

@potiuk

potiuk commented Oct 27, 2023

Copy link
Copy Markdown
Member

I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests

Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.

@hussein-awala

hussein-awala commented Oct 27, 2023

Copy link
Copy Markdown
MemberAuthor

I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests

Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.

I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.

I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.

@potiukpotiuk removed the full tests needed We need to run full set of tests for this PR to merge label Oct 28, 2023
@potiuk
potiukforce-pushed the fix/k8s_executor_ns_override branch from d358c56 to dcf9696CompareOctober 28, 2023 20:36
@potiuk

potiuk commented Oct 28, 2023

Copy link
Copy Markdown
Member

FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )

@potiuk

potiuk commented Oct 31, 2023

Copy link
Copy Markdown
Member

I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.

Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.

@potiuk

potiuk commented Oct 31, 2023

Copy link
Copy Markdown
Member

BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.

@hussein-awala
hussein-awala marked this pull request as draft November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed the fix/k8s_executor_ns_override branch 5 times, most recently from 489bd08 to 04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed the fix/k8s_executor_ns_override branch from 04c4e2f to a50da76CompareNovember 13, 2023 21:40
@hussein-awala

hussein-awala commented Nov 14, 2023

Copy link
Copy Markdown
MemberAuthor

K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.

@potiuk

potiuk commented Nov 14, 2023

Copy link
Copy Markdown
Member

Oh. What was it?

hussein-awala
hussein-awala commented Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py
Comment on lines +1053 to +1089
if multi_namespace_mode:
# duplicate Airflow configmaps, secrets and service accounts to test namespace
run_command_with_k8s_env(
f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
"--field-selector type!=helm.sh/release.v1 -o yaml "
f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
f"| kubectl apply -n {TEST_NAMESPACE} -f -",
python=python,
kubernetes_version=kubernetes_version,
output=output,
check=False,
shell=True,
)

run_command_with_k8s_env(
f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
"--field-selector metadata.name!=kube-root-ca.crt -o yaml "
f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
f"| kubectl apply -n {TEST_NAMESPACE} -f -",
python=python,
kubernetes_version=kubernetes_version,
output=output,
check=False,
shell=True,
)

run_command_with_k8s_env(
f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
"--field-selector metadata.name!=default -o yaml "
f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
f"| kubectl apply -n {TEST_NAMESPACE} -f -",
python=python,
kubernetes_version=kubernetes_version,
output=output,
check=False,
shell=True,
)

@hussein-awalahussein-awalaNov 23, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:

secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.

@hussein-awala
hussein-awala marked this pull request as ready for review November 23, 2023 19:24
potiuk
potiuk approved these changes Nov 23, 2023
View reviewed changes

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS

@potiuk

potiuk commented Nov 23, 2023

Copy link
Copy Markdown
Member

I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.

@hussein-awala

hussein-awala commented Nov 23, 2023

Copy link
Copy Markdown
MemberAuthor

I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.

I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.

@hussein-awala
hussein-awala merged commit 0e157b3 into apache:mainNov 23, 2023
@hussein-awala

hussein-awala commented Nov 23, 2023

Copy link
Copy Markdown
MemberAuthor

Finally 🎉

@eladkaleladkal mentioned this pull request Nov 24, 2023
Closed
69 tasks
ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham mentioned this pull request Apr 22, 2024
Merged
jedcunningham added a commit that referenced this pull request Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees

No one assigned

Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Fix K8S executor override config using pod_override_object - #35185
Merged
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Nov 23, 2023
Merged
Fix K8S executor override config using pod_override_object#35185
hussein-awala merged 13 commits into
apache:mainfrom 
hussein-awala:fix/k8s_executor_ns_override
Conversation
@hussein-awala
@hussein-awalahussein-awala
commented
Oct 25, 2023

Copy link


Copy Markdown

Member
related: #22298
Currently, we don't use all the configs from pod_override_object where we override some of them by the default K8S executor configurations or the kube config file (ex: default namespace).
This PR fixes the issue by changing the order of pod_override_object in the configs source list to use all the configurations provided in this object.
@boring-cyborgboring-cyborgBot
added

area:providers

provider:cncf-kubernetes
Kubernetes (k8s) provider related issues
labels
Oct 25, 2023
eladkal
eladkal
approved these changes
Oct 25, 2023
View reviewed changes
@eladkaleladkal
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix!
@eladkal
eladkal
commented
Oct 25, 2023

Copy link


Copy Markdown

Contributor
I am not sure why helm chart tests are skipped here cc @potiuk

In previous attempt to address the issue #34505 we had failing helm tests so I will add full tests labels just to make sure this change is OK
@eladkaleladkal
added
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 25, 2023
@eladkaleladkal
closed this
Oct 25, 2023
@eladkaleladkal
reopened this
Oct 25, 2023
@eladkal
eladkal
commented
Oct 26, 2023

Copy link


Copy Markdown

Contributor
Helm tests are failing :(
@hussein-awala
hussein-awala
commented
Oct 26, 2023

Copy link


Copy Markdown

MemberAuthor
I'm still trying to reproduce the tests failure locally:
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag PASSED [ 1%]
test_kubernetes_executor.py::TestKubernetesExecutor::test_integration_run_dag_with_scheduler_failure PASSED [ 3%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_do_xcom_push_defaults_false PASSED [ 5%]
test_kubernetes_pod_operator.py::TestKubernetesPodOperatorSystem::test_config_path_move PASSED 
@hussein-awalahussein-awala
mentioned this pull request
Oct 27, 2023
 Closed

@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
@potiuk, do you have an idea why the K8S executor tests failed? I tested locally with Mac arm and Linux amd, the tests finished successfully.
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
5ebc0c9 to
d98b7ceCompareOctober 27, 2023 21:38
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
This branch is 4 commits ahead, 18 commits behind apache:main.
I just rebased it. I think #35191 should fix it.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
But let's see.
@potiuk
potiuk
commented
Oct 27, 2023

Copy link


Copy Markdown

Member
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
@hussein-awala
hussein-awala
commented
Oct 27, 2023

Copy link


Copy Markdown

MemberAuthor
I guess changing timeout will not work here. This looks like a real problem introduced by the change - k8s executor stops working and it should be investigated. It's rather easy to reproduce all that CI does here locally and it's well described in https://github.com/apache/airflow/blob/main/TESTING.rst#typical-testing-pattern-for-kubernetes-tests
Following the steps describing there will setup precisely the same environment, kind cluster, will build airflow and deploy the image with airflow and test dags + test environment where the tests will interact with the cluster and (for some reason) fail.
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
I also triggered the dag manually via the UI deployed in the kind cluster, and all worked as expected.
@potiukpotiuk
removed
the 
full tests needed
We need to run full set of tests for this PR to merge
label
Oct 28, 2023
@potiuk
potiukforce-pushed
the
fix/k8s_executor_ns_override
branch
from
d358c56 to
dcf9696CompareOctober 28, 2023 20:36
@potiuk
potiuk
commented
Oct 28, 2023

Copy link


Copy Markdown

Member
FYI. I removed the "full tests needed" and rebased to check if there is indeed an issue that "K8S System tests" are not triggered for such PR - I think they should be - looking at selective checks output and the code, but let's see (cc: @eladkal )
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
I already reproduced all the steps on two different computers step by step, and I also tested run-complete-tests, in both cases, the tests finished successfully.
Oh I missed that. Maybe the logs will help ? If you look at summary https://github.com/apache/airflow/actions/runs/6679106455 and scroll down a bit - you will find that there are complete logs (dumps from kind logs) available as artifacts - they should contain more details on what's going on during the tests.
@potiuk
potiuk
commented
Oct 31, 2023

Copy link


Copy Markdown

Member
BTW. Those tests are pretty stable in general, so it's quite for sure some effect of those changes.
@hussein-awala
hussein-awala
marked this pull request as draft
November 12, 2023 12:12
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
5 times, most recently
from
489bd08 to
04c4e2fCompareNovember 13, 2023 19:54
@hussein-awala
hussein-awalaforce-pushed
the
fix/k8s_executor_ns_override
branch
from
04c4e2f to
a50da76CompareNovember 13, 2023 21:40
@hussein-awala
hussein-awala
commented
Nov 14, 2023

Copy link


Copy Markdown

MemberAuthor
K8S tests are green, I will move the helm chart changes to a separate PR and provide a small patch for these tests instead.
@potiuk
potiuk
commented
Nov 14, 2023

Copy link


Copy Markdown

Member
Oh. What was it?
hussein-awala
hussein-awala
commented
Nov 23, 2023
View reviewed changes
Comment threaddev/breeze/src/airflow_breeze/commands/kubernetes_commands.py

Comment on lines

+1053
 to 
+1089

 if multi_namespace_mode:
 # duplicate Airflow configmaps, secrets and service accounts to test namespace
 run_command_with_k8s_env(
 f"kubectl get secret -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector type!=helm.sh/release.v1 -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get configmap -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=kube-root-ca.crt -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )

 run_command_with_k8s_env(
 f"kubectl get serviceaccount -n {HELM_AIRFLOW_NAMESPACE} "
 "--field-selector metadata.name!=default -o yaml "
 f"| sed 's/namespace: {HELM_AIRFLOW_NAMESPACE}/namespace: {TEST_NAMESPACE}/' "
 f"| kubectl apply -n {TEST_NAMESPACE} -f -",
 python=python,
 kubernetes_version=kubernetes_version,
 output=output,
 check=False,
 shell=True,
 )
@hussein-awalahussein-awalaNov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These commands will duplicate the resources used in pod_template to test-namespace:
secret/airflow-broker-url created
secret/airflow-fernet-key created
secret/airflow-metadata created
secret/airflow-postgresql created
secret/airflow-redis-password created
secret/airflow-webserver-secret-key created
configmap/airflow-config created
configmap/airflow-statsd created
serviceaccount/airflow-create-user-job created
serviceaccount/airflow-migrate-database-job created
serviceaccount/airflow-scheduler created
serviceaccount/airflow-statsd created
serviceaccount/airflow-triggerer created
serviceaccount/airflow-webserver created
serviceaccount/airflow-worker created

I excluded the helm secrets/configmaps and the default service account from the duplication operation.
@hussein-awala
hussein-awala
marked this pull request as ready for review
November 23, 2023 19:24
potiuk
potiuk
approved these changes
Nov 23, 2023
View reviewed changes
@potiukpotiuk
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAAAJS
@potiuk
potiuk
commented
Nov 23, 2023

Copy link


Copy Markdown

Member
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
I guess later we can add multi-namespace mode to run in CI. We likely do not want to add another dimention for k8s tests but we can do what we do in databases with some kind of rotating scheme of test combos (k8s version, standard-naming, multi-namespace) - but it can be added later.
I agree, and it will be much simpler with #35639, I hope finding some time to finish it before the next chart release.
@hussein-awala
hussein-awala
merged commit 0e157b3
into
apache:mainNov 23, 2023
@hussein-awala
hussein-awala
commented
Nov 23, 2023

Copy link


Copy Markdown

MemberAuthor
Finally 🎉
@eladkaleladkal
mentioned this pull request
Nov 24, 2023

Closed

69 tasks
ephraimbuddy pushed a commit
that referenced
this pull request
Nov 26, 2023
@hussein-awala@ephraimbuddy
Fix K8S executor override config using pod_override_object (#35185)
f1e5109
* Fix K8S executor override config using pod_override_object
* Activate multi namespace mode for K8S tests
* Force multi namespace for k8s tests
* Increase timeout to test
* Increase pytest execution-timeout
* Support setuping multiple worker namespaces in the helm chart
* Rollback chart changes
* Revert timeout increase
* Duplicate Airflow resources to test namespace
* Fix the commands used to duplicate the resources
jedcunningham added a commit
to astronomer/airflow
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override
9d9edbb
This was changed in apache#35185 but this doc line was missed.
@jedcunninghamjedcunningham
mentioned this pull request
Apr 22, 2024
 Merged

jedcunningham added a commit
that referenced
this pull request
Apr 22, 2024
@jedcunningham
Fix docs on what KE pod_override can override (#39169)
bcbcb8e
This was changed in #35185 but this doc line was missed.
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@eladkaleladkaleladkal approved these changes
@jedcunninghamjedcunninghamAwaiting requested review from jedcunninghamjedcunningham is a code owner
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
@dstandishdstandishAwaiting requested review from dstandish

Assignees


No one assigned


Labels

area:providersprovider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@hussein-awala@eladkal@potiuk@jedcunningham