fix(handlers): disable filesystem layer link metadata if the database is enabled

added Category:Container Registry backend devopspackage golang sectionops labels

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.

Reviewer	Maintainer
Jaime Martínez (`@jaime`) (UTC+10, 17 hours ahead of `@hswimelar`)	Kamil Trzciński (`@ayufan`) (UTC+2, 9 hours ahead of `@hswimelar`)

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

@suleimiahmed could you please review?

requested review from @suleimiahmed

Proper labels assigned to this merge request. Please ignore me.

@hswimelar - please see the following guidance and update this merge request.

	1 Error
	Please add typebug typefeature, or typemaintenance label to this merge request.

changed the description

added bugfunctional label

added typebug label

@hswimelar I think we can also repurpose/remove the following tests:

TestManifestAPI_Get_Schema2LayersAndConfigNotInDatabase
TestManifestAPI_Put_OCIImageIndexByTagManifestsNotPresentInDatabase
TestManifestAPI_Delete_Schema2ManifestNotInDatabase

Since they also target the FS metadata, when the metadata database is enabled.

My understanding is these case should no longer be possible in reality, and the current failures are test specific (resulting from bypassing the registry test configs via the writeToFilesystemOnly option here)

requested review from @stanhu

removed review request for @stanhu

added 2 commits

bd0c0e8d - chore: fix failing tests
e2c948db - chore: add test to ensure the database isn't writting layer metadata

Compare with previous version

added 3 commits

7dbb222b - fix(handlers): disable filesystem layer link metadata if the databae is enabled
2dae9252 - chore: fix failing tests
7d8ee5ad - chore: add test to ensure the database isn't writting layer metadata

Compare with previous version

added 3 commits

16c7fcfc - fix(handlers): disable filesystem layer link metadata if the database is enabled
9e76e171 - chore: fix failing tests
608f340b - chore: add test to ensure the database isn't writting layer metadata

Compare with previous version

changed title from fix: fsmirroring being stuck on to fix(handlers): disable filesystem layer link metadata if the database is enabled

This test is changing from the 'manifest in database, but layers on FS' scenario, which is now basically impossible without migration mode and FS mirroring.

We're now simply checking the registry has no visibility into FS metadata when the database is enabled.

Should this fail if I comment out the boolean?

