Bug 1320879

Generic problem for all properties. The wrong behavior is only when the VM has some disks

The question is whether is it correct for stateless VM to make changes in VM configuration when VM is running and plan that they will be applied on the next run. Because general rule about stateless VM is that all changes that are made when VM runs (in data on disks or in VM configuration) are erased on VM shutdown. On VM shutdown the VM must be restored exactly to the same state it was in before the VM was started.

Currently in ProcessDownVmCommand we remove the stateless snapshot after applying the NEXT_RUN snapshot. This effectively throws out all NEXT_RUN changes for a stateless VM. If we will remove the stateless snapshot before applying the NEXT_RUN changes, this will cause strange behaviour: if user will change a property that may be applied immediately, it will be erased after VM shutdown (because VM is stateless), but the changes for the next run will be preserved.

From my point of view, the current behaviour for stateless VM is correct. We just need to inform user and explicitly prevent creation of NEXT_RUN snapshot for a stateless VM.

Michal, what do you think?

well, there are the following problems:

- the stateless VM behaves differently if it has a disk and if not (if it has a disk, the next_run snapshot is not applied, if it does not have a disk, it gets applied). In other words: if the stateless VM does not have a disk, the next run behaves like a stateful VM, if not, not

- the stateless VM behaves the same as a stateful if it is down (you can edit and the configuration is persisted) but differently when it is running

Long story short, the statelessness is defined only for disks, not for configuration. We need to handle the VM configuration the same regardless of the stateless flag.

Allowing to modify running stateless VM in a way that these changes apply after the VM goes down raises many questions:

1. What if the VM was committed to snapshot with memory and then ran as stateless?  allowing the user to change its memory and applying it on next run will prevent the memory from being restored on next run (so should we not apply next-run configuration for such VMs?)

2. When user change anything immediately on running stateless VM, even its name, the change is reverted when the VM goes down. Now what if we edit the VM name and its memory and chose to apply the memory change on next run? anyway it is confusing - if the name will be reverted then why the memory is not? if the name remains then we get different result based on the memory?

Taking this kind of questions into consideration and considering that codewise it is better to keep restore stateless VM as a particular case for restore snapshot with no exceptions, I think we should prevent users from setting next-run configuration for stateless VMs (immediate changes are allowed, and they are reverted when the VM goes down).

Regarding the 2 points in comment 3:
- the first sounds like a bug, the handling should be the same for stateless VM with disk(s) and stateless diskless VM.
- the second is not a bug, stateless VM is not read-only. it can be modified while down as regular VM. the 'stateless' only applied to it while running.

(In reply to Arik from comment #4)
> Allowing to modify running stateless VM in a way that these changes apply
> after the VM goes down raises many questions:
> 
> 1. What if the VM was committed to snapshot with memory and then ran as
> stateless?  allowing the user to change its memory and applying it on next
> run will prevent the memory from being restored on next run (so should we
> not apply next-run configuration for such VMs?)

Not sure I follow. Considering you can not create snapshots from stateless VMs, is the flow you mean this?
1: create a non-stateless VM
2: run and create snapshot with memory
3: turn VM off and change to stateless
4: run
5: change memory and apply on next run
6: power off
7: restore snapshot

Is this the flow you mean? In that case I dont understand how is it different from not changing it to stateless in meantime.

> 
> 2. When user change anything immediately on running stateless VM, even its
> name, the change is reverted when the VM goes down. 

yes, and I think that this is a bug. Or at least an undocumented hidden feature introduced by an unrelated feature of next run configuration :)

> Now what if we edit the
> VM name and its memory and chose to apply the memory change on next run?
> anyway it is confusing - if the name will be reverted then why the memory is
> not? if the name remains then we get different result based on the memory?

yes, it should not depend on memory or anything, configuration should behave the same as for stateful.

> 
> Taking this kind of questions into consideration and considering that
> codewise it is better to keep restore stateless VM as a particular case for
> restore snapshot with no exceptions, I think we should prevent users from
> setting next-run configuration for stateless VMs (immediate changes are
> allowed, and they are reverted when the VM goes down).
> 
> Regarding the 2 points in comment 3:
> - the first sounds like a bug, the handling should be the same for stateless
> VM with disk(s) and stateless diskless VM.
> - the second is not a bug, stateless VM is not read-only. it can be modified
> while down as regular VM. the 'stateless' only applied to it while running.

Changing stateless to apply also for VM configuration raises some questions too:
- what to do with the fields which we can not apply immediately and make sense only on next run? This is lots of fields like all kinds of devices, everything boot related, stuff like stateless flag itself, start in pause mode... lots of things. Disallow changing them like for pool? What if I call it from REST? Fail in validate?
- disallow the "apply on next run" for fields which can be hotplugged but do not have to be? How to show this? With some warning? Or?

