CNV#50738: Doc: Update downstream doc Delete Protection for VMs #90981
Conversation
openshift-ci
bot
added
the
size/S
openshift-ci
bot
added
size/M
aspauldi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@aspauldi: This pull request references HPUX-549 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
aspauldi
changed the title
|
@aspauldi: This pull request references CNV-45952 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.19.0" version, but no target version was set. In 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 openshift-eng/jira-lifecycle-plugin repository. |
aspauldi
changed the title
openshift-ci-robot
removed
the
jira/valid-reference
|
@aspauldi: No Jira issue is referenced in the title of this pull request. In 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 openshift-eng/jira-lifecycle-plugin repository. |
aspauldi
changed the title
There was a problem hiding this comment.
As general comment, I would state that the VM delete protection is off by default or is an opt-in feature.
Also, I think we should include a section of opt-out VM delete protection entirely. In other words, how can cluster admins can prevent cluster users to enable this feature in their deployments.
virt/managing_vms/virt-enabling-disabling-vm-delete-protection.adoc
Outdated
Show resolved
Hide resolved
aspauldi
changed the title
openshift-ci
bot
added
size/L
| // * virt/managing-vms/virt-enabling-disabling-vm-delete-protection.adoc | ||
|
|
||
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-removing-vm-delete-protection{context}"] |
There was a problem hiding this comment.
🤖 [error] OpenShiftAsciiDoc.IdHasContextVariable: ID is missing the '_{context}' variable at the end of the ID.
|
|
||
| = Removing the virtual machine delete protection option | ||
|
|
||
| When you enable delete protection on a VM (virtual machine), you ensure that the VM cannot be inadvertently deleted. You can also choose to disable the protection for a VM. |
There was a problem hiding this comment.
🤖 [error] RedHat.Spacing: Keep one space between words in 'deleted. You'. For more information, see RedHat.Spacing.
| + | ||
| .Example configuration file | ||
| [source,yaml] | ||
| ---- |
There was a problem hiding this comment.
🤖 [error] AsciiDoc.ValidCodeBlocks: Unterminated listing block found in file.
There was a problem hiding this comment.
When I lint this YAML file, I get the following error:
Missing closing "quote at line 17, column 170
Implicit keys need to be on a single line at line 18, column 5
Looks like it could use a check-over.
There was a problem hiding this comment.
Checking again with Javier. The YAML file had an issue earlier, and Javier provide a new file. Not sure what the issue is now.
There was a problem hiding this comment.
Let's try like this now:
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingAdmissionPolicy
metadata:
name: "disable-vm-delete-protection"
spec:
failurePolicy: Fail
matchConstraints:
resourceRules:
- apiGroups: ["kubevirt.io"]
apiVersions: ["*"]
operations: ["UPDATE", "CREATE"]
resources: ["virtualmachines"]
variables:
- expression: string('kubevirt.io/vm-delete-protection')
name: vmDeleteProtectionLabel
validations:
- expression: >-
!has(object.metadata.labels) ||
!object.metadata.labels.exists(label, label == variables.vmDeleteProtectionLabel) ||
has(oldObject.metadata.labels) &&
oldObject.metadata.labels.exists(label, label == variables.vmDeleteProtectionLabel)
message: "Virtual Machine delete protection feature is disabled"| ---- | ||
|
|
||
|
|
||
| apiVersion: admissionregistration.k8s.io/v1 |
There was a problem hiding this comment.
🤖 [error] RedHat.TermsErrors: Use 'Kubernetes' rather than 'k8s'. For more information, see RedHat.TermsErrors.
There was a problem hiding this comment.
CNV docs use k8s all over the place, so no change made here.
There was a problem hiding this comment.
Hmm, @aireilly is that a new bug in CI? I've never seen this pop up as an error in the apiVersion field before.
There was a problem hiding this comment.
|
|
||
| = Enabling or disabling virtual machine delete protection by using the command line | ||
|
|
||
| To prevent the inadvertent deletion of a virtual machine, you can enable virtual machine delete protection by using the command line. The feature is disabled by default. |
There was a problem hiding this comment.
| To prevent the inadvertent deletion of a virtual machine, you can enable virtual machine delete protection by using the command line. The feature is disabled by default. | |
| To prevent the inadvertent deletion of a virtual machine, you can enable virtual machine delete protection by using the command line. Virtual machines are not delete protected by default. |
Feel free to ignore this suggestion. But I feel that this may lead to think that you need to enable the feature cluster-wide and then enable the protection for each vm.
|
|
||
| = Enabling or disabling virtual machine delete protection by using the web console | ||
|
|
||
| To prevent the inadvertent deletion of a virtual machine, you can enable virtual machine delete protection by using the {product-title} web console. The feature is disabled by default. |
|
Thanks for all effort @aspauldi! |
|
/label peer-review-in-progress |
openshift-ci
bot
added
the
peer-review-in-progress
|
|
||
| When you enable delete protection on a VM (virtual machine), you ensure that the VM cannot be inadvertently deleted. You can also choose to disable the protection for a VM. | ||
|
|
||
| As a cluster administrator, you can choose not to make the VM delete protection option available. VMs with delete protection already enabled retain that setting; for any new VMs that are created, enabling the option is not allowed. |
There was a problem hiding this comment.
"choose not to make" sounds a little wordy. Maybe something like this?
Also a suggestion for the second sentence.
| As a cluster administrator, you can choose not to make the VM delete protection option available. VMs with delete protection already enabled retain that setting; for any new VMs that are created, enabling the option is not allowed. | |
| As a cluster administrator, you can remove the VM delete protection option completely. VMs with delete protection already enabled retain that setting; any new VMs that are created do not have delete protection as an option. |
There was a problem hiding this comment.
@aspauldi Looks like this comment was missed.
| validationActions: [Deny] | ||
| matchResources: | ||
| ---- | ||
|
|
There was a problem hiding this comment.
This procedure only creates the YAML files - it doesn't actually apply them to the cluster. I'd recommend adding two more steps for applying them.
Here's an example of another procedure that does this: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html-single/security_and_compliance/index#installing-nbde-tang-server-operator-using-cli_installing-nbde-tang-server-operator
There was a problem hiding this comment.
@jcanocan and @geetikakay, the documentation peer reviewer Andrea Hoffer recommended adding two steps to this procedure. Can you please take a look and see if what I have added is correct? Thanks! The procedure is documented here: https://90981--ocpdocs-pr.netlify.app/openshift-enterprise/latest/virt/managing_vms/virt-enabling-disabling-vm-delete-protection#virt-removing-vm-delete-protection_virt-enabling-disabling-vm-delete-protection
There was a problem hiding this comment.
@aspauldi Sure, I will review again. But I have a quick question:
When enabling vm-delete-protection, we patch the VM with a label like this:
-- enable labels
$ oc patch vm <vm_name> --type merge -p '{"metadata":{"labels":{"kubevirt.io/vm-delete-protection":"True"}}}' -n
To disable it, there can be two approaches:
- Set the label value to "False"
$ oc patch vm <vm_name> --type merge -p '{"metadata":{"labels":{"kubevirt.io/vm-delete-protection":"False"}}}' -n
- Remove the label entirely
$ oc patch vm <vm_name> --type json -p '[{"op": "remove", "path": "/metadata/labels/kubevirt.io~1vm-delete-protection"}]' -n
Should we treat disabling (setting to "False") and removing the label as two different cases? Technically, both result in the same outcomethe protection is no longer enforced. But from a documentation or behavior clarity perspective, does it make sense to distinguish between them?
cc @jcanocan
There was a problem hiding this comment.
Hi @geetikakay, I think that we can keep the procedure just to one method to make it easier for the users. Thanks!
There was a problem hiding this comment.
@bergerhoffer's suggestion sounds fine to me! Great idea.
@geetikakay's suggestion makes sense. However, I have chosen the approach of removing the label to keep the final yaml manifest as sort as possible. I tend to agree with @aspauldi. Thanks for the suggestion, nevertheless.
There was a problem hiding this comment.
Looks like this was done, as I see an oc apply here. Is this resolved?
virt/managing_vms/virt-enabling-disabling-vm-delete-protection.adoc
Outdated
Show resolved
Hide resolved
|
/label peer-review-done |
openshift-ci
bot
added
peer-review-done
aspauldi
force-pushed
the
CNV-50738
branch
2 times, most recently
from
1860b47 to
27fce7c
Compare
openshift-ci
bot
added
the
merge-review-needed
|
@geetikakay, can you confirm that you approve this PR? Thanks. |
ShaunaDiaz
removed
the
merge-review-needed
|
|
||
| When you enable delete protection on a VM (virtual machine), you ensure that the VM cannot be inadvertently deleted. You can also choose to disable the protection for a VM. | ||
|
|
||
| As a cluster administrator, you can choose not to make the VM delete protection option available. VMs with delete protection already enabled retain that setting; for any new VMs that are created, enabling the option is not allowed. |
There was a problem hiding this comment.
@aspauldi Looks like this comment was missed.
| + | ||
| .Example configuration file | ||
| [source,yaml] | ||
| ---- |
There was a problem hiding this comment.
When I lint this YAML file, I get the following error:
Missing closing "quote at line 17, column 170
Implicit keys need to be on a single line at line 18, column 5
Looks like it could use a check-over.
| validationActions: [Deny] | ||
| matchResources: | ||
| ---- | ||
|
|
There was a problem hiding this comment.
Looks like this was done, as I see an oc apply here. Is this resolved?
|
lgtm |
|
@ShaunaDiaz, can you take another look? I believe all the merge review comments have been resolved. Thanks! |
|
|
||
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-enabling-disabling-vm-delete-protection-cli_{context}"] | ||
|
|
There was a problem hiding this comment.
delete extra line
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-enabling-disabling-vm-delete-protection-cli_{context}"] | ||
|
|
||
| = Enabling or disabling virtual machine delete protection by using the command line |
There was a problem hiding this comment.
| = Enabling or disabling virtual machine delete protection by using the command line | |
| = Enabling or disabling VM delete protection by using the command line |
I think you can get away with VM in the headline since you define it right away in text. Be more scannable for users.
|
|
||
| = Enabling or disabling virtual machine delete protection by using the command line | ||
|
|
||
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable virtual machine delete protection by using the command line. You can also disable delete protection for a VM. |
There was a problem hiding this comment.
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable virtual machine delete protection by using the command line. You can also disable delete protection for a VM. | |
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable VM delete protection by using the command line. You can also disable delete protection for a VM. |
| // * virt/managing-vms/virt-enabling-disabling-vm-delete-protection.adoc | ||
|
|
||
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-enabling-disabling-vm-delete-protection-cli_{context}"] |
There was a problem hiding this comment.
IMHO these should be two separate one-command procedures, not a single one. Probably want to modularize these some time later prior to DITA migration.
There was a problem hiding this comment.
I can do that in another PR; thanks.
|
|
||
| .Prerequisites | ||
|
|
||
| * You have installed the {oc-first}. |
There was a problem hiding this comment.
You also need to know the VMs you want to protect or allow to be deleted, right?
There was a problem hiding this comment.
Andrea had me remove that prereq in her peer review.
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-enabling-disabling-vm-delete-protection-web_{context}"] | ||
|
|
||
| = Enabling or disabling virtual machine delete protection by using the web console |
There was a problem hiding this comment.
| = Enabling or disabling virtual machine delete protection by using the web console | |
| = Enabling or disabling VM delete protection by using the web console |
|
|
||
| = Enabling or disabling virtual machine delete protection by using the web console | ||
|
|
||
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable virtual machine delete protection by using the {product-title} web console. You can also disable delete protection for a VM. |
There was a problem hiding this comment.
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable virtual machine delete protection by using the {product-title} web console. You can also disable delete protection for a VM. | |
| To prevent the inadvertent deletion of a virtual machine (VM), you can enable VM delete protection by using the {product-title} web console. You can also disable delete protection for a VM. |
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-removing-vm-delete-protection_{context}"] | ||
|
|
||
| = Removing the virtual machine delete protection option |
There was a problem hiding this comment.
| = Removing the virtual machine delete protection option | |
| = Removing the VM delete protection option |
|
|
||
| :_mod-docs-content-type: PROCEDURE | ||
| [id="virt-removing-vm-delete-protection_{context}"] | ||
|
|
openshift-ci
bot
added
the
merge-review-needed
|
@aspauldi: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/label merge-review-needed |
jab-rh
added
merge-review-in-progress
|
/cherry-pick enterprise-4.19 |
|
@jab-rh: new pull request created: #92505 In 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-sigs/prow repository. |
Version(s): 4.19+
Issues: CNV-50738 and CNV-45952
Links to docs preview:
QE review:
Additional information: This PR covers both the CLI (CNV-50738) and the web console (CNV-49592) implementations of the VM delete protection feature. SMEs Javier Cano Cano and Phillip Rhodes and QE engineers Leon Kladnitsky and Geetika Kapoor have all approved this PR.