Fix for bug showing incorrect awareness attributes count in AwarenessAllocationDecider

[anshu1106]

…Decider

This commit fixes allocation explanation reason displaying incorrect number of awareness attributes in some cases when forced zone awareness is enabled.

Signed-off-by: Anshu Agarwal [email protected]

Description

This commit fixes bug in AwarenessAllocationDecider showing incorrect number of awareness attribute in allocation explanation reason. More details on the bug and how to reproduce it can be found in the issue.

Issues Resolved

#3413

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

❌   Gradle Check failure a60a4ac86714bf4b44f786ead2ade511efdda725
Log 5536

Reports 5536

[opensearch-ci-bot]

❌   Gradle Check failure 22f4a3432e1077d80f7123ac30bbf5aea4ecbdc4
Log 5537

Reports 5537

[reta]

Please revert the change to use *, the concrete types are preferred.

[Bukhtawar]

+1

[anshu1106]

Will take care of this.

[reta]

Could you please explain this change? AFAIK, the allocation is about shards, not nodes, as such usage of shardPerAttribute is about to be right (to me).

[Bukhtawar]

Can we simplify the logic to reflect

ceil(count(union(discovered_zone, force_zone))/total_shard_copies)

[anshu1106]

Could you please explain this change? AFAIK, the allocation is about shards, not nodes, as such usage of shardPerAttribute is about to be right (to me).

@reta Yeah, thats true. But this implementation is showing incorrect attribute count in some cases when forced awareness is enabled for the domain which I mentioned in the issue. I have created a unit test which fails as well with incorrect awareness attribute count.

`

        int numberOfAttributes = nodesPerAttribute.size();
        List<String> fullValues = forcedAwarenessAttributes.get(awarenessAttribute);
        if (fullValues != null) {
            for (String fullValue : fullValues) {
                if (shardPerAttribute.containsKey(fullValue) == false) {
                    numberOfAttributes++;
                }
            }
        }

`

In code snippet, we are first calculating number of awareness attributes using discovered zones. Assume the domain has 3 zones but one zone (zone-c) is completely down, then nodesPerAttribute.size() returns 2.
Assume forced awareness value is set to "zone-a, zone-b, zone-c". So this returns three. ie fullValues =3.

Now assume a node came up in zone-c which had outage earlier. But no shards has been assigned to that node till now. So in this case shardPerAttribute.size()=2 since there are no shards assigned to any node in zone-c but nodesPerAttribute.size()=3. Now when we go inside inner for loop, shardPerAttribute won't have key for zone-c and it increases numberOfAttribute count to 4 which is incorrect.(ie zone-c has been counted twice)
So basically numAttributes = distinct(union(discovered_zone, forced_zone))

[anshu1106]

Can we simplify the logic to reflect

ceil(count(union(discovered_zone, force_zone))/total_shard_copies)

Sure will try to make the logic simpler.

[reta]

I think it makes sense now, thank @anshu1106 !

[nknize]

Rebase from main.

[opensearch-ci-bot]

❌   Gradle Check failure 8180f07b011be0141ceafac4934248239ebed0d8
Log 5846

Reports 5846

[opensearch-ci-bot]

❌   Gradle Check failure 7b2eedc83f557e1a316c22470b42723c6ac99269
Log 5847

Reports 5847

[opensearch-ci-bot]

✅   Gradle Check success f27d1e6993afb591278b0d9ba0c7f6fde189bd65
Log 5850

Reports 5850

[Bukhtawar]

Suggested change

	attributesSet.addAll(forcedAwarenessAttributes.get(awarenessAttribute));
	attributesSet.addAll(fullValues);

[Bukhtawar]

Lets modify this accordingly

[Bukhtawar]

Why do we need to have only a single decider here?

[anshu1106]

This test is to check allocation decision by AwarenessAllocationDecider Line no -1053. Wanted the test to be more predictable, thats why didn't introduce all the deciders.

[Bukhtawar]

start gradle check

[opensearch-ci-bot]

❌   Gradle Check failure f27d1e6993afb591278b0d9ba0c7f6fde189bd65
Log 5921

Reports 5921

[opensearch-ci-bot]

✅   Gradle Check success 3d5bff2
Log 5971

Reports 5971

Comments addressed