Re: handling Hyper-V automatic startup values

2020-09-28 Thread Matt Coleman
> The Go code represents it as a boolean for example.
> 
> I think our only option is to define a new API
> 
> virDomainSetAutostartPolicy()
> 
> and make it take an int parameter for which we can define an enum.
> 
> The existing API will just have to map new values onto true/false
> as appropriate.
That’s the same conclusion I came to. As with the version numbers, I’ll 
continue implementing functions within the existing API before I 
attempt to add anything new to it.

> I think it helps to consider the use case where someone might pick to
> set  "Restart if previously active".
> 
> Consider if you have 2 hosts, and the same VM config is present on
> both hosts. You only want the VM running on one host at any point
> in time.  In this case you do *not* want to use "always start" as
> if both hosts reboot, you'd get two copies of the VM.
> 
> "Restart if previously active" solves this scenario by only starting
> it on the host it was originally running on.
> 
> With this in mind, making libvirt treat "restart if previously active"
> the same as "always start" is a dangerous configuration. The safe option
> is to consider "restart if previously active" the same as "none".
> 
> So I think values  2 and 3 should map to autostart disabled and
> value 4 to autostart enabled.
I hadn’t considered things like high availability setups. I’ll update my 
code to only treat 4 as enabled.

Thanks!
Matt




Re: handling Hyper-V automatic startup values

2020-09-28 Thread Daniel P . Berrangé
On Wed, Sep 23, 2020 at 07:06:01PM -0400, Matt Coleman wrote:
> Hello,
> 
> I’m implementing some new functionality in the Hyper-V driver and
> could use some input on how I should handle automatic startup values.
> 
> Microsoft’s Msvm_VirtualSystemSettingData class stores this setting
> in a property named AutomaticStartupAction:
> https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/msvm-virtualsystemsettingdata
> 
> This property has several possible values with different meanings:
> * 2 means “None.”, which represents that the VM will not automatically
> start at boot. This corresponds to libvirt’s domainGetAutostart
> outputting boolean false.
> * 3 means “Restart if previously active.” This will start the machine
> at boot if it was running when the host was shut down or unexpectedly
> powered off. It appears libvirt does not have a way to represent this.
> * 4 means “Always start.” This corresponds to libvirt’s domainGetAutostart
> outputting boolean true.
> * 5 through 32768 are reserved.
> 
> I’m unsure how to handle the value 3, since libvirt treats this setting as
> a boolean...
> 
> The domainGetAutostart function places the value in an int:
> * 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L1498
> * 
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L461-463
> 
> However, virDomainGetAutostart’s docblock says that it will be treated as a 
> boolean:
> https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt-domain.c#L6690-6729
> 
> Its usage in `virsh dominfo` confirms that:
> https://gitlab.com/libvirt/libvirt/-/blob/master/tools/virsh-domain-monitor.c#L1354-1355
> 
> I haven’t investigated how other languages' bindings treat the field.

The Go code represents it as a boolean for example.

I think our only option is to define a new API

  virDomainSetAutostartPolicy()

and make it take an int parameter for which we can define an enum.

The existing API will just have to map new values onto true/false
as appropriate.

> Currently, my code treats anything over 2 as autostart being enabled
> (although, perhaps I should ignore 5+). I feel like that pretty closely
> represents the VM’s configuration, since it will autostart in certain
> cases and it definitely isn’t configured to never autostart.
> 
> For 3 (“Restart if previously active.”), it could be argued that libvirt
> should only say that autostart is enabled when the VM is running. This
> would more closely represent what the VM’s runtime behavior will be,
> since `virsh list —autostart` would show you which VMs would boot if
> the hypervisor rebooted at that point in time. However, it could be
> considered confusing because the VM’s configuration would appear to
> change depending on whether or not it was running.
> 
> Ultimately, it seems like libvirt’s concept of autostart functionality
> needs to be extended. Along those lines, it was pointed out on IRC that
> libvirt lacks the ability to represent hypervisors’ host shutdown activities:
> > danpb: we should introduce a .. lifecycle
>  action in the XML to express what actions to take on host shutdown
>  and one of those actions can be “stop-and-restart-on-next-boot”
> 
> 
> Hyper-V has a separate setting, AutomaticShutdownAction, which governs the 
> actions taken when the host stops:
> * 2 means “Turn off.” This abruptly powers off the VM.
> * 3 means “Save state.” This saves the running VM’s state to disk.
> * 4 means “Shutdown.” The performs a clean shutdown of the VM.
> * 5 through 32768 are reserved.
> 
> It seems to me that the two settings complement each other but cover
> different functionality. I feel like 3 (“Restart if previously active.”)
> is more closely related to startup activities than the host stopping.

