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.

Sign up for GitHub

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

Jump to bottom

feat: migrate velero controller from kubebuilder v2 to v3 #4382

Merged
merged 1 commit into from
Nov 24, 2021
Merged

feat: migrate velero controller from kubebuilder v2 to v3 #4382

merged 1 commit into from
Nov 24, 2021

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented Nov 22, 2021 •
edited
Loading

  1. remove config/crd/v1beta1
  2. remove PROJECT file
  3. update controller-gen and kubebuilder version
  4. generate client and CRD file
  5. add changelog and remove v1beta1 CRD generated code.
  6. add kubebuilder test bundle setup command.
  7. due to apiextensions.k8s.io/v1beta1 is not supported, only k8s after v1.16 is supported, so remove v1.15 check.
  8. add CRD and k8s suppored version update in changelog.

Signed-off-by: Xun Jiang jxun@vmware.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

@blackpiglet blackpiglet self-assigned this Nov 22, 2021
@blackpiglet blackpiglet removed the request for review from carlisia November 22, 2021 05:19
reasonerjt
reasonerjt reviewed Nov 22, 2021
View reviewed changes
pkg/install/resources.go
@@ -226,11 +225,6 @@ func AllCRDs(perferredAPIVersion string) *unstructured.UnstructuredList {
resources.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "List"})

switch perferredAPIVersion {
case "v1beta1":
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my understanding after moving to kubebuilder v3, all CRDs of velero will be v1, so the preferredAPIVersion is unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In release 1.8, there is only one possible version, v1, but preferredAPIVersion is passed from velero client command line parameter "--crds-version".
I think it may be useful for future potential multiple versions need.
In my opinion, keeping the logic there and adding validation to only support v1 may be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check with @zubron
By checking the commit:
fca2b5c

It seems this was added as a temp workaround.

If the conclusion is to remove this param I'm fine to handle that in another PR/issue so that this big PR is not blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this flag was only in place to allow users to explicitly set which version to use if needed. Given that we always use v1 now, you can remove this flag.

There is also some code in the uninstall path to remove v1beta1 CRDS (https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L139-L150) but I think we may want to keep that for now to handle any v1beta1 CRDs that a user may have installed. We can probably remove that in 1.9.

Copy link
Contributor Author

@blackpiglet blackpiglet Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I think the agreement is to remove the CRD version selection parameter in install command, and keep the v1beta1 check in uninstall command until 1.9 release.
If then, I will open another PR to the remove the CRD version selection parameter.

@blackpiglet blackpiglet marked this pull request as ready for review November 22, 2021 11:22
zubron
zubron reviewed Nov 23, 2021
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @blackpiglet! I only have one comment on the changelog but everything else looks good 👍

changelogs/unreleased/4382-jxun Outdated
@@ -0,0 +1 @@
feat: migrate velero controller from kubebuilder v2 to v3
Copy link
Contributor

Choose a reason for hiding this comment

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

This changelog should include details to state that v1beta1 CRDs are no longer supported and that we only support Kubernetes 1.16 and later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks.

pkg/install/resources.go
@@ -226,11 +225,6 @@ func AllCRDs(perferredAPIVersion string) *unstructured.UnstructuredList {
resources.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "List"})

switch perferredAPIVersion {
case "v1beta1":
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this flag was only in place to allow users to explicitly set which version to use if needed. Given that we always use v1 now, you can remove this flag.

There is also some code in the uninstall path to remove v1beta1 CRDS (https://github.com/vmware-tanzu/velero/blob/main/pkg/cmd/cli/uninstall/uninstall.go#L139-L150) but I think we may want to keep that for now to handle any v1beta1 CRDs that a user may have installed. We can probably remove that in 1.9.

@blackpiglet blackpiglet mentioned this pull request Nov 23, 2021
Closed
@blackpiglet
Copy link
Contributor Author

#4389 is opened to trace --crds-version removing.

@reasonerjt
Copy link
Contributor

Could you squash to commits?

feat: migrate kubebuilder from v2 to v3
303d3dc 
1. remove config/crd/v1beta1
2. remove PROJECT file
3. update controller-gen and kubebuilder version
4. generate client and CRD file
5. add changelog and remove v1beta1 CRD generated code.
6. add kubebuilder test bundle setup command.
7. due to apiextensions.k8s.io/v1beta1 is not supported, only k8s after v1.16 is supported, so remove v1.15 check.
8. add CRD and k8s suppored version update in changelog.

Signed-off-by: Xun Jiang <jxun@vmware.com>
@blackpiglet blackpiglet force-pushed the 4369-bsl-from-kubebuilder-v2-to-v3 branch from 8485daf to 303d3dc Compare November 23, 2021 11:34
@blackpiglet
Copy link
Contributor Author

Sure. Commits are squashed.

zubron
zubron approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@reasonerjt reasonerjt merged commit 02013ef into vmware-tanzu:main Nov 24, 2021
@blackpiglet blackpiglet mentioned this pull request Nov 24, 2021
Closed
3 tasks
@jenting jenting self-requested a review November 25, 2021 02:30
@blackpiglet
Copy link
Contributor Author

Fix issue #4369, #4370 and #4371

This was referenced Nov 29, 2021
Closed
Closed
Closed
@blackpiglet blackpiglet deleted the 4369-bsl-from-kubebuilder-v2-to-v3 branch October 15, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reasonerjt reasonerjt reasonerjt approved these changes

@zubron zubron zubron approved these changes

@dsu-igeek dsu-igeek Awaiting requested review from dsu-igeek

@ywk253100 ywk253100 Awaiting requested review from ywk253100

@jenting jenting Awaiting requested review from jenting

Assignees

@blackpiglet blackpiglet

Labels
has-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blackpiglet @reasonerjt @zubron