Make update object metadata API atomic #8264

arielshaqed · 2024-10-06T13:58:04Z

Protect updating object metadata on an existing entry to be safe against concurrent modifications, deletions, and commits and merges to the branch. This is a bit tricky because it needs to work for both committed and staged objects.

Fixes #8262 - a race that would give an odd result if update object metadata managed to lose against concurrent delete and uncommitted GC, or potentially merges into the branch.

github-actions · 2024-10-06T14:05:27Z

E2E Test Results - Quickstart

github-actions · 2024-10-06T14:06:04Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-10-06T14:10:48Z

🎊 PR Preview bb6c444 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8264.surge.sh

🕐 Build time: 0.01s

_{🤖 By surge-preview}

arielshaqed · 2024-10-06T14:47:03Z

Reviewers: I know that tests fail. Please review anyway, we will likely need >1 round -- and these will be easy enough to fix, and not in the areas modified in this PR.

N-o-Z

LGTM!

By mistake

arielshaqed · 2024-10-08T12:21:58Z

LookED good to Nir 😭

Protect updating object metadata on an existing entry to be safe against concurrent modifications, deletions, and commits and merges to the branch. This is a bit tricky because it needs to work for both committed and staged objects. Fixes #8262 - a race that would give an odd result if update object metadata managed to lose against concurrent delete and uncommitted GC, or potentially merges into the branch.

Allow tests to pass. (Also rebase on trunk latest)

N-o-Z · 2024-10-08T13:12:58Z

LookED good to Nir 😭

I'll review it today 😃

N-o-Z

Thanks - looks neat.
I did not review the tests yet as I had some questions regarding the logic itself

N-o-Z · 2024-10-08T19:24:08Z

pkg/graveler/graveler.go

