On Mon, Aug 22, 2022 at 03:17:34PM -0400, Stefan Berger wrote:
On 8/22/22 12:46, Daniel P. Berrangé wrote:
On Mon, Aug 22, 2022 at 08:05:53AM -0400, Stefan Berger wrote:
When share storage for the TPM state files has been setup betwen hosts then remove the TPM state files and directory only when undefining a VM and only if the attribute persistent_state is not set. Avoid removing the TPM state files and directory structure when a VM is migrated and shared storage is used since this would also remove those files and directory structure on the destination side.
I think our current undefine behaviour is probably flawed. We go to the trouble of refusing to remove the firmware NVRAM when undefining because it contains important VM state, but then happily blow away the TPM state. Totally inconsistent behaviour :-( Its too late to change the default behaviour, but we likely ought to add a flag
VIR_DOMAIN_UNDEFINE_KEEP_TPM
and plumb that through the varius code paths, which would remove the need for this specific 'qemuDomainUndefineReason' enum.
I think the granularity encoded in the reason is necessary for the following patch I was going to post later on:
Subject: [PATCH] qemu: tpm: Remove TPM state with non-shared storage
This patch 'fixes' the behavior of the persistent_state TPM domain XML attribute that intends to preserve the state of the TPM but should not keep the state around on all the hosts a VM has been migrated to. It removes the TPM state directory structure from the source host upon successful migration when non-shared storage is used. Similarly, it removes it from the destination host upon migration failure when non-shared storage is used.
'persistent_state' is something that applies to transient VMs. What I'm suggesting is that we should have some control over this for persistent VMs, when calling 'undefine'. ie virsh undefine --keep-nvram --keep-tpm $VMNAME if we have this feature in the public API, then its impl should support both this feature and your future patch for 'persistent_state'.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_migration.c | 4 ++-- src/qemu/qemu_tpm.c | 3 +++ 3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8e5dacf624..4cf6a640c2 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -685,6 +685,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, typedef enum { QEMU_DOMAIN_UNDEFINE_UNSPEC = 0, QEMU_DOMAIN_UNDEFINE_DOMAIN, /* virsh undefine type of command */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC, /* undefine due to successful migration on src */ + QEMU_DOMAIN_UNDEFINE_MIGRATION_DST, /* undefine due to failed migration on dst */ } qemuDomainUndefineReason;
void qemuDomainRemoveInactive(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b15c1ccc22..f50a488072 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4055,7 +4055,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, vm->persistent = 0; } qemuDomainRemoveInactive(driver, vm, - QEMU_DOMAIN_UNDEFINE_UNSPEC); + QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC); }
cleanup: @@ -6668,7 +6668,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, }
if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_UNSPEC); + qemuDomainRemoveInactive(driver, vm, QEMU_DOMAIN_UNDEFINE_MIGRATION_DST);
virErrorRestore(&orig_err); return NULL; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d1639318e7..ad9bc9f1a5 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -739,6 +739,9 @@ qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } + } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || + undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { + qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } -- 2.37.1
The complete qemuTPMEmulatorCleanupHost handling shared and non-shared storage along with the persistent_state attribute now looks like this:
static void qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, qemuDomainUndefineReason undefReason) { if (tpm->data.emulator.shared_storage) { /* When using shared storage remove the domain only if this is due to * a 'virsh undefine' type of command and only if persistent_state == * false. Avoid removal of the state files/directory during migration. */ if (undefReason == QEMU_DOMAIN_UNDEFINE_DOMAIN && !tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } } else if (undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_SRC || undefReason == QEMU_DOMAIN_UNDEFINE_MIGRATION_DST) { qemuTPMEmulatorDeleteStorage(tpm); } else if (!tpm->data.emulator.persistent_state) { qemuTPMEmulatorDeleteStorage(tpm); } }
Regards, Stefan
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|