Skip to content

RFC: Use Go 1.23 iterators for locking+traversing stores. #2288

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

RFC: Use Go 1.23 iterators for locking+traversing stores. #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,13 +661,16 @@ func (s *store) Check(options *CheckOptions) (CheckReport, error) {
// Now go back through all of the layer stores, and flag any layers which don't belong
// to an image or a container, and has been around longer than we can reasonably expect
// such a layer to be present before a corresponding image record is added.
if _, _, err := readAllLayerStores(s, func(store roLayerStore) (struct{}, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return CheckReport{}, err
}
if isReadWrite := roLayerStoreIsReallyReadWrite(store); !isReadWrite {
return struct{}{}, false, nil
continue
}
layers, err := store.Layers()
if err != nil {
return struct{}{}, true, err
return CheckReport{}, err
}
for _, layer := range layers {
maximumAge := defaultMaximumUnreferencedLayerAge
Expand All @@ -685,9 +688,6 @@ func (s *store) Check(options *CheckOptions) (CheckReport, error) {
}
}
}
return struct{}{}, false, nil
}); err != nil {
return CheckReport{}, err
}

// If the driver can tell us about which layers it knows about, we should have previously
Expand Down
10 changes: 4 additions & 6 deletions check_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,14 @@ func TestCheckDetectWriteable(t *testing.T) {
require.NoError(t, err, "unexpected error initializing test store")
s, ok := stoar.(*store)
require.True(t, ok, "unexpected error making type assertion")
_, done, err := readAllLayerStores(s, func(store roLayerStore) (struct{}, bool, error) {
for store, err := range s.readAllLayerStores() {
require.NoError(t, err)
if roLayerStoreIsReallyReadWrite(store) { // implicitly checking that the type assertion in this function doesn't panic
sawRWlayers = true
}
return struct{}{}, false, nil
})
assert.False(t, done, "unexpected error from readAllLayerStores")
assert.NoError(t, err, "unexpected error from readAllLayerStores")
}
assert.True(t, sawRWlayers, "unexpected error detecting which layer store is writeable")
_, done, err = readAllImageStores(s, func(store roImageStore) (struct{}, bool, error) {
_, done, err := readAllImageStores(s, func(store roImageStore) (struct{}, bool, error) {
if roImageStoreIsReallyReadWrite(store) { // implicitly checking that the type assertion in this function doesn't panic
sawRWimages = true
}
Expand Down
175 changes: 79 additions & 96 deletions store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"io"
"iter"
"maps"
"os"
"path/filepath"
Expand Down Expand Up @@ -1237,41 +1238,36 @@ func (s *store) allLayerStores() ([]roLayerStore, error) {
}

// readAllLayerStores processes allLayerStores() in order:
// It locks the store for reading, checks for updates, and calls
//
// (data, done, err) := fn(store)
//
// until the callback returns done == true, and returns the data from the callback.
//
// If reading any layer store fails, it immediately returns ({}, true, err).
// It locks the store for reading, checks for updates, and iterates through (store, nil).
//
// If all layer stores are processed without setting done == true, it returns ({}, false, nil).
// If reading any layer store fails, it yields (nil, err) and terminates.
//
// Typical usage:
//
// if res, done, err := s.readAllLayerStores(store, func(…) {
// …
// }; done {
// return res, err
// for rlstore, err := range s.readAllLayerStores() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note to self: This looks almost too easy, without warning callers about the semantics. Maybe range iterateLockingAllLayerStores ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iterateReadLockingAllLayerStores? As usual, I tend towards unwieldy names.

// if err != nil {
// return err
// }
// }
func readAllLayerStores[T any](s *store, fn func(store roLayerStore) (T, bool, error)) (T, bool, error) {
var zeroRes T // A zero value of T

layerStores, err := s.allLayerStores()
if err != nil {
return zeroRes, true, err
}
for _, s := range layerStores {
store := s
if err := store.startReading(); err != nil {
return zeroRes, true, err
func (s *store) readAllLayerStores() iter.Seq2[roLayerStore, error] {
return func(yield func(roLayerStore, error) bool) {
layerStores, err := s.allLayerStores()
if err != nil {
_ = yield(nil, err)
return
}
defer store.stopReading()
if res, done, err := fn(store); done {
return res, true, err

for _, store := range layerStores {
if err := store.startReading(); err != nil {
_ = yield(nil, err)
return
}
defer store.stopReading()
if !yield(store, nil) {
return
}
}
}
return zeroRes, false, nil
}

// writeToLayerStore is a helper for working with store.getLayerStore():
Expand Down Expand Up @@ -1984,14 +1980,13 @@ func (s *store) SetMetadata(id, metadata string) error {
}

func (s *store) Metadata(id string) (string, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (string, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return "", err
}
if store.Exists(id) {
res, err := store.Metadata(id)
return res, true, err
return store.Metadata(id)
}
return "", false, nil
}); done {
return res, err
}

if res, done, err := readAllImageStores(s, func(store roImageStore) (string, bool, error) {
Expand Down Expand Up @@ -2083,17 +2078,17 @@ func (s *store) ImageBigData(id, key string) ([]byte, error) {
// named data associated with an layer.
func (s *store) ListLayerBigData(id string) ([]string, error) {
foundLayer := false
if res, done, err := readAllLayerStores(s, func(store roLayerStore) ([]string, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
data, err := store.BigDataNames(id)
if err == nil {
return data, true, nil
return data, nil
}
if store.Exists(id) {
foundLayer = true
}
return nil, false, nil
}); done {
return res, err
}
if foundLayer {
return nil, fmt.Errorf("locating big data for layer with ID %q: %w", id, os.ErrNotExist)
Expand All @@ -2105,17 +2100,17 @@ func (s *store) ListLayerBigData(id string) ([]string, error) {
// associated with a layer.
func (s *store) LayerBigData(id, key string) (io.ReadCloser, error) {
foundLayer := false
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (io.ReadCloser, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
data, err := store.BigData(id, key)
if err == nil {
return data, true, nil
return data, nil
}
if store.Exists(id) {
foundLayer = true
}
return nil, false, nil
}); done {
return res, err
}
if foundLayer {
return nil, fmt.Errorf("locating item named %q for layer with ID %q: %w", key, id, os.ErrNotExist)
Expand Down Expand Up @@ -2345,20 +2340,16 @@ func (s *store) SetContainerBigData(id, key string, data []byte) error {
}

func (s *store) Exists(id string) bool {
found, _, err := readAllLayerStores(s, func(store roLayerStore) (bool, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return false
}
if store.Exists(id) {
return true, true, nil
return true
}
return false, false, nil
})
if err != nil {
return false
}
if found {
return true
}

found, _, err = readAllImageStores(s, func(store roImageStore) (bool, bool, error) {
found, _, err := readAllImageStores(s, func(store roImageStore) (bool, bool, error) {
if store.Exists(id) {
return true, true, nil
}
Expand Down Expand Up @@ -2492,13 +2483,13 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e
}

func (s *store) Names(id string) ([]string, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) ([]string, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
if l, err := store.Get(id); l != nil && err == nil {
return l.Names, true, nil
return l.Names, nil
}
return nil, false, nil
}); done {
return res, err
}

if res, done, err := readAllImageStores(s, func(store roImageStore) ([]string, bool, error) {
Expand All @@ -2523,13 +2514,13 @@ func (s *store) Names(id string) ([]string, error) {
}

func (s *store) Lookup(name string) (string, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (string, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return "", err
}
if l, err := store.Get(name); l != nil && err == nil {
return l.ID, true, nil
return l.ID, nil
}
return "", false, nil
}); done {
return res, err
}

if res, done, err := readAllImageStores(s, func(store roImageStore) (string, bool, error) {
Expand Down Expand Up @@ -2997,17 +2988,13 @@ func (s *store) Changes(from, to string) ([]archive.Change, error) {
}

func (s *store) DiffSize(from, to string) (int64, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (int64, bool, error) {
if store.Exists(to) {
res, err := store.DiffSize(from, to)
return res, true, err
}
return -1, false, nil
}); done {
for store, err := range s.readAllLayerStores() {
if err != nil {
return -1, err
}
return res, nil
if store.Exists(to) {
return store.DiffSize(from, to)
}
}
return -1, ErrLayerUnknown
}
Expand Down Expand Up @@ -3144,18 +3131,18 @@ func (s *store) ApplyDiff(to string, diff io.Reader) (int64, error) {

func (s *store) layersByMappedDigest(m func(roLayerStore, digest.Digest) ([]Layer, error), d digest.Digest) ([]Layer, error) {
var layers []Layer
if _, _, err := readAllLayerStores(s, func(store roLayerStore) (struct{}, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
storeLayers, err := m(store, d)
if err != nil {
if !errors.Is(err, ErrLayerUnknown) {
return struct{}{}, true, err
if errors.Is(err, ErrLayerUnknown) {
continue
}
return struct{}{}, false, nil
return nil, err
}
layers = append(layers, storeLayers...)
return struct{}{}, false, nil
}); err != nil {
return nil, err
}
if len(layers) == 0 {
return nil, ErrLayerUnknown
Expand Down Expand Up @@ -3185,17 +3172,13 @@ func (s *store) LayersByTOCDigest(d digest.Digest) ([]Layer, error) {
}

func (s *store) LayerSize(id string) (int64, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (int64, bool, error) {
if store.Exists(id) {
res, err := store.Size(id)
return res, true, err
}
return -1, false, nil
}); done {
for store, err := range s.readAllLayerStores() {
if err != nil {
return -1, err
}
return res, nil
if store.Exists(id) {
return store.Size(id)
}
}
return -1, ErrLayerUnknown
}
Expand Down Expand Up @@ -3240,15 +3223,15 @@ func (s *store) ContainerParentOwners(id string) ([]int, []int, error) {

func (s *store) Layers() ([]Layer, error) {
var layers []Layer
if _, done, err := readAllLayerStores(s, func(store roLayerStore) (struct{}, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
storeLayers, err := store.Layers()
if err != nil {
return struct{}{}, true, err
return nil, err
}
layers = append(layers, storeLayers...)
return struct{}{}, false, nil
}); done {
return nil, err
}
return layers, nil
}
Expand Down Expand Up @@ -3277,14 +3260,14 @@ func (s *store) Containers() ([]Container, error) {
}

func (s *store) Layer(id string) (*Layer, error) {
if res, done, err := readAllLayerStores(s, func(store roLayerStore) (*Layer, bool, error) {
for store, err := range s.readAllLayerStores() {
if err != nil {
return nil, err
}
layer, err := store.Get(id)
if err == nil {
return layer, true, nil
return layer, nil
}
return nil, false, nil
}); done {
return res, err
}
return nil, ErrLayerUnknown
}
Expand Down