HDDS-14662. Container Balancer does not validate include/exclude datanode names#9812
HDDS-14662. Container Balancer does not validate include/exclude datanode names#9812sravani-revuri wants to merge 9 commits intoapache:masterfrom
Conversation
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks for identifying and fixing this issue @sravani-revuri. Please add a test to verify the same.
...m/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
Gargi-jais11
left a comment
There was a problem hiding this comment.
Please find the inlined comments. Rest all looks good to me.
...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
Outdated
Show resolved
Hide resolved
...m/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @sravani-revuri for working on this, overall LGTM
just left few minor comments.
...erver-scm/src/main/java/org/apache/hadoop/hdds/scm/container/balancer/ContainerBalancer.java
Show resolved
Hide resolved
...m/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @sravani-revuri for updating.
...m/src/test/java/org/apache/hadoop/hdds/scm/container/balancer/TestContainerBalancerTask.java
Outdated
Show resolved
Hide resolved
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @sravani-revuri
LGTM
siddhantsangwan
left a comment
There was a problem hiding this comment.
From a code organisation point of view, it's better to test this in TestContainerBalancer.java as that's where all the tests for ContainerBalancer.java are. Some mocking for scm.getScmNodeManager().getNodesByAddress(node) using Mockito should help you in getting the test to work.
What changes were proposed in this pull request?
The ozone admin containerbalancer start command accepts invalid datanode names in -- include-datanodes- and --exclude-datanodes and reports success instead of failing with a validation error. This differs from other admin commands that validate hostnames and fail on invalid input.
Proposed Fix
Add validation for -- include-datanodes and -- exclude-datanodes before starting the balancer.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14662
How was this patch tested?
manual testing done.
Incorrect value given for include node:
Incorrect value given for exclude node:
Correct values given for exclude node:
Correct values given for include node: