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

KEP-2644: promote HonorPVReclaimPolicy to GA #5035

Merged
merged 2 commits into from
Feb 10, 2025
Merged

KEP-2644: promote HonorPVReclaimPolicy to GA #5035

merged 2 commits into from
Feb 10, 2025

Conversation

carlory
Copy link
Member

@carlory carlory commented Jan 13, 2025

  • One-line PR description: promote HonorPVReclaimPolicy to GA
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jan 13, 2025
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 13, 2025
@carlory
Copy link
Member Author

carlory commented Jan 13, 2025

/cc @ehashman @deepakkinni @xing-yang

@carlory carlory mentioned this pull request Jan 13, 2025
Merged
@deepakkinni
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2025
@sftim
Copy link
Contributor

sftim commented Jan 22, 2025

/retitle KEP-2644: promote HonorPVReclaimPolicy to GA

@k8s-ci-robot k8s-ci-robot changed the title promote HonorPVReclaimPolicy to GA KEP-2644: promote HonorPVReclaimPolicy to GA Jan 22, 2025
@xing-yang
Copy link
Contributor

@deepakkinni @carlory I don't see any e2e test results from this link: https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy. Can you trigger a run?

@pohly
Copy link
Contributor

pohly commented Jan 22, 2025

Are you trying to promote a feature which has not been continuously tested in a periodic job in the past? That sounds like it doesn't meet the GA criteria yet.

@deepakkinni
Copy link
Member

@deepakkinni @carlory I don't see any e2e test results from this link: https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy. Can you trigger a run?

Hi Xing, I noticed https://testgrid.k8s.io/sig-storage-kubernetes#kind-storage-alpha-beta-features. It seems like it's been running for a while.

@carlory
Copy link
Member Author

carlory commented Jan 23, 2025

@deepakkinni @carlory I don't see any e2e test results from this link: https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy. Can you trigger a run?

@xing-yang the above link only shows the failure tests. @pohly @xing-yang periodic job is https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-storage/sig-storage-kind.yaml#L194.

@xing-yang
Copy link
Contributor

@carlory This link https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy is included in the KEP. Can you update it with the correct link?

@carlory
Copy link
Member Author

carlory commented Jan 23, 2025 •
edited
Loading 

##### e2e tests

<!--
This question should be filled when targeting a release.
For Alpha, describe what tests will be added to ensure proper quality of the enhancement.

For Beta and GA, add links to added tests together with links to k8s-triage for those tests:
https://storage.googleapis.com/k8s-triage/index.html

We expect no non-infra related flakes in the last month as a GA graduation criteria.
-->

- <test>: <link to test coverage>

@xing-yang the KEP template requires this link. Once the feature is GA, https://testgrid.k8s.io/sig-storage-kubernetes#kind-storage-alpha-beta-features won't include these tests. we will see it at https://testgrid.k8s.io/sig-testing. maybe we need to add a new tab (kind-storage) to show storage tests from sig-testing.

image

@xing-yang
Copy link
Contributor

Not sure how the "Include Filter" works. If I enter a non-existing test, I get the same results.

@pohly
Copy link
Contributor

pohly commented Jan 23, 2025

the KEP template requires this link

The text is misleading. The intent clearly is that it should be possible to validate that tests were written, executed regularly, and didn't fail.

Therefore I would add:

  • a link to source code of the tests
  • a link to test grid that shows that tests have been running
  • a link to triage to show success rate (should be empty)

TBH, I think the last one is the least interesting.

@carlory carlory force-pushed the promote-HonorPVReclaimPolicy-to-GA branch from 04ff649 to 9fb844e Compare January 23, 2025 10:19
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 23, 2025
@carlory
Copy link
Member Author

carlory commented Jan 23, 2025

@pohly @xing-yang updated.

@carlory carlory force-pushed the promote-HonorPVReclaimPolicy-to-GA branch from 9fb844e to c672439 Compare January 23, 2025 10:22
@pohly pohly mentioned this pull request Jan 23, 2025
Open
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 2, 2025
@carlory carlory marked this pull request as draft February 2, 2025 04:07
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2025
@xing-yang xing-yang mentioned this pull request Jan 29, 2025
Open
13 tasks
soltysh
soltysh requested changes Feb 4, 2025
View reviewed changes
keps/sig-storage/2644-honor-pv-reclaim-policy/README.md
- [ ] (R) Production readiness review completed
- [ ] (R) Production readiness review approved
- [ ] (R) "Implementation History" section is up-to-date for milestone
- [ ] "Implementation History" section is up-to-date for milestone
- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io]
- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes
Copy link
Contributor

Choose a reason for hiding this comment

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

In the ## Release Signoff Checklist check the appropriate boxes to match the state of the document.

this comment from last time still holds

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this one still holds, I believe most of the above should be checked. Except maybe the one about e2e meeting conformance requirements.

keps/sig-storage/2644-honor-pv-reclaim-policy/README.md
An e2e test to exercise deletion of PV prior to deletion of PVC for a `Bound` PV-PVC pair.

