HDDS-12810. Check and reserve space atomically in VolumeChoosingPolicy

[peterxcli]

What changes were proposed in this pull request?

If VolumeChoosingPolicy#chooseVolume is not synchronized and caller required space reserved before chooseVolume returns, there is no easy way to guarantee chosen volume will have enough space.

VolumeChoosingPolicy#chooseVolume is used for create container and container import. Make VolumeChoosingPolicy#chooseVolume synchronized or part of chooseVolume synchronized will add some latency, which is negligible for container import, may noticeable for create container. We can compare the create container latency metrics with and without the synchronization.

VolumeChoosingPolicy#chooseVolume, its Java Doc says "The implementations of this interface must be thread-safe.", regarding the space full check and space reservation, it can be done as an atomic operation in the chooseVolume internally, so that there will no over allocation of space due to concurrent container creation and container import.

Also need to add test to check if committedBytes is checked in container creation unit tests and container import, container replication unit tests. Besides, we also need the unit tests to verify that if the container creation fails, container import fails, or container replication fails, the resevered committedBytes are released.

What is the link to the Apache JIRA

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

How was this patch tested?

Added coverage of commit space would be reserved/released correctly when creating/importing containers in TestKeyValueContainer, TestDownloadAndImportReplicator and TestSendContainerRequestHandler. There're also some existing tests covering this change, like TestContainerPerisistance and TestOzoneContainer.

CI:
https://github.com/peterxcli/ozone/actions/runs/14911933939
https://github.com/peterxcli/ozone/actions/runs/15001277972

[peterxcli]

I noticed that the push/pull replicator reserves twice the container size retrieved from the local configuration (OZONE_SCM_CONTAINER_SIZE). I was wondering if it might be more appropriate to use the maximum size of the replicated container instead, since the container size config may vary across different nodes.

@ChenSammi, what are your thoughts on this approach? Does it seem reasonable to you?

[siddhantsangwan]

I was wondering if it might be more appropriate to use the maximum size of the replicated container instead

Good point to use the actual container size instead, but we don't know what the actual size is (when committing space) before actually downloading the container, right?

[siddhantsangwan]

@peterxcli Thanks for taking this up, it's a good fix. I've left a few review comments.

[peterxcli]

Good point to use the actual container size instead, but we don't know what the actual size is (when committing space) before actually downloading the container, right?

Right, so I think that info should be brought in proto message.

[peterxcli]

@siddhantsangwan @ChenSammi I have addressed all review comments, please take another look. Thanks!

[ChenSammi]

Good point to use the actual container size instead, but we don't know what the actual size is (when committing space) before actually downloading the container, right?

Right, so I think that info should be brought in proto message.

Agree. We can do this in a new JIRA. Generally, the tar zip file of container will have smaller size than it's real size, so reserve a 2* max container size is conservative enough in most cases. But there are container over-allocated case, where container size can be double or triple the max container size as @siddhantsangwan saw in user's environment, have a accurate container size can handle this case welly.

[ChenSammi]

1. please move this newContainerData.releaseCommitSpace() into the exception handling directly.
2. we might first check whether space is committed before release, for newContainer.create can throw out exception from volumeChoosingPolicy.chooseVolume too.

[peterxcli]

2. we might first check whether space is committed before release, for newContainer.create can throw out exception from volumeChoosingPolicy.chooseVolume too.

releaseCommitSpace itself already checked that:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java

Lines 359 to 361 in 57f254d

	if (unused > 0 && committedSpace) {
	getVolume().incCommittedBytes(0 - unused);
	}

[ChenSammi]

Okay.

[peterxcli]

Agree. We can do this in a new JIRA. Generally, the tar zip file of container will have smaller size than it's real size, so reserve a 2* max container size is conservative enough in most cases. But there are container over-allocated case, where container size can be double or triple the max container size as @siddhantsangwan saw in user's environment, have a accurate container size can handle this case welly.

Created one. https://issues.apache.org/jira/browse/HDDS-12998

