[PATCH v2 2/9] qemu: Introduced VIR_MIGRATE_TPM_SHARED_STORAGE for TPM migration

[Daniel P. Berrangé]

On Mon, Oct 17, 2022 at 09:39:52AM -0400, Stefan Berger wrote:
> 
> 
> On 10/14/22 11:28, Daniel P. Berrangé wrote:
> > On Thu, Oct 06, 2022 at 04:07:13PM +0200, Michal Prívozník wrote:
> > > On 10/6/22 15:47, Daniel P. Berrangé wrote:
> > > > On Wed, Oct 05, 2022 at 10:02:00AM -0400, Stefan Berger wrote:
> > > > > Introduced VIR_MIGRATE_TPM_SHARED_STORAGE for migrating a TPM across
> > > > > shared storage.
> > > > > 
> > > > > At this point do not support this flag in 'virsh', yet.
> > > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb at linux.ibm.com>
> > > > > ---
> > > > >   include/libvirt/libvirt-domain.h | 8 ++++++++
> > > > >   1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> > > > > index 8357aea797..110929039d 100644
> > > > > --- a/include/libvirt/libvirt-domain.h
> > > > > +++ b/include/libvirt/libvirt-domain.h
> > > > > @@ -1098,6 +1098,14 @@ typedef enum {
> > > > >        * Since: 8.5.0
> > > > >        */
> > > > >       VIR_MIGRATE_ZEROCOPY = (1 << 20),
> > > > > +
> > > > > +    /* Support TPM migration across hosts that have shared storage setup for
> > > > > +     * the directory structure holding the state of TPMs. Typically this would
> > > > > +     * mean that the directory /var/lib/libvirt/swtpm is shared.
> > > > > +     *
> > > > > +     * Since: 8.9.0
> > > > > +     */
> > > > > +    VIR_MIGRATE_TPM_SHARED_STORAGE = (1 << 21),
> > > > 
> > > > Why do we need this flag at all.  We don't require users to set any flag
> > > > when dealing with shared storage for disks, we just make sure we detect
> > > > shared storage and "do the right thing" with it.
> > > 
> > > That could work. Until the state is stored on a shared FS but not shared
> > > across migration hosts. But I guess our disk migration code would fail
> > > then too, wouldn't it?
> > 
> > Exactly, if our existing code is good enough for disks for the last
> > NNN years, then its good enough for TPM too. If someone finds a broken
> > scenario, then we'll need to fix it for all cases, and that'll require
> > something more general than a VIR_MIGRATE_TPM_SHARED_STORAGE flag.
> > 
> > It is basically akin to a  "make it work=yes" setting, and actually
> > introduces failure scenarios that would not otherwise exist. eg
> > if someone sets VIR_MIGRATE_TPM_SHARED_STORAGE when the TPM is on
> > local only storage, or fails to set it when using shared storage.
> 
> 
> I was trying to find out how storage is being copied and whether there
> is any testing going on regarding whether storage for storage is shared
> or not and how that's done. However, it seems there's an explicit hint
> coming from the user encoded in the virsh command line options about
> non-shared storage for storage. Or maybe I not dig deep enough?
> 
>     if (vshCommandOptBool(cmd, "copy-storage-all"))
>         flags |= VIR_MIGRATE_NON_SHARED_DISK;
> 
>     if (vshCommandOptBool(cmd, "copy-storage-inc"))
>         flags |= VIR_MIGRATE_NON_SHARED_INC;
> 
>     if (vshCommandOptBool(cmd, "copy-storage-synchronous-writes")) {
>         if (!(flags & VIR_MIGRATE_NON_SHARED_DISK) &&
>             !(flags & VIR_MIGRATE_NON_SHARED_INC)) {
>             vshError(ctl, "'--copy-storage-synchronous-writes' requires one of '--copy-storage-all', 'copy-storage-inc'");
>             goto out;
>         }
>         flags |= VIR_MIGRATE_NON_SHARED_SYNCHRONOUS_WRITES;
>     }

The key is in qemuMigrationSrcIsSafe(), and how it determines if a
migration is safe.

  * Disk on local storage, no flags  => unsafe, migration error
  * Disk on local storage, VIR_MIGRATE_NON_SHARED_DISK => ok, copies disk storage
  * Disk on shared storage, no flags => safe
  * Disk on shared storage, VIR_MIGRATE_NON_SHARED_DISK => ok, but needlessly copies disk storage

The key helper methods are virFileIsSharedFS and virFileIsClusterFS
which check the filesystem type for the path against a known list
of shared/cluster FS.

> So we won't do it this way for TPM state migration. Instead we can
> try to write on the source migration side
> 
> a) a simple file and detect whether the file is at the destination
> b) a file with either a name or content that only the source and
>    destination libvirts would know at this point
> 
> b) is to prevent stale files from becoming indicators for shared storage.

No, please use the virFileIsSharedFS/ClusterFS helpers.


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