KEP Proposal: API for PVC Namespace Transfer #1555

mhenriks · 2020-02-17T21:11:26Z

Signed-off-by: Michael Henriksen [email protected]

This follows on previous works by @j-griffith here and here

For now, let's focus on storage resources and specifically PVCs.

k8s-ci-robot · 2020-02-17T21:11:33Z

Welcome @mhenriks!

It looks like this is your first PR to kubernetes/enhancements 🎉. 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/enhancements 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 · 2020-02-17T21:11:33Z

Hi @mhenriks. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

k8s-ci-robot · 2020-02-17T21:11:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhenriks
To complete the pull request process, please assign childsb
You can assign the PR to them by writing /assign @childsb 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:

keps/sig-storage/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tedyu · 2020-02-20T16:19:27Z

keps/sig-storage/20200217-namespace-transfer.md

+
+- Make sure the associated PersistentVolume is not deleted/recycled during the transfer by setting reclaim policy to `Retain` (if not already).
+- Delete the PVC `source-namespace\foo`.
+- Bind The PersistentVolume to `target-namespace\bar`.


Earlier, targetName (with value of bar) is said to be optional.
If the value is omitted, the transfer would use existing name from the source.

It is better to explain this in the procedure.

Personally I'd prefer not to be implicit, that's one of the reasons I proposed using the datasource model, we can easily enforce things like quota, limits and required claim parameters that way IMO.

Yeah, with the datasource model, the user will directly see if creating the target PVC fails because of a quota limitation. Otherwise, it will happen in the background and be reported in the status of the StorageTransferRequest.

What other required parameters are a concern? And does validating the new target PVC require referencing the source PVC to validate compatible parameters? If so, that would be complicated.

My main philosophy has been just to make it as close to the existing create workflow as possible and avoid assumptions. It also means you don't have to duplicate parameters or testing for them. Instead you use the existing create implementation as your foundation and build on it rather than trying to make it better or offering a divergent path. Just my 2 cents though.

tedyu · 2020-02-20T16:24:55Z

keps/sig-storage/20200217-namespace-transfer.md

+
+  // TargetName allows for the source and target resources to have different names
+  // It is optional
+  TargetName *string


It may be better to always keep this field populated (which would simplify checking in the code)

Hi @tedyu - thanks for taking a look. Yes, your suggestion would definitely make the implementation simpler. But I wonder about the case in which the user doesn't have access to the source namespace and some coordination with an admin is necessary. Now one more piece of information is required to be communicated. Maybe it's not a big deal, but at this point I kind of feel like the benefit outweighs the cost.

Since the transfer is secure, I wonder under what scenario the user doesn't have access to the source namespace.

A hypothetical case could be something like: Department A needs a copy of a database (PVC) owned by department B. Each department operates only in their own namespace. The admins for A and B can work together to facilitate the transfer rather than getting a cluster admin involved.

Maybe it makes sense to assume that a typical user of this API has access to both namespaces. If so, binding requests/responses is simpler. Dynamic binding can be a later feature.

tedyu · 2020-02-20T16:30:05Z

keps/sig-storage/20200217-namespace-transfer.md

+
+```golang
+type StorageTransferRequestStatus struct {
+  Complete bool


Can we change the bool field to int field where 100 (percent) means completion ?

Progress int

This way, if the controller can estimate the progress, user would get better idea.

How do you suggest measuring progress? No data is transferred. The transfer simply involves updating metadata and should occur quickly after requests/approvals are matched.

Another option is to use enum to represent each step as outlined starting line 147 above. Actually this field can be a string where 'done' or 'complete' marks the end of operation.
It is clearer than a single bool.

tedyu · 2020-02-20T16:35:23Z

keps/sig-storage/20200217-namespace-transfer.md

+
+I believe the API can be implemented as CRDs and an external controller.  If so, is a KEP really required?  Would there be advantages to implementing in-tree?
+
+What is the best way to deal with unbound PVCs (WaitForFirstConsumer)?  Transfer the unbound PVC?  Wait until it's bound?  May be race conditions with the former.


One option is to report error for unbound PVCs.
When the PVC is bound, the transfer process can start again.

mikedanese · 2020-02-22T22:52:02Z

keps/sig-storage/20200217-namespace-transfer.md

+to other resource types as compelling use cases come up.
+
+`StorageTransferRequest` resources are created in the target namespace.
+`StorageTransferApproval` resources are created in the source namespace.


@kubernetes/sig-auth-api-reviews FYI.

sftim · 2020-02-22T23:25:29Z

/ok-to-test

sftim · 2020-02-22T23:26:41Z

keps/sig-storage/20200217-namespace-transfer.md

@@ -0,0 +1,283 @@
+---
+title: Namespace Transfer


Suggested change

title: Namespace Transfer

title: Namespace transfer for storage

fejta-bot · 2020-02-22T23:55:33Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Signed-off-by: Michael Henriksen <[email protected]>

j-griffith

Thanks for working on this @mhenriks I had a couple of questions and a thought around tieing this together with DataSources; would be curious what you think of the idea.

j-griffith · 2020-02-28T23:20:49Z

keps/sig-storage/20200217-namespace-transfer.md

+When a controller detects matching request/approval resources, a transfer in initiated.
+
+When transferring a PersistentVolumeClaim, the associated PersistentVolume will be updated,
+the source PersistentVolumeClaim will be deleted, and the target PersistentVolumeClaim will be created.


The more I thought about this the last time around, the more I liked the idea of deleting the original claim; I think that's the right step FWIW.

In another comment here, you propose having the triggering the transfer via the pvc spec.dataSource field. In that case do you think it would still be preferable to delete the original claim? And if so, when? When requests/approvals are matched or the target PVC is created?

Yeah, that still aligns IMO, once the transfer process is completed then you'd delete the claim from the original namespace.

j-griffith · 2020-02-28T23:23:08Z

keps/sig-storage/20200217-namespace-transfer.md

+    kind: PersistentVolumeClaim
+  targetName: bar #this is optional
+```
+


I don't know that it's necessary, but do we want to consider requiring a secret associated with this? It may be overkill because it's an active operation on both sides and it will only transfer to the destination specified by the source; but just something to think about (briefly). Maybe that's an implementation detail and we address it later.

j-griffith · 2020-02-28T23:34:53Z

keps/sig-storage/20200217-namespace-transfer.md

+```
+
+When matching `StorageTransferRequest` and `StorageTransferApproval` resources are detected, the transfer process is initiated.
+


One thing I thought might improve this, rather than immediately taking the action on this approval object, would it make sense to leverage the pvc datasource model that we've been using for other things? So for example what I'm thinking is that to import or accept a transfer the new namespace still issues a new PVC request, but they use a StorageTransferApproval object like you have here as the DataSource.

So the controller would wait for not only the matching requests, but the final even to kick this off would be a create pvc from that transfer datasource. Maybe that's more of an implementation detail.

The StorageTransferApproval is not in the same namespace as the target PVC, so the DataSource would have to refer to the StorageTransferRequest.

I agree that this would make the controller simpler. But decided against it because:

its one more thing for the user to do. And what if the target spec doesn't match the source?

the DataSource field is not appropriately ignored/handled for all provisioners at current.

2 is obviously also an issue for the populators, so maybe shouldn't be too concerned about it now? But it could lead to some undesirable behavior in the meantime.

Sorry, I often reverse who does the request and who does the accept... As far as the other points, I see both sides (mine and yours). Would be curious to see if anybody else has thoughts, I'm open to your ideas. Just want to make sure usability is good and if there's places to leverage existing work we do so.

j-griffith · 2020-02-28T23:36:12Z

keps/sig-storage/20200217-namespace-transfer.md

+
+- Make sure the associated PersistentVolume is not deleted/recycled during the transfer by setting reclaim policy to `Retain` (if not already).
+- Delete the PVC `source-namespace\foo`.
+- Bind The PersistentVolume to `target-namespace\bar`.


Personally I'd prefer not to be implicit, that's one of the reasons I proposed using the datasource model, we can easily enforce things like quota, limits and required claim parameters that way IMO.

mhenriks

Thanks for the feedback @j-griffith and @tedyu.

I think the biggest thing to decide is whether the user creates the target PVC is created internally or explicitly by the user (via spec.dataSource).

mhenriks · 2020-03-02T16:41:23Z

keps/sig-storage/20200217-namespace-transfer.md

+When a controller detects matching request/approval resources, a transfer in initiated.
+
+When transferring a PersistentVolumeClaim, the associated PersistentVolume will be updated,
+the source PersistentVolumeClaim will be deleted, and the target PersistentVolumeClaim will be created.


In another comment here, you propose having the triggering the transfer via the pvc spec.dataSource field. In that case do you think it would still be preferable to delete the original claim? And if so, when? When requests/approvals are matched or the target PVC is created?

mhenriks · 2020-03-02T19:18:46Z

keps/sig-storage/20200217-namespace-transfer.md

+```
+
+When matching `StorageTransferRequest` and `StorageTransferApproval` resources are detected, the transfer process is initiated.
+


The StorageTransferApproval is not in the same namespace as the target PVC, so the DataSource would have to refer to the StorageTransferRequest.

I agree that this would make the controller simpler. But decided against it because:

its one more thing for the user to do. And what if the target spec doesn't match the source?

the DataSource field is not appropriately ignored/handled for all provisioners at current.

2 is obviously also an issue for the populators, so maybe shouldn't be too concerned about it now? But it could lead to some undesirable behavior in the meantime.

mhenriks · 2020-03-02T19:45:51Z

keps/sig-storage/20200217-namespace-transfer.md

+
+- Make sure the associated PersistentVolume is not deleted/recycled during the transfer by setting reclaim policy to `Retain` (if not already).
+- Delete the PVC `source-namespace\foo`.
+- Bind The PersistentVolume to `target-namespace\bar`.


Yeah, with the datasource model, the user will directly see if creating the target PVC fails because of a quota limitation. Otherwise, it will happen in the background and be reported in the status of the StorageTransferRequest.