Also thanks you for spending time to run test and find ways to make the benchmark data better!

[jojochuang]

i'm curious why capacity-based is several times slower than round-robin.
But regardless, 12 thousand CreateContainer per second is more than enough.

[peterxcli]

i'm curious why capacity-based is several times slower than round-robin. But regardless, 12 thousand CreateContainer per second is more than enough.

It seems that these two lines are the cause:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/CapacityVolumeChoosingPolicy.java

Lines 58 to 60 in f52069b

	List<HddsVolume> volumesWithEnoughSpace = volumes.stream()
	.filter(filter)
	.collect(Collectors.toList());

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/CapacityVolumeChoosingPolicy.java

Lines 82 to 83 in f52069b

	int firstIndex = ThreadLocalRandom.current().nextInt(count);
	int secondIndex = ThreadLocalRandom.current().nextInt(count);

Round-robin doesn't iterate or use randomness — in an ideal case, each call should just run one AvailableSpaceFilter test and return volumeSet[i++].

[peterxcli]

Final benchmark result(NUM_VOLUMES=10, NUM_THREADS=100, NUM_ITERATIONS = 100000):
Here’s the data in Markdown table format for both Average Latency (ns) and Operations Per Second (ops):

Table

• Average Latency (ns):

Policy	Run1	Run2	Run3	Run4	Run5
Capacity - with	100795.44	100943.33	105477.20	108602.40	106464.37
Capacity - without	320108.34	321287.44	316904.45	312877.90	314582.18
RoundRobin - with	18258.44	15234.60	15403.40	14317.04	13702.91
RoundRobin - without	36139.49	40500.70	38969.58	40013.86	39282.06

• Operations Per Second (ops):

Policy	Run1	Run2	Run3	Run4	Run5
Capacity - with	9921.08	9906.55	9480.72	9207.90	9392.81
Capacity - without	3123.94	3112.48	3155.53	3196.13	3178.82
RoundRobin - with	54769.18	65640.04	64920.74	69846.84	72977.20
RoundRobin - without	27670.56	24690.93	25661.04	24991.34	25456.91

Charts

cc @ChenSammi

[ChenSammi]

@peterxcli , the data looks same as the last one I shared. Are you getting the same data ?

[peterxcli]

@ChenSammi I was asking gpt to generate the table and result for me, and I give the previous table image you shared as format example, and I didn't notice it use the data in it directly😥. Sorry I didn't check it.

BTW, I updated the test result, and I have no idea why synced choosing policies are performing better than no sync. Maybe f52069b really helped.

[sumitagrawl]

@peterxcli Thanks for working over this, having few minor comments

[ChenSammi]

@ChenSammi I was asking gpt to generate the table and result for me, and I give the previous table image you shared as format example, and I didn't notice it use the data in it directly😥. Sorry I didn't check it.

BTW, I updated the test result, and I have no idea why synced choosing policies are performing better than no sync. Maybe f52069b really helped.

Yea, the optimization effect is really good. Here is the data I collected for 100000000 operations.

[ChenSammi]

The last patch LGTM. Thanks @peterxcli . @siddhantsangwan , @sumitagrawl , would you like to take another look?

[sumitagrawl]

@ChenSammi One comment is pending to be fixed, after that is ok to be merged.

[peterxcli]

@ChenSammi This part is pending, need move this logic at line 162 just after chooseVolume, so that in future if any code is added after that which may throw exception, it does not induce issue of not releasing commitedSpace in exception handler, as release depends on this flag to be "true"

https://github.com/apache/ozone/pull/8360/files#diff-c1cf18e174dcc55efd5f1177d6ea8e916353d2a59681388b225e97c512d9b0baL161:~:text=containerVolume-,%3D,-volumeChoosingPolicy.chooseVolume

Thanks for catching this! Done.

[sumitagrawl]

LGTM

[peterxcli]

Thanks @ChenSammi for reviewing, optimizing and running the benchmark for this patch! And thanks @siddhantsangwan, @sumitagrawl for the review!