KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role #28047

XD-DENG · 2022-12-02T03:03:42Z

Currently KubernetesExecutor's multi_namespace_mode requires the Scheduler to have cluster-scope role on the Kubernetes Cluster, because it's using function list_pod_for_all_namespaces().

However, in certain enterprise environments, it's not possible for users to have cluster-scope role. For example, they may only get permissions in a namespace, rather on the whole cluster. Always allowing the Scheduler pod to have cluster-scope role is not a good from security aspect either.

This change aims to make KubernetesExecutor's multi_namespace_mode work without cluster-scope role.

(This was discussed at the mail list at https://lists.apache.org/thread/xxsppw7qwvky78l6nx41vlz593gj4zqb)

I'm sure folks would have suggestions and we need to future refine this change, but I would like to bring up the discussion by creating this PR first.

UPDATE:
Advantages this change brings:

Better fits enterprise environment
Better security: limit the permissions that the Scheduler Pod needs, so that it doesn't have too much permissions which it doesn't have to have (earlier it has to have a cluster role in order to use multi_namespace_mode)

XD-DENG · 2022-12-02T03:05:53Z

Hi @potiuk and @ferruzzi tagging you both, to follow up our earlier discussion in the mail list.

airflow/executors/kubernetes_executor.py

ferruzzi

Other than the comments already made, LGTM

airflow/config_templates/default_airflow.cfg

XD-DENG · 2022-12-03T00:54:31Z

Hi @dstandish , clarified/addressed your earlier comments. Pls help take another look when you get time? Thanks a lot!

airflow/executors/kubernetes_executor.py

airflow/kubernetes/kube_config.py

airflow/executors/kubernetes_executor.py

dstandish · 2022-12-08T04:13:01Z

i just noticed something coincidentally... you may want to look at file task handler. when reading logs from k8s task, seems to assume single namespace

XD-DENG · 2022-12-08T04:16:28Z

i just noticed something coincidentally... you may want to look at file task handler. when reading logs from k8s task, seems to assume single namespace

Thanks @dstandish , yes, I have already noticed that earlier. I plan to address that separately later though if by then other folks haven't taken it up. That part was totally missed when this multi_namespace_mode was introduced (again, questioning how people are using this feature so far)

dstandish · 2022-12-08T04:23:32Z

i just noticed something coincidentally... you may want to look at file task handler. when reading logs from k8s task, seems to assume single namespace

Thanks @dstandish , yes, I have already noticed that earlier. I plan to address that separately later though if by then other folks haven't taken it up. That part was totally missed when this multi_namespace_mode was introduced (again, questioning how people are using this feature so far)

sounds good

XD-DENG · 2022-12-08T04:28:03Z

Hi @dstandish , I tried to address your earlier comments (very nice inputs!), either changes have been made or I have clarified why I would prefer to do differently. Please let me know your thoughts.

(Sorry for getting back to you a bit slowly. Only have some time after work for this)

dstandish · 2022-12-08T04:40:44Z

there is nothing slow about it at all... lemme look

tests/executors/test_kubernetes_executor.py

airflow/executors/kubernetes_executor.py

dstandish

looks good to me. maybe worth getting second set of eyes e.g. from @jedcunningham or @uranusjr

uranusjr · 2022-12-08T06:54:20Z

airflow/executors/kubernetes_executor.py

-    resource_version = "0"
+    resource_version: dict[str, str] = {}


We can probably improve this class further, but that can be a separate PR.

uranusjr

I didn’t think everything entirely through but this should be good.

XD-DENG · 2022-12-08T16:02:39Z

Thanks both @dstandish @uranusjr ! The whole PR has improved a lot through the review.

May I also get some inputs from you about the failing test? The log isn't helping much.
Is there any known issue of the test pipeline, or just something missed/wrong here? Thanks a lot

potiuk · 2022-12-08T18:15:05Z

This looks like it's coming from one of your changes @XD-DENG - it's hard to say what precisely it is - because one of your tests is likely exhausting all 64 GB of memory that the builders have (that's what error 137 is) and it happens very consistently on all your tests - and it does not seem to be happening on others.

You can easily reproduce those builds locally with Breeze and see what happens https://github.com/apache/airflow/blob/main/TESTING.rst#running-tests-using-breeze-from-the-host - seems like "Core" test type is failing. The main reason for having breeze is that you should be able to reproduce it locally.

If not the memory exhaution you'd also get the instrunctions how to do it - but basically looks like some of the core tests fail and you could easily attempt to reproduce it the hash is in the CI logs:

This will get you to the shell of the image that was used in the last CI build here:

breeze shell --image-tag b64f96bc9e0cb67cd7b75baad3933638c23e4935

And you should be able to run the tests there.

potiuk · 2022-12-09T14:06:21Z

Yep @XD-DENG - exactly as I expected:

At some point in time those tests start eating memory at the rate of > 1GB / 10 seconds. Just before the crash we have just 800 MB left, Something spins out of control and causes that - this only happens on your change, not all the other PRs nor main - so it MUST be something here that triggers it. Now we just need to find out what it might be:

potiuk · 2022-12-09T14:10:00Z

My guess is - this is problem with cleanup. You have a LOT of parameterized tests and they are not cleaned up after each test and some of the left-overs (threads, processes etc. keep on running after each test).

XD-DENG · 2022-12-09T17:05:30Z

Thanks a lot @potiuk for helping check and sharing the Breeze/CI tips!
I will do another check later today.

XD-DENG · 2022-12-10T06:41:02Z

Thanks again @potiuk . The hint you pointed out was very useful. After adding cf8ce59, the CI is finally running successfully! (we just need to end the executor explicitly in the tests).

There are three items we would like to further check later:

File Task Handler cannot read log properly from K8S tasks when it's multi_namespace_mode (it's designed in a way that it only handled one K8S namespace). It's a separate issue from this PR though.
@uranusjr suggested to further refactor ResourceVersion class, but agreed it can be done later as a separate PR.
@dstandish suggested we can consider using threads to manage multiple watchers.

But for now I believe we are good to go ahead and merge this PR itself.

I would like to have @dstandish and @potiuk as co-authors for your significant contribution to this PR, if you have no objection :-)

