HDDS-4568. Add SCMContext to SCM HA

[GlenGeng-awx]

What changes were proposed in this pull request?

We want to provide SCMContext, which would be a single source of truth for some key information that is shared across all components within SCM.

SCMContext holds two kind of key information:

• RaftServer related info: isLeader, term.
• SafeMode related info: inSafeMode, preCheckComplete.

What is the link to the Apache JIRA

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

How was this patch tested?

CI

[GlenGeng-awx]

cc @nandakumar131 @ChenSammi @amaliujia

[amaliujia]

I think some of the changes related to new Ratis API are in master branch already. It might be better to merge master to 2823 branch and then rebase this PR to remove those changes (e.g. changes in XceiverServerRatis)

[amaliujia]

This part of a change is contained in master (#1728)

[amaliujia]

I see. so this is from the cherry picked commit from master.

[amaliujia]

Will it make sense to use SCMContext to get current term?

[GlenGeng-awx]

We use SCMContext to encapsulate raft related info, so that components in SCM won't need to hold a reference of SCMHAManager or SCMRatisServer.

For non-HA mode or unit test, we just need an empty SCMContext, instead of a mocked SCMHAManager or a mocked SCMRatisServer.

[linyiqun]

@GlenGeng , I left one minor comment below.

I see now we use the term index value as the SCM leader check. This is used across SCM internal components. Does this will cover the client request behaviour?

For example, one client configured a single SCM address that is a Follower role and then send the request. Will it update the SCM metadata, like pipeline, containers? I see there was a isLeader check before but now that was removed in HDDS-4551. Do you know the context for this?

[linyiqun]

Could we also catch the NotLeaderException like other places does? There are some other places we don't catch this exception that is swallowed by IOException.

        } catch (NotLeaderException nle) {

        }  catch (IOException e) {
          // We may tolerate a number of failures for sometime
          // but if it continues to fail, at some point we need to raise
          // an exception and probably fail the SCM ? At present, it simply
          // continues to retry the scanning.
          LOG.error("Failed to get block deletion transactions from delTX log",
              e);
          return EmptyTaskResult.newResult();
        }

[GlenGeng-awx]

Sure, I will fix them in the next patch.

[GlenGeng-awx]

@GlenGeng , I left one minor comment below.

I see now we use the term index value as the SCM leader check. This is used across SCM internal components. Does this will cover the client request behaviour?

For example, one client configured a single SCM address that is a Follower role and then send the request. Will it update the SCM metadata, like pipeline, containers? I see there was a isLeader check before but now that was removed in HDDS-4551. Do you know the context for this?

Hey Yiqun, thanks for the review!

For example, one client configured a single SCM address that is a Follower role and then send the request.

For now, we need client to know all the SCM instances that engage in the SCM raft cluster, if client send request to a follower SCM, it will get a NotLeaderException, and failover to the next SCM instance.

Will it update the SCM metadata, like pipeline, containers?

We removed the leader check in HDDS-4551, since all the metadata updates that will be saved into rocksdb will go through ratis, as a RaftClientRequest, so if underly Raft is in a non-Leader role, the replied RaftClientReply will be injected with a NotLeaderException.

Please check SCMHAInvocationHandler to see how we implement this: for now, add container, remove container, update container state, add pipeline, remove pipeline, update pipeline state will go through ratis.

If needed, we can schedule a zoom meeting to discuss about current SCM HA design.

[linyiqun]

@GlenGeng , thanks for the detailed explanation!

[amaliujia]

Thanks Glen to rebase PR! Will try to give another pass on this PR.

[amaliujia]

Overall LGTM. Left a comment.

[runzhiwang]

LGTM

[bshashikant]

Thanks @GlenGeng for detailed explanation. The changes look good to me except one minor comment. Thanks for the efforts.