HDDS-7383. Basic framework of DiskBalancerService #3874

symious · 2022-10-24T07:53:38Z

What changes were proposed in this pull request?

HDDS-7233 is a big patch that is not good for review.

I will split HDDS-7233 to multi smaller tickets.

This ticket introduces the DiskBalancerService on Datanode side, while the detailed implementation of move operation will be added in other tickets.

What is the link to the Apache JIRA

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

How was this patch tested?

unit test.

…#3701)

symious · 2022-10-24T07:54:37Z

@ChenSammi @ferhui @lokeshj1703 @siddhantsangwan @sodonnel @neils-dev @JacksonYao287 Could you help to review this PR?

myskov · 2022-10-25T07:08:03Z

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

      LoggerFactory.getLogger(DiskBalancerConfiguration.class);

+  // The path where datanode diskBalancer's conf is to be written to.
+  public static final String HDDS_DATANODE_DISK_BALANCER_INFO_DIR =


These configuration properties should be put to ozone-default.xml

@lokeshj1703 Could you help to check this comment?

Please add comments that if this property is not defined, ozone will fallback to use meta data directory instead.

myskov · 2022-10-25T07:12:15Z

...ner-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java

  private static final Logger LOG =
      LoggerFactory.getLogger(StorageVolume.class);

+  public static final String TMP_DIR = "tmp";


Probably it makes sense to parametrize this (like hdds.datanode.replication.work.dir for example)

This one should be related to another ticket, https://issues.apache.org/jira/browse/HDDS-6449.
I think we can resolve this value after HDDS-6449.

Hi @symious , does your previous container replication improvement patch cover this part?

I can not find the definition of "tmp" now, seems being removed from StorageVolume class.

the tmp dir defined in HDDS-6449 , in review, currently defines the tmp directory in the HddsVolume. Does it need to be defined in this the underlying base class? What was discussed in the previous patch was,

hdds/clusterID/tmp would be the best place to put it, but we need to make sure this will not affect datanode startup on downgrade.

@neils-dev , I think define tmp in StorageVolume is more generic than define it in HddsVolume, in case in future other types of Volume need tmp too. what do you think?

...r-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerYaml.java

...rc/test/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerServiceTestImpl.java

myskov · 2022-10-25T07:25:28Z

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

      Optional<List<String>> hosts) throws IOException {
-    // TODO: Send message to datanodes
-    return new ArrayList<DatanodeAdminError>();
+    // check admin authorisation


I believe this comment is redundant

myskov · 2022-10-25T07:25:36Z

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

      throws IOException {
-    // TODO: Send message to datanodes
-    return new ArrayList<DatanodeAdminError>();
+    // check admin authorisation


I believe this comment is redundant

myskov · 2022-10-25T07:25:44Z

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

      throws IOException {
-    // TODO: Send message to datanodes
-    return new ArrayList<DatanodeAdminError>();
+    // check admin authorisation


I believe this comment is redundant

...ainer-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/package-info.java

symious · 2022-10-25T08:22:17Z

@myskov Thank you for the review. Updated the PR, please have a look.

DaveTeng0 · 2023-01-30T08:15:34Z

@ChenSammi @ferhui @lokeshj1703 @siddhantsangwan @sodonnel @neils-dev @JacksonYao287 Could you help to review this PR?

hey guys! Let's continue back to gradually digest this PR when we have some time! lol

ChenSammi · 2023-02-07T09:06:16Z

...c/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java

+  public void setDiskBalancerTimeout(Duration duration) {
+    this.diskBalancerTimeout = duration.toMillis();
+  }
+


Can we move all disk balancer related configuration to DiskBalancerConfiguration?

ChenSammi · 2023-02-07T13:12:42Z

hadoop-hdds/interface-client/src/main/proto/hdds.proto

    UNKNOWN = 3;
 }

+enum DatanodeDiskBalancerOpType{


DatanodeDiskBalancerOpType -> DiskBalancerOpType, consistent with DiskBalancerRunningStatus.

Updated, please have a check.

ChenSammi · 2023-02-07T13:16:48Z

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

+  public static final String
+      HDDS_DATANODE_DISK_BALANCER_CONTAINER_CHOOSING_POLICY =
+      "hdds.datanode.disk.balancer.container.choosing.policy";
+


Do these three property have the default value? Can they use the @config format as other properties in this file?

ChenSammi · 2023-02-07T13:23:34Z

...he/hadoop/ozone/container/common/statemachine/commandhandler/DiskBalancerCommandHandler.java

+      case update:
+        diskBalancerInfo.updateFromConf(diskBalancerConf);
+        break;
+      default:


Please throw exception here.

ChenSammi · 2023-02-07T13:48:31Z

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/DiskBalancerManager.java

+    List<DatanodeDetails> dns;
+    if (hosts.isPresent()) {
+      dns = NodeUtils.mapHostnamesToDatanodes(nodeManager, hosts.get(),
+          useHostnames);


Should avoid send command to dn which is not healthy too.

Updated, please have a check.

ChenSammi · 2023-02-07T13:58:03Z

...r-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerInfo.java

+  private boolean shouldRun;
+  private double threshold;
+  private long bandwidthInMB;
+  private int parallelThread;


Add a field to identify the version number. In future, there could be new item added.

@ChenSammi Could you share some more infor on the "version number"? I didn't quite get the idea.

Version number is usually to support the backward compatibility.
Currently, these four properties are persistent on disk, let's say it's V1. In future, for example, to support one bandwidthInMB for night time, and one bandwidthInMB in day time, then there will be new properties need persist, it is V2.

If ozone binaries are upgraded to V2, but the on disk info file is still V1, then ozone should recognize the file format as V1 and properly handle it. With a version number persist, we can do that. You can refer to the layout version in container file.

symious · 2023-02-08T02:15:22Z

does your previous container replication improvement patch cover this part?

@ChenSammi Yes, there should be many conflicts between this PR and master. I will try to do the rebase late this week.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java

...src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerServiceMetrics.java

...ervice/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

hadoop-hdds/interface-client/src/main/proto/hdds.proto

neils-dev · 2023-02-16T00:20:26Z

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

+      tags = { DATANODE, ConfigTag.DISKBALANCER},
+      description = "The version of the disk balancer service"
+  )
+  private String diskBalancerVersion = "ONE";


Numeric version string would be nice (1.0). Same with Config version default value. Any particular reason it is not? Better fidelity with numeric value string.

@symious , we don't need to expose this version as a configurable property. We can remove it in this class.

neils-dev · 2023-02-16T00:20:34Z

...-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/storage/DiskBalancerConfiguration.java

+
+  public DiskBalancerConfiguration(Optional<Double> threshold,
+      Optional<Long> bandwidthInMB,
+      Optional<Integer> parallelThread) {


The DiskBalancerConfiguration constructor and state variables for configuration do not appear to include the balancePolicy configuration that is documented in the design doc. Is it an oversight? It would be great to include it in the basic framework PR and support balancePolicies, default DefaultBalancePolicy, can balance between different disk mediums.

include the following configurations:
threshold: threshold is used to calculate the volume data density to decide which datanode is empty and needs balance.
bandwidth: a parameter to control the balance speed, so that the balance job won't affect the datanode's IO performance.
parallelThreads: a parameter that determines how many balance jobs are running simultaneously.
balancePolicy: the policy to choose which disk to balance, for example, only balance between SSD disks.

@neils-dev Thanks for the review.

I was preparing to add the balancePolicy part in another ticket after the basic framework.

neils-dev · 2023-02-16T00:20:46Z

...ner-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/StorageVolume.java

  private static final Logger LOG =
      LoggerFactory.getLogger(StorageVolume.class);

+  public static final String TMP_DIR = "tmp";


the tmp dir defined in HDDS-6449 , in review, currently defines the tmp directory in the HddsVolume. Does it need to be defined in this the underlying base class? What was discussed in the previous patch was,

hdds/clusterID/tmp would be the best place to put it, but we need to make sure this will not affect datanode startup on downgrade.

…lume

ChenSammi · 2023-02-23T09:10:39Z

The last patch LGTM +1.

Thanks @symious for working on this, and @myskov @neils-dev for the code review.

symious and others added 5 commits August 24, 2022 16:37

HDDS-7106. [DiskBalancer] Client-SCM interface (apache#3663)

09a0b8e

HDDS-7155. [DiskBalancer] Create interface between SCM and DN (apache…

b239018

…#3701)

HDDS-7205. DiskBalancer CLI (apache#3739)

4a0132e

HDDS-7234. Add a common option for DiskBalancer commands (apache#3762)

35791f5

HDDS-7383. Basic framework of DiskBalancerService

34c5b5e

HDDS-7383. Remove unused field

40bebd8

myskov reviewed Oct 25, 2022

View reviewed changes

...ainer-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/package-info.java Show resolved Hide resolved

HDDS-7383. Fix as comments

7c2f62b

symious added 3 commits October 25, 2022 20:59

trigger new CI check

627d50a

trigger new CI check

c526b0e

trigger new CI check

10183be

ChenSammi reviewed Feb 7, 2023

View reviewed changes