[pve-devel] [PATCH qemu-server v2 1/5] add create and remove reboot trigger subs

[Dominik Csapak]

On 9/5/19 12:42 PM, Thomas Lamprecht wrote:
> On 05.09.19 12:27, Dominik Csapak wrote:
>> On 9/5/19 11:47 AM, Thomas Lamprecht wrote:
>>> On 23.08.19 10:55, Dominik Csapak wrote:
>>>> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
>>>
>>>> +    open(my $fh, '>', "/run/qemu-server/$vmid.reboot")
>>>> +    or die "failed to create reboot trigger file: $!\n";
>>>
>>> maybe indentation issue for or
>>
>> what do you mean?
> 
> sorry, misinterpreted the tabulator in my MTA, ignore that
> 
>>
>>      open[..]
>> <tab>or die[..]
>>
>> is our indentation scheme?
>>
>>>
>>>> +    close($fh);
>>>> +}
>>>> +
>>>> +sub remove_reboot_trigger {
>>>
>>> maybe clear_reboot_request?
>>
>> ok
>>
>>>
>>>> +    my ($vmid) = @_;
>>>> +    return unlink("/run/qemu-server/$vmid.reboot");
>>>
>>> FYI, unlink can also fail for other reasons that ENOENT,
>>> which you use as return value, see `man 2 unlink`
>>>
>>> Maybe do a bit more and actively check if the file is here first?
>>> (This will be always racy and thus should only be done in locked
>>> context anyway)
>>>
>>> Or explicitly check if the error was ENOENT ?
>>
>> was also copied from container, but yeah it makes to check if
>> it exists and log an error in case we cannot remove it
> 
> 
> I nowhere saw anything mentioning that this all is just copied over from CT.
> It makes sense that it's there too, but maybe we could add the common code
> then to guest-common? (and fix it for both)

yeah i only briefly mentioned that its inspired by pve-container in 3/5

but yes makes sense to fix it for both and put it into
PVE::GuestHelpers in guest-common
i would also put the trigger/request creation code there

> 
>>
>> it would only reboot it in case we can remove the file though,
>> otherwise we risk that the user cannot shutdown the vm...
>>
> yes, all else is an error, but I would warn and say that reboot was ignored
> because of "blah".
> 
> Another possible alternative, in combination with the separate vm_reboot
> method from my 3/5 review, could be to remove this early in the locked context,
> before doing the stop, and in the case of a error on removal just abort and
> do nothing - then the VM would keep running, maybe a better option?
> As a reboot always has the end goal to have a running machine again, if you
> stop it in case of error the goal is violated, if you do nothing it isn't fully
> violated (as it may still be outdated on pending changes)

but we have to somehow detect a reboot in the qm cleanup step
in which the vm is already stopped so that we can start it again.
i am not sure how this should work if we remove the file before
stopping? or am i missing something here?

> 
> 
>>>
>>>> +}
>>>> +
>>>>    # bash completion helper
>>>>      sub complete_backup_archives {
>>>>
>>>
> 
>