Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace #8551

itaigilo · 2025-01-26T14:19:51Z

Closes #8550.

Notes

DefaultResolveNamespace is still not using StorageID. This might be implemented later on.
BlockstoreType() for the stats in server Setup still needs a decision.

github-actions · 2025-01-26T14:28:09Z

E2E Test Results - Quickstart

github-actions · 2025-01-26T14:28:13Z

E2E Test Results - DynamoDB Local - Local Block Adapter

nadavsteindler · 2025-01-26T15:13:19Z

cmd/lakefs/cmd/run.go

@@ -430,7 +430,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager
 		logger.Debug("lakeFS isn't initialized, skipping mismatched adapter checks")
 	} else {
 		logger.
-			WithField("adapter_type", blockStore.BlockstoreType()).
+			// TODO (gilo): uncomment this?


why is this commented out?

It's a log for repos validation, adding the adapter_type.
The BlockstoreType in now actually in the context of a repo -
So I think it's ok to remove this from the log.

nadavsteindler

I suggest a slight change to the design that will make this much cleaner

pkg/api/controller.go

pkg/block/adapter.go

nadavsteindler · 2025-01-27T14:07:35Z

pkg/block/adapter.go

-	BlockstoreMetadata(ctx context.Context) (*BlockstoreMetadata, error)
-	GetStorageNamespaceInfo() StorageNamespaceInfo
-	ResolveNamespace(storageNamespace, key string, identifierType IdentifierType) (QualifiedKey, error)
+	BlockstoreType(storageID string) string


If these methods take storageid then they need to return error in the case that they are passed an invalid storageid

Good point. Changing.

nadavsteindler

Please have a look at my open comments

N-o-Z

Thanks,

I think we can solves this in a simpler way if we switch the wrapping order MetricsAdapter <-> BlockAdapter.
See if this makes more sense

N-o-Z · 2025-01-27T17:28:40Z

cmd/lakefs/cmd/run.go

@@ -430,7 +430,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager
 		logger.Debug("lakeFS isn't initialized, skipping mismatched adapter checks")
 	} else {
 		logger.
-			WithField("adapter_type", blockStore.BlockstoreType()).
+			// TODO (gilo): uncomment this?


Question: Why don't we just pass the responsibility to check the repo to the block adapter, provide it with the repository object and let it do this validation?

This might make sense.
But -
We'll anyway need to refactor this part soon, when adding a couple more validations.
So I suggest we'll postpone it to this stage.

N-o-Z · 2025-01-27T17:31:38Z

pkg/block/metrics.go

@@ -23,12 +23,20 @@ func (m *MetricsAdapter) InnerAdapter() Adapter {
 }

 func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) {
-	ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType())
+	blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID)


Just a thought:
If BlockAdapter will wrap MetricsAdapter instead of the other way around, you'd probably won't need to provide the storageID to BlockstoreType

I'm not sure if this will improve the code,
And I'm kinda ok with the MetricsAdapter being a bit more "technical".

In this case also, we'll (probably) have to make such changes in this area when dealing with the stats and metrics of this change. So I suggest linking this comment to the relevant Issue, and dealing with this later.
Does this make sense to you?

What does it mean more "technical"?
Switching the wrapping order might release us from the need to pass the storageID to the blockstore type.
This PR handles the issue of "Add StorageID to adapter metadata functions". What is the relevant issue for this comment if not this one?

Look at the example of statsCollector

itaigilo

@N-o-Z thanks for the interesting suggestions.

Can you PTAL again?

itaigilo · 2025-01-27T18:43:41Z

cmd/lakefs/cmd/run.go

