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
PV controller changes to support PV Deletion protection finalizer #105773
Conversation
@deepakkinni: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
Welcome @deepakkinni! |
Hi @deepakkinni. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/assign @jsafrane |
/assign |
/milestone v1.23 |
/retest |
Can you please add also code for the feature? IMO it's so small that it would fit a single PR (perhaps except e2e tests). |
e097910
to
05e7c89
Compare
done. |
44e08b3
to
6b8d8fd
Compare
6b8d8fd
to
3f4ecf0
Compare
/retest |
3f4ecf0
to
7231416
Compare
/retest |
7231416
to
df7e84e
Compare
df7e84e
to
f5d9f05
Compare
Signed-off-by: Deepak Kinni <dkinni@vmware.com>
f5d9f05
to
bfd5f23
Compare
/lgtm There are "sibling" PRs is other components that look mostly OK: kubernetes-sigs/sig-storage-lib-external-provisioner#117 kubernetes-csi/external-provisioner#679 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deepakkinni, jsafrane 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 |
// provisioner name depending if the annotation came from a PVC or not. | ||
// It will then add a "pv.kubernetes.io/migrated-to" annotation if migration with | ||
// the CSI driver name for that provisioner is "on" based on feature flags, it will also | ||
// remove the annotation if migration is "off" for that provisioner in rollback | ||
// scenarios. Returns true if the annotations map was modified and false otherwise. | ||
// remove the PV deletion protection finalizer and "pv.kubernetes.io/migrated-to" annotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jsafrane @xing-yang I'm not clear why the finalizer feature is related to csi migration. Could you explain the reasoning?
We're trying to turn on gce migration in #104722 but the unit test is failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the finalizer feature is turned on, the finalizer will be added to all csi volumes including csi migrated volumes as well. When the finalizer feature is turned off, we need to make sure the finalizers are deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear why the logic to remove the finalizer is tied with the logic that determines if migration is enabled or not. I think it should be independent of any migration logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The finalizer is added by the external-provisioner on migrated volumes, however, when migration is disabled pv-controller becomes responsible to remove the finalizer since external-provisioner is no longer responsible for that pv. The side-effect of detecting a migration off
is removing the finalizer on the pv. The fix is pretty much what is being done in #104722, i.e, introduce a disabledDriverGates
in the test struct and add CSIMigrationGCE
to if the test description mentions disabled migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deepakkinni can you please fix the unit tests? They should set their own feature gate(s) and not depend on the current default in kube_features.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I remember @deepakkinni said something about public holiday, I submitted #106376
What type of PR is this?
/kind feature
/sig storage
What this PR does / why we need it:
Add the alpha HonorPVReclaimPolicy feature gate
Which issue(s) this PR fixes:
Fixes # kubernetes/enhancements#2644
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Signed-off-by: Deepak Kinni dkinni@vmware.com