[PATCH v2 0/2] Add uniquifier to storage pool path

[PATCH v2 0/2] Add uniquifier to storage pool path

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Last time I posted 2/2, there were some comments like "Why don't
the existing storage pool-related Kconfig settings work?" I can't
find any currently exposed setting that will do just what I need.

So, comments and suggestions for alternatives are welcome.

Chuck Lever (2):
  guestfs: Fix definition of guestfs_storage_dir
  guestfs: Per-user storage pools

 kconfigs/Kconfig.guestfs   | 2 +-
 kconfigs/Kconfig.libvirt   | 2 +-
 scripts/bringup_guestfs.sh | 2 +-
 scripts/destroy_guestfs.sh | 2 +-
 scripts/guestfs.Makefile   | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] guestfs: Fix definition of guestfs_storage_dir

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

{{ kdevops_storage_pool_path }} already ends with "/kdevops", so
remove that string from the end of guestfs_storage_dir to ensure
the permissions checking is looking for the correct directory.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 kconfigs/Kconfig.guestfs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kconfigs/Kconfig.guestfs b/kconfigs/Kconfig.guestfs
index d309436fa7c9..460ee703fd68 100644
--- a/kconfigs/Kconfig.guestfs
+++ b/kconfigs/Kconfig.guestfs
@@ -8,7 +8,7 @@ config STORAGE_POOL_PATH
 config GUESTFS_STORAGE_DIR
 	string
 	output yaml
-	default "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
+	default "{{ kdevops_storage_pool_path }}/guestfs"
 
 config GUESTFS_BASE_IMAGE_DIR
 	string
-- 
2.49.0

[PATCH v2 2/2] guestfs: Per-user storage pools

[cel]

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

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 }}"
 
 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/*
-- 
2.49.0

Re: [PATCH v2 1/2] guestfs: Fix definition of guestfs_storage_dir

[Daniel Gomez]

On Wed, Apr 09, 2025 at 11:23:34AM +0100, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> {{ kdevops_storage_pool_path }} already ends with "/kdevops", so
> remove that string from the end of guestfs_storage_dir to ensure
> the permissions checking is looking for the correct directory.

I'm currently getting this:

storage_pool_path: "/var/lib/libvirt/images"
libvirt_storage_pool_path: "/var/lib/libvirt/images"
kdevops_storage_pool_path: "{{ libvirt_storage_pool_path }}/kdevops"
"{{ kdevops_storage_pool_path }}/kdevops/guestfs"

guestfs_storage_dir resolves into:
/var/lib/libvirt/images/kdevops/kdevops/guestfs

Shall we also cleanup the STORAGE_POOL_PATH?

kconfigs/Kconfig.guestfs:config STORAGE_POOL_PATH
playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml:  storage_pool_path: "{{ kdevops_storage_pool_path }}"
playbooks/roles/install_vagrant/tasks/main.yml:    path: "{{ storage_pool_path }}"
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:- name: Test SELinux context of {{ storage_pool_path }}
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:  command: "/usr/bin/stat -c %C {{ storage_pool_path }}"
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:  register: storage_pool_path_ctx
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:- name: Set SELinux context on {{ storage_pool_path }}
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:    target: "{{ storage_pool_path }}(/.*)?"
playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:    - storage_pool_path_ctx.stdout.find(':svirt_home_t:') == -1

I think we should also remove it and replace it with kdevops_storage_pool_path
in the above playbooks.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

> ---
>  kconfigs/Kconfig.guestfs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kconfigs/Kconfig.guestfs b/kconfigs/Kconfig.guestfs
> index d309436fa7c9..460ee703fd68 100644
> --- a/kconfigs/Kconfig.guestfs
> +++ b/kconfigs/Kconfig.guestfs
> @@ -8,7 +8,7 @@ config STORAGE_POOL_PATH
>  config GUESTFS_STORAGE_DIR
>  	string
>  	output yaml
> -	default "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
> +	default "{{ kdevops_storage_pool_path }}/guestfs"
>  
>  config GUESTFS_BASE_IMAGE_DIR
>  	string
> -- 
> 2.49.0
> 

Re: [PATCH v2 2/2] guestfs: Per-user storage pools

[Daniel Gomez]

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
> 

Re: [PATCH v2 1/2] guestfs: Fix definition of guestfs_storage_dir

[Chuck Lever]

On 4/10/25 4:41 AM, Daniel Gomez wrote:
> On Wed, Apr 09, 2025 at 11:23:34AM +0100, cel@kernel.org wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>>
>> {{ kdevops_storage_pool_path }} already ends with "/kdevops", so
>> remove that string from the end of guestfs_storage_dir to ensure
>> the permissions checking is looking for the correct directory.
> 
> I'm currently getting this:
> 
> storage_pool_path: "/var/lib/libvirt/images"
> libvirt_storage_pool_path: "/var/lib/libvirt/images"
> kdevops_storage_pool_path: "{{ libvirt_storage_pool_path }}/kdevops"
> "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
> 
> guestfs_storage_dir resolves into:
> /var/lib/libvirt/images/kdevops/kdevops/guestfs

Yes, exactly.


> Shall we also cleanup the STORAGE_POOL_PATH?

Agreed, there is plenty of unnecessary redundancy.


> kconfigs/Kconfig.guestfs:config STORAGE_POOL_PATH
> playbooks/roles/gen_nodes/templates/kdevops_nodes_split_start.j2.yaml:  storage_pool_path: "{{ kdevops_storage_pool_path }}"
> playbooks/roles/install_vagrant/tasks/main.yml:    path: "{{ storage_pool_path }}"
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:- name: Test SELinux context of {{ storage_pool_path }}
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:  command: "/usr/bin/stat -c %C {{ storage_pool_path }}"
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:  register: storage_pool_path_ctx
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:- name: Set SELinux context on {{ storage_pool_path }}
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:    target: "{{ storage_pool_path }}(/.*)?"
> playbooks/roles/libvirt_user/tasks/enable-user/redhat/main.yml:    - storage_pool_path_ctx.stdout.find(':svirt_home_t:') == -1
> 
> I think we should also remove it and replace it with kdevops_storage_pool_path
> in the above playbooks.

I'll propose a second patch that attempts to clean these up.

Not sure I want to touch install_vagrant/tasks/main.yml ... Perhaps
I should remove that role first.


>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> 
> Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
> 
>> ---
>>  kconfigs/Kconfig.guestfs | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kconfigs/Kconfig.guestfs b/kconfigs/Kconfig.guestfs
>> index d309436fa7c9..460ee703fd68 100644
>> --- a/kconfigs/Kconfig.guestfs
>> +++ b/kconfigs/Kconfig.guestfs
>> @@ -8,7 +8,7 @@ config STORAGE_POOL_PATH
>>  config GUESTFS_STORAGE_DIR
>>  	string
>>  	output yaml
>> -	default "{{ kdevops_storage_pool_path }}/kdevops/guestfs"
>> +	default "{{ kdevops_storage_pool_path }}/guestfs"
>>  
>>  config GUESTFS_BASE_IMAGE_DIR
>>  	string
>> -- 
>> 2.49.0
>>


-- 
Chuck Lever

Re: [PATCH v2 2/2] guestfs: Per-user storage pools

[Chuck Lever]

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


-- 
Chuck Lever

Re: [PATCH v2 2/2] guestfs: Per-user storage pools

[Daniel Gomez]

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

Re: [PATCH v2 2/2] guestfs: Per-user storage pools

[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


-- 
Chuck Lever