HDDS-9959. Propagate group remove to other datanodes during pipeline close #5827

ivandika3 · 2023-12-19T07:31:46Z

What changes were proposed in this pull request?

In https://issues.apache.org/jira/browse/RATIS-1947, it was found that there might be cases where RaftServer for Datanodes in the same pipeline are closed hours apart.

# dn1
2023-11-29 15:22:59,477 [Command processor thread] INFO org.apache.hadoop.ozone.container.common.statemachine.commandhandler.ClosePipelineCommandHandler: Close Pipeline PipelineID=23e46782-6b48-4559-b3ac-0f95993cf0bc command on datanode 1669a7e6-fe3c-4f7e-8fcb-ec5d5027b0eb.


#dn5
2023-11-29 14:07:55,442 [Command processor thread] INFO org.apache.hadoop.ozone.container.common.statemachine.commandhandler.ClosePipelineCommandHandler: Close Pipeline PipelineID=23e46782-6b48-4559-b3ac-0f95993cf0bc command on datanode bd1e72ab-cfd5-4cc1-8fbf-6ec9d9654c98.


# dn8
2023-11-29 16:57:53,894 [Command processor thread] INFO org.apache.hadoop.ozone.container.common.statemachine.commandhandler.ClosePipelineCommandHandler: Close Pipeline PipelineID=23e46782-6b48-4559-b3ac-0f95993cf0bc command on datanode 4a23d1e8-d526-4a4d-8ed1-13ffbab3a5cc.
This might happen when there are a lot of commands in some of the Datanode commandQueue, causing some pipeline close commands to be handled by datanodes later than the others.

Furthermore, Ratis group remove operation is local to the Raft server and is not propagated to the other Raft peers in the same group. Therefore, datanodes that have not received the group remove operation will keep operating (e.g. sending RequestVote / AppendEntries RPCs), although the pipeline (Raft group) is supposed to be closed. The might affect the client communicating with the Raft peer in datanodes that have not been closed yet.

Therefore, similar to CreatePipelineCommandHandler, the first datanode that receives the close pipeline command needs to propagate the group remove command to the other datanodes (Raft peers) in the same pipeline. This will close the pipeline immediately on all the datanodes. The subsequent close pipeline commands will be ignored silently as the pipeline has been successfully closed.