diff --git a/registry/handlers/app.go b/registry/handlers/app.go
index 8d7cc30c..035a89c5 100644
--- a/registry/handlers/app.go
+++ b/registry/handlers/app.go
@@ -320,7 +320,7 @@ func NewApp(ctx context.Context, config *configuration.Configuration) (*App, err
 		log.Warn("the metadata database is an experimental feature, please do not enable it in production")
 
 		// Do not write or check for repository layer link metadata on the filesystem when the database is enabled.
-		options = append(options, storage.DisableMirrorFS)
+		//options = append(options, storage.DisableMirrorFS)
 
 		// TODO: this function only exists to test https://gitlab.com/gitlab-org/container-registry/-/issues/890
 		// as the migration code runs on a separate container that does not output any searchable logs.

I tried running the tests via:

go test -tags=integration,handlers_test ./registry/handlers

Oh, I guess I need to have a database enabled?

Oh, I guess I need to have a database enabled? @stanhu Right. Also these tests pick up the DB info from the environment. See: https://gitlab.com/gitlab-org/container-registry/-/blob/master/docs-gitlab/database-local-setup.md#integration-tests

@hswimelar Ok, I got the database running, but none of the tests failed when I applied the diff above. I ran:

go run gotest.tools/gotestsum@v1.8.2 --format testname -- ./registry/handlers  -timeout 25m  --tags api_conformance_test,integration

<snip>
PASS registry/handlers.TestAPIConformance/manifest_Get_OCIIndex_MatchingEtag_all/by_digest_quoted_etag (0.00s)
PASS registry/handlers.TestAPIConformance/manifest_Get_OCIIndex_MatchingEtag_all/by_tag_non_quoted_etag (0.01s)
PASS registry/handlers.TestAPIConformance/manifest_Get_OCIIndex_MatchingEtag_all/by_digest_non_quoted_etag (0.00s)
PASS registry/handlers.TestAPIConformance/manifest_Get_OCIIndex_MatchingEtag_all (1.27s)
PASS registry/handlers.TestAPIConformance/manifest_Put_OCIImageIndex_ByTag_all (1.11s)
PASS registry/handlers.TestAPIConformance/manifest_Put_OCIImageIndex_ByTag_with_notifications_enabled (1.15s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_with_notifications_enabled/by_tag (0.01s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_with_notifications_enabled/by_digest (0.01s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_with_notifications_enabled (0.71s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_all/by_tag (0.01s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_all/by_digest (0.01s)
PASS registry/handlers.TestAPIConformance/manifest_Head_Schema2_MissingManifest_all (0.80s)
PASS registry/handlers.TestAPIConformance/manifest_Put_OCI_ByDigest_with_notifications_enabled (0.76s)
PASS registry/handlers.TestAPIConformance/manifest_Put_OCI_ByDigest_all (0.70s)
PASS registry/handlers.TestAPIConformance/manifest_Delete_Schema2_Reupload_with_notifications_enabled (0.73s)
PASS registry/handlers.TestAPIConformance/manifest_Delete_Schema2_Reupload_all (0.83s)
PASS registry/handlers.TestAPIConformance (93.39s)
PASS registry/handlers

DONE 294 tests in 110.505s

I'll approve for merge since the change seems fairly straightforward. I'll leave it up to you if you want to wrangle this test into shape.

Fixed with !1298 (b36e49cf)

CC: @jaime @suleimiahmed

With this particular fix, only TestManifestAPI_Put_Schema2WritesNoFilesystemBlobLinkMetadata is expected to fail with options = append(options, storage.DisableMirrorFS) commented out. Since that's looking at the layer link metadata, the other tests are checking against visibility into manifest metadata to prevent a regression there, which isn't controlled by storage.DisableMirrorFS, which is confusing, see @jaime 's comment here for a follow-up

I'm not sure what's going on with my test environment, but with PostgreSQL 13 I did:

initdb /tmp/pg
postgres -D /tmp/pg

Then:

psql -U template1
create database registry_test;

In my env, I have:

REGISTRY_DATABASE_ENABLED=true
REGISTRY_DATABASE_PORT=5432
REGISTRY_DATABASE_HOST=127.0.0.1
REGISTRY_DATABASE_USER=stanhu
REGISTRY_DATABASE_PASSWORD=
REGISTRY_DATABASE_SSLMODE=disable

I ran:

go run gotest.tools/gotestsum@v1.8.2 --format testname -- ./... -timeout 25m  --tags handlers_test,integration

Then I got all sorts of odd failures:

test.txt

Not sure if this is just a test setup or PostgreSQL issue, but I don't really have more time to spend on this.

I recommend that this project start testing against PostgreSQL 13 since that's now the default in GitLab 16.0 and Go v1.19.8 if you aren't already.

go run gotest.tools/gotestsum@v1.8.2 --format testname -- ./... -timeout 25m  --tags handlers_test,integration

The ./... is the issue here, this is running both the API integration tests and the lower-level database tests. These tests suites handle set ups and tear downs of the single database environment differently, which is causing the issues.

These tests are all ran in separate CI jobs with there own database and we isolate them when running locally by convention as well. Both the following worked for me on PG 13:

go run gotest.tools/gotestsum@v1.8.2 --format testname -- github.com/docker/distribution/registry/handlers -timeout 25m  --tags handlers_test,integration

go run gotest.tools/gotestsum@v1.8.2 --format testname -- github.com/docker/distribution/registry/datastore -timeout 25m  --tags handlers_test,integration

I recommend that this project start testing against PostgreSQL 13 since that's now the default in GitLab 16.0 and Go v1.19.8 if you aren't already.

The go version is covered, I'll open an issue for PostgreSQL 13

requested review from @jdrpereira

changed milestone to %16.0

removed review request for @jdrpereira

Setting label(s) groupcontainer registry based on Category:Container Registry.

added groupcontainer registry label

mentioned in issue gitlab-org/quality/quality-engineering/team-tasks#1796 (closed)

approved this merge request

changed the description

Updated the description to point in the right failure location (i.e https://gitlab.com/gitlab-org/container-registry/-/blob/v3.73.0-gitlab/registry/storage/linkedblobstore.go#L76 rather than https://gitlab.com/gitlab-org/container-registry/-/blob/v3.73.0-gitlab/registry/storage/blobserver.go#L34, please change back if this is not correct

@jaime could you check this over too, always good to get more scrutiny on something like this;

requested review from @jaime

approved this merge request

mentioned in issue #1023

requested review from @stanhu

approved this merge request

added 1 commit

b36e49cf - chore: use shared driver for test environments

Compare with previous version

approved this merge request

resolved all threads

merged

mentioned in commit 6c061598

mentioned in issue #1024 (closed)

mentioned in issue #936 (closed)

mentioned in issue gitlab-org/ci-cd/package-stage/container-registry-group/async-updates#3 (closed)

mentioned in issue gitlab-org/ci-cd/package-stage/container-registry-group/async-updates#4 (closed)

mentioned in merge request !1595 (merged)

 	return
+}
 func TestManifestAPI_Get_Schema2LayersAndConfigNotInDatabase(t *testing.T) {
 	env := newTestEnv(t)
 	defer env.Shutdown()
 func TestManifestAPI_Get_Schema2NotInDatabase(t *testing.T) {

fix(handlers): disable filesystem layer link metadata if the database is enabled

Merge request reports

Activity

Reviewer roulette