Bug 1820192: ask before deleting referenced VM resources

[atiratree]

- split delete device modal into delete disk and nic modal
- ask if referenced volume resource should be deleted
  - on disk removal
  - remove owner reference if delete checkbox is off
- move logic from delete disk modal into a hook
- add DeleteVMLikeEntityModal
- allow freeing up volume resources on VM/Template delete
- add option for cleaning up vm import
- add hooks for vmlike entity and vmimport

[openshift-ci-robot]

@suomiy: This pull request references Bugzilla bug 1820192, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target release (4.5.0) matches configured target release for branch (4.5.0)
• bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1820192: ask before deleting referenced VM resources

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.

[atiratree]

This looks at resources which are referenced through volumes (most notably DataVolume and PVC, but also Secret, Config Map etc.). And investigates if the VM owns the resource, and gives user an option of preserving that resources and separating it from VM lifecycle.

Option of keeping that resource when VM owns the resource (when checkbox deselected)

DV:

Secret

No option when VM doesn't own the resource

Same behaviour when deleting the VM, but has only possibility of selecting the resources in bulk.

when VM owns 2 disks (one DV and one Secret)

No option when VM doesn't own any resource

[atiratree]

Please review @yaacov @pcbailey @glekner @irosenzw

@jelkosz @matthewcarleton

The main problem is that we do not offer such option for cdroms and also for environment tab (less priority).
So the user can't choose that it wants to preserve cdrom resources (for example downloaded URL DV and PVC) when deleting it.

How could we present that with our cdrom UI?

Would it make sense to align it with the disk flow as it allows more features (also bus editing)?

We could use disk filtering which @yaacov added in #5453 and send them to disks tab filtered for CDs only, when the user clicks on CDs editing button in VM details tab

[matthewcarleton]

Hey @suomiy this is awesome! I really think these type of effort is exactly what we need to ensure the user is more informed on their actions, nice work! I have some questions/ comments :)

This looks at resources which are referenced through volumes (most notably DataVolume and PVC, but also Secret, Config Map etc.). And investigates if the VM owns the resource, and gives user an option of preserving that resources and separating it from VM lifecycle.

Option of keeping that resource when VM owns the resource (when checkbox deselected)

DV:

Can we show just the disk name here and not the name of the vm (makes it easier to read and we can assume they already know that it's for this VM b/c they took the action) I think we need to come up with an overall strategy on how we are presenting data volumes and PVCs to the user. When they create a disk they aren't informed of the backing DV/PVC so it could confuse them here. I like adding DV/PVC exposure but I don't think we should do it at the end like this but rather the beginning when they create the Disk.

Secret

What about adding some clarity : This will also delete the example-secret connected to this VM.

No option when VM doesn't own the resource

Can we add some assurance here for the user where we state that it won't be deleted entirely but just from the VM?

Same behaviour when deleting the VM, but has only possibility of selecting the resources in bulk.

when VM owns 2 disks (one DV and one Secret)

I think we could add a bit more clarity here too : Are you sure you want to delete this VM and the following resources:
Disk-1
Disk-2
[] delete vm and listed resources

No option when VM doesn't own any resource

Clarity here could be nice too where the user is aware that the VM is not deleting the resources and they will still be intact.

[atiratree]

Option of keeping that resource when VM owns the resource (when checkbox deselected)

Can we show just the disk name here and not the name of the vm (makes it easier to read and we can assume they already know that it's for this VM b/c they took the action) I think we need to come up with an overall strategy on how we are presenting data volumes and PVCs to the user. When they create a disk they aren't informed of the backing DV/PVC so it could confuse them here. I like adding DV/PVC exposure but I don't think we should do it at the end like this but rather the beginning when they create the Disk.

Nice, added. I like the idea of making this clear to the user at the beginning, but I still think it has a value of emphasizing this when deleting too.

Secret
What about adding some clarity : This will also delete the example-secret connected to this VM.

I think this might indicate a wrong action if the checkbox is deselected.

No option when VM doesn't own the resource

Can we add some assurance here for the user where we state that it won't be deleted entirely but just from the VM?

Clarity here could be nice too where the user is aware that the VM is not deleting the resources and they will still be intact.

It is not that straightforward, because we would have to mention that some disks reference different resource, but some might not. So the VM might reference something, or in case of container image nothing.

when VM owns 2 disks (one DV and one Secret)

I think we could add a bit more clarity here too : Are you sure you want to delete this VM and the following resources:
Disk-1
Disk-2
[] delete vm and listed resources

The VM will be always deleted and is not dependent on the checkbox. We would like to add a list of disks and possibility to pick them in the future.

Currently there is also stale VirtualMachineImport resource marked for deletion. So we have actually 2 checkboxes (forgot to show it) as it is better to have them listed separately.

[matthewcarleton]

I might be thinking of these checkboxes wrong.
Is the idea that we want to ensure the user knows what they are doing so we require a checkbox so they don't do something accidentally OR are we providing additional options WHEN they delete the VM. Maybe it's a mix of both?

[atiratree]

we are providing additional options (what to delete/preserve) when they delete the VM/disk

[yaacov]

@matthewcarleton @suomiy hi,

This looks like a feature, we may want to consider this after 4.5, added notes about this bug in the BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1820192#c8

Un-targeting the BZ from 4.5 (see comment in BZ).
/bugzilla refresh

[openshift-ci-robot]

@yaacov: This pull request references Bugzilla bug 1820192, which is invalid:

• expected the bug to target the "4.5.0" release, but it targets "4.6.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

@matthewcarleton @suomiy hi,

This looks like a feature, we may want to consider this after 4.5, added notes about this bug in the BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1820192#c8

Un-targeting the BZ from 4.5 (see comment in BZ).
/bugzilla refresh

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.

[yaacov]

@mattf @suomiy we may want to add a hint educating users what are data-volumes, why we do not delete them by default, and what are the risks when deleting them with the vm.

[yaacov]

Re-targeting the BZ to 4.5 (see comment in BZ).
/bugzilla refresh

[openshift-ci-robot]

@yaacov: This pull request references Bugzilla bug 1820192, which is valid.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target release (4.5.0) matches configured target release for branch (4.5.0)
• bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Re-targeting the BZ to 4.5 (see comment in BZ).
/bugzilla refresh

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.

[matthewcarleton]

@suomiy ah I think what's confusing me here is the pattern that we see in other places in OCP where the user is required to check the checkbox to ensure they are aware of the destructive action they are about to take.
I think we could help clarify that but having addition text that precedes the checkboxes like:

The following checked items will also be removed (uncheck if you'd prefer to keep them)

We could also have something like
The following items can not be deleted and will persist after this virtual machine is deleted
and then list out what will stay.
WDYT?

[atiratree]

done,

remove 3 owned resources

remove 1 owned resource

remove 0 owned resources

[matthewcarleton]

done,

remove 3 owned resources

remove 1 owned resource

remove 0 owned resources

So when they uncheck items do we change the text above? and if they uncheck them all we remove the text? Or is this just in cases where the modal is shown and only some (or none) of the check boxes are checked?
It would be nice if we just had a singe string of text for the checkboxes regardless of them being checked or not:
Select the resources you'd like to delete with the virtual machine.
Then the options are not checked by default. WDYT? We could then remove "delete" from the checkbox label.

[atiratree]

So when they uncheck items do we change the text above? and if they uncheck them all we remove the text? Or is this just in cases where the modal is shown and only some (or none) of the check boxes are checked?

it is checked by default and we change the text if they get unchecked

t would be nice if we just had a singe string of text for the checkboxes regardless of them being checked or not:
Select the resources you'd like to delete with the virtual machine.
Then the options are not checked by default. WDYT? We could then remove "delete" from the checkbox label.

I think it is preferable to have them checked by default (especially VMImport resource - as they probably will not know what it is for)

For example, oVirt has the same behaviour - has deletion of disks checked by default.

[matthewcarleton]

So when they uncheck items do we change the text above? and if they uncheck them all we remove the text? Or is this just in cases where the modal is shown and only some (or none) of the check boxes are checked?

it is checked by default and we change the text if they get unchecked

t would be nice if we just had a singe string of text for the checkboxes regardless of them being checked or not:
Select the resources you'd like to delete with the virtual machine.
Then the options are not checked by default. WDYT? We could then remove "delete" from the checkbox label.

I think it is preferable to have them checked by default (especially VMImport resource - as they probably will not know what it is for)

Yes I agree, checked by default is the best. I Would say we don't need to change the text string though it could be a bit weird to see it shifting around. I think this string could work:

The following resources will be deleted along with this virtual machine. Unchecked items will not be deleted.

WDYT?

For example, oVirt has the same behaviour - has deletion of disks checked by default.

[atiratree]

ok, done

[yaacov]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: suomiy, yaacov

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:

• frontend/packages/kubevirt-plugin/OWNERS [suomiy,yaacov]

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

[openshift-ci-robot]

@suomiy: All pull requests linked via external trackers have merged: openshift/console#5457. Bugzilla bug 1820192 has been moved to the MODIFIED state.

In response to this:

Bug 1820192: ask before deleting referenced VM resources

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.