@@ -430,7 +430,8 @@ func checkRepos(ctx context.Context, logger logging.Logger, authMetadataManager
 		logger.Debug("lakeFS isn't initialized, skipping mismatched adapter checks")
 	} else {
 		logger.
-			WithField("adapter_type", blockStore.BlockstoreType()).
+			// TODO (gilo): uncomment this?


This might make sense.
But -
We'll anyway need to refactor this part soon, when adding a couple more validations.
So I suggest we'll postpone it to this stage.

itaigilo · 2025-01-27T18:48:33Z

pkg/block/metrics.go

@@ -23,12 +23,20 @@ func (m *MetricsAdapter) InnerAdapter() Adapter {
 }

 func (m *MetricsAdapter) Put(ctx context.Context, obj ObjectPointer, sizeBytes int64, reader io.Reader, opts PutOpts) (*PutResponse, error) {
-	ctx = httputil.SetClientTrace(ctx, m.adapter.BlockstoreType())
+	blockstoreType, err := m.adapter.BlockstoreType(obj.StorageID)


I'm not sure if this will improve the code,
And I'm kinda ok with the MetricsAdapter being a bit more "technical".

In this case also, we'll (probably) have to make such changes in this area when dealing with the stats and metrics of this change. So I suggest linking this comment to the relevant Issue, and dealing with this later.
Does this make sense to you?

This reverts commit 850f17b.

This reverts commit 7bf9acc.

This reverts commit 9d3f0a2.

This reverts commit a2a4ab5.

This reverts commit 06b32ee.

itaigilo · 2025-01-28T09:50:48Z

@N-o-Z removed storageID param from StorageType().
PTAL again.

N-o-Z

Thanks

# This is the 1st commit message: StorageConfigList response changes # The commit message #2 will be skipped: # Fix newConfig # The commit message #3 will be skipped: # More fixes # The commit message #4 will be skipped: # More fixes # The commit message #5 will be skipped: # Add admin/superuser error handling and cleanup (#8559) # # * Add admin/superuser error handling and cleanup # # - Added a defer function to clean up the admin user if an error occurs during creation. # - Added tests for creating an admin user, verifying successful creation, attempting to create another admin user, deleting the admin user, creating an admin user without a secret access key, and re-adding the admin user. # - Included a new test `TestAddAdminUser` to verify the behavior of the `AddAdminUser` function in various scenarios. # # * Added a logger to log the error message and warn about deleting the user # The commit message #6 will be skipped: # Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace (#8551) # # * Add StorageID to adapter metadata functions # # * Fix tests # # * Add error handling # # * Fix tests # # * Fix lint # # * Revert "Fix lint" # # This reverts commit 850f17b. # # * Revert "Fix tests" # # This reverts commit 7bf9acc. # # * Revert "Add error handling" # # This reverts commit 9d3f0a2. # # * Revert "Fix tests" # # This reverts commit a2a4ab5. # # * Revert "Add StorageID to adapter metadata functions" # # This reverts commit 06b32ee. # # * Add storageID to metadata functions # # * Fix tests # # * Fix tests # # * Remove storageID from BlockstoreMetadata

Add StorageID to adapter metadata functions

06b32ee

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

Fix tests

a2a4ab5

nadavsteindler reviewed Jan 26, 2025

View reviewed changes

itaigilo requested review from N-o-Z and guy-har January 26, 2025 15:32

nadavsteindler requested changes Jan 26, 2025

View reviewed changes

pkg/api/controller.go Outdated Show resolved Hide resolved

pkg/block/adapter.go Outdated Show resolved Hide resolved

nadavsteindler reviewed Jan 27, 2025

View reviewed changes

pkg/block/adapter.go Outdated Show resolved Hide resolved

nadavsteindler approved these changes Jan 27, 2025

View reviewed changes

nadavsteindler reviewed Jan 27, 2025

View reviewed changes

nadavsteindler approved these changes Jan 27, 2025

View reviewed changes

Add error handling

9d3f0a2

itaigilo requested a review from nadavsteindler January 27, 2025 15:37

Fix tests

7bf9acc

N-o-Z requested changes Jan 27, 2025

View reviewed changes

itaigilo commented Jan 27, 2025

View reviewed changes

itaigilo requested a review from N-o-Z January 27, 2025 19:05

itaigilo added 7 commits January 27, 2025 21:07

Fix lint

850f17b

Revert "Fix lint"

de5213e

This reverts commit 850f17b.

Revert "Fix tests"

00fe06e

This reverts commit 7bf9acc.

Revert "Add error handling"

9d4f4bc

This reverts commit 9d3f0a2.

Revert "Fix tests"

24f5136

This reverts commit a2a4ab5.

Revert "Add StorageID to adapter metadata functions"

17f2e8e

This reverts commit 06b32ee.

Add storageID to metadata functions

1d6d56f

itaigilo added 3 commits January 28, 2025 11:53

Fix tests

eee05b3

Fix tests

9be4765

Remove storageID from BlockstoreMetadata

82e261d

itaigilo changed the title ~~Add StorageID to adapter metadata functions~~ Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace Jan 29, 2025

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

View reviewed changes

itaigilo enabled auto-merge (squash) January 29, 2025 17:47

itaigilo merged commit 9badc63 into master Jan 29, 2025
40 checks passed

itaigilo deleted the task/add-storage-id-to-adapter-metadata-funcs branch January 29, 2025 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace #8551

Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace #8551

itaigilo commented Jan 26, 2025

github-actions bot commented Jan 26, 2025

github-actions bot commented Jan 26, 2025

nadavsteindler Jan 26, 2025 •

edited

Loading

itaigilo Jan 27, 2025

nadavsteindler left a comment

nadavsteindler Jan 27, 2025

itaigilo Jan 27, 2025

itaigilo Jan 27, 2025

nadavsteindler left a comment

N-o-Z left a comment

N-o-Z Jan 27, 2025

itaigilo Jan 27, 2025

N-o-Z Jan 27, 2025

itaigilo Jan 27, 2025

N-o-Z Jan 27, 2025

N-o-Z Jan 27, 2025 •

edited

Loading

itaigilo left a comment

itaigilo Jan 27, 2025

itaigilo Jan 27, 2025

itaigilo commented Jan 28, 2025

N-o-Z left a comment

Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace #8551

Add StorageID to adapter GetStorageNamespaceInfo and ResolveNamespace #8551

Conversation

itaigilo commented Jan 26, 2025

Notes

github-actions bot commented Jan 26, 2025

E2E Test Results - Quickstart

github-actions bot commented Jan 26, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

nadavsteindler Jan 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadavsteindler 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

nadavsteindler 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

N-o-Z Jan 27, 2025 • edited Loading

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

itaigilo commented Jan 28, 2025

N-o-Z left a comment

Choose a reason for hiding this comment

nadavsteindler Jan 26, 2025 •

edited

Loading

N-o-Z Jan 27, 2025 •

edited

Loading