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

Upgrade api client to networking v1 API #299

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

VMois
Copy link

@VMois VMois commented Sep 23, 2021

@VMois VMois marked this pull request as draft September 24, 2021 12:17
@VMois VMois changed the title (WIP) upgrades api_client to networking API v1 upgrades api_client to networking API v1 Sep 24, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #299 (8f2a695) into master (3525623) will not change coverage.
The diff coverage is 16.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #299   +/-   ##
=======================================
  Coverage   36.60%   36.60%           
=======================================
  Files          20       20           
  Lines         959      959           
=======================================
  Hits          351      351           
  Misses        608      608           
Impacted Files Coverage Δ
reana_commons/k8s/api_client.py 45.45% <16.66%> (ø)

@VMois VMois force-pushed the k8s_networking_api_upgrade branch 2 times, most recently from f50c3b9 to 862fd69 Compare November 30, 2021 13:10
@VMois VMois changed the title upgrades api_client to networking API v1 Upgrade api client to networking v1 API Nov 30, 2021
@@ -17,7 +17,7 @@

# FIXME: monkeypatch to avoid `Invalid value for `names`, must not be `None``
# error when calling `current_k8s_corev1_api_client.list_node()`.
# After new Kubernetes release this should not be needed.
# After new Kubernetes release (1.22+) this should not be needed.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for now, we still support 1.16+ clusters even if 1.22+ is possible so I kept this comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reana's repository says 1.19+ only, so I guess we could remove this. (Or better change reana's lower bound to 1.16+ if that still works well.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to kubernetes-client/python#895 the issue is still present on the 1.19 cluster. But, I have checked v22 of the k8s python client and it looks like they fixed the problem. Our mock corresponds to exactly the same code as in the library (details)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v21 still needs mock, so I set version k8s python client version to be 22.x.

@VMois VMois marked this pull request as ready for review November 30, 2021 14:08
@VMois VMois force-pushed the k8s_networking_api_upgrade branch 2 times, most recently from ec6f7de to bf74347 Compare February 1, 2022 09:45
@@ -17,7 +17,7 @@

# FIXME: monkeypatch to avoid `Invalid value for `names`, must not be `None``
# error when calling `current_k8s_corev1_api_client.list_node()`.
# After new Kubernetes release this should not be needed.
# After new Kubernetes release (1.22+) this should not be needed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reana's repository says 1.19+ only, so I guess we could remove this. (Or better change reana's lower bound to 1.16+ if that still works well.)

elif api == "networking.k8s.io/v1beta1":
api_client = client.NetworkingV1beta1Api()
elif api == "networking.k8s.io/v1":
api_client = client.NetworkingV1Api()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write a line in CHANGES.rst about this. We shall target 0.9.0a1 release.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is the procedure for version change. I put 0.9.0a1 in version.py

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's good. You can also use reana-dev git-create-release-commit -c . to create an explicit release commit.

BTW this will need to amend cluster components's setup.py requirements to bump minimal requirements. So a set of PR to everything on the client and cluster side will be necessary. I can pair up on this in the afternoon.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's pair up. Not sure how to do release and reana-dev git-create-release-commit -c . command is not working (need to tag commit).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a problem with the release-commit command. Add a release commit.

setup.py Outdated
@@ -31,7 +31,7 @@ def get_snakemake_pkg(extras=""):
extras_require = {
"docs": ["Sphinx>=1.4.4", "sphinx-rtd-theme>=0.1.9",],
"tests": tests_require,
"kubernetes": ["kubernetes>=11.0.0,<12.0.0",],
"kubernetes": ["kubernetes>=21.0.0,<22.0.0",],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the kubernetes library now has version 22.6 released on PyPI. Is it interesting to release upper boundary constraint?

Copy link
Author

@VMois VMois Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can. Should I release it completely or just increase the upper boundary to 23.0.0? I would prefer to put an upper boundary to <23.0.0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put version >=22,<23, for now, because it fixes the mock issue. Should we remove the upper boundary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please keep the upper boundary; It'll be safer.

@VMois VMois force-pushed the k8s_networking_api_upgrade branch 2 times, most recently from 0dbbccd to 760d6e7 Compare February 22, 2022 10:23
- set Kubernetes Python client version to 22.x;
- remove mock on V1ContainerImage as it was fixed starting Kubernetes Python client version 22+;

closes reanahub/reana#482
@@ -19,7 +19,7 @@
history = open("CHANGES.rst").read()

tests_require = [
"pytest-reana>=0.8.1,<0.9.0",
"pytest-reana>=0.9.0a1,<0.10.0",
Copy link
Member

@tiborsimko tiborsimko Feb 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: depends on reanahub/pytest-reana#95

@tiborsimko tiborsimko force-pushed the k8s_networking_api_upgrade branch 4 times, most recently from 3f547dc to 2b7fbb5 Compare February 25, 2022 14:50
@tiborsimko tiborsimko merged commit 5734e68 into reanahub:master Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm: upgrade to networking.k8s.io/v1 ingress
3 participants