docs/formatstorage.html.in | 87 ++++++++++ docs/schemas/storagepool.rng | 43 +++++ src/conf/storage_conf.c | 51 +++++- src/conf/storage_conf.h | 24 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 127 ++++++++++++++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- src/storage/storage_util.c | 36 ++++ src/storage/storage_util.h | 14 ++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 4 + .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 17 ++ .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 7 + tools/virsh-pool.c | 39 ++++- tools/virsh.pod | 5 + 19 files changed, 680 insertions(+), 7 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml
v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html Kept the subject the same, but the concept has been adjusted to follow issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. This series adds both the NFS and the RBD adjustments that were essentially in the referenced series from review. Changes since v1: * Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool XML namespace. Could be split again, but separately the patches were too small IMO. * Patch 2: (NEW) Implementation of providing XML Namespace functions. The details of the operation are in the subsequent patch. I could combine 2 and 3, but perhaps it's easier to review separated. * Patch 3: (REWORK) Rework the previous series patch1 in order to utilize the Storage Pool XML Namespace. Adjust the description of the configuration and modify the storagepoolxml2xmltest appropriately. * Patch 4: (REWORK) Utilize the XML Namespace data to generate the command line which doesn't change since v1. * Patch 5: (REWORK) Rework the virsh code in order to generate the proper syntax to utilize Storage Pool XML Namespaces. * Patch 6: (NEW) Well, new as in this series, but it's essentially a rework of what was posted in the series jtomko referenced. The code will "prove" (to a degree) that the concept can work for multiple pool types. The "downside" is that there's no (easy) way that I could find to generate output to show the commands are executed. John Ferlan (6): conf: Introduce virStoragePoolXMLNamespace nfs: Add infrastructure to manage XML namespace options docs,tests: Add schema, description, and tests for NFS namespace storage: Add NFS storage pool mount options to command line virsh: Add source-mount-opts for pool commands rbd: Utilize storage pool namespace to manage config options docs/formatstorage.html.in | 87 ++++++++++ docs/schemas/storagepool.rng | 43 +++++ src/conf/storage_conf.c | 51 +++++- src/conf/storage_conf.h | 24 +++ src/libvirt_private.syms | 1 + src/storage/storage_backend_fs.c | 127 ++++++++++++++ src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- src/storage/storage_util.c | 36 ++++ src/storage/storage_util.h | 14 ++ tests/Makefile.am | 4 +- .../pool-netfs-mountopts.argv | 1 + tests/storagepoolxml2argvtest.c | 4 + .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 17 ++ .../pool-netfs-mountopts.xml | 24 +++ .../pool-rbd-configopts.xml | 20 +++ tests/storagepoolxml2xmltest.c | 7 + tools/virsh-pool.c | 39 ++++- tools/virsh.pod | 5 + 19 files changed, 680 insertions(+), 7 deletions(-) create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/8/19 6:52 PM, John Ferlan wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html > > Kept the subject the same, but the concept has been adjusted to follow > issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. > This series adds both the NFS and the RBD adjustments that were essentially > in the referenced series from review. > > Changes since v1: > > * Patch 1: (NEW) Implements a bare-bones infrastructure for storage pool > XML namespace. Could be split again, but separately the patches > were too small IMO. > > * Patch 2: (NEW) Implementation of providing XML Namespace functions. The > details of the operation are in the subsequent patch. I could > combine 2 and 3, but perhaps it's easier to review separated. > > * Patch 3: (REWORK) Rework the previous series patch1 in order to utilize > the Storage Pool XML Namespace. Adjust the description of the > configuration and modify the storagepoolxml2xmltest appropriately. > > * Patch 4: (REWORK) Utilize the XML Namespace data to generate the command > line which doesn't change since v1. > > * Patch 5: (REWORK) Rework the virsh code in order to generate the proper > syntax to utilize Storage Pool XML Namespaces. > > * Patch 6: (NEW) Well, new as in this series, but it's essentially a rework > of what was posted in the series jtomko referenced. The code will > "prove" (to a degree) that the concept can work for multiple pool > types. The "downside" is that there's no (easy) way that I could > find to generate output to show the commands are executed. > > John Ferlan (6): > conf: Introduce virStoragePoolXMLNamespace > nfs: Add infrastructure to manage XML namespace options > docs,tests: Add schema, description, and tests for NFS namespace > storage: Add NFS storage pool mount options to command line > virsh: Add source-mount-opts for pool commands > rbd: Utilize storage pool namespace to manage config options > > docs/formatstorage.html.in | 87 ++++++++++ > docs/schemas/storagepool.rng | 43 +++++ > src/conf/storage_conf.c | 51 +++++- > src/conf/storage_conf.h | 24 +++ > src/libvirt_private.syms | 1 + > src/storage/storage_backend_fs.c | 127 ++++++++++++++ > src/storage/storage_backend_rbd.c | 159 +++++++++++++++++- > src/storage/storage_util.c | 36 ++++ > src/storage/storage_util.h | 14 ++ > tests/Makefile.am | 4 +- > .../pool-netfs-mountopts.argv | 1 + > tests/storagepoolxml2argvtest.c | 4 + > .../pool-netfs-mountopts.xml | 24 +++ > .../pool-rbd-configopts.xml | 17 ++ > .../pool-netfs-mountopts.xml | 24 +++ > .../pool-rbd-configopts.xml | 20 +++ > tests/storagepoolxml2xmltest.c | 7 + > tools/virsh-pool.c | 39 ++++- > tools/virsh.pod | 5 + > 19 files changed, 680 insertions(+), 7 deletions(-) > create mode 100644 tests/storagepoolxml2argvdata/pool-netfs-mountopts.argv > create mode 100644 tests/storagepoolxml2xmlin/pool-netfs-mountopts.xml > create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-configopts.xml > create mode 100644 tests/storagepoolxml2xmlout/pool-netfs-mountopts.xml > create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-configopts.xml > ACK My only worry is that we don't validate the options in any way. They are basically a text free form which we will put onto 'mount' cmd line directly. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/9/19 11:55 AM, Michal Privoznik wrote: [...] > > > ACK > Thanks for the review... although it seems I'll have to consider a 3rd approach as a result of Daniel's comments. Nothing is ever easy. > My only worry is that we don't validate the options in any way. They are > basically a text free form which we will put onto 'mount' cmd line directly. > I thought about making sure of specific chars for the attributes (there are some examples in other .rng files), but I know as soon as I do, there's some request for some char that wasn't included. Going with <text/> was just the bail out especially since I find regex's to be unintelligible to fully decipher. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote: > v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html > > Kept the subject the same, but the concept has been adjusted to follow > issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. > This series adds both the NFS and the RBD adjustments that were essentially > in the referenced series from review. Can you give any info about what motivated this addition. Anything that is implemented via a separate XML namespace is generally considered "unsupported, you're on your own when it breaks" so if this feature is neeeded for any management application like oVirt, OpenStack, etc, then using a custom namespace is not going to be a suitable approach. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/9/19 12:09 PM, Daniel P. Berrangé wrote: > On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote: >> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html >> >> Kept the subject the same, but the concept has been adjusted to follow >> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. >> This series adds both the NFS and the RBD adjustments that were essentially >> in the referenced series from review. > > Can you give any info about what motivated this addition. There's a bz: https://bugzilla.redhat.com/show_bug.cgi?id=1584663 which along the way has had private comments - I probably should add the bz to at least one of the patches, but I think because of the (probably unnecessary) private comments in the bz, I was hesitant to do so. I tried a different mechanism and Jano pointed out: https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html that previous attempts were rejected due to the arbitrary text. > > Anything that is implemented via a separate XML namespace is > generally considered > > "unsupported, you're on your own when it breaks" > > so if this feature is neeeded for any management application like > oVirt, OpenStack, etc, then using a custom namespace is not going > to be a suitable approach. And I don't think it would be palatable to add N 'text' options to the XML that map to the same N 'text' options in the mount command. So this was the next best option as far as I saw it. That'd be one of those never ending tasks to add the favorite option. John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote: > > > On 1/9/19 12:09 PM, Daniel P. Berrangé wrote: > > On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote: > >> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html > >> > >> Kept the subject the same, but the concept has been adjusted to follow > >> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. > >> This series adds both the NFS and the RBD adjustments that were essentially > >> in the referenced series from review. > > > > Can you give any info about what motivated this addition. > > There's a bz: > > https://bugzilla.redhat.com/show_bug.cgi?id=1584663 > > which along the way has had private comments - I probably should add the > bz to at least one of the patches, but I think because of the (probably > unnecessary) private comments in the bz, I was hesitant to do so. > > I tried a different mechanism and Jano pointed out: > > https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html > > that previous attempts were rejected due to the arbitrary text. Ok, from the rather limited information in the BZ above, it is clear that this feature is required to be production supported, so that rules out use of private XML namespaces as a viable solution. I agree with Jano's rejected of arbitrary mount option passthrough in v1 too though. I think the right answer involves multiple approaches in the XML. For the NFS pool, we should have an explicit way to request the NFS protocol version in the XML. For the general nosuid, nodev flags, I think we can do something like we have for the <features/> element in the domain XML. Or on second thoughts, the only reason for the storage pools mounts is to provide VM disk images, so we should just unconditionally always set nosuid & nodev when running mount. No storage volume we report should be setuid or be a device node. If the NFS mount has a directory tree for container filesystems, then the previously discussed virFileSystem APIs should be actually implemented. We could none the less also still have a private XML namespace for doing arbitrary mount argument passthrough, but that shouldn't be the solution for this particular BZ. It is just a way to get a quick workaround for an option while we decide how to model the desired new mount option explicitly in the XML, as we do for QEMU options. > > > > > Anything that is implemented via a separate XML namespace is > > generally considered > > > > "unsupported, you're on your own when it breaks" > > > > so if this feature is neeeded for any management application like > > oVirt, OpenStack, etc, then using a custom namespace is not going > > to be a suitable approach. > > And I don't think it would be palatable to add N 'text' options to the > XML that map to the same N 'text' options in the mount command. So this > was the next best option as far as I saw it. That'd be one of those > never ending tasks to add the favorite option. 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 1/9/19 12:40 PM, Daniel P. Berrangé wrote: > On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote: >> >> >> On 1/9/19 12:09 PM, Daniel P. Berrangé wrote: >>> On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote: >>>> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html >>>> >>>> Kept the subject the same, but the concept has been adjusted to follow >>>> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. >>>> This series adds both the NFS and the RBD adjustments that were essentially >>>> in the referenced series from review. >>> >>> Can you give any info about what motivated this addition. >> >> There's a bz: >> >> https://bugzilla.redhat.com/show_bug.cgi?id=1584663 >> >> which along the way has had private comments - I probably should add the >> bz to at least one of the patches, but I think because of the (probably >> unnecessary) private comments in the bz, I was hesitant to do so. >> >> I tried a different mechanism and Jano pointed out: >> >> https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html >> >> that previous attempts were rejected due to the arbitrary text. > > Ok, from the rather limited information in the BZ above, it is clear > that this feature is required to be production supported, so that > rules out use of private XML namespaces as a viable solution. > > I agree with Jano's rejected of arbitrary mount option passthrough in > v1 too though. I understand the rejection and I also found namespaces to not be very user or developer friendly. Even less easy to understand were those "other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard place scenario. It's no small wonder why the patches that were rejected in June 2014 had no attempted followup that I found - it just didn't seem worth the effort and using their own private code to suit their own needs was far more simple! > > I think the right answer involves multiple approaches in the XML. > > For the NFS pool, we should have an explicit way to request the NFS > protocol version in the XML. You mean mount option "nfsvers=n"? > > For the general nosuid, nodev flags, I think we can do something like we > have for the <features/> element in the domain XML. > So that means to me that every option currently possible in mount would need to be listed. Doesn't that feel excessive? Not that mount changes that often, but I would think it's awfully painful to take that route. Especially for some of the "more complicated" mount options. > Or on second thoughts, the only reason for the storage pools mounts is to > provide VM disk images, so we should just unconditionally always set nosuid > & nodev when running mount. No storage volume we report should be setuid or > be a device node. If the NFS mount has a directory tree for container > filesystems, then the previously discussed virFileSystem APIs should be > actually implemented. That'd be the really simple simple fix at least for nosuid and nodev. I didn't approach it from the viewpoint of providing VM disk images as it seemed to be more "general" provide the ability to pick-n-choose which mount option to use. Side bar, I also have this weird recollection about usage of rsize=n and/or wsize=n in some previous NFS issue I've chased (I think it was at my previous employer). There's also mention of noexec in one of the private comments, so while nosuid and nodev would be easier, it still doesn't cover everything noted in the bz. > > We could none the less also still have a private XML namespace for > doing arbitrary mount argument passthrough, but that shouldn't be > the solution for this particular BZ. It is just a way to get a quick > workaround for an option while we decide how to model the desired > new mount option explicitly in the XML, as we do for QEMU options. > Does it become more work than it's worth though ;-) How large of a wall needs to be built to stop the flow of code ;-) John >> >>> >>> Anything that is implemented via a separate XML namespace is >>> generally considered >>> >>> "unsupported, you're on your own when it breaks" >>> >>> so if this feature is neeeded for any management application like >>> oVirt, OpenStack, etc, then using a custom namespace is not going >>> to be a suitable approach. >> >> And I don't think it would be palatable to add N 'text' options to the >> XML that map to the same N 'text' options in the mount command. So this >> was the next best option as far as I saw it. That'd be one of those >> never ending tasks to add the favorite option. > > Regards, > Daniel > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jan 09, 2019 at 01:26:19PM -0500, John Ferlan wrote: > > > On 1/9/19 12:40 PM, Daniel P. Berrangé wrote: > > On Wed, Jan 09, 2019 at 12:31:02PM -0500, John Ferlan wrote: > >> > >> > >> On 1/9/19 12:09 PM, Daniel P. Berrangé wrote: > >>> On Tue, Jan 08, 2019 at 12:52:20PM -0500, John Ferlan wrote: > >>>> v1: https://www.redhat.com/archives/libvir-list/2018-December/msg00558.html > >>>> > >>>> Kept the subject the same, but the concept has been adjusted to follow > >>>> issues pointed out by jtomko vis-a-vis allowing arbitrary options via XML. > >>>> This series adds both the NFS and the RBD adjustments that were essentially > >>>> in the referenced series from review. > >>> > >>> Can you give any info about what motivated this addition. > >> > >> There's a bz: > >> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1584663 > >> > >> which along the way has had private comments - I probably should add the > >> bz to at least one of the patches, but I think because of the (probably > >> unnecessary) private comments in the bz, I was hesitant to do so. > >> > >> I tried a different mechanism and Jano pointed out: > >> > >> https://www.redhat.com/archives/libvir-list/2018-December/msg00667.html > >> > >> that previous attempts were rejected due to the arbitrary text. > > > > Ok, from the rather limited information in the BZ above, it is clear > > that this feature is required to be production supported, so that > > rules out use of private XML namespaces as a viable solution. > > > > I agree with Jano's rejected of arbitrary mount option passthrough in > > v1 too though. > > I understand the rejection and I also found namespaces to not be very > user or developer friendly. Even less easy to understand were those > "other' namespaces using "ns0" and/or "ns1". Proverbial rock and hard > place scenario. > > It's no small wonder why the patches that were rejected in June 2014 had > no attempted followup that I found - it just didn't seem worth the > effort and using their own private code to suit their own needs was far > more simple! > > > > > I think the right answer involves multiple approaches in the XML. > > > > For the NFS pool, we should have an explicit way to request the NFS > > protocol version in the XML. > > You mean mount option "nfsvers=n"? Yes, which would be set using something like <source> .... <protocol ver="4"/> </source> > > For the general nosuid, nodev flags, I think we can do something like we > > have for the <features/> element in the domain XML. > > > > So that means to me that every option currently possible in mount would > need to be listed. Doesn't that feel excessive? Not that mount changes > that often, but I would think it's awfully painful to take that route. > Especially for some of the "more complicated" mount options. The mount command options are a tiny, trivial set compared to QEMU command line options. The goal here is not to have simple libvirt code, rather is to have portable & simple application code. Libvirt exists to provide an abstraction layer over OS specific commands line "mount" which can have different syntax on each OS. eg FreeBSD mount command options may differ from Linux mount command options. I'm not suggesting implementing support for every single mount option though. Only those that we actually have a compelling reason to support, just as we've not implemented every single QEMU arg. > > Or on second thoughts, the only reason for the storage pools mounts is to > > provide VM disk images, so we should just unconditionally always set nosuid > > & nodev when running mount. No storage volume we report should be setuid or > > be a device node. If the NFS mount has a directory tree for container > > filesystems, then the previously discussed virFileSystem APIs should be > > actually implemented. > > That'd be the really simple simple fix at least for nosuid and nodev. I > didn't approach it from the viewpoint of providing VM disk images as it > seemed to be more "general" provide the ability to pick-n-choose which > mount option to use. Side bar, I also have this weird recollection about > usage of rsize=n and/or wsize=n in some previous NFS issue I've chased > (I think it was at my previous employer). That was needed in NFSv2, but in modern NFSv3/4 IIUC the server decides those values. > There's also mention of noexec in one of the private comments, so while > nosuid and nodev would be easier, it still doesn't cover everything > noted in the bz. noexec would be reasonable to turn on by default too IMHO. > > We could none the less also still have a private XML namespace for > > doing arbitrary mount argument passthrough, but that shouldn't be > > the solution for this particular BZ. It is just a way to get a quick > > workaround for an option while we decide how to model the desired > > new mount option explicitly in the XML, as we do for QEMU options. > > > > Does it become more work than it's worth though ;-) How large of a wall > needs to be built to stop the flow of code ;-) 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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[...] >>> For the general nosuid, nodev flags, I think we can do something like we >>> have for the <features/> element in the domain XML. >>> >> >> So that means to me that every option currently possible in mount would >> need to be listed. Doesn't that feel excessive? Not that mount changes >> that often, but I would think it's awfully painful to take that route. >> Especially for some of the "more complicated" mount options. > > The mount command options are a tiny, trivial set compared to QEMU > command line options. The goal here is not to have simple libvirt > code, rather is to have portable & simple application code. Libvirt > exists to provide an abstraction layer over OS specific commands > line "mount" which can have different syntax on each OS. eg FreeBSD > mount command options may differ from Linux mount command options. And this last line is the biggest concern over providing or "by default" adding "-o nosuid,nodev,noexec" for NFS mount command line. Secondary to that is the "what if" by doing so we break some existing configuration for some reason that was making use of a pool volume in some really strange manner that we didn't expect just because one of those mount options wasn't set by default. Not that it seems the 3 mentioned ones would cause such pain, but the precedence of adding options by default in general. > > I'm not suggesting implementing support for every single mount > option though. Only those that we actually have a compelling > reason to support, just as we've not implemented every single > QEMU arg. > Understood. Of course as soon a couple are implemented, someone else's favorite is desired, etc. It's the picking and choosing which are compelling enough to support vs. just allowing or even providing a supportable mechanism to allow arbitrary command options vs. implementing "some" and leaving the rest to be added via namespace. Right now the "some" are: nfsvers='n' [add only if new XML exists] nosuid nodev noexec and maybe additionally 'ro' if [new] <readonly/> was set. Just trying to get all the "parameters" set before starting a v3... John [...] -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.