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

Add doc for DeletionPolicy and Finalizers #14719

Closed
wants to merge 2 commits into from

Conversation

xing-yang
Copy link
Contributor

This PR adds doc for snapshot DeletionPolicy
and Finalizers added in Kubernetes v1.13.

Fixes: kubernetes-csi/external-snapshotter#124

This PR adds doc for snapshot DeletionPolicy
and Finalizers added in Kubernetes v1.13.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jun 4, 2019
@k8s-ci-robot k8s-ci-robot requested review from saad-ali and thockin June 4, 2019 14:45
@netlify
Copy link

netlify bot commented Jun 4, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 1c560a1

https://deploy-preview-14719--kubernetes-io-master-staging.netlify.com

`VolumeSnapshotClass` object is created, it will default to `Delete`.

Volume snapshots that are created manually and managed via a volume snapshot class will have
whatever deletion policy they were assigned at creation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if user does not specify the policy for manually created volume snapshot and content without a snapshot class will have default value of retain? but need to double check.

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind, I see your explanation below

@jingxu97
Copy link
Contributor

jingxu97 commented Jun 4, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2019
The following example demonstrates how to check the deletion policy of a dynamically provisioned `VolumeSnapshotContent`.

```
$ kubectl create -f ./examples/kubernetes/demo-defaultsnapshotclass.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the yaml file?

Copy link
Contributor

Choose a reason for hiding this comment

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

To follow the style guide, don't include the command prompt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tengqm Good catch. I need to add the example yaml file here. Should I create a new folder called "storage" under website/content/en/examples and put the yaml file there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


With PV/PVC pairs, when a user is done with a volume, they can delete the PVC. And the reclaim policy on the PV determines what happens to the PV (whether it is also deleted or retained).

In the initial alpha release, snapshots did not support the ability to specify a reclaim policy. Instead when a snapshot object was deleted it always resulted in the snapshot being deleted. In Kubernetes v1.13, a snapshot content `DeletionPolicy` was added. It enables an admin to configure what happens to a `VolumeSnapshotContent` after the `VolumeSnapshot` object it is bound to is deleted. The `DeletionPolicy` of a volume snapshot can either be `Retain` or `Delete`. If the value is not specified, the default depends on whether the `VolumeSnapshotContent` object was created via static binding or dynamic provisioning.
Copy link
Contributor

Choose a reason for hiding this comment

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

Break the lines at a reasonable column would make the review a bit easier.

"snapshots did not support the ability to specify a reclaim policy." This sounds a wordy sentence though I'm not a native speaker. You could simply say "snapshots did not support reclaim policies".

Copy link
Contributor

Choose a reason for hiding this comment

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

"Instead when a snapshot object was deleted it always resulted in the snapshot being deleted."

=>

"When a snapshot resource is deleted, the snapshot is deleted as well".

Choose a reason for hiding this comment

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

I agree with your suggestion, with just a slight update to the tense, so from:

"In the initial alpha release, snapshots did not support the ability to specify a reclaim policy. Instead when a snapshot object was deleted it always resulted in the snapshot being deleted."

to:

"In the initial alpha release, snapshots did not support reclaim policies. When a snapshot resource was deleted, the snapshot was deleted as well."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tengqm
Copy link
Contributor

tengqm commented Jun 6, 2019

/hold
Asking some native speakers for help here.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2019
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Here are some suggestions. One of them is quite a big change: I suggest using a Task page to explain checking and changing the deletion policy for a VolumeSnapshotContent.


Deletion removes both the `VolumeSnapshotContent` object from the Kubernetes API, as well as the associated storage asset in the external infrastructure.
The purpose of the Snapshot Object in Use Protection feature is to ensure that in-use snapshot API objects are not removed from the system (as this may result in data loss). There are two cases that require “in-use” protection:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The purpose of the Snapshot Object in Use Protection feature is to ensure that in-use snapshot API objects are not removed from the system (as this may result in data loss). There are two cases that require “in-use” protection:
The _Snapshot Object in Use Protection_ feature lets you ensure that in-use snapshot API objects are not removed from the system (as this may result in data loss). There are two cases that require “in-use” protection:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Deletion removes both the `VolumeSnapshotContent` object from the Kubernetes API, as well as the associated storage asset in the external infrastructure.
The purpose of the Snapshot Object in Use Protection feature is to ensure that in-use snapshot API objects are not removed from the system (as this may result in data loss). There are two cases that require “in-use” protection:

* If a volume snapshot is in active use by a persistent volume claim as a source to create a volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If a volume snapshot is in active use by a persistent volume claim as a source to create a volume.
* If a volume snapshot is in active use by a {{< glossary_tooltip text="PersistentVolumeClaim" term_id="persistent-volume-claim" >}} (PVC) as a source to create a Volume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* If a volume snapshot is in active use by a persistent volume claim as a source to create a volume.
* If a `VolumeSnapshotContent` API object is bound to a `VolumeSnapshot` API object, the content object is considered in use.

If a user deletes a `VolumeSnapshot` API object in active use by a PVC, the `VolumeSnapshot` object is not removed immediately. Instead, removal of the `VolumeSnapshot` object is postponed until the `VolumeSnapshot` is no longer actively used by any PVCs. Similarly, if an admin deletes a `VolumeSnapshotContent` that is bound to a `VolumeSnapshot`, the `VolumeSnapshotContent` is not removed immediately. Instead, the `VolumeSnapshotContent` removal is postponed until the `VolumeSnapshotContent` is not bound to the `VolumeSnapshot` object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If a user deletes a `VolumeSnapshot` API object in active use by a PVC, the `VolumeSnapshot` object is not removed immediately. Instead, removal of the `VolumeSnapshot` object is postponed until the `VolumeSnapshot` is no longer actively used by any PVCs. Similarly, if an admin deletes a `VolumeSnapshotContent` that is bound to a `VolumeSnapshot`, the `VolumeSnapshotContent` is not removed immediately. Instead, the `VolumeSnapshotContent` removal is postponed until the `VolumeSnapshotContent` is not bound to the `VolumeSnapshot` object.
If you delete a `VolumeSnapshot` API object that a PVC is actively using, the `VolumeSnapshot` object is not removed immediately. Instead, removal of the `VolumeSnapshot` object is postponed until the `VolumeSnapshot` is no longer actively used by any PVCs. Similarly, if an admin deletes a `VolumeSnapshotContent` that is bound to a `VolumeSnapshot`, the `VolumeSnapshotContent` is not removed immediately. Instead, the `VolumeSnapshotContent` removal is postponed until the `VolumeSnapshotContent` is not bound to the `VolumeSnapshot` object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


### VolumeSnapshotContent Deletion/Retain Policy

The Kubernetes snapshot APIs are similar to the PV/PVC APIs: just like a volume is represented by a bound PVC and PV pair, a snapshot is represented by a bound `VolumeSnapshot` and `VolumeSnapshotContent` pair.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The Kubernetes snapshot APIs are similar to the PV/PVC APIs: just like a volume is represented by a bound PVC and PV pair, a snapshot is represented by a bound `VolumeSnapshot` and `VolumeSnapshotContent` pair.
The Kubernetes snapshot APIs are similar to the PV/PVC APIs: just like a volume is represented by a bound PVC and PV pair, a snapshot is represented by a bound VolumeSnapshot and VolumeSnapshotContent pair.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

The following example demonstrates how to check the deletion policy of a dynamically provisioned `VolumeSnapshotContent`.

```
$ kubectl create -f ./examples/kubernetes/demo-defaultsnapshotclass.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow the style guide, don't include the command prompt.


