HDDS-6808. EC: Fix datanode exclusion check in client

[kaijchen]

What changes were proposed in this pull request?

We should call HddsClientUtils.checkForException before calling checkIfContainerToExclude.

This bug was introduced in HDDS-6400 (#3372).

What is the link to the Apache JIRA

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

How was this patch tested?

• With the patch: https://github.com/kaijchen/ozone/runs/6650562194?check_suite_focus=true
• Without the patch: https://github.com/kaijchen/ozone/runs/6650731778?check_suite_focus=true#step:5:1649

[kaijchen]

@guihecheng PTAL.

[umamaheswararao]

How did this issue triggered in your case? I am wondering if there is a chance to add some coverage

[kaijchen]

How did this issue triggered in your case? I am wondering if there is a chance to add some coverage

I found it by writing some very large key (40 GB) in a real cluster, and to many datanodes are excluded at some point.

ozone freon ockg -n 100 --type EC --replication rs-10-4-1024k --size 4000000000

Maybe we can mock DN to return ContainerNotOpenException to test this?
The change here is "If caused by container not open, don't exclude datanode".

[umamaheswararao]

Got it, thats a good idea. We have mock datanode ( MockDatanodeStorage). We used it for failure testing. Ex: see TestOzoneECClient#testNodeFailuresWhileWriting

[kaijchen]

Hi @umamaheswararao, I have added the test to detect this problem.
There is still some undeterministic code because the pipeline chosen for write is unpredictable.
Do you have any better idea to improve it?

[umamaheswararao]

Anyway we are marking nodes failed after key create and and getting the nodes which are part of pipeline. MultiNodePipelineBlockAllocator#allocateBlock is more predictable node selection. So, we will choose first required nodes for first allocate block and then next remaining nodes in order for next allocateBlock. So, by default we initialize this class with 15 nodes in cluster. So, if we just create one key with one block in it, it should always choose first required nodes.

Probably you can create your own client like this test doing, so that other tests would not have used that nodes.
Ex:

public void testStripeWriteRetriesOnFailures(OzoneConfiguration con,
      int clusterSize, int[] nodesIndexesToMarkFailure) throws IOException {
    close();
    MultiNodePipelineBlockAllocator blkAllocator =
        new MultiNodePipelineBlockAllocator(con, dataBlocks + parityBlocks,
            clusterSize);
    createNewClient(con, blkAllocator);

[umamaheswararao]

This test using default initialized Allocator. One we marked some nodes failed, they may be reflected to other tests.
It would be good to have own allocator and client created for the failure related tests, so that state would not interfere with others.

[kaijchen]

@umamaheswararao Thanks for the advice, I have tested each of these 100x and it seems stable.

[umamaheswararao]

Thanks for verifying.
Latest changes LGTM.

[umamaheswararao]

Pending CI.

[kaijchen]

Thanks @umamaheswararao for the review.