What other required parameters are a concern? And does validating the new target PVC require referencing the source PVC to validate compatible parameters? If so, that would be complicated.

mhenriks · 2020-03-02T19:52:37Z

keps/sig-storage/20200217-namespace-transfer.md

+
+  // TargetName allows for the source and target resources to have different names
+  // It is optional
+  TargetName *string


Maybe it makes sense to assume that a typical user of this API has access to both namespaces. If so, binding requests/responses is simpler. Dynamic binding can be a later feature.

…oval.spec.requestName required parameters Also, StorageTransferRequest.spec.targetName is required Also removed some details that may be better for design doc. Signed-off-by: Michael Henriksen <[email protected]>

fejta-bot · 2020-06-13T21:38:21Z

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

fejta-bot · 2020-07-13T22:19:33Z

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

fejta-bot · 2020-08-12T23:01:12Z

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 · 2020-08-12T23:01:25Z

@fejta-bot: Closed this PR.

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.

mhenriks · 2020-10-21T22:25:34Z

/reopen
/remove-lifecycle rotten

k8s-ci-robot · 2020-10-21T22:25:47Z

@mhenriks: Failed to re-open PR: state cannot be changed. The namespace-transfer branch was force-pushed or recreated.

In response to this:

/reopen
/remove-lifecycle rotten

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.

mhenriks · 2020-10-21T22:28:04Z

/reopen

k8s-ci-robot · 2020-10-21T22:28:16Z

@mhenriks: Failed to re-open PR: state cannot be changed. The namespace-transfer branch was force-pushed or recreated.

In response to this:

/reopen

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.

update enhancement template

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 17, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 17, 2020

k8s-ci-robot requested review from childsb and saad-ali February 17, 2020 21:12

k8s-ci-robot added 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 Feb 17, 2020

xing-yang mentioned this pull request Feb 20, 2020

Cloning volumes across different namespaces kubernetes/kubernetes#85519

Closed

tedyu reviewed Feb 20, 2020

View reviewed changes

mikedanese reviewed Feb 22, 2020

View reviewed changes

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Feb 22, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2020

sftim reviewed Feb 22, 2020

View reviewed changes

mhenriks added 2 commits February 24, 2020 18:53

initial namespace transfer kep

bf5784a

Signed-off-by: Michael Henriksen <[email protected]>

update title and participating sigs

e134abf

Signed-off-by: Michael Henriksen <[email protected]>

mhenriks force-pushed the namespace-transfer branch from fbc0ff0 to e134abf Compare February 24, 2020 23:53

j-griffith reviewed Feb 28, 2020

View reviewed changes

mhenriks commented Mar 2, 2020

View reviewed changes

mhenriks force-pushed the namespace-transfer branch from f9aad52 to d4071c4 Compare March 15, 2020 20:55

mhenriks force-pushed the namespace-transfer branch from d4071c4 to ee28d1f Compare March 15, 2020 20:59

nrb mentioned this pull request Mar 16, 2020

Relation between VolumeSnapshotContent and VolumeSnapshot should be 1:many kubernetes-csi/external-snapshotter#273

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 13, 2020

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 Jul 13, 2020

k8s-ci-robot closed this Aug 12, 2020

JohnStrunk mentioned this pull request Aug 30, 2020

Protect against accidental namespace deletion backube/snapscheduler#107

Open

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 21, 2020

mhenriks mentioned this pull request Oct 21, 2020

API for PVC Namespace Transfer #2110

Closed

huffmanca mentioned this pull request Mar 4, 2021

Add proposal for transfer of Storage resources, such as PersistentVolumeClaims or VolumeSnapshots #2326

Closed

Elbehery mentioned this pull request Aug 2, 2021

Add proposal for transfer of VolumeSnapshot #2849

Closed

RomanBednar pushed a commit to RomanBednar/enhancements that referenced this pull request Mar 1, 2024

Merge pull request kubernetes#1555 from dhellmann/template-update-2024

e989a9d

update enhancement template


		I believe the API can be implemented as CRDs and an external controller. If so, is a KEP really required? Would there be advantages to implementing in-tree?

		What is the best way to deal with unbound PVCs (WaitForFirstConsumer)? Transfer the unbound PVC? Wait until it's bound? May be race conditions with the former.

	title: Namespace Transfer
	title: Namespace transfer for storage

		```

		When matching `StorageTransferRequest` and `StorageTransferApproval` resources are detected, the transfer process is initiated.

KEP Proposal: API for PVC Namespace Transfer #1555

KEP Proposal: API for PVC Namespace Transfer #1555

Conversation

mhenriks commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

tedyu Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedyu Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sftim commented Feb 22, 2020

Choose a reason for hiding this comment

fejta-bot commented Feb 22, 2020

j-griffith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Jun 13, 2020

fejta-bot commented Jul 13, 2020

fejta-bot commented Aug 12, 2020

k8s-ci-robot commented Aug 12, 2020

mhenriks commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020

mhenriks commented Oct 21, 2020

k8s-ci-robot commented Oct 21, 2020

tedyu Feb 20, 2020 •

edited

Loading

tedyu Feb 21, 2020 •

edited

Loading