Yep, I think we'll need to treat them separately.

> 
> Since it seems like more pervasive changes are needed, I’d prefer to
> commit something “good enough” for the Hyper-V driver and then refine
> it along with the rest of the drivers.
> 
> Should I make domainGetAutostart only output true for Hyper-V VMs
> configured with AutomaticStartupAction=3 when they are running? Or,
> should I leave it as I currently have it (2 = autostart is disabled;
> 3+ = autostart is enabled)?

I think it helps to consider the use case where someone might pick to
set  "Restart if previously active".

Consider if you have 2 hosts, and the same VM config is present on
both hosts. You only want the VM running on one host at any point
in time.  In this case you do *not* want to use "always start" as
if both hosts reboot, you'd get two copies of the VM.

"Restart if previously active" solves this scenario by only starting
it on the host it was originally running on.

With this in mind, making libvirt treat "restart if previously active"
the same as "always start" is a dangerous configuration. The safe option
is to consider "restart if previously active" the same as "none".

So I think values  

handling Hyper-V automatic startup values

2020-09-23 Thread Matt Coleman
Hello,

I’m implementing some new functionality in the Hyper-V driver and could use 
some input on how I should handle automatic startup values.

Microsoft’s Msvm_VirtualSystemSettingData class stores this setting in a 
property named AutomaticStartupAction:
https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/msvm-virtualsystemsettingdata

This property has several possible values with different meanings:
* 2 means “None.”, which represents that the VM will not automatically start at 
boot. This corresponds to libvirt’s domainGetAutostart outputting boolean false.
* 3 means “Restart if previously active.” This will start the machine at boot 
if it was running when the host was shut down or unexpectedly powered off. It 
appears libvirt does not have a way to represent this.
* 4 means “Always start.” This corresponds to libvirt’s domainGetAutostart 
outputting boolean true.
* 5 through 32768 are reserved.

I’m unsure how to handle the value 3, since libvirt treats this setting as a 
boolean...

The domainGetAutostart function places the value in an int:
* https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L1498
* 
https://gitlab.com/libvirt/libvirt/-/blob/master/src/driver-hypervisor.h#L461-463

However, virDomainGetAutostart’s docblock says that it will be treated as a 
boolean:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/libvirt-domain.c#L6690-6729

Its usage in `virsh dominfo` confirms that:
https://gitlab.com/libvirt/libvirt/-/blob/master/tools/virsh-domain-monitor.c#L1354-1355

I haven’t investigated how other languages' bindings treat the field.

Currently, my code treats anything over 2 as autostart being enabled (although, 
perhaps I should ignore 5+). I feel like that pretty closely represents the 
VM’s configuration, since it will autostart in certain cases and it definitely 
isn’t configured to never autostart.

For 3 (“Restart if previously active.”), it could be argued that libvirt should 
only say that autostart is enabled when the VM is running. This would more 
closely represent what the VM’s runtime behavior will be, since `virsh list 
—autostart` would show you which VMs would boot if the hypervisor rebooted at 
that point in time. However, it could be considered confusing because the VM’s 
configuration would appear to change depending on whether or not it was running.

Ultimately, it seems like libvirt’s concept of autostart functionality needs to 
be extended. Along those lines, it was pointed out on IRC that libvirt lacks 
the ability to represent hypervisors’ host shutdown activities:
> danpb: we should introduce a .. lifecycle action 
> in the XML to express what actions to take on host shutdown and one of those 
> actions can be “stop-and-restart-on-next-boot”


Hyper-V has a separate setting, AutomaticShutdownAction, which governs the 
actions taken when the host stops:
* 2 means “Turn off.” This abruptly powers off the VM.
* 3 means “Save state.” This saves the running VM’s state to disk.
* 4 means “Shutdown.” The performs a clean shutdown of the VM.
* 5 through 32768 are reserved.

It seems to me that the two settings complement each other but cover different 
functionality. I feel like 3 (“Restart if previously active.”) is more closely 
related to startup activities than the host stopping.

Since it seems like more pervasive changes are needed, I’d prefer to commit 
something “good enough” for the Hyper-V driver and then refine it along with 
the rest of the drivers.

Should I make domainGetAutostart only output true for Hyper-V VMs configured 
with AutomaticStartupAction=3 when they are running? Or, should I leave it as I 
currently have it (2 = autostart is disabled; 3+ = autostart is enabled)?

Thanks for your time and input, and apologies for how long this got.
Matt