From 1ff6f609b70c18f927f2a815f309ac39b2830562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?U=C4=9Fur=20Tafral=C4=B1?= Date: Sat, 28 Feb 2026 20:37:01 +0300 Subject: [PATCH] fix: skip OCI conversion when image is already synced #3823 * Fix #3823: skip OCI conversion when image is already synced When syncRef determines an image is already synced, it now returns a bool to signal the skip. syncImage checks this and returns early before attempting OCI conversion, preventing misleading 'failed to convert docker image to oci' errors caused by a non-existent temp directory. * Keep syncReferrers and CommitAll running for already-synced images Address review feedback: new referrers can be added upstream after initial sync, so we must not skip syncReferrers. Only the OCI conversion is guarded by the skipped flag, since converting an already-stored image is both unnecessary and incorrect. Signed-off-by: Ugur Tafrali --------- Signed-off-by: Ugur Tafrali --- pkg/extensions/sync/service.go | 14 +++---- pkg/extensions/sync/sync_internal_test.go | 51 +++++++++++++++++++++++ 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/pkg/extensions/sync/service.go b/pkg/extensions/sync/service.go index 4c69f993..117aa190 100644 --- a/pkg/extensions/sync/service.go +++ b/pkg/extensions/sync/service.go @@ -461,7 +461,7 @@ func (service *BaseService) SyncRepo(ctx context.Context, repo string) error { func (service *BaseService) syncRef(ctx context.Context, localRepo string, remoteImageRef, localImageRef ref.Ref, remoteDigest godigest.Digest, recursive bool, -) error { +) (bool, error) { var reference string var skipImage bool @@ -498,13 +498,13 @@ func (service *BaseService) syncRef(ctx context.Context, localRepo string, remot Str("local image", fmt.Sprintf("%s:%s", localRepo, remoteImageRef.Tag)).Msg("failed to sync image") } - return err + return false, err } service.log.Info().Str("image", remoteImageRef.CommonName()). Msg("skipping image because it's already synced") - return nil + return true, nil } // get "would be" digest of image after synced. @@ -617,7 +617,7 @@ func (service *BaseService) syncImage(ctx context.Context, localRepo, remoteRepo defer service.destination.CleanupImage(localImageRef, localRepo) //nolint: errcheck // first sync image - err = service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, localDigest, false) + skipped, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, localDigest, false) if err != nil { return err } @@ -627,7 +627,7 @@ func (service *BaseService) syncImage(ctx context.Context, localRepo, remoteRepo } // convert image to oci if needed - if isConverted && !service.config.PreserveDigest { + if !skipped && isConverted && !service.config.PreserveDigest { localImageRef, err = mod.Apply(ctx, service.rc, localImageRef, mod.WithRefTgt(localImageRef), mod.WithManifestToOCI(), @@ -737,7 +737,7 @@ func (service *BaseService) syncReferrers(ctx context.Context, tags []string, lo localImageRef = localImageRef.SetDigest(desc.Digest.String()) - err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, desc.Digest, false) + _, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, desc.Digest, false) if err != nil { service.log.Error().Err(err).Str("errortype", common.TypeOf(err)). Str("repo", localRepo).Str("local reference", localImageRef.Tag). @@ -755,7 +755,7 @@ func (service *BaseService) syncReferrers(ctx context.Context, tags []string, lo localImageRef = localImageRef.SetTag(tag) - err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, remoteDigest, true) + _, err := service.syncRef(ctx, localRepo, remoteImageRef, localImageRef, remoteDigest, true) if err != nil { service.log.Error().Err(err).Str("errortype", common.TypeOf(err)). Str("repo", localRepo).Str("local reference", localImageRef.Tag). diff --git a/pkg/extensions/sync/sync_internal_test.go b/pkg/extensions/sync/sync_internal_test.go index 258b6afb..06cafca8 100644 --- a/pkg/extensions/sync/sync_internal_test.go +++ b/pkg/extensions/sync/sync_internal_test.go @@ -127,6 +127,57 @@ func TestService(t *testing.T) { So(panicOccurred, ShouldBeTrue) }) + Convey("test syncImage skips OCI conversion but still commits when image already synced", t, func() { + conf := syncconf.RegistryConfig{ + URLs: []string{"http://localhost"}, + } + + service, err := New(conf, "", nil, t.TempDir(), storage.StoreController{}, mocks.MetaDBMock{}, log.NewTestLogger()) + So(err, ShouldBeNil) + + // Mock remote returns isConverted=true so OCI conversion would be attempted if not skipped + mockRemote := &mocks.SyncRemoteMock{ + GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) { + return ref.New("mock-registry/" + repo + ":" + tag) + }, + GetOCIDigestFn: func(ctx context.Context, repo, tag string) (godigest.Digest, godigest.Digest, bool, error) { + // isConverted=true means OCI conversion would be attempted + return godigest.Digest("sha256:abc123"), godigest.Digest("sha256:def456"), true, nil + }, + } + service.remote = mockRemote + + commitAllCalled := false + + // Mock destination returns CanSkipImage=true (already synced) + mockDest := &mocks.SyncDestinationMock{ + GetImageReferenceFn: func(repo string, tag string) (ref.Ref, error) { + return ref.New("local/" + repo + ":" + tag) + }, + CanSkipImageFn: func(repo string, tag string, digest godigest.Digest) (bool, error) { + return true, nil + }, + CommitAllFn: func(repo string, imageReference ref.Ref) error { + commitAllCalled = true + return nil + }, + CleanupImageFn: func(imageReference ref.Ref, repo string) error { + return nil + }, + } + service.destination = mockDest + + ctx := context.Background() + err = service.syncImage(ctx, "localrepo", "remoterepo", "tag1", []string{}, false) + + // Should succeed without error + So(err, ShouldBeNil) + // CommitAll should still be called to persist any new referrers even when image was skipped + So(commitAllCalled, ShouldBeTrue) + // OCI conversion is NOT called because skipped=true guards it. + // If mod.Apply were called, it would fail since service.rc is nil - proving the guard works. + }) + Convey("test syncImage ReferrerList error with OnlySigned", t, func() { onlySigned := true conf := syncconf.RegistryConfig{