Note: There seems to be an unused methods in RatisPipelineUtils related to pipeline destroy (RatisPipelineUtils#destroyPipeline) which were re-introduced in #538 although they were not really used. However, it seems the intent of the methods are for the SCM to destroy the pipeline directly, without going through the command queue. We can delete in this patch if needed.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9959

How was this patch tested?

Unit test for ClosePipelineCommandHandler.

CI run: https://github.com/ivandika3/ozone/actions/runs/7258608368/job/19774884832

…close

ivandika3 · 2023-12-20T02:07:00Z

@sumitagrawl @DaveTeng0 @szetszwo Could you help to review if you have time?

szetszwo

@ivandika3 , thanks for working on this! Please see the comments inlined.

... There seems to be an unused methods in RatisPipelineUtils related to pipeline destroy (RatisPipelineUtils#destroyPipeline) ...

Sure, let's remove the unused methods.

szetszwo · 2023-12-21T01:53:51Z

...e/hadoop/ozone/container/common/statemachine/commandhandler/ClosePipelineCommandHandler.java

+            final Collection<RaftPeer> peers = ratisServer.getRaftPeersInPipeline(pipelineID);
+            final boolean shouldDeleteRatisLogDirectory = ratisServer.getShouldDeleteRatisLogDirectory();
+            peers.stream()
+                .filter(peer -> peer.getId() != ratisServer.getServer().getId())


We should use equals(..) instead of !=.

Updated, thank you for catching it.

szetszwo · 2023-12-21T02:00:30Z

...e/hadoop/ozone/container/common/statemachine/commandhandler/ClosePipelineCommandHandler.java

+                    // ignore silently since this means that the group has been closed by earlier close pipeline
+                    // command in another datanode
+                  } catch (IOException ioe) {
+                    LOG.warn("Remove group failed for peer {}", peer.getId(), ioe);


Let's include the group id.

LOG.warn("Failed to remove group {} for peer {}", raftGroupId, peer.getId(), ioe);

Updated. PTAL.

szetszwo

+1 the change looks good.

adoroszlai · 2023-12-21T08:20:41Z

Test failure in TestBlockOutputStreamWithFailures.test2DatanodesFailure seems related, and it has not been flaky before.

https://github.com/apache/ozone/actions/runs/7283278738/job/19847237481?pr=5827#step:6:132

adoroszlai

Thanks @ivandika3 for the patch.

adoroszlai · 2024-01-03T20:29:02Z

...doop/ozone/container/common/statemachine/commandhandler/TestClosePipelineCommandHandler.java

+/**
+ * Test cases to verify ClosePipelineCommandHandler.
+ */
+@ExtendWith(MockitoExtension.class)


MockitoExtension is unnecessary, there are no variables annotated as @Mock.

Thank you for the review. Updated.

We might also need to remove this for TestCreatePipelineCommandHandler.

adoroszlai · 2024-01-03T20:30:04Z

...doop/ozone/container/common/statemachine/commandhandler/TestClosePipelineCommandHandler.java

+  @BeforeEach
+  public void setup() throws Exception {
+    conf = new OzoneConfiguration();
+    ozoneContainer = Mockito.mock(OzoneContainer.class);


nit: Please add static import for mock(), when(), any(), etc.

Thank you for the review. Updated.

adoroszlai · 2024-01-04T13:32:54Z

Thanks @ivandika3 for the patch, @szetszwo for the review.

szetszwo · 2024-05-17T20:28:20Z

@ivandika3 , TestECContainerRecovery is failing with GroupMismatchException. The failure seems to be related to this change. See @duongkame 's comment.

ivandika3 · 2024-05-18T01:24:10Z

@szetszwo Thanks for the info. Let me take a look.

…close (apache#5827) (cherry picked from commit ac68072)

ivandika3 marked this pull request as ready for review December 19, 2023 14:27

HDDS-9959. Propagate group remove to other datanodes during pipeline …

49d0356

…close

ivandika3 force-pushed the HDDS-9959 branch from 34e1327 to 49d0356 Compare December 20, 2023 01:33

szetszwo reviewed Dec 21, 2023

View reviewed changes

ivandika3 added 3 commits December 21, 2023 10:29

Remove destroy pipeline logic from RatisPipelineUtils

da492b1

Use equals

0392153

Add group id to log

85aeb91

szetszwo approved these changes Dec 21, 2023

View reviewed changes

adoroszlai reviewed Jan 3, 2024

View reviewed changes

ivandika3 added 2 commits January 4, 2024 14:26

Remove MockitoExtension and use static import

83e7549

Merge branch 'master' into HDDS-9959

5d139ff

adoroszlai merged commit ac68072 into apache:master Jan 4, 2024

ivandika3 deleted the HDDS-9959 branch January 25, 2024 04:24

ivandika3 self-assigned this Apr 23, 2024

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-9959. Propagate group remove to other datanodes during pipeline …

89c1eec

…close (apache#5827) (cherry picked from commit ac68072)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-9959. Propagate group remove to other datanodes during pipeline …

2a130b7

…close (apache#5827) (cherry picked from commit ac68072)

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Jul 17, 2024

HDDS-9959. Propagate group remove to other datanodes during pipeline …

18b9ccf

…close (apache#5827) (cherry picked from commit ac68072)

xichen01 mentioned this pull request Jul 18, 2024

[DO NOT MERGE] Backport some fixes, performance optimizations from master to ozone-1.4 #6929 #6964

Merged

HDDS-9959. Propagate group remove to other datanodes during pipeline close #5827

HDDS-9959. Propagate group remove to other datanodes during pipeline close #5827

Uh oh!

Conversation

ivandika3 commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

ivandika3 commented Dec 20, 2023

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Dec 21, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jan 4, 2024

Uh oh!

szetszwo commented May 17, 2024

Uh oh!

ivandika3 commented May 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ivandika3 commented Dec 19, 2023 •

edited

Loading