Bump CRD API version to v1 #3614

jenting · 2021-03-23T15:55:35Z

Please add a summary of your change

Generate both v1beta1 and v1 CRDs.
Add a new flag --crds-version to allow the user to specify which CRDs version to render. If the Kubernetes CRDs preferred API version is discoverable, use the discovered version.

Does your change fix a particular issue?

Please indicate you've done the following:

Accepted the DCO. Commits without the DCO will delay acceptance.
Created a changelog file or added /kind changelog-not-required.
Updated the corresponding documentation in site/content/docs/main.

ashish-amarnath · 2021-03-23T16:21:32Z

@jenting Considering Velero is used for upgrading from older Kubernetes versions to newer versions we will want to be able to Velero going back to kubernetes 1.12. For that reason, we might want to hold off on this PR.
I know that v1beta1 CRDs are deprecated and going to be removed soon, but I think we still have some time for this.

jenting · 2021-03-24T01:45:49Z

Make sense. Also, it seems like we need to update the Velero prerequisite on Kubernetes version, ref to

nrb · 2021-03-29T19:29:55Z

Kubernetes v1.22 is where the v1beta1 APIs will be completely removed.

I'd like to make progress on this, but I think we'll be looking at leveraging Carvel to do so.

We have #2505 to track the work.

.github/workflows/crds-verify-kind.yaml

sseago

More of a question at this point than a specific request for changes, but should we use v1 as the default rather than v1beta1?

Also, I'd like to have the flexibility to choose a CRD apiversion when running e2e tests. I actually made a local change to add that option in my git checkout so I could test running e2e tests with the new CRDs. It might be easier for me to just submit that as a follow-on PR rather than trying to get that added here. Without that, e2e tests just use the default CRDsVersion from NewInstallOptions()

sseago · 2021-07-09T20:11:14Z

pkg/cmd/cli/install/install.go

@@ -128,6 +133,7 @@ func NewInstallOptions() *InstallOptions {
 		UseVolumeSnapshots:      true,
 		NoDefaultBackupLocation: false,
 		CRDsOnly:                false,
+		CRDsVersion:             "v1beta1",


I wonder whether we should default to "v1" instead of "v1beta1" since it's the preferred version for recent kube versions, even before 1.22. This would require users on kube 1.15 and older to override this, rather than requiring users on current kubernetes to override it.

I agree with this. I think if we switch the default now, then we only need to give one notice to users: The default is v1, if you need a previous version, you can override it. Otherwise we would need to instruct Kubernetes 1.22+ users to override now, and then tell both user groups to either add or drop the override once the defaults are switched.

@zubron Does it make sense to ignore e2e tests in this PR (nothing here breaks them), and I'll submit a follow-on PR to add CRDS_VERSION as an option (so you can do CRDS_VERSION=v1beta1 make test-e2e) I already have the code for it in my local checkout.

sseago · 2021-07-09T21:17:47Z

@zubron @jenting I went ahead and created a separate PR with my e2e test changes: #3941

That PR has to wait until this one is merged, since it depends on the new InstallOptions CRDsVersion field.

jenting · 2021-07-10T00:36:16Z

Thank you @sseago for quickly go ahead, appreciate.

zubron

There are further changes needed in this PR to ensure that the uninstall behaviour works in 1.22. Currently our uninstall code uses the v1beta1 API to fetch the CRDs to delete: https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L127-L154

This will result in an error when running on Kubernetes 1.22:

$ v uninstall --kubecontext kind-1.22 
You are about to uninstall Velero.
Are you sure you want to continue (Y/N)? y
Velero namespace "velero" does not exist, skipping.
Velero ClusterRoleBinding "velero" does not exist, skipping.
Errors while attempting to uninstall Velero: "no matches for kind \"CustomResourceDefinition\" in version \"apiextensions.k8s.io/v1beta1\""An error occurred: no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1beta1"

I'm not sure of the best way to solve this. I don't know if there is a generic way to get the CRDs regardless of the API version used, or whether we need to attempt to delete both v1beta1 and v1 CRDs (checking for the appropriate errors from the API server).

zubron · 2021-07-27T19:27:44Z

pkg/cmd/cli/install/install.go

+			}
+
+			// Update the group apiextensions.k8s.io preferred API version
+			o.CRDsVersion = gvr.Version


With this, the API server preferred version will always take precedence over the setting provided by the user with the --crds-version flag. Is there a case where the user may want to explicitly use v1beta1 when the preferred version is v1?

Also, in the case where Velero can't find the server preferred version, we should output a warning or log to state that the preferred version couldn't be found and that instead we will use v1 as the default and that it can be changed with the --crds-version flag. Otherwise, I think we may encounter users who think that the CRD installation is broken on pre-1.16 clusters if the discovery fails.

zubron · 2021-07-27T21:51:33Z

@jenting After discussing with @dsu-igeek, we decided to go ahead and merge this branch as is and @dsu-igeek will work on fixing the uninstall issue. I am going to rebase this branch on top of main and push it to resolve the merge conflicts. Thank you so much for taking care of this issue! 😄

Signed-off-by: JenTing Hsiao <[email protected]>

If the Velero CLI can't discover the Kubernetes preferred CRD API version, use the flag --crds-version to determine the CRDs version. Signed-off-by: JenTing Hsiao <[email protected]>

Signed-off-by: JenTing Hsiao <[email protected]>

update the path of crds.go to ignore it. Signed-off-by: JenTing Hsiao <[email protected]>

Signed-off-by: JenTing Hsiao <[email protected]>

zubron

Thank you again, @jenting!

dsu-igeek

LGTM

jenting · 2021-07-28T01:55:46Z

There are further changes needed in this PR to ensure that the uninstall behaviour works in 1.22. Currently our uninstall code uses the v1beta1 API to fetch the CRDs to delete: https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L127-L154

This will result in an error when running on Kubernetes 1.22:
$ v uninstall --kubecontext kind-1.22 
You are about to uninstall Velero.
Are you sure you want to continue (Y/N)? y
Velero namespace "velero" does not exist, skipping.
Velero ClusterRoleBinding "velero" does not exist, skipping.
Errors while attempting to uninstall Velero: "no matches for kind \"CustomResourceDefinition\" in version \"apiextensions.k8s.io/v1beta1\""An error occurred: no matches for kind "CustomResourceDefinition" in version "apiextensions.k8s.io/v1beta1"
I'm not sure of the best way to solve this. I don't know if there is a generic way to get the CRDs regardless of the API version used, or whether we need to attempt to delete both v1beta1 and v1 CRDs (checking for the appropriate errors from the API server).

Sorry, I missed the velero uninstall command.
In general, if we delete CRDs by the native kubectl command, the kubectl gets the preferred CRDs API version. So, we don't have to have extra work on it.
But I believe we call Kubernetes client-go functions to delete the CRDs.

jenting marked this pull request as draft March 23, 2021 15:55

github-actions bot added the has-changelog label Mar 23, 2021

github-actions bot requested review from ashish-amarnath and nrb March 23, 2021 15:55

github-actions bot assigned jenting Mar 23, 2021

carlisia removed the request for review from nrb April 5, 2021 14:30

jenting mentioned this pull request Apr 9, 2021

Any plan to update API version to v1? vmware-tanzu/helm-charts#203

Closed

dsu-igeek requested a review from nrb April 15, 2021 21:53

nrb reviewed May 12, 2021

View reviewed changes

.github/workflows/crds-verify-kind.yaml Outdated Show resolved Hide resolved

github-actions bot added the has-e2e-tests label Jul 8, 2021

jenting removed the request for review from ashish-amarnath July 8, 2021 17:00

github-actions bot added Documentation and removed has-e2e-tests labels Jul 9, 2021

jenting marked this pull request as ready for review July 9, 2021 05:55

github-actions bot requested review from a-mccarthy and sseago July 9, 2021 05:55

jenting requested review from dsu-igeek and zubron and removed request for a-mccarthy July 9, 2021 05:57

sseago reviewed Jul 9, 2021

View reviewed changes

jenting requested a review from sseago July 13, 2021 02:37

sseago previously approved these changes Jul 13, 2021

View reviewed changes

zubron added this to the v1.6.3 milestone Jul 20, 2021

zubron suggested changes Jul 27, 2021

View reviewed changes

JenTing Hsiao added 9 commits July 27, 2021 17:55

Update script to generate both v1beta1 and v1 CRDs

11fcace

Signed-off-by: JenTing Hsiao <[email protected]>

Run script to generate both v1beta1 and v1 CRDs

616676e

Signed-off-by: JenTing Hsiao <[email protected]>

Add new flag --crds-version of default CRD version to generated

fca2b5c

If the Velero CLI can't discover the Kubernetes preferred CRD API version, use the flag --crds-version to determine the CRDs version. Signed-off-by: JenTing Hsiao <[email protected]>

Add changelog

ee920ea

Signed-off-by: JenTing Hsiao <[email protected]>

Update CI codespell check

e4dd555

update the path of crds.go to ignore it. Signed-off-by: JenTing Hsiao <[email protected]>

Add CI CRD check on K8s v1.21.1

26d6db1

Signed-off-by: JenTing Hsiao <[email protected]>

Update site documentation

a104254

Signed-off-by: JenTing Hsiao <[email protected]>

Update the Tiltfile to use v1 CRDs

527c5e8

Signed-off-by: JenTing Hsiao <[email protected]>

Change CRDs default API version v1

c3fd492

Signed-off-by: JenTing Hsiao <[email protected]>

zubron dismissed sseago’s stale review via c3fd492 July 27, 2021 21:55

zubron approved these changes Jul 27, 2021

View reviewed changes

dsu-igeek approved these changes Jul 27, 2021

View reviewed changes

zubron merged commit 57b72c0 into vmware-tanzu:main Jul 27, 2021

jenting deleted the bump-crd-api-version-to-v1 branch July 27, 2021 23:19

jenting mentioned this pull request Jul 28, 2021

enable e2e tests to choose crd apiVersion #3941

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump CRD API version to v1 #3614

Bump CRD API version to v1 #3614

jenting commented Mar 23, 2021 •

edited

Loading

ashish-amarnath commented Mar 23, 2021

jenting commented Mar 24, 2021

nrb commented Mar 29, 2021

sseago left a comment •

edited

Loading

sseago Jul 9, 2021

zubron Jul 9, 2021

sseago Jul 9, 2021 •

edited

Loading

sseago commented Jul 9, 2021

jenting commented Jul 10, 2021

zubron left a comment

zubron Jul 27, 2021

zubron commented Jul 27, 2021

zubron left a comment

dsu-igeek left a comment

jenting commented Jul 28, 2021

Bump CRD API version to v1 #3614

Bump CRD API version to v1 #3614

Conversation

jenting commented Mar 23, 2021 • edited Loading

Please add a summary of your change

Does your change fix a particular issue?

Please indicate you've done the following:

ashish-amarnath commented Mar 23, 2021

jenting commented Mar 24, 2021

nrb commented Mar 29, 2021

sseago left a comment • edited Loading

Choose a reason for hiding this comment

sseago Jul 9, 2021

Choose a reason for hiding this comment

zubron Jul 9, 2021

Choose a reason for hiding this comment

sseago Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

sseago commented Jul 9, 2021

jenting commented Jul 10, 2021

zubron left a comment

Choose a reason for hiding this comment

zubron Jul 27, 2021

Choose a reason for hiding this comment

zubron commented Jul 27, 2021

zubron left a comment

Choose a reason for hiding this comment

dsu-igeek left a comment

Choose a reason for hiding this comment

jenting commented Jul 28, 2021

jenting commented Mar 23, 2021 •

edited

Loading

sseago left a comment •

edited

Loading

sseago Jul 9, 2021 •

edited

Loading