+// getFromBranchWithStaging returns the value for key on repository and
+// branch.  It returns true if the object was found in the staging area,
+// otherwise false.  It does not micro-batch.
+func (g *Graveler) getFromBranchWithStaging(ctx context.Context, repository *RepositoryRecord, branch *Branch, key Key) (*Value, bool, error) {


How is this function different from g.Get (which is used today by Set and resetKey) and can we reuse this logic there if needed

You're right! Actually below I want to do something else on a compacted metarange. So I removed this function in favour of getFromStaging below, and added a test.

N-o-Z · 2024-10-08T21:32:56Z

pkg/graveler/graveler.go

+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
+			if currentValue == nil {
+				if onStaging {
+					// Object previously on staging, and


nit:
I know this is your line length configuration but this way of breaking the comment makes it very confusing to understand. Consider leaving it in a single line

Changed. Now I find it harder to read, but I don't get to review my own code :-/

N-o-Z · 2024-10-08T21:35:33Z

pkg/graveler/graveler.go

+
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
+			if currentValue == nil {
+				if onStaging {


Do we really need this piece of code?
Considering this will just fail during the update due to the change on the branch?
This can also result in inconsistent behavior:
If we catch the deletion before this part we will return ErrNotFound but if the delete occurs after this check we will get PreconditionFalied.
This might also cause confusion when debugging

Excellent question!

Considering this will just fail during the update due to the change on the branch?

Yes but: returning ErrNotFound here will immediately fail safeBranchWrite and return the same error. This is correct behaviour.

I am not sure how this ever returns PreconditionFailed TBH.

Well due to the code changes it is not relevant but, consider the two timings:

Object is deleted from staging before this line - we will return ErrNotFound

Object is initially read (with pred) at the beginning of g.StagingManager.Update then immediately deleted and we use the setMsgIf using the pred - and we get ErrPredicateFailed
In both scenarios the object was deleted in staging during UpdateMetadata

Thanks!

I think that you were right and still are, and that this still happens. But only during a race. It is legit to fail an update if it occurs concurrently with a deletion. Returning ErrNotFound is the best way, which serializes DELETE before UPDATE. But returning ErrPredicateFailed is equally valid - it indicates that Graveler was unable to serialize the operations.

I doubt this will matter in practice. If it does, we will need to add a retry.

N-o-Z · 2024-10-08T21:39:54Z

pkg/graveler/graveler.go

+				}
+				// Use committed value (in practice it came
+				// from committed, so it is non-nil).
+				currentValue = value


If I understand correctly, the only reason we get the value before the update is because we perform retries. In that case I think it's important to document that so it will be clear what we're doing here.

You're right, so I decided to remove this prefetch entirely in favour of a single fetch-from-committed. That means I don't need to protect pick "the right" value, I can just use the latest. Changed to do that, and I think the new code is much easier to read!

I think the version you did not like would not work with a compacted base metarange. Currently we never set a compacted base metarange, so this was not tested before and is still not tested.

I like that the new code has fewer comments, I think it doesn't need them (diff stats are +15 -42). Please let me know if you think that more comments are needed :-/

arielshaqed

THANKS for a thorough review! PTAL, I think I changed the bits you did not like. It is definitely shorter now; this implies fewer bugs if we assume equal bug densities.

arielshaqed · 2024-10-09T08:25:16Z

pkg/graveler/graveler.go

+// getFromBranchWithStaging returns the value for key on repository and
+// branch.  It returns true if the object was found in the staging area,
+// otherwise false.  It does not micro-batch.
+func (g *Graveler) getFromBranchWithStaging(ctx context.Context, repository *RepositoryRecord, branch *Branch, key Key) (*Value, bool, error) {


You're right! Actually below I want to do something else on a compacted metarange. So I removed this function in favour of getFromStaging below, and added a test.

arielshaqed · 2024-10-09T10:54:55Z

pkg/graveler/graveler.go

+
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
+			if currentValue == nil {
+				if onStaging {


Excellent question!

Considering this will just fail during the update due to the change on the branch?

Yes but: returning ErrNotFound here will immediately fail safeBranchWrite and return the same error. This is correct behaviour.

I am not sure how this ever returns PreconditionFailed TBH.

arielshaqed · 2024-10-09T11:01:37Z

pkg/graveler/graveler.go

+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
+			if currentValue == nil {
+				if onStaging {
+					// Object previously on staging, and


Changed. Now I find it harder to read, but I don't get to review my own code :-/

arielshaqed · 2024-10-09T11:05:33Z

pkg/graveler/graveler.go

+				}
+				// Use committed value (in practice it came
+				// from committed, so it is non-nil).
+				currentValue = value


You're right, so I decided to remove this prefetch entirely in favour of a single fetch-from-committed. That means I don't need to protect pick "the right" value, I can just use the latest. Changed to do that, and I think the new code is much easier to read!

I think the version you did not like would not work with a compacted base metarange. Currently we never set a compacted base metarange, so this was not tested before and is still not tested.

I like that the new code has fewer comments, I think it doesn't need them (diff stats are +15 -42). Please let me know if you think that more comments are needed :-/

itaiad200

Overall looks good - thank you! Requesting changes due to the entry update failure on predicate failure

pkg/catalog/catalog.go

pkg/graveler/graveler.go

itaiad200 · 2024-10-09T14:45:11Z

pkg/graveler/graveler.go

+	// UpdateObjectMetadata atomically runs update on repository /
+	// branch by key.  (Of course, if entry is only on committed, the
+	// updated entry will still be created (atomically) on staging.)
+	UpdateObjectMetadata(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error


I don't know if you meant to write it this way, but you can update more than just the metadata. If it's intended, let's rename the func. If not, let's rewrite the logic.

Suggested change

UpdateObjectMetadata(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error

UpdateObject(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error

IMO it can update only object metadata - not object data. The name UpdateObject makes it seem like it can do more. OTOH, the parameters make it clear exactly what it can do... so done.

itaiad200 · 2024-10-09T14:55:15Z

pkg/api/controller_test.go

-			Checksum:        "this_is_a_checksum",
-		}
-		testutil.Must(t, deps.catalog.CreateEntry(ctx, repo, "main", entry))
+	bools := []bool{false, true}


Nice tests!

:-)

As you can imagine, I added the loops only after writing the third of the four tests 🤡 .

itaiad200 · 2024-10-09T14:58:02Z

pkg/graveler/graveler.go

+	var committedValue *Value
+
+	err = g.safeBranchWrite(ctx, log, repository, branchID, safeBranchWriteOptions{MaxTries: options.MaxTries}, func(branch *Branch) error {
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {


I believe you shouldn't retry if the entry was changed, i.e. the StagingManager SetMsgIf failed due to the predicate

If staging operation fails (i.e. predicate failed) there will not be any retry

Indeed. The retry is in case the branch head changes (same as in Set). In that case, we must retry: the update might have succeeded on staging after the commit scanned the key. If we return and succeed, that would be a lost update. So we try again; this might cause staging to have an equal value to committed or not - and both cases are good.

N-o-Z · 2024-10-09T20:41:19Z

pkg/catalog/catalog.go

+	}
+
+	key := graveler.Key(path)
+	updater := graveler.ValueUpdateFunc(func(value *graveler.Value) (*graveler.Value, error) {


I think I have forgotten to submit this comment in the previous review.
It also relates to @itaiad200 comment:
If the intention of the function is to update the metadata only - let's pass the metadata instead of an update function. make it explicit, inline and easier to read.
Otherwise lets modify the graveler function to be UpdateObject and provide the update function accordingly (which is infact exactly the same as calling g.StagingManager.Update).

Unfortunately graveler cannot run ValueToEntry - graveler is not supposed to know what type of values it handles.

Graveler.UpdateObject, as it is now called, indeed uses g.StagingManager.Update. But it cannot use just that! It needs to be able to update (in staging after the operation) a committed object (not in staging before the operation).

N-o-Z · 2024-10-09T20:44:50Z

pkg/graveler/graveler.go

+	var committedValue *Value
+
+	err = g.safeBranchWrite(ctx, log, repository, branchID, safeBranchWriteOptions{MaxTries: options.MaxTries}, func(branch *Branch) error {
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {


If staging operation fails (i.e. predicate failed) there will not be any retry

N-o-Z · 2024-10-09T20:47:15Z

pkg/graveler/graveler.go

+			if currentValue == nil {
+				// Object not on staging: need to update committed value.
+				if committedValue == nil {


Suggested change

if currentValue == nil {

// Object not on staging: need to update committed value.

if committedValue == nil {

if currentValue == nil && committedValue == nil {

// Object not on staging: need to update committed value.

Not done, this changes behaviour. The current code reads the committed value once, if it needs to, and then always uses that value for an update if the value is not on staging.

For correctness, it is enough to use a committed value (from after the start of the operation). For efficiency, we want to read that committed value as few times as possible. This will be 0 times if the object exists on staging, so we read it the first time we need it.

N-o-Z · 2024-10-09T20:52:41Z

pkg/graveler/graveler.go

+
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {
+			if currentValue == nil {
+				if onStaging {


Well due to the code changes it is not relevant but, consider the two timings:

Object is deleted from staging before this line - we will return ErrNotFound

Object is initially read (with pred) at the beginning of g.StagingManager.Update then immediately deleted and we use the setMsgIf using the pred - and we get ErrPredicateFailed
In both scenarios the object was deleted in staging during UpdateMetadata

N-o-Z

Code is much clearer now IMO!
Added one more major comment which I seem to have left out of the previous review

arielshaqed

Thanks!

I ended up accepting comments about naming and error messages, but not changing the interface, using StagingManager.Update directly, or changing the interface of Graveler.Update. I hope I explained why in my comments.

PTAL...

arielshaqed · 2024-10-10T06:39:47Z

pkg/graveler/graveler.go

+	// UpdateObjectMetadata atomically runs update on repository /
+	// branch by key.  (Of course, if entry is only on committed, the
+	// updated entry will still be created (atomically) on staging.)
+	UpdateObjectMetadata(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error


IMO it can update only object metadata - not object data. The name UpdateObject makes it seem like it can do more. OTOH, the parameters make it clear exactly what it can do... so done.

arielshaqed · 2024-10-10T06:44:41Z

pkg/graveler/graveler.go

+	var committedValue *Value
+
+	err = g.safeBranchWrite(ctx, log, repository, branchID, safeBranchWriteOptions{MaxTries: options.MaxTries}, func(branch *Branch) error {
+		return g.StagingManager.Update(ctx, branch.StagingToken, key, func(currentValue *Value) (*Value, error) {


Indeed. The retry is in case the branch head changes (same as in Set). In that case, we must retry: the update might have succeeded on staging after the commit scanned the key. If we return and succeed, that would be a lost update. So we try again; this might cause staging to have an equal value to committed or not - and both cases are good.

arielshaqed · 2024-10-10T06:50:15Z

pkg/graveler/graveler.go

+			if currentValue == nil {
+				// Object not on staging: need to update committed value.
+				if committedValue == nil {


Not done, this changes behaviour. The current code reads the committed value once, if it needs to, and then always uses that value for an update if the value is not on staging.

For correctness, it is enough to use a committed value (from after the start of the operation). For efficiency, we want to read that committed value as few times as possible. This will be 0 times if the object exists on staging, so we read it the first time we need it.

pkg/graveler/graveler.go

arielshaqed · 2024-10-10T06:51:27Z

pkg/api/controller_test.go

-			Checksum:        "this_is_a_checksum",
-		}
-		testutil.Must(t, deps.catalog.CreateEntry(ctx, repo, "main", entry))
+	bools := []bool{false, true}


:-)

As you can imagine, I added the loops only after writing the third of the four tests 🤡 .

pkg/catalog/catalog.go

arielshaqed · 2024-10-10T06:57:32Z

pkg/catalog/catalog.go

+	}
+
+	key := graveler.Key(path)
+	updater := graveler.ValueUpdateFunc(func(value *graveler.Value) (*graveler.Value, error) {


Unfortunately graveler cannot run ValueToEntry - graveler is not supposed to know what type of values it handles.

Graveler.UpdateObject, as it is now called, indeed uses g.StagingManager.Update. But it cannot use just that! It needs to be able to update (in staging after the operation) a committed object (not in staging before the operation).

pkg/catalog/catalog.go

itaiad200

LGTM, thanks for the quick fixes

arielshaqed · 2024-10-10T07:53:37Z

ObMeme:

N-o-Z

Thanks!!!!
One typo comment

N-o-Z · 2024-10-11T21:19:34Z

pkg/graveler/graveler.go

+	log := g.log(ctx).WithFields(logging.Fields{"key": key, "operation": "update_user_metadata"})
+
+	// committedValue, if non-nil is a value read from either uncommitted or committed.  Usually
+	// it it is read from committed.  If there is a value on staging, that entry will be


Suggested change

// it it is read from committed. If there is a value on staging, that entry will be

// it is read from committed. If there is a value on staging, that entry will be

N-o-Z · 2024-10-11T21:19:51Z

pkg/graveler/graveler.go

@@ -1841,6 +1844,47 @@ func (g *Graveler) safeBranchWrite(ctx context.Context, log logging.Logger, repo
 	return nil
 }

+func (g *Graveler) Update(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error {


🙏🏽 👍🏽

arielshaqed

Thanks! Pulling...

arielshaqed · 2024-10-13T06:26:49Z

pkg/graveler/graveler.go

+	log := g.log(ctx).WithFields(logging.Fields{"key": key, "operation": "update_user_metadata"})
+
+	// committedValue, if non-nil is a value read from either uncommitted or committed.  Usually
+	// it it is read from committed.  If there is a value on staging, that entry will be


arielshaqed requested review from N-o-Z and yonipeleg33 October 6, 2024 13:58

arielshaqed added area/cataloger Improvements or additions to the cataloger area/API Improvements or additions to the API include-changelog PR description should be included in next release changelog labels Oct 6, 2024

arielshaqed mentioned this pull request Oct 7, 2024

Update cli.md #8266

Merged

N-o-Z previously approved these changes Oct 7, 2024

View reviewed changes

arielshaqed added 3 commits October 8, 2024 15:22

make gen

a15b37f

Add missing method to FakeGraveler

16e5da2

Allow tests to pass. (Also rebase on trunk latest)

arielshaqed force-pushed the bug/8262-update-metadata-delete-gc-race branch from bb6c444 to 16e5da2 Compare October 8, 2024 12:30

N-o-Z requested changes Oct 8, 2024

View reviewed changes

[CR] Simplify "use committed value" logic in code

c546a0d

arielshaqed commented Oct 9, 2024

View reviewed changes

arielshaqed requested a review from N-o-Z October 9, 2024 12:01

itaiad200 requested changes Oct 9, 2024

View reviewed changes

N-o-Z reviewed Oct 9, 2024

View reviewed changes

Rename UpdateObjectMetadata -> Update; fix some errors

e15e7f8

arielshaqed commented Oct 10, 2024

View reviewed changes

itaiad200 approved these changes Oct 10, 2024

View reviewed changes

arielshaqed requested a review from N-o-Z October 10, 2024 07:53

N-o-Z approved these changes Oct 11, 2024

View reviewed changes

arielshaqed commented Oct 13, 2024

View reviewed changes

[CR] s/it it/it/ in comment

0d25da1

arielshaqed enabled auto-merge (squash) October 13, 2024 06:37

arielshaqed merged commit aaf2190 into master Oct 13, 2024
37 checks passed

arielshaqed deleted the bug/8262-update-metadata-delete-gc-race branch October 13, 2024 06:45

	UpdateObjectMetadata(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error
	UpdateObject(ctx context.Context, repository *RepositoryRecord, branchID BranchID, key Key, update ValueUpdateFunc, opts ...SetOptionsFunc) error

	// it it is read from committed. If there is a value on staging, that entry will be
	// it is read from committed. If there is a value on staging, that entry will be

Make update object metadata API atomic #8264

Make update object metadata API atomic #8264

Conversation

arielshaqed commented Oct 6, 2024

github-actions bot commented Oct 6, 2024

E2E Test Results - Quickstart

github-actions bot commented Oct 6, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Oct 6, 2024 • edited Loading

arielshaqed commented Oct 6, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

arielshaqed commented Oct 8, 2024

LookED good to Nir 😭

N-o-Z commented Oct 8, 2024

LookED good to Nir 😭

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed 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

itaiad200 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

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 left a comment

Choose a reason for hiding this comment

arielshaqed 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

itaiad200 left a comment

Choose a reason for hiding this comment

arielshaqed commented Oct 10, 2024

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

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2024 •

edited

Loading