Simplify KubernetesPodOperator

[dstandish]

Refactor of KPO with the following goals:

• simpler, easier-to-follow execute method
• remove reliance on mutation of pod and namespace instance attributes
  * change the default to delete pods
  * allow usage of kubernetes hook (waiting on Add config and context params to KubernetesHook #19695 before implementing)

Notes on changes

pod_launcher.py

changes to launcher

start_pod split up into two steps:

• create_pod asynchronously creates the pod
• await_pod_start waits for it to leave the Pending phase

monitor_pod split up into two components

• follow_container_logs: this bit of logic was previously in monitor_pod and invoked only if get_logs is true. it also monitored for completion of the base container in this case. here we pull that if self.get_logs decisionmaking up to the execute method for greater transparency, and we parameterize the follow_container_logs method so that it could be used for any container (and also make it more transparent which container it is following).
• monitor_pod remains and does two things: wait for pod to reach a phase that is terminal, and harvest xcom if applicable.

PodStatus -> PodPhase

I rename the PodStatus enums to have the same case the actual kubernetes phase values. I do not think that we need to lower them and doing so introduces a need for more status evaluation methods.

I also do away with the way pod phase was always translated to task states. It's more transparent to handle the phases directly. In the old code sometimes you would look at the "task state" -- which the pod phase had been translated to -- in order to infer the pod phase; better to be more direct. In any case these two changes allow us to chop a few methods.

misc

kubernetes_pod.py

simplified execute

The main goal here was simplifying the execute method so it's easier to follow. I tried to better reveal the key orchesration decisions that have to be made (e.g. do we get logs or not; do we get xcom or not; do we find a pod or create one), and provide a clearer separation of the processess that can be separated.

other

I moved the "get client" logic to a cached property.

I did away with mutation of the self.pod attribute, and none of the methods reference the attribute directly -- the object is always passed to them in the method. And I made a clear separation between the pod request object (which is used when a new pod is created) and pod -- the pod we're actually monitoring e.g. if we find a pod after failure and resume..

Similar for self.namespace which was mutated in the run without an obvious reason. The reason was that sometimes operator namespace is none but through templates or whatever you have a namespace and when you go to find a pod you need the namespace.

outdated

[dstandish]

@jedcunningham one thing i have not given a ton of thought is what should go in PodLauncher vs what should go in the operator. do you think that's something that i need to look into?

[jedcunningham]

Yeah, probably worth giving that some thought while we are at it.

[dstandish]

@jedcunningham one thing i have not given a ton of thought is what should go in PodLauncher vs what should go in the operator. do you think that's something that i need to look into?

Yeah, probably worth giving that some thought while we are at it.

yeah i took another look and gave it some more thought. i think it's ok.

i think PodLauncher is sort of an instance of a kind of "manager" design pattern which just helps to make the operator more about orchestration, more higher level, and can make it easier to write tests sometimes. it's like equivalent to a hook but specific to this operator.

we could shuffle things around but i'm not sure there is much value to be gained there.

let me know if you think otherwise.

[dstandish]

any reason not to go ahead and rename this to PodPhase while we're at it?

[BasPH]

+1 for PodPhase!

[dstandish]

Just to confirm, is this going to be major release @kaxil?

(and since we're renaming "public" methods i think it has to be, right?)

If so I think we also rename PodLauncher -> PodManager since it does more general pod management, not only launching. (could also do this in folllowup, lmk)

Do we need to also keep and deprecate old names PodStatus and PodLauncher?

[BasPH]

@dstandish I know at least 1 company that would be impacted by changing start_pod to create_pod.

PodLauncher doesn't cover the behaviour anymore so renaming that makes sense IMO.

[dstandish]

@dstandish I know at least 1 company that would be impacted by changing start_pod to create_pod.

PodLauncher doesn't cover the behaviour anymore so renaming that makes sense IMO.

Thanks @BasPH

So major it is.

Do we need to keep the old names (with deprecation)? Or is it fine to simply rename. E.g. pod_launcher.py will move to pod_manager.py

[dstandish]

I will save PodLauncher -> PodManager for a folllowup PR

[kaxil]

Yeah, major version is fine as this is a big refactor with breaking changes. As long as we have documented it, we are good

[potiuk]

Ah... .That means I will release it today :)

[dstandish]

no!

[dstandish]

there were fast folllows i intended before release

[dstandish]

chatted with @potiuk he agreed to postpone release of this provider so we can get in those followups, which for the record include the following:

rename PodLauncher -> PodManager
set "delete pods"  to true by default
allow usage of k8s hook
possibly, remove ability for PodManager to build client itself (currently can either receive client or be  given same params as  kpo and fetch its own client)