[libvirt] [PATCH v3 01/11] conf: Add some defined NFS Storage Pool mount options

Wed Jan 16 18:13:30 UTC 2019

On Wed, Jan 16, 2019 at 12:47:32PM -0500, John Ferlan wrote:
> On 1/16/19 7:00 AM, Daniel P. Berrangé wrote:
> > Hmm, this has gone back to the "Mount options" concept from v1
> > which I don't think is appropriate. In v2 feedback I had suggested
> > that we should have some more explicit XML description for the subset
> > of options we wish to support, with each option potentially using a
> > different syntax, no generic mount options syntax.
> 
> In v1 it seems to me at least the objection was arbitrarily named
> mount_opts, so specifically named mount_opts felt like a fair trade-off.

I guess I should have been clearer that I didn't like both parts.

> In v2 you noted the features element to be similar to the domain
> features, but then pointed out that you felt we could just default to
> using them. A followup pointed out one concern over OS syntax
> differences, which is essentially why I took this approach - force
> someone to provide the option. If that option used different syntax on
> some other OS, then someone else who cares about that OS could send a
> patch to change the name generated (and we avoid making any assumptions).

If we force apps to provide the option in the XML then it
more likely to cause incompatibilities, as most apps won't
test whether an OS supports a given option.

If we default enable them in libvirt then libvirt can just
put this in in ifdef __linux__ so we don't risk breaking
other non-Linux OS.

> > For 'ro' we should use our normal  <readonly/> empty element syntax.
> 
> True we could, but believe it or not readonly is not even defined in
> storagepool.rng. It's a domaincommon.rng to be used for hypervisor
> options. Adding ro to the mount_opts list was essentially a shortcut of
> patches to include ro with the others. I saw nfsvers a bit different
> since it would provide more than a boolean option.

Even though its a different schema, it is worth using a consistent
element naming approach IMHO.

> Still beyond NFS I'm trying to envision how we'd supply readonly such
> that it would matter for the pool. Wouldn't networked pools require the
> server side to have set up readonly? For local pools how much of the
> exposure of the volume would be controllable such that the pool could
> manage the readonly-ness? A virDomainDiskTranslateSourcePool "option" I
> suppose that could be associated with the domain readonly attribute.
> 
> Perhaps ro or readonly should just be separate then because it involves
> a lot more interaction. Does the overhead of code addition outweigh the
> usefulness of a generic pool readonly option? No one's asked for such a
> feature, so sneaking in ro to/for an NFS mount option felt reasonable.

We can't predict the future reliably. If we use <mountopts><ro/></mountopts>
and  then in future find a need for readonly on other storage pools where
<mountopts> is not relevant, we'll have to invent a second way to specify
readonly. By using <readonly/> we've not tied our hands, so we have more
flexibility to cope with the future needs. It may never matter, but I'd
always aim to avoid restricting ourselves.

> > For nosetuid, nodev, noexec, I'm still inclined to say we should
> > enable them by default. Despite it being a semantic change, I think
> > it is the right thing for a volume that is being used for storing
> > VM disk images.
> 
> Again, the fear of doing that is some OS has different syntax (from your
> v2 response it's possible that FreeBSD mount command options may differ
> from Linux mount command options).
> 
> So, using named options seemed to be a better tradeoff than just
> altering the command generation to add those options for NFS mounts.

If we did it in defaults, then we'd have to be conservative
in what we enable, so if we don't know they are supported
by an OS we shouldn't enable them in the code when building
for that OS.

BTW, I did just check FreeBSD and it supports nosuid and
noexec but not  nodev.

So we'd want

  #ifdef __linux__
  #define opts ",nodev,nosuid,noexec"
  #else
  #ifdef__FreeBSD__
  #define opts ",nosuid,noexec"
  #else
  #define opts ""
  #endif

I'm thinking perhaps we should never have been considering the
XML at all for nodev, nosuid, noexec.

Instead it might be better as a configurable at a host level
rather than XML level.  eg  a /etc/libvirt/storage.conf
file with an 'nfs_extra_mount_opts' config parameter,
whose default value is "nodev,nosuid,noexec" on Linux
hosts.

That would let sysadmins enforce a host-wide policy without
needing to worry about whether an application has been updated
to support setting the XML to add extra mount options.

That would avoid most debate about mount options / features
in the XML.

Probably the protocol version is the only thing that is
important for the XML as that can vary per-server.

Read-only is also probably relevant in XML.

> I would think that would be far more confusing, but seems to me to
> follow how features are assigned/used for specific hypervisors. It still
> requires some sort of mapping of names and we're not allowing arbitrary
> ones. Does nosuid, noexec, or nodev have meaning beyond that of the
> meaning used for NFS? If the answer is no, then it's a very specific
> feature to a very specific pool.

BTW nosuid, noexec, nodev are nothing todo with NFS - they're general
options that can be set on any mount type.

> > This provides a generic framework that will work for other storage
> > pool backend features where there's no mount concept involved eg
> > LVM, iSCSI, etc
> > 
> 
> And what "features" would we consider allowing without having to craft a
> different backend?
> 
> Allowing some provided option for LVM commands could result in needing
> to write code that would be able to handle multiple different formats.
> In particular I'm thinking about how thin provisioning is supported.
> Very different way to configure and a completely different means to scan
> the generated output looking for usable volumes. Previous attempts to
> co-mingle in the current logical backend were NACK'd and no one has
> taken up the create a new backend that supports thin pools vs thin
> snapshots (I think the latter is the terminology used, it's been a few
> years).
> 
> For iSCSI we now have the iscsi-direct backend which perhaps could make
> use of something, but it'd be something fairly specific to some iSCSI
> feature which is no different (to me at least) than providing some
> specific option for netfs.

There's perhaps a need for <features> under the pool <source> to
make it clear that the valid features are specific to the pool
type.  Then bearing in mind what I said earlier, perhaps we should
just forget the entire features / mount opts idea for XML and use
the host config file.

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