[pve-devel] [RFC qemu-server] migration: nbd export: switch away from deprecated QMP command

[pve-devel] [RFC qemu-server] migration: nbd export: switch away from deprecated QMP command

[Fiona Ebner]

The 'nbd-server-add' QMP command has been deprecated since QEMU 5.2 in
favor of a more general 'block-export-add'.

When using 'nbd-server-add', QEMU internally converts the parameters
and calls blk_exp_add() which is also used by 'block-export-add'. It
does one more thing, namely calling nbd_export_set_on_eject_blk() to
auto-remove the export from the server when the backing drive goes
away. But that behavior is not needed in our case, stopping the NBD
server removes the exports anyways.

It was checked with a debugger that the parameters to blk_exp_add()
are still the same after this change. Well, the block node names are
autogenerated and not consistent across invocations.

The alternative to using 'query-block' would be specifying a
predictable 'node-name' for our '-drive' commandline. It's not that
difficult for this use case, but in general one needs to be careful
(e.g. it can't be specified for an empty CD drive, but would need to
be set when inserting a CD later). Querying the actual 'node-name'
seemed a bit more future-proof.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

RFC, because I'm not sure which approach is better.

 PVE/QemuServer.pm            | 17 ++++++++++++++++-
 test/MigrationTest/QmMock.pm |  4 +++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index a52a883e..07ee2d67 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5969,10 +5969,25 @@ sub vm_start_nolock {
 	    $migrate_storage_uri = "nbd:${localip}:${storage_migrate_port}";
 	}
 
