Re: [PATCH qemu-server] fix #7786: migrate: deactivate shared storage volumes of offline VMs

[Daniel Herzig <d.herzig@proxmox.com>]

"Daniel Kral" <d.kral@proxmox.com> writes:

>>>
>>> As discussed off-list, I wonder whether this can be a
>>>
>>>     if (!$self->{running}) {
>>>         my $vollist = PVE::QemuServer::get_vm_volumes($conf);
>>>         PVE::Storage::deactivate_volumes($self->{storecfg}, $vollist);
>>>     }
>>>
>>> Just to be sure that there are no active volumes in the case of a
>>> offline migration.
>>>
>> I guess a call to `deactivate_volumes` without restricting to shared
>> storage (as in `deactivate_volumes_on_shared_storage()`) would also
>> do. But in such a case we won't have complications anyway, as
>> `sync_offline_local_volumes` is taking care of these with its own
>> call to `deactivate_volumes` in the end.
>>
>> I'm not against shortening down the code (will see what I can
>> do) -- but I'd like to keep the `eval->error->log` sequence and not
>> bloat `phase1` at the same time, as it's such a lean sub.
>>
>
> No hard feelings from my side, just thought that it might not hurt here
> as with an offline migration it seems natural that the _local_ volumes
> on the source node are deactivated with or without shared storage.
>
> But you're right, sync_offline_local_volumes() already does handle that
> for some volumes.
>
>>> I wonder whether there are any assumptions in later migration steps
>>> where this might break something that I'm not aware of?
>>
>> I'll double-check, but for now I think the issue is, that we
>> currently don't do anything with the activation state in case of an
>> offline migration on shared storage. Clearly the `if (!self->{running})`
>> check is important, as it would cause issues with online-migrations,
>> where volume deactivation is handled in a dedicated step.
>>
>
> Yeah, it might be worth to investigate if we can reduce the amount of
> time volumes are active when these are needed, e.g. after the disk
> resize, clone, and disk move without deleting the original volume.

I second on that. A definite deactivation of volumes on shared storage
in the end of a migration should be in place for both offline and online
migrations in any case, as we cannot exclude user-activated volumes
either (which would cause the same pattern). We're actually already
doing this 'sanity-check' for containers, but I guess there we already
had more focus on it, as everything's about offline migrations with
containers.

>
>>>
>>>> +
>>>>      $self->phase1_remote($vmid) if $self->{opts}->{remote};
>>>>  }
>>>>