Make namespace optional for KPO #27116
Conversation
boring-cyborg
bot
added
provider:cncf-kubernetes
ferruzzi
left a comment
ferruzzi
left a comment
There was a problem hiding this comment.
Not a fan of using k as a variable name, but I know you did it to maintain consistency. Other than that, looks like a nice reasonable addition. 👍
dstandish
force-pushed
the
kpo-add-default-namespace
branch
from
102bea1 to
846646f
Compare
There was a problem hiding this comment.
here we remove the global hook patch because we rely on it for namespace test
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)
There was a problem hiding this comment.
you see this added in many places because we're not patching it globally
There was a problem hiding this comment.
when we're just building the pod object we don't need the full "run" method
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
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...
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
There was a problem hiding this comment.
Should we document this priority?
There was a problem hiding this comment.
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
4 participants
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.