Skip to content

HDDS-16173. Add cluster utilization analysis for container balancer CLI commands - #11037

Draft
sravani-revuri wants to merge 5 commits into
apache:masterfrom
sravani-revuri:HDDS-16173
Draft

HDDS-16173. Add cluster utilization analysis for container balancer CLI commands#11037
sravani-revuri wants to merge 5 commits into
apache:masterfrom
sravani-revuri:HDDS-16173

Conversation

@sravani-revuri

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

We are adding three new CLI commands — assessment, dry-run, and recommend — that all need the same cluster view. These commands run at client-side and fetch datanode usage info via the existing getDatanodeUsageInfo RPC.

This Jira builds the shared foundation that is a common module that takes datanode usage protos,

Responsibilities:

Apply include/exclude filters on the proto list
Compute cluster average utilization
Compute upper/lower utilization limits from cluster average and threshold
Compute each eligible node’s utilization (utilization = (capacity - remaining) / capacity) and classify each as source, target, or balanced by comparing with the upper/lower limits.
Accumulate source/target counts and totalOverUtilizedBytes/totalUnderUtilizedBytes
Compute MaxUtilization, MinUtilization
Compute cluster imbalance and bytes to move
Build and return ContainerBalancerClusterSnapshot
The logic should match the way the running balancer uses in ContainerBalancerTask.initializeIteration(), so CLI output and actual balancer behaviour stay consistent.

Analyzer should return an object(ContainerBalancerClusterSnapshot) containing totalEligibleDatanodes count, clusterAvgUtilization, clusterCapacityBytes, MaxUtilization, MinUtilization, upperLimit, lowerLimit, sourceCount, targetCount, totalOverUtilizedBytes, totalUnderUtilizedBytes, bytesToMove, Imbalance (drift), top 5 sourceNodes hostnames, bottom 5 targetNodes hostnames.

We should also test this analysis.

What is the link to the Apache JIRA

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

How was this patch tested?

Written Unit Tests.

@sravani-revuri

Copy link
Copy Markdown
Contributor Author

@ashishkumar50 , @sreejasahithi could you please review these changes?

@sreejasahithi sreejasahithi left a comment

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.

Thanks @sravani-revuri for this PR,
left some initial comments.

@sreejasahithi sreejasahithi left a comment

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.

Thanks @sravani-revuri for updating the patch

@sarvekshayr sarvekshayr left a comment

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.

Thanks @sravani-revuri for working on this.

0,
0,
thresholdRatio,
-thresholdRatio,

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.

upperLimit / lowerLimit are derived from cluster average:

upperLimit = clusterAvgUtilization + thresholdRatio
lowerLimit = clusterAvgUtilization - thresholdRatio

When filterEligibleNodes() returns empty, there is no cluster average, so those limits are undefined.

Consider 0 for the limits as well.

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.

This check is already done.

 if (eligible.isEmpty()) {
      return emptySnapshot(thresholdRatio);
    }

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.

That's correct.

My question is, do we need to set upperLimit to thresholdRatio and lowerLimit to -thresholdRatio for emptySnapshot?

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.

This is how it is currently in ContainerBalancerTask#initializeIteration

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.

I think the situation of no eligible nodes can be handled by the respective CLI commands using totalEligibleDatanodes for display. Although the limits are not used in this case and it would not cause harm to set them to 0, I think it is better to align with the existing balancer implementation.

Currently keeping the upperLimit/lowerLimit values as +/-thresholdRatio to match ContainerBalancerTask. If there are any issues or edge cases with this approach, please let me know and I’ll adjust accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants