Add StorageID to Repository entity #8502

itaigilo · 2025-01-15T13:42:37Z

Closes #8495.

github-actions · 2025-01-15T13:44:00Z

♻️ PR Preview 6d2d2e7 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2025-01-15T13:50:29Z

E2E Test Results - Quickstart

github-actions · 2025-01-15T13:52:10Z

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z

Solid work.
I have some comments that I think require our attention before approval.
As well as missing unit tests for graveler/catalog level

N-o-Z · 2025-01-15T23:46:26Z

pkg/gateway/testutil/gateway_setup.go

@@ -60,7 +60,8 @@ func GetBasicHandler(t *testing.T, authService *FakeAuthService, repoName string
 		storageNamespace = "replay"
 	}

-	_, err = c.CreateRepository(ctx, repoName, storageNamespace, "main", false)
+	// TODO (gilo): test storageID here?


Can you explain? This is a test utility - what would you like to test here?

N-o-Z · 2025-01-15T23:52:45Z

pkg/graveler/validate.go

+		panic(ErrInvalidType)
+	}
+
+	// TODO (gilo): Any other validations?


I don't think this validation is required at all at this point. The storage ids already exist in the lakefs config so the parameter either matches one of them or it doesn't

We should however definitely define the requirements for a valid storage_id string and enforce during configuration load.
Lets just decide on min+max length + allowed chars and coordinate with @treeverse/product on this.
We should open an appropriate issue to enforce this validation.

N-o-Z · 2025-01-15T23:57:35Z

pkg/catalog/catalog.go

 	storageNS := graveler.StorageNamespace(storageNamespace)
 	branchID := graveler.BranchID(branch)
 	if err := validator.Validate([]validator.ValidateArg{
 		{Name: "name", Value: repositoryID, Fn: graveler.ValidateRepositoryID},
+		{Name: "storageID", Value: storageIdentifier, Fn: graveler.ValidateStorageID},


No need for a validation on storage_id it's a given in the configuration at this point

N-o-Z · 2025-01-16T00:01:43Z

pkg/api/controller.go

@@ -2010,7 +2010,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo
 	if swag.BoolValue(params.Bare) {
 		// create a bare repository. This is useful in conjunction with refs-restore to create a copy
 		// of another repository by e.g. copying the _lakefs/ directory and restoring its refs
-		repo, err := c.Catalog.CreateBareRepository(ctx, body.Name, body.StorageNamespace, defaultBranch, swag.BoolValue(body.ReadOnly))
+		repo, err := c.Catalog.CreateBareRepository(ctx, body.Name, "", body.StorageNamespace, defaultBranch, swag.BoolValue(body.ReadOnly))


Since only the blockstore is aware of the current configuration (using either blockstore or blockstores), we should have a validation at the adapter level which checks whether this value is "Valid". For OSS the validation should allow empty value (or enforce it - I don't really know if we should allow providing any storage_id in that case)

Makes sense.
Removed this validation.

itaigilo

@N-o-Z validation removed.
PTAL again.

N-o-Z

Thanks! Some more comments

N-o-Z · 2025-01-16T22:08:19Z

pkg/catalog/catalog.go

@@ -456,12 +457,13 @@ func (c *Catalog) CreateRepository(ctx context.Context, repository string, stora
 	}); err != nil {
 		return nil, err
 	}
-	repo, err := c.Store.CreateRepository(ctx, repositoryID, storageNS, branchID, readOnly)
+	repo, err := c.Store.CreateRepository(ctx, repositoryID, storageIdentifier, storageNS, branchID, readOnly)


As mentioned in the previous comment and also discussed with @guy-har.
We should probably have someone validate the storage_id here.
Either by a dedicated method in the adapter or by getting the list of block configurations.
In the OSS this should lead to an error whenever create repository is provided with a storageID which is not empty.
We cannot merge the changes in the current state since this could lead to potential bugs in case we release a lakeFS version during development.
If we want to defer the decision of how we want to handle this, the minimum we need to do is to hardcode the current behavior so that any value other than empty will return error, and not to allow creating repositories with explicit storage IDs ATM

N-o-Z · 2025-01-16T22:10:43Z

pkg/api/controller_test.go

 		testutil.Must(t, err)
-		_, err = deps.catalog.CreateRepository(ctx, "foo2", onBlock(deps, "foo1"), "main", false)
+		_, err = deps.catalog.CreateRepository(ctx, "foo2", "storage", onBlock(deps, "foo1"), "main", false)


We should use empty storageID for controller tests ATM

N-o-Z · 2025-01-16T22:12:28Z

pkg/catalog/catalog_test.go

@@ -72,12 +72,12 @@ func TestCatalog_ListRepositories(t *testing.T) {
 	// prepare data tests
 	now := time.Now()
 	gravelerData := []*graveler.RepositoryRecord{


Please make sure to have one record with no StorageID (empty) as part of our tests

Right. Added.

N-o-Z · 2025-01-16T22:13:54Z

pkg/catalog/catalog_test.go

@@ -871,6 +871,7 @@ func createPrepareUncommittedTestScenario(t *testing.T, repositoryID string, num
 	repository := &graveler.RepositoryRecord{
 		RepositoryID: graveler.RepositoryID(repositoryID),
 		Repository: &graveler.Repository{
+			StorageID:        "storage",


Is this change relevant at the moment?

We can remove this row and it will still work -
But I rather keep this as an extra test.

Lets remove that - and address these tests in the right context (GC).
I prefer we don't modify the test without understanding what we'd want to test here

itaigilo

Thanks for your comments.

@N-o-Z can you PTAL again?

itaigilo · 2025-01-17T15:17:00Z

pkg/catalog/catalog_test.go

@@ -72,12 +72,12 @@ func TestCatalog_ListRepositories(t *testing.T) {
 	// prepare data tests
 	now := time.Now()
 	gravelerData := []*graveler.RepositoryRecord{


Right. Added.

itaigilo · 2025-01-17T15:23:23Z

pkg/catalog/catalog_test.go

@@ -871,6 +871,7 @@ func createPrepareUncommittedTestScenario(t *testing.T, repositoryID string, num
 	repository := &graveler.RepositoryRecord{
 		RepositoryID: graveler.RepositoryID(repositoryID),
 		Repository: &graveler.Repository{
+			StorageID:        "storage",


We can remove this row and it will still work -
But I rather keep this as an extra test.

This reverts commit 298bc0d.

This reverts commit 299c292.

N-o-Z

Great!
Thanks for the hard work!
I'm approving but I guess we still have some decisions to make here

* Add StorageID to Repo endpoints * Add param to Creation * Add basic unit-tests * Fix param * Update tests

Add StorageID to Repository entity

c1d3587

itaigilo added the exclude-changelog PR description should not be included in next release changelog label Jan 15, 2025

itaigilo added 2 commits January 15, 2025 20:11

Bug fixes

4e51981

Remove API changes

30b3849

itaigilo closed this Jan 15, 2025

itaigilo reopened this Jan 15, 2025

itaigilo added 2 commits January 15, 2025 20:38

Almost empty commit

02e4404

Revert almso empty commit

74e2202

itaigilo marked this pull request as ready for review January 15, 2025 20:39

itaigilo requested review from N-o-Z, nadavsteindler and guy-har January 15, 2025 20:40

Remove from API

222b126

N-o-Z requested changes Jan 16, 2025

View reviewed changes

itaigilo added 2 commits January 16, 2025 09:27

Fix PR comments

af9476e

Fix tests

4e8e423

itaigilo commented Jan 16, 2025

View reviewed changes

N-o-Z requested changes Jan 16, 2025

View reviewed changes

itaigilo added 2 commits January 17, 2025 15:26

Update tests

989a388

Allow only empty StorageID

298bc0d

itaigilo commented Jan 17, 2025

View reviewed changes

itaigilo added 4 commits January 17, 2025 16:41

Revert "Allow only empty StorageID"

299c292

This reverts commit 298bc0d.

Revert "Revert "Allow only empty StorageID""

b81eff3

This reverts commit 299c292.

Change validation

acb70e8

Remove StorageID in a test

3dbd9d1

N-o-Z approved these changes Jan 17, 2025

View reviewed changes

Merge branch 'master' into feature/add-storage-id-to-repo

1ced59b

Add StorageID to Repo endpoints (#8506)

6d2d2e7

* Add StorageID to Repo endpoints * Add param to Creation * Add basic unit-tests * Fix param * Update tests

itaigilo merged commit bca4801 into master Jan 18, 2025
40 checks passed

itaigilo deleted the feature/add-storage-id-to-repo branch January 18, 2025 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StorageID to Repository entity #8502

Add StorageID to Repository entity #8502

itaigilo commented Jan 15, 2025

github-actions bot commented Jan 15, 2025 •

edited

Loading

github-actions bot commented Jan 15, 2025

github-actions bot commented Jan 15, 2025

N-o-Z left a comment

N-o-Z Jan 15, 2025

N-o-Z Jan 15, 2025

N-o-Z Jan 15, 2025

N-o-Z Jan 16, 2025

itaigilo Jan 16, 2025

itaigilo left a comment

N-o-Z left a comment

N-o-Z Jan 16, 2025

N-o-Z Jan 16, 2025

N-o-Z Jan 16, 2025

itaigilo Jan 17, 2025

N-o-Z Jan 16, 2025

itaigilo Jan 17, 2025

N-o-Z Jan 17, 2025

itaigilo left a comment

itaigilo Jan 17, 2025

itaigilo Jan 17, 2025

N-o-Z left a comment

Add StorageID to Repository entity #8502

Add StorageID to Repository entity #8502

Conversation

itaigilo commented Jan 15, 2025

github-actions bot commented Jan 15, 2025 • edited Loading

github-actions bot commented Jan 15, 2025

E2E Test Results - Quickstart

github-actions bot commented Jan 15, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 15, 2025 •

edited

Loading