Make namespace optional for KPO #27116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
boring-cyborg
bot
added
provider:cncf-kubernetes
ferruzzi
approved these changes
Oct 18, 2022
dstandish
force-pushed
the
kpo-add-default-namespace
branch
from
102bea1
to
846646f
Compare
dstandish
commented
Oct 19, 2022
| self.create_pod_patch = mock.patch(f"{POD_MANAGER_CLASS}.create_pod") | ||
| self.await_pod_patch = mock.patch(f"{POD_MANAGER_CLASS}.await_pod_start") | ||
| self.await_pod_completion_patch = mock.patch(f"{POD_MANAGER_CLASS}.await_pod_completion") | ||
| self.hook_patch = mock.patch(HOOK_CLASS) |
There was a problem hiding this comment.
here we remove the global hook patch because we rely on it for namespace test
dstandish
commented
Oct 19, 2022
Comment on lines
-105
to
-107
| if not isinstance(self.hook_mock.return_value.is_in_cluster, bool): | ||
| self.hook_mock.return_value.is_in_cluster = True |
There was a problem hiding this comment.
instead of doing this, i just read the value from the actual hook (since we're no longer patching it globally)
dstandish
commented
Oct 19, 2022
| operator.execute(context=context) | ||
| return self.await_start_mock.call_args[1]['pod'] | ||
|
|
||
| def sanitize_for_serialization(self, obj): | ||
| return ApiClient().sanitize_for_serialization(obj) | ||
|
|
||
| def test_config_path(self): | ||
| @patch(HOOK_CLASS) |
There was a problem hiding this comment.
you see this added in many places because we're not patching it globally
dstandish
commented
Oct 19, 2022
| @@ -169,7 +172,7 @@ def test_security_context(self): | |||
| in_cluster=False, | |||
| do_xcom_push=False, | |||
| ) | |||
| pod = self.run_pod(k) | |||
| pod = k.build_pod_request_obj(create_context(k)) | |||
There was a problem hiding this comment.
when we're just building the pod object we don't need the full "run" method
dstandish
commented
Oct 19, 2022
| @@ -574,6 +581,11 @@ def build_pod_request_obj(self, context: Context | None = None) -> k8s.V1Pod: | |||
| if self.random_name_suffix: | |||
| pod.metadata.name = PodGenerator.make_unique_pod_id(pod.metadata.name) | |||
|
|
|||
| if not pod.metadata.namespace: | |||
| hook_namespace = self.hook.conn_extras.get('extra__kubernetes__namespace') | |||
There was a problem hiding this comment.
there is a get_namespace method on the hook but it's problematic. i may resolve that in future PR and then can update this. i'll resolve that and update this in a followup PR
ferruzzi
reviewed
Oct 20, 2022
| @@ -123,20 +123,18 @@ def test_config_path(self, hook_mock): | |||
| labels={"foo": "bar"}, | |||
| name="test", | |||
| task_id="task", | |||
There was a problem hiding this comment.
Removed so much boilerplate. 😍
There was a problem hiding this comment.
now if only i can get the tests to pass...
ferruzzi
reviewed
Oct 20, 2022
dstandish
force-pushed
the
kpo-add-default-namespace
branch
from
53a1f5a
to
230a255
Compare
dstandish
changed the title
dstandish
changed the title
dstandish
changed the title
kaxil
reviewed
Oct 21, 2022
| @@ -35,7 +35,9 @@ Previously KubernetesPodOperator considered some settings from the Airflow confi | |||
| Features | |||
| ~~~~~~~~ | |||
|
|
|||
| Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used. | |||
| * Previously, ``name`` was a required argument for KubernetesPodOperator (when also not supplying pod template or full pod spec). Now, if ``name`` is not supplied, ``task_id`` will be used. | |||
| * KubernetsPodOperator argument ``namespace`` is now optional. If not supplied, we'll check the airflow conn, | |||
There was a problem hiding this comment.
Should we document this priority?
There was a problem hiding this comment.
eladkal
approved these changes
Oct 21, 2022
dstandish
force-pushed
the
kpo-add-default-namespace
branch
from
192b65e
to
dd22bea
Compare
In KPO namespace is currently required, either through pod template file, airflow connection, or KPO arg. If we're in the in_cluster scenario though, we should be able to derive the current namespace from /var/run/secrets/kubernetes.io/serviceaccount/namespace. This seems like a reasonable thing to do as a default.
dstandish
force-pushed
the
kpo-add-default-namespace
branch
from
1395e13
to
d0bb221
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In KPO, currently, if you do not provide namespace as a KPO arg (and it's not otherwise specified through pod template or full pod spec) the task will fail when trying to create the pod, because the kube client does not fill it in for you like e.g. kubectl does.
This PR makes it optional.
If it's not specified through KPO arg, or full pod spec, or pod template, then first we'll check the hook to see if you've configured a namespace there. And if that is unspecified, if we're in a cluster already, we'll check /var/run/secrets/kubernetes.io/serviceaccount/namespace for the value.