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

[v23.3.x] Fix some concurrent memory access problems in partition balancer #18450

Merged
merged 3 commits into from
May 16, 2024

Conversation

ztlpn
Copy link
Contributor

@ztlpn ztlpn commented May 13, 2024

Backport of PR #18305

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.

(cherry picked from commit 30bbfb1)
@ztlpn ztlpn added this to the v23.3.x-next milestone May 13, 2024
@ztlpn ztlpn added the kind/backport PRs targeting a stable branch label May 13, 2024
@ztlpn ztlpn marked this pull request as ready for review May 13, 2024 20:00
@piyushredpanda
Copy link
Contributor

@ztlpn build is failing, it appears?

ztlpn added 2 commits May 14, 2024 12:39
It is handy to have a base class for all instances of concurrent
modifications.

(cherry picked from commit 627db4d)

Conflicts:
	src/v/cluster/topic_table.h
	src/v/utils/stable_iterator_adaptor.h
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.

(cherry picked from commit a57a575)

Conflicts:
	src/v/cluster/scheduling/types.cc
@ztlpn
Copy link
Contributor Author

ztlpn commented May 14, 2024

build is failing, it appears?

Fixed

@ztlpn
Copy link
Contributor Author

ztlpn commented May 14, 2024

Huh, the test has uncovered another problem - that we update topic_table and partition_allocator separately and the planner can see inconsistent state. This problem is present in 24.1 as well I think, it just doesn't lead to a crash.

@ztlpn ztlpn marked this pull request as draft May 14, 2024 20:10
@ztlpn ztlpn marked this pull request as draft May 14, 2024 20:10
@ztlpn ztlpn marked this pull request as ready for review May 15, 2024 15:51
@ztlpn
Copy link
Contributor Author

ztlpn commented May 15, 2024

I removed commit 1c63990 from the backport so as not to block backporting the actual fixes. I will backport it later with another fix.

@piyushredpanda piyushredpanda merged commit cf8e798 into redpanda-data:v23.3.x May 16, 2024
16 checks passed
@piyushredpanda piyushredpanda modified the milestones: v23.3.x-next, v23.3.16 May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redpanda kind/backport PRs targeting a stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v23.3.x] Fix some concurrent memory access problems in partition balancer
2 participants