Skip to content

Commit e1681d8

Browse files
ckyrouacckyrouac
committed
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]>
1 parent 5744e9b commit e1681d8

File tree

12 files changed

+156
-147
lines changed

12 files changed

+156
-147
lines changed

‎cmd/list.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55

66
"gitlab.com/bootc-org/podman-bootc/pkg/config"
77
"gitlab.com/bootc-org/podman-bootc/pkg/user"
8-
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
98
"gitlab.com/bootc-org/podman-bootc/pkg/vm"
109

1110
"github.com/containers/common/pkg/report"
@@ -84,19 +83,14 @@ func getVMInfo(user user.User, libvirtUri string, imageId string) (*vm.BootcVMCo
8483
ImageID: imageId,
8584
User: user,
8685
LibvirtUri: libvirtUri,
87-
Locking: utils.Shared,
8886
})
8987

9088
if err != nil {
9189
return nil, err
9290
}
9391

94-
// Let's be explicit instead of relying on the defer exec order
9592
defer func() {
9693
bootcVM.CloseConnection()
97-
if err := bootcVM.Unlock(); err != nil {
98-
logrus.Warningf("unable to unlock VM %s: %v", imageId, err)
99-
}
10094
}()
10195

10296
cfg, err := bootcVM.GetConfig()

