[RFC qemu-server 0/5] drive: account for differing TPM sizes

[RFC qemu-server 0/5] drive: account for differing TPM sizes

[Thomas Ellmenreich]

Our current systems to handle TPM drives always assume that the disks have a
fixed 4MiB size that cannot be changed even if that may not be the case. In
situations where the granularity of the backing storage exceeds the 4MiB, an
allocation of a TPM drive will be at least as big as the granularity of said
backing storage.

To solve this mismatch in a very simple way, the default size is retained for
allocation. Only after the allocation do we check the actual size of the
created TPM disk and update the configuration accordingly. A minimum size of
4MiB is always applied to situations where that may not be the case.

In the case of migrations, movement and cloning of disks, we just refer to the
actual disk size instead of the TPM size constant. Doing so also makes it now
possible to move a larger (than 4MiB) TPM disk to a different storage.

Open Questions
--------------

1. During a migration a TPM disk might change from a storage with 4MiB
   granularity to a storage with 64MiB granularity. As already mentioned, the
   TPM disk's size is then automatically increased. The fix I have implemented
   for this, performs a "brute" `qm disk rescan --vmid` on the remote. This
   does the job, but also much more.

   To avoid all of the other side effects of a rescan, I could extend `qm disk
   rescan` to also take another parameter that specifies the disk and so Avoid
   any of the other side effects of calling a rescan on the whole VM. could
   This approach be better? or is there a even better approach?

2. The rescan is currently disabled for remote migrations, but it should also
   be safe for those. So does it make sense to also implement the rescan for
   remote migrations?

3. A small thing, but how should I handle errors produced by PVE::Tools::run_command?
   I didn't quite understand if run_command returns an error if an error is
   emitted by the internal command. If that is not the case, the context is
   never actually shown.


qemu-server:

Thomas Ellmenreich (5):
  drive: improve and extend TPM drive size helpers
  drive: update TPM size on creation
  drive: read actual size when cloning TPM drive
  tests: make qm mock accept disk command
  drive: rescan drives after migrating to different storage

 src/PVE/API2/Qemu.pm                      | 16 ++++++++++----
 src/PVE/QemuMigrate.pm                    | 19 +++++++++++++---
 src/PVE/QemuServer.pm                     | 27 ++++++++++++++++++++---
 src/test/MigrationTest/QemuMigrateMock.pm |  2 ++
 4 files changed, 54 insertions(+), 10 deletions(-)


Summary over all repositories:
  4 files changed, 54 insertions(+), 10 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH qemu-server 1/5] drive: improve and extend TPM drive size helpers

[Thomas Ellmenreich]

The size of a created TPM drive is always based on a constant of 4 MiB, but
if the backing storage has a larger granularity than this, the TPM silently
ignores it. Account for this properly.

Adjust the 'update_tpmstate_size' subroutine so that it checks for the
actual TPM drive size, but falls back on the constant to ensure a
minimum size of 4MiB.

Introduce a new 'get_tpmstate_size' subroutine that only determines
the current best size and returns it as bytes.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuMigrate.pm |  7 ++++---
 src/PVE/QemuServer.pm  | 25 +++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 6605f5d8..3845fb41 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -685,10 +685,11 @@ sub config_update_local_disksizes {
         PVE::QemuServer::update_efidisk_size($conf);
     }
 
-    # TPM state might have an irregular filesize, to avoid problems on transfer
-    # we always assume the static size of 4M to allocate on the target
+    # TPM state might have an irregular file size, or have increased
+    # significantly due to differences in granularity. To avoid problems on
+    # transfer, update the TPM disk size.
     if (defined($conf->{tpmstate0})) {
-        PVE::QemuServer::update_tpmstate_size($conf);
+        PVE::QemuServer::update_tpmstate_size($conf, $self->{storecfg});
     }
 }
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 135c993f..89192e9f 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7972,14 +7972,35 @@ sub update_efidisk_size {
     return;
 }
 
+# Takes a VM config and the storage configuration. Parses the configuration,
+# checks the drive size and updates it to a more up to date value and finally
+# writes the new size back into the provided config.
 sub update_tpmstate_size {
-    my ($conf) = @_;
+    my ($conf, $storecfg) = @_;
+    return if !defined($conf) || !defined($storecfg) || !defined($conf->{tpmstate0});
 
     my $disk = PVE::QemuServer::parse_drive('tpmstate0', $conf->{tpmstate0});
-    $disk->{size} = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
+
+    $disk->{size} = get_tpmstate_size($disk->{file}, $storecfg);
     $conf->{tpmstate0} = print_drive($disk);
 }
 
+# Takes in a volume id and the storage configuration. Will then try to find
+# the actual storage size of the TPM drive.
+sub get_tpmstate_size {
+    my ($volid, $storecfg) = @_;
+
+    return if !defined($volid) || !defined($storecfg);
+
+    my $size = eval { PVE::Storage::volume_size_info($storecfg, $volid, 10) };
+
+    if (!$size || $size < PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE) {
+        $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
+    }
+
+    return $size;
+}
+
 sub vm_iothreads_list {
     my ($vmid) = @_;
 
-- 
2.47.3

[PATCH qemu-server 2/5] drive: update TPM size on creation

[Thomas Ellmenreich]

When creating a TPM drive, the default size is 4MiB. This default is
constant and cannot be changed, unless the backing storage has a bigger
granularity, in which case it is forcibly changed to the bigger size.

To directly show the correct size of a TPM drive, query its size after
creation, and set that as the drive size.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index cfbc1f51..e42cad74 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -665,13 +665,21 @@ my sub create_disks : prototype($$$$$$$$$$$) {
                         $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm, $cvm_type,
                     );
                 } elsif ($ds eq 'tpmstate0') {
-                    # A fixed size is used for TPM state volumes
-                    $size = PVE::Tools::convert_size(
+
+                    # A fixed size is requested for TPM state volumes, but the
+                    # storage might increase it up to its allocation granularity
+                    my $target_size = PVE::Tools::convert_size(
                         PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE,
                         'b' => 'kb',
                     );
-                    $volid =
-                        PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
+
+                    $volid = PVE::Storage::vdisk_alloc(
+                        $storecfg, $storeid, $vmid, $fmt, undef, $target_size,
+                    );
+                    $size = PVE::Tools::convert_size(
+                        PVE::QemuServer::get_tpmstate_size($volid, $storecfg),
+                        'b' => 'kb',
+                    );
                 } else {
                     $volid =
                         PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
-- 
2.47.3

[PATCH qemu-server 3/5] drive: read actual size when cloning TPM drive

[Thomas Ellmenreich]

When cloning a TPM drive, don't assume the default size of 4MiB, instead
read and then use the actual size. The default size is constant, but can
be changed, if the granularity of the backing storage is bigger than 4MiB.

This allows TPM storages larger than 4MiB be moved to different storages
without producing an error.

Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 89192e9f..c93fd4c5 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -7876,7 +7876,7 @@ sub clone_disk {
         } elsif ($dst_drivename eq 'efidisk0') {
             $size = $efisize or die "internal error - need to specify EFI disk size\n";
         } elsif ($dst_drivename eq 'tpmstate0') {
-            $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
+            $size = get_tpmstate_size($drive->{file}, $storecfg);
         } else {
             clone_disk_check_io_uring(
                 $vmid, $drive, $storecfg, $src_storeid, $storeid, $use_drive_mirror,
-- 
2.47.3

[PATCH qemu-server 4/5] tests: make qm mock accept disk command

[Thomas Ellmenreich]

The integration tests fail if the mock does not recognize 'qm disk' as a
valid command, so add it as an option.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/test/MigrationTest/QemuMigrateMock.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/QemuMigrateMock.pm
index c715a60e..4741b1d1 100644
--- a/src/test/MigrationTest/QemuMigrateMock.pm
+++ b/src/test/MigrationTest/QemuMigrateMock.pm
@@ -374,6 +374,8 @@ $MigrationTest::Shared::tools_module->mock(
                         return 0;
                     } elsif ($cmd eq 'stop') {
                         return 0;
+                    } elsif ($cmd eq 'disk') {
+                        return 0;
                     }
                     die "run_command (mocked) ssh qm command - implement me: ${cmd_msg}";
                 } elsif ($cmd eq 'pvesm') {
-- 
2.47.3

[PATCH qemu-server 5/5] drive: rescan drives after migrating to different storage

[Thomas Ellmenreich]

In some cases, migrating to a different storage can change the size of
disks due to the underlying granularity of the storage. In the case of a
storage with 64MiB granularity, this can increase a 4MiB TPM disk to
64MiB. To avoid out of sync ui, rescan all disks of the migrated vm after
such a storage change.

The rescan not only adjusts the disk size, but also shows all unused
disks that may not have been shown before.

Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
---
 src/PVE/QemuMigrate.pm | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 3845fb41..fe5f422a 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -1905,6 +1905,18 @@ sub phase3_cleanup {
         $self->cmd_logerr($cmd, errmsg => "failed to clear migrate lock");
     }
 
+    if (!$self->{opts}->{remote} && scalar(keys %{ $self->{volume_map} })) {
+        my $cmd = [@{ $self->{rem_ssh} }, 'qm', 'disk', 'rescan', '--vmid', $vmid];
+        eval {
+            PVE::Tools::run_command(
+                $cmd,
+                outfunc => sub { },
+                errfunc => sub { $self->log('warn', shift) },
+            );
+        };
+        $self->log('warn', "failed to rescan disks after migration - $@") if $@;
+    }
+
     if ($self->{opts}->{remote} && $self->{opts}->{delete}) {
         eval { PVE::QemuServer::destroy_vm($self->{storecfg}, $vmid, 1, undef, 0) };
         warn "Failed to remove source VM - $@\n" if $@;
-- 
2.47.3

Re: [RFC qemu-server 0/5] drive: account for differing TPM sizes

[Fiona Ebner]

Am 19.10.26 um 11:54 AM schrieb Thomas Ellmenreich:
> Our current systems to handle TPM drives always assume that the disks have a
> fixed 4MiB size that cannot be changed even if that may not be the case. In
> situations where the granularity of the backing storage exceeds the 4MiB, an
> allocation of a TPM drive will be at least as big as the granularity of said
> backing storage.
> 
> To solve this mismatch in a very simple way, the default size is retained for
> allocation. Only after the allocation do we check the actual size of the
> created TPM disk and update the configuration accordingly. A minimum size of
> 4MiB is always applied to situations where that may not be the case.
> 
> In the case of migrations, movement and cloning of disks, we just refer to the
> actual disk size instead of the TPM size constant. Doing so also makes it now
> possible to move a larger (than 4MiB) TPM disk to a different storage.
> 
> Open Questions
> --------------
> 
> 1. During a migration a TPM disk might change from a storage with 4MiB
>    granularity to a storage with 64MiB granularity. As already mentioned, the
>    TPM disk's size is then automatically increased. The fix I have implemented
>    for this, performs a "brute" `qm disk rescan --vmid` on the remote. This
>    does the job, but also much more.
> 
>    To avoid all of the other side effects of a rescan, I could extend `qm disk
>    rescan` to also take another parameter that specifies the disk and so Avoid
>    any of the other side effects of calling a rescan on the whole VM. could
>    This approach be better? or is there a even better approach?
> 
> 2. The rescan is currently disabled for remote migrations, but it should also
>    be safe for those. So does it make sense to also implement the rescan for
>    remote migrations?
> 

See my reply to the last patch.

> 3. A small thing, but how should I handle errors produced by PVE::Tools::run_command?
>    I didn't quite understand if run_command returns an error if an error is
>    emitted by the internal command. If that is not the case, the context is
>    never actually shown.

Yes, if the invoked command ends with a non-zero exit status,
run_command() will die. The stderr output is handled by the errfunc in
your patch.

> 
> 
> qemu-server:
> 
> Thomas Ellmenreich (5):
>   drive: improve and extend TPM drive size helpers
>   drive: update TPM size on creation
>   drive: read actual size when cloning TPM drive
>   tests: make qm mock accept disk command
>   drive: rescan drives after migrating to different storage
> 
>  src/PVE/API2/Qemu.pm                      | 16 ++++++++++----
>  src/PVE/QemuMigrate.pm                    | 19 +++++++++++++---
>  src/PVE/QemuServer.pm                     | 27 ++++++++++++++++++++---
>  src/test/MigrationTest/QemuMigrateMock.pm |  2 ++
>  4 files changed, 54 insertions(+), 10 deletions(-)
> 
> 
> Summary over all repositories:
>   4 files changed, 54 insertions(+), 10 deletions(-)
> 

Re: [PATCH qemu-server 1/5] drive: improve and extend TPM drive size helpers

[Fiona Ebner]

Nit: "improve and extend" could be replaced with something more concrete

Am 19.10.26 um 11:53 AM schrieb Thomas Ellmenreich:
> The size of a created TPM drive is always based on a constant of 4 MiB, but
> if the backing storage has a larger granularity than this, the TPM silently
> ignores it. Account for this properly.

Here "the TPM silently ignores it" and "Account for this properly" are
also rather vague. You can mention more concretely that swtpm can deal
with block-based volumes larger than 4 MiB and that this needs to be
accounted for migration.

> Adjust the 'update_tpmstate_size' subroutine so that it checks for the
> actual TPM drive size, but falls back on the constant to ensure a
> minimum size of 4MiB.

I'd move this to after the next paragraph and mention that the new
method is used in update_tpmstate_size, and that this addresses migration.

> 
> Introduce a new 'get_tpmstate_size' subroutine that only determines
> the current best size and returns it as bytes.

What does "current best size" mean? Again, I suggest mentioning what it
is concretely, i.e. "the maximum of the current size and the static size
of 4 MiB".

> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuMigrate.pm |  7 ++++---
>  src/PVE/QemuServer.pm  | 25 +++++++++++++++++++++++--

Pre-existing, but please move/add the helpers to the Drive module
instead. Your commit title is very suggestive of this too ;)

>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
> index 6605f5d8..3845fb41 100644
> --- a/src/PVE/QemuMigrate.pm
> +++ b/src/PVE/QemuMigrate.pm
> @@ -685,10 +685,11 @@ sub config_update_local_disksizes {
>          PVE::QemuServer::update_efidisk_size($conf);
>      }
>  
> -    # TPM state might have an irregular filesize, to avoid problems on transfer
> -    # we always assume the static size of 4M to allocate on the target
> +    # TPM state might have an irregular file size, or have increased
> +    # significantly due to differences in granularity. To avoid problems on

Nit: I would replace "or have increased significantly due to differences
in granularity" with "or be larger than the static size of 4 MiB if the
storage has a larger granularity".

> +    # transfer, update the TPM disk size.
>      if (defined($conf->{tpmstate0})) {
> -        PVE::QemuServer::update_tpmstate_size($conf);
> +        PVE::QemuServer::update_tpmstate_size($conf, $self->{storecfg});
>      }
>  }
>  
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 135c993f..89192e9f 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7972,14 +7972,35 @@ sub update_efidisk_size {
>      return;
>  }
>  
> +# Takes a VM config and the storage configuration. Parses the configuration,
> +# checks the drive size and updates it to a more up to date value and finally

I suggest replacing "more up to date value" with "the maximum of the
current size and the static size of 4 MiB".

> +# writes the new size back into the provided config.
>  sub update_tpmstate_size {
> -    my ($conf) = @_;
> +    my ($conf, $storecfg) = @_;
> +    return if !defined($conf) || !defined($storecfg) || !defined($conf->{tpmstate0});

I'd consider $conf and $storecfg required parameters, i.e. the function
should rather die if they are missing and not quietly return.

>  
>      my $disk = PVE::QemuServer::parse_drive('tpmstate0', $conf->{tpmstate0});

When moving to the Drive module, you can replace this with the proper,
i.e. non-imported, parse_drive() function.

> -    $disk->{size} = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> +
> +    $disk->{size} = get_tpmstate_size($disk->{file}, $storecfg);
>      $conf->{tpmstate0} = print_drive($disk);
>  }
>  
> +# Takes in a volume id and the storage configuration. Will then try to find
> +# the actual storage size of the TPM drive.

Should mention that it uses the static TPMSTATE_DISK_SIZE as a minimum.

> +sub get_tpmstate_size {
> +    my ($volid, $storecfg) = @_;
> +
> +    return if !defined($volid) || !defined($storecfg);

Similarly here, those are required parameters.

> +
> +    my $size = eval { PVE::Storage::volume_size_info($storecfg, $volid, 10) };

Please print the error message as a warning if there is one.

> +
> +    if (!$size || $size < PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE) {
> +        $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> +    }
> +
> +    return $size;
> +}
> +
>  sub vm_iothreads_list {
>      my ($vmid) = @_;
>  

Re: [PATCH qemu-server 4/5] tests: make qm mock accept disk command

[Fiona Ebner]

Am 19.10.26 um 11:54 AM schrieb Thomas Ellmenreich:
> The integration tests fail if the mock does not recognize 'qm disk' as a
> valid command, so add it as an option.
> 

This is not true at this stage. It only becomes relevant with the next
patch, so it should just be part of that directly.

> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/test/MigrationTest/QemuMigrateMock.pm | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/test/MigrationTest/QemuMigrateMock.pm b/src/test/MigrationTest/QemuMigrateMock.pm
> index c715a60e..4741b1d1 100644
> --- a/src/test/MigrationTest/QemuMigrateMock.pm
> +++ b/src/test/MigrationTest/QemuMigrateMock.pm
> @@ -374,6 +374,8 @@ $MigrationTest::Shared::tools_module->mock(
>                          return 0;
>                      } elsif ($cmd eq 'stop') {
>                          return 0;
> +                    } elsif ($cmd eq 'disk') {
> +                        return 0;
>                      }
>                      die "run_command (mocked) ssh qm command - implement me: ${cmd_msg}";
>                  } elsif ($cmd eq 'pvesm') {

Re: [PATCH qemu-server 3/5] drive: read actual size when cloning TPM drive

[Fiona Ebner]

The commit subject should start with "clone disk:"

Am 19.10.26 um 11:53 AM schrieb Thomas Ellmenreich:
> When cloning a TPM drive, don't assume the default size of 4MiB, instead
> read and then use the actual size. The default size is constant, but can
> be changed, if the granularity of the backing storage is bigger than 4MiB.

Style nit: line too long.

> 
> This allows TPM storages larger than 4MiB be moved to different storages

Replace "TPM storages larger than" with "TPM volumes on storages with a
granularity larger than".

> without producing an error.
> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>

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

> ---
>  src/PVE/QemuServer.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 89192e9f..c93fd4c5 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7876,7 +7876,7 @@ sub clone_disk {
>          } elsif ($dst_drivename eq 'efidisk0') {
>              $size = $efisize or die "internal error - need to specify EFI disk size\n";
>          } elsif ($dst_drivename eq 'tpmstate0') {
> -            $size = PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE;
> +            $size = get_tpmstate_size($drive->{file}, $storecfg);
>          } else {
>              clone_disk_check_io_uring(
>                  $vmid, $drive, $storecfg, $src_storeid, $storeid, $use_drive_mirror,

Re: [PATCH qemu-server 5/5] drive: rescan drives after migrating to different storage

[Fiona Ebner]

Am 19.10.26 um 11:54 AM schrieb Thomas Ellmenreich:
> In some cases, migrating to a different storage can change the size of
> disks due to the underlying granularity of the storage. In the case of a
> storage with 64MiB granularity, this can increase a 4MiB TPM disk to
> 64MiB. To avoid out of sync ui, rescan all disks of the migrated vm after
> such a storage change.
> 
> The rescan not only adjusts the disk size, but also shows all unused
> disks that may not have been shown before.
> 

This is not specific to TPM or this series and warrants more discussion,
so I would suggest leaving it out.

I don't think we should do a full rescan:

1. It can help to find orphaned volumes, but it would be nicer to have a
proper view/panel for all orphaned volumes in the UI.
2. It will iterate all storages with images and thus migration can
suddenly produce an error (not a bad one, everything is safe on the
target already, but still), because of an unrelated storage with issues.

If we go for this, I feel like it could be a tunnel command, that does
an update for all volumes in the configuration. See also the FIXME in
the beginning of phase3_cleanup() and the (git) history behind it.

For remote migration, it doesn't need a special command, it could be
part of the 'quit'. We could also introduce a 'quit' for intra-cluster
migration and do it as part of that. Then we can add other such
operations there without requiring a new command in the future.

> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>
> ---
>  src/PVE/QemuMigrate.pm | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
> index 3845fb41..fe5f422a 100644
> --- a/src/PVE/QemuMigrate.pm
> +++ b/src/PVE/QemuMigrate.pm
> @@ -1905,6 +1905,18 @@ sub phase3_cleanup {
>          $self->cmd_logerr($cmd, errmsg => "failed to clear migrate lock");
>      }
>  
> +    if (!$self->{opts}->{remote} && scalar(keys %{ $self->{volume_map} })) {
> +        my $cmd = [@{ $self->{rem_ssh} }, 'qm', 'disk', 'rescan', '--vmid', $vmid];
> +        eval {
> +            PVE::Tools::run_command(

Nit: please use PVE::Cmd::run() for new calls

> +                $cmd,
> +                outfunc => sub { },

Nit: the resulting stdout could also be logged as info for completeness

> +                errfunc => sub { $self->log('warn', shift) },
> +            );
> +        };
> +        $self->log('warn', "failed to rescan disks after migration - $@") if $@;
> +    }
> +
>      if ($self->{opts}->{remote} && $self->{opts}->{delete}) {
>          eval { PVE::QemuServer::destroy_vm($self->{storecfg}, $vmid, 1, undef, 0) };
>          warn "Failed to remove source VM - $@\n" if $@;

Re: [PATCH qemu-server 2/5] drive: update TPM size on creation

[Fiona Ebner]

The commit subject should start out with "api: create disks:"

Am 19.10.26 um 11:53 AM schrieb Thomas Ellmenreich:
> When creating a TPM drive, the default size is 4MiB. This default is
> constant and cannot be changed, unless the backing storage has a bigger
> granularity, in which case it is forcibly changed to the bigger size.
> 
> To directly show the correct size of a TPM drive, query its size after
> creation, and set that as the drive size.
> 
> Suggested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Thomas Ellmenreich <t.ellmenreich@proxmox.com>

With the commit subject and nits addressed:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> ---
>  src/PVE/API2/Qemu.pm | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index cfbc1f51..e42cad74 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -665,13 +665,21 @@ my sub create_disks : prototype($$$$$$$$$$$) {
>                          $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm, $cvm_type,
>                      );
>                  } elsif ($ds eq 'tpmstate0') {
> -                    # A fixed size is used for TPM state volumes
> -                    $size = PVE::Tools::convert_size(
> +

Style nit: blank line here is inconsistent with the surrounding style

> +                    # A fixed size is requested for TPM state volumes, but the
> +                    # storage might increase it up to its allocation granularity
> +                    my $target_size = PVE::Tools::convert_size(
>                          PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE,
>                          'b' => 'kb',
>                      );
> -                    $volid =
> -                        PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);
> +

Style nit: I'd drop this blank too, since otherwise the first bit with
the comment feels a bit detached

> +                    $volid = PVE::Storage::vdisk_alloc(
> +                        $storecfg, $storeid, $vmid, $fmt, undef, $target_size,
> +                    );
> +                    $size = PVE::Tools::convert_size(
> +                        PVE::QemuServer::get_tpmstate_size($volid, $storecfg),
> +                        'b' => 'kb',
> +                    );
>                  } else {
>                      $volid =
>                          PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size);