Add read_only block argument to opensearch-node unsafe-bootstrap command #599

harmishlakhani · 2021-04-22T12:27:52Z

…p command

Signed-off-by: Harmish Lakhani harmish.lakhani@gmail.com

Description

This change will add one optional argument to opensearch-node unsafe-bootstrap command known as apply-cluster-read-only-block. It will apply read only block to cluster based on its value (true or false) after successful bootstrap.

Usage : ./bin/opensearch-node unsafe-bootstrap --apply-cluster-read-only-block true

Older PR for previous conversations
#493

Issues Resolved

closes: #492

Check List

[ x ] New functionality includes testing.
- [ x ] All tests pass
New functionality has been documented.
- New functionality has javadoc added
[ x ] 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.

…p command Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

odfe-release-bot · 2021-04-22T12:31:58Z

✅ Gradle Wrapper Validation success f9ccaab

odfe-release-bot · 2021-04-22T12:31:58Z

✅ DCO Check Passed f9ccaab

odfe-release-bot · 2021-04-22T12:37:13Z

❌ Gradle Precommit failure f9ccaab
Log 305

Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

odfe-release-bot · 2021-04-23T05:35:47Z

✅ DCO Check Passed 46e5796

odfe-release-bot · 2021-04-23T05:35:47Z

✅ Gradle Wrapper Validation success 46e5796

odfe-release-bot · 2021-04-23T05:45:31Z

✅ Gradle Precommit success 46e5796

saratvemulapalli · 2021-04-27T06:53:19Z

start gradle check

odfe-release-bot · 2021-04-27T07:07:14Z

❌ Gradle Check failure 46e5796
Log 142

Reports 142

saratvemulapalli · 2021-04-28T16:27:48Z

@harmishlakhani can you take a look at the gradle check?

opensearch-ci-bot · 2021-05-03T04:40:16Z

❌ DCO Check Failed e7cb4001039b8a80516ea612e693182777709950
Run ./dev-tools/signoff-check.sh remotes/origin/main e7cb4001039b8a80516ea612e693182777709950 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

opensearch-ci-bot · 2021-05-03T04:40:16Z

✅ Gradle Wrapper Validation success e7cb4001039b8a80516ea612e693182777709950

opensearch-ci-bot · 2021-05-03T04:47:54Z

✅ Gradle Precommit success e7cb4001039b8a80516ea612e693182777709950

…ad_only_block Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

…unsafe_bootstrap_read_only_block Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

opensearch-ci-bot · 2021-05-04T07:10:15Z

✅ DCO Check Passed 6e77034

opensearch-ci-bot · 2021-05-04T07:10:15Z

✅ Gradle Wrapper Validation success 6e77034

opensearch-ci-bot · 2021-05-04T07:15:27Z

✅ Gradle Precommit success 6e77034

saratvemulapalli · 2021-05-04T14:44:12Z

start gradle check

opensearch-ci-bot · 2021-05-04T15:05:58Z

✅ Gradle Check success 6e77034
Log 155

Reports 155

shwetathareja · 2021-05-05T14:13:00Z

...lClusterTest/java/org/opensearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java

