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

["Daniel Kral" <d.kral@proxmox.com>]

On Thu Jul 9, 2026 at 3:30 PM CEST, Daniel Herzig wrote:
> 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.
>

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.

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