HDDS-14926. Allow QUASI_CLOSED containers in DiskBalancer with improved debug logs for containers

[Gargi-jais11]

What changes were proposed in this pull request?

Currently DiskBalancer only supports CLOSED containers to be moved but if user wants to also move quasi closed containers then we should support that as well.

1st Improvement:
Below scenario can happen in real, so Disk balancer can only attempt those 10 CLOSED containers. Even if it moves all 10 successfully, disk A might drop only from 90% → 85% — nowhere near balance. The QUASI_CLOSED containers are the real bulk occupying the space and disk balancer is completely blind to them.

Disk A: 90% utilized 
├── 600 containers: QUASI_CLOSED 
└── 10 containers: CLOSED 
Disk B: 10% utilized 
Disk C: 11% utilized

Added a config hdds.datanode.disk.balancer.container.states default=CLOSED,QUASI_CLOSED which indicate the container lifecycle state names (CLOSED, QUASI_CLOSED) that are eligible to be moved between disks on a datanode.
Extended this to a CLI option for start or update config command so there is no need to restart dn after updating the property.

2nd Improvement:
We need to add debug logs for chooseContainer method as user might not understand why if they have over and under utilised volume still container is not moved. This parts needs more clarification. Because I see in escalation it helped a lot with balancer debug logs for container not choose to identify what state the container or volume was in.
I suggest adding these logs for container not choose :

// UsedBytes less than 0
 LOG.debug("Skipping container {} from volume {}: bytes used is {}", containerId, src.getStorageDir().getPath(), containerData.getBytesUsed());
              
//  Skip containers already in progress
LOG.debug("Skipping container {} from volume {}: disk balancer move already in progress", containerId, src.getStorageDir().getPath());

// only closed and quasi closed containers should be moved
LOG.debug("Skipping container {} from volume {}: state is {}. Requires {}", containerId, src.getStorageDir().getPath(), containerData.getState(), phase.getEligibilityCriteria());

// skipping container move as its size is more than destination available space.
LOG.debug("Skipping container {} ({}B) from volume {}: exceeds destination {} usable space {}B, containerId, containerSize, src.getStorageDir().getPath(), dst.getStorageDir().getPath(), usableSpace);

// skipping container move as it will make dest more utilised after movement.
LOG.debug("Skipping container {} ({}B) from volume {}: moving to {} would  result in utilization {} exceeding upper threshold {}", containerId, containerSize, src.getStorageDir().getPath(), dst.getStorageDir().getPath(),

What is the link to the Apache JIRA

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

How was this patch tested?

Added unit test.

Tested manually as well for CLI:

bash-5.1$ ozone admin datanode diskbalancer update -c CLSD --in-service-datanodes
Error on node [172.18.0.9:19864]: Invalid container state 'CLSD' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.2:19864]: Invalid container state 'CLSD' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.8:19864]: Invalid container state 'CLSD' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Failed to update DiskBalancer configuration on nodes: [ozone-datanode-2.ozone_default (172.18.0.9:19864), ozone-datanode-3.ozone_default (172.18.0.2:19864), ozone-datanode-1.ozone_default (172.18.0.8:19864)]
bash-5.1$ ozone admin datanode diskbalancer status --in-service-datanodes
Status result:
Datanode                                                     Status       Threshold(%)    BandwidthInMB   Threads      StopAfterDiskEven    ContainerStates                          SuccessMove  FailureMove  BytesMoved(MB)  EstBytesToMove(MB) EstTimeLeft(min)    
ozone-datanode-2.ozone_default (172.18.0.9:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   
ozone-datanode-3.ozone_default (172.18.0.2:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   
ozone-datanode-1.ozone_default (172.18.0.8:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   

Note:
 - EstBytesToMove is calculated based on the target disk even state with the configured threshold.
 - EstTimeLeft is calculated based on EstimatedBytesToMove and configured disk bandwidth.
 - Both EstimatedBytes and EstTimeLeft could be non-zero while no containers can be moved, especially when the configured threshold or disk capacity is too small. 
 - ContainerStates indicate the state of container which is eligible for balancing, by default CLOSED and QUASI_CLOSED are allowed.
 
bash-5.1$ ozone admin datanode diskbalancer update -c CLOSED,NOT_A_STATE --in-service-datanodes
Error on node [172.18.0.9:19864]: Invalid container state 'NOT_A_STATE' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.2:19864]: Invalid container state 'NOT_A_STATE' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.8:19864]: Invalid container state 'NOT_A_STATE' in hdds.datanode.disk.balancer.container.states. Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Failed to update DiskBalancer configuration on nodes: [ozone-datanode-2.ozone_default (172.18.0.9:19864), ozone-datanode-3.ozone_default (172.18.0.2:19864), ozone-datanode-1.ozone_default (172.18.0.8:19864)]

bash-5.1$ ozone admin datanode diskbalancer status --in-service-datanodes
Status result:
Datanode                                                     Status       Threshold(%)    BandwidthInMB   Threads      StopAfterDiskEven    ContainerStates                          SuccessMove  FailureMove  BytesMoved(MB)  EstBytesToMove(MB) EstTimeLeft(min)    
ozone-datanode-2.ozone_default (172.18.0.9:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   
ozone-datanode-3.ozone_default (172.18.0.2:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   
ozone-datanode-1.ozone_default (172.18.0.8:19864)            STOPPED      10.0000         10              5            true                 CLOSED,QUASI_CLOSED                      0            0            0               0                  0                   

Note:
 - EstBytesToMove is calculated based on the target disk even state with the configured threshold.
 - EstTimeLeft is calculated based on EstimatedBytesToMove and configured disk bandwidth.
 - Both EstimatedBytes and EstTimeLeft could be non-zero while no containers can be moved, especially when the configured threshold or disk capacity is too small. - ContainerStates indicate the state of container which is eligible for balancing, by default CLOSED and QUASI_CLOSED are allowed.
 
bash-5.1$ ozone admin datanode diskbalancer update -c closed --in-service-datanodes
Error on node [172.18.0.9:19864]: Invalid container state 'closed': use uppercase enum names (e.g. CLOSED, QUASI_CLOSED). Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.2:19864]: Invalid container state 'closed': use uppercase enum names (e.g. CLOSED, QUASI_CLOSED). Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Error on node [172.18.0.8:19864]: Invalid container state 'closed': use uppercase enum names (e.g. CLOSED, QUASI_CLOSED). Valid names are: [OPEN, CLOSING, QUASI_CLOSED, CLOSED, UNHEALTHY, INVALID, DELETED, RECOVERING]
Failed to update DiskBalancer configuration on nodes: [ozone-datanode-2.ozone_default (172.18.0.9:19864), ozone-datanode-3.ozone_default (172.18.0.2:19864), ozone-datanode-1.ozone_default (172.18.0.8:19864)]

bash-5.1$ ozone admin datanode diskbalancer update -c CLOSED --in-service-datanodes
Updated DiskBalancer configuration on all IN_SERVICE nodes.

bash-5.1$ ozone admin datanode diskbalancer start --in-service-datanodes
Started DiskBalancer on all IN_SERVICE nodes.
bash-5.1$ ozone admin datanode diskbalancer status --in-service-datanodes
Status result:
Datanode                                                     Status       Threshold(%)    BandwidthInMB   Threads      StopAfterDiskEven    ContainerStates                          SuccessMove  FailureMove  BytesMoved(MB)  EstBytesToMove(MB) EstTimeLeft(min)    
ozone-datanode-2.ozone_default (172.18.0.9:19864)            RUNNING      10.0000         10              5            true                 CLOSED                                   0            0            0               0                  0                   
ozone-datanode-3.ozone_default (172.18.0.2:19864)            RUNNING      10.0000         10              5            true                 CLOSED                                   0            0            0               0                  0                   
ozone-datanode-1.ozone_default (172.18.0.8:19864)            RUNNING      10.0000         10              5            true                 CLOSED                                   0            0            0               0                  0                   

Note:
 - EstBytesToMove is calculated based on the target disk even state with the configured threshold.
 - EstTimeLeft is calculated based on EstimatedBytesToMove and configured disk bandwidth.
 - Both EstimatedBytes and EstTimeLeft could be non-zero while no containers can be moved, especially when the configured threshold or disk capacity is too small. - ContainerStates indicate the state of container which is eligible for balancing, by default CLOSED and QUASI_CLOSED are allowed.

[Gargi-jais11]

@ChenSammi Please review the PR.

[errose28]

Added a config hdds.datanode.disk.balancer.include.non.standard.containers default=false. If true, balancer include non-standard states, i.e, QUASI_CLOSED. So both CLOSED and QUASI_CLOSED state containers are eligible for move. If false (default), balancer only moves CLOSED containers.

I think we should remove the config and have all containers that would otherwise be eligible for replication between datanodes (non-open containers) eligible for disk balancing. Since they can be safely copied between datanodes there is no reason to exclude them when copying between disks as well.

[Gargi-jais11]

I think we should remove the config and have all containers that would otherwise be eligible for replication between datanodes (non-open containers) eligible for disk balancing. Since they can be safely copied between datanodes there is no reason to exclude them when copying between disks as well.

Thanks for the suggestion — the idea that anything safe to replicate between datanodes should be safe to move between disks on the same node is reasonable.
But the config was added hdds.datanode.disk.balancer.include.non-standard.containers (default false) to stay aligned with container balancer (in this PR): there, non-standard states such as QUASI_CLOSED are only considered when the corresponding include flag is set. Intially, I and @ChenSammi had the opinion of moving QUASI_CLOSED containers regardless of any config setting. But after discussion we decided to align with container Balancer.

[errose28]

#9964 largely applies to over-replicated containers. It is required because the balancer will create more over-replication as it runs if deletes are not processed. The disk balancer does not create over-replication because a datanode can only load one container at a time, and furthermore the datanodes themselves are not aware of whether a container is over-replicated. The only overlap between the two is handling quasi-closed containers. For quasi-closed containers I don't see a reason for either process to exclude them, and I'm not sure #9964 made the right call here. I can't find any documented reason why quasi-closed containers should be excluded from either process, only jira descriptions saying it will be done with no supporting reasoning.

@siddhantsangwan @sarvekshayr @sadanand48 what do you think?

[sadanand48]

I can't find any documented reason why quasi-closed containers should be excluded from either process.

Same here, Theoretically I think we could skip excluding quasi-closed containers however my guess based on some discussions is that we didn't want cases where same container is acted upon by both balancer and RM, Say balancer schedules a move for a quasi-closed container and RM sends a command to close the container at the same time.

While balancer moves the QC container, If we don't do the bcsId check it can increase the containers that have lower bcsId by the balancer selecting it and delete not processing in the system. Still should not be a major issue. I think we just wanted to play safe by only moving immutable container state. If we have tested these scenarios I believe we can include these in the default criteria

The origin of the check is in https://github.com/apache/ozone/pull/2441/changes#diff-b0ac1e8086a1da3247489a1a6ad742996052aa17a46544f1f6aaf24bfd92a762R76-R82

@siddhantsangwan please correct me if I am wrong

[Gargi-jais11]

While balancer moves the QC container, If we don't do the bcsId check it can increase the containers that have lower bcsId by the balancer selecting it and delete not processing in the system.

@sadanand48 What I know is that BCSID increases when new block commits go through the container pipeline (Ratis log index on PutBlock). Balancers relocate bytes and metadata; they should preserve BCSID.
Neither Disk Balancer nor Container Balancer is meant to raise BCSID when a container is moved. They copy the existing replica , so the moved replica should keep the same BCSID as the source and stay aligned with SCM’s container sequence id—same as any other replication or import of a closed/quasi-closed replica.
So I don't think even after moving quasi closed containers there will be any issue related to processing the deletion of quasi clsoed containers. It will be deleted correctly by RM with new updated location, if I am not wrong here in my understanding.

I think we allow quasi closed containers to be moved without any config unless we want to play safe.
Please share you opinions.
cc: @ChenSammi @errose28 @ashishkumar50

[sadanand48]

Neither Disk Balancer nor Container Balancer is meant to raise BCSID when a container is moved

Right my comment above does not state that it alters bcsID , but it can increase the replicas with lower bcsId by replicating them (given the delete fails in the move). As I stated above, if all such scenarios are tested thoroughly , IMO we can bring the quasi-closed containers in the default criteria for selection

[alielhaj023]

Approved?

[Copilot]

Pull request overview

This PR extends the datanode DiskBalancer to move not only CLOSED containers but also additional lifecycle states (notably QUASI_CLOSED) via a new configurable “movable container states” setting, and improves debuggability by adding detailed skip-reason debug logs during container selection. It also wires the new setting end-to-end (CLI → protobuf → persisted YAML → service runtime) and updates docs/tests accordingly.

Changes:

• Add a new config property to control which container lifecycle states are eligible for disk balancing, and propagate it through DiskBalancer service state/reporting.
• Update container selection to filter by the configured movable states and add granular debug logs for skip conditions.
• Extend CLI/status output, protobuf, YAML persistence, and tests/docs to include the new containerStates setting.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/node/TestDiskBalancerPolicyPerformance.java	Updates performance tests to pass the new movable-states parameter.
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/datanode/TestDiskBalancerSubCommands.java	Adds CLI test for invalid container state reporting during update.
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerUpdateSubcommand.java	Adds --container-states option and includes it in update config proto/output.
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStatusSubcommand.java	Displays configured container states in status output and JSON.
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerStartSubcommand.java	Adds --container-states option to start; wires to config proto/output.
hadoop-hdds/interface-client/src/main/proto/hdds.proto	Adds containerStates field to DiskBalancerConfigurationProto.
hadoop-hdds/docs/content/feature/DiskBalancer.md	Documents the new hdds.datanode.disk.balancer.container.states setting.
hadoop-hdds/docs/content/feature/DiskBalancer.zh.md	Chinese documentation update for the new container-states config.
hadoop-hdds/docs/content/design/diskbalancer.md	Updates design doc to reflect state-driven movability (CLOSED/QUASI_CLOSED default).
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/ContainerChoosingPolicy.java	Extends the policy interface to accept movable container states.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/policy/DefaultContainerChoosingPolicy.java	Implements state filtering + detailed debug skip logs; updates method signature.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerConfiguration.java	Introduces container.states config, default states, parsing/validation helper.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerInfo.java	Persists containerStates as part of DiskBalancerInfo equality/state.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerYaml.java	Adds containerStates to YAML read/write with backward-compatible defaulting.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java	Tracks/persists container states and enforces movability both at choose-time and move-time.
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerProtocolServer.java	Returns containerStates in the disk balancer info RPC/status proto.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerYaml.java	Parameterizes YAML round-trip tests to include container states.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerTask.java	Removes prior “test mode” gating for QUASI_CLOSED; aligns tests with new config-based eligibility.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerService.java	Adds unit tests for parsing/validation of movable container states.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerProtocolServer.java	Updates protocol server tests to verify containerStates propagation.
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDefaultContainerChoosingPolicy.java	Adds tests validating QUASI_CLOSED eligibility based on configured state list.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ChenSammi]

Please make it private, call it from setContainerStates. We should verify the new setting value in setContainerStates.

[Gargi-jais11]

Done.

[Gargi-jais11]

Done verifying the new setting value in setContainerStates.
In order to make getMovableContainerStates private and only use it from setContainerStates. That only works if every time the config is loaded, setContainerStates runs.

It doesn’t: for OzoneConfiguration.getObject(DiskBalancerConfiguration.class), the framework binds hdds.datanode.disk.balancer.container.states by writing the containerStates field directly and setContainerStates is not invoked.
So code like DefaultContainerChoosingPolicy, DiskBalancerService, tests, etc. still need a public way to get Set from that object. Today that’s getMovableContainerStates(), which parses the injected string on read.

If we made that method private and only called it from setContainerStates, those getObject(...) instances would never get a parsed set.

[ChenSammi]

Can we do the parseMovableContainerStates in the constructor?

[Gargi-jais11]

As far as I understand the contsructor cannot validate because @Config fields are injected via reflection after the constructor returns (field.set(obj, value) in injectConfiguration). The constructor only ever sees the default value of the field not the configured value. So we cannot move parseMovableContainerStates in constructor.

[Gargi-jais11]

@ChenSammi Please take a look, I have updated the patch.

[ChenSammi]

The last patch LGTM, +1.

Thanks @Gargi-jais11 , and @errose28 @sadanand48 for the discussion.