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

StorageObjectInUseProtection (was Persistent Volume Protection) #499

Closed
pospispa opened this Issue Oct 26, 2017 · 42 comments

Comments

@pospispa

pospispa commented Oct 26, 2017

Feature Description

  • One-line feature description (can be used as a release note): Prevent deletion of Storage Object that is In active Use (was Prevent deletion of Persistent Volume that is bound to a Persistent Volume Claim)
  • Primary contact (assignee): Pavel Pospisil (@pospispa)
  • Responsible SIGs: @kubernetes/sig-storage-feature-requests
  • Design proposal link (community repo):
  • Reviewer(s) - (for LGTM) recommend having 2+ reviewers (at least one from code-area OWNERS file) agreed to review. Reviewers from multiple companies preferred: @jsafrane @msau42
  • Approver (likely from SIG/area to which feature belongs): @jsafrane
  • Feature target (which target equals to which milestone):
    • Alpha release target (x.y): 1.10 skipped
    • Beta release target (x.y): 1.10
    • Stable release target (x.y): 1.11
@pospispa

This comment has been minimized.

pospispa commented Oct 26, 2017

/assign

@pospispa

This comment has been minimized.

pospispa commented Oct 26, 2017

This feature aims to fix the bug: kubernetes/kubernetes#33355

@saad-ali

This comment has been minimized.

Member

saad-ali commented Nov 17, 2017

This did not make it in to 1.9, punting to next release

@NickrenREN

This comment has been minimized.

Member

NickrenREN commented Jan 12, 2018

/assign

@idvoretskyi

This comment has been minimized.

Member

idvoretskyi commented Jan 15, 2018

@pospispa

This comment has been minimized.

pospispa commented Jan 15, 2018

@NickrenREN has taken over development of this feature. @NickrenREN will you, please, comment on the status of the development of this feature.

@NickrenREN

This comment has been minimized.

Member

NickrenREN commented Jan 15, 2018

Yeah, on track for 1.10. I am writing the proposal . will send it out tomorrow.

@jsafrane

This comment has been minimized.

Member

jsafrane commented Jan 19, 2018

added 1.10 milestone

@Bradamant3

This comment has been minimized.

Member

Bradamant3 commented Mar 2, 2018

@NickrenREN do we need additional doc for this feature? If so, please submit PR as soon as possible. Either way, please update the 1.10 feature tracking spreadsheet
Thanks!

@msau42

This comment has been minimized.

Member

msau42 commented Mar 2, 2018

The PR is here, but I think needs to be updated to use the new name of the admission controller:
https://github.com/kubernetes/website/pull/7165/files

@NickrenREN

This comment has been minimized.

Member

NickrenREN commented Mar 3, 2018

@msau42 admission controller name is being updated here: https://github.com/kubernetes/website/pull/7576/files
@Bradamant3 i am writing PV protection description, will send it out soon

@NickrenREN

This comment has been minimized.

Member

NickrenREN commented Mar 3, 2018

@saad-ali

This comment has been minimized.

Member

saad-ali commented Mar 13, 2018

Correct me if I am wrong but this feature skipped alpha and went directly to beta because it piggy backed on the work done for #498.

Changing labels to reflect that.

@saad-ali saad-ali added stage/beta and removed stage/alpha labels Mar 13, 2018

@AishSundar

This comment has been minimized.

AishSundar commented May 23, 2018

@pospispa I am the CI Signal lead for 1.11 and also work on Conformance testing program for K8s. I see this feature is going to Stable in 1.11.
I assume you already have e2e tests for it. I am following up to see if and which of those tests should we promote to the conformance suite in 1.11
As part of the process to increase conformance coverage, outlined by Conformance WG and Sig-Arch, we expect features going into stable/GA to have representation in Conformance suite. Your update on the same will help us evaluate this feature better.

@pospispa

This comment has been minimized.

pospispa commented May 27, 2018

@AishSundar there're 2 E2E tests for the Storage Object In Use Protection feature:

IMHO, both E2E tests should be added to Conformance suite.

Please, what am I supposed to do now?

@AishSundar

This comment has been minimized.

AishSundar commented May 27, 2018

@pospispa if you/owning SIG determine that these tests should be part of Conformance (for base profile) then the next step is to send a PR to promote the test to Conformance suite. Here's a sample PR

  • Please use "Promote xxx e2e test to Conformance" as template of your PR title
  • Tag your PR with "/area conformance" label
  • Send your PR to Sig-Architecture for review by adding "@kubernetes/sig-architecture-pr-reviews"
  • CC your Sig, Sig-Architecture and me in your PR

Lastly Code freeze for v1.11 is 6/5, so we should ideally aim for the PR to be merged by then.

cc @jagosan @mithrav as FYI
cc @jberkus @tpepper we should look to make sure this PR makes it in before code freeze.

@AishSundar

This comment has been minimized.

AishSundar commented May 27, 2018

cc @timothysc as FYI

@msau42

This comment has been minimized.

Member

msau42 commented May 29, 2018

Are PVCs and dynamic provisioning currently part of conformance?

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