+
+    private void removeBlock() {
+        if (isInternalCluster()) {
+            Settings settings = Settings.builder().putNull(Metadata.SETTING_READ_ONLY_SETTING.getKey()).build()  ;


[minor]: extra space before ";"

Noted, fix will come in next commit.

shwetathareja · 2021-05-05T14:14:06Z

...lClusterTest/java/org/opensearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java

@@ -235,20 +261,24 @@ public void test3MasterNodes2Failed() throws Exception {

        logger.info("--> start 1st master-eligible node");
        masterNodes.add(internalCluster().startMasterOnlyNode(Settings.builder()
-                .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), "0s")
-                .build())); // node ordinal 0
+            .put(DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.getKey(), "0s")


you have updated the indentation at multiple places?

IntelliJ automatically updated indentation. Do we have to import any specific rules for IntelliJ for code formatting or defaults are suffice ?

@saratvemulapalli can you help with this? Do we have specific code formatter?

Just use ./gradlew spotlessApply it will format all of the code with the rules the project has defined.

@saratvemulapalli Thanks, I have checked with that and build is passing successfully without any changes.
So I guess all formatting is according to project rules only. See my comment for build output.

shwetathareja · 2021-05-05T14:18:08Z

...lClusterTest/java/org/opensearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java

@@ -321,9 +356,10 @@ public void test3MasterNodes2Failed() throws Exception {
        detachCluster(environmentMaster3, false);

        logger.info("--> start 2nd and 3rd master-eligible nodes and ensure 4 nodes stable cluster");
-        internalCluster().startMasterOnlyNode(master2DataPathSettings);


Earlier, it was providing the data path explicitly, why did we remove it?

Not able to recall why I removed it but yes I will add it back.

shwetathareja · 2021-05-05T14:21:25Z

server/src/main/java/org/opensearch/cluster/coordination/UnsafeBootstrapMasterCommand.java

@@ -122,7 +134,9 @@ protected void processNodePaths(Terminal terminal, Path[] dataPaths, int nodeLoc
        Settings persistentSettings = Settings.builder()
            .put(metadata.persistentSettings())
            .put(UNSAFE_BOOTSTRAP.getKey(), true)
+            .put(Metadata.SETTING_READ_ONLY_SETTING.getKey(), applyClusterReadOnlyBlock)


if there is a cluster block already present in the cluster and user doesnt pass set it explicitly, then it will get removed unintentionally. It should add this block only if it is true.

Ohh nice catch, let me fix it.

…s existing settings Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

opensearch-ci-bot · 2021-05-06T05:57:17Z

✅ DCO Check Passed 1363ac4

opensearch-ci-bot · 2021-05-06T05:57:55Z

✅ Gradle Wrapper Validation success 1363ac4

opensearch-ci-bot · 2021-05-06T06:06:41Z

✅ Gradle Precommit success 1363ac4

shwetathareja

Minor comments, otherwise LGTM

shwetathareja · 2021-05-11T07:14:33Z

...lClusterTest/java/org/opensearch/cluster/coordination/UnsafeBootstrapAndDetachCommandIT.java

-import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.notNullValue;


revert all the un-necessary import changes due to intellij or check with @saratvemulapalli which code formatter to use.

I guess IntelliJ has reformatted all changes according to project rules only. See below check which is passing.

┌[kiwish-4.2]-(workplace/harmishlakhani-OpenSearch)-[git:unsafe_bootstrap_read_only_block]- └> ./gradlew spotlessApply > Configure project : ========================= WARNING ========================= Backwards compatibility tests are disabled! See https://github.com/opensearch-project/OpenSearch/issues/105 =========================================================== > Configure project :qa:os Cannot add task 'destructiveDistroTest.docker' as a task with that name already exists. ======================================= OpenSearch Build Hamster says Hello! Gradle Version : 6.6.1 OS Info : Mac OS X 10.15.7 (x86_64) Runtime JDK Version : 11 (JDK) Runtime java.home : /Library/Java/JavaVirtualMachines/amazon-corretto-11.jdk/Contents/Home Gradle JDK Version : 15 (JDK) Gradle java.home : /Library/Java/JavaVirtualMachines/amazon-corretto-15.jdk/Contents/Home Random Testing Seed : 6884CF4EAA1728F3 In FIPS 140 mode : false ======================================= BUILD SUCCESSFUL in 2s 23 actionable tasks: 23 up-to-date ┌[kiwish-4.2]-(workplace/harmishlakhani-OpenSearch)-[git:unsafe_bootstrap_read_only_block]- └> git status On branch unsafe_bootstrap_read_only_block Your branch is up to date with 'origin/unsafe_bootstrap_read_only_block'. nothing to commit, working tree clean

shwetathareja · 2021-05-11T07:17:27Z

server/src/main/java/org/opensearch/cluster/coordination/UnsafeBootstrapMasterCommand.java

            .build();

+        Boolean applyClusterReadOnlyBlock = applyClusterReadOnlyBlockOption.value(options);
+        if(Objects.nonNull(applyClusterReadOnlyBlock)) {


I am wondering if the user passes false explicitly, then what should be the behavior. Should we keep the existing block as is (in case it exists on the cluster)? Thoughts?

I think we should entertain what user has given in input, because unsafe-bootstrap should be used consciously and whoever is running the command should know what it will do. Also if cluster block is already present then we hope that user will aware about the same while using this command.

shwetathareja · 2021-05-12T10:49:14Z

we might need to raise a backport PR for 1.x after this is merged.

harmishlakhani · 2021-05-12T11:17:04Z

@shwetathareja Yes I will raise backport PR to 1.x after this is merged.
Just one question, shouldn't we merge main branch altogether to 1.x because there will me many PRs which we want to merge to 1.x after they merged to main branch ? Thoughts ?

saratvemulapalli · 2021-05-12T21:37:07Z

start gradle check

opensearch-ci-bot · 2021-05-12T22:34:51Z

✅ Gradle Check success 1363ac4
Log 177

Reports 177

saratvemulapalli

The change looks good to me!

@harmishlakhani if need this change in 1.x, please backport it.

…p command #599 (#725) * apply user defined cluster wide read only block after unsafe bootstrap command Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com> * Fixed gradle precommit failures Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com> * remove default as false for read only block to avoid overriding user's existing settings Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

apply user defined cluster wide read only block after unsafe bootstra…

f9ccaab

…p command Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

Fixed gradle precommit failures

46e5796

Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

saratvemulapalli self-assigned this Apr 27, 2021

harmishlakhani added 2 commits May 4, 2021 12:31

Merge remote-tracking branch 'upstream/main' into unsafe_bootstrap_re…

a7c6abe

…ad_only_block Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

Merge branch 'main' of github.com:opensearch-project/OpenSearch into …

6e77034

…unsafe_bootstrap_read_only_block Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

harmishlakhani force-pushed the unsafe_bootstrap_read_only_block branch from e7cb400 to 6e77034 Compare May 4, 2021 07:05

shwetathareja reviewed May 5, 2021

View reviewed changes

remove default as false for read only block to avoid overriding user'…

1363ac4

…s existing settings Signed-off-by: Harmish Lakhani <harmish.lakhani@gmail.com>

shwetathareja reviewed May 11, 2021

View reviewed changes

shwetathareja approved these changes May 12, 2021

View reviewed changes

saratvemulapalli approved these changes May 18, 2021

View reviewed changes

saratvemulapalli merged commit 6732239 into opensearch-project:main May 18, 2021

harmishlakhani mentioned this pull request May 19, 2021

[1.x] Add read_only block argument to opensearch-node unsafe-bootstrap command #599 #725

Merged

Add read_only block argument to opensearch-node unsafe-bootstrap command #599

Add read_only block argument to opensearch-node unsafe-bootstrap command #599

Conversation

harmishlakhani commented Apr 22, 2021 • edited Loading

Description

Issues Resolved

Check List

odfe-release-bot commented Apr 22, 2021

odfe-release-bot commented Apr 22, 2021

odfe-release-bot commented Apr 22, 2021

odfe-release-bot commented Apr 23, 2021

odfe-release-bot commented Apr 23, 2021

odfe-release-bot commented Apr 23, 2021

saratvemulapalli commented Apr 27, 2021

odfe-release-bot commented Apr 27, 2021

saratvemulapalli commented Apr 28, 2021

opensearch-ci-bot commented May 3, 2021

opensearch-ci-bot commented May 3, 2021

opensearch-ci-bot commented May 3, 2021

opensearch-ci-bot commented May 4, 2021

opensearch-ci-bot commented May 4, 2021

opensearch-ci-bot commented May 4, 2021

saratvemulapalli commented May 4, 2021

opensearch-ci-bot commented May 4, 2021

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

opensearch-ci-bot commented May 6, 2021

opensearch-ci-bot commented May 6, 2021

opensearch-ci-bot commented May 6, 2021

shwetathareja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmishlakhani May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shwetathareja commented May 12, 2021

harmishlakhani commented May 12, 2021

saratvemulapalli commented May 12, 2021

opensearch-ci-bot commented May 12, 2021

saratvemulapalli left a comment • edited Loading

Choose a reason for hiding this comment

harmishlakhani commented Apr 22, 2021 •

edited

Loading

harmishlakhani May 12, 2021 •

edited

Loading

saratvemulapalli left a comment •

edited

Loading