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

hinted handoff: Prevent segmentation fault when initializing endpoint managers #18650

Merged
merged 3 commits into from May 14, 2024

Conversation

dawmd
Copy link
Contributor

@dawmd dawmd commented May 13, 2024

We don't attempt to create an endpoint manager for a hint directory if there is no mapping host ID–IP corresponding to the directory's name, an IP address. That prevents a segmentation fault.

Fixes #18649

@dawmd dawmd requested a review from piodul as a code owner May 13, 2024 13:07
@dawmd dawmd added type/bug area/hinted handoff Issues related to Hinted Handoff backport/none Backport is not required labels May 13, 2024
@@ -873,11 +873,11 @@ future<> manager::perform_migration() {
const auto lock = co_await seastar::get_units(_drain_lock, 1);

using lock_type = std::unique_lock<seastar::shared_mutex>;
// We're taking this lock because we're about to stop endpoint managers here, wheras
// We're taking this lock because we're about to stop endpoint managers here, whereas
Copy link
Contributor

Choose a reason for hiding this comment

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

Spelling fixes are no longer needed, they were fixed by #18567 which is already on master

dawmd added 3 commits May 13, 2024 16:40
If hinted handoff is still IP-based and there is
a hint directory representing an IP without
a corresponding mapping to a host ID in
`locator::token_metadata`, an attemp to initialize
its endpoint manager will result in a segmentation
fault. This commit prevents that.
Before these changes, if initializing endpoint
managers after the migration of hinted handoff
to host ID is done throws an exception, we
don't remove the flag indicating the migration
is still in progress. However, the migration
has, in practice, finished -- all of the
hint directories have been mapped to host IDs
and all of the nodes in the cluster are
host-ID-based. Because of that, it makes sense
to remove the flag early on.
@dawmd
Copy link
Contributor Author

dawmd commented May 13, 2024

Version 2:

  • dropped the commits fixing spelling

@dawmd dawmd requested a review from piodul May 13, 2024 15:54
Copy link
Contributor

@piodul piodul left a comment

Choose a reason for hiding this comment

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

LGTM, will wait for CI before merging

@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
✅ - Unit Tests

Build Details:

  • Duration: 6 hr 17 min
  • Builder: i-0837c30dcff31fc76 (r5ad.8xlarge)

@piodul
Copy link
Contributor

piodul commented May 14, 2024

Queued

@scylladb-promoter scylladb-promoter merged commit 448f651 into scylladb:master May 14, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hinted handoff Issues related to Hinted Handoff backport/none Backport is not required promoted-to-master type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hinted handoff: Segmentation fault before migration to host ID
3 participants