beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Don't fail on two concurrent reference.store.AddDigest calls

Browse source 
reference.store.addReference fails when adding a digest reference
that already exists (regardless of the reference target).  Both
callers (via reference.store.AddDigest) do check in advance, using
reference.store.Get, whether the digest reference exists before
calling AddDigest, but the reference store lock is released between
the two calls, so if another thread sets the reference in the meantime,
AddDigest may fail with
> Cannot overwrite digest ...
.

Handle this by checking that the pre-existing reference points at the
same image, i.e. that there is nothing to do, and succeeding immediately
in that case.  This is even cheaper, avoids a reference.store.save() call.

(In principle, the same failure could have happened via
reference.store.AddTag, as
> Conflict: Tag %s is already set to image %s, if you want to replace it, please use -f option
but almost all callers (except for migrate/v1.Migrate, which is run
single-threaded anyway) set the "force" parameter of AddTag to true,
which makes the race invisible.  This commit does not change the behavior
of that case, except for speeding it up by avoiding the
reference.store.save() call.)

The existing reference.store.Get checks are now, in a sense, redundant
as such, but their existence allows the callers to provide nice
context-dependent error messages, so this commit leaves them unchanged.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This commit is contained in:
Miloslav Trmač 2018-09-06 23:24:06 +02:00
parent 4842f7a867
commit f29dda9acd
2 changed files with 13 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
reference/store.go
View file

@ -149,6 +149,11 @@ func (store *store) addReference(ref reference.Named, id digest.Digest, force bo
oldID, exists := repository[refStr]
if exists {
if oldID == id {
// Nothing to do. The caller may have checked for this using store.Get in advance, but store.mu was unlocked in the meantime, so this can legitimately happen nevertheless.
return nil
}
// force only works for tags
if digested, isDigest := ref.(reference.Canonical); isDigest {
return errors.WithStack(conflictingTagError("Cannot overwrite digest " + digested.Digest().String()))

8
reference/store_test.go
View file

@ -163,6 +163,10 @@ func TestAddDeleteGet(t *testing.T) {
if err = store.AddTag(ref4, testImageID2, false); err != nil {
t.Fatalf("error adding to store: %v", err)
}
// Write the same values again; should silently succeed
if err = store.AddTag(ref4, testImageID2, false); err != nil {
t.Fatalf("error redundantly adding to store: %v", err)
}
ref5, err := reference.ParseNormalizedNamed("username/repo3@sha256:58153dfb11794fad694460162bf0cb0a4fa710cfa3f60979c177d920813e267c")
if err != nil {
@ -171,6 +175,10 @@ func TestAddDeleteGet(t *testing.T) {
if err = store.AddDigest(ref5.(reference.Canonical), testImageID2, false); err != nil {
t.Fatalf("error adding to store: %v", err)
}
// Write the same values again; should silently succeed
if err = store.AddDigest(ref5.(reference.Canonical), testImageID2, false); err != nil {
t.Fatalf("error redundantly adding to store: %v", err)
}
// Attempt to overwrite with force == false
if err = store.AddTag(ref4, testImageID3, false); err == nil || !strings.HasPrefix(err.Error(), "Conflict:") {