-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove RefreshConfiguration workaround for K8s token refreshing #20759
Conversation
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
This is a bit of a risky change, and I wonder if we should wait until we have at least one release that is compatible with a wide range of Kubernetes Client versions ie. until a provider package is released that is compatible with a old and a new version of Kubernetes client versions. See: #18797 After we release a package that includes this change, we can release another package that will only be compatible with the new versions of the K8s client to avoid having to upgrade the kubernetes client and the provider package simultaneously. It especially makes sense that this change only includes code cleanup and we are under no pressure to merge it quickly. We can wait for users to test new versions of k8s clients and return to us with feedback. My fear is that there are some breaking changes between v3 and v21 of k8s client, so upgrading may not be easy. What do you think? Do we need to merge it quickly or can we wait for the compatibility with the new version of the k8s client to mature? |
I have no objections to delaying. Why I personally am invested in this change is, making this change will smooth the path to (1) making client creation consistent between k8s hook and internal k8s (e.g. used with k8s exec), (2) letting k8s pod operator use k8s hook, and (3) reducing dependencies between internal k8s and k8s provider given (2). But it's not a complete blocker to those efforts and we should do it in the right way, whatever that is.
Yeah it sounds reasonable. Once #18797 is out people will start getting the latest kubernetes library and we'll see what happens. I think we may have to wait until next airflow release to start getting feedback though... because i think people still won't be able to install kubernetes > 12 after only tomorrow's provider patch release goes out because the versions are controllled by airflow.... I could be wrong about that though. |
Agree. @mik-laj is right. And I think we can simply release it next month. The current `cncf.kubernetes' release is breaking already enough and indeed the change from #18797 will indeed allow people to upgrade to newer k8s client library first. And then - increasing the min version will be far less painful, as they will already be likely at that version (and can always choose to keep the "non-limiting" version if it blocks them. I will release the two providers (SFTP and cncf.kubernetes) now and we can merge this one ater (and after fixed tests :) so that we release it end Jan. Three weeks will make little diference in this case - I think it's important that we know how to proceed and make k8s integration better - whether it's now or in 3 months, it does not really matter in the grand scheme of things :D |
Yup totally sounds good 👍 |
49ec070
to
a3bfb17
Compare
ef0ea30
to
c07e714
Compare
So just want to document the plan here so it is not forgotten. Airflow 2.2.4 will be the first airflow release where the upper limit of kubernetes library is removed. This will allow users to use latest version of k8s library without being forced to. This will enable users to work on upgrading the library and infra if necessariy from the same version of airflow. Additionally this cause some users to inadvertently upgrade to the latest k8s version. Which will help to smoke out anything that might break as a result of using latest kubernetes library version. Following release of 2.2.4 then, we should be able to merge this PR ahead of the next airflow release, whether that is 2.2.5 or 2.3. And we'll have some time to incorporate fixes that may be prompted from user experience. |
So cool! |
c07e714
to
094ce6e
Compare
094ce6e
to
e860745
Compare
No problems whatsoever |
A workaround was added (apache#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
e860745
to
1aca591
Compare
…he#20759) A workaround was added (apache#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).
A workaround was added (#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170). (cherry picked from commit 7bd165f)
A workaround was added (#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170). (cherry picked from commit 7bd165f)
A workaround was added (#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170). (cherry picked from commit 7bd165f)
A workaround was added (#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170). (cherry picked from commit 7bd165f)
Note this requires bumping
kubernetes
version to 21.7.0 -- i don't know if we can freely do this.A workaround was added (#5731) to handle the refreshing of EKS tokens. It was necessary because of an upstream bug. It has since been fixed (kubernetes-client/python-base@70b78cd) and released in v21.7.0 (https://github.com/kubernetes-client/python/blob/master/CHANGELOG.md#v2170).