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

HDDS-10231. ContainerStateManager should not finalize the OPEN containers without a Pipeline. #6123

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

nandakumar131
Copy link
Contributor

What changes were proposed in this pull request?

If there is an OPEN container without any pipeline, that container has to be moved to CLOSING state and CLOSE container command has to be sent to the datanodes.
Currently, this is done while SCM is initialized. Since Ratis is not set up completely (mostly no leader is elected at this point), the state update is done locally (not via Ratis).
Doing this on SCM start-up is error-prone.

The container state update has to be done only via Ratis. If not, there are chances of container state getting diverged between SCMs.

This PR changes the logic and moves it to ReplicationManager's OpenContainerHandler, where we check if the OPEN Container has a healthy Pipeline, if not the container is closed.

What is the link to the Apache JIRA

HDDS-10231

How was this patch tested?

Added new unit test to cover the scenario.

@sodonnel
Copy link
Contributor

Looks mostly good. I just have a suggestion to add another check in a couple of the tests.

@nandakumar131
Copy link
Contributor Author

Thanks for the review @sodonnel.
Added additional assert to check the metric.

Copy link
Contributor

@siddhantsangwan siddhantsangwan left a comment

Choose a reason for hiding this comment

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

LGTM. @nandakumar131 Thanks for the PR.

Copy link
Contributor

@sodonnel sodonnel left a comment

Choose a reason for hiding this comment

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

LGTM

@adoroszlai adoroszlai merged commit 04d6e0b into apache:master Jan 30, 2024
35 checks passed
@adoroszlai
Copy link
Contributor

Thanks @nandakumar131 for the patch, @siddhantsangwan, @sodonnel for the review.

@nandakumar131 nandakumar131 deleted the HDDS-10231 branch January 30, 2024 16:25
@sumitagrawl
Copy link
Contributor

@nandakumar131 Warn logs needs to be updated as telling it "marking container for closing", but this logic is moved now to Replication manager

@nandakumar131
Copy link
Contributor Author

@sumitagrawl, thanks for the review. I will update the log in one of my following PRs.

myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 2024
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 2024
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 2024
swamirishi pushed a commit to swamirishi/ozone that referenced this pull request Jun 10, 2024
…N containers without a Pipeline. (apache#6123)

(cherry picked from commit 04d6e0b)
Change-Id: If6fa9aa4447c7664e0a4f9794244af4711fc2ca6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants