Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
4ec7f55
HDDS-4926. Support start/stop for container balancer via command line
May 21, 2021
f7cea16
add command line parameters
May 24, 2021
8a26e33
fix for checkstyle
May 24, 2021
5efb584
remove default value
May 27, 2021
a96c1ba
Merge remote-tracking branch 'origin/master' into bak
Jun 1, 2021
fff764b
update proto to add new parameters
Jun 1, 2021
4526b6a
add unit test for container balancer subcommand
Jun 1, 2021
558c0fc
add unit and acceptance test
Jun 1, 2021
45e79e4
fix unit test
Jun 3, 2021
d913cf4
triger CI for TestReportPublisher.testCRLStatusReportPublisher
Jun 3, 2021
2879fb8
Merge remote-tracking branch 'origin/master' into HDDS-4926
Jun 3, 2021
029cf09
Merge remote-tracking branch 'origin/master' into HDDS-4926
Jun 3, 2021
c4f2fc4
update
Jun 7, 2021
3cd3fae
make startContainerBalancer parameters optional
Jun 7, 2021
b575197
use lock instead of sychronized
Jun 7, 2021
e50cfc4
Update hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm…
Jun 9, 2021
11a93dd
update
Jun 10, 2021
040c4de
change maxSizeToMove to maxSizeToMoveInGB
Jun 15, 2021
26a6e54
remove default value for idleiteration
Jun 15, 2021
91e795c
change maxSizeToMoveInGB to MaxSizeToMoveInGB
Jun 15, 2021
a6260d4
fix unit text
Jun 15, 2021
b84dd23
use optional for container balancer start command
Jun 16, 2021
5b483ae
trigger CI
Jun 17, 2021
993bc31
trigger CI again
Jun 17, 2021
62f2b36
trigger CI again
Jun 17, 2021
81a657d
trigger CI again
Jun 17, 2021
6a04ed2
update
Jun 21, 2021
00e34f4
Merge remote-tracking branch 'origin/master' into HDDS-4926
Jun 23, 2021
50029a7
update
Jun 23, 2021
0199f26
update
Jun 23, 2021
44aa8b1
update
Jun 23, 2021
e7abf32
triger CI
Jun 23, 2021
e99c865
triger CI
Jun 23, 2021
b69c5c3
triger CI
Jun 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,24 @@ Map<String, Pair<Boolean, String>> getSafeModeRuleStatuses()
*/
boolean getReplicationManagerStatus() throws IOException;

/**
* Start ContainerBalancer.
*/
boolean startContainerBalancer(double threshold, int idleiterations,
int maxDatanodesToBalance, long maxSizeToMove) throws IOException;

/**
* Stop ContainerBalancer.
*/
void stopContainerBalancer() throws IOException;

/**
* Returns ContainerBalancer status.
*
* @return True if ContainerBalancer is running, false otherwise.
*/
boolean getContainerBalancerStatus() throws IOException;

/**
* returns the list of ratis peer roles. Currently only include peer address.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,24 @@ Map<String, Pair<Boolean, String>> getSafeModeRuleStatuses()
*/
boolean getReplicationManagerStatus() throws IOException;

/**
* Start ContainerBalancer.
*/
boolean startContainerBalancer(double threshold, int idleiterations,
int maxDatanodesToBalance, long maxSizeToMove) throws IOException;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above.


/**
* Stop ContainerBalancer.
*/
void stopContainerBalancer() throws IOException;

/**
* Returns ContainerBalancer status.
*
* @return True if ContainerBalancer is running, false otherwise.
*/
boolean getContainerBalancerStatus() throws IOException;

/**
* Get Datanode usage information by ip or uuid.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public enum SCMAction implements AuditAction {
START_REPLICATION_MANAGER,
STOP_REPLICATION_MANAGER,
GET_REPLICATION_MANAGER_STATUS,
START_CONTAINER_BALANCER,
STOP_CONTAINER_BALANCER,
GET_CONTAINER_BALANCER_STATUS,
GET_CONTAINER_WITH_PIPELINE_BATCH,
ADD_SCM;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.RecommissionNodesResponseProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.ReplicationManagerStatusRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.ReplicationManagerStatusResponseProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.ContainerBalancerStatusRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.ContainerBalancerStatusResponseProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.SCMCloseContainerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.SCMDeleteContainerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.SCMListContainerRequestProto;
Expand All @@ -69,6 +71,9 @@
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StartMaintenanceNodesResponseProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StartReplicationManagerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StopReplicationManagerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StartContainerBalancerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StartContainerBalancerResponseProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.StopContainerBalancerRequestProto;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerLocationProtocolProtos.Type;
import org.apache.hadoop.hdds.scm.DatanodeAdminError;
import org.apache.hadoop.hdds.scm.ScmInfo;
Expand Down Expand Up @@ -712,6 +717,45 @@ public boolean getReplicationManagerStatus() throws IOException {

}

@Override
public boolean startContainerBalancer(double threshold, int idleiterations,
int maxDatanodesToBalance, long maxSizeToMove) throws IOException {
StartContainerBalancerRequestProto request =
StartContainerBalancerRequestProto.newBuilder()
.setIdleiterations(idleiterations)
.setMaxDatanodesToBalance(maxDatanodesToBalance)
.setMaxSizeToMove(maxSizeToMove)
.setThreshold(threshold).build();
StartContainerBalancerResponseProto response =
submitRequest(Type.StartContainerBalancer,
builder -> builder.setStartContainerBalancerRequest(request))
.getStartContainerBalancerResponse();
return response.getStart();
}

@Override
public void stopContainerBalancer() throws IOException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unnecessary blank line can be removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks , will remove it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we remove these extra head and tail black line?

StopContainerBalancerRequestProto request =
StopContainerBalancerRequestProto.getDefaultInstance();
submitRequest(Type.StopContainerBalancer,
builder -> builder.setStopContainerBalancerRequest(request));

}

@Override
public boolean getContainerBalancerStatus() throws IOException {

ContainerBalancerStatusRequestProto request =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

seems i have already used 4 black space indent here

ContainerBalancerStatusRequestProto.getDefaultInstance();
ContainerBalancerStatusResponseProto response =
submitRequest(Type.GetContainerBalancerStatus,
builder -> builder.setContainerBalancerStatusRequest(request))
.getContainerBalancerStatusResponse();
return response.getIsRunning();

}

/**
* Builds request for datanode usage information and receives response.
*
Expand Down
36 changes: 36 additions & 0 deletions hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ message ScmContainerLocationRequest {
optional DatanodeUsageInfoRequestProto DatanodeUsageInfoRequest = 30;
optional GetExistContainerWithPipelinesInBatchRequestProto getExistContainerWithPipelinesInBatchRequest = 31;
optional GetContainerTokenRequestProto containerTokenRequest = 32;
optional StartContainerBalancerRequestProto startContainerBalancerRequest = 33;
optional StopContainerBalancerRequestProto stopContainerBalancerRequest = 34;
optional ContainerBalancerStatusRequestProto containerBalancerStatusRequest = 35;
}

message ScmContainerLocationResponse {
Expand Down Expand Up @@ -109,6 +112,9 @@ message ScmContainerLocationResponse {
optional DatanodeUsageInfoResponseProto DatanodeUsageInfoResponse = 30;
optional GetExistContainerWithPipelinesInBatchResponseProto getExistContainerWithPipelinesInBatchResponse = 31;
optional GetContainerTokenResponseProto containerTokenResponse = 32;
optional StartContainerBalancerResponseProto startContainerBalancerResponse = 33;
optional StopContainerBalancerResponseProto stopContainerBalancerResponse = 34;
optional ContainerBalancerStatusResponseProto containerBalancerStatusResponse = 35;

enum Status {
OK = 1;
Expand Down Expand Up @@ -147,6 +153,9 @@ enum Type {
DatanodeUsageInfo = 25;
GetExistContainerWithPipelinesInBatch = 26;
GetContainerToken = 27;
StartContainerBalancer = 28;
StopContainerBalancer = 29;
GetContainerBalancerStatus = 30;
}

/**
Expand Down Expand Up @@ -445,6 +454,33 @@ message GetContainerTokenResponseProto {
required TokenProto token = 1;
}

message StartContainerBalancerRequestProto {
optional string traceID = 1;
required double threshold = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we using KeyValue here if there will be more fields in future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do all these required fileds have default values? If so, they are optional, insteal of required.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ChenSammi for the reivew. there will not be default value, see this discuss

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think @ChenSammi is pointing out that since all these fields do not have default values they should be optional.

required int32 idleiterations = 3;
required int32 maxDatanodesToBalance = 4;
required int64 maxSizeToMove = 5;
}

message StartContainerBalancerResponseProto {
required bool start = 1;
}

message StopContainerBalancerRequestProto {
optional string traceID = 1;
}

message StopContainerBalancerResponseProto {
}

message ContainerBalancerStatusRequestProto {
optional string traceID = 1;
}

message ContainerBalancerStatusResponseProto {
required bool isRunning = 1;
}

/**
* Protocol used from an HDFS node to StorageContainerManager. See the request
* and response messages for details of the RPC calls.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public class ContainerBalancer {
private double threshold;
private int maxDatanodesToBalance;
private long maxSizeToMove;
private int idleIteration;
private List<DatanodeUsageInfo> unBalancedNodes;
private List<DatanodeUsageInfo> overUtilizedNodes;
private List<DatanodeUsageInfo> underUtilizedNodes;
Expand All @@ -63,6 +64,7 @@ public class ContainerBalancer {
private long clusterRemaining;
private double clusterAvgUtilisation;
private final AtomicBoolean balancerRunning = new AtomicBoolean(false);
private Thread currentBalancingThread;

/**
* Constructs ContainerBalancer with the specified arguments. Initializes
Expand Down Expand Up @@ -97,7 +99,6 @@ public ContainerBalancer(
this.unBalancedNodes = new ArrayList<>();
this.withinThresholdUtilizedNodes = new ArrayList<>();
}

/**
* Starts ContainerBalancer. Current implementation is incomplete.
*
Expand All @@ -111,27 +112,42 @@ public boolean start(ContainerBalancerConfiguration balancerConfiguration) {

ozoneConfiguration = new OzoneConfiguration();
this.config = balancerConfiguration;
this.idleIteration = config.getIdleIteration();
this.threshold = config.getThreshold();
this.maxDatanodesToBalance = config.getMaxDatanodesToBalance();
this.maxSizeToMove = config.getMaxSizeToMove();
this.unBalancedNodes = new ArrayList<>();

LOG.info("Starting Container Balancer...{}", this);
balance();
//we should start a new balancer thread async
//and response to cli as soon as possible


//this is a temporary implementation
Comment thread
lokeshj1703 marked this conversation as resolved.
Outdated
//modify this later
currentBalancingThread = new Thread(() -> balance());
currentBalancingThread.start();
////////////////////////


return true;
}

/**
* Balances the cluster.
*/
private void balance() {
initializeIteration();

// unBalancedNodes is not cleared since the next iteration uses this
// iteration's unBalancedNodes to find out how many nodes were balanced
overUtilizedNodes.clear();
underUtilizedNodes.clear();
withinThresholdUtilizedNodes.clear();
for (int i = 0; i < idleIteration; i++) {
if (!initializeIteration()) {
//balancer should be stopped immediately
break;
}
// unBalancedNodes is not cleared since the next iteration uses this
// iteration's unBalancedNodes to find out how many nodes were balanced
overUtilizedNodes.clear();
underUtilizedNodes.clear();
withinThresholdUtilizedNodes.clear();
}
balancerRunning.compareAndSet(true, false);
}

