Skip to content
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

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

dstandish
Copy link
Contributor

@dstandish dstandish commented Jan 7, 2022

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).

@boring-cyborg boring-cyborg bot added the provider:cncf-kubernetes Kubernetes provider related issues label Jan 7, 2022
@dstandish dstandish marked this pull request as draft January 7, 2022 22:19
@dstandish dstandish marked this pull request as ready for review January 7, 2022 22:20
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jan 7, 2022
@github-actions
Copy link

github-actions bot commented Jan 7, 2022

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.

@mik-laj
Copy link
Member

mik-laj commented Jan 8, 2022

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?

setup.py Show resolved Hide resolved
@dstandish
Copy link
Contributor Author

dstandish commented Jan 8, 2022

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.

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

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.

@potiuk
Copy link
Member

potiuk commented Jan 8, 2022

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

@dstandish
Copy link
Contributor Author

Yup totally sounds good 👍

@dstandish dstandish force-pushed the remove-refresh-config-hack branch 2 times, most recently from ef0ea30 to c07e714 Compare January 28, 2022 20:42
@dstandish
Copy link
Contributor Author

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.

@potiuk
Copy link
Member

potiuk commented Feb 4, 2022

So cool!

@dstandish
Copy link
Contributor Author

ok @potiuk @mik-laj any reason not to merge this now?

@potiuk
Copy link
Member

potiuk commented Mar 9, 2022

No problems whatsoever

@dstandish dstandish merged commit 7bd165f into apache:main Mar 16, 2022
@dstandish dstandish deleted the remove-refresh-config-hack branch March 16, 2022 19:33
shuhoy pushed a commit to shuhoy/airflow that referenced this pull request Mar 17, 2022
…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).
@potiuk potiuk added this to the Airflow 2.2.5 milestone Mar 26, 2022
potiuk pushed a commit that referenced this pull request Mar 26, 2022
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)
potiuk pushed a commit that referenced this pull request Mar 26, 2022
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)
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Mar 26, 2022
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
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)
ephraimbuddy pushed a commit that referenced this pull request Mar 26, 2022
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)
potiuk added a commit that referenced this pull request Mar 28, 2022
@ephraimbuddy ephraimbuddy removed this from the Airflow 2.2.5 milestone Mar 28, 2022
ephraimbuddy pushed a commit that referenced this pull request Mar 28, 2022
ephraimbuddy pushed a commit that referenced this pull request Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge provider:cncf-kubernetes Kubernetes provider related issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants