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

DeletionPolicy contradictory docs #133

Closed
nrb opened this issue Jun 28, 2019 · 9 comments
Closed

DeletionPolicy contradictory docs #133

nrb opened this issue Jun 28, 2019 · 9 comments
Assignees
@xing-yang
Labels
kind/documentation Categorizes issue or PR as related to documentation. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@nrb
Copy link
Contributor

nrb commented Jun 28, 2019

On VolumeSnapshotClass, the comments specify that the default behavior is Delete if a DeletionPolicy is not defined.

However, the VolumeSnapshotContentRetain constant says that the default behavior is Retain.

The controller sets the value to Delete.

I assume the comments on the VolumeSnapshotContentRetain constant should be removed; is that correct?
@xing-yang
Copy link
Collaborator

I'll take a look.

@xing-yang xing-yang self-assigned this Jul 1, 2019
@xing-yang
Copy link
Collaborator

The default policy for dynamic provisioning is Delete. That's why the controller sets it to Delete. The default policy for static provisioning is Retain. That's the reasoning for those comments on VolumeSnapshotContentRetain which is still correct (if not override by the policy set in storage class).

I can see that it is very confusing. I'll try to clarify the comments. Thanks.

@xing-yang xing-yang added the kind/documentation Categorizes issue or PR as related to documentation. label Jul 8, 2019
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 6, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 5, 2019
@xing-yang
Copy link
Collaborator

In the beta version of snapshot API, deletion policy is a required field in both VolumeSnapshotClass and VolumeSnapshotContent. Therefore we won't have this confusion any more.

@nrb
Copy link
Contributor Author

nrb commented Nov 8, 2019

Thanks!

Since it's required for VolumeSnapshotContent now, is it simply copied from the VolumeSnapshotClass each time a Content object is created?

@xing-yang
Copy link
Collaborator

Yes, controller will copy it from snapshot class in the case of dynamic provisioning. For pre-provisioned snapshots, admin has to set it when creating the content manually.

@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

@pohly pohly mentioned this issue Feb 3, 2021
Merged
pohly added a commit to pohly/external-snapshotter that referenced this issue Feb 3, 2021
@pohly
Squashed 'release-tools/' changes from 1d60e77..7bc70e5
7c126df 
7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation
e0b02e7 README.md: document usage of --squash
316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate
26e2ab1 fix: default boilerplate path
1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag
3e811d6 prow.sh: fix "on-master" prow jobs

git-subtree-dir: release-tools
git-subtree-split: 7bc70e5
ggriffiths pushed a commit to ggriffiths/external-snapshotter that referenced this issue Apr 15, 2021
Squashed 'release-tools/' changes from 3b6d17b..bc0504a
4683101 
bc0504a Merge pull request kubernetes-csi#140 from jsafrane/remove-unused-k8s-libs
5b1de1a go-get-kubernetes.sh: remove unused k8s libs
49b4269 Merge pull request kubernetes-csi#120 from pohly/add-kubernetes-release
a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest
1c0fb09 prow.sh: use KinD main for latest Kubernetes
1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10
bff2fb7 prow.sh: KinD 0.10.0
95eac33 Merge pull request kubernetes-csi#137 from pohly/fix-go-version-check
437e431 verify-go-version.sh: fix check after removal of travis.yml
1748b16 Merge pull request kubernetes-csi#136 from pohly/go-1.16
ec844ea remove travis.yml, Go 1.16
df76aba Merge pull request kubernetes-csi#134 from andyzhangx/add-build-arg
e314a56 add build-arg ARCH for building multi-arch images, e.g. ARG ARCH FROM k8s.gcr.io/build-image/debian-base-${ARCH}:v2.1.3
7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation
e0b02e7 README.md: document usage of --squash
316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate
26e2ab1 fix: default boilerplate path
1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag
3e811d6 prow.sh: fix "on-master" prow jobs
1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag
9f10459 prow.sh: support building Kubernetes for a specific version
f7e7ee4 docs: steps for adding testing against new Kubernetes release
fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check
8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master
1c94220 fix: fix a bug of csi-sanity
a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate
ece0f50 check namespace for snapshot-controller
dbd8967 verify-boilerplate.sh: fix path to script
9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks
ad29307 Make the spelling and boilerplate checks optional
5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests
48186eb Fix spelling and boilerplate errors
71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests
981be3f Adding spelling and boilerplate checks.
2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master
4ab8b15 use the tag to replace commit of csi-test
5d74e45 change the csi-test import path to v4
7dcd0a9 upgrade csi-test to v4.0.2

git-subtree-dir: release-tools
git-subtree-split: bc0504a
@ggriffiths ggriffiths mentioned this issue Apr 16, 2021
Merged
xing-yang pushed a commit to xing-yang/external-snapshotter that referenced this issue Jul 26, 2021
@k8s-ci-robot
Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag
1add8c1 
prow.sh: fix "on-master" prow jobs
dobsonj pushed a commit to dobsonj/external-snapshotter that referenced this issue Mar 8, 2024
@openshift-merge-bot
Merge pull request kubernetes-csi#133 from dobsonj/STOR-1688
35aafb7 
STOR-1688: Chore: add .snyk file to ignore false positives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@xing-yang xing-yang

Labels
kind/documentation Categorizes issue or PR as related to documentation. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nrb @xing-yang @k8s-ci-robot @fejta-bot