Simplify the K8sExecutor and K8sPodOperator - #10393
Conversation
turbaszek
left a comment
There was a problem hiding this comment.
-2.5k of lines - love it ❤️ This is still WIP but we should remember about note in UPDATING.md
Indeed. I love the stats on this one. |
|
Nice <3 -- some static checks are failing https://github.com/apache/airflow/pull/10393/checks?check_run_id=1003310026 |
0541262 to
0977f4b
Compare
ee7d498 to
1b6bce9
Compare
|
@davlum interesting, though without the reconcile methods how would users use the pod_override setting, which is an arbitrary pod? We need some way to ensure features are either added or overwritten based on these three stages. It's also worth mentioning that we now have a |
|
Pod override would do exactly that, it would completely override the base definition, not merge with it. |
|
@davlum I don't think we would want the pod_override to override all values. The examples I can think of off the top of my head are labels and volume_mounts. The base pod file will have volume mounts for the airflow.cfg and other potential secrets. I as a non-admin should be allowed to ADD volume mounts, but I shouldn't be able to delete them via override. Same with labels. Admin might have labels or node affinities they don't want messed with. While yes it's POSSIBLE to ensure those are there using the pod_mutation_hook, doing so places onus on the admin for the user's messups. |
1616218 to
88cb3cf
Compare
There was a problem hiding this comment.
Can you add a full list of configuration options that have been deleted? This is important if we are going to work on airflow upgrade-check, because having the list we can detect incorrect configuration and notify the user at runtime.
There was a problem hiding this comment.
Do we have this deprecation warning? I didn't see it in the code, but I might have missed it.
There was a problem hiding this comment.
Hi @mik-laj, the deprecation warning was already merged so it didn't show up in the PR https://github.com/apache/airflow/pull/10393/files#diff-868ed785b2a336f20cb6a577dde2502aR198-R201
There was a problem hiding this comment.
This sentence needs to be corrected. The plural is used when one class is described.
There was a problem hiding this comment.
Do we still support standard python dicts? (Line 60)
Also: why did this bit need to change -- should Volume or VolumeMount not be used anymore?
There was a problem hiding this comment.
Fixed. @ashb yes Volume and VolumeMount would now be k8s.V1Volume and k8s.V1VolumeMount, I go into more detail on this in the UPDATING.md
88cb3cf to
3a68783
Compare
Removes thousands of lines of code that essentially ammount to us re-creating the Kubernetes API. Will offer a faster, simpler KubernetesExecutor for 2.0
bb57bb7 to
8d0070a
Compare
|
@mik-laj can I get another lookover? :) |
| ) | ||
| namespaced['resources'] = resources | ||
| return PodGenerator(**namespaced).gen_pod() | ||
| return PodGeneratorDeprecated(**namespaced).gen_pod() |
There was a problem hiding this comment.
Can/should we issue a FutureDeprecationWarning here, so that we can remove this code path in 2.1/2.2?
There was a problem hiding this comment.
@ashb there is a future deprecation https://github.com/apache/airflow/pull/10393/files#diff-868ed785b2a336f20cb6a577dde2502aR198-R201
There was a problem hiding this comment.
Oh, you issue a warning a few lines up (from the previous PR. right?)
Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
As discussed in an earlier email thread, this PR removes much of the configuration elements of the K8sExecutor for a much simpler design.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.