Adding feature to exclude indexes starting with dot from shard validator #4695

nishchay21 · 2022-10-06T13:26:43Z

Signed-off-by: Nishchay Malhotra nishcha@amazon.com

Description

The Current PR helps in solving the issue #4675.

Currently, all plugins do not follow a standard method of inheriting the system indices and are not ignored during the shard limit validation phase. The goal of this PR is to exempt indexes starting with a '.' character to be ignored during this shard limit validation phase.

Further, this PR also adds a cluster setting "cluster.ignore_hidden_indexes" which works as a gated mechanism for the feature to be enabled or disabled. This is a dynamic cluster setting which takes in a boolean value. If the setting is set to true the indexes starting with the '.' character are ignored otherwise considered like other normal indexes. Adding to this the default value of this is added to be true so that the indexes created with '.' are ignored if this setting is not explicitly set.

This is in continuation to PR #4674 as the previous PR has some DCO issue

Issues Resolved

#4675 [Add feature to ignore indexes starting with dot on shard limit validation]

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: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-06T13:56:55Z

Gradle Check (Jenkins) Run Completed with:

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

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

nishchay21 · 2022-10-06T14:03:14Z

CHANGELOG.md

@@ -134,6 +134,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
  - Addition of Doc values on the GeoShape Field
  - Addition of GeoShape ValueSource level code interfaces for accessing the DocValues.
  - Addition of Missing Value feature in the GeoShape Aggregations.
+- Added feature to ignore indexes starting with dot during shard limit validation.([#4695](https://github.com/opensearch-project/OpenSearch/pull/4695))


This is the parent comment that is the reason it looks distorted from above comments as those are sub comments.

github-actions · 2022-10-06T14:06:54Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3957/
CommitID: 0c972e9

github-actions · 2022-10-06T14:27:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/3961/
CommitID: 10524a6

github-actions · 2022-10-07T04:27:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4084/
CommitID: 1ac4ca0

ashking94 · 2022-10-07T09:29:20Z

server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java

+                .put(SETTING_NUMBER_OF_REPLICAS, counts.getFirstIndexReplicas())
+                .build()
+        );
+


It would be good to add assertions around the max shard per node setting value and the total number of shards created so far.

ACK will add

ashking94 · 2022-10-07T09:29:56Z

server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java

+            verifyException(dataNodes, counts, e);
+        }
+        ClusterState clusterState = client().admin().cluster().prepareState().get().getState();
+        assertTrue(clusterState.getMetadata().hasIndex(".test-index"));


Lets add an assertion here as well.

ACK will add

ashking94 · 2022-10-07T09:30:26Z

server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java

+    private void setIgnoreHiddenIndex(boolean ignoreHiddenIndex) {
+        try {
+            ClusterUpdateSettingsResponse response;
+            if (frequently()) {


can we revisit if the usage of frequently is required here?

yes we will check if for both persistent and transient the setting is getting applied.

ashking94 · 2022-10-07T09:31:18Z

server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java

+                response = client().admin()
+                    .cluster()
+                    .prepareUpdateSettings()
+                    .setTransientSettings(Settings.builder().put(shardsPerNodeKey, ignoreHiddenIndex).build())


It should be ignoreHiddenIndexKey in put of the setting builder.

ashking94 · 2022-10-07T09:33:00Z

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java

+     * @param indexName  The index which needs to be validated.
+     */
+    private boolean validateHiddenIndex(String indexName) {
+        return this.ignoreHiddenIndexes && indexName.charAt(0) == '.';


indexName.charAt(0) == '.' could this be little more generic where we could have pattern match or list of pattern match to allow user customisations?

This will only be done for hidden and system indexes and index names starting with a dot should be reserved for hidden and system indices

should we exclude datastream indices from this ? we can use BACKING_INDEX_PREFIX to match prefix for it .

Ack will add that

hidden should be index.hidden in index metadata

ashking94

Please make the necessary changes.

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-07T11:43:00Z

Gradle Check (Jenkins) Run Completed with:

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

anasalkouz · 2022-10-12T00:30:29Z

Is this a backward compatible change? Can OpenSearch's users create indexes started with dot?

dblock

See below re:backwards incompatible change. It looks like such.

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-14T06:24:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4687/
CommitID: 86dbd99

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-14T07:05:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4693/
CommitID: 6276ddb

ashking94

LGTM

gbbafna · 2022-10-18T08:13:32Z

server/src/main/java/org/opensearch/common/settings/ClusterSettings.java

@@ -252,6 +252,7 @@ public void apply(Settings value, Settings current, Settings previous) {
                Metadata.SETTING_READ_ONLY_SETTING,
                Metadata.SETTING_READ_ONLY_ALLOW_DELETE_SETTING,
                ShardLimitValidator.SETTING_CLUSTER_MAX_SHARDS_PER_NODE,
+                ShardLimitValidator.SETTING_CLUSTER_IGNORE_HIDDEN_INDEXES,


indices beginning with . are not necessarily hidden and vice-versa. index.hidden is a separate index level setting. Many opensearch plugin use that like cross-cluster and many don't like security-plugin. Should we say dot_indices ?

Previously named this to dot_index however was asked to rename as mentioned in this close PR #4674

@Bukhtawar : what is your take on using hidden or dot ?

dot indices have been deprecated so going forward we should use hidden and system indices only. For 2.x we might still have to support dot indices. Any reason why security plugin is still using dot index?

I am fine with dot index for the purpose of this PR and backporting to 2.x but please open a PR to deprecate dot indices going forward

@Bukhtawar @gbbafna so should we change to dot or go with hidden ??

gbbafna · 2022-10-18T08:16:07Z

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java

+     * @param indexName  The index which needs to be validated.
+     */
+    private boolean validateHiddenIndex(String indexName) {
+        return this.ignoreHiddenIndexes && indexName.charAt(0) == '.';


should we exclude datastream indices from this ? we can use BACKING_INDEX_PREFIX to match prefix for it .

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-19T04:14:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4910/
CommitID: 4f6e176

…ore_dot_indexes Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-19T14:46:56Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/4949/
CommitID: 8e70553

codecov-commenter · 2022-10-19T14:49:16Z

Codecov Report

Merging #4695 (8e70553) into main (f5ea1a4) will decrease coverage by 0.02%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##               main    #4695      +/-   ##
============================================
- Coverage     70.59%   70.57%   -0.03%     
+ Complexity    57637    57594      -43     
============================================
  Files          4661     4661              
  Lines        277152   277160       +8     
  Branches      40550    40550              
============================================
- Hits         195667   195607      -60     
- Misses        65124    65180      +56     
- Partials      16361    16373      +12

Impacted Files	Coverage Δ
...rg/opensearch/common/settings/ClusterSettings.java	`91.89% <ø> (ø)`
...va/org/opensearch/indices/ShardLimitValidator.java	`96.22% <77.77%> (-3.78%)`	⬇️
...g/opensearch/index/analysis/CharFilterFactory.java	`0.00% <0.00%> (-100.00%)`	⬇️
...search/indices/recovery/RecoveryTargetHandler.java	`0.00% <0.00%> (-100.00%)`	⬇️
...ava/org/opensearch/action/NoSuchNodeException.java	`0.00% <0.00%> (-50.00%)`	⬇️
.../action/admin/indices/flush/ShardFlushRequest.java	`50.00% <0.00%> (-50.00%)`	⬇️
...opensearch/persistent/PersistentTasksExecutor.java	`22.22% <0.00%> (-44.45%)`	⬇️
.../search/aggregations/pipeline/HoltLinearModel.java	`20.33% <0.00%> (-42.38%)`	⬇️
.../java/org/opensearch/search/dfs/AggregatedDfs.java	`54.83% <0.00%> (-41.94%)`	⬇️
...cluster/coordination/PublishClusterStateStats.java	`33.33% <0.00%> (-37.51%)`	⬇️
... and 471 more

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

Bukhtawar · 2022-10-19T15:42:09Z

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java

+        Further also validates if the cluster.ignore_dot_indexes is set to true.
+        If so then it does not validate any index which starts with '.'.
+        */
+        if ((systemIndices.validateSystemIndex(indexName) || validateDotIndex(indexName)) && !isDataStreamIndex(indexName)) {


Isn't the first condition a subset of the dot index

Yes it would be but to have consistency that system index are also ignore have not removed that completely. Also if the setting is false we still need system indexes to be ignored

Bukhtawar · 2022-10-19T15:49:15Z

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java

+    private boolean validateDotIndex(String indexName) {
+        return this.ignoreDotIndexes && indexName.charAt(0) == '.';
+    }


lets not club both the logic in this method looks confusing to read, just have indexName.charAt(0) == '.'

…itions hold true Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

github-actions · 2022-10-19T16:53:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/4965/
CommitID: 4cffc5b

github-actions · 2022-10-19T17:08:55Z

Gradle Check (Jenkins) Run Completed with:

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

dblock · 2022-10-19T18:28:06Z

Looks like a transient failure in precommit checks, kicked it.

…tor (#4695) * Adding feature to exclude indexes starting with dot from shard validator Signed-off-by: Nishchay Malhotra <nishcha@amazon.com> (cherry picked from commit 04eb817)

…tor (#4695) (#4953) * Adding feature to exclude indexes starting with dot from shard validator Signed-off-by: Nishchay Malhotra <nishcha@amazon.com> (cherry picked from commit 04eb817)

…tor (opensearch-project#4695) * Adding feature to exclude indexes starting with dot from shard validator Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Adding feature to exclude indexes starting with dot from shard validator

e7db666

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

nishchay21 mentioned this pull request Oct 6, 2022

Added feature to ignore indexes starting with dot during shard limit validation #4674

Closed

6 tasks

Made changes to changelog

0c972e9

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Made changes to changelog.md

10524a6

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

nishchay21 commented Oct 6, 2022

View reviewed changes

Merge branch 'opensearch-project:main' into ignore-hidden-indexes

1ac4ca0

nishchay21 marked this pull request as ready for review October 7, 2022 04:27

nishchay21 requested review from a team and reta as code owners October 7, 2022 04:27

ashking94 reviewed Oct 7, 2022

View reviewed changes

ashking94 requested changes Oct 7, 2022

View reviewed changes

Adding more assertions to integ test

bc5b974

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

dblock requested changes Oct 13, 2022

View reviewed changes

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java Outdated Show resolved Hide resolved

server/src/main/java/org/opensearch/indices/ShardLimitValidator.java Show resolved Hide resolved

Changed the default value of cluster.ignore_hidden_indexes to false.

86dbd99

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Added spotless formating

6276ddb

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

ashking94 approved these changes Oct 18, 2022

View reviewed changes

gbbafna reviewed Oct 18, 2022

View reviewed changes

Added logic to not ignore data-stream indexes from shard validation

4f6e176

Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Changed the setting name cluster.ignore_hidden_indexes to cluster.ign…

8e70553

…ore_dot_indexes Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Bukhtawar reviewed Oct 19, 2022

View reviewed changes

nishchay21 and others added 2 commits October 19, 2022 21:58

Added feature to ignore indexes on shard validation when certain cond…

4cffc5b

…itions hold true Signed-off-by: Nishchay Malhotra <nishcha@amazon.com>

Merge branch 'main' into ignore-hidden-indexes

d537103

Bukhtawar approved these changes Oct 19, 2022

View reviewed changes

nishchay21 requested a review from dblock October 19, 2022 16:41

dblock approved these changes Oct 19, 2022

View reviewed changes

Bukhtawar merged commit 04eb817 into opensearch-project:main Oct 19, 2022

Bukhtawar added the backport 2.x Backport to 2.x branch label Oct 27, 2022

opensearch-trigger-bot bot mentioned this pull request Oct 27, 2022

[Backport 2.x] Adding feature to exclude indexes starting with dot from shard validator #4953

Merged

Adding feature to exclude indexes starting with dot from shard validator #4695

Adding feature to exclude indexes starting with dot from shard validator #4695

Conversation

nishchay21 commented Oct 6, 2022 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Oct 6, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 6, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 7, 2022

Gradle Check (Jenkins) Run Completed with:

ashking94 Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashking94 left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2022

Gradle Check (Jenkins) Run Completed with:

anasalkouz commented Oct 12, 2022

dblock 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:

ashking94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishchay21 Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bukhtawar Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 19, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Oct 19, 2022

Codecov Report

Choose a reason for hiding this comment

nishchay21 Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Oct 19, 2022

Gradle Check (Jenkins) Run Completed with:

dblock commented Oct 19, 2022

nishchay21 commented Oct 6, 2022 •

edited

Loading

ashking94 Oct 7, 2022 •

edited

Loading

nishchay21 Oct 18, 2022 •

edited

Loading

Bukhtawar Oct 19, 2022 •

edited

Loading

nishchay21 Oct 19, 2022 •

edited

Loading