-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Remove finalizers on CSI snapshot objects #29475
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
Conversation
|
@huffmanca and @duanwei33 PTAL for technical and QE review. Also, please confirm that this is for 4.7+ only. (CSI Snapshot is GA in 4.7.) Thanks. |
|
Deploy preview for osdocs ready! Built with commit b484bf1 |
|
I'm very hesitant on including instructions for removing VolumeSnapshot finalizers as the default documentation. What I'd do is:
Then in this optional section we can include the |
|
@huffmanca Thanks for the feedback! I reworked this so that it ends as an optional set of steps to follow if the initial default instructions are not enough. I dropped "it is required/must" wording because it falls in the "optional" step, so hopefully your suggested meaning still remains. PTAL and let me know if this ordering is logical to you. |
|
@bobfuru I noticed this section name is Deleting a volume snapshot, so from the user side, we need a common use case and an optional part to introduce this bug fix. |
|
Thanks, @duanwei33! I'm not sure that I fully captured what you are suggesting but could you PTAL and let me know if the ordering and wording make sense with my latest update? |
Hi @bobfuru, thanks for the update, yes this is what I mean to. Just one comment, deleting volumesnapshotcontent depends on the user requirement in the following section, so do you think the update or something like that is ok? Delete the volume snapshot: $ oc delete volumesnapshot <volumesnapshot_name> If the deletion policy is Retain and you want to delete volumesnapshotcontent object, enter the following commands: $ oc delete volumesnapshotcontent <volumesnapshotcontent_name> |
It's helpful that you pointed out this out. I've made some small tweaks along the lines of your suggestion. |
|
@huffmanca and @duanwei33 Could you both PTAL once more and let me know if this looks good? Thanks. |
|
/LGTM |
|
@huffmanca - Ignore my earlier comment, I think this should be backported to all versions back to 4.5, correct? |
huffmanca
left a comment
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.
Just some nitpicks and rewording. Otherwise it looks good to me!
|
@bobfuru , In regards to backporting, VolumeSnapshots were introduced as tech preview in OCP 4.4. I think we'd need to backport it through all versions to 4.4. |
|
New changes are detected. LGTM label has been removed. |
|
Thanks, @huffmanca! I made a few small edits to be more in line with your most recent comments. You are correct that CSI snapshot was introduced in 4.4 but now that we're in 4.7 GA, we only support back to 4.5 as 4.4 support has ended. |
|
LGTM from @huffmanca via Slack. |
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.
This is existing content and might be out of scope of this PR, but we've been avoiding future tense (e.g., will be). I'll leave it up to you if you want to change it. :)
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.
Good catch! I :) updated two instances using future tense in this note.
|
LGTM! Just one optional comment. |
|
/cherrypick enterprise-4.8 |
|
/cherrypick enterprise-4.7 |
|
/cherrypick enterprise-4.6 |
|
/cherrypick enterprise-4.5 |
|
@bobfuru: new pull request created: #30025 DetailsIn response to this:
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. |
|
@bobfuru: new pull request created: #30026 DetailsIn response to this:
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. |
|
@bobfuru: new pull request created: #30027 DetailsIn response to this:
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. |
|
@bobfuru: new pull request created: #30028 DetailsIn response to this:
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. |
BZ1896084
Adds instructions for removing finalizers to support volume snapshot delete operations. Includes note that force option is not sufficient.