smengcl commented on code in PR #11399:
URL: https://github.com/apache/ozone/pull/11399#discussion_r4265780128
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -652,15 +658,84 @@ private boolean isValidSCMState() {
}
private IterationResult doIteration() {
+ moveSelectionToFutureMap = new ConcurrentHashMap<>();
+ iterationResult = IterationResult.ITERATION_COMPLETED;
+ boolean isMoveGeneratedInThisIteration = false;
+
+ // Balance one storage type at a time so a container never moves to a
+ // datanode volume of a different tier. Iterating StorageType.values()
keeps
+ // the order stable across iterations.
+ for (StorageType storageType : storageTypesToBalance()) {
+ if (balanceStorageType(storageType)) {
+ isMoveGeneratedInThisIteration = true;
+ }
+ // Stop early when the balancer was stopped, or when the iteration-wide
+ // size limit is reached, since both apply across all storage types.
+ if (iterationResult == IterationResult.ITERATION_INTERRUPTED
+ || reachedMaxSizeToMovePerIteration()) {
+ break;
+ }
+ }
+
+ checkIterationResults(isMoveGeneratedInThisIteration);
+ return iterationResult;
+ }
+
+ /**
+ * Storage types worth balancing, that is those present on at least one of
the
+ * unbalanced datanodes. Returns a single null element when no datanode
reports
+ * a storage type, which runs one tier-agnostic pass and preserves the
behaviour
+ * from before storage type support.
+ */
+ private List<StorageType> storageTypesToBalance() {
+ Set<StorageType> present = EnumSet.noneOf(StorageType.class);
+ for (DatanodeUsageInfo node : overUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ for (DatanodeUsageInfo node : underUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ if (present.isEmpty()) {
+ return Collections.singletonList(null);
+ }
+ List<StorageType> ordered = new ArrayList<>();
+ for (StorageType type : StorageType.values()) {
+ if (present.contains(type)) {
+ ordered.add(type);
+ }
+ }
+ return ordered;
+ }
+
+ /**
+ * Runs one source-to-target matching pass restricted to a single storage
type.
+ *
+ * @param storageType the tier to balance, or null to consider all tiers
+ * @return true if at least one move was generated
+ */
+ private boolean balanceStorageType(@Nullable StorageType storageType) {
+ // Only consider nodes that actually have a volume of this storage type,
+ // otherwise a source with no capacity on this tier would be picked and
then
+ // find no container to move.
+ List<DatanodeUsageInfo> potentialTargets =
+ nodesWithStorageType(getPotentialTargets(), storageType);
+ List<DatanodeUsageInfo> potentialSources =
+ nodesWithStorageType(getPotentialSources(), storageType);
Review Comment:
Use utilization of the tier being balanced.
This filters nodes by storage-type presence, but classification, ranking,
and utilization bounds still use whole-node statistics. The new per-tier
`calculateUtilization()` methods have no balancer callers. A probe selected a
95%-full SSD over a 20%-full SSD because their overall utilization was 47.5%
and 50%, then accepted a move that would exceed the first SSD’s capacity.
Please use tier-specific statistics throughout classification and move
selection.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/RatisUnderReplicationHandler.java:
##########
@@ -147,8 +148,10 @@ public int processAndSendCommands(
throw e;
}
+ // Keep new replicas on the same tier as the existing ones.
int commandsSent = sendReplicationCommands(
- containerInfo, sourceDatanodes, targetDatanodes);
+ containerInfo, sourceDatanodes, targetDatanodes,
+ targetStorageTypeFor(replicaCount.getReplicas()));
Review Comment:
Use the same storage type for target selection and replication.
`getTargets()` still requests `StorageType.DEFAULT` (DISK), but this command
can now require SSD or ARCHIVE. A DISK-only target therefore passes placement
selection and rejects the import. This can leave the container under-replicated
even when suitable SSD nodes are available. Please pass the required type into
target selection too. The changed EC and misreplication handlers have the same
mismatch.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -652,15 +658,84 @@ private boolean isValidSCMState() {
}
private IterationResult doIteration() {
+ moveSelectionToFutureMap = new ConcurrentHashMap<>();
+ iterationResult = IterationResult.ITERATION_COMPLETED;
+ boolean isMoveGeneratedInThisIteration = false;
+
+ // Balance one storage type at a time so a container never moves to a
+ // datanode volume of a different tier. Iterating StorageType.values()
keeps
+ // the order stable across iterations.
+ for (StorageType storageType : storageTypesToBalance()) {
+ if (balanceStorageType(storageType)) {
+ isMoveGeneratedInThisIteration = true;
+ }
+ // Stop early when the balancer was stopped, or when the iteration-wide
+ // size limit is reached, since both apply across all storage types.
+ if (iterationResult == IterationResult.ITERATION_INTERRUPTED
+ || reachedMaxSizeToMovePerIteration()) {
+ break;
+ }
+ }
+
+ checkIterationResults(isMoveGeneratedInThisIteration);
+ return iterationResult;
+ }
+
+ /**
+ * Storage types worth balancing, that is those present on at least one of
the
+ * unbalanced datanodes. Returns a single null element when no datanode
reports
+ * a storage type, which runs one tier-agnostic pass and preserves the
behaviour
+ * from before storage type support.
+ */
+ private List<StorageType> storageTypesToBalance() {
+ Set<StorageType> present = EnumSet.noneOf(StorageType.class);
+ for (DatanodeUsageInfo node : overUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ for (DatanodeUsageInfo node : underUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ if (present.isEmpty()) {
+ return Collections.singletonList(null);
+ }
+ List<StorageType> ordered = new ArrayList<>();
+ for (StorageType type : StorageType.values()) {
+ if (present.contains(type)) {
+ ordered.add(type);
+ }
+ }
+ return ordered;
+ }
+
+ /**
+ * Runs one source-to-target matching pass restricted to a single storage
type.
+ *
+ * @param storageType the tier to balance, or null to consider all tiers
+ * @return true if at least one move was generated
+ */
+ private boolean balanceStorageType(@Nullable StorageType storageType) {
+ // Only consider nodes that actually have a volume of this storage type,
+ // otherwise a source with no capacity on this tier would be picked and
then
+ // find no container to move.
+ List<DatanodeUsageInfo> potentialTargets =
+ nodesWithStorageType(getPotentialTargets(), storageType);
+ List<DatanodeUsageInfo> potentialSources =
+ nodesWithStorageType(getPotentialSources(), storageType);
+ if (potentialSources.isEmpty() || potentialTargets.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skipping storage type {}: {} sources and {} targets have a "
+ + "volume of this type.", storageType, potentialSources.size(),
+ potentialTargets.size());
+ }
+ return false;
+ }
+
// note that potential and selected targets are updated in the following
// loop
- List<DatanodeUsageInfo> potentialTargets = getPotentialTargets();
findTargetStrategy.reInitialize(potentialTargets, config, upperLimit);
- findSourceStrategy.reInitialize(getPotentialSources(), config, lowerLimit);
+ findSourceStrategy.reInitialize(potentialSources, config, lowerLimit);
- moveSelectionToFutureMap = new ConcurrentHashMap<>();
boolean isMoveGeneratedInThisIteration = false;
- iterationResult = IterationResult.ITERATION_COMPLETED;
boolean canAdaptWhenNearingLimits = true;
boolean canAdaptOnReachingLimits = true;
Review Comment:
Keep the storage-type filter when adapting to iteration limits.
Both adaptation methods reset candidates from `selectedTargets` and
`selectedSources`, which contain nodes from all earlier passes. A later DISK
pass can therefore regain an SSD-only target and select it for a DISK move. The
probe confirmed this replacement. Please intersect the selected nodes with the
current tier’s candidates before resetting the strategies.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancerTask.java:
##########
@@ -652,15 +658,84 @@ private boolean isValidSCMState() {
}
private IterationResult doIteration() {
+ moveSelectionToFutureMap = new ConcurrentHashMap<>();
+ iterationResult = IterationResult.ITERATION_COMPLETED;
+ boolean isMoveGeneratedInThisIteration = false;
+
+ // Balance one storage type at a time so a container never moves to a
+ // datanode volume of a different tier. Iterating StorageType.values()
keeps
+ // the order stable across iterations.
+ for (StorageType storageType : storageTypesToBalance()) {
+ if (balanceStorageType(storageType)) {
+ isMoveGeneratedInThisIteration = true;
+ }
+ // Stop early when the balancer was stopped, or when the iteration-wide
+ // size limit is reached, since both apply across all storage types.
+ if (iterationResult == IterationResult.ITERATION_INTERRUPTED
+ || reachedMaxSizeToMovePerIteration()) {
+ break;
+ }
+ }
+
+ checkIterationResults(isMoveGeneratedInThisIteration);
+ return iterationResult;
+ }
+
+ /**
+ * Storage types worth balancing, that is those present on at least one of
the
+ * unbalanced datanodes. Returns a single null element when no datanode
reports
+ * a storage type, which runs one tier-agnostic pass and preserves the
behaviour
+ * from before storage type support.
+ */
+ private List<StorageType> storageTypesToBalance() {
+ Set<StorageType> present = EnumSet.noneOf(StorageType.class);
+ for (DatanodeUsageInfo node : overUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ for (DatanodeUsageInfo node : underUtilizedNodes) {
+ present.addAll(node.getStorageTypes());
+ }
+ if (present.isEmpty()) {
+ return Collections.singletonList(null);
+ }
+ List<StorageType> ordered = new ArrayList<>();
+ for (StorageType type : StorageType.values()) {
+ if (present.contains(type)) {
+ ordered.add(type);
+ }
+ }
+ return ordered;
+ }
+
+ /**
+ * Runs one source-to-target matching pass restricted to a single storage
type.
+ *
+ * @param storageType the tier to balance, or null to consider all tiers
+ * @return true if at least one move was generated
+ */
+ private boolean balanceStorageType(@Nullable StorageType storageType) {
+ // Only consider nodes that actually have a volume of this storage type,
+ // otherwise a source with no capacity on this tier would be picked and
then
+ // find no container to move.
+ List<DatanodeUsageInfo> potentialTargets =
+ nodesWithStorageType(getPotentialTargets(), storageType);
+ List<DatanodeUsageInfo> potentialSources =
+ nodesWithStorageType(getPotentialSources(), storageType);
+ if (potentialSources.isEmpty() || potentialTargets.isEmpty()) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Skipping storage type {}: {} sources and {} targets have a "
+ + "volume of this type.", storageType, potentialSources.size(),
+ potentialTargets.size());
+ }
+ return false;
+ }
+
// note that potential and selected targets are updated in the following
// loop
- List<DatanodeUsageInfo> potentialTargets = getPotentialTargets();
findTargetStrategy.reInitialize(potentialTargets, config, upperLimit);
- findSourceStrategy.reInitialize(getPotentialSources(), config, lowerLimit);
+ findSourceStrategy.reInitialize(potentialSources, config, lowerLimit);
Review Comment:
Preserve transfer counters across storage-type passes.
Both `reInitialize()` methods clear their traffic maps. Each pass therefore
forgets the transfers scheduled by earlier passes. A probe scheduled two
60-byte moves into one target with a 100-byte limit, while the final target
counter reported only 60 bytes. Initialize the strategies once per iteration,
then reset only their candidates for each tier.
```diff
@@
private IterationResult doIteration() {
+ findTargetStrategy.reInitialize(getPotentialTargets(), config,
upperLimit);
+ findSourceStrategy.reInitialize(getPotentialSources(), config,
lowerLimit);
moveSelectionToFutureMap = new ConcurrentHashMap<>();
@@
- findTargetStrategy.reInitialize(potentialTargets, config, upperLimit);
- findSourceStrategy.reInitialize(potentialSources, config, lowerLimit);
+ findTargetStrategy.resetPotentialTargets(potentialTargets.stream()
+
.map(DatanodeUsageInfo::getDatanodeDetails).collect(Collectors.toList()));
+ findSourceStrategy.resetPotentialSources(potentialSources.stream()
+
.map(DatanodeUsageInfo::getDatanodeDetails).collect(Collectors.toList()));
```
This passed `git apply --check` and the Java 21 probe: the second move was
blocked and the counter stayed correct.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
