From 703cbe965b1d19c5a54a87b64521958c00a99be2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 03:24:20 +0200 Subject: [PATCH 1/4] Simplify the name update call stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of going 3 store methods store.updateNames+enum 3 sub-store methods subStore.updateNames+enum applyNameOperation+enum, simplify to 3 store methods store.updateNames+enum subStore.updateNames+enum applyNameOperation+enum, Should not change behavior. Looking purely at updateNameOperation, invalid values would now be detected after doing more work, but there is no way for an external caller to trigger use of an invalid value anyway. Signed-off-by: Miloslav Trmač --- containers.go | 27 ++------------------------- images.go | 30 +++--------------------------- images_test.go | 8 ++++---- layers.go | 27 ++------------------------- store.go | 33 +++------------------------------ 5 files changed, 14 insertions(+), 111 deletions(-) diff --git a/containers.go b/containers.go index 9d8b9fa03..c9650215b 100644 --- a/containers.go +++ b/containers.go @@ -81,18 +81,8 @@ type rwContainerStore interface { // convenience of the caller, nothing more. Create(id string, names []string, image, layer, metadata string, options *ContainerOptions) (*Container, error) - // SetNames updates the list of names associated with the container - // with the specified ID. - // Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. - SetNames(id string, names []string) error - - // AddNames adds the supplied values to the list of names associated with the container with - // the specified id. - AddNames(id string, names []string) error - - // RemoveNames removes the supplied values from the list of names associated with the container with - // the specified id. - RemoveNames(id string, names []string) error + // updateNames modifies names associated with a container based on (op, names). + updateNames(id string, names []string, op updateNameOperation) error // Get retrieves information about a container given an ID or name. Get(id string) (*Container, error) @@ -388,19 +378,6 @@ func (r *containerStore) removeName(container *Container, name string) { container.Names = stringSliceWithoutValue(container.Names, name) } -// Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. -func (r *containerStore) SetNames(id string, names []string) error { - return r.updateNames(id, names, setNames) -} - -func (r *containerStore) AddNames(id string, names []string) error { - return r.updateNames(id, names, addNames) -} - -func (r *containerStore) RemoveNames(id string, names []string) error { - return r.updateNames(id, names, removeNames) -} - func (r *containerStore) updateNames(id string, names []string, op updateNameOperation) error { container, ok := r.lookup(id) if !ok { diff --git a/images.go b/images.go index 50559f0dc..ce45942b6 100644 --- a/images.go +++ b/images.go @@ -129,21 +129,10 @@ type rwImageStore interface { // read-only) layer. That layer can be referenced by multiple images. Create(id string, names []string, layer, metadata string, created time.Time, searchableDigest digest.Digest) (*Image, error) - // SetNames replaces the list of names associated with an image with the - // supplied values. The values are expected to be valid normalized + // updateNames modifies names associated with an image based on (op, names). + // The values are expected to be valid normalized // named image references. - // Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. - SetNames(id string, names []string) error - - // AddNames adds the supplied values to the list of names associated with the image with - // the specified id. The values are expected to be valid normalized - // named image references. - AddNames(id string, names []string) error - - // RemoveNames removes the supplied values from the list of names associated with the image with - // the specified id. The values are expected to be valid normalized - // named image references. - RemoveNames(id string, names []string) error + updateNames(id string, names []string, op updateNameOperation) error // Delete removes the record of the image. Delete(id string) error @@ -516,19 +505,6 @@ func (i *Image) addNameToHistory(name string) { i.NamesHistory = dedupeNames(append([]string{name}, i.NamesHistory...)) } -// Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. -func (r *imageStore) SetNames(id string, names []string) error { - return r.updateNames(id, names, setNames) -} - -func (r *imageStore) AddNames(id string, names []string) error { - return r.updateNames(id, names, addNames) -} - -func (r *imageStore) RemoveNames(id string, names []string) error { - return r.updateNames(id, names, removeNames) -} - func (r *imageStore) updateNames(id string, names []string, op updateNameOperation) error { if !r.IsReadWrite() { return fmt.Errorf("not allowed to change image name assignments at %q: %w", r.imagespath(), ErrStoreIsReadOnly) diff --git a/images_test.go b/images_test.go index 01594c11f..03b3746af 100644 --- a/images_test.go +++ b/images_test.go @@ -24,7 +24,7 @@ func addTestImage(t *testing.T, store rwImageStore, id string, names []string) { ) require.Nil(t, err) - require.Nil(t, store.SetNames(id, names)) + require.Nil(t, store.updateNames(id, names, setNames)) } func TestAddNameToHistorySuccess(t *testing.T) { @@ -72,7 +72,7 @@ func TestHistoryNames(t *testing.T) { // And When store.Lock() defer store.Unlock() - require.Nil(t, store.SetNames(firstImageID, []string{"1", "2", "3", "4"})) + require.Nil(t, store.updateNames(firstImageID, []string{"1", "2", "3", "4"}, setNames)) // Then firstImage, err = store.Get(firstImageID) @@ -91,13 +91,13 @@ func TestHistoryNames(t *testing.T) { require.Equal(t, secondImage.NamesHistory[1], "2") // test independent add and remove operations - require.Nil(t, store.AddNames(firstImageID, []string{"5"})) + require.Nil(t, store.updateNames(firstImageID, []string{"5"}, addNames)) firstImage, err = store.Get(firstImageID) require.Nil(t, err) require.Equal(t, firstImage.NamesHistory, []string{"4", "3", "2", "1", "5"}) // history should still contain old values - require.Nil(t, store.RemoveNames(firstImageID, []string{"5"})) + require.Nil(t, store.updateNames(firstImageID, []string{"5"}, removeNames)) firstImage, err = store.Get(firstImageID) require.Nil(t, err) require.Equal(t, firstImage.NamesHistory, []string{"4", "3", "2", "1", "5"}) diff --git a/layers.go b/layers.go index e1fa41ea6..74ac6f3b3 100644 --- a/layers.go +++ b/layers.go @@ -214,18 +214,8 @@ type rwLayerStore interface { // Put combines the functions of CreateWithFlags and ApplyDiff. Put(id string, parent *Layer, names []string, mountLabel string, options map[string]string, moreOptions *LayerOptions, writeable bool, flags map[string]interface{}, diff io.Reader) (*Layer, int64, error) - // SetNames replaces the list of names associated with a layer with the - // supplied values. - // Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. - SetNames(id string, names []string) error - - // AddNames adds the supplied values to the list of names associated with the layer with the - // specified id. - AddNames(id string, names []string) error - - // RemoveNames remove the supplied values from the list of names associated with the layer with the - // specified id. - RemoveNames(id string, names []string) error + // updateNames modifies names associated with a layer based on (op, names). + updateNames(id string, names []string, op updateNameOperation) error // Delete deletes a layer with the specified name or ID. Delete(id string) error @@ -1091,19 +1081,6 @@ func (r *layerStore) removeName(layer *Layer, name string) { layer.Names = stringSliceWithoutValue(layer.Names, name) } -// Deprecated: Prone to race conditions, suggested alternatives are `AddNames` and `RemoveNames`. -func (r *layerStore) SetNames(id string, names []string) error { - return r.updateNames(id, names, setNames) -} - -func (r *layerStore) AddNames(id string, names []string) error { - return r.updateNames(id, names, addNames) -} - -func (r *layerStore) RemoveNames(id string, names []string) error { - return r.updateNames(id, names, removeNames) -} - func (r *layerStore) updateNames(id string, names []string, op updateNameOperation) error { if !r.IsReadWrite() { return fmt.Errorf("not allowed to change layer name assignments at %q: %w", r.layerspath(), ErrStoreIsReadOnly) diff --git a/store.go b/store.go index d7dc08381..1cbb40e66 100644 --- a/store.go +++ b/store.go @@ -2137,16 +2137,7 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e return nil } layerFound = true - switch op { - case setNames: - return rlstore.SetNames(id, deduped) - case removeNames: - return rlstore.RemoveNames(id, deduped) - case addNames: - return rlstore.AddNames(id, deduped) - default: - return errInvalidUpdateNameOperation - } + return rlstore.updateNames(id, deduped, op) }); err != nil || layerFound { return err } @@ -2161,16 +2152,7 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e return err } if ristore.Exists(id) { - switch op { - case setNames: - return ristore.SetNames(id, deduped) - case removeNames: - return ristore.RemoveNames(id, deduped) - case addNames: - return ristore.AddNames(id, deduped) - default: - return errInvalidUpdateNameOperation - } + return ristore.updateNames(id, deduped, op) } // Check is id refers to a RO Store @@ -2204,16 +2186,7 @@ func (s *store) updateNames(id string, names []string, op updateNameOperation) e return nil } containerFound = true - switch op { - case setNames: - return rcstore.SetNames(id, deduped) - case removeNames: - return rcstore.RemoveNames(id, deduped) - case addNames: - return rcstore.AddNames(id, deduped) - default: - return errInvalidUpdateNameOperation - } + return rcstore.updateNames(id, deduped, op) }); err != nil || containerFound { return err } From 61c00d1399af98fb50c656377c178de93d30418b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 14 Oct 2022 01:52:25 +0200 Subject: [PATCH 2/4] Only lock the primary layer store for reading in Layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and use readAllLayerStores. Signed-off-by: Miloslav Trmač --- store.go | 36 ++++++------------------------------ 1 file changed, 6 insertions(+), 30 deletions(-) diff --git a/store.go b/store.go index d7dc08381..e51783d99 100644 --- a/store.go +++ b/store.go @@ -2946,40 +2946,16 @@ func (s *store) ContainerParentOwners(id string) ([]int, []int, error) { } func (s *store) Layers() ([]Layer, error) { - lstore, err := s.getLayerStore() - if err != nil { - return nil, err - } - - layers, err := func() ([]Layer, error) { - lstore.Lock() - defer lstore.Unlock() - if err := lstore.Load(); err != nil { - return nil, err - } - return lstore.Layers() - }() - if err != nil { - return nil, err - } - - lstores, err := s.getROLayerStores() - if err != nil { - return nil, err - } - - for _, s := range lstores { - store := s - store.RLock() - defer store.Unlock() - if err := store.ReloadIfChanged(); err != nil { - return nil, err - } + var layers []Layer + if done, err := s.readAllLayerStores(func(store roLayerStore) (bool, error) { storeLayers, err := store.Layers() if err != nil { - return nil, err + return true, err } layers = append(layers, storeLayers...) + return false, nil + }); done { + return nil, err } return layers, nil } From d498257fd0f24fe1f6da7cc50f1bc4dc5ce6d68c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 14 Oct 2022 01:55:41 +0200 Subject: [PATCH 3/4] Only lock the primary layer store for reading in CreateImage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There seems to be no reason to lock for writing. Signed-off-by: Miloslav Trmač --- store.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/store.go b/store.go index e51783d99..d0ba6f996 100644 --- a/store.go +++ b/store.go @@ -1286,22 +1286,14 @@ func (s *store) CreateLayer(id, parent string, names []string, mountLabel string func (s *store) CreateImage(id string, names []string, layer, metadata string, options *ImageOptions) (*Image, error) { if layer != "" { - lstore, err := s.getLayerStore() - if err != nil { - return nil, err - } - lstores, err := s.getROLayerStores() + layerStores, err := s.allLayerStores() if err != nil { return nil, err } var ilayer *Layer - for _, s := range append([]roLayerStore{lstore}, lstores...) { + for _, s := range layerStores { store := s - if store == lstore { - store.Lock() - } else { - store.RLock() - } + store.RLock() defer store.Unlock() err := store.ReloadIfChanged() if err != nil { From 0b5faf994fd339f6255293f834162a681a325c26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 13 Oct 2022 02:07:47 +0200 Subject: [PATCH 4/4] Remove a manual layerStore.Touch() call from store.Shutdown() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's completely unclear why this is necessary; the unmount calls actually write to the separate mountpoints.json file, with a separate lock. When this was originally introduced in b046fb5a9a1ad6d25bd6c854400f090131c8a21c , there wasn't a separate mountpoints.json file, and store.go was manually calling Touch() everywhere; so my best guess is that this is just an artifact of making those two API changes, and the Touch() is not actually necessary. Signed-off-by: Miloslav Trmač --- store.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/store.go b/store.go index d7dc08381..aa04d61d7 100644 --- a/store.go +++ b/store.go @@ -3313,7 +3313,6 @@ func (s *store) FromContainerRunDirectory(id, file string) ([]byte, error) { func (s *store) Shutdown(force bool) ([]string, error) { mounted := []string{} - modified := false rlstore, err := s.getLayerStore() if err != nil { @@ -3347,7 +3346,6 @@ func (s *store) Shutdown(force bool) ([]string, error) { } break } - modified = true } } } @@ -3363,16 +3361,6 @@ func (s *store) Shutdown(force bool) ([]string, error) { err = fmt.Errorf("(graphLock.Touch failed: %v) %w", err2, err) } } - modified = true - } - if modified { - if err2 := rlstore.Touch(); err2 != nil { - if err == nil { - err = err2 - } else { - err = fmt.Errorf("rlstore.Touch failed: %v) %w", err2, err) - } - } } return mounted, err }