@pospispa I had a chance to talk to 1.11 RT about this PR. Considering we will be entering CodeFreeze next Tuesday, 6/5, we should ideally aim for the promotion PR to be reviewed and merge by EOD Thursday 5/31 for this to make into 1.11. We will then have time for a couple of test runs before code freeze and branch cut. Please let me know if you think this can be wrapped by Thursday. Thanks.

@msau42

This comment has been minimized.

Member

msau42 commented May 29, 2018

I spoke with @saad-ali briefly about adding this to the conformance suite. We're not sure dynamic provisioning in general can be supported in conformance because it requires vendor-specific volume plugins to work.

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

@bgrant0607 for some guidance on adding this scenario to Conformance suite.

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented May 29, 2018

There are multiple issues:

  1. Dynamic provisioning is higher-level automation, such as cluster autoscaling, and is optional.
  2. All NAS volume sources are optional, provider-specific, and non-portable.
  3. We don't have any PV, PVC, or StorageClass conformance tests currently.

We need to finalize the notion of profiles before we can add optional and/or non-portable functionality to the suite.

The way I'd like to craft the profile(s) is based on the abstract functionality provided, such as by the default/standard StorageClass(es), rather than by provider-specific functionality/types. For example, "PVCs with shared-read volumes" could be something we add to a conformance profile.

It would be helpful for the Storage SIG to draft a proposal for what functionality could be expected to be reasonably portable and widely supported that we could shape into a profile or add to a profile (e.g., a cloud-provider profile).

cc @kubernetes/sig-architecture-api-reviews @kubernetes/sig-storage-api-reviews

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

Thanks @bgrant0607.

@msau42 @pospispa looks like this warrants a more involved discussion and might not be in scope for 1.11 timeframe(Code freeze is 6/5). Correct me if you think otherwise.

I will schedule a followup meeting with @msau42 @saad-ali @pospispa @WilliamDenniss to explore profiles in context of Sig Storage.

@AishSundar

This comment has been minimized.

AishSundar commented May 29, 2018

@pospispa I am trying to add you to a meeting, but am unable to find your contact in Slack. Please provide me with an email id to add to the sync.

@pospispa

This comment has been minimized.

pospispa commented May 30, 2018

@msau42 @pospispa looks like this warrants a more involved discussion and might not be in scope for 1.11 timeframe(Code freeze is 6/5). Correct me if you think otherwise.

My understanding of the comment is same. As it requires several prerequisites described in comment IMHO, it's too late for the 1.11 timeframe.

@AishSundar my email is pospispa@gmail.com and I live in Central European timezone.

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented May 31, 2018

I'm also not so keen to add a mechanism that's only applicable to storage resources.

What mechanism is this intending to use? Finalizers?

@pospispa

This comment has been minimized.

pospispa commented May 31, 2018

@bgrant0607 the StorageObjectInUseProtection feature prevents PV removal from the system in case it is bound to a PVC. Or the feature prevents PVC removal in case it's actively used by a pod. The StorageObjectInUseProtection feature uses finalizers to prevent the removal.

Current StorageObjectInUseProtection E2E tests use a StorageClass to create PV <-> PVC pairs because of ease of use. However, usage of the StorageClass is not necessary. The E2E tests also create a pod.

@bgrant0607 do you currently need any additional information about the StorageObjectInUseProtection feature?

@bgrant0607

This comment has been minimized.

Member

bgrant0607 commented Jun 1, 2018

@pospispa Prevents how?

@msau42

This comment has been minimized.

Member

msau42 commented Jun 1, 2018

It prevents removal by adding a finalizer to the PV if it is bound to a PVC, and/or to the PVC if it's in use by a Pod

@pospispa

This comment has been minimized.

pospispa commented Jun 2, 2018

I'd like to add that there's a K8s server API feature that prevents removal of objects whose list of finalizers is not empty. In case such an object is deleted by a user the object with non-empty finalizers list is not removed but it's deletion timestamp is set. As soon as the object's finalizers list becomes empty the object is removed from the system.

@kacole2

This comment has been minimized.

Contributor

kacole2 commented Jul 23, 2018

@pospispa This feature was worked on in the previous milestone, so we'd like to check in and see if there are any plans for this to graduate stages in Kubernetes 1.12. This still has the 1.11 milestone as well so we need to update it accordingly.

Can you please update the feature milestone tracking in the original post as well?
If there are any updates, please explicitly ping @justaugustus, @kacole2, @robertsandoval, @rajendar38 to note that it is ready to be included in the Features Tracking Spreadsheet for Kubernetes 1.12.


Please note that the Features Freeze is July 31st, after which any incomplete Feature issues will require an Exception request to be accepted into the milestone.

In addition, please be aware of the following relevant deadlines:

  • Docs deadline (open placeholder PRs): 8/21
  • Test case freeze: 8/28

Please make sure all PRs for features have relevant release notes included as well.

Happy shipping!

@msau42

This comment has been minimized.

Member

msau42 commented Jul 23, 2018

This feature went GA in 1.11. I think this can be closed?

@justaugustus

This comment has been minimized.

Member

justaugustus commented Jul 23, 2018

Good catch, @msau42!
/close

justaugustus pushed a commit to justaugustus/features that referenced this issue Sep 3, 2018

Merge pull request kubernetes#499 from diwu1989/patch-1
Update aws_under_the_hood.md to clarify pod vs node instance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment