machines: Make readonly and shareable disk options configurable

[skobyda]

Allow multiple running VMs share a single disk.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1684304
Closes #12314

[skobyda]

Only disks of format 'raw' have shareable option:

Changes takes effect only after shutting down the VM:

Disks which are not persistent (e.g. 3rd disk in row) do not have configurable options:

[KKoukiou]

This affects the design of the disk row as well, @garrett are you good with this change?

[garrett]

1. We shouldn't have checkboxes without obvious labels.
2. When would someone want read-only disks? Would they ordinarily toggle them? Is it changeable on the fly or only after a reboot? If it's possible to change a disk from RO to RW (or vice versa) when running, would things prevent that? Are errors handled, if so?
3. What does "Sharable" mean?
4. The values under "source" are oddly aligned.
5. There's too much space on the [-] buttons. (I can look into and fix this. It's probably a bit of CSS.)

[garrett]

I think we need to rework the design. (See my previous comment for questions that need answering.)

[skobyda]

1. We shouldn't have checkboxes without obvious labels.

I cannot think of sensible labels.
For readonly, what about: "Forbid/allow modification of disk" ?
For shareable: "Available for sharing by multible VMs"?

2. When would someone want read-only disks?

Usually when user wants multiple VMs to use the same disk. To prevent write conflicts you set this disk as readonly for some VMs, so they loose write access. Or disks which you want to act as "CDrom" or other RO disk types.

Would they ordinarily toggle them?

Again, mainly for specific use cases such as multiple VMs share disk, or to simulate readonly disk type.

Is it changeable on the fly or only after a reboot? If it's possible to change a disk from RO to RW (or vice versa) when running, would things prevent that? Are errors handled, if so?

No, this can be only updated after a reboot. You can modify readonly/shareable options only for permanent VM's XML, which will be loaded with all the new modifications after VM reboots. I forgot to take into account error handling, good idea. (altought I can't think of any error apart from some undocumented libvirt/qemu internal error).

3. What does "Sharable" mean?

Again, used when you want to share a disk with multiple VMs, but in this case you want all of them to have a write access. From what I can read in documentation, it will turn off I/O caching, locking of the disk which will make disk available for modification by multiple VMs. It's mainly used for cluster-aware guests.

4. The values under "source" are oddly aligned.
5. There's too much space on the [-] buttons. (I can look into and fix this. It's probably a bit of CSS.)

Both of these things are not affected by this PR.

[skobyda]

@garrett After our IRC discussion to pull readonly and shareable configuration into separate disk Edit dialog, here is the first take of how it could look:

Alternatively checkboxes can be replaced by dropdown with options (Read-only / Read-write) and (Shareable / Non-shareable).
@KKoukiou What do you think about introducing Edit Disk dialog?

[garrett]

@skobyda: Cool! Thanks for working on splitting the state changing into a dialog.

1. Are we going to have additional ways to edit the disk in the future?
2. How are we editing the disk? "Edit disk" might not be specific enough. (On the other hand, we want to avoid saying things like "mount" in the UI.) "Edit disk settings" or "Edit disk state" perhaps? 🤷‍♂️
3. Which disk is this? There's no indication what is being edited. (We should add the name to the title or above the checkboxes.)
4. Can you share read-write disks?
5. "Readonly" should be "Read only" or "Read-only".
6. Will this be instant-apply, or will a reboot have to happen for this to take effect? We should say if a reboot is needed. 😉
7. Is there an indication that a reboot is needed for this to take effect after the disk mount settings have been applied?

[skobyda]

1. Are we going to have additional ways to edit the disk in the future?

Yes, you can edit stuff like Bus, storage format and several performance options.

2. How are we editing the disk? "Edit disk" might not be specific enough. (On the other hand, we want to avoid saying things like "mount" in the UI.) "Edit disk settings" or "Edit disk state" perhaps? man_shrugging
3. Which disk is this? There's no indication what is being edited. (We should add the name to the title or above the checkboxes.)

Disk is uniquely specified by it's path. So dialog name could be something like "Edit /var/lib/libvirt/images/fedora30.qcow2 Disk Settings".

4. Can you share read-write disks?

Yes. You can attach disk which is already used by another VM without setting readonly or shareable. Then however if you will try to run both VMs, you will have problem (e.g. write conflict). To prevent that, you have 2 solutions: make disk readonly for others VMs, so there will be no write conflict, or make disk shareable so libvirt/qemu will prevent conflicts by using shared file locks, turning I/O caching off...

5. "Readonly" should be "Read only" or "Read-only".

Agree.

6. Will this be instant-apply, or will a reboot have to happen for this to take effect? We should say if a reboot is needed. wink

It wil happen only after reboot. I will put warning at the dialog footer similar to Vcpu or Network Interface Edit dialogs.

7. Is there an indication that a reboot is needed for this to take effect after the disk mount settings have been applied?
   Yes, the warning will show in disks table next to property which value will change after reboot.

[garrett]

What if the dialog would be something like this:

Edit disk state	×
Name	Disk-123
Size	6GB
State	● Read & write ○ Read-only ○ Read-only and shared

Name and size are not editable (at least in this iteration). They're in the dialog for reference, to make sure someone knows what they're editing.

State is a group of 3 radio buttons.

If "read-only and shared" is selected, then perhaps there should be a warning message about how this makes the disk read-only until it is removed from other VMs. It would also be good to know which VMs are using the disk (from somewhere, within Cockpit).

The × is centered because this is a limitation of doing mockups in tables in Markdown on GitHub. Same for other alignment issues. 😉

What do you think?

[marusak]

Needs to rebase to master since #12367 changed tests names

[skobyda]

@garrett Current state of dialog:

Also as you requested I:

• moved alignment of the action buttons to the right
• replaced words yes/no -> Read-Only/Read-Write or Shareable/Not Shareable
• And also changed the [-] button into [Detach], so it fits more nicely together with [Edit] Button.

Now:

Before:

[skobyda]

Tests seem to be green. It also seems there are no more design objections.
@KKoukiou once you have some free time, may I ask for code review?

[KKoukiou]

I have some minor comments, but no need to block on these for these to get merged
Retriggering for testCreate on rhel-8-1 to get green.

Outdated

[skobyda]

Screenshots for release notes: