-
Notifications
You must be signed in to change notification settings - Fork 380
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
cannot update snapshot metadata #149
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhucan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/assign @xing-yang |
klog.Infof("can not find VolumeSnapshotResource: %s, err: %#v", crd.Name, err) | ||
} | ||
} else { | ||
err = clientset.ApiextensionsV1beta1().CustomResourceDefinitions().Delete(crd.Name, &metav1.DeleteOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must not use delete crd method, when we delete the crd, all the CR will also be deleted. we should compare the crd spec and update the crd if the spec changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, if the spec not changes, no need to delete the crd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not install crd with the snapshotter and instead make it admin operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wackxu @xing-yang Thanks, it's a good suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msau42 What you mean is to put 'install crd' step in admin operation, for example, create from yaml instead of install crd from snapshotter?
@zhucan any update on this one? |
@Madhu-1 Maybe create CRD from yaml? I'm not sure. Maybe you can ask for xingyang @xing-yang |
We no longer installs CRDs directly from the controller when snapshot goes beta. So we don't need to fix it here any more. |
@zhucan: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@zhucan: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
I believe this is no longer needed @zhucan |
yes,I will close it. |
c0a4fb1 Merge pull request kubernetes-csi#164 from anubha-v-ardhan/patch-1 9c6a6c0 Master to main cleanup 682c686 Merge pull request kubernetes-csi#162 from pohly/pod-name-via-shell-command 36a29f5 Merge pull request kubernetes-csi#163 from pohly/remove-bazel 68e43ca prow.sh: remove Bazel build support c5f59c5 prow.sh: allow shell commands in CSI_PROW_SANITY_POD 71c810a Merge pull request kubernetes-csi#161 from pohly/mock-test-fixes 9e438f8 prow.sh: fix mock testing d7146c7 Merge pull request kubernetes-csi#160 from pohly/kind-update 4b6aa60 prow.sh: update to KinD v0.11.0 7cdc76f Merge pull request kubernetes-csi#159 from pohly/fix-deployment-selection ef8bd33 prow.sh: more flexible CSI_PROW_DEPLOYMENT, part II 204bc89 Merge pull request kubernetes-csi#158 from pohly/fix-deployment-selection 61538bb prow.sh: more flexible CSI_PROW_DEPLOYMENT 2b0e6db Merge pull request kubernetes-csi#157 from humblec/csi-release a2fcd6d Adding myself to csi reviewers group f325590 Merge pull request kubernetes-csi#149 from pohly/cluster-logs 4b03b30 Merge pull request kubernetes-csi#155 from pohly/owners a6453c8 owners: introduce aliases ad83def Merge pull request kubernetes-csi#153 from pohly/fix-image-builds 5561780 build.make: fix image publishng 29bd39b Merge pull request kubernetes-csi#152 from pohly/bump-csi-test bc42793 prow.sh: use csi-test v4.2.0 b546baa Merge pull request kubernetes-csi#150 from mauriciopoppe/windows-multiarch-args bfbb6f3 add parameter base_image and addon_image to BUILD_PARAMETERS 2d61d3b Merge pull request kubernetes-csi#151 from humblec/cm 48e71f0 Replace `which` command ( non standard) with `command -v` builtin feb20e2 prow.sh: collect cluster logs 7b96bea Merge pull request kubernetes-csi#148 from dobsonj/add-checkpathcmd-to-prow 2d2e03b prow.sh: enable -csi.checkpathcmd option in csi-sanity 09d4151 Merge pull request kubernetes-csi#147 from pohly/mock-testing 74cfbc9 prow.sh: support mock tests 4a3f110 prow.sh: remove obsolete test suppression 6616a6b Merge pull request kubernetes-csi#146 from pohly/kubernetes-1.21 510fb0f prow.sh: support Kubernetes 1.21 c63c61b prow.sh: add CSI_PROW_DEPLOYMENT_SUFFIX 51ac11c Merge pull request kubernetes-csi#144 from pohly/pull-jobs dd54c92 pull-test.sh: test importing csi-release-tools into other repo 7d2643a Merge pull request kubernetes-csi#143 from pohly/path-setup 6880b0c prow.sh: avoid creating paths unless really running tests git-subtree-dir: release-tools git-subtree-split: c0a4fb1
prow.sh: collect cluster logs
What type of PR is this?
/kind bug
What this PR does / why we need it:
When external-snapshotter image version upgraded, we should rebuild CRD, delete old CRD and use new CRD
Which issue(s) this PR fixes:
Fixes #147
Special notes for your reviewer:
@xing-yang
@msau42
Does this PR introduce a user-facing change?: