[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