Skip to content
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

API break: Remove Lockfile.Locked #1399

Merged
merged 6 commits into from
Oct 25, 2022
Merged

API break: Remove Lockfile.Locked #1399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5791b9d
Add Lockfile.AssertLocked and AssertLockedForWriting
mtrmac Oct 18, 2022
a5be9f3
Avoid use of Locker.Locked() in tests
mtrmac Oct 14, 2022
268b3e8
Modify containerStore not to rely on lockfile.Locked()
mtrmac Oct 18, 2022
cf6f6f7
Modify imageStore not to rely on lockfile.Locked()
mtrmac Oct 18, 2022
8e27034
Modify layerStore not to rely on lockfile.Locked()
mtrmac Oct 18, 2022
d4462aa
API BREAK: Drop Lockfile.Locked
mtrmac Oct 18, 2022
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
33 changes: 20 additions & 13 deletions containers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func (r *containerStore) startWritingWithReload(canReload bool) error {
}()

if canReload {
if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(true); err != nil {
return err
}
}
Expand Down Expand Up @@ -222,7 +222,7 @@ func (r *containerStore) startReading() error {
}
}()

if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(false); err != nil {
return err
}

Expand All @@ -235,14 +235,17 @@ func (r *containerStore) stopReading() {
r.lockfile.Unlock()
}

// ReloadIfChanged reloads the contents of the store from disk if it is changed.
func (r *containerStore) ReloadIfChanged() error {
// reloadIfChanged reloads the contents of the store from disk if it is changed.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *containerStore) reloadIfChanged(lockedForWriting bool) error {
r.loadMut.Lock()
defer r.loadMut.Unlock()

modified, err := r.lockfile.Modified()
if err == nil && modified {
return r.Load()
return r.load(lockedForWriting)
}
return err
}
Expand All @@ -267,9 +270,11 @@ func (r *containerStore) datapath(id, key string) string {
return filepath.Join(r.datadir(id), makeBigDataBaseName(key))
}

