On Mon, Nov 13, 2023 at 09:03:53AM -0800, Andrea Bolognani wrote:
> On Wed, Nov 01, 2023 at 12:52:15PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Oct 31, 2023 at 06:12:59PM +0100, Andrea Bolognani wrote:
> > > if ((rc = virFileIsSharedFS(src)) < 0) {
> > > return false;
> > > } else if (rc == 0) {
> > > + /* Ignore the outcome of this check if we've been
> > > + * asked to assume that storage is shared */
> > > + if (flags & VIR_MIGRATE_ASSUME_SHARED_STORAGE)
> > > + break;
> >
> > So this is essentially telling this particular bit of code to ignore
> > what virFileIsSharedFS reported.
> >
> > The problem this is not the only piece of code relevant to live
> > migration that cares about the virFileIsSharedFS answer. The TPM
> > code also checks virFileIsSharedFS in the context of migration.
> >
> > Also in the security manager, we check virFileIsSharedFS and skip
> > restoring ownership on the src host, as that would kill access on
> > the dst host.
> >
> > So while VIR_MIGRATE_ASSUME_SHARED_STORAGE may work in the precise
> > scenario you've tested, it looks like an incomplete solution to
> > the described problem, and liable to cause new problems.
>
> That's excellent insight, thanks!
>
> > I feel like the root problem here needs addressing in the
> > virFileIsSharedFS method in some manner. Whether or not a given
> > location is shared between two hosts cannot be reliably determined
> > from stat() alone, we need some other inputs.
> >
> > Maybe this is as simple as having a qemu.conf parameter
> >
> > shared_storage_paths = [ "/some/dir", "/other/dir"....]
> >
> > and then we register those paths via virFile during startup.
> >
> > Extending virFileIsSharedFS to work correctly in special deployment
> > scenarios can now be done by the admin when configuring the host.
>
> Yes, this sounds like a much more reasonably sized hammer. I'll look
> into it.
>
> One concern that I have is that if this is configured statically at
> the daemon level, i.e. not fully under control of the management app,
> there might be scenarios where things don't work as expected.
>
> For example, if /some/dir is exported for hostA only, then trying to
> migrate towards hostB will be allowed even though it's clearly not
> going to work. Same for the scenario in which you're trying to
> migrate towards hostA but the mount is not active on the remote side
> for whatever reason.
Don't we do the shared storage check on both hosts ?
ie, if both hostA and hostB libvirts' check /some/dir, then
it will see that hostB's /some/dir is not an NFS volume.
Of course both hostA and hostB could have an /etc/exports
for their respective /some/dir.
At some point we have to decide how much protection we want
to try to give.
Even today's shared storage check is not foolproof. Ie we
check both src & target are shared storage, but we don't
check they're actually the same shared storage.
> Would the migration parameter mechanism perhaps be a better fit? As
> in, the management app would pass a list of paths only after
> performing all the relevant checks, with the advantage of having the
> full information about the migration at its disposal.
Mgmt apps don't neccessarily know any better than libvirt what config
the admin has done on the host OS wrt NFS mounts. We're just as liable
to have mgmt apps blindly pass in a set of paths just to bypass the
libvirt check. Making this a host admin config task gets us alignment
with what the admin has actually setup, as well as what a mgmt app
might have done.
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 :|
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