‎cmd/rm.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,9 @@ import (
44
"fmt"
55
"os"
66

7+
"gitlab.com/bootc-org/podman-bootc/pkg/cache"
78
"gitlab.com/bootc-org/podman-bootc/pkg/config"
89
"gitlab.com/bootc-org/podman-bootc/pkg/user"
9-
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
1010
"gitlab.com/bootc-org/podman-bootc/pkg/vm"
1111

1212
"github.com/sirupsen/logrus"
@@ -44,6 +44,7 @@ func oneOrAll() cobra.PositionalArgs {
4444
}
4545

4646
func doRemove(_ *cobra.Command, args []string) error {
47+
4748
if removeAll {
4849
return pruneAll()
4950
}
@@ -57,20 +58,33 @@ func prune(id string) error {
5758
return err
5859
}
5960

61+
// take an exclusive lock on the cache directory
62+
cacheDir, err := cache.NewCache(id, user)
63+
if err != nil {
64+
return err
65+
}
66+
err = cacheDir.Create()
67+
if err != nil {
68+
return err
69+
}
70+
err = cacheDir.Lock(cache.Exclusive)
71+
if err != nil {
72+
return err
73+
}
74+
6075
bootcVM, err := vm.NewVM(vm.NewVMParameters{
6176
ImageID: id,
6277
LibvirtUri: config.LibvirtUri,
6378
User: user,
64-
Locking: utils.Exclusive,
6579
})
6680
if err != nil {
6781
return fmt.Errorf("unable to get VM %s: %v", id, err)
6882
}
6983

70-
// Let's be explicit instead of relying on the defer exec order
7184
defer func() {
85+
// Let's be explicit instead of relying on the defer exec order
7286
bootcVM.CloseConnection()
73-
if err := bootcVM.Unlock(); err != nil {
87+
if err := cacheDir.Unlock(); err != nil {
7488
logrus.Warningf("unable to unlock VM %s: %v", id, err)
7589
}
7690
}()

‎cmd/run.go

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -109,25 +109,40 @@ func doRun(flags *cobra.Command, args []string) error {
109109
return fmt.Errorf("unable to pull container image: %w", err)
110110
}
111111

112-
// create the cache directory
113-
cache := cache.NewCache(containerImage.GetId(), user)
114-
err = cache.Create()
112+
// create the cacheDir directory
113+
cacheDir, err := cache.NewCache(containerImage.GetId(), user)
114+
if err != nil {
115+
return fmt.Errorf("unable to create cache: %w", err)
116+
}
117+
err = cacheDir.Create()
115118
if err != nil {
116119
return fmt.Errorf("unable to create cache: %w", err)
117120
}
118121

122+
err = cacheDir.Lock(cache.Exclusive)
123+
if err != nil {
124+
return err
125+
}
126+
119127
// check if the vm is already running
120128
bootcVM, err := vm.NewVM(vm.NewVMParameters{
121129
ImageID: containerImage.GetId(),
122130
User: user,
123131
LibvirtUri: config.LibvirtUri,
124-
Locking: utils.Shared,
125132
})
126133

127134
if err != nil {
128135
return fmt.Errorf("unable to initialize VM: %w", err)
129136
}
130137

138+
defer func() {
139+
// Let's be explicit instead of relying on the defer exec order
140+
bootcVM.CloseConnection()
141+
if err := cacheDir.Unlock(); err != nil {
142+
logrus.Warningf("unable to unlock cache %s: %v", cacheDir.ImageId, err)
143+
}
144+
}()
145+
131146
isRunning, err := bootcVM.IsRunning()
132147
if err != nil {
133148
return fmt.Errorf("unable to check if VM is running: %w", err)
@@ -145,7 +160,7 @@ func doRun(flags *cobra.Command, args []string) error {
145160
}
146161

147162
// create the disk image
148-
bootcDisk := bootc.NewBootcDisk(containerImage, ctx, user, cache, bustCache)
163+
bootcDisk := bootc.NewBootcDisk(containerImage, ctx, user, cacheDir, bustCache)
149164
err = bootcDisk.Install(vmConfig.Quiet, diskImageConfigInstance)
150165

151166
if err != nil {
@@ -159,14 +174,6 @@ func doRun(flags *cobra.Command, args []string) error {
159174
return fmt.Errorf("unable to get free port for SSH: %w", err)
160175
}
161176

162-
// Let's be explicit instead of relying on the defer exec order
163-
defer func() {
164-
bootcVM.CloseConnection()
165-
if err := bootcVM.Unlock(); err != nil {
166-
logrus.Warningf("unable to unlock VM %s: %v", containerImage.GetId(), err)
167-
}
168-
}()
169-
170177
cmd := args[1:]
171178
err = bootcVM.Run(vm.RunVMParameters{
172179
Cmd: cmd,
@@ -189,6 +196,14 @@ func doRun(flags *cobra.Command, args []string) error {
189196
return err
190197
}
191198

199+
// done modifying the cache, so remove the Exclusive lock
200+
cacheDir.Unlock()
201+
202+
// take a RO lock for the SSH connection
203+
// it will be unlocked at the end of this function
204+
// by the previous defer()
205+
cacheDir.Lock(cache.Shared)
206+
192207
if !vmConfig.Background {
193208
if !vmConfig.Quiet {
194209
var vmConsoleWg sync.WaitGroup
@@ -227,6 +242,14 @@ func doRun(flags *cobra.Command, args []string) error {
227242

228243
// Always remove when executing a command
229244
if vmConfig.RemoveVm || len(cmd) > 0 {
245+
// remove the RO lock
246+
cacheDir.Unlock()
247+
248+
// take an exclusive lock to remove the VM
249+
// it will be unlocked at the end of this function
250+
// by the previous defer()
251+
cacheDir.Lock(cache.Exclusive)
252+
230253
err = bootcVM.Delete() //delete the VM, but keep the disk image
231254
if err != nil {
232255
return fmt.Errorf("unable to remove VM from cache: %w", err)

‎cmd/ssh.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package cmd
22

33
import (
4+
"gitlab.com/bootc-org/podman-bootc/pkg/cache"
45
"gitlab.com/bootc-org/podman-bootc/pkg/config"
56
"gitlab.com/bootc-org/podman-bootc/pkg/user"
67
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
@@ -32,21 +33,34 @@ func doSsh(_ *cobra.Command, args []string) error {
3233

3334
id := args[0]
3435

36+
//take a read only lock on the cache directory
37+
cacheDir, err := cache.NewCache(id, user)
38+
if err != nil {
39+
return err
40+
}
41+
err = cacheDir.Create()
42+
if err != nil {
43+
return err
44+
}
45+
err = cacheDir.Lock(cache.Shared)
46+
if err != nil {
47+
return err
48+
}
49+
3550
vm, err := vm.NewVM(vm.NewVMParameters{
3651
ImageID: id,
3752
User: user,
3853
LibvirtUri: config.LibvirtUri,
39-
Locking: utils.Shared,
4054
})
4155

4256
if err != nil {
4357
return err
4458
}
4559

46-
// Let's be explicit instead of relying on the defer exec order
4760
defer func() {
61+
// Let's be explicit instead of relying on the defer exec order
4862
vm.CloseConnection()
49-
if err := vm.Unlock(); err != nil {
63+
if err := cacheDir.Unlock(); err != nil {
5064
logrus.Warningf("unable to unlock VM %s: %v", id, err)
5165
}
5266
}()

‎cmd/stop.go

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
package cmd
22

33
import (
4+
"gitlab.com/bootc-org/podman-bootc/pkg/cache"
45
"gitlab.com/bootc-org/podman-bootc/pkg/config"
56
"gitlab.com/bootc-org/podman-bootc/pkg/user"
6-
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
77
"gitlab.com/bootc-org/podman-bootc/pkg/vm"
88

99
"github.com/sirupsen/logrus"
@@ -23,26 +23,40 @@ func init() {
2323
}
2424

2525
func doStop(_ *cobra.Command, args []string) (err error) {
26+
id := args[0]
27+
2628
user, err := user.NewUser()
2729
if err != nil {
2830
return err
2931
}
3032

31-
id := args[0]
33+
//take an exclusive lock on the cache directory
34+
cacheDir, err := cache.NewCache(id, user)
35+
if err != nil {
36+
return err
37+
}
38+
err = cacheDir.Create()
39+
if err != nil {
40+
return err
41+
}
42+
err = cacheDir.Lock(cache.Exclusive)
43+
if err != nil {
44+
return err
45+
}
46+
3247
bootcVM, err := vm.NewVM(vm.NewVMParameters{
3348
ImageID: id,
3449
LibvirtUri: config.LibvirtUri,
3550
User: user,
36-
Locking: utils.Exclusive,
3751
})
3852
if err != nil {
3953
return err
4054
}
4155

42-
// Let's be explicit instead of relying on the defer exec order
4356
defer func() {
57+
// Let's be explicit instead of relying on the defer exec order
4458
bootcVM.CloseConnection()
45-
if err := bootcVM.Unlock(); err != nil {
59+
if err := cacheDir.Unlock(); err != nil {
4660
logrus.Warningf("unable to unlock VM %s: %v", id, err)
4761
}
4862
}()

‎pkg/cache/cache.go

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,42 +5,37 @@ import (
55
"os"
66
"path/filepath"
77

8-
"github.com/sirupsen/logrus"
98
"gitlab.com/bootc-org/podman-bootc/pkg/user"
109
"gitlab.com/bootc-org/podman-bootc/pkg/utils"
1110
)
1211

13-
func NewCache(imageId string, user user.User) Cache {
12+
// NewCache creates a new cache object
13+
// Parameters:
14+
// - id: the full or partial image ID. If partial, it will be expanded to the full image ID
15+
// - user: the user who is running the podman-bootc command
16+
func NewCache(id string, user user.User) (cache Cache, err error) {
17+
fullImageId, err := utils.FullImageIdFromPartial(id, user)
18+
if err != nil {
19+
return Cache{}, err
20+
}
21+
1422
return Cache{
15-
ImageId: imageId,
23+
ImageId: fullImageId,
1624
User: user,
17-
}
25+
}, nil
1826
}
1927

2028
type Cache struct {
2129
User user.User
2230
ImageId string
2331
Directory string
2432
Created bool
33+
lock CacheLock
2534
}
2635

2736
// Create VM cache dir; one per oci bootc image
2837
func (p *Cache) Create() (err error) {
2938
p.Directory = filepath.Join(p.User.CacheDir(), p.ImageId)
30-
lock := utils.NewCacheLock(p.User.RunDir(), p.Directory)
31-
locked, err := lock.TryLock(utils.Exclusive)
32-
if err != nil {
33-
return fmt.Errorf("error locking the VM cache path: %w", err)
34-
}
35-
if !locked {
36-
return fmt.Errorf("unable to lock the VM cache path")
37-
}
38-
39-
defer func() {
40-
if err := lock.Unlock(); err != nil {
41-
logrus.Errorf("unable to unlock VM %s: %v", p.ImageId, err)
42-
}
43-
}()
4439

4540
if err := os.MkdirAll(p.Directory, os.ModePerm); err != nil {
4641
return fmt.Errorf("error while making bootc disk directory: %w", err)
@@ -64,3 +59,21 @@ func (p *Cache) GetDiskPath() string {
6459
}
6560
return filepath.Join(p.GetDirectory(), "disk.raw")
6661
}
62+
63+
func (p *Cache) Lock(mode AccessMode) error {
64+
p.lock = NewCacheLock(p.User.RunDir(), p.Directory)
65+
locked, err := p.lock.TryLock(Exclusive)
66+
if err != nil {
67+
return fmt.Errorf("error locking the cache path: %w", err)
68+
}
69+
if !locked {
70+
return fmt.Errorf("unable to lock the cache path")
71+
}
72+
73+
return nil
74+
}
75+
76+
func (p *Cache) Unlock() error {
77+
return p.lock.Unlock()
78+
}
79+

‎pkg/utils/locks.go renamed to ‎pkg/cache/lock.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package utils
1+
package cache
22

33
import (
44
"path/filepath"

0 commit comments

Comments
 (0)