-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Changes from 2 commits
1207b53
d1a7b87
6a3cb90
8633846
411e7a5
ec4a3d6
c9ff4d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be private? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -774,4 +779,22 @@ Version getVersion() { | |
version.set(Runtime.version()); | ||
BootstrapChecks.check(emptyContext, true, Collections.singletonList(check)); | ||
} | ||
|
||
public void testMultipleDataPathCheck() throws NodeValidationException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there should be 3 cases here:
You have case 2 covered, but not really the other two. They should be separate test methods as well because it is generally a best practice to make each test case test one thing whenever possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it! Thanks for the reviews Andrew :) |
||
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 role is not allowed")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is preferable to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// Validate the check passes under default setting. | ||
BootstrapChecks.check(emptyContext, true, checks); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this "Disallow multiple data paths for search nodes". Please update the commit message and PR title as well.