Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow multiple data paths for search nodes #6427

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Disallow multiple data paths for search nodes #6427

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1207b53
Add bootstrap check to avoid search node has multiple data path
xuezhou25 Feb 21, 2023
d1a7b87
Add changelog
xuezhou25 Feb 22, 2023
6a3cb90
Modify comments
xuezhou25 Feb 23, 2023
8633846
Merge branch 'main' into Prevent_search_node_startup_for_multiple_dat…
xuezhou25 Feb 23, 2023
411e7a5
fix spotless check
xuezhou25 Feb 23, 2023
ec4a3d6
Modify comments
xuezhou25 Feb 23, 2023
c9ff4d1
Remove duplication
xuezhou25 Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
## [Unreleased 2.x]
### Added
- Add GeoTile and GeoHash Grid aggregations on GeoShapes. ([#5589](https://github.com/opensearch-project/OpenSearch/pull/5589))
- Disallow multiple data paths for search nodes ([#6427](https://github.com/opensearch-project/OpenSearch/pull/6427))

### Dependencies

Expand Down
25 changes: 25 additions & 0 deletions server/src/main/java/org/opensearch/bootstrap/BootstrapChecks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,18 @@
import org.apache.lucene.util.Constants;
import org.opensearch.bootstrap.jvm.DenyJvmVersionsParser;
import org.opensearch.cluster.coordination.ClusterBootstrapService;
import org.opensearch.cluster.node.DiscoveryNodeRole;
import org.opensearch.common.SuppressForbidden;
import org.opensearch.common.io.PathUtils;
import org.opensearch.common.settings.Setting;
import org.opensearch.common.transport.BoundTransportAddress;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.discovery.DiscoveryModule;
import org.opensearch.env.Environment;
import org.opensearch.index.IndexModule;
import org.opensearch.monitor.jvm.JvmInfo;
import org.opensearch.monitor.process.ProcessProbe;
import org.opensearch.node.NodeRoleSettings;
import org.opensearch.node.NodeValidationException;

import java.io.BufferedReader;
Expand Down Expand Up @@ -228,6 +231,7 @@ static List<BootstrapCheck> checks() {
checks.add(new JavaVersionCheck());
checks.add(new AllPermissionCheck());
checks.add(new DiscoveryConfiguredCheck());
checks.add(new MultipleDataPathCheck());
return Collections.unmodifiableList(checks);
}

Expand Down Expand Up @@ -751,4 +755,25 @@ public BootstrapCheckResult check(BootstrapContext context) {
);
}
}

/**
* Bootstrap check that if a search node contains multiple data paths
*/
static class MultipleDataPathCheck implements BootstrapCheck {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this class be private? I think it is reasonable to be default

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The answer to my question is "no" because it is referenced in the unit test, which I missed when I made this comment :)


@Override
public BootstrapCheckResult check(BootstrapContext context) {
if (NodeRoleSettings.NODE_ROLES_SETTING.get(context.settings()).contains(DiscoveryNodeRole.SEARCH_ROLE)
&& Environment.PATH_DATA_SETTING.get(context.settings()).size() > 1) {
return BootstrapCheckResult.failure("Having multiple data paths in the search node is not allowed");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nitpick on the wording, but something like "Multiple data paths are not allowed for search nodes" is probably a bit clearer and more direct.

}
return BootstrapCheckResult.success();
}

@Override
public final boolean alwaysEnforce() {
return true;
}

}
}
51 changes: 51 additions & 0 deletions server/src/test/java/org/opensearch/bootstrap/BootstrapChecksTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,24 @@
import org.apache.lucene.util.Constants;
import org.opensearch.cluster.coordination.ClusterBootstrapService;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.cluster.node.DiscoveryNodeRole;
import org.opensearch.common.CheckedConsumer;
import org.opensearch.common.io.PathUtils;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.transport.BoundTransportAddress;
import org.opensearch.common.transport.TransportAddress;
import org.opensearch.discovery.DiscoveryModule;
import org.opensearch.discovery.SettingsBasedSeedHostsProvider;
import org.opensearch.env.Environment;
import org.opensearch.monitor.jvm.JvmInfo;
import org.opensearch.node.NodeRoleSettings;
import org.opensearch.node.NodeValidationException;
import org.opensearch.test.AbstractBootstrapCheckTestCase;
import org.hamcrest.Matcher;

import java.lang.Runtime.Version;
import java.net.InetAddress;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -774,4 +779,50 @@ Version getVersion() {
version.set(Runtime.version());
BootstrapChecks.check(emptyContext, true, Collections.singletonList(check));
}

public void testMultipleDataPathsForSearchNodeCheck() throws NodeValidationException {
Path path = PathUtils.get(createTempDir().toString());
String[] paths = new String[] { path.resolve("a").toString(), path.resolve("b").toString() };

final BootstrapContext context = createTestContext(
Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.SEARCH_ROLE.roleName()))
.putList(Environment.PATH_DATA_SETTING.getKey(), paths)
.build(),
Metadata.EMPTY_METADATA
);
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck());
final NodeValidationException e = expectThrows(NodeValidationException.class, () -> BootstrapChecks.check(context, true, checks));
assertThat(e.getMessage(), containsString("Having multiple data paths in the search node is not allowed"));
}
Copy link
Contributor

@gashutos gashutos Feb 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this kind of private method help to remove some duplicate effort ?

public void performDataPathsCheck(String[] paths, String roleName) throws NodeValidationException {
         final BootstrapContext context = createTestContext(
            Settings.builder()
                .putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(roleName))
                .putList(Environment.PATH_DATA_SETTING.getKey(), paths)
                .build(),
            Metadata.EMPTY_METADATA
        );
        final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck());
        BootstrapChecks.check(emptyContext, true, checks);
    }


public void testMultipleDataPathsForDataNodeCheck() throws NodeValidationException {
Path path = PathUtils.get(createTempDir().toString());
String[] paths = new String[] { path.resolve("a").toString(), path.resolve("b").toString() };

final BootstrapContext context = createTestContext(
Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.DATA_ROLE.roleName()))
.putList(Environment.PATH_DATA_SETTING.getKey(), paths)
.build(),
Metadata.EMPTY_METADATA
);
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck());
BootstrapChecks.check(emptyContext, true, checks);
}

public void testSingleDataPathForSearchNodeCheck() throws NodeValidationException {
Path path = PathUtils.get(createTempDir().toString());
String[] paths = new String[] { path.resolve("a").toString() };

final BootstrapContext context = createTestContext(
Settings.builder()
.putList(NodeRoleSettings.NODE_ROLES_SETTING.getKey(), Collections.singletonList(DiscoveryNodeRole.SEARCH_ROLE.roleName()))
.putList(Environment.PATH_DATA_SETTING.getKey(), paths)
.build(),
Metadata.EMPTY_METADATA
);
final List<BootstrapCheck> checks = Collections.singletonList(new BootstrapChecks.MultipleDataPathCheck());
BootstrapChecks.check(emptyContext, true, checks);
}
}