Re: [PATCH] storage: Linstor support
On Mon, Jan 18, 2021 at 02:12:04PM +0100, Peter Krempa wrote: > On Fri, Jan 15, 2021 at 08:16:47 +0100, Rene Peinthor wrote: > > Implement a LINSTOR backend storage driver. > > The Linstor client needs to be installed and it needs to be configured > > on the nodes used by the controller. > > > > It supports most pool/vol commands, except for pool-build/pool-delete > > and provides a block device in RAW file mode. > > Linstor supports more than just DRBD so it would also be possible to have > > it provide LVM, ZFS or NVME volumes, but the common case will be to provide > > DRBD volumes in a cluster. > > > > Sample pool XML: > > > > linstor > > > > > > libvirtgrp > > > > > > > > element must point to an already created LINSTOR > > resource-group, which is used to spawn resources/volumes. > > attribute should be the local linstor node name, > > if missing it will try to get the hosts uname and use that instead. > > > > Result volume XML sample: > > > > alpine12 > > libvirtgrp/alpine12 > > 5368709120 > > 5540028416 > > > > /dev/drbd1000 > > > > > > > > > > Signed-off-by: Rene Peinthor > > --- > > Firstly I'd like you to split the patch into more granular commits as > it's customary in our project ... > > > docs/schemas/storagepool.rng | 27 + > > docs/storage.html.in | 39 + > > include/libvirt/libvirt-storage.h | 1 + > > meson.build | 6 + > > meson_options.txt | 1 + > > po/POTFILES.in| 1 + > > src/conf/domain_conf.c| 1 + > > src/conf/storage_conf.c | 14 +- > > src/conf/storage_conf.h | 1 + > > src/conf/virstorageobj.c | 4 +-a > > Conf changes are usually separate > > > src/storage/meson.build | 25 + > > src/storage/storage_backend.c | 6 + > > src/storage/storage_backend_linstor.c | 803 ++ > > src/storage/storage_backend_linstor.h | 23 + > > src/storage/storage_backend_linstor_priv.h| 53 ++ > > src/storage/storage_driver.c | 1 + > > Implementation should also be separate > > > src/test/test_driver.c| 1 + > > tests/linstorjsondata/broken.json | 1 + > > tests/linstorjsondata/resource-group.json | 1 + > > .../linstorjsondata/resource-list-test2.json | 332 > > .../storage-pools-ssdpool.json| 72 ++ > > tests/linstorjsondata/storage-pools.json | 192 + > > tests/linstorjsondata/volume-def-list.json| 158 > > .../volume-definition-test2.json | 1 + > > tests/meson.build | 6 + > > tests/storagebackendlinstortest.c | 371 > > .../storagepoolcapsschemadata/poolcaps-fs.xml | 7 + > > .../poolcaps-full.xml | 7 + > > tests/storagepoolxml2argvtest.c | 1 + > > tests/storagepoolxml2xmlin/pool-linstor.xml | 8 + > > tests/storagevolxml2xmlin/vol-linstor.xml | 10 + > > Placing tests depends. Usually XML related tests go with the commits > implementing the parser/formatter or follow that commit. > > Other tests should be added separately. > > > tools/virsh-pool.c| 3 + > > This is adding the support for the new type so it goes where the type is > declared > > > 32 files changed, 2175 insertions(+), 2 deletions(-) > > [...] > > > diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng > > index bd24b8b8d0..9b163e611d 100644 > > --- a/docs/schemas/storagepool.rng > > +++ b/docs/schemas/storagepool.rng > > @@ -26,6 +26,7 @@ > > > > > > > > + > > > > > > > > @@ -224,6 +225,21 @@ > > > > > > > > + > > + > > + linstor > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > > > > > > > @@ -463,6 +479,17 @@ > > > > > > > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > + > > > > > > > > diff --git a/docs/storage.html.in b/docs/storage.html.in > > index b2cf343933..9130fbd180 100644 > > --- a/docs/storage.html.in > > +++ b/docs/storage.html.in > > @@ -829,5 +829,44 @@ > > > > Valid volume format types > > The valid volume types are the same as for the directory pool. > > + > > + > > +LINSTOR pool > > + > > + This provides a pool using the LINSTOR software-defined-storage. > > + LINSTOR can provide block storage devices based on DRBD or basic > > + LVM/ZFS volumes. > > + > > + > > + > > + To use LINSTOR in libvirt, setup a working
Re: [PATCH] storage: Linstor support
Hi! Thanks for the review, I applied all of your corrections and will retest the code and send a splitted version of the changes. > Also I'd like to ask you to provide a way to setup this storage on our > CI system so that we can compile-test the new driver. Well the easiest would be to use an Ubuntu VM, as Linbit provides a PPA with packages for DRBD and Linstor: https://launchpad.net/~linbit/+archive/ubuntu/linbit-drbd9-stack Add the PPA, install: drbd-dkms linstor-controller linstor-satellite linstor-client Add the node to the Linstor controller: linstor node create Add a storage pool to the node: linstor storage-pool create DfltStorPool Assuming you are only using 1 node for testing, you can create a resource-group with 1 replica: linstor resource-group create libvirtgrp --place-count 1 linstor volume-group create libvirtgrp With that you can use libvirtgrp in the pool definition and should be ready to go. It might be simpler to skip DRBD and just use the Linstor storage layer, then you don't need the drbd kernel module. And the resource-group create would look like this: linstor resource-group create libvirtgrp --place-count 1 -l storage > We'd also like to know if you are willing to continue maintaining this > storage driver, so that it doesn't become abandonware right at commit > time. Yeah sure, I'm willing to maintain it (or at least someone from Linbit will in the future). >So if you use it like this, it means that the storage must be accessible > via regular open() on the host system, is that so? (Asking because the > pool XML hints that the pool is accessed via a network protocol) Linstor will create a block device(DRBD, LVM, ZFS, NVME) on the node, that can be opened with open(), but in the case of DRBD/NVME it is some kind of network storage so I wasn't sure how this best fits in libvirt. > This should be defined via the build system once you detect where the > program is located, to prevent PATH env variable from playing a role in > which program is actually used. If it is ok I would also suggest to not let meson search for the linstor client, as said it is a pure runtime dependency and doesn't have any impact at compile time. Best regards, Rene
Re: [PATCH] storage: Linstor support
On Mon, 2021-01-18 at 14:12 +0100, Peter Krempa wrote: > On Fri, Jan 15, 2021 at 08:16:47 +0100, Rene Peinthor wrote: > > +#define LINSTORCLI "linstor" > > This should be defined via the build system once you detect where the > program is located, to prevent PATH env variable from playing a role in > which program is actually used. Note that there seems to be an agreement on moving away from performing build time checks for programs that are only used at runtime, which I believe is the case here. https://www.redhat.com/archives/libvir-list/2020-November/msg00650.html -- Andrea Bolognani / Red Hat / Virtualization
Re: [PATCH] storage: Linstor support
On Fri, Jan 15, 2021 at 08:16:47 +0100, Rene Peinthor wrote: > Implement a LINSTOR backend storage driver. > The Linstor client needs to be installed and it needs to be configured > on the nodes used by the controller. > > It supports most pool/vol commands, except for pool-build/pool-delete > and provides a block device in RAW file mode. > Linstor supports more than just DRBD so it would also be possible to have > it provide LVM, ZFS or NVME volumes, but the common case will be to provide > DRBD volumes in a cluster. > > Sample pool XML: > > linstor > > > libvirtgrp > > > > element must point to an already created LINSTOR > resource-group, which is used to spawn resources/volumes. > attribute should be the local linstor node name, > if missing it will try to get the hosts uname and use that instead. > > Result volume XML sample: > > alpine12 > libvirtgrp/alpine12 > 5368709120 > 5540028416 > > /dev/drbd1000 > > > > > Signed-off-by: Rene Peinthor > --- Firstly I'd like you to split the patch into more granular commits as it's customary in our project ... > docs/schemas/storagepool.rng | 27 + > docs/storage.html.in | 39 + > include/libvirt/libvirt-storage.h | 1 + > meson.build | 6 + > meson_options.txt | 1 + > po/POTFILES.in| 1 + > src/conf/domain_conf.c| 1 + > src/conf/storage_conf.c | 14 +- > src/conf/storage_conf.h | 1 + > src/conf/virstorageobj.c | 4 +-a Conf changes are usually separate > src/storage/meson.build | 25 + > src/storage/storage_backend.c | 6 + > src/storage/storage_backend_linstor.c | 803 ++ > src/storage/storage_backend_linstor.h | 23 + > src/storage/storage_backend_linstor_priv.h| 53 ++ > src/storage/storage_driver.c | 1 + Implementation should also be separate > src/test/test_driver.c| 1 + > tests/linstorjsondata/broken.json | 1 + > tests/linstorjsondata/resource-group.json | 1 + > .../linstorjsondata/resource-list-test2.json | 332 > .../storage-pools-ssdpool.json| 72 ++ > tests/linstorjsondata/storage-pools.json | 192 + > tests/linstorjsondata/volume-def-list.json| 158 > .../volume-definition-test2.json | 1 + > tests/meson.build | 6 + > tests/storagebackendlinstortest.c | 371 > .../storagepoolcapsschemadata/poolcaps-fs.xml | 7 + > .../poolcaps-full.xml | 7 + > tests/storagepoolxml2argvtest.c | 1 + > tests/storagepoolxml2xmlin/pool-linstor.xml | 8 + > tests/storagevolxml2xmlin/vol-linstor.xml | 10 + Placing tests depends. Usually XML related tests go with the commits implementing the parser/formatter or follow that commit. Other tests should be added separately. > tools/virsh-pool.c| 3 + This is adding the support for the new type so it goes where the type is declared > 32 files changed, 2175 insertions(+), 2 deletions(-) [...] > diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng > index bd24b8b8d0..9b163e611d 100644 > --- a/docs/schemas/storagepool.rng > +++ b/docs/schemas/storagepool.rng > @@ -26,6 +26,7 @@ > > > > + > > > > @@ -224,6 +225,21 @@ > > > > + > + > + linstor > + > + > + > + > + > + > + > + > + > + > + > + > > > > @@ -463,6 +479,17 @@ > > > > + > + > + > + > + > + > + > + > + > + > + > > > > diff --git a/docs/storage.html.in b/docs/storage.html.in > index b2cf343933..9130fbd180 100644 > --- a/docs/storage.html.in > +++ b/docs/storage.html.in > @@ -829,5 +829,44 @@ > > Valid volume format types > The valid volume types are the same as for the directory pool. > + > + > +LINSTOR pool > + > + This provides a pool using the LINSTOR software-defined-storage. > + LINSTOR can provide block storage devices based on DRBD or basic > + LVM/ZFS volumes. > + > + > + > + To use LINSTOR in libvirt, setup a working LINSTOR cluster, > documentation > + for that is in the LINSTOR Users-guide. Could you link to it? Also I'd like to ask you to provide a way to setup this storage on our CI system so that we can compile-test the new driver. We'd also like to know if you are willing to continue maintaining this storage driver, so that it doesn't become abandonware right at commit time. ...
[PATCH] storage: Linstor support
Implement a LINSTOR backend storage driver. The Linstor client needs to be installed and it needs to be configured on the nodes used by the controller. It supports most pool/vol commands, except for pool-build/pool-delete and provides a block device in RAW file mode. Linstor supports more than just DRBD so it would also be possible to have it provide LVM, ZFS or NVME volumes, but the common case will be to provide DRBD volumes in a cluster. Sample pool XML: linstor libvirtgrp element must point to an already created LINSTOR resource-group, which is used to spawn resources/volumes. attribute should be the local linstor node name, if missing it will try to get the hosts uname and use that instead. Result volume XML sample: alpine12 libvirtgrp/alpine12 5368709120 5540028416 /dev/drbd1000 Signed-off-by: Rene Peinthor --- docs/schemas/storagepool.rng | 27 + docs/storage.html.in | 39 + include/libvirt/libvirt-storage.h | 1 + meson.build | 6 + meson_options.txt | 1 + po/POTFILES.in| 1 + src/conf/domain_conf.c| 1 + src/conf/storage_conf.c | 14 +- src/conf/storage_conf.h | 1 + src/conf/virstorageobj.c | 4 +- src/storage/meson.build | 25 + src/storage/storage_backend.c | 6 + src/storage/storage_backend_linstor.c | 803 ++ src/storage/storage_backend_linstor.h | 23 + src/storage/storage_backend_linstor_priv.h| 53 ++ src/storage/storage_driver.c | 1 + src/test/test_driver.c| 1 + tests/linstorjsondata/broken.json | 1 + tests/linstorjsondata/resource-group.json | 1 + .../linstorjsondata/resource-list-test2.json | 332 .../storage-pools-ssdpool.json| 72 ++ tests/linstorjsondata/storage-pools.json | 192 + tests/linstorjsondata/volume-def-list.json| 158 .../volume-definition-test2.json | 1 + tests/meson.build | 6 + tests/storagebackendlinstortest.c | 371 .../storagepoolcapsschemadata/poolcaps-fs.xml | 7 + .../poolcaps-full.xml | 7 + tests/storagepoolxml2argvtest.c | 1 + tests/storagepoolxml2xmlin/pool-linstor.xml | 8 + tests/storagevolxml2xmlin/vol-linstor.xml | 10 + tools/virsh-pool.c| 3 + 32 files changed, 2175 insertions(+), 2 deletions(-) create mode 100644 src/storage/storage_backend_linstor.c create mode 100644 src/storage/storage_backend_linstor.h create mode 100644 src/storage/storage_backend_linstor_priv.h create mode 100644 tests/linstorjsondata/broken.json create mode 100644 tests/linstorjsondata/resource-group.json create mode 100644 tests/linstorjsondata/resource-list-test2.json create mode 100644 tests/linstorjsondata/storage-pools-ssdpool.json create mode 100644 tests/linstorjsondata/storage-pools.json create mode 100644 tests/linstorjsondata/volume-def-list.json create mode 100644 tests/linstorjsondata/volume-definition-test2.json create mode 100644 tests/storagebackendlinstortest.c create mode 100644 tests/storagepoolxml2xmlin/pool-linstor.xml create mode 100644 tests/storagevolxml2xmlin/vol-linstor.xml diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index bd24b8b8d0..9b163e611d 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -26,6 +26,7 @@ + @@ -224,6 +225,21 @@ + + + linstor + + + + + + + + + + + + @@ -463,6 +479,17 @@ + + + + + + + + + + + diff --git a/docs/storage.html.in b/docs/storage.html.in index b2cf343933..9130fbd180 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -829,5 +829,44 @@ Valid volume format types The valid volume types are the same as for the directory pool. + + +LINSTOR pool + + This provides a pool using the LINSTOR software-defined-storage. + LINSTOR can provide block storage devices based on DRBD or basic + LVM/ZFS volumes. + + + + To use LINSTOR in libvirt, setup a working LINSTOR cluster, documentation + for that is in the LINSTOR Users-guide. + And create a resource-group that will be used by libvirt, also make sure + the resource-group is setup in a way so that all nodes you want to use with libvirt + will create a resource. So either use diskless-on-remaining or make sure + replica-count is the same as you have nodes