Taking into account that since always the stateless meant only stateless disks and enriching the meaning of stateless also to configuration is not a small task (especially from UI perspective) and can be seen quite controversial (at least for me it sounds like we have removed a feature rather than added one) it really does not sound like a material for 3.6.6.

yeah and I've forgotten to mention - I think the correct approach for 3.6.6 is to change the behavior so the configuration for stateless VM with disk behaves the same as without disk and also the same as the stateful.

Tomas, lets separate 2 things:
1. Immediate changes - it has not been changed by the next-run feature
2. Next-run changes

For #1, let me explain by example.
Lets say that we have the following snapshot structure for a VM:
             snapshot_id              | snapshot_type | status
--------------------------------------+---------------+--------
 99a2f0a1-0a16-44b1-a10d-f2eab69686fe | ACTIVE        | OK
 0b32816b-f269-48c6-b2cb-545e9b64b39e | REGULAR       | OK
 e3e98b6f-13a3-4cd6-b05d-c314c51db0e3 | REGULAR       | OK

The REGULAR snapshots has VM-configuration attached. Every snapshot has disks state and configuration, except for the ACTIVE snapshot for which the configuration is the one in vm_static table.

Now lets say that we run it as stateless, we will get the following structure:
             snapshot_id              | snapshot_type | status
--------------------------------------+---------------+--------
 99a2f0a1-0a16-44b1-a10d-f2eab69686fe | STATELESS     | OK
 65a2ee5e-71c7-49e6-8e91-77e6b0042c89 | ACTIVE        | OK
 0b32816b-f269-48c6-b2cb-545e9b64b39e | REGULAR       | OK
 e3e98b6f-13a3-4cd6-b05d-c314c51db0e3 | REGULAR       | OK

Again, every snapshot has both disks state and configuration, except for the ACTIVE. So the stateless snapshot (note its snapshot_id, that's the snapshot that was previously ACTIVE) also has configuration attached.
When the VM goes down we apply a generic process to revert to snapshot in which we restore both the disks state and the configuration attached to the snapshot.

So it was always the case that changes that were applied to running stateless VM were reverted when the VM goes down. IMO that is the correct behaviour.

Now let say that we want to break this assumption that a snapshot contains both disks state and configuration, and when reverting the stateless snapshot its configuration will not be restored - 
first, it breaks the generic design.
second, let's say that the stateless snapshot has memory attached. that means that every time the user starts this VM it starts with the same memory state. unless we are willing to break this use-case, we have to restore the vm-configuration so it will match the one that was at the moment the ram snapshot was taken. so for this case we actually have to restore the configuration from the snapshot.

Now regarding #2 - next-run configuration was introduced to allow users to make changes without shutting down the VM. not as a bypass for modifying stateless VMs. stateless VMs are designed in a way that all changes to them, while they are running, are reverted. either it be something on the disks or in their configuration. so in order to prevent confusion, I really think we should prevent next-run changes for running stateless VMs.

hm, it seems to me that the feature was actually designed to work only with disks and than half accidentally evolved to also incorporate the configuration. This is why we are taking stateless snapshots only if the VM has disks with a note in code:
// If there are no snappable disks, there is no meaning for
// running as stateless, log a warning and run normally

But OK, I understand your point and start to agree that the stateless can mean stateless also for configuration. But to make this happen properly, it needs some work. A little to 3.6.6 and some more to 4.x:

in 3.6.6:
- make a stateless snapshot also when the VM has no (snapshot-able) disks
- enrich the documentation (just put a doc text here saying that also the VM configuration will be restored because the documentation is quite explicit that it is about disks)
- add some warning to "pending virtual machine changes" that you are editing a stateless VM and any changes made will be lost anyway, since the label: "Changes that require Virtual Machine restart:" quite implies that they will be applied after restart.

in 4.x (not 4.0):
- it would be good to be able to disable the fields which can not be applied immediately (similarly how the pool dialog changes fields which it can not edit)
- do not allow to apply changes later (since they will anyway not be applied)
- change the "stateless VM with configuration for next run" label when highlighting the vm icon to something like "stateless VM contains changes which will be lost after restart"

What do you think?

OK, after thinking a bit more about it - considering this behavior has been there since 3.5 and it does not break anything, it is only confusing and we don't have spare capacity for 4.0, postponing to 4.1. 
We can decide on scope later on.

The fix for this issue should be included in oVirt 4.1.0 beta 1 released on December 1st. If not included please move back to modified.

So after a long discussion the only actual issue here was that the stateless VM did not have next run snapshot if the VM did not have a disk.

To verify the fix:
- create a diskless stateless VM with description "before"
- run the VM
- when running, edit and change the description to "after"
- turn the VM off
- check the description, should be "before" (e.g. the change has been reverted)

Verify with: 4.1.1.4-0.1.el7

Steps:
- create a diskless stateless VM with description "before"
- run the VM
- when running, edit and change the description to "after"
- turn the VM off
- check the description, should be "before" (e.g. the change has been reverted)

PASS