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