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.

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

Fix some concurrent memory access problems in partition balancer #18305

Merged

Conversation

ztlpn
Copy link
Contributor

@ztlpn ztlpn commented May 8, 2024

Partition balancer relies on _topics_map_revision checks to safely iterate over topic table collections with partition granularity (i.e. references to partition data and replica sets are stored and accessed across yield points). To make this safe, increment _topics_map_revision every time _topics, _updates_in_progress, _disabled_partitions or nested collections are modified in a way that invalidates references or iterators.

Likely fixes:

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

Partition balancer relies on _topics_map_revision checks to safely iterate
over topic table collections with partition granularity (i.e. references
to partition data and replica sets are stored and accessed across yield
points). To make this safe, increment _topics_map_revision every time
_topics, _updates_in_progress, _disabled_partitions or nested
collections are modified in a way that invalidates references or
iterators.
@ztlpn
Copy link
Contributor Author

ztlpn commented May 8, 2024

@dotnwat you can feel vindicated for pushing back on #18091 :)

@ztlpn
Copy link
Contributor Author

ztlpn commented May 10, 2024

This test is good, another bug uncovered.

Copy link
Contributor

@bashtanov bashtanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ztlpn added 3 commits May 10, 2024 15:56
It is handy to have a base class for all instances of concurrent
modifications.
Shard-local allocation_state object is replaced when we are applying
a controller snapshot. After this happens, all live allocated_partition
objects become invalid. Detect this and throw
concurrent_modification_error in case these objects are still used and
make destructors no-op.
@ztlpn ztlpn force-pushed the fix-balancer-memory-corruption branch from 4fa0e47 to 1c63990 Compare May 10, 2024 13:57
@ztlpn
Copy link
Contributor Author

ztlpn commented May 10, 2024

where do we bump _topics_map_revision after this emplace? https://github.com/redpanda-data/redpanda/pull/18305/files#diff-fe5b0d76af2702db674fdadb63ce097a8ea25f5b420f921374e0d1332464f549R1182

It's incremented here. Well, technically it is before, but what matters is that it is in the same continuation.

@ztlpn
Copy link
Contributor Author

ztlpn commented May 10, 2024

Test failure is #13275

@ztlpn ztlpn requested a review from bashtanov May 10, 2024 20:24
Copy link
Contributor

@bharathv bharathv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@piyushredpanda piyushredpanda merged commit 32ad2d8 into redpanda-data:dev May 10, 2024
14 of 17 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.2.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18305-v23.3.x-725 remotes/upstream/v23.3.x
git cherry-pick -x 30bbfb1f24f7e289c6a02956e9718bff4e2f6754 627db4d751bc2e583f0800be0856a3cfb2aaf67b a57a5757e278ac6b7ca0c58de0f1b23075a93402 1c63990fe32e86e85f8137cb637ab60304fce5ea

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-18305-v23.2.x-945 remotes/upstream/v23.2.x
git cherry-pick -x 30bbfb1f24f7e289c6a02956e9718bff4e2f6754 627db4d751bc2e583f0800be0856a3cfb2aaf67b a57a5757e278ac6b7ca0c58de0f1b23075a93402 1c63990fe32e86e85f8137cb637ab60304fce5ea

Workflow run logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants