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

chore(argo-cd): Upgrade supported Kubernetes version to v1.23.0 #2087

Merged
merged 7 commits into from
Jun 5, 2023

Conversation

yu-croco
Copy link
Collaborator

@yu-croco yu-croco commented May 31, 2023

EKS v1.22 is over the EoL, so let's move on.

https://endoflife.date/amazon-eks

image

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@yu-croco yu-croco force-pushed the upgrade-supported-k8s branch 2 times, most recently from feee54e to ad70aad Compare May 31, 2023 14:01
… Amazon EKS EoL

Signed-off-by: yu-croco <yu.croco@gmail.com>
@yu-croco yu-croco force-pushed the upgrade-supported-k8s branch from ad70aad to 199c777 Compare May 31, 2023 14:01
@yu-croco yu-croco changed the title chore(argo-cd): Upgrade supported Kubernetes version to 1.23.0 due to Amazon EKS EoL chore(argo-cd): Upgrade supported Kubernetes version to v1.23.0 Jun 5, 2023
Signed-off-by: yu-croco <yu.croco@gmail.com>
@yu-croco yu-croco force-pushed the upgrade-supported-k8s branch from 0d41a4b to cd76289 Compare June 5, 2023 13:23
@yu-croco yu-croco marked this pull request as ready for review June 5, 2023 13:36
@@ -104,6 +104,12 @@ For full list of changes please check ArtifactHub [changelog].

Highlighted versions provide information about additional steps that should be performed by user when upgrading to newer version.

### 5.35.0
This version supports Kubernetes version from `>=1.22.0-0` to `>=1.23.0-0`. Though the supported version of Kubernetes is v1.24 or later, we align with Amazon EKS because there are a lot of EKS users.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a typo and should it be >=1.23.0-0 instead of both. Reads a bit confusing now 🤔

Copy link
Collaborator Author

@yu-croco yu-croco Jun 5, 2023

Choose a reason for hiding this comment

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

Thank you for reviewing. Ah... now I realize it's so confusing..

Below would be simpler...? 🙋

- This version supports Kubernetes version from `>=1.22.0-0` to `>=1.23.0-0`.
+ This version supports Kubernetes version `>=1.23.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.

Apologies for missing this on the review. My brain filled in the words instead of reading them :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed at 2b2e351 🙋

Signed-off-by: yu-croco <yu.croco@gmail.com>
@@ -105,6 +105,11 @@ For full list of changes please check ArtifactHub [changelog].

Highlighted versions provide information about additional steps that should be performed by user when upgrading to newer version.

### 5.35.0
This version supports Kubernetes version `>=1.23.0-0`. Though the supported version of Kubernetes is v1.24 or later, we align with Amazon EKS because there are a lot of EKS users.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about something like this?

Suggested change
This version supports Kubernetes version `>=1.23.0-0`. Though the supported version of Kubernetes is v1.24 or later, we align with Amazon EKS because there are a lot of EKS users.
This version supports Kubernetes version `>=1.23.0-0`. The current supported version of Kubernetes is v1.24 or later and we align with Amazon EKS calendar, because many of AWS users and conservative approach.

@@ -104,6 +104,12 @@ For full list of changes please check ArtifactHub [changelog].

Highlighted versions provide information about additional steps that should be performed by user when upgrading to newer version.

### 5.35.0
This version supports Kubernetes version `>=1.23.0-0`. Though the supported version of Kubernetes is v1.24 or later, we align with Amazon EKS because there are a lot of EKS users.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about something like this?

Suggested change
This version supports Kubernetes version `>=1.23.0-0`. Though the supported version of Kubernetes is v1.24 or later, we align with Amazon EKS because there are a lot of EKS users.
This version supports Kubernetes version `>=1.23.0-0`. The current supported version of Kubernetes is v1.24 or later and we align with Amazon EKS calendar, because many of AWS users and conservative approach.

@mbevc1
Copy link
Collaborator

mbevc1 commented Jun 5, 2023

Thanks, looks better. I've left a potential improvement. But it's not a blocker.

@yu-croco
Copy link
Collaborator Author

yu-croco commented Jun 5, 2023

@mbevc1
Thank you !
I fixed at 724e891 . 😄

@mbevc1
Copy link
Collaborator

mbevc1 commented Jun 5, 2023

Thanks 👍

@yu-croco yu-croco merged commit a6a7468 into argoproj:main Jun 5, 2023
@agaudreault
Copy link
Member

agaudreault commented Jun 30, 2023

This is kind of annoying to have that configuration in the chart. IMO, the kubeVersion defined in this chart should not be coupled to EKS release cycle. Not all users of this chart use EKS. The value should represent the minimum value of the cluster on which it can be installed on, like specified in the Helm doc. If it works on 1.22 and all resources are supported, why make it unusable?

Because of this change, users deploying argo on older versions of Kubernetes will need to fork the chart to have fixes and improvements.

@mkilchhofer
Copy link
Member

Do you know the support matrix of Argo CD?
While the newest version mostly works also with very old kubernetes versions, it is simply not tested by the upstream project.
The upstream project supports only the 3 recent minor versions of kubernetes.

Refs:

@yu-croco
Copy link
Collaborator Author

yu-croco commented Jul 1, 2023

In addition to @mkilchhofer 's opinion, the kubeVersion itself covers more than Kubernetes's EoL. So I think this change was conservative enough.
https://endoflife.date/kubernetes
image

Also other cloud services' K8s are also covered.

@agaudreault
Copy link
Member

agaudreault commented Jul 1, 2023

The upstream project supports only the 3 recent minor versions of kubernetes.

While the container image may or may not work with some versions of Kubernetes, this is not the same as the compatibility of a Helm chart. Both do not need to be coupled. The chart can be used with an older version of ArgoCD by overriding the tag.

The kubeVersion field in a chart is a hard limit and should only be set if the chart resources aren't compatible with older Kubernetes versions or container image. AFAIK this chart works fine with older Kubernetes versions, assuming that the image tag is changed to use a suitable ArgoCD image version.

My suggestion would be to remove kubeVersion and update the README with a compatibility table instead of setting a hard limit.

The only limitation I can see is that 5.31.0 must be used with ArgoCD >= 2.7.0. And while ArgoCD is tested up to the N-1 release of k8s, it does not mean that it does not work on earlier version than that. So this chart should not hinder the users to install it on older clusters.

Another advantage to have a compatibility matrix is the ability to update it when problems are discovered after the release. Usage of kubeVersion cannot be reverted unless overriding the chart version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants