[pve-devel] applied: [PATCH qemu-server 2/2] migration: add missing use statements

[Fiona Ebner]

Am 23.05.24 um 11:03 schrieb Fabian Grünbichler:
> but slight follow-up question below
> 
> On May 3, 2024 10:34 am, Fiona Ebner wrote:
>> +use PVE::Storage::Plugin;
> 
> this one is only used for a single check_connection call in `prepare`:
> 
> 	if ($scfg->{shared}) {
> 	    # PVE::Storage::activate_storage checks this for non-shared storages
> 	    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> 	    warn "Used shared storage '$sid' is not online on source node!\n"
> 		if !$plugin->check_connection($sid, $scfg);
> 	}
> 
> can't we get rid of this? either we are live-migrating, then the storage
> is already active and in use. or we are migrating offline, then shared
> storages are irrelevant (unless it's a remote migration, but then we
> will activate the storage anyhow)?
> 

This warning check was introduced by

qemu-server: 73f5ee92 ("fix #971: don't activate shared storage in
offline migration")

pve-container: 20ab40f ("fix #971: don't activate shared storage in
offline migration")

I experimented a bit what happens when you live-migrate when QEMU cannot
talk to an NFS volume. Migration will fail with error messages on the
target side:

> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: Failed to load virtio_pci/modern_queue_state:used
> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: Failed to load virtio_pci/modern_state:vqs
> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: Failed to load virtio/extra_state:extra_state
> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: Failed to load virtio-blk:virtio
> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: error while loading state for instance 0x0 of device '0000:00:0a.0/virtio-blk'
> May 28 09:30:27 pve8a1 QEMU[46691]: kvm: load of migration failed: Input/output error

I'd guess, because the source side never gets to write the state for the
disk.

However, I can imagine there are cases where our storage layer's
check_connection/activate would fail on the source side, but work on the
target side and QEMU still can talk to the volume. Then the warning
might even be more confusing than helpful.

> or, couldn't we replace it with an eval-ed call to activate_storage for
> the same effect (to get a warning if the storage is somehow broken, even
> though it is not a pre-requisite for migration)?
> 

Both variants are fine by me. The warning is good to have in case of
hard-to-understand errors like mentioned above, but even then not super
useful IMHO, so I'd slightly prefer removal.

> I'm currently trying to eliminate direct calls to plugin code (i.e., not
> via PVE::Storage itself as entry point), and this seems like low-hanging
> fruit ;) the same also exists in pve-container for migration..
> 

For containers, the call is even guarded by !$remote additionally ;) And
there is no live-migration, so I think the warning is even less useful.