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@xxxxxxxxxxxxx>
> ---
> 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 :|
