HDDS-4295. Add SCMServiceManager to SCM HA. #1784

GlenGeng-awx · 2021-01-12T11:03:59Z

What changes were proposed in this pull request?

SCM ServiceManager is going to control all the SCM background service so that they are only serving as the leader.

Design doc: https://docs.google.com/document/d/1DbbqP0m3g_iEpY9qkSGOuQgcCN-QqlSNgWpvBOLv5h0/edit

What is the link to the Apache JIRA

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

How was this patch tested?

CI

bshashikant · 2021-01-13T09:46:30Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMServiceManager.java

+   */
+  public synchronized void leaderToFollower() {
+    LOG.info("leaderToFollower is called.");
+    raftCondition = RaftCondition.FOLLOWER_TO_LEADER;


RaftCondition.FOLLOWER_TO_LEADER -> RaftCondition.LEADER_TO_FOLLOWER

Thanks for the catch.

bshashikant · 2021-01-13T09:51:28Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

+   * @return term
+   * @throws NotLeaderException if isLeader is false
+   */
+  public long getTerm() throws NotLeaderException {


I think the function name shoulld be changed to getTermOfLeader() or something like this to make it intuitive enough as this is supposed to be only a leader executed code

bshashikant · 2021-01-13T09:53:15Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

+   * @param newIsLeader : is leader or not
+   * @param newTerm     : term if current SCM becomes leader
+   */
+  public void updateIsLeaderAndTerm(boolean newIsLeader, long newTerm) {


newIsLeader doesn't make much sense here

Agree, will find a better name. Previously it is isLeader and term, but can not pass checkstyle.

bshashikant · 2021-01-13T09:54:01Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

+   * @param newIsLeader : is leader or not
+   * @param newTerm     : term if current SCM becomes leader
+   */
+  public void updateIsLeaderAndTerm(boolean newIsLeader, long newTerm) {


updateIsLeaderAndTerm -> updateLeaderAndTerm

bshashikant · 2021-01-13T10:02:06Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMService.java

+   * @param raftCondition latest raft condition
+   * @param safeModeCondition latest safe mode condition
+   */
+  void notifyRaftCondOrSafeModeCondChanged(


Can we add some more info here, regarding what would be one time transitional events and what would be recurring which needs to be notified ?

bshashikant · 2021-01-13T10:08:31Z

Thanks @GlenGeng for working on this. I went through the patch once and given some comments inline. Still need to look through in more details.

I also want to know, how are we handling case of a stale leader here? I know there is an effort inside ratis to address/detect a case of stale leader., but don't we really need anything specific to handle such case here ?

Also, the transitional cases seem to be handled for LEADER_TO_FOLLOWER and FOLLOWER_TO_LEADER? How about transition to CANDIDATE state in general?

amaliujia · 2021-01-13T20:42:31Z

cc @amaliujia

GlenGeng-awx · 2021-01-14T03:55:20Z

@bshashikant Thanks for reviewing this patch!

I also want to know, how are we handling case of a stale leader here? I know there is an effort inside ratis to address/detect a case of stale leader., but don't we really need anything specific to handle such case here ?

The request inside SCM can be divided into two categories, read/write request and read only request.

The read/write request will go through ratis as a RaftClientRequest, so we don’t need to care about whether the underlying RaftServer is a still leader or has already stepped down. These read/write requests include add/remove/update pipeline/container/deleted block.

For the read only request, they will not go through ratis, a stale leader SCM might make a dangerous decision if it misses the latest committed writes, a.k.a the split-brain issues occurred, two leaders of different terms live together.

Nanda pointed out such a case:

Our major concern here is the DeleteContainerCommand sent by the stale leader. It is possible that the ICR of the recently created container is received by the stale leader which has no idea about the new container. The stale leader will end up sending DeleteContainerCommand to the datanode in this case.

See SCM HA Handling Stale Leader drafted by @nandakumar131 for more details.

With help of the lease solution, leader SCM can handle the read only request, then call isLeader/getTermOfLeader in SCMContext to confirm its leadership, ensuring that when it handles the read only request, it has seen the latest commit writes.

Also, the transitional cases seem to be handled for LEADER_TO_FOLLOWER and FOLLOWER_TO_LEADER? How about transition to CANDIDATE state in general?

The candidate state is not expose by Ratis, and SCM does not cares about this state either. SCM just treat candidate the same as follower: it should stop the work if not leader.

SCMServiceManager#leaderToFollower is called in StateMachine#notifyNotLeader and SCMServiceManager#followerToLeader is called in StateMachine#notifyLeaderChanged when groupMemberId.getPeerId().equals(newLeaderId)

We may have a offline talk to discuss more about the subtle cases in how we integrate SCM HA with ratis.

GlenGeng-awx · 2021-01-14T12:09:21Z

@bshashikant @amaliujia I've update according to your comments, please take one more look Thanks!

bshashikant · 2021-01-18T16:16:36Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

+ *  - SafeMode related info, e.g., inSafeMode, preCheckComplete.
+ */
+public final class SCMContext implements EventHandler<SafeModeStatus> {
+  private static final Logger LOG = LoggerFactory.getLogger(SCMContext.class);


In case where SCM HA feature is not enabled (no Ratis), SCM context still seem to be relevant. In such cases, leader info and term info don't make sense. Can we add some constants to define leader state and term when ratis is not enabled?

Hey @bshashikant, this is a good point.

Due to PR #1744, integration test which is bases on MiniOzoneCluster has been running upon single-server raft cluster. But we still have cases that run without Ratis, e.g., UnitTest, Recon.

Here is the design decision of SCMContext:

The initial value of currentTerm in raft is 0. After RaftServer starts, they are all followers of term 0, then election timeout happens, one becomes candidate of term 1, finally the first leader will be elected, which is on a term that is greater or equal to 1.

SCMContext has two raft related field, term and isLeader.

For the cases using Ratis (no matter single server or multi server), the initial value of term and isLeader for SCMContext is 0 and false, they will be updated by SCMStateMachine#notifyNotLeader and SCMStateMachine#notifyLeaderChanged.

For the cases not using Ratis, the initial value of term and isLeader for SCMContext is 0 and true, means leader of term 0.

Since all the isLeader and getTermOfLeader check are going through SCMConext, and they are spreading across the whole SCM, for Ratis mode, it returns real raft info, for non Ratis mode, it behaviors as leader on term 0, so that we treat them in the same way.

What do you think ? We can discuss offline if needed.

GlenGeng-awx · 2021-01-21T10:44:44Z

cc @bshashikant Please take one more look !

...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMService.java

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMServiceManager.java

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/BackgroundPipelineCreatorV2.java

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

GlenGeng-awx · 2021-01-22T14:03:32Z

@bshashikant Comments for SCMServiceManager has been addressed, Please take one more look. Thanks!

bshashikant

Thanks @GlenGeng for updating the patch.The changes look good to me. I need a clarification regarding replicationManager.

bshashikant · 2021-01-25T09:45:13Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMService.java

+   */
+  enum ServiceStatus {
+    RUNNING,
+    PAUSING


I think, we should add a status called STOPPED as well here. For example, if i want to suspend replication i can issue, ozone admin replicationManager stop. This state might need to be propagated over Ratis as well. Even after leader re-relection, the service should not auto start on the new leader.

Thanks for the insight.
I will handle bin/ozone admin replicationmanager start|stop|status in this PR.
I agree that the status of RM should also be replicated through ratis. I created a jira https://issues.apache.org/jira/browse/HDDS-4740 to track this issue, will schedule a discussion in this week's sync.

create jira for Add STOP state to SCMService.
https://issues.apache.org/jira/browse/HDDS-4744

I guess the JIRA link of adding STOP state to SCM service was not properly linked?

Thanks for the catch.

amaliujia · 2021-01-27T05:47:46Z

...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMBlockDeletingService.java

+   * SCMService related variables.
+   */
+  private final Lock serviceLock = new ReentrantLock();
+  private ServiceStatus serviceStatus = ServiceStatus.PAUSING;


Instead of setting default value as ServiceStatus.PAUSING, will it make sense to initialize this variable as RUNNING during construction?

Actually not. BlockDeletingService should not run on follower SCMs.

O yes thanks!

amaliujia · 2021-01-27T05:50:53Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMService.java

+   */
+  enum ServiceStatus {
+    RUNNING,
+    PAUSING


I guess the JIRA link of adding STOP state to SCM service was not properly linked?

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMServiceManager.java

amaliujia · 2021-01-27T05:56:13Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMServiceManager.java

+  /**
+   * Notify raft or safe mode related status changed.
+   */
+  public synchronized void notifyStatusChanged() {


Question: Does notifyStatusChanged has to be a blocking call?

The execution of this method is quite quick. We may consider relax it if we meet performance issue in future.

amaliujia · 2021-01-27T05:58:24Z

...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/BackgroundPipelineCreatorV2.java

+/**
+ * Implements api for running background pipeline creation jobs.
+ */
+public class BackgroundPipelineCreatorV2 implements SCMService {


I might have missed: is this class tested somehow?

BackgroundPipelineCreator is replaced by BackgroundPipelineCreatorV2, and it has been fully test in integration test and docker test.

amaliujia · 2021-01-27T18:06:51Z

LGTM!

runzhiwang · 2021-01-28T02:14:55Z

@GlenGeng Thanks the patch. @bshashikant @amaliujia Thanks for review. I have merged it.

GlenGeng-awx force-pushed the HDDS-4295-Latest branch from 7978388 to b03897f Compare January 12, 2021 16:06

GlenGeng-awx closed this Jan 13, 2021

GlenGeng-awx reopened this Jan 13, 2021

bshashikant reviewed Jan 13, 2021

View reviewed changes

GlenGeng-awx mentioned this pull request Jan 14, 2021

HDDS-4568. Add SCMContext to SCM HA #1737

Merged

GlenGeng-awx force-pushed the HDDS-4295-Latest branch 3 times, most recently from 2686e67 to 6e8b787 Compare January 18, 2021 09:38

bshashikant reviewed Jan 18, 2021

View reviewed changes

GlenGeng-awx force-pushed the HDDS-4295-Latest branch from 6e8b787 to d0c9ef9 Compare January 19, 2021 05:22

GlenGeng-awx closed this Jan 19, 2021

GlenGeng-awx reopened this Jan 19, 2021

GlenGeng-awx closed this Jan 19, 2021

GlenGeng-awx reopened this Jan 19, 2021

mukul1987 added the scm-ha label Jan 19, 2021

Glen Geng added 3 commits January 21, 2021 11:24

HDDS-4295. Add SCMServiceManager to SCM HA.

69404d2

HDDS-4295. Fix typo

1141a7f

HDDS-4295: Fix comments

58145fa

GlenGeng-awx force-pushed the HDDS-4295-Latest branch from 972c969 to 58145fa Compare January 21, 2021 09:02

bshashikant requested changes Jan 21, 2021

View reviewed changes

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMContext.java Show resolved Hide resolved

HDDS-4295. Fix comments

3cbc6e6

GlenGeng-awx closed this Jan 22, 2021

GlenGeng-awx reopened this Jan 22, 2021

GlenGeng-awx requested a review from bshashikant January 22, 2021 14:17

bshashikant requested changes Jan 25, 2021

View reviewed changes

amaliujia reviewed Jan 27, 2021

View reviewed changes

HDDS-4295. Fix comments.

6b97491

bshashikant approved these changes Jan 27, 2021

View reviewed changes

runzhiwang merged commit 3b6918e into apache:HDDS-2823 Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-4295. Add SCMServiceManager to SCM HA. #1784

HDDS-4295. Add SCMServiceManager to SCM HA. #1784

GlenGeng-awx commented Jan 12, 2021

bshashikant Jan 13, 2021

GlenGeng-awx Jan 14, 2021

bshashikant Jan 13, 2021

GlenGeng-awx Jan 14, 2021

bshashikant Jan 13, 2021

GlenGeng-awx Jan 14, 2021

bshashikant Jan 13, 2021

bshashikant Jan 13, 2021

GlenGeng-awx Jan 14, 2021

bshashikant commented Jan 13, 2021

amaliujia commented Jan 13, 2021

GlenGeng-awx commented Jan 14, 2021

GlenGeng-awx commented Jan 14, 2021

bshashikant Jan 18, 2021 •

edited

GlenGeng-awx Jan 19, 2021

GlenGeng-awx commented Jan 21, 2021

GlenGeng-awx commented Jan 22, 2021

bshashikant left a comment

bshashikant Jan 25, 2021

GlenGeng-awx Jan 25, 2021

GlenGeng-awx Jan 26, 2021 •

edited

amaliujia Jan 27, 2021

GlenGeng-awx Jan 27, 2021

amaliujia Jan 27, 2021

GlenGeng-awx Jan 27, 2021

amaliujia Jan 27, 2021

amaliujia Jan 27, 2021

amaliujia Jan 27, 2021

GlenGeng-awx Jan 27, 2021

amaliujia Jan 27, 2021

amaliujia Jan 27, 2021

GlenGeng-awx Jan 27, 2021

amaliujia commented Jan 27, 2021

runzhiwang commented Jan 28, 2021

HDDS-4295. Add SCMServiceManager to SCM HA. #1784

HDDS-4295. Add SCMServiceManager to SCM HA. #1784

Conversation

GlenGeng-awx commented Jan 12, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshashikant commented Jan 13, 2021

amaliujia commented Jan 13, 2021

GlenGeng-awx commented Jan 14, 2021

GlenGeng-awx commented Jan 14, 2021

bshashikant Jan 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenGeng-awx commented Jan 21, 2021

GlenGeng-awx commented Jan 22, 2021

bshashikant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GlenGeng-awx Jan 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia commented Jan 27, 2021

runzhiwang commented Jan 28, 2021

bshashikant Jan 18, 2021 •

edited

GlenGeng-awx Jan 26, 2021 •

edited