Skip to content

Block requests to partially initialized partitions #1804

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
Merged
merged 7 commits into from
Jul 28, 2021
Merged

Block requests to partially initialized partitions #1804

merged 7 commits into from
Jul 28, 2021

Conversation

tobim
Copy link
Member

@tobim tobim commented Jul 27, 2021 •
edited
Loading

📔 Description

This PR fixes a segfault that occurs when a partition gets status or erase messages before it is fully initialized.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Review commit-by-commit. Test locallly.

tobim added 4 commits July 27, 2021 17:42
@tobim
Unbreak a few string literals
aa1b02b
@tobim
Set name and id in passive partition state at spawn
1229cda 
The fields name and id in the `passive_partition_state` used to
be filled in the unpack method that does a delayed initialization
after requesting the on disk data from the filesystem actor.
If any request hits the `passive_partition` before it receives
its flatbuffer chunk these values held incorret data, which lead
to wrong or misleading information in the log messages.

Since the partition id is available in the spawn function we can
simply use it to initialize those parts right away to avoid the
problem described above.
@tobim
Skip earse message for uninitialized partitions
3d59b7a
@tobim
Use a minimal status response for uninitialized partitions
4ad68d6
@tobim tobim added the bug Incorrect behavior label Jul 27, 2021
@tobim tobim requested a review from a team July 27, 2021 19:30
mavam
mavam reviewed Jul 28, 2021
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

If the segfault caused dereferencing an invalid point of self->state.partition_chunk, then I get the fix.

I don't know how to trigger/test this locally, so leaving that part to @lava.

@tobim tobim force-pushed the story/ch23374/partially-initialized-partitions branch from db0e7ef to 7834899 Compare July 28, 2021 07:34
tobim added 2 commits July 28, 2021 10:33
@tobim
Test that partially initialized partitions behave sanely
87bdeca 
This adds a check to ensure that partitions that didn't receive
the chunk with their state from the filesystem actor don't try
to execute commands that require that state yet.
@tobim
Verify the partition id outside of the unpack routine
347f0ce
lava
lava approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Code looks good; I also ran it locally and it seemed to work.

@tobim tobim enabled auto-merge July 28, 2021 08:41
@tobim tobim merged commit b524cdf into master Jul 28, 2021
@tobim tobim deleted the story/ch23374/partially-initialized-partitions branch July 28, 2021 09:10
@lava lava mentioned this pull request Aug 26, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam left review comments

@JoeLoser JoeLoser JoeLoser left review comments

@lava lava lava approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobim @lava @mavam @JoeLoser