From a296968c1a0de0a1a79e517c3a38ebeb47fb452b Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Mon, 24 Nov 2025 19:23:52 +0200 Subject: [PATCH] fix: multiple fixes based on recent test failures (#3582) * fix(monitoring): fix race condition in TestPopulateStorageMetrics Write images to filesystem before starting controller to prevent GC from reading index.json while it's being written. See failure: https://github.com/project-zot/zot/actions/runs/19633819097/job/56219606041 Signed-off-by: Andrei Aaron * fix(sync): fix TestSignatures assertion on referrers order Search through all referrers to find the expected OCI ref digest instead of assuming it's at index.Manifests[1], as the order is not guaranteed. See https://github.com/project-zot/zot/actions/runs/19630382617/job/56208492397?pr=3579 Signed-off-by: Andrei Aaron * fix(redis): fix error handling in GetMultipleRepoMeta The function was returning err at the end, which could be nil even when HGetAll should have failed, because err gets reassigned in the loop. Changed to return nil on success path, ensuring HGetAll errors are properly propagated. Fixes TestRedisUnreachable failure. See https://github.com/project-zot/zot/actions/runs/19634990856/job/56223414955?pr=3582 Signed-off-by: Andrei Aaron * refactor(sync): simplify sync test structure and restore file permissions Remove unnecessary nested Convey blocks and integrate error tests into parent blocks. Add defer statements to restore file permissions after chmod operations to prevent side effects on subsequent tests. Fix potential root cause of https://github.com/project-zot/zot/actions/runs/19636284485/job/56227682319?pr=3579 Signed-off-by: Andrei Aaron --------- Signed-off-by: Andrei Aaron --- pkg/extensions/monitoring/monitoring_test.go | 10 ++-- pkg/extensions/sync/sync_test.go | 61 +++++++++++++------- pkg/meta/redis/redis.go | 2 +- 3 files changed, 46 insertions(+), 27 deletions(-) diff --git a/pkg/extensions/monitoring/monitoring_test.go b/pkg/extensions/monitoring/monitoring_test.go index 4eb34edc..76c9e4cc 100644 --- a/pkg/extensions/monitoring/monitoring_test.go +++ b/pkg/extensions/monitoring/monitoring_test.go @@ -453,17 +453,17 @@ func TestPopulateStorageMetrics(t *testing.T) { So(ctlr, ShouldNotBeNil) ctlr.Log = log.NewLoggerWithWriter("debug", writers) - cm := test.NewControllerManager(ctlr) - cm.StartAndWait(port) - defer cm.StopServer() - - // write a couple of images + // Write images before starting controller to avoid race condition with garbage collection srcStorageCtlr := ociutils.GetDefaultStoreController(rootDir, ctlr.Log) err = WriteImageToFileSystem(CreateDefaultImage(), "alpine", "0.0.1", srcStorageCtlr) So(err, ShouldBeNil) err = WriteImageToFileSystem(CreateDefaultImage(), "busybox", "0.0.1", srcStorageCtlr) So(err, ShouldBeNil) + cm := test.NewControllerManager(ctlr) + cm.StartAndWait(port) + defer cm.StopServer() + metrics := monitoring.NewMetricsServer(true, ctlr.Log) sch := scheduler.NewScheduler(conf, metrics, ctlr.Log) sch.RunScheduler() diff --git a/pkg/extensions/sync/sync_test.go b/pkg/extensions/sync/sync_test.go index 0bbe232e..b90d0e4e 100644 --- a/pkg/extensions/sync/sync_test.go +++ b/pkg/extensions/sync/sync_test.go @@ -1461,18 +1461,22 @@ func TestDockerImagesAreSkipped(t *testing.T) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) - Convey("trigger config blob upstream error", func() { - // remove synced image - err := os.RemoveAll(path.Join(destDir, testImage)) - So(err, ShouldBeNil) + // trigger config blob upstream error + // remove synced image + err = os.RemoveAll(path.Join(destDir, testImage)) + So(err, ShouldBeNil) - err = os.Chmod(path.Join(srcDir, testImage, "blobs/sha256", configBlobDigest.Encoded()), 0o000) - So(err, ShouldBeNil) + configBlobPath := path.Join(srcDir, testImage, "blobs/sha256", configBlobDigest.Encoded()) + err = os.Chmod(configBlobPath, 0o000) + So(err, ShouldBeNil) - resp, err = resty.R().Get(destBaseURL + "/v2/" + testImage + "/manifests/" + testImageTag) - So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) - }) + defer func() { + _ = os.Chmod(configBlobPath, storageConstants.DefaultFilePerms) + }() + + resp, err = resty.R().Get(destBaseURL + "/v2/" + testImage + "/manifests/" + testImageTag) + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) }) Convey("skipping already synced multiarch docker image", func() { @@ -1627,18 +1631,22 @@ func TestDockerImagesAreSkipped(t *testing.T) { So(err, ShouldBeNil) So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) - Convey("trigger config blob upstream error", func() { - // remove synced image - err := os.RemoveAll(path.Join(destDir, indexRepoName)) - So(err, ShouldBeNil) + // trigger config blob upstream error + // remove synced image + err = os.RemoveAll(path.Join(destDir, indexRepoName)) + So(err, ShouldBeNil) - err = os.Chmod(path.Join(srcDir, indexRepoName, "blobs/sha256", configBlobDigest.Encoded()), 0o000) - So(err, ShouldBeNil) + configBlobPath := path.Join(srcDir, indexRepoName, "blobs/sha256", configBlobDigest.Encoded()) + err = os.Chmod(configBlobPath, 0o000) + So(err, ShouldBeNil) - resp, err = resty.R().Get(destBaseURL + "/v2/" + indexRepoName + "/manifests/" + "latest") - So(err, ShouldBeNil) - So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) - }) + defer func() { + _ = os.Chmod(configBlobPath, storageConstants.DefaultFilePerms) + }() + + resp, err = resty.R().Get(destBaseURL + "/v2/" + indexRepoName + "/manifests/" + "latest") + So(err, ShouldBeNil) + So(resp.StatusCode(), ShouldEqual, http.StatusNotFound) }) }) } @@ -4855,7 +4863,18 @@ func TestSignatures(t *testing.T) { So(err, ShouldBeNil) So(len(index.Manifests), ShouldEqual, 2) - So(index.Manifests[1].Digest, ShouldEqual, ociRefDigest) + + foundOCIRef := false + + for _, manifest := range index.Manifests { + if manifest.Digest == ociRefDigest { + foundOCIRef = true + + break + } + } + + So(foundOCIRef, ShouldBeTrue) // test negative cases (trigger errors) // test notary signatures errors diff --git a/pkg/meta/redis/redis.go b/pkg/meta/redis/redis.go index d44eb0f3..91435a3b 100644 --- a/pkg/meta/redis/redis.go +++ b/pkg/meta/redis/redis.go @@ -1296,7 +1296,7 @@ func (rc *RedisDB) GetMultipleRepoMeta(ctx context.Context, filter func(repoMeta } } - return foundRepos, err + return foundRepos, nil } // AddManifestSignature adds signature metadata to a given manifest in the database.