+	my $block_info = mon_cmd($vmid, "query-block");
+	$block_info = { map { $_->{device} => $_ } $block_info->@* };
+
 	foreach my $opt (sort keys %$nbd) {
 	    my $drivestr = $nbd->{$opt}->{drivestr};
 	    my $volid = $nbd->{$opt}->{volid};
-	    mon_cmd($vmid, "nbd-server-add", device => "drive-$opt", writable => JSON::true );
+
+	    my $block_node = $block_info->{"drive-$opt"}->{inserted}->{'node-name'};
+
+	    mon_cmd(
+		$vmid,
+		"block-export-add",
+		id => "drive-$opt",
+		'node-name' => $block_node,
+		writable => JSON::true,
+		type => "nbd",
+		name => "drive-$opt", # NBD export name
+	    );
+
 	    my $nbd_uri = "$migrate_storage_uri:exportname=drive-$opt";
 	    print "storage migration listens on $nbd_uri volume:$drivestr\n";
 	    print "re-using replicated volume: $opt - $volid\n"
diff --git a/test/MigrationTest/QmMock.pm b/test/MigrationTest/QmMock.pm
index 2d5d5c6c..e34686e3 100644
--- a/test/MigrationTest/QmMock.pm
+++ b/test/MigrationTest/QmMock.pm
@@ -103,8 +103,10 @@ $MigrationTest::Shared::qemu_server_module->mock(
 
 	if ($command eq 'nbd-server-start') {
 	    return;
-	} elsif ($command eq 'nbd-server-add') {
+	} elsif ($command eq 'block-export-add') {
 	    return;
+	} elsif ($command eq 'query-block') {
+	    return [];
 	} elsif ($command eq 'qom-set') {
 	    return;
 	}
-- 
2.30.2

Re: [pve-devel] [RFC qemu-server] migration: nbd export: switch away from deprecated QMP command

[Thomas Lamprecht]

On 02/12/2022 13:54, Fiona Ebner wrote:
> The 'nbd-server-add' QMP command has been deprecated since QEMU 5.2 in
> favor of a more general 'block-export-add'.
> 
> When using 'nbd-server-add', QEMU internally converts the parameters
> and calls blk_exp_add() which is also used by 'block-export-add'. It
> does one more thing, namely calling nbd_export_set_on_eject_blk() to
> auto-remove the export from the server when the backing drive goes
> away. But that behavior is not needed in our case, stopping the NBD
> server removes the exports anyways.
> 
> It was checked with a debugger that the parameters to blk_exp_add()
> are still the same after this change. Well, the block node names are
> autogenerated and not consistent across invocations.
> 
> The alternative to using 'query-block' would be specifying a
> predictable 'node-name' for our '-drive' commandline. It's not that
> difficult for this use case, but in general one needs to be careful
> (e.g. it can't be specified for an empty CD drive, but would need to
> be set when inserting a CD later). Querying the actual 'node-name'
> seemed a bit more future-proof.

are there other examples than the CD-Drive, as that one wouldn't be
really relevant for us here or?

IMO they key disadvantage of switching to computed node-names is the
upgrade path, as I'd expect that for running VMs we cannot change them
to computed ones and thus we would need to still query (or keep the
legacy command) for those...

Re: [pve-devel] [RFC qemu-server] migration: nbd export: switch away from deprecated QMP command

[Fiona Ebner]

Am 16.12.22 um 12:52 schrieb Thomas Lamprecht:
> On 02/12/2022 13:54, Fiona Ebner wrote:
>> The 'nbd-server-add' QMP command has been deprecated since QEMU 5.2 in
>> favor of a more general 'block-export-add'.
>>
>> When using 'nbd-server-add', QEMU internally converts the parameters
>> and calls blk_exp_add() which is also used by 'block-export-add'. It
>> does one more thing, namely calling nbd_export_set_on_eject_blk() to
>> auto-remove the export from the server when the backing drive goes
>> away. But that behavior is not needed in our case, stopping the NBD
>> server removes the exports anyways.
>>
>> It was checked with a debugger that the parameters to blk_exp_add()
>> are still the same after this change. Well, the block node names are
>> autogenerated and not consistent across invocations.
>>
>> The alternative to using 'query-block' would be specifying a
>> predictable 'node-name' for our '-drive' commandline. It's not that
>> difficult for this use case, but in general one needs to be careful
>> (e.g. it can't be specified for an empty CD drive, but would need to
>> be set when inserting a CD later). Querying the actual 'node-name'
>> seemed a bit more future-proof.
> 
> are there other examples than the CD-Drive, as that one wouldn't be
> really relevant for us here or?

Not relevant here, but I'd expect that *if* we introduce node-names,
that we can rely on them in the future in other situations too. The CD
drive example is the only one I found.

> 
> IMO they key disadvantage of switching to computed node-names is the
> upgrade path, as I'd expect that for running VMs we cannot change them
> to computed ones and thus we would need to still query (or keep the
> legacy command) for those...

Since introduction of node-names and export happen on the fresh target
instance there should not be an issue with the upgrade path.

[pve-devel] applied: [RFC qemu-server] migration: nbd export: switch away from deprecated QMP command

[Thomas Lamprecht]

Am 02/12/2022 um 13:54 schrieb Fiona Ebner:
> The 'nbd-server-add' QMP command has been deprecated since QEMU 5.2 in
> favor of a more general 'block-export-add'.
> 
> When using 'nbd-server-add', QEMU internally converts the parameters
> and calls blk_exp_add() which is also used by 'block-export-add'. It
> does one more thing, namely calling nbd_export_set_on_eject_blk() to
> auto-remove the export from the server when the backing drive goes
> away. But that behavior is not needed in our case, stopping the NBD
> server removes the exports anyways.
> 
> It was checked with a debugger that the parameters to blk_exp_add()
> are still the same after this change. Well, the block node names are
> autogenerated and not consistent across invocations.
> 
> The alternative to using 'query-block' would be specifying a
> predictable 'node-name' for our '-drive' commandline. It's not that
> difficult for this use case, but in general one needs to be careful
> (e.g. it can't be specified for an empty CD drive, but would need to
> be set when inserting a CD later). Querying the actual 'node-name'
> seemed a bit more future-proof.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> RFC, because I'm not sure which approach is better.

for now this works out fine, we can always switch to persistent node-names
if it shows to has some relevant advantage.

> 
>  PVE/QemuServer.pm            | 17 ++++++++++++++++-
>  test/MigrationTest/QmMock.pm |  4 +++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
>

applied, thanks!