Add validation in Decommission Request for minimum awareness attributes #4767

imRishN · 2022-10-13T07:30:58Z

Description

This PR adds another layer of validation before executing decommission request. The decommission request hence would be acknowledged only if there are atleast 3 awareness attribute value set to the cluster to fail fast

Issues Resolved

#4768

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
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-13T08:00:09Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4560/
CommitID: 0930ba0

github-actions · 2022-10-13T08:03:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4562/
CommitID: a020121

Signed-off-by: Rishab Nahata <[email protected]>

…tion

github-actions · 2022-10-13T12:53:51Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4572/
CommitID: 647389a

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-13T14:34:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4585/
CommitID: b90ffbb

codecov-commenter · 2022-10-13T14:36:27Z

Codecov Report

Merging #4767 (24d0638) into main (8fda187) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@             Coverage Diff              @@
##               main    #4767      +/-   ##
============================================
- Coverage     70.82%   70.80%   -0.03%     
+ Complexity    57885    57855      -30     
============================================
  Files          4689     4689              
  Lines        276913   276914       +1     
  Branches      40301    40303       +2     
============================================
- Hits         196122   196063      -59     
+ Misses        64548    64490      -58     
- Partials      16243    16361     +118

Impacted Files	Coverage Δ
...r/decommission/DecommissioningFailedException.java	`38.46% <50.00%> (+5.12%)`	⬆️
...arch/cluster/decommission/DecommissionService.java	`32.90% <61.53%> (+0.86%)`	⬆️
...ava/org/opensearch/action/NoSuchNodeException.java	`0.00% <0.00%> (-50.00%)`	⬇️
...cluster/coordination/PendingClusterStateStats.java	`20.00% <0.00%> (-48.00%)`	⬇️
...regations/metrics/AbstractHyperLogLogPlusPlus.java	`51.72% <0.00%> (-44.83%)`	⬇️
...n/admin/indices/readonly/AddIndexBlockRequest.java	`35.71% <0.00%> (-35.72%)`	⬇️
...ion/admin/cluster/node/info/PluginsAndModules.java	`53.12% <0.00%> (-34.38%)`	⬇️
...search/search/aggregations/pipeline/EwmaModel.java	`24.44% <0.00%> (-33.34%)`	⬇️
...nsearch/index/shard/IndexShardClosedException.java	`66.66% <0.00%> (-33.34%)`	⬇️
...min/cluster/snapshots/get/GetSnapshotsRequest.java	`52.63% <0.00%> (-31.58%)`	⬇️
... and 459 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gbbafna · 2022-10-14T04:59:51Z

server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java

        if (awarenessAttributes == null) {
-            msg = "awareness attribute not set to the cluster.";
+            msg = "awareness attribute not set to the cluster";
        } else if (forcedAwarenessAttributes == null) {
-            msg = "forced awareness attribute not set to the cluster.";
+            msg = "forced awareness attribute not set to the cluster";
        } else if (awarenessAttributes.contains(decommissionAttribute.attributeName()) == false) {
-            msg = "invalid awareness attribute requested for decommissioning";
+            msg = "invalid awareness attribute requested for decommissioning, eligible attributes are ["
+                + awarenessAttributes.toString()
+                + "]";
        } else if (forcedAwarenessAttributes.containsKey(decommissionAttribute.attributeName()) == false) {
            msg = "forced awareness attribute [" + forcedAwarenessAttributes.toString() + "] doesn't have the decommissioning attribute";
        } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName())
            .contains(decommissionAttribute.attributeValue()) == false) {
-                msg = "invalid awareness attribute value requested for decommissioning. Set forced awareness values before to decommission";
+                msg = "invalid awareness attribute value requested for decommissioning. Eligible forced awareness attributes ["
+                    + forcedAwarenessAttributes.toString()
+                    + "]";
+            } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size() < 3) {
+                msg = "total awareness attribute value set to cluster is ["
+                    + forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size()
+                    + "] which is less than minimum attribute value count required [3]";


Rather than so many if..else if , this can be simplified with asserting just the expected values. If not we throw generic validation exception .

I agree that it can be simplified to a single if statement or two, but the disadvantage of it would be not exposing a verbose exception to the end user which would just leave the user wondering what went wrong with the request.

Simplified the validations a bit now

Signed-off-by: Rishab Nahata <[email protected]>

imRishN · 2022-10-14T05:49:06Z

server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java

+
+        if (awarenessAttributes == null
+            || forcedAwarenessAttributes == null
+            || awarenessAttributes.isEmpty()
+            || forcedAwarenessAttributes.isEmpty()) {
+            msg = "awareness attribute ["
+                + awarenessAttributes
+                + "] and forced awareness attribute ["
+                + forcedAwarenessAttributes
+                + "] must be set to execute decommissioning";
+        } else if (awarenessAttributes.contains(decommissionAttribute.attributeName()) == false
+            || forcedAwarenessAttributes.containsKey(decommissionAttribute.attributeName()) == false) {
+                msg = "invalid awareness attribute requested for decommissioning, eligible attributes are ["
+                    + forcedAwarenessAttributes
+                    + "]";
+            } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName())
+                .contains(decommissionAttribute.attributeValue()) == false) {
+                    msg = "invalid awareness attribute value requested for decommissioning. Eligible forced awareness attributes ["
+                        + forcedAwarenessAttributes
+                        + "]";
+                } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size() < 3) {
+                    msg = "total awareness attribute value set to cluster is ["
+                        + forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size()
+                        + "] which is less than minimum attribute value count required [3]";
+                }


