Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Alternator: concurrent TagResource from multiple nodes may race #6389

Closed
nyh opened this issue May 7, 2020 · 7 comments · Fixed by #13150
Closed

Alternator: concurrent TagResource from multiple nodes may race #6389

nyh opened this issue May 7, 2020 · 7 comments · Fixed by #13150
Assignees
@nyh
Labels
area/alternator Alternator related Issues P2 High Priority
Milestone
5.3

Comments

@nyh
Copy link
Contributor

nyh commented May 7, 2020

Tags are stored in a schema table (system_schema.tables.extensions['tags']).
This means means that concurrent adding of tags for a single table on more than a single node may result in a race, until Scylla schema agreement is reimplemented to avoid them, or we use some other mechanism to serialize the operation.
@nyh nyh added low area/alternator Alternator related Issues labels May 7, 2020
psarna pushed a commit that referenced this issue May 7, 2020
@nyh @psarna
doc, alternator: shorten description of "tags" compatibility
7da9490 
The "current compatibility with DynamoDB" section in alternator.md is where
we should list very briefly our state of compatibility - it's not the right
place to explain implementation details or track obscure bugs. I've
significantly shortened the "Tags" section because, in brief, we do
fully support tags and should say that we do.

I moved the two bugs mentioned in the text into the bug tracker:
Refs #6389
Refs #6391

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Message-Id: <20200507125022.22608-1-nyh@scylladb.com>
@slivne slivne added this to the 4.x milestone May 17, 2020
@nyh
Copy link
Contributor Author

nyh commented Oct 18, 2022

This problem will be solved automatically when we switch to using Raft for schema operations.

@nyh
Copy link
Contributor Author

nyh commented Nov 29, 2022 •
edited

At this point, most Alternator table operations like CreateTable already use Raft for serializing the schema operations: They take mm.start_group0_operation(), do whatever reading and writing to the schema they want, until finally calling mm.announce() to complete the operation. So CreateTable with tags should already be safe.

But this issue still exists because in our TagResource implementation (executor::tag_resource()) doesn't have this protection: It reads the tags from the current schema, modifies them, and writes them back - without any locking.

The last "writes them back" step is done in a function update_tags() (which in commit 041cb77 @havaker moved to db/tags/utils.cc). The problem is that this function does both start_group0_operation() and announce(). This is wrong: The start_group0_operation should be called before preparing the new tags based on the old tags to keep the read-modify-write operation atomic. Perhaps update_tags() needs to be modified to take not the new tags_map but a function which generates these new tags - and this generation function will be called under the lock.

@DoronArazii DoronArazii added P4 Low Priority and removed low labels Dec 28, 2022
@nyh nyh self-assigned this Mar 12, 2023
nyh added a commit to nyh/scylla that referenced this issue Mar 13, 2023
@nyh
test/alternator: test concurrent TagResource / UntagResource
c41b2d3 
This patch adds an Alternator test reproducing issue scylladb#6389 - that
concurrent TagResource and/or UntagResource operations was broken and
some of the concurrent modifications were lost.

The test has two threads, one loops adds and removes a tag A, the
other adds and removes a tag B. After we add tag A, we expect tag A
to be there - but due to issue scylladb#6389 this modification was sometimes
lost when it raced with an operation on B.

This test consistently failed before issue scylladb#6389 was fixed, and passes
now after the issue was fixed by the previous patches. The bug reproduces
by chance, so it requires a fairly long loop (a few seconds) to be sure
it reproduces - so is marked a "veryslow" test and will not run in CI,
but can be used to manually reproduce this issue with:

    test/alternator/run --runveryslow test_tag.py::test_concurrent_tag

Refs scylladb#6389.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@nyh nyh mentioned this issue Mar 13, 2023
Merged
@mykaul mykaul modified the milestones: 5.x, 5.3 Mar 13, 2023
@mykaul mykaul added P2 High Priority Backport candidate and removed P4 Low Priority labels Mar 13, 2023
denesb added a commit that referenced this issue Apr 11, 2023
@denesb
Merge 'alternator: fix isolation of concurrent modifications to tags'…
dba1d36 
… from Nadav Har'El

Alternator's implementation of TagResource, UntagResource and UpdateTimeToLive (the latter uses tags to store the TTL configuration) was unsafe for concurrent modifications - some of these modifications may be lost. This short series fixes the bug, and also adds (in the last patch) a test that reproduces the bug and verifies that it's fixed.

The cause of the incorrect isolation was that we separately read the old tags and wrote the modified tags. In this series we introduce a new function, `modify_tags()` which can do both under one lock, so concurrent tag operations are serialized and therefore isolated as expected.

Fixes #6389.

Closes #13150

* github.com:scylladb/scylladb:
  test/alternator: test concurrent TagResource / UntagResource
  db/tags: drop unsafe update_tags() utility function
  alternator: isolate concurrent modification to tags
  db/tags: add safe modify_tags() utility functions
  migration_manager: expose access to storage_proxy