potiuk · 2022-12-10T09:00:59Z

Actually I think we need a bit more protection, otherwise the same situation happens if for any reason those asserts will start to raise exceptions. try/finally and making sure that we always .end() after we .start() acrosss all the k8s tests is a much more robust solution.

Follow up here #28281 @XD-DENG

As a follow up after apache#28047, this PR will make the test cleanup more robust and resilient to any errors that might have caused kubernetes_executors left behind. wrapping start()/end() in try/finally will make the tests completely resilient to cases where the asserts start to fail - without those, any failure in tests would cause the same resource leakage as we initially had when #28407 was iterated on.

As a follow up after #28047, this PR will make the test cleanup more robust and resilient to any errors that might have caused kubernetes_executors left behind. wrapping start()/end() in try/finally will make the tests completely resilient to cases where the asserts start to fail - without those, any failure in tests would cause the same resource leakage as we initially had when #28407 was iterated on.

XD-DENG · 2022-12-15T06:27:33Z

Hi @dstandish , for the File Task Handler issue we discussed earlier, I'm preparing the fix and should be ready within this week. FYI

After apache#28047 the test_recover_from_resource_too_old started to fail in a flaky way. Turned out that - depend on some other test run the Singleton ResourceVersion could containt not one but two namespaces (including default namespace). Also while fixing the tests it's been noticed that the test missed an assert - it did not assert that the Exception was in fact thrown, so the test could have succeeded even if the exception was not really thrown (there was assert in "except" clause but if the exception was not thrown, it would not have been called at all).

After #28047 the test_recover_from_resource_too_old started to fail in a flaky way. Turned out that - depend on some other test run the Singleton ResourceVersion could containt not one but two namespaces (including default namespace). Also while fixing the tests it's been noticed that the test missed an assert - it did not assert that the Exception was in fact thrown, so the test could have succeeded even if the exception was not really thrown (there was assert in "except" clause but if the exception was not thrown, it would not have been called at all).

As a follow up after #28047, this PR will make the test cleanup more robust and resilient to any errors that might have caused kubernetes_executors left behind. wrapping start()/end() in try/finally will make the tests completely resilient to cases where the asserts start to fail - without those, any failure in tests would cause the same resource leakage as we initially had when #28407 was iterated on. (cherry picked from commit 3b203bc)

XD-DENG requested review from dstandish and jedcunningham as code owners December 2, 2022 03:03

boring-cyborg bot added provider:cncf-kubernetes Kubernetes provider related issues area:Scheduler including HA (high availability) scheduler labels Dec 2, 2022

XD-DENG requested review from ferruzzi and potiuk December 2, 2022 03:04

XD-DENG force-pushed the external_k8s-executor-for-enterprise-k8s-env branch 2 times, most recently from ae1a487 to ef75732 Compare December 2, 2022 05:22

dstandish reviewed Dec 2, 2022

View reviewed changes

airflow/executors/kubernetes_executor.py Outdated Show resolved Hide resolved

dstandish reviewed Dec 2, 2022

View reviewed changes

airflow/executors/kubernetes_executor.py Outdated Show resolved Hide resolved

ferruzzi approved these changes Dec 2, 2022

View reviewed changes

dstandish reviewed Dec 2, 2022