/**
Expand All @@ -152,7 +168,6 @@ private boolean initializeIteration() {
if (datanodeUsageInfos.isEmpty()) {
LOG.info("Container Balancer could not retrieve nodes from Node " +
"Manager.");
stop();
return false;
}

Expand Down Expand Up @@ -221,15 +236,21 @@ private boolean initializeIteration() {
maxDatanodesToBalance) {
LOG.info("Approaching Max Datanodes To Balance limit in Container " +
"Balancer. Stopping Balancer.");
stop();
return false;
} else {
unBalancedNodes.addAll(overUtilizedNodes);
unBalancedNodes.addAll(underUtilizedNodes);

//for now, we just sleep to simulate the execution of balancer
//this if for acceptance test now. modify this later when balancer
//if fully completed
try {
Thread.sleep(50);
} catch (InterruptedException e) {}
/////////////////////////////

if (unBalancedNodes.isEmpty()) {
LOG.info("Did not find any unbalanced Datanodes.");
stop();
return false;
} else {
LOG.info("Container Balancer has identified Datanodes that need to be" +
Expand Down Expand Up @@ -323,8 +344,22 @@ public static double calculateUtilization(
* Stops ContainerBalancer.
*/
public void stop() {
Comment thread
lokeshj1703 marked this conversation as resolved.
balancerRunning.set(false);
LOG.info("Container Balancer stopped.");
//we should stop the balancer thread gracefully
if(!balancerRunning.get()) {
LOG.info("Container Balancer is not running.");
return;
}


//this is a temporary implementation
//modify this later
if (currentBalancingThread.isAlive()) {
currentBalancingThread.stop();
}
///////////////////////////

balancerRunning.compareAndSet(true, false);
LOG.info("Container Balancer stopped successfully.");
}

public void setNodeManager(NodeManager nodeManager) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ public final class ContainerBalancerConfiguration {
"by Container Balancer.")
private long maxSizeToMove = 10 * OzoneConsts.GB;

@Config(key = "idle.iterations", type = ConfigType.INT,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Based on current implementation, I would suggest changing the "idle.iterations" to "max.iterations". As Lokesh said, we can do it in a seperate PR later to revisit all these configurations and namings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yea, we could revisit it later

defaultValue = "10", tags = {ConfigTag.BALANCER},
description = "The idle iteration count of Container Balancer")
private int idleIterations = 10;

/**
* Gets the threshold value for Container Balancer.
*
Expand All @@ -78,6 +83,28 @@ public void setThreshold(double threshold) {
this.threshold = String.valueOf(threshold);
}

/**
* Gets the idle iteration value for Container Balancer.
*
* @return a idle iteration count larger than 0
*/
public int getIdleIteration() {
return idleIterations;
}

/**
* Sets the idle iteration value for Container Balancer.
*
* @param count a idle iteration count larger than 0
*/
public void setIdleIteration(int count) {
if (count < 1) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aren't we also allowing idleIterations to be -1? In that case this condition needs to be changed.

throw new IllegalArgumentException(
"Idle iteration count must be larger than 0.");
}
this.idleIterations = count;
}

/**
* Gets the value of maximum number of datanodes that will be balanced by
* Container Balancer.
Expand Down
Loading