locks: Refactor locking mechanism into cmd functions

Previously, due to the coupling of the container image pull to the bootc disk code, the cache directory couldn't be locked until the image id was obtained. Now that the image ID is retrieved first in the run function, the locks can be bound to each command. Signed-off-by: Chris Kyrouac <[email protected]>
containers · May 22, 2024 · 7c4c34f · 7c4c34f
1 parent 4ccf515
commit 7c4c34f
Show file tree

Hide file tree

Showing 12 changed files with 193 additions and 158 deletions.
diff --git a/cmd/list.go b/cmd/list.go
@@ -3,9 +3,9 @@ package cmd
 import (
 	"os"
 
+	"gitlab.com/bootc-org/podman-bootc/pkg/cache"
 	"gitlab.com/bootc-org/podman-bootc/pkg/config"
 	"gitlab.com/bootc-org/podman-bootc/pkg/user"
-	"gitlab.com/bootc-org/podman-bootc/pkg/utils"
 	"gitlab.com/bootc-org/podman-bootc/pkg/vm"
 
 	"github.com/containers/common/pkg/report"
@@ -79,24 +79,34 @@ func CollectVmList(user user.User, libvirtUri string) (vmList []vm.BootcVMConfig
 	return vmList, nil
 }
 
-func getVMInfo(user user.User, libvirtUri string, imageId string) (*vm.BootcVMConfig, error) {
+func getVMInfo(user user.User, libvirtUri string, fullImageId string) (*vm.BootcVMConfig, error) {
+	cacheDir, err := cache.NewCache(fullImageId, user)
+	if err != nil {
+		return nil, err
+	}
+	err = cacheDir.Lock(cache.Shared)
+	if err != nil {
+		return nil, err
+	}
+
+	defer func() {
+		if err := cacheDir.Unlock(); err != nil {
+			logrus.Warningf("unable to unlock VM %s: %v", fullImageId, err)
+		}
+	}()
+
 	bootcVM, err := vm.NewVM(vm.NewVMParameters{
-		ImageID:    imageId,
+		ImageID:    fullImageId,
 		User:       user,
 		LibvirtUri: libvirtUri,
-		Locking:    utils.Shared,
 	})
 
 	if err != nil {
 		return nil, err
 	}
 
-	// Let's be explicit instead of relying on the defer exec order
 	defer func() {
 		bootcVM.CloseConnection()
-		if err := bootcVM.Unlock(); err != nil {
-			logrus.Warningf("unable to unlock VM %s: %v", imageId, err)
-		}
 	}()
 
 	cfg, err := bootcVM.GetConfig()

diff --git a/cmd/rm.go b/cmd/rm.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 
+	"gitlab.com/bootc-org/podman-bootc/pkg/cache"
 	"gitlab.com/bootc-org/podman-bootc/pkg/config"
 	"gitlab.com/bootc-org/podman-bootc/pkg/user"
 	"gitlab.com/bootc-org/podman-bootc/pkg/utils"
@@ -44,6 +45,7 @@ func oneOrAll() cobra.PositionalArgs {
 }
 
 func doRemove(_ *cobra.Command, args []string) error {
+
 	if removeAll {
 		return pruneAll()
 	}
@@ -57,20 +59,33 @@ func prune(id string) error {
 		return err
 	}
 
+	// take an exclusive lock on the cache directory
+	fullImageId, err := utils.FullImageIdFromPartial(id, user)
+	if err != nil {
+		return err
+	}
+	cacheDir, err := cache.NewCache(fullImageId, user)
+	if err != nil {
+		return err
+	}
+	err = cacheDir.Lock(cache.Exclusive)
+	if err != nil {
+		return err
+	}
+
 	bootcVM, err := vm.NewVM(vm.NewVMParameters{
 		ImageID:    id,
 		LibvirtUri: config.LibvirtUri,
 		User:       user,
-		Locking:    utils.Exclusive,
 	})
 	if err != nil {
 		return fmt.Errorf("unable to get VM %s: %v", id, err)
 	}
 
-	// Let's be explicit instead of relying on the defer exec order
 	defer func() {
+		// Let's be explicit instead of relying on the defer exec order
 		bootcVM.CloseConnection()
-		if err := bootcVM.Unlock(); err != nil {
+		if err := cacheDir.Unlock(); err != nil {
 			logrus.Warningf("unable to unlock VM %s: %v", id, err)
 		}
 	}()

diff --git a/cmd/run.go b/cmd/run.go
@@ -109,9 +109,16 @@ func doRun(flags *cobra.Command, args []string) error {
 		return fmt.Errorf("unable to pull container image: %w", err)
 	}
 
-	// create the cache directory
-	cache := cache.NewCache(containerImage.GetId(), user)
-	err = cache.Create()
+	// create the cacheDir directory
+	cacheDir, err := cache.NewCache(containerImage.GetId(), user)
+	if err != nil {
+		return fmt.Errorf("unable to create cache: %w", err)
+	}
+	err = cacheDir.Lock(cache.Exclusive)
+	if err != nil {
+		return err
+	}
+	err = cacheDir.Create()
 	if err != nil {
 		return fmt.Errorf("unable to create cache: %w", err)
 	}
@@ -121,13 +128,20 @@ func doRun(flags *cobra.Command, args []string) error {
 		ImageID:    containerImage.GetId(),
 		User:       user,
 		LibvirtUri: config.LibvirtUri,
-		Locking:    utils.Shared,
 	})
 
 	if err != nil {
 		return fmt.Errorf("unable to initialize VM: %w", err)
 	}
 
+	defer func() {
+		// Let's be explicit instead of relying on the defer exec order
+		bootcVM.CloseConnection()
+		if err := cacheDir.Unlock(); err != nil {
+			logrus.Warningf("unable to unlock cache %s: %v", cacheDir.ImageId, err)
+		}
+	}()
+
 	isRunning, err := bootcVM.IsRunning()
 	if err != nil {
 		return fmt.Errorf("unable to check if VM is running: %w", err)
@@ -145,7 +159,7 @@ func doRun(flags *cobra.Command, args []string) error {
 	}
 
 	// create the disk image
-	bootcDisk := bootc.NewBootcDisk(containerImage, ctx, user, cache, bustCache)
+	bootcDisk := bootc.NewBootcDisk(containerImage, ctx, user, cacheDir, bustCache)
 	err = bootcDisk.Install(vmConfig.Quiet, diskImageConfigInstance)
 
 	if err != nil {
@@ -159,14 +173,6 @@ func doRun(flags *cobra.Command, args []string) error {
 		return fmt.Errorf("unable to get free port for SSH: %w", err)
 	}
 
-	// Let's be explicit instead of relying on the defer exec order
-	defer func() {
-		bootcVM.CloseConnection()
-		if err := bootcVM.Unlock(); err != nil {
-			logrus.Warningf("unable to unlock VM %s: %v", containerImage.GetId(), err)
-		}
-	}()
-
 	cmd := args[1:]
 	err = bootcVM.Run(vm.RunVMParameters{
 		Cmd:           cmd,
@@ -189,6 +195,20 @@ func doRun(flags *cobra.Command, args []string) error {
 		return err
 	}
 
+	// done modifying the cache, so remove the Exclusive lock
+	err = cacheDir.Unlock()
+	if err != nil {
+		return fmt.Errorf("unable to unlock cache: %w", err)
+	}
+
+	// take a RO lock for the SSH connection
+	// it will be unlocked at the end of this function
+	// by the previous defer()
+	err = cacheDir.Lock(cache.Shared)
+	if err != nil {
+		return err
+	}
+
 	if !vmConfig.Background {
 		if !vmConfig.Quiet {
 			var vmConsoleWg sync.WaitGroup
@@ -230,6 +250,21 @@ func doRun(flags *cobra.Command, args []string) error {
 
 		// Always remove when executing a command
 		if vmConfig.RemoveVm || len(cmd) > 0 {
+			// remove the RO lock
+			err = cacheDir.Unlock()
+			if err != nil {
+				return err
+			}
+
+
+			// take an exclusive lock to remove the VM
+			// it will be unlocked at the end of this function
+			// by the previous defer()
+			err = cacheDir.Lock(cache.Exclusive)
+			if err != nil {
+				return err
+			}
+
 			err = bootcVM.Delete() //delete the VM, but keep the disk image
 			if err != nil {
 				return fmt.Errorf("unable to remove VM from cache: %w", err)

diff --git a/cmd/ssh.go b/cmd/ssh.go
@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"gitlab.com/bootc-org/podman-bootc/pkg/cache"
 	"gitlab.com/bootc-org/podman-bootc/pkg/config"
 	"gitlab.com/bootc-org/podman-bootc/pkg/user"
 	"gitlab.com/bootc-org/podman-bootc/pkg/utils"
@@ -32,21 +33,34 @@ func doSsh(_ *cobra.Command, args []string) error {
 
 	id := args[0]
 
+	//take a read only lock on the cache directory
+	fullImageId, err := utils.FullImageIdFromPartial(id, user)
+	if err != nil {
+		return err
+	}
+	cacheDir, err := cache.NewCache(fullImageId, user)
+	if err != nil {
+		return err
+	}
+	err = cacheDir.Lock(cache.Shared)
+	if err != nil {
+		return err
+	}
+
 	vm, err := vm.NewVM(vm.NewVMParameters{
 		ImageID:    id,
 		User:       user,
 		LibvirtUri: config.LibvirtUri,
-		Locking:    utils.Shared,
 	})
 
 	if err != nil {
 		return err
 	}
 
-	// Let's be explicit instead of relying on the defer exec order
 	defer func() {
+		// Let's be explicit instead of relying on the defer exec order
 		vm.CloseConnection()
-		if err := vm.Unlock(); err != nil {
+		if err := cacheDir.Unlock(); err != nil {
 			logrus.Warningf("unable to unlock VM %s: %v", id, err)
 		}
 	}()

diff --git a/cmd/stop.go b/cmd/stop.go
@@ -1,6 +1,7 @@
 package cmd
 
 import (
+	"gitlab.com/bootc-org/podman-bootc/pkg/cache"
 	"gitlab.com/bootc-org/podman-bootc/pkg/config"
 	"gitlab.com/bootc-org/podman-bootc/pkg/user"
 	"gitlab.com/bootc-org/podman-bootc/pkg/utils"
@@ -23,26 +24,40 @@ func init() {
 }
 
 func doStop(_ *cobra.Command, args []string) (err error) {
+	id := args[0]
+
 	user, err := user.NewUser()
 	if err != nil {
 		return err
 	}
 
-	id := args[0]
+	//take an exclusive lock on the cache directory
+	fullImageId, err := utils.FullImageIdFromPartial(id, user)
+	if err != nil {
+		return err
+	}
+	cacheDir, err := cache.NewCache(fullImageId, user)
+	if err != nil {
+		return err
+	}
+	err = cacheDir.Lock(cache.Exclusive)
+	if err != nil {
+		return err
+	}
+
 	bootcVM, err := vm.NewVM(vm.NewVMParameters{
 		ImageID:    id,
 		LibvirtUri: config.LibvirtUri,
 		User:       user,
-		Locking:    utils.Exclusive,
 	})
 	if err != nil {
 		return err
 	}
 
-	// Let's be explicit instead of relying on the defer exec order
 	defer func() {
+		// Let's be explicit instead of relying on the defer exec order
 		bootcVM.CloseConnection()
-		if err := bootcVM.Unlock(); err != nil {
+		if err := cacheDir.Unlock(); err != nil {
 			logrus.Warningf("unable to unlock VM %s: %v", id, err)
 		}
 	}()