View reviewed changes

airflow/config_templates/default_airflow.cfg Outdated Show resolved Hide resolved

XD-DENG force-pushed the external_k8s-executor-for-enterprise-k8s-env branch 2 times, most recently from d75171b to 1c1f25d Compare December 3, 2022 00:52

dstandish reviewed Dec 3, 2022

View reviewed changes

dstandish changed the title ~~Ensure KubernetesExecutor's multi_namespace_mode work without cluster-scope role~~ KubernetesExecutor multi_namespace_mode can use namespace list Dec 3, 2022

dstandish reviewed Dec 8, 2022

View reviewed changes

tests/executors/test_kubernetes_executor.py Outdated Show resolved Hide resolved

dstandish reviewed Dec 8, 2022

View reviewed changes

airflow/executors/kubernetes_executor.py Outdated Show resolved Hide resolved

dstandish approved these changes Dec 8, 2022

View reviewed changes

uranusjr reviewed Dec 8, 2022

View reviewed changes

uranusjr approved these changes Dec 8, 2022

View reviewed changes

End the executor in the test

cf8ce59

XD-DENG merged commit c739a6a into apache:main Dec 10, 2022

XD-DENG changed the title ~~KubernetesExecutor multi_namespace_mode can use namespace list~~ KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role Dec 10, 2022

XD-DENG deleted the external_k8s-executor-for-enterprise-k8s-env branch December 10, 2022 06:51

XD-DENG added this to the Airflow 2.6.0 milestone Dec 10, 2022

potiuk mentioned this pull request Dec 10, 2022

More robust cleanup of executors in test_kubernetes_executor #28281

Merged

XD-DENG mentioned this pull request Dec 18, 2022

Log FileTaskHandler to work with multi_namespace_mode when running using KubernetesExecutor #28436

Merged

Taragolis mentioned this pull request Dec 19, 2022

Add deferrable mode to MLEngineStartTrainingJobOperator #27405

Merged

potiuk mentioned this pull request Dec 19, 2022

Fix flaky test_recover_from_resource_too_old exception #28475

Merged

ephraimbuddy added the type:new-feature Changelog: New Features label Apr 11, 2023

hussein-awala mentioned this pull request Aug 16, 2023

Scheduler fails to remove pods in Completed state #33402

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role #28047

KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role #28047

XD-DENG commented Dec 2, 2022 •

edited

Loading

XD-DENG commented Dec 2, 2022

ferruzzi left a comment

XD-DENG commented Dec 3, 2022

dstandish commented Dec 8, 2022 •

edited

Loading

XD-DENG commented Dec 8, 2022

dstandish commented Dec 8, 2022 •

edited

Loading

XD-DENG commented Dec 8, 2022

dstandish commented Dec 8, 2022

dstandish left a comment

uranusjr Dec 8, 2022

uranusjr left a comment

XD-DENG commented Dec 8, 2022

potiuk commented Dec 8, 2022

potiuk commented Dec 9, 2022 •

edited

Loading

potiuk commented Dec 9, 2022 •

edited

Loading

XD-DENG commented Dec 9, 2022

XD-DENG commented Dec 10, 2022

potiuk commented Dec 10, 2022

XD-DENG commented Dec 15, 2022

KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role #28047

KubernetesExecutor multi_namespace_mode can use namespace list to avoid requiring cluster role #28047

Conversation

XD-DENG commented Dec 2, 2022 • edited Loading

XD-DENG commented Dec 2, 2022

ferruzzi left a comment

Choose a reason for hiding this comment

XD-DENG commented Dec 3, 2022

dstandish commented Dec 8, 2022 • edited Loading

XD-DENG commented Dec 8, 2022

dstandish commented Dec 8, 2022 • edited Loading

XD-DENG commented Dec 8, 2022

dstandish commented Dec 8, 2022

dstandish left a comment

Choose a reason for hiding this comment

uranusjr Dec 8, 2022

Choose a reason for hiding this comment

uranusjr left a comment

Choose a reason for hiding this comment

XD-DENG commented Dec 8, 2022

potiuk commented Dec 8, 2022

potiuk commented Dec 9, 2022 • edited Loading

potiuk commented Dec 9, 2022 • edited Loading

XD-DENG commented Dec 9, 2022

XD-DENG commented Dec 10, 2022

potiuk commented Dec 10, 2022

XD-DENG commented Dec 15, 2022

XD-DENG commented Dec 2, 2022 •

edited

Loading

dstandish commented Dec 8, 2022 •

edited

Loading

dstandish commented Dec 8, 2022 •

edited

Loading

potiuk commented Dec 9, 2022 •

edited

Loading

potiuk commented Dec 9, 2022 •

edited

Loading