// Load reloads the contents of the store from disk. It should be called
// with the lock held.
func (r *containerStore) Load() error {
// load reloads the contents of the store from disk.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *containerStore) load(lockedForWriting bool) error {
needSave := false
rpath := r.containerspath()
data, err := os.ReadFile(rpath)
Expand Down Expand Up @@ -302,17 +307,19 @@ func (r *containerStore) Load() error {
r.bylayer = layers
r.byname = names
if needSave {
if !lockedForWriting {
// Eventually, the callers should be modified to retry with a write lock, instead.
return errors.New("container store is inconsistent and the current caller does not hold a write lock")
Copy link
Contributor

Choose a reason for hiding this comment

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

How can callers know when they need to try again? I mean how to distinguish this error (which requires retry) from any other? I see ErrDuplicateImageNames in layerStore.Load()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They can’t, currently.

I plan to add that (“try again”, instead of callers hard-coding specific error values) both to load and the callers simultaneously.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The retries will be added in #1407 .

}
return r.Save()
}
return nil
}

// Save saves the contents of the store to disk. It should be called with
// the lock held.
// the lock held, locked for writing.
func (r *containerStore) Save() error {
if !r.lockfile.Locked() {
return errors.New("container store is not locked")
}
r.lockfile.AssertLockedForWriting()
rpath := r.containerspath()
if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
return err
Expand Down Expand Up @@ -347,7 +354,7 @@ func newContainerStore(dir string) (rwContainerStore, error) {
return nil, err
}
defer cstore.stopWriting()
if err := cstore.Load(); err != nil {
if err := cstore.load(true); err != nil {
return nil, err
}
return &cstore, nil
Expand Down
37 changes: 20 additions & 17 deletions images.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package storage

import (
"errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -154,7 +153,7 @@ type rwImageStore interface {
}

type imageStore struct {
lockfile Locker
lockfile Locker // lockfile.IsReadWrite can be used to distinguish between read-write and read-only image stores.
dir string
images []*Image
idindex *truncindex.TruncIndex
Expand Down Expand Up @@ -209,7 +208,7 @@ func (r *imageStore) startWritingWithReload(canReload bool) error {
}()

if canReload {
if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(true); err != nil {
return err
}
}
Expand Down Expand Up @@ -244,7 +243,7 @@ func (r *imageStore) startReadingWithReload(canReload bool) error {
}()

if canReload {
if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(false); err != nil {
return err
}
}
Expand All @@ -264,14 +263,17 @@ func (r *imageStore) stopReading() {
r.lockfile.Unlock()
}

// ReloadIfChanged reloads the contents of the store from disk if it is changed.
func (r *imageStore) ReloadIfChanged() error {
// reloadIfChanged reloads the contents of the store from disk if it is changed.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *imageStore) reloadIfChanged(lockedForWriting bool) error {
r.loadMut.Lock()
defer r.loadMut.Unlock()

modified, err := r.lockfile.Modified()
if err == nil && modified {
return r.Load()
return r.load(lockedForWriting)
}
return err
}
Expand Down Expand Up @@ -336,9 +338,11 @@ func (i *Image) recomputeDigests() error {
return nil
}

// Load reloads the contents of the store from disk. It should be called
// with the lock held.
func (r *imageStore) Load() error {
// load reloads the contents of the store from disk.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *imageStore) load(lockedForWriting bool) error {
shouldSave := false
rpath := r.imagespath()
data, err := os.ReadFile(rpath)
Expand Down Expand Up @@ -376,7 +380,8 @@ func (r *imageStore) Load() error {
image.ReadOnly = !r.lockfile.IsReadWrite()
}
}
if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) {
if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
return ErrDuplicateImageNames
}
r.images = images
Expand All @@ -391,14 +396,12 @@ func (r *imageStore) Load() error {
}

// Save saves the contents of the store to disk. It should be called with
// the lock held.
// the lock held, locked for writing.
func (r *imageStore) Save() error {
if !r.lockfile.IsReadWrite() {
return fmt.Errorf("not allowed to modify the image store at %q: %w", r.imagespath(), ErrStoreIsReadOnly)
}
if !r.lockfile.Locked() {
return errors.New("image store is not locked for writing")
}
r.lockfile.AssertLockedForWriting()
rpath := r.imagespath()
if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
return err
Expand Down Expand Up @@ -433,7 +436,7 @@ func newImageStore(dir string) (rwImageStore, error) {
return nil, err
}
defer istore.stopWriting()
if err := istore.Load(); err != nil {
if err := istore.load(true); err != nil {
return nil, err
}
return &istore, nil
Expand All @@ -456,7 +459,7 @@ func newROImageStore(dir string) (roImageStore, error) {
return nil, err
}
defer istore.stopReading()
if err := istore.Load(); err != nil {
if err := istore.load(false); err != nil {
return nil, err
}
return &istore, nil
Expand Down
40 changes: 21 additions & 19 deletions layers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,7 @@ func (r *layerStore) startWritingWithReload(canReload bool) error {
}()

if canReload {
if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(true); err != nil {
return err
}
}
Expand Down Expand Up @@ -366,7 +366,7 @@ func (r *layerStore) startReadingWithReload(canReload bool) error {
}()

if canReload {
if err := r.ReloadIfChanged(); err != nil {
if err := r.reloadIfChanged(false); err != nil {
return err
}
}
Expand Down Expand Up @@ -420,14 +420,17 @@ func (r *layerStore) Modified() (bool, error) {
return tmodified, nil
}

// ReloadIfChanged reloads the contents of the store from disk if it is changed.
func (r *layerStore) ReloadIfChanged() error {
// reloadIfChanged reloads the contents of the store from disk if it is changed.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *layerStore) reloadIfChanged(lockedForWriting bool) error {
r.loadMut.Lock()
defer r.loadMut.Unlock()

modified, err := r.Modified()
if err == nil && modified {
return r.Load()
return r.load(lockedForWriting)
}
return err
}
Expand All @@ -448,9 +451,11 @@ func (r *layerStore) layerspath() string {
return filepath.Join(r.layerdir, "layers.json")
}

// Load reloads the contents of the store from disk. It should be called
// with the lock held.
func (r *layerStore) Load() error {
// load reloads the contents of the store from disk.
//
// The caller must hold r.lockfile for reading _or_ writing; lockedForWriting is true
// if it is held for writing.
func (r *layerStore) load(lockedForWriting bool) error {
shouldSave := false
rpath := r.layerspath()
info, err := os.Stat(rpath)
Expand Down Expand Up @@ -499,7 +504,8 @@ func (r *layerStore) Load() error {
}
err = nil
}
if shouldSave && (!r.lockfile.IsReadWrite() || !r.lockfile.Locked()) {
if shouldSave && (!r.lockfile.IsReadWrite() || !lockedForWriting) {
// Eventually, the callers should be modified to retry with a write lock if IsReadWrite && !lockedForWriting, instead.
return ErrDuplicateLayerNames
}
r.layers = layers
Expand All @@ -520,7 +526,7 @@ func (r *layerStore) Load() error {
// Last step: as we’re writable, try to remove anything that a previous
// user of this storage area marked for deletion but didn't manage to
// actually delete.
if r.lockfile.Locked() {
if lockedForWriting {
for _, layer := range r.layers {
if layer.Flags == nil {
layer.Flags = make(map[string]interface{})
Expand Down Expand Up @@ -581,7 +587,7 @@ func (r *layerStore) loadMounts() error {
}

// Save saves the contents of the store to disk. It should be called with
// the lock held.
// the lock held, locked for writing.
func (r *layerStore) Save() error {
r.mountsLockfile.Lock()
defer r.mountsLockfile.Unlock()
Expand All @@ -595,9 +601,7 @@ func (r *layerStore) saveLayers() error {
if !r.lockfile.IsReadWrite() {
return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly)
}
if !r.lockfile.Locked() {
return errors.New("layer store is not locked for writing")
}
r.lockfile.AssertLockedForWriting()
rpath := r.layerspath()
if err := os.MkdirAll(filepath.Dir(rpath), 0700); err != nil {
return err
Expand All @@ -616,9 +620,7 @@ func (r *layerStore) saveMounts() error {
if !r.lockfile.IsReadWrite() {
return fmt.Errorf("not allowed to modify the layer store at %q: %w", r.layerspath(), ErrStoreIsReadOnly)
}
if !r.mountsLockfile.Locked() {
return errors.New("layer store mount information is not locked for writing")
}
r.mountsLockfile.AssertLockedForWriting()
mpath := r.mountspath()
if err := os.MkdirAll(filepath.Dir(mpath), 0700); err != nil {
return err
Expand Down Expand Up @@ -675,7 +677,7 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri
return nil, err
}
defer rlstore.stopWriting()
if err := rlstore.Load(); err != nil {
if err := rlstore.load(true); err != nil {
return nil, err
}
return &rlstore, nil
Expand All @@ -700,7 +702,7 @@ func newROLayerStore(rundir string, layerdir string, driver drivers.Driver) (roL
return nil, err
}
defer rlstore.stopReading()
if err := rlstore.Load(); err != nil {
if err := rlstore.load(false); err != nil {
return nil, err
}
return &rlstore, nil
Expand Down
9 changes: 7 additions & 2 deletions pkg/lockfile/lockfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,13 @@ type Locker interface {
// IsReadWrite() checks if the lock file is read-write
IsReadWrite() bool

// Locked() checks if lock is locked for writing by a thread in this process
Locked() bool
// AssertLocked() can be used by callers that _know_ that they hold the lock (for reading or writing), for sanity checking.
// It might do nothing at all, or it may panic if the caller is not the owner of this lock.
AssertLocked()

// AssertLocked() can be used by callers that _know_ that they hold the lock locked for writing, for sanity checking.
// It might do nothing at all, or it may panic if the caller is not the owner of this lock for writing.
AssertLockedForWriting()
}

var (
Expand Down
Loading