- `test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go`: [k8s-triage](https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy)
- [test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go](https://github.com/kubernetes/kubernetes/blob/2334b8469e1983c525c0c6382125710093a25883/test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go): [k8s-triage](https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy), [k8s-test-grid](https://testgrid.k8s.io/sig-storage-kubernetes#kind-storage-alpha-beta-features&include-filter-by-regex=CSI%20Mock%20honor%20pv%20reclaim%20policy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, the current e2e seem to ONLY verify the order delete PV and then delete PVC, based on my understanding this document we should have tests ensuring that no matter what the order of removing PV and PVC is we should receive appropriate reclaim handling. So I'd expect tests covering both orders, iow. PV then PVC, and PVC then PV.

This still holds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will submit a PR to add the new tests to k/k.

Copy link
Member Author

Choose a reason for hiding this comment

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

will update it once kubernetes/kubernetes#129997 is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

keps/sig-storage/2644-honor-pv-reclaim-policy/README.md

###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?

The current SLOs for `persistentvolume_delete_duration_seconds` and `volume_operation_total_seconds` would be increased by the amount of time taken to remove the newly introduced finalizer. This should be an insignificant increase.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you be more specific what is the insignificant increase? Is it 1% or 10%?

Copy link
Member Author

Choose a reason for hiding this comment

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

@soltysh It depends on the specific CSI driver how much time it takes to delete a volume. It may take a few seconds, a few minutes, or even longer. An API calling for updating a PV object to kube-aiserver should be faster than deleting a volume from the storage backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, maybe worth explicitly calling it out in this document, for other readers.

@carlory carlory force-pushed the promote-HonorPVReclaimPolicy-to-GA branch from 081ef45 to fa392e6 Compare February 5, 2025 09:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2025
@carlory carlory force-pushed the promote-HonorPVReclaimPolicy-to-GA branch 2 times, most recently from 77f0434 to c64778f Compare February 6, 2025 03:15
@carlory carlory mentioned this pull request Feb 6, 2025
Merged
@carlory
update KEP with the latest template
82b916f 
Signed-off-by: carlory <baofa.fan@daocloud.io>
@carlory carlory force-pushed the promote-HonorPVReclaimPolicy-to-GA branch from c64778f to 82b916f Compare February 6, 2025 08:22
@carlory carlory marked this pull request as ready for review February 6, 2025 09:04
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2025
soltysh
soltysh approved these changes Feb 10, 2025
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

A few nits, but overall PRR is good
/approve

keps/sig-storage/2644-honor-pv-reclaim-policy/README.md
- [ ] (R) Production readiness review completed
- [ ] (R) Production readiness review approved
- [ ] (R) "Implementation History" section is up-to-date for milestone
- [ ] "Implementation History" section is up-to-date for milestone
- [ ] User-facing documentation has been created in [kubernetes/website], for publication to [kubernetes.io]
- [ ] Supporting documentation—e.g., additional design documents, links to mailing list discussions/SIG meetings, relevant PRs/issues, release notes
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this one still holds, I believe most of the above should be checked. Except maybe the one about e2e meeting conformance requirements.

keps/sig-storage/2644-honor-pv-reclaim-policy/README.md

###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?

The current SLOs for `persistentvolume_delete_duration_seconds` and `volume_operation_total_seconds` would be increased by the amount of time taken to remove the newly introduced finalizer. This should be an insignificant increase.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, maybe worth explicitly calling it out in this document, for other readers.

keps/sig-storage/2644-honor-pv-reclaim-policy/README.md
An e2e test to exercise deletion of PV prior to deletion of PVC for a `Bound` PV-PVC pair.

- `test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go`: [k8s-triage](https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy)
- [test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go](https://github.com/kubernetes/kubernetes/blob/2334b8469e1983c525c0c6382125710093a25883/test/e2e/storage/csimock/csi_honor_pv_reclaim_policy.go): [k8s-triage](https://storage.googleapis.com/k8s-triage/index.html?sig=scheduling,storage&test=honor%20pv%20reclaim%20policy), [k8s-test-grid](https://testgrid.k8s.io/sig-storage-kubernetes#kind-storage-alpha-beta-features&include-filter-by-regex=CSI%20Mock%20honor%20pv%20reclaim%20policy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, soltysh, xing-yang

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 Feb 10, 2025
@soltysh
Copy link
Contributor

soltysh commented Feb 10, 2025

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 10, 2025
@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 Feb 10, 2025
@k8s-ci-robot k8s-ci-robot merged commit b2b3a1e into kubernetes:master Feb 10, 2025
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.33 milestone Feb 10, 2025
wongchar pushed a commit to ajcaldelas/enhancements that referenced this pull request Feb 11, 2025
@carlory @wongchar
KEP-2644: promote HonorPVReclaimPolicy to GA (kubernetes#5035)
50bd531 
* promote HonorPVReclaimPolicy to GA

* update KEP with the latest template

Signed-off-by: carlory <baofa.fan@daocloud.io>

---------

Signed-off-by: carlory <baofa.fan@daocloud.io>
yliaog pushed a commit to yliaog/enhancements that referenced this pull request Feb 11, 2025
@carlory @yliaog
KEP-2644: promote HonorPVReclaimPolicy to GA (kubernetes#5035)
b73d1da 
* promote HonorPVReclaimPolicy to GA

* update KEP with the latest template

Signed-off-by: carlory <baofa.fan@daocloud.io>

---------

Signed-off-by: carlory <baofa.fan@daocloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly left review comments

@dipesh-rawat dipesh-rawat dipesh-rawat left review comments

@soltysh soltysh soltysh approved these changes

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

@xing-yang xing-yang Awaiting requested review from xing-yang

@deepakkinni deepakkinni Awaiting requested review from deepakkinni

@ehashman ehashman Awaiting requested review from ehashman

@kikisdeliveryservice kikisdeliveryservice Awaiting requested review from kikisdeliveryservice

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@xing-yang xing-yang

@deepakkinni deepakkinni

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Milestone
v1.33
Development

Successfully merging this pull request may close these issues.
8 participants
@carlory @deepakkinni @sftim @xing-yang @pohly @k8s-ci-robot @soltysh @dipesh-rawat