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

Doc: Add blogpost for honor PV reclaim policy fix #30556

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

deepakkinni
Copy link
Member

This PR is a blog post explaining the change in behavior described in the KEP https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/2644-honor-pv-reclaim-policy

cc: @xing-yang @jsafrane

Signed-off-by: Deepak Kinni dkinni@vmware.com

@k8s-ci-robot
Copy link
Contributor

Welcome @deepakkinni!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 18, 2021
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 18, 2021
@netlify
Copy link

netlify bot commented Nov 18, 2021

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: 5b375a6

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/61a7c0174dc46a0007ee3dac

😎 Browse the preview: https://deploy-preview-30556--kubernetes-io-main-staging.netlify.app

sftim
sftim previously requested changes Nov 18, 2021
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.

Hi. Here's some partial feedback.

If you take this on board, we'll have more feedback I'm afraid - but it should only be a few things to fix.

@sftim sftim dismissed their stale review November 19, 2021 09:17

Feedback was incorporated

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.

Hi. Some important nits, if that's not a contradiction in terms.

@xing-yang
Copy link
Contributor

/assign

@sftim
Copy link
Contributor

sftim commented Nov 20, 2021

BTW, there is also a v1.23 feature about StatefulSets and PVC reclaim. I suggest scheduling this post to be just before or just after the StatefulSet PVC reclaim article (or even: both on the same day).

@deepakkinni
Copy link
Member Author

BTW, there is also a v1.23 feature about StatefulSets and PVC reclaim. I suggest scheduling this post to be just before or just after the StatefulSet PVC reclaim article (or even: both on the same day).

I am not sure how to control the release day.

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.

Hi Deepak. Here's some more feedback.

I'm not sure about the title. We're not making any changes to reclaim policies; what we're doing is a partial bug fix, and then only for CSI storage integrations.

I preferred the title I proposed before:

Kubernetes 1.23: PersistentVolume reclaim improvements

I'd also consider:

A fix for a bug in PersistentVolume storage reclaim

@sftim
Copy link
Contributor

sftim commented Nov 23, 2021

If someone's running a v1.22 cluster and upgrades external-provisioner to v4.0.0 or later, and enable the HonorPVReclaimPolicy feature gate for external-provisioner, is that enough to get the fix? Do they need Kubernetes v1.23 at all?

@xing-yang
Copy link
Contributor

Regarding the title, how about "Prevent PV leaks when deleting out of order"?

@deepakkinni
Copy link
Member Author

If someone's running a v1.22 cluster and upgrades external-provisioner to v4.0.0 or later, and enable the HonorPVReclaimPolicy feature gate for external-provisioner, is that enough to get the fix? Do they need Kubernetes v1.23 at all?

The fix is applicable only to CSI volumes and migrated volumes after upgrading only the external-provisioner to v4.0.0 or later. However, if they turn off CSI migration and the cluster is at v1.22 the pv controller does not remove the finalizer added by the external-provisioner this will prevent the volumes from being deleted, and the finalizers have to be removed manually by the admin. It can be said that if only CSI volumes are in picture then upgrading external-provisioner is sufficient, if there are csi migrated volumes then the k8s v1.23 is needed.

@deepakkinni
Copy link
Member Author

Regarding the title, how about "Prevent PV leaks when deleting out of order"?

@sftim what do you think of this title?

Options:

  1. Kubernetes 1.23: PersistentVolume reclaim improvements
  2. A fix for a bug in PersistentVolume storage reclaim
  3. Prevent PV leaks when deleting out of order

@sftim
Copy link
Contributor

sftim commented Nov 24, 2021

I'd go for a slightly longer version: Prevent PersistentVolume leaks when deleting out of order
(bytes are cheap 😉)

@sftim
Copy link
Contributor

sftim commented Nov 24, 2021

@deepakkinni let's be clear about what components need the new feature gate turned on in order to get the fix.

@deepakkinni
Copy link
Member Author

@deepakkinni let's be clear about what components need the new feature gate turned on in order to get the fix.

It's emphasized in "PV reclaim policy with Kubernetes v1.23"

Signed-off-by: Deepak Kinni <dkinni@vmware.com>
@sftim
Copy link
Contributor

sftim commented Dec 3, 2021

Folks, is this article looking technically correct?

@xing-yang
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 3, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5f17ecafa367a65c1dd4673e55eb5819c483dd57

@jimangel
Copy link
Member

jimangel commented Dec 6, 2021

/milestone 1.23
/hold
/cc @jlbutler

@k8s-ci-robot k8s-ci-robot requested a review from jlbutler December 6, 2021 04:03
@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 Dec 6, 2021
@k8s-ci-robot k8s-ci-robot added this to the 1.23 milestone Dec 6, 2021
@karenhchu
Copy link
Contributor

BTW, there is also a v1.23 feature about StatefulSets and PVC reclaim. I suggest scheduling this post to be just before or just after the StatefulSet PVC reclaim article (or even: both on the same day).

I am not sure how to control the release day.

Hello! K8s 1.23 Release Team Comms Lead here. Confirming that the StatefulSet AutoDelete blog post is being updated to be published on 12/16, a day after this blog.

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.

With technical LGTM in place, let's merge this.

I'll open a PR with proposed fixes.

/hold cancel
/approve

@@ -0,0 +1,199 @@
---
layout: blog
title: "Kubernetes 1.23 Prevent PersistentVolume leaks when deleting out of order"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
title: "Kubernetes 1.23 Prevent PersistentVolume leaks when deleting out of order"
title: "Kubernetes 1.23: Prevent PersistentVolume leaks when deleting out of order"


**Author:** Deepak Kinni (VMware)

[PersistentVolume](/docs/concepts/storage/persistent-volumes/) (or PVs for short) are
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
[PersistentVolume](/docs/concepts/storage/persistent-volumes/) (or PVs for short) are
Kubernetes [PersistentVolumes](/docs/concepts/storage/persistent-volumes/) (or PVs for short) are

Comment on lines +11 to +12
associated with [Reclaim Policy](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reclaim-policy).
The Reclaim Policy is used to determine the actions that need to be taken by the storage
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
associated with [Reclaim Policy](https://kubernetes.io/docs/concepts/storage/persistent-volumes/#reclaim-policy).
The Reclaim Policy is used to determine the actions that need to be taken by the storage
associated with a [_reclaim policy_](/docs/concepts/storage/persistent-volumes/#reclaim-policy).
That reclaim policy is used to determine the actions that need to be taken by the storage


## How did reclaim work in previous Kubernetes releases?

[PersistentVolumeClaim](/docs/concepts/storage/persistent-volumes/#Introduction) (or PVC for short) is
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
[PersistentVolumeClaim](/docs/concepts/storage/persistent-volumes/#Introduction) (or PVC for short) is
A [PersistentVolumeClaim](/docs/concepts/storage/persistent-volumes/#Introduction) (or PVC for short) is

## How did reclaim work in previous Kubernetes releases?

[PersistentVolumeClaim](/docs/concepts/storage/persistent-volumes/#Introduction) (or PVC for short) is
a request for storage by a user. A PV and PVC are considered [Bound](/docs/concepts/storage/persistent-volumes/#Binding)
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
a request for storage by a user. A PV and PVC are considered [Bound](/docs/concepts/storage/persistent-volumes/#Binding)
a request for storage by a user. A PV and PVC are considered
[_bound_](/docs/concepts/storage/persistent-volumes/#binding) to each other

? (note case fix for fragment identifier #binding)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit 90f6bf7 into kubernetes:main Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/blog Issues or PRs related to the Kubernetes Blog subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.

7 participants