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

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

Thanks for looking into this!

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

>> @@ -846,6 +866,13 @@ sub phase1 {
>>      $self->handle_replication($vmid);
>>  
>>      $self->sync_offline_local_volumes();
>> +
>> +    # make sure that volumes of non-running VMs on shared storage
>> +    # are deactivated.
>> +    if (! $self->{running}) {
>> +        $self->deactivate_volumes_on_shared_storage();
>> +    }
>
> 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.

> 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.

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