Skip to content

disk: Recreate the bootc disk image when passed certain parameters #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

disk: Recreate the bootc disk image when passed certain parameters #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff78afc
disk: Refactor image pull and cache dir creation
ckyrouac May 15, 2024
4ccf515
disk: Recreate the bootc disk image when passed certain parameters
ckyrouac May 14, 2024
0521272
locks: Refactor locking mechanism into cmd functions
ckyrouac May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions cmd/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Comment on lines 101 to 112
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootcVM.GetConfig() is not holding any lock while reading the cache,

Expand Down
21 changes: 18 additions & 3 deletions cmd/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -44,6 +45,7 @@ func oneOrAll() cobra.PositionalArgs {
}

func doRemove(_ *cobra.Command, args []string) error {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

if removeAll {
return pruneAll()
}
Expand All @@ -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)
}
}()
Comment on lines +75 to 91
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if vm.NewVM() returns an error, cacheDir never unlocks

Expand Down
98 changes: 82 additions & 16 deletions cmd/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import (
"time"

"gitlab.com/bootc-org/podman-bootc/pkg/bootc"
"gitlab.com/bootc-org/podman-bootc/pkg/cache"
"gitlab.com/bootc-org/podman-bootc/pkg/config"
"gitlab.com/bootc-org/podman-bootc/pkg/container"
"gitlab.com/bootc-org/podman-bootc/pkg/user"
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
"gitlab.com/bootc-org/podman-bootc/pkg/vm"
Expand Down Expand Up @@ -100,41 +102,77 @@ func doRun(flags *cobra.Command, args []string) error {
return err
}

// create the disk image
idOrName := args[0]
bootcDisk := bootc.NewBootcDisk(idOrName, ctx, user)
err = bootcDisk.Install(vmConfig.Quiet, diskImageConfigInstance)

// pull the container image
containerImage := container.NewContainerImage(args[0], ctx)
err = containerImage.Pull()
if err != nil {
return fmt.Errorf("unable to install bootc image: %w", err)
return fmt.Errorf("unable to pull container image: %w", err)
}

//start the VM
println("Booting the VM...")
sshPort, err := utils.GetFreeLocalTcpPort()
// create the cacheDir directory
cacheDir, err := cache.NewCache(containerImage.GetId(), user)
if err != nil {
return fmt.Errorf("unable to get free port for SSH: %w", err)
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)
}

// check if the vm is already running
bootcVM, err := vm.NewVM(vm.NewVMParameters{
ImageID: bootcDisk.GetImageId(),
ImageID: containerImage.GetId(),
User: user,
LibvirtUri: config.LibvirtUri,
Locking: utils.Shared,
})

if err != nil {
return fmt.Errorf("unable to initialize VM: %w", 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 {
logrus.Warningf("unable to unlock VM %s: %v", bootcDisk.GetImageId(), err)
if err := cacheDir.Unlock(); err != nil {
logrus.Warningf("unable to unlock cache %s: %v", cacheDir.ImageId, err)
}
}()

Comment on lines 132 to 144
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheDir never unlocks if vm.NewVM() returns an error

isRunning, err := bootcVM.IsRunning()
if err != nil {
return fmt.Errorf("unable to check if VM is running: %w", err)
}
if isRunning {
return fmt.Errorf("VM already running, use the ssh command to connect to it")
}

// if any of these parameters are set, we need to rebuild the disk image if one exists
bustCache := false
if diskImageConfigInstance.DiskSize != "" ||
diskImageConfigInstance.RootSizeMax != "" ||
diskImageConfigInstance.Filesystem != "" {
bustCache = true
}

// create the disk image
bootcDisk := bootc.NewBootcDisk(containerImage, ctx, user, cacheDir, bustCache)
err = bootcDisk.Install(vmConfig.Quiet, diskImageConfigInstance)

if err != nil {
return fmt.Errorf("unable to install bootc image: %w", err)
}

//start the VM
println("Booting the VM...")
sshPort, err := utils.GetFreeLocalTcpPort()
if err != nil {
return fmt.Errorf("unable to get free port for SSH: %w", err)
}

cmd := args[1:]
err = bootcVM.Run(vm.RunVMParameters{
Cmd: cmd,
Expand All @@ -153,7 +191,21 @@ func doRun(flags *cobra.Command, args []string) error {
}

// write down the config file
if err = bootcVM.WriteConfig(*bootcDisk); err != nil {
if err = bootcVM.WriteConfig(*bootcDisk, containerImage); err != nil {
Comment on lines -156 to +194
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also changing the cache using a shared lock instead of an exclusive one, however currently this is incorrect in the main branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to the previous comment, I'd prefer to fix what's in main in a separate MR and keep this to what's needed for the disk size fix.

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
}

Expand Down Expand Up @@ -198,6 +250,20 @@ 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)
Expand Down
20 changes: 17 additions & 3 deletions cmd/ssh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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)
}
Comment on lines 50 to 65
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheDir never unlocks if vm.NewVM() returns an error

}()
Expand Down
23 changes: 19 additions & 4 deletions cmd/stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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)
}
Comment on lines 48 to 62
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cacheDir never unlocks if vm.NewVM() returns an error

}()
Expand Down
Loading