Skip to content

Commit 60bfd16

Browse files
OrexiiMalcolm MorganTerryHowe
authored
fix(copy): re-push referenced manifests when the destination reports them missing (#1213)
Backport of #1212 to the `v2` branch. ## What this does Makes `CopyGraph` self-heal a partially-copied graph. When the destination rejects a manifest push because it is missing referenced content (`MANIFEST_BLOB_UNKNOWN` / `BLOB_UNKNOWN` / `MANIFEST_UNKNOWN`), the copy now re-pushes the node's manifest successors and retries the push once, instead of returning the error. ## Why `CopyGraph` skips a node whose sub-DAG it believes already exists ("skip if a rooted sub-DAG exists"). If a prior copy was interrupted after a child manifest's blobs were uploaded but before the child manifest was committed, the destination can report the child as present (`Exists`/HEAD) yet reject the parent that references it. For a multi-arch image index this means the index push fails with `MANIFEST_BLOB_UNKNOWN`, and since the child is skipped on every run, re-running the same `oras cp --recursive` never recovers. This is the line that reaches the ORAS CLI. This was hit in production mirroring a multi-arch image (`mongo:7.0.34`) between two registries; the only manual workaround was copying each child manifest by digest and re-pushing the index. ## How - `copyGraph`: wrap the node push; on a "missing referenced content" error, re-copy the manifest successors unconditionally (bypassing the existence short-circuit) and retry the push once. No-op on the success path; blob successors are not re-pushed. - `isMissingReferencedContentError`: typed classification via `errcode` (handles both a single `errcode.Error` and a multi-error `errcode.Errors`); no string matching. ## Testing - `TestCopyGraph_SelfHealsInterruptedIndexCopy`: end-to-end reproduction against a destination that emulates the interrupted state — fails without this change (`MANIFEST_BLOB_UNKNOWN`), passes with it. - `TestIsMissingReferencedContentError`: unit test for the classifier (100% covered). - `go test -race ./...` passes; `gofmt` / `go vet` clean; new files carry the Apache license header. The diff is identical to #1212 apart from the module import paths (`github.com/oras-project/oras-go/v3` -> `oras.land/oras-go/v2`). Relates to #1211. Signed-off-by: Malcolm Morgan <Malcolm.Morgan@Microsoft.com> Co-authored-by: Malcolm Morgan <Malcolm.Morgan@Microsoft.com> Co-authored-by: Terry Howe <terrylhowe@gmail.com>
1 parent 105715e commit 60bfd16

3 files changed

Lines changed: 366 additions & 3 deletions

File tree

‎copy.go‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ import (
3232
"oras.land/oras-go/v2/internal/status"
3333
"oras.land/oras-go/v2/internal/syncutil"
3434
"oras.land/oras-go/v2/registry"
35+
"oras.land/oras-go/v2/registry/remote/errcode"
3536
)
3637

3738
// defaultConcurrency is the default value of CopyGraphOptions.Concurrency.
@@ -272,15 +273,85 @@ func copyGraph(ctx context.Context, src content.ReadOnlyStorage, dst content.Sto
272273
if err != nil {
273274
return fmt.Errorf("failed to check cache existence: %s: %w", desc.Digest, err)
274275
}
275-
if exists {
276-
return copyNode(ctx, proxy.Cache, dst, desc, opts)
276+
pushNode := func() error {
277+
if exists {
278+
return copyNode(ctx, proxy.Cache, dst, desc, opts)
279+
}
280+
return mountOrCopyNode(ctx, src, dst, desc, opts)
277281
}
278-
return mountOrCopyNode(ctx, src, dst, desc, opts)
282+
pushErr := pushNode()
283+
if pushErr != nil && len(successors) != 0 && isMissingReferencedContentError(pushErr) {
284+
// The push was rejected because the destination is missing content
285+
// that this manifest references. This can happen when a previous copy
286+
// was interrupted after a referenced manifest's blobs were uploaded but
287+
// before that manifest itself was committed: the existence check above
288+
// then reports the referenced manifest as present and skips it, yet the
289+
// registry rejects this node for referencing it. Re-copy the manifest
290+
// successors unconditionally and retry the push once so that an
291+
// interrupted copy can self-heal instead of failing identically on
292+
// every retry.
293+
for _, node := range successors {
294+
if !descriptor.IsManifest(node) {
295+
// Blob existence is reported reliably by Exists, so
296+
// re-pushing large blobs on this recovery path would be
297+
// wasteful.
298+
continue
299+
}
300+
if err := doCopyNode(ctx, src, dst, node); err != nil {
301+
return err
302+
}
303+
}
304+
pushErr = pushNode()
305+
}
306+
return pushErr
279307
}
280308

281309
return syncutil.Go(ctx, limiter, fn, root)
282310
}
283311

312+
// isMissingReferencedContentError reports whether err indicates that a
313+
// destination registry rejected a manifest because one or more of the
314+
// descriptors referenced by that manifest are not present in the destination.
315+
//
316+
// A registry returns MANIFEST_BLOB_UNKNOWN or BLOB_UNKNOWN when a pushed
317+
// manifest references content that the registry does not have; for an image
318+
// index that referenced content is its child manifests. copyGraph uses this to
319+
// trigger a one-shot, self-healing retry for graphs whose interior nodes were
320+
// skipped as already-existing but were not in fact durably committed by a prior
321+
// interrupted copy.
322+
//
323+
// References:
324+
// - https://github.com/opencontainers/distribution-spec/blob/v1.1.1/spec.md#error-codes
325+
func isMissingReferencedContentError(err error) bool {
326+
isMissingCode := func(code string) bool {
327+
switch code {
328+
case errcode.ErrorCodeManifestBlobUnknown,
329+
errcode.ErrorCodeBlobUnknown,
330+
errcode.ErrorCodeManifestUnknown:
331+
return true
332+
default:
333+
return false
334+
}
335+
}
336+
// A single inner error, e.g. exactly one missing descriptor.
337+
var ec errcode.Error
338+
if errors.As(err, &ec) && isMissingCode(ec.Code) {
339+
return true
340+
}
341+
// Multiple inner errors, e.g. several missing descriptors. errcode.Errors
342+
// only unwraps to a single errcode.Error when it holds exactly one element,
343+
// so the multi-error case must be inspected explicitly.
344+
var ecs errcode.Errors
345+
if errors.As(err, &ecs) {
346+
for _, e := range ecs {
347+
if isMissingCode(e.Code) {
348+
return true
349+
}
350+
}
351+
}
352+
return false
353+
}
354+
284355
// mountOrCopyNode tries to mount the node, if not falls back to copying.
285356
func mountOrCopyNode(ctx context.Context, src content.ReadOnlyStorage, dst content.Storage, desc ocispec.Descriptor, opts CopyGraphOptions) error {
286357
// Need MountFrom and it must be a blob

‎copy_selfheal_internal_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
Copyright The ORAS Authors.
3+
Licensed under the Apache License, Version 2.0 (the "License");
4+
you may not use this file except in compliance with the License.
5+
You may obtain a copy of the License at
6+
7+
http://www.apache.org/licenses/LICENSE-2.0
8+
9+
Unless required by applicable law or agreed to in writing, software
10+
distributed under the License is distributed on an "AS IS" BASIS,
11+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
See the License for the specific language governing permissions and
13+
limitations under the License.
14+
*/
15+
16+
package oras
17+
18+
import (
19+
"errors"
20+
"testing"
21+
22+
"oras.land/oras-go/v2/registry/remote/errcode"
23+
)
24+
25+
func TestIsMissingReferencedContentError(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
err error
29+
want bool
30+
}{
31+
{
32+
name: "nil error",
33+
err: nil,
34+
want: false,
35+
},
36+
{
37+
name: "unrelated error",
38+
err: errors.New("some other error"),
39+
want: false,
40+
},
41+
{
42+
name: "single MANIFEST_BLOB_UNKNOWN",
43+
err: errcode.Error{Code: errcode.ErrorCodeManifestBlobUnknown},
44+
want: true,
45+
},
46+
{
47+
name: "single BLOB_UNKNOWN",
48+
err: errcode.Error{Code: errcode.ErrorCodeBlobUnknown},
49+
want: true,
50+
},
51+
{
52+
name: "single MANIFEST_UNKNOWN",
53+
err: errcode.Error{Code: errcode.ErrorCodeManifestUnknown},
54+
want: true,
55+
},
56+
{
57+
name: "single unrelated code",
58+
err: errcode.Error{Code: errcode.ErrorCodeNameUnknown},
59+
want: false,
60+
},
61+
{
62+
name: "multiple errors with a missing code",
63+
err: errcode.Errors{
64+
{Code: errcode.ErrorCodeNameUnknown},
65+
{Code: errcode.ErrorCodeManifestBlobUnknown},
66+
},
67+
want: true,
68+
},
69+
{
70+
name: "multiple errors without a missing code",
71+
err: errcode.Errors{
72+
{Code: errcode.ErrorCodeNameUnknown},
73+
{Code: errcode.ErrorCodeDenied},
74+
},
75+
want: false,
76+
},
77+
{
78+
name: "wrapped in CopyError",
79+
err: newCopyError("Push", CopyErrorOriginDestination, errcode.Errors{
80+
{Code: errcode.ErrorCodeManifestBlobUnknown},
81+
}),
82+
want: true,
83+
},
84+
}
85+
86+
for _, tt := range tests {
87+
t.Run(tt.name, func(t *testing.T) {
88+
if got := isMissingReferencedContentError(tt.err); got != tt.want {
89+
t.Errorf("isMissingReferencedContentError() = %v, want %v", got, tt.want)
90+
}
91+
})
92+
}
93+
}

‎copy_selfheal_test.go‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
/*
2+
Copyright The ORAS Authors.
3+
Licensed under the Apache License, Version 2.0 (the "License");
4+
you may not use this file except in compliance with the License.
5+
You may obtain a copy of the License at
6+
7+
http://www.apache.org/licenses/LICENSE-2.0
8+
9+
Unless required by applicable law or agreed to in writing, software
10+
distributed under the License is distributed on an "AS IS" BASIS,
11+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
See the License for the specific language governing permissions and
13+
limitations under the License.
14+
*/
15+
16+
package oras_test
17+
18+
import (
19+
"bytes"
20+
"context"
21+
_ "crypto/sha256"
22+
"encoding/json"
23+
"io"
24+
"sync"
25+
"testing"
26+
27+
"github.com/opencontainers/go-digest"
28+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
29+
"oras.land/oras-go/v2"
30+
"oras.land/oras-go/v2/content"
31+
"oras.land/oras-go/v2/internal/cas"
32+
"oras.land/oras-go/v2/registry/remote/errcode"
33+
)
34+
35+
// interruptedIndexDst simulates a destination registry left in the partially
36+
// populated state produced by an interrupted recursive copy of a multi-arch
37+
// image index: the child manifests' blobs have already been uploaded, but the
38+
// child manifests themselves were never durably committed. The destination
39+
// nonetheless reports those child manifests as already existing (a HEAD
40+
// false-positive), so a plain copy skips re-pushing them; and it rejects any
41+
// index that references a not-yet-committed child manifest with
42+
// MANIFEST_BLOB_UNKNOWN, exactly as a spec-compliant registry does.
43+
type interruptedIndexDst struct {
44+
content.Storage
45+
mu sync.Mutex
46+
phantom map[digest.Digest]bool // reported as existing but not yet committed
47+
committed map[digest.Digest]bool // actually pushed during the copy
48+
}
49+
50+
func newInterruptedIndexDst(storage content.Storage, phantom ...digest.Digest) *interruptedIndexDst {
51+
d := &interruptedIndexDst{
52+
Storage: storage,
53+
phantom: make(map[digest.Digest]bool),
54+
committed: make(map[digest.Digest]bool),
55+
}
56+
for _, dg := range phantom {
57+
d.phantom[dg] = true
58+
}
59+
return d
60+
}
61+
62+
func (d *interruptedIndexDst) Exists(ctx context.Context, target ocispec.Descriptor) (bool, error) {
63+
d.mu.Lock()
64+
phantom := d.phantom[target.Digest] && !d.committed[target.Digest]
65+
d.mu.Unlock()
66+
if phantom {
67+
// Falsely report the not-yet-committed child manifest as present.
68+
return true, nil
69+
}
70+
return d.Storage.Exists(ctx, target)
71+
}
72+
73+
func (d *interruptedIndexDst) Push(ctx context.Context, expected ocispec.Descriptor, r io.Reader) error {
74+
body, err := io.ReadAll(r)
75+
if err != nil {
76+
return err
77+
}
78+
if expected.MediaType == ocispec.MediaTypeImageIndex {
79+
var idx ocispec.Index
80+
if err := json.Unmarshal(body, &idx); err != nil {
81+
return err
82+
}
83+
var missing errcode.Errors
84+
d.mu.Lock()
85+
for _, m := range idx.Manifests {
86+
if d.phantom[m.Digest] && !d.committed[m.Digest] {
87+
missing = append(missing, errcode.Error{
88+
Code: errcode.ErrorCodeManifestBlobUnknown,
89+
Message: "blob unknown to registry",
90+
Detail: m.Digest.String(),
91+
})
92+
}
93+
}
94+
d.mu.Unlock()
95+
if len(missing) > 0 {
96+
// Reject the index just like a registry whose referenced child
97+
// manifests are not all present.
98+
return missing
99+
}
100+
}
101+
d.mu.Lock()
102+
if d.phantom[expected.Digest] {
103+
d.committed[expected.Digest] = true
104+
}
105+
d.mu.Unlock()
106+
return d.Storage.Push(ctx, expected, bytes.NewReader(body))
107+
}
108+
109+
// TestCopyGraph_SelfHealsInterruptedIndexCopy reproduces the failure in which a
110+
// re-run of a recursive copy of a multi-arch image index never self-heals: the
111+
// not-yet-committed child manifests are reported as already existing and thus
112+
// skipped, and the subsequent index push is rejected with MANIFEST_BLOB_UNKNOWN.
113+
// With the self-healing retry in copyGraph, the skipped child manifests are
114+
// re-pushed and the index push succeeds.
115+
func TestCopyGraph_SelfHealsInterruptedIndexCopy(t *testing.T) {
116+
ctx := context.Background()
117+
src := cas.NewMemory()
118+
119+
var blobs [][]byte
120+
var descs []ocispec.Descriptor
121+
appendBlob := func(mediaType string, blob []byte) ocispec.Descriptor {
122+
desc := ocispec.Descriptor{
123+
MediaType: mediaType,
124+
Digest: digest.FromBytes(blob),
125+
Size: int64(len(blob)),
126+
}
127+
blobs = append(blobs, blob)
128+
descs = append(descs, desc)
129+
return desc
130+
}
131+
makeManifest := func(config ocispec.Descriptor, layers ...ocispec.Descriptor) ocispec.Descriptor {
132+
m := ocispec.Manifest{Config: config, Layers: layers}
133+
j, err := json.Marshal(m)
134+
if err != nil {
135+
t.Fatal(err)
136+
}
137+
return appendBlob(ocispec.MediaTypeImageManifest, j)
138+
}
139+
makeIndex := func(manifests ...ocispec.Descriptor) ocispec.Descriptor {
140+
idx := ocispec.Index{Manifests: manifests}
141+
j, err := json.Marshal(idx)
142+
if err != nil {
143+
t.Fatal(err)
144+
}
145+
return appendBlob(ocispec.MediaTypeImageIndex, j)
146+
}
147+
148+
// A two-platform index: each child manifest has its own config + layer,
149+
// mirroring linux/amd64 and linux/arm64 children of mongo:7.0.34.
150+
cfgA := appendBlob(ocispec.MediaTypeImageConfig, []byte("config-amd64"))
151+
layerA := appendBlob(ocispec.MediaTypeImageLayer, []byte("layer-amd64"))
152+
cfgB := appendBlob(ocispec.MediaTypeImageConfig, []byte("config-arm64"))
153+
layerB := appendBlob(ocispec.MediaTypeImageLayer, []byte("layer-arm64"))
154+
manifestA := makeManifest(cfgA, layerA)
155+
manifestB := makeManifest(cfgB, layerB)
156+
root := makeIndex(manifestA, manifestB)
157+
158+
for i := range blobs {
159+
if err := src.Push(ctx, descs[i], bytes.NewReader(blobs[i])); err != nil {
160+
t.Fatalf("failed to seed src: %d: %v", i, err)
161+
}
162+
}
163+
164+
// Simulate the interrupted-copy state in the destination: every blob has
165+
// already been uploaded, but the two child manifests were never committed
166+
// (and the destination reports them as existing).
167+
backing := cas.NewMemory()
168+
for _, d := range []ocispec.Descriptor{cfgA, layerA, cfgB, layerB} {
169+
body, err := content.FetchAll(ctx, src, d)
170+
if err != nil {
171+
t.Fatal(err)
172+
}
173+
if err := backing.Push(ctx, d, bytes.NewReader(body)); err != nil {
174+
t.Fatal(err)
175+
}
176+
}
177+
dst := newInterruptedIndexDst(backing, manifestA.Digest, manifestB.Digest)
178+
179+
// Copy the index. Without the self-healing retry this fails with
180+
// MANIFEST_BLOB_UNKNOWN; with it, the copy succeeds.
181+
if err := oras.CopyGraph(ctx, src, dst, root, oras.CopyGraphOptions{}); err != nil {
182+
t.Fatalf("CopyGraph() error = %v, want nil", err)
183+
}
184+
185+
// The index and both child manifests must now be present and correct.
186+
for _, d := range []ocispec.Descriptor{manifestA, manifestB, root} {
187+
got, err := content.FetchAll(ctx, backing, d)
188+
if err != nil {
189+
t.Fatalf("missing content %s after copy: %v", d.Digest, err)
190+
}
191+
want, err := content.FetchAll(ctx, src, d)
192+
if err != nil {
193+
t.Fatal(err)
194+
}
195+
if !bytes.Equal(got, want) {
196+
t.Fatalf("content mismatch for %s", d.Digest)
197+
}
198+
}
199+
}

0 commit comments

Comments
 (0)