A `Delete` policy enables automatic deletion of the bound `VolumeSnapshotContent` object from Kubernetes and the associated storage asset in the external infrastructure (such as an AWS EBS snapshot or GCE PD snapshot, etc.). Snapshots that are dynamically provisioned inherit the deletion policy of their `VolumeSnapshotClass`, which defaults to `Delete`. The administrator should configure the `VolumeSnapshotClass` with the desired retention policy. The policy may be changed for individual `VolumeSnapshotContent` after it is created by patching the object.

The following example demonstrates how to check the deletion policy of a dynamically provisioned `VolumeSnapshotContent`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work better as a Task page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. Does the Task page have a link to the Concept page?

Copy link
Contributor

Choose a reason for hiding this comment

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

@xing-yang When you create a new task page, you can certainly link to the relevant concept page(s).

@tengqm
Copy link
Contributor

tengqm commented Jun 11, 2019

@xing-yang We need your help on polishing this one. Thanks!

@xing-yang
Copy link
Contributor Author

@tengqm Sure, I'll update soon. Thanks.

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign zacharysarah
You can assign the PR to them by writing /assign @zacharysarah in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2019
@xing-yang
Copy link
Contributor Author

Addressed comments except for the one about Task page. Thanks.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@k8s-ci-robot
Copy link
Contributor

@xing-yang: 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.

@zacharysarah
Copy link
Contributor

@xing-yang 👋 Thanks for this PR. There's good content here, and I agree with other reviewers that it really belongs as a separate task page. The contribution guidelines can help with creating a new page using a task template.

Please feel free to /reopen when you're ready to continue.

/close

@k8s-ci-robot
Copy link
Contributor

@zacharysarah: Closed this PR.

In response to this:

@xing-yang 👋 Thanks for this PR. There's good content here, and I agree with other reviewers that it really belongs as a separate task page. The contribution guidelines can help with creating a new page using a task template.

Please feel free to /reopen when you're ready to continue.

/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update deletion policy in snapshot doc
7 participants