| Message ID | 20250409152335.890665-3-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add uniquifier to storage pool path | expand |
On Wed, Apr 09, 2025 at 11:23:35AM +0100, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > I'd like to be able to run more than one instance of kdevops per > physical host. Currently the kdevops guestfs set-up steers all We hit this problem recently too. Adding Swarna to the thread. > storage pool activity into ${STORAGE_POOL}/kdevops/guestfs, which > means there's a good change that two different logged-in users will > create virtual machines whose names (and block devices) conflict. > > So far I haven't been able to get the other storage pool-related > settings to add sufficient uniqueness to prevent this conflict. I think I would like to have user defined pools and images in a user directory rather than the common/system wide pool and image directories in /var/lib. Then, besides the pool, I think we also need a unique user name for the libvirt configuration file (CUSTOM_SOURCE) [1], and the image name returned by virt-builder should be unique too. Specially when the image is located at the $USER directory rather than in the common /var/lib path. [1] CUSTOM_SOURCE="/etc/virt-builder/repos.d/kdevops-custom-images-${OS_VERSION}.conf" [2] sudo virt-builder -l | grep debian debian-13-generic-amd64-daily x86_64 trixie/sid In addition, shouldn't we also control virt-builder image generation without sudo if a $USER image is found? > > Instead, replace the "kdevops" string in the storage pool path with > the name of the user account running kdevops. > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > kconfigs/Kconfig.libvirt | 2 +- > scripts/bringup_guestfs.sh | 2 +- > scripts/destroy_guestfs.sh | 2 +- > scripts/guestfs.Makefile | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt > index cba8abf1e24b..74a93f177732 100644 > --- a/kconfigs/Kconfig.libvirt > +++ b/kconfigs/Kconfig.libvirt > @@ -167,7 +167,7 @@ config LIBVIRT_STORAGE_POOL_PATH > config KDEVOPS_STORAGE_POOL_PATH > string > output yaml > - default "{{ libvirt_storage_pool_path }}/kdevops" > + default "{{ libvirt_storage_pool_path }}/{{ kdevops_storage_pool_user }}" We don't need to set up a user pool path in the default case, right? Storage pool would be placed in the common /var/lib/libvirt/images directory and all users would be able to access that. I think for this case we don't want to use the system wide common directory but rather user path. > > config QEMU_BIN_PATH > string > diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh > index bc0176f8f5b4..6019241c8d99 100755 > --- a/scripts/bringup_guestfs.sh > +++ b/scripts/bringup_guestfs.sh > @@ -15,7 +15,7 @@ if [ "${CONFIG_LIBVIRT_EXTRA_DRIVE_FORMAT_RAW}" = "y" ]; then > IMG_FMT="raw" > fi > STORAGETOPDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}" > -STORAGEDIR="${STORAGETOPDIR}/kdevops/guestfs" > +STORAGEDIR="${STORAGETOPDIR}/${USER}/guestfs" I would rather come here already with either system wide pool or user being assigned properly to the STORAGETOPDIR. > QEMU_GROUP=$CONFIG_LIBVIRT_QEMU_GROUP > GUESTFSDIR="${TOPDIR}/guestfs" > OS_VERSION=${CONFIG_VIRT_BUILDER_OS_VERSION} > diff --git a/scripts/destroy_guestfs.sh b/scripts/destroy_guestfs.sh > index ee5dc2b57d6d..8cf58e4d91c8 100755 > --- a/scripts/destroy_guestfs.sh > +++ b/scripts/destroy_guestfs.sh > @@ -7,7 +7,7 @@ source ${TOPDIR}/scripts/lib.sh > > export LIBVIRT_DEFAULT_URI=$CONFIG_LIBVIRT_URI > > -STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/kdevops/guestfs" > +STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/${USER}/guestfs" > GUESTFSDIR="${TOPDIR}/guestfs" > > if [ -f "$GUESTFSDIR/kdevops_nodes.yaml" ]; then > diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile > index e1cf25d62d04..e0a7b4e5341c 100644 > --- a/scripts/guestfs.Makefile > +++ b/scripts/guestfs.Makefile > @@ -104,4 +104,4 @@ destroy_guestfs: > PHONY += destroy_guestfs > > cleancache: > - $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/kdevops/guestfs/base_images/* > + $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/$(USER)/guestfs/base_images/* > -- > 2.49.0 >
On 4/10/25 5:11 AM, Daniel Gomez wrote: > On Wed, Apr 09, 2025 at 11:23:35AM +0100, cel@kernel.org wrote: >> From: Chuck Lever <chuck.lever@oracle.com> >> >> I'd like to be able to run more than one instance of kdevops per >> physical host. Currently the kdevops guestfs set-up steers all > > We hit this problem recently too. Adding Swarna to the thread. > >> storage pool activity into ${STORAGE_POOL}/kdevops/guestfs, which >> means there's a good change that two different logged-in users will >> create virtual machines whose names (and block devices) conflict. >> >> So far I haven't been able to get the other storage pool-related >> settings to add sufficient uniqueness to prevent this conflict. > > I think I would like to have user defined pools and images in a user directory > rather than the common/system wide pool and image directories in /var/lib. On my lab systems, /var/lib/libvirt/images is a file system on an NVMe add-on card. A per-user directory is created there for both base images and guest disks. I think sticking close to the directory structure that is expected by the distro packaging for libvirt would help kdevops reuse that instead of inventing our own. > Then, besides the pool, I think we also need a unique user name for the > libvirt configuration file (CUSTOM_SOURCE) [1], and the image name returned by > virt-builder should be unique too. Specially when the image is located at the > $USER directory rather than in the common /var/lib path. > > [1] CUSTOM_SOURCE="/etc/virt-builder/repos.d/kdevops-custom-images-${OS_VERSION}.conf" > > [2] > sudo virt-builder -l | grep debian > debian-13-generic-amd64-daily x86_64 trixie/sid > > In addition, shouldn't we also control virt-builder image generation without > sudo if a $USER image is found? Removing an unnecessary privilege escalation is a Good Thing (tm). But doesn't virt-builder do a path search for base images? kdevops could leverage that, maybe. >> Instead, replace the "kdevops" string in the storage pool path with >> the name of the user account running kdevops. >> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> kconfigs/Kconfig.libvirt | 2 +- >> scripts/bringup_guestfs.sh | 2 +- >> scripts/destroy_guestfs.sh | 2 +- >> scripts/guestfs.Makefile | 2 +- >> 4 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt >> index cba8abf1e24b..74a93f177732 100644 >> --- a/kconfigs/Kconfig.libvirt >> +++ b/kconfigs/Kconfig.libvirt >> @@ -167,7 +167,7 @@ config LIBVIRT_STORAGE_POOL_PATH >> config KDEVOPS_STORAGE_POOL_PATH >> string >> output yaml >> - default "{{ libvirt_storage_pool_path }}/kdevops" >> + default "{{ libvirt_storage_pool_path }}/{{ kdevops_storage_pool_user }}" > > We don't need to set up a user pool path in the default case, right? Storage > pool would be placed in the common /var/lib/libvirt/images directory and all > users would be able to access that. I think for this case we don't want to use > the system wide common directory but rather user path. As above, the libvirt directory might be accelerated storage. My user directories are on NFS or even an old-school SATA SSD. These work fine for virtualization but are slower than I prefer. I'm not saying a special user path is bad, but I would like to see both usage scenarios ( /var/lib/libvirt/images and a user path ) continue to be supported somehow. >> config QEMU_BIN_PATH >> string >> diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh >> index bc0176f8f5b4..6019241c8d99 100755 >> --- a/scripts/bringup_guestfs.sh >> +++ b/scripts/bringup_guestfs.sh >> @@ -15,7 +15,7 @@ if [ "${CONFIG_LIBVIRT_EXTRA_DRIVE_FORMAT_RAW}" = "y" ]; then >> IMG_FMT="raw" >> fi >> STORAGETOPDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}" >> -STORAGEDIR="${STORAGETOPDIR}/kdevops/guestfs" >> +STORAGEDIR="${STORAGETOPDIR}/${USER}/guestfs" > > I would rather come here already with either system wide pool or user being > assigned properly to the STORAGETOPDIR. +1 So, I can continue using this rickety old patch in my lab, and wait for you and Swarna to propose patches for some of these deeper changes. >> QEMU_GROUP=$CONFIG_LIBVIRT_QEMU_GROUP >> GUESTFSDIR="${TOPDIR}/guestfs" >> OS_VERSION=${CONFIG_VIRT_BUILDER_OS_VERSION} >> diff --git a/scripts/destroy_guestfs.sh b/scripts/destroy_guestfs.sh >> index ee5dc2b57d6d..8cf58e4d91c8 100755 >> --- a/scripts/destroy_guestfs.sh >> +++ b/scripts/destroy_guestfs.sh >> @@ -7,7 +7,7 @@ source ${TOPDIR}/scripts/lib.sh >> >> export LIBVIRT_DEFAULT_URI=$CONFIG_LIBVIRT_URI >> >> -STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/kdevops/guestfs" >> +STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/${USER}/guestfs" >> GUESTFSDIR="${TOPDIR}/guestfs" >> >> if [ -f "$GUESTFSDIR/kdevops_nodes.yaml" ]; then >> diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile >> index e1cf25d62d04..e0a7b4e5341c 100644 >> --- a/scripts/guestfs.Makefile >> +++ b/scripts/guestfs.Makefile >> @@ -104,4 +104,4 @@ destroy_guestfs: >> PHONY += destroy_guestfs >> >> cleancache: >> - $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/kdevops/guestfs/base_images/* >> + $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/$(USER)/guestfs/base_images/* >> -- >> 2.49.0 >>
On Thu, Apr 10, 2025 at 11:18:02AM +0100, Chuck Lever wrote: > On 4/10/25 5:11 AM, Daniel Gomez wrote: > > On Wed, Apr 09, 2025 at 11:23:35AM +0100, cel@kernel.org wrote: > >> From: Chuck Lever <chuck.lever@oracle.com> > >> > >> I'd like to be able to run more than one instance of kdevops per > >> physical host. Currently the kdevops guestfs set-up steers all > > > > We hit this problem recently too. Adding Swarna to the thread. > > > >> storage pool activity into ${STORAGE_POOL}/kdevops/guestfs, which > >> means there's a good change that two different logged-in users will > >> create virtual machines whose names (and block devices) conflict. > >> > >> So far I haven't been able to get the other storage pool-related > >> settings to add sufficient uniqueness to prevent this conflict. > > > > I think I would like to have user defined pools and images in a user directory > > rather than the common/system wide pool and image directories in /var/lib. > > On my lab systems, /var/lib/libvirt/images is a file system on an NVMe > add-on card. A per-user directory is created there for both base images > and guest disks. I think sticking close to the directory structure that > is expected by the distro packaging for libvirt would help kdevops reuse > that instead of inventing our own. Makes sense. > > > > Then, besides the pool, I think we also need a unique user name for the > > libvirt configuration file (CUSTOM_SOURCE) [1], and the image name returned by > > virt-builder should be unique too. Specially when the image is located at the > > $USER directory rather than in the common /var/lib path. > > > > [1] CUSTOM_SOURCE="/etc/virt-builder/repos.d/kdevops-custom-images-${OS_VERSION}.conf" > > > > [2] > > sudo virt-builder -l | grep debian > > debian-13-generic-amd64-daily x86_64 trixie/sid > > > > In addition, shouldn't we also control virt-builder image generation without > > sudo if a $USER image is found? > > Removing an unnecessary privilege escalation is a Good Thing (tm). > > But doesn't virt-builder do a path search for base images? kdevops could > leverage that, maybe. My suggestion only makes sense for directories with user permission only. If we stick to /var/lib, we still need the sudo. Please, ignore my comment. BTW, I've just found this in virt-builder documentation: SOURCES OF TEMPLATES virt-builder reads the available sources from configuration files, with the .conf extension and located in the following paths: • $XDG_CONFIG_HOME/virt-builder/repos.d/ ($XDG_CONFIG_HOME is $HOME/.config if not set). • $VIRT_BUILDER_DIRS/virt-builder/repos.d/ (where $VIRT_BUILDER_DIRS means any of the directories in that environment variable, or just / etc if not set). So, I think we can potentially use the home/user directory instead of the system /etc for the virt-builder .conf file. This is just a project TODO, not necessary releated to this patch series. > > > >> Instead, replace the "kdevops" string in the storage pool path with > >> the name of the user account running kdevops. > >> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> kconfigs/Kconfig.libvirt | 2 +- > >> scripts/bringup_guestfs.sh | 2 +- > >> scripts/destroy_guestfs.sh | 2 +- > >> scripts/guestfs.Makefile | 2 +- > >> 4 files changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt > >> index cba8abf1e24b..74a93f177732 100644 > >> --- a/kconfigs/Kconfig.libvirt > >> +++ b/kconfigs/Kconfig.libvirt > >> @@ -167,7 +167,7 @@ config LIBVIRT_STORAGE_POOL_PATH > >> config KDEVOPS_STORAGE_POOL_PATH > >> string > >> output yaml > >> - default "{{ libvirt_storage_pool_path }}/kdevops" > >> + default "{{ libvirt_storage_pool_path }}/{{ kdevops_storage_pool_user }}" > > > > We don't need to set up a user pool path in the default case, right? Storage > > pool would be placed in the common /var/lib/libvirt/images directory and all > > users would be able to access that. I think for this case we don't want to use > > the system wide common directory but rather user path. > > As above, the libvirt directory might be accelerated storage. My user > directories are on NFS or even an old-school SATA SSD. These work fine > for virtualization but are slower than I prefer. > > I'm not saying a special user path is bad, but I would like to see both > usage scenarios ( /var/lib/libvirt/images and a user path ) continue to > be supported somehow. > > > >> config QEMU_BIN_PATH > >> string > >> diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh > >> index bc0176f8f5b4..6019241c8d99 100755 > >> --- a/scripts/bringup_guestfs.sh > >> +++ b/scripts/bringup_guestfs.sh > >> @@ -15,7 +15,7 @@ if [ "${CONFIG_LIBVIRT_EXTRA_DRIVE_FORMAT_RAW}" = "y" ]; then > >> IMG_FMT="raw" > >> fi > >> STORAGETOPDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}" > >> -STORAGEDIR="${STORAGETOPDIR}/kdevops/guestfs" > >> +STORAGEDIR="${STORAGETOPDIR}/${USER}/guestfs" > > > > I would rather come here already with either system wide pool or user being > > assigned properly to the STORAGETOPDIR. > > +1 > > So, I can continue using this rickety old patch in my lab, and wait for > you and Swarna to propose patches for some of these deeper changes. To clarify, I was confusing 2 topics: 1. Allow kdevops to use a common, system-wide path that can be shared by multiple users. I believe this series addresses that, and it's something we likely want, especially for large systems with multiple kdevops instances running in parallel. 2. Prevent kdevops instances from using user image directories created by other users. For example, if user1 creates images in $HOME/libvirt/images, user2 may see these images available but lack the necessary permissions to use them (and this is what I thought these patches were addressing). So, if you are still up for it, you can send a v2 including the missing conflicts you mentioned in the patch description. Can you explain how are these changes working at your deployment? I'm not sure I fully understand whether they are sufficient to make kdevops run per user. These are the changes I believe are necessary to get it working ("the other storage pool-related settings" comment in your commit message). Let me know what you think: Regarding the STORAGEDIR change above. I think we need to decouple the base image from it and "promote" the $USER changes to Ansible. The reason to decouple is that we want to reuse the same base image for multiple users while allowing every user to have it's own custom_image (and disks). Then, I think the custom image name and path needs to include the user for uniqueness and avoid conflicts. * File: /etc/virt-builder/repos.d/kdevops-custom-images-debian-13-generic-amd64-daily.conf [local] uri=file:////var/lib/libvirt/images/kdevops/guestfs/custom_images/debian-13-generic-amd64-daily/index proxy=off This file name at /etc needs to have the $USER (preferably in Ansible) string in the name. And point to the $USER but system-wide path (already addressed in the changes). * File: /var/lib/libvirt/images/kdevops/guestfs/custom_images/debian-13-generic-amd64-daily/index [debian-13-generic-amd64-daily] name=trixie/sid osinfo=unknown file=debian-13-generic-amd64-daily.raw arch=x86_64 checksum[sha512]=e93cc7f4fa9f2e9a1a166b... revision=1 format=raw size=3221225472 compressed_size=3221225472 expand=/dev/sda1 This file needs to change it's name in braquets to include uniqueness, so when running virt-builder --list every user has it's own custom image: sudo virt-builder --list | grep debian debian-13-generic-amd64-daily x86_64 trixie/sid > > > >> QEMU_GROUP=$CONFIG_LIBVIRT_QEMU_GROUP > >> GUESTFSDIR="${TOPDIR}/guestfs" > >> OS_VERSION=${CONFIG_VIRT_BUILDER_OS_VERSION} > >> diff --git a/scripts/destroy_guestfs.sh b/scripts/destroy_guestfs.sh > >> index ee5dc2b57d6d..8cf58e4d91c8 100755 > >> --- a/scripts/destroy_guestfs.sh > >> +++ b/scripts/destroy_guestfs.sh > >> @@ -7,7 +7,7 @@ source ${TOPDIR}/scripts/lib.sh > >> > >> export LIBVIRT_DEFAULT_URI=$CONFIG_LIBVIRT_URI > >> > >> -STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/kdevops/guestfs" > >> +STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/${USER}/guestfs" > >> GUESTFSDIR="${TOPDIR}/guestfs" > >> > >> if [ -f "$GUESTFSDIR/kdevops_nodes.yaml" ]; then > >> diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile > >> index e1cf25d62d04..e0a7b4e5341c 100644 > >> --- a/scripts/guestfs.Makefile > >> +++ b/scripts/guestfs.Makefile > >> @@ -104,4 +104,4 @@ destroy_guestfs: > >> PHONY += destroy_guestfs > >> > >> cleancache: > >> - $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/kdevops/guestfs/base_images/* > >> + $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/$(USER)/guestfs/base_images/* > >> -- > >> 2.49.0 > >> > > > -- > Chuck Lever
On 4/14/25 3:48 AM, Daniel Gomez wrote: > On Thu, Apr 10, 2025 at 11:18:02AM +0100, Chuck Lever wrote: >> On 4/10/25 5:11 AM, Daniel Gomez wrote: >>> On Wed, Apr 09, 2025 at 11:23:35AM +0100, cel@kernel.org wrote: >>>> From: Chuck Lever <chuck.lever@oracle.com> >>>> >>>> I'd like to be able to run more than one instance of kdevops per >>>> physical host. Currently the kdevops guestfs set-up steers all >>> >>> We hit this problem recently too. Adding Swarna to the thread. >>> >>>> storage pool activity into ${STORAGE_POOL}/kdevops/guestfs, which >>>> means there's a good change that two different logged-in users will >>>> create virtual machines whose names (and block devices) conflict. >>>> >>>> So far I haven't been able to get the other storage pool-related >>>> settings to add sufficient uniqueness to prevent this conflict. >>> >>> I think I would like to have user defined pools and images in a user directory >>> rather than the common/system wide pool and image directories in /var/lib. >> >> On my lab systems, /var/lib/libvirt/images is a file system on an NVMe >> add-on card. A per-user directory is created there for both base images >> and guest disks. I think sticking close to the directory structure that >> is expected by the distro packaging for libvirt would help kdevops reuse >> that instead of inventing our own. > > Makes sense. > >> >> >>> Then, besides the pool, I think we also need a unique user name for the >>> libvirt configuration file (CUSTOM_SOURCE) [1], and the image name returned by >>> virt-builder should be unique too. Specially when the image is located at the >>> $USER directory rather than in the common /var/lib path. >>> >>> [1] CUSTOM_SOURCE="/etc/virt-builder/repos.d/kdevops-custom-images-${OS_VERSION}.conf" >>> >>> [2] >>> sudo virt-builder -l | grep debian >>> debian-13-generic-amd64-daily x86_64 trixie/sid >>> >>> In addition, shouldn't we also control virt-builder image generation without >>> sudo if a $USER image is found? >> >> Removing an unnecessary privilege escalation is a Good Thing (tm). >> >> But doesn't virt-builder do a path search for base images? kdevops could >> leverage that, maybe. > > My suggestion only makes sense for directories with user permission only. If we > stick to /var/lib, we still need the sudo. Please, ignore my comment. Setting the sticky bit on /var/lib/libvirt/images means that an unprivileged user can create a directory there for itself. That's not to everyone's tastes, but my systems are firewalled and do not have other users on them. > BTW, I've just found this in virt-builder documentation: > > SOURCES OF TEMPLATES > virt-builder reads the available sources from configuration files, with > the .conf extension and located in the following paths: > > • $XDG_CONFIG_HOME/virt-builder/repos.d/ ($XDG_CONFIG_HOME is > $HOME/.config if not set). > > • $VIRT_BUILDER_DIRS/virt-builder/repos.d/ (where $VIRT_BUILDER_DIRS > means any of the directories in that environment variable, or just / > etc if not set). > > So, I think we can potentially use the home/user directory instead of the > system /etc for the virt-builder .conf file. This is just a project TODO, not > necessary releated to this patch series. >>>> Instead, replace the "kdevops" string in the storage pool path with >>>> the name of the user account running kdevops. >>>> >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >>>> --- >>>> kconfigs/Kconfig.libvirt | 2 +- >>>> scripts/bringup_guestfs.sh | 2 +- >>>> scripts/destroy_guestfs.sh | 2 +- >>>> scripts/guestfs.Makefile | 2 +- >>>> 4 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt >>>> index cba8abf1e24b..74a93f177732 100644 >>>> --- a/kconfigs/Kconfig.libvirt >>>> +++ b/kconfigs/Kconfig.libvirt >>>> @@ -167,7 +167,7 @@ config LIBVIRT_STORAGE_POOL_PATH >>>> config KDEVOPS_STORAGE_POOL_PATH >>>> string >>>> output yaml >>>> - default "{{ libvirt_storage_pool_path }}/kdevops" >>>> + default "{{ libvirt_storage_pool_path }}/{{ kdevops_storage_pool_user }}" >>> >>> We don't need to set up a user pool path in the default case, right? Storage >>> pool would be placed in the common /var/lib/libvirt/images directory and all >>> users would be able to access that. I think for this case we don't want to use >>> the system wide common directory but rather user path. >> >> As above, the libvirt directory might be accelerated storage. My user >> directories are on NFS or even an old-school SATA SSD. These work fine >> for virtualization but are slower than I prefer. >> >> I'm not saying a special user path is bad, but I would like to see both >> usage scenarios ( /var/lib/libvirt/images and a user path ) continue to >> be supported somehow. >> >> >>>> config QEMU_BIN_PATH >>>> string >>>> diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh >>>> index bc0176f8f5b4..6019241c8d99 100755 >>>> --- a/scripts/bringup_guestfs.sh >>>> +++ b/scripts/bringup_guestfs.sh >>>> @@ -15,7 +15,7 @@ if [ "${CONFIG_LIBVIRT_EXTRA_DRIVE_FORMAT_RAW}" = "y" ]; then >>>> IMG_FMT="raw" >>>> fi >>>> STORAGETOPDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}" >>>> -STORAGEDIR="${STORAGETOPDIR}/kdevops/guestfs" >>>> +STORAGEDIR="${STORAGETOPDIR}/${USER}/guestfs" >>> >>> I would rather come here already with either system wide pool or user being >>> assigned properly to the STORAGETOPDIR. >> >> +1 >> >> So, I can continue using this rickety old patch in my lab, and wait for >> you and Swarna to propose patches for some of these deeper changes. > > To clarify, I was confusing 2 topics: > > 1. Allow kdevops to use a common, system-wide path that can be shared by multiple > users. I believe this series addresses that, and it's something we likely > want, especially for large systems with multiple kdevops instances running in > parallel. > > 2. Prevent kdevops instances from using user image directories created by other > users. For example, if user1 creates images in $HOME/libvirt/images, user2 may > see these images available but lack the necessary permissions to use them (and > this is what I thought these patches were addressing). > > So, if you are still up for it, you can send a v2 including the missing > conflicts you mentioned in the patch description. > > Can you explain how are these changes working at your deployment? I'm not sure I > fully understand whether they are sufficient to make kdevops run per user. Agreed, I could have missed something. In my configurations so far I haven't found anything else is needed. The host systems have multiple kdevops users (say, "kdevops", "kdevops2", and "kdevops3"). Each user has a buildbot worker. For each run, the buildbot worker downloads kdevops, builds a kernel, then runs a workflow. The proposed changes enable the same kdevops .config to work on any of the three "kdevops?" user IDs without alteration. The kdevops user name is baked into the libvirt storage path. So the buildmaster is free to pick whichever worker is idle to run the next job. > These > are the changes I believe are necessary to get it working ("the other storage > pool-related settings" comment in your commit message). Let me know what you > think: > > Regarding the STORAGEDIR change above. I think we need to decouple the base > image from it and "promote" the $USER changes to Ansible. To make sure I understand: you would like to see the pathname to the base image directory be maybe entirely different than the pathname to the block device files, or even managed via virt-builder instead of by kdevops. No objection from me. > The reason to decouple > is that we want to reuse the same base image for multiple users while allowing > every user to have it's own custom_image (and disks). Then, I think the > custom image name and path needs to include the user for uniqueness and avoid > conflicts. > > * File: /etc/virt-builder/repos.d/kdevops-custom-images-debian-13-generic-amd64-daily.conf > [local] > uri=file:////var/lib/libvirt/images/kdevops/guestfs/custom_images/debian-13-generic-amd64-daily/index > proxy=off > > This file name at /etc needs to have the $USER (preferably in Ansible) string > in the name. And point to the $USER but system-wide path (already addressed in > the changes). > > * File: /var/lib/libvirt/images/kdevops/guestfs/custom_images/debian-13-generic-amd64-daily/index > [debian-13-generic-amd64-daily] > name=trixie/sid > osinfo=unknown > file=debian-13-generic-amd64-daily.raw > arch=x86_64 > checksum[sha512]=e93cc7f4fa9f2e9a1a166b... > revision=1 > format=raw > size=3221225472 > compressed_size=3221225472 > expand=/dev/sda1 > > This file needs to change it's name in braquets to include uniqueness, so when > running virt-builder --list every user has it's own custom image: > > sudo virt-builder --list | grep debian > debian-13-generic-amd64-daily x86_64 trixie/sid > >> >> >>>> QEMU_GROUP=$CONFIG_LIBVIRT_QEMU_GROUP >>>> GUESTFSDIR="${TOPDIR}/guestfs" >>>> OS_VERSION=${CONFIG_VIRT_BUILDER_OS_VERSION} >>>> diff --git a/scripts/destroy_guestfs.sh b/scripts/destroy_guestfs.sh >>>> index ee5dc2b57d6d..8cf58e4d91c8 100755 >>>> --- a/scripts/destroy_guestfs.sh >>>> +++ b/scripts/destroy_guestfs.sh >>>> @@ -7,7 +7,7 @@ source ${TOPDIR}/scripts/lib.sh >>>> >>>> export LIBVIRT_DEFAULT_URI=$CONFIG_LIBVIRT_URI >>>> >>>> -STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/kdevops/guestfs" >>>> +STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/${USER}/guestfs" >>>> GUESTFSDIR="${TOPDIR}/guestfs" >>>> >>>> if [ -f "$GUESTFSDIR/kdevops_nodes.yaml" ]; then >>>> diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile >>>> index e1cf25d62d04..e0a7b4e5341c 100644 >>>> --- a/scripts/guestfs.Makefile >>>> +++ b/scripts/guestfs.Makefile >>>> @@ -104,4 +104,4 @@ destroy_guestfs: >>>> PHONY += destroy_guestfs >>>> >>>> cleancache: >>>> - $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/kdevops/guestfs/base_images/* >>>> + $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/$(USER)/guestfs/base_images/* >>>> -- >>>> 2.49.0 >>>> >> >> >> -- >> Chuck Lever
diff --git a/kconfigs/Kconfig.libvirt b/kconfigs/Kconfig.libvirt index cba8abf1e24b..74a93f177732 100644 --- a/kconfigs/Kconfig.libvirt +++ b/kconfigs/Kconfig.libvirt @@ -167,7 +167,7 @@ config LIBVIRT_STORAGE_POOL_PATH config KDEVOPS_STORAGE_POOL_PATH string output yaml - default "{{ libvirt_storage_pool_path }}/kdevops" + default "{{ libvirt_storage_pool_path }}/{{ kdevops_storage_pool_user }}" config QEMU_BIN_PATH string diff --git a/scripts/bringup_guestfs.sh b/scripts/bringup_guestfs.sh index bc0176f8f5b4..6019241c8d99 100755 --- a/scripts/bringup_guestfs.sh +++ b/scripts/bringup_guestfs.sh @@ -15,7 +15,7 @@ if [ "${CONFIG_LIBVIRT_EXTRA_DRIVE_FORMAT_RAW}" = "y" ]; then IMG_FMT="raw" fi STORAGETOPDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}" -STORAGEDIR="${STORAGETOPDIR}/kdevops/guestfs" +STORAGEDIR="${STORAGETOPDIR}/${USER}/guestfs" QEMU_GROUP=$CONFIG_LIBVIRT_QEMU_GROUP GUESTFSDIR="${TOPDIR}/guestfs" OS_VERSION=${CONFIG_VIRT_BUILDER_OS_VERSION} diff --git a/scripts/destroy_guestfs.sh b/scripts/destroy_guestfs.sh index ee5dc2b57d6d..8cf58e4d91c8 100755 --- a/scripts/destroy_guestfs.sh +++ b/scripts/destroy_guestfs.sh @@ -7,7 +7,7 @@ source ${TOPDIR}/scripts/lib.sh export LIBVIRT_DEFAULT_URI=$CONFIG_LIBVIRT_URI -STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/kdevops/guestfs" +STORAGEDIR="${CONFIG_LIBVIRT_STORAGE_POOL_PATH}/${USER}/guestfs" GUESTFSDIR="${TOPDIR}/guestfs" if [ -f "$GUESTFSDIR/kdevops_nodes.yaml" ]; then diff --git a/scripts/guestfs.Makefile b/scripts/guestfs.Makefile index e1cf25d62d04..e0a7b4e5341c 100644 --- a/scripts/guestfs.Makefile +++ b/scripts/guestfs.Makefile @@ -104,4 +104,4 @@ destroy_guestfs: PHONY += destroy_guestfs cleancache: - $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/kdevops/guestfs/base_images/* + $(Q)rm -f $(subst ",,$(CONFIG_LIBVIRT_STORAGE_POOL_PATH))/$(USER)/guestfs/base_images/*