denesb pushed a commit that referenced this issue Dec 14, 2023
@nyh @denesb
alternator: isolate concurrent modification to tags
5b6f758 
Alternator modifies tags in three operations - TagResource, UntagResource
and UpdateTimeToLive (the latter uses a tag to store the TTL configuration).

All three operations were implemented by three separate steps:

 1. Read the current tags.
 2. Modify the tags according to the desired operation.
 3. Write the modified tags back with update_tags().

This implementation was not safe for concurrent operations - some
modifications may be be lost. We fix this in this patch by using the new
modify_tags() function introduced in the previous patch, which performs
all three steps under one lock so the tag operations are serialized and
correctly isolated.

Fixes #6389

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
(cherry picked from commit c196bd7)
@denesb
Copy link
Contributor

denesb commented Dec 14, 2023

Backported to 5.2, 5.4 already has it.

@denesb
Copy link
Contributor

denesb commented Dec 15, 2023

Backport to 5.2. dequeued, breaks the build. @nyh please open a backport PR.

@nyh
Copy link
Contributor Author

nyh commented Dec 18, 2023

Doing the backport to 5.2 now.

nyh pushed a commit to nyh/scylla that referenced this issue Dec 18, 2023
@denesb @nyh
Merge 'alternator: fix isolation of concurrent modifications to tags'…
0242e47 
… from Nadav Har'El

Alternator's implementation of TagResource, UntagResource and UpdateTimeToLive (the latter uses tags to store the TTL configuration) was unsafe for concurrent modifications - some of these modifications may be lost. This short series fixes the bug, and also adds (in the last patch) a test that reproduces the bug and verifies that it's fixed.

The cause of the incorrect isolation was that we separately read the old tags and wrote the modified tags. In this series we introduce a new function, `modify_tags()` which can do both under one lock, so concurrent tag operations are serialized and therefore isolated as expected.

Fixes scylladb#6389.

Closes scylladb#13150

* github.com:scylladb/scylladb:
  test/alternator: test concurrent TagResource / UntagResource
  db/tags: drop unsafe update_tags() utility function
  alternator: isolate concurrent modification to tags
  db/tags: add safe modify_tags() utility functions
  migration_manager: expose access to storage_proxy

(cherry picked from commit dba1d36)
@nyh nyh mentioned this issue Dec 18, 2023
Closed
@nyh
Copy link
Contributor Author

nyh commented Dec 18, 2023

Created backport PR for 5.2 in #16453

denesb added a commit that referenced this issue Dec 19, 2023
@denesb
Merge 'alternator: fix isolation of concurrent modifications to tags'…
46a29e9 
… from Nadav Har'El

Alternator's implementation of TagResource, UntagResource and UpdateTimeToLive (the latter uses tags to store the TTL configuration) was unsafe for concurrent modifications - some of these modifications may be lost. This short series fixes the bug, and also adds (in the last patch) a test that reproduces the bug and verifies that it's fixed.

The cause of the incorrect isolation was that we separately read the old tags and wrote the modified tags. In this series we introduce a new function, `modify_tags()` which can do both under one lock, so concurrent tag operations are serialized and therefore isolated as expected.

Fixes #6389.

Closes #13150

* github.com:scylladb/scylladb:
  test/alternator: test concurrent TagResource / UntagResource
  db/tags: drop unsafe update_tags() utility function
  alternator: isolate concurrent modification to tags
  db/tags: add safe modify_tags() utility functions
  migration_manager: expose access to storage_proxy

(cherry picked from commit dba1d36)

Closes #16453
@denesb
Copy link
Contributor

denesb commented Dec 19, 2023

Created backport PR for 5.2 in #16453

This was queued, so I'm removing the backport labels.

MeepoYang pushed a commit to storage-scsg/scylladb that referenced this issue Apr 28, 2024
@nyh
test/alternator: test concurrent TagResource / UntagResource
24586aa 
This patch adds an Alternator test reproducing issue scylladb#6389 - that
concurrent TagResource and/or UntagResource operations was broken and
some of the concurrent modifications were lost.

The test has two threads, one loops adds and removes a tag A, the
other adds and removes a tag B. After we add tag A, we expect tag A
to be there - but due to issue scylladb#6389 this modification was sometimes
lost when it raced with an operation on B.

This test consistently failed before issue scylladb#6389 was fixed, and passes
now after the issue was fixed by the previous patches. The bug reproduces
by chance, so it requires a fairly long loop (a few seconds) to be sure
it reproduces - so is marked a "veryslow" test and will not run in CI,
but can be used to manually reproduce this issue with:

    test/alternator/run --runveryslow test_tag.py::test_concurrent_tag

Refs scylladb#6389.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nyh nyh

Labels
area/alternator Alternator related Issues P2 High Priority
Projects
None yet
Milestone
5.3
Development

Successfully merging a pull request may close this issue.

alternator: fix isolation of concurrent modifications to tags nyh/scylla
5 participants
@nyh @denesb @slivne @mykaul @DoronArazii