Not sure why the spotless check is messing up the indentation in the same level of if/else block

gbbafna

code LTGM . Thanks Rishabh.

github-actions · 2022-10-14T06:17:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4683/
CommitID: a4a31f5

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-14T08:29:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: ABORTED ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4692/
CommitID: 9f213cd

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-15T12:53:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4738/
CommitID: dc780a3

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-16T09:54:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4759/
CommitID: ff33c64

github-actions · 2022-10-16T11:37:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4762/
CommitID: 59eedb6

Bukhtawar · 2022-10-16T16:27:46Z

server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java

+            } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName())
+                .contains(decommissionAttribute.attributeValue()) == false) {
+                    msg = "invalid awareness attribute value requested for decommissioning. Eligible forced awareness attributes ["
+                        + forcedAwarenessAttributes
+                        + "]";
+                } else if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size() < 3) {
+                    msg = "total awareness attribute value set to cluster is ["
+                        + forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size()
+                        + "] which is less than minimum attribute value count required [3]";
+                }


Lets say force zone awareness is set to AZ1, AZ2, AZ3 while actual discovered zone values are only

AZ1, AZ4, AZ5

AZ1 only

will decommission on AZ1 be supported?

I think force zone awareness values is the only source of truth today to know the actual zone values. If the user has set the zones to AZ1, AZ2, AZ3 would it be wrong to assume that these are the actual zone values? And force zone values is a dynamically updatable setting which is set by the user and defaults to empty.

Another point here is, if the zone values that are set by the user is AZ1,AZ2,AZ3 and user requests to decommission to AZ4, these checks would anyways fail and hence decommission would fail

…tion Signed-off-by: Rishab Nahata <[email protected]>

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-21T05:30:36Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5144/
CommitID: e3a9d04

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-21T08:27:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5154/
CommitID: 856ba74

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-21T09:28:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5161/
CommitID: 1a58068

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-21T12:24:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5172/
CommitID: f15180a

…tion Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-25T09:59:37Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5506/
CommitID: 24d0638

github-actions · 2022-10-26T05:40:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5638/
CommitID: fc72676
Please examine the workflow log, locate, and copy-paste the failure below.
Flakey test?
Iterate to green.

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-26T06:20:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5642/
CommitID: 1c58628
Please examine the workflow log, locate, and copy-paste the failure below.
Flakey test?
Iterate to green.

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-26T07:19:05Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5652/
CommitID: aa6e732

Signed-off-by: Rishab Nahata <[email protected]>

github-actions · 2022-10-26T07:40:57Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/5654/
CommitID: a46550a
Please examine the workflow log, locate, and copy-paste the failure below.
Flakey test?
Iterate to green.

github-actions · 2022-10-26T07:41:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5653/
CommitID: 3a97f68

github-actions · 2022-10-26T07:58:14Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/5657/
CommitID: 0701ce4

Bukhtawar · 2022-10-26T14:38:33Z

server/src/main/java/org/opensearch/cluster/decommission/DecommissionService.java

+                    + "]"
+            );
+        }
+        if (forcedAwarenessAttributes.get(decommissionAttribute.attributeName()).size() < 3) {


should this be a configuration?

Didn't get you. Configuration as in?

imRishN added 2 commits October 13, 2022 11:55

Add validation for attribute value count

362443a

Signed-off-by: Rishab Nahata <[email protected]>

Update failure msg

0930ba0

Signed-off-by: Rishab Nahata <[email protected]>

imRishN changed the title ~~Decommission/validation~~ Add validation in Decommission Request for minimum awareness attributes Oct 13, 2022

imRishN added 2 commits October 13, 2022 13:04

Add changelog

7dd9f40

Signed-off-by: Rishab Nahata <[email protected]>

Fix spotless check

a020121

Signed-off-by: Rishab Nahata <[email protected]>

imRishN added 3 commits October 13, 2022 17:55

Fix UT and add UT

4b7981a

Signed-off-by: Rishab Nahata <[email protected]>

Fix spotless check

7a3a229

Signed-off-by: Rishab Nahata <[email protected]>

Merge remote-tracking branch 'upstream/main' into decommission/valida…

647389a

…tion

imRishN marked this pull request as ready for review October 13, 2022 12:29

imRishN requested review from a team and reta as code owners October 13, 2022 12:29

Empty-Commit

b90ffbb

Signed-off-by: Rishab Nahata <[email protected]>

gbbafna reviewed Oct 14, 2022

View reviewed changes

Simplify validations

a4a31f5

Signed-off-by: Rishab Nahata <[email protected]>

imRishN commented Oct 14, 2022

View reviewed changes

gbbafna approved these changes Oct 14, 2022

View reviewed changes

Empty-Commit

9f213cd

Signed-off-by: Rishab Nahata <[email protected]>

Empty-Commit

dc780a3

Signed-off-by: Rishab Nahata <[email protected]>

Empty-Commit

ff33c64

Signed-off-by: Rishab Nahata <[email protected]>

Bukhtawar reviewed Oct 16, 2022

View reviewed changes

imRishN added 2 commits October 21, 2022 10:19

Merge remote-tracking branch 'upstream/main' into decommission/valida…

e7962aa

…tion Signed-off-by: Rishab Nahata <[email protected]>

Simplify validation

e3a9d04

Signed-off-by: Rishab Nahata <[email protected]>

imRishN force-pushed the decommission/validation branch from 59eedb6 to e3a9d04 Compare October 21, 2022 04:59

Empty-Commit

856ba74

Signed-off-by: Rishab Nahata <[email protected]>

Empty-Commit

1a58068

Signed-off-by: Rishab Nahata <[email protected]>

Empty-Commit

f15180a

Signed-off-by: Rishab Nahata <[email protected]>

Merge remote-tracking branch 'upstream/main' into decommission/valida…

24d0638

…tion Signed-off-by: Rishab Nahata <[email protected]>

Merge branch 'main' into decommission/validation

fc72676

imRishN added 2 commits October 26, 2022 11:12

Fix

312f398

Signed-off-by: Rishab Nahata <[email protected]>

Bug fix

1c58628

Signed-off-by: Rishab Nahata <[email protected]>

imRishN added 3 commits October 26, 2022 12:18

Empty-Commit

aa6e732

Signed-off-by: Rishab Nahata <[email protected]>

bug fix

3a97f68

Signed-off-by: Rishab Nahata <[email protected]>

Bug fix

a46550a

Signed-off-by: Rishab Nahata <[email protected]>

Fix spotless check

0701ce4

Signed-off-by: Rishab Nahata <[email protected]>

Bukhtawar reviewed Oct 26, 2022

View reviewed changes

imRishN closed this Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation in Decommission Request for minimum awareness attributes #4767

Add validation in Decommission Request for minimum awareness attributes #4767

imRishN commented Oct 13, 2022 •

edited

Loading

github-actions bot commented Oct 13, 2022

github-actions bot commented Oct 13, 2022

github-actions bot commented Oct 13, 2022

github-actions bot commented Oct 13, 2022

codecov-commenter commented Oct 13, 2022 •

edited

Loading

gbbafna Oct 14, 2022

imRishN Oct 14, 2022

imRishN Oct 14, 2022

imRishN Oct 14, 2022

gbbafna left a comment

github-actions bot commented Oct 14, 2022

github-actions bot commented Oct 14, 2022

github-actions bot commented Oct 15, 2022

github-actions bot commented Oct 16, 2022

github-actions bot commented Oct 16, 2022

Bukhtawar Oct 16, 2022 •

edited

Loading

imRishN Oct 16, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 21, 2022

github-actions bot commented Oct 25, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

github-actions bot commented Oct 26, 2022

Bukhtawar Oct 26, 2022

imRishN Oct 26, 2022

Add validation in Decommission Request for minimum awareness attributes #4767

Add validation in Decommission Request for minimum awareness attributes #4767

Conversation

imRishN commented Oct 13, 2022 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 13, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Oct 13, 2022 • edited Loading

Codecov Report

gbbafna Oct 14, 2022

Choose a reason for hiding this comment

imRishN Oct 14, 2022

Choose a reason for hiding this comment

imRishN Oct 14, 2022

Choose a reason for hiding this comment

imRishN Oct 14, 2022

Choose a reason for hiding this comment

gbbafna left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 14, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 16, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 16, 2022

Gradle Check (Jenkins) Run Completed with:

Bukhtawar Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

imRishN Oct 16, 2022

Choose a reason for hiding this comment

github-actions bot commented Oct 21, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 21, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 21, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 21, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 25, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 26, 2022

Gradle Check (Jenkins) Run Completed with:

Bukhtawar Oct 26, 2022

Choose a reason for hiding this comment

imRishN Oct 26, 2022

Choose a reason for hiding this comment

imRishN commented Oct 13, 2022 •

edited

Loading

codecov-commenter commented Oct 13, 2022 •

edited

Loading

Bukhtawar Oct 16, 2022 •

edited

Loading