Skip to content

Commit

Permalink
Skip on any node capability being present (#111585)
Browse files Browse the repository at this point in the history
Update capabilities skip behavior to skip on any node having the capability, not all nodes
  • Loading branch information
thecoop authored Aug 7, 2024
1 parent 0b064c7 commit 5da4f31
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
"capabilities": {
"type": "string",
"description": "Comma-separated list of arbitrary API capabilities to check"
},
"local_only": {
"type": "boolean",
"description": "True if only the node being called should be considered",
"visibility": "private"
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@ public class NodesCapabilitiesRequest extends BaseNodesRequest<NodesCapabilities
private RestApiVersion restApiVersion = RestApiVersion.current();

public NodesCapabilitiesRequest() {
// always send to all nodes
// send to all nodes
super(Strings.EMPTY_ARRAY);
}

public NodesCapabilitiesRequest(String nodeId) {
// only send to this node (the local node)
super(new String[] { nodeId });
}

public NodesCapabilitiesRequest path(String path) {
this.path = path;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
public class RestFeatures implements FeatureSpecification {
@Override
public Set<NodeFeature> getFeatures() {
return Set.of(RestNodesCapabilitiesAction.CAPABILITIES_ACTION, UNIFIED_HIGHLIGHTER_MATCHED_FIELDS);
return Set.of(
RestNodesCapabilitiesAction.CAPABILITIES_ACTION,
RestNodesCapabilitiesAction.LOCAL_ONLY_CAPABILITIES,
UNIFIED_HIGHLIGHTER_MATCHED_FIELDS
);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
public class RestNodesCapabilitiesAction extends BaseRestHandler {

public static final NodeFeature CAPABILITIES_ACTION = new NodeFeature("rest.capabilities_action");
public static final NodeFeature LOCAL_ONLY_CAPABILITIES = new NodeFeature("rest.local_only_capabilities");

@Override
public List<Route> routes() {
Expand All @@ -38,7 +39,7 @@ public List<Route> routes() {

@Override
public Set<String> supportedQueryParameters() {
return Set.of("timeout", "method", "path", "parameters", "capabilities");
return Set.of("timeout", "method", "path", "parameters", "capabilities", "local_only");
}

@Override
Expand All @@ -48,7 +49,11 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
NodesCapabilitiesRequest r = new NodesCapabilitiesRequest().timeout(getTimeout(request))
NodesCapabilitiesRequest requestNodes = request.paramAsBoolean("local_only", false)
? new NodesCapabilitiesRequest(client.getLocalNodeId())
: new NodesCapabilitiesRequest();

NodesCapabilitiesRequest r = requestNodes.timeout(getTimeout(request))
.method(RestRequest.Method.valueOf(request.param("method", "GET")))
.path(URLDecoder.decode(request.param("path"), StandardCharsets.UTF_8))
.parameters(request.paramAsStringArray("parameters", Strings.EMPTY_ARRAY))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,12 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.client.Node;
import org.elasticsearch.client.NodeSelector;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.util.Maps;
import org.elasticsearch.rest.action.admin.cluster.RestNodesCapabilitiesAction;
import org.elasticsearch.test.rest.Stash;
import org.elasticsearch.test.rest.TestFeatureService;
import org.elasticsearch.test.rest.yaml.restspec.ClientYamlSuiteRestApi;
Expand Down Expand Up @@ -280,8 +282,14 @@ public boolean clusterHasFeature(String featureId) {
return testFeatureService.clusterHasFeature(featureId);
}

public Optional<Boolean> clusterHasCapabilities(String method, String path, String parametersString, String capabilitiesString) {
Map<String, String> params = Maps.newMapWithExpectedSize(5);
public Optional<Boolean> clusterHasCapabilities(
String method,
String path,
String parametersString,
String capabilitiesString,
boolean any
) {
Map<String, String> params = Maps.newMapWithExpectedSize(6);
params.put("method", method);
params.put("path", path);
if (Strings.hasLength(parametersString)) {
Expand All @@ -291,8 +299,46 @@ public Optional<Boolean> clusterHasCapabilities(String method, String path, Stri
params.put("capabilities", capabilitiesString);
}
params.put("error_trace", "false"); // disable error trace

if (clusterHasFeature(RestNodesCapabilitiesAction.LOCAL_ONLY_CAPABILITIES.id()) == false) {
// can only check the whole cluster
if (any) {
logger.warn(
"Cluster does not support checking individual nodes for capabilities,"
+ "check for [{} {}?{} {}] may be incorrect in mixed-version clusters",
method,
path,
parametersString,
capabilitiesString
);
}
return checkCapability(NodeSelector.ANY, params);
} else {
// check each node individually - we can actually check any here
params.put("local_only", "true"); // we're calling each node individually

// individually call each node, so we can control whether we do an 'any' or 'all' check
List<Node> nodes = clientYamlTestClient.getRestClient(NodeSelector.ANY).getNodes();

for (Node n : nodes) {
Optional<Boolean> nodeResult = checkCapability(new SpecificNodeSelector(n), params);
if (nodeResult.isEmpty()) {
return Optional.empty();
} else if (any == nodeResult.get()) {
// either any == true and node has cap,
// or any == false (ie all) and this node does not have cap
return nodeResult;
}
}

// if we got here, either any is true and no node has it, or any == false and all nodes have it
return Optional.of(any == false);
}
}

private Optional<Boolean> checkCapability(NodeSelector nodeSelector, Map<String, String> params) {
try {
ClientYamlTestResponse resp = callApi("capabilities", params, emptyList(), emptyMap());
ClientYamlTestResponse resp = callApi("capabilities", params, emptyList(), emptyMap(), nodeSelector);
// anything other than 200 should result in an exception, handled below
assert resp.getStatusCode() == 200 : "Unknown response code " + resp.getStatusCode();
return Optional.ofNullable(resp.evaluate("supported"));
Expand All @@ -305,4 +351,17 @@ public Optional<Boolean> clusterHasCapabilities(String method, String path, Stri
throw new UncheckedIOException(ioException);
}
}

private record SpecificNodeSelector(Node node) implements NodeSelector {
@Override
public void select(Iterable<Node> nodes) {
// between getting the list of nodes, and checking here, the thing that is consistent is the host
// which becomes one of the bound addresses
for (var it = nodes.iterator(); it.hasNext();) {
if (it.next().getBoundHosts().contains(node.getHost()) == false) {
it.remove();
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,18 @@ static Predicate<ClientYamlTestExecutionContext> skipOnKnownIssue(List<KnownIssu

static CapabilitiesPredicate requireCapabilities(List<CapabilitiesCheck> checks) {
// requirement not fulfilled if unknown / capabilities API not supported
return context -> checks.stream().allMatch(check -> checkCapabilities(context, check).orElse(false));
return context -> checks.stream().allMatch(check -> checkCapabilities(context, check, false).orElse(false));
}

static CapabilitiesPredicate skipCapabilities(List<CapabilitiesCheck> checks) {
// skip if unknown / capabilities API not supported
return context -> checks.stream().anyMatch(check -> checkCapabilities(context, check).orElse(true));
return context -> checks.stream().anyMatch(check -> checkCapabilities(context, check, true).orElse(true));
}

interface CapabilitiesPredicate extends Predicate<ClientYamlTestExecutionContext> {}

private static Optional<Boolean> checkCapabilities(ClientYamlTestExecutionContext context, CapabilitiesCheck check) {
Optional<Boolean> b = context.clusterHasCapabilities(check.method(), check.path(), check.parameters(), check.capabilities());
private static Optional<Boolean> checkCapabilities(ClientYamlTestExecutionContext context, CapabilitiesCheck check, boolean any) {
Optional<Boolean> b = context.clusterHasCapabilities(check.method(), check.path(), check.parameters(), check.capabilities(), any);
return b;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.oneOf;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -596,17 +597,17 @@ public void testEvaluateCapabilities() {
assertTrue(section.skipCriteriaMet(context)); // always skip if unavailable
assertFalse(section.requiresCriteriaMet(context)); // always fail requirements / skip if unavailable

when(context.clusterHasCapabilities(anyString(), anyString(), any(), any())).thenReturn(Optional.of(FALSE));
when(context.clusterHasCapabilities(anyString(), anyString(), any(), any(), anyBoolean())).thenReturn(Optional.of(FALSE));
assertFalse(section.skipCriteriaMet(context));
assertFalse(section.requiresCriteriaMet(context));

when(context.clusterHasCapabilities("GET", "/s", null, "c1,c2")).thenReturn(Optional.of(TRUE));
when(context.clusterHasCapabilities("GET", "/s", null, "c1,c2", true)).thenReturn(Optional.of(TRUE));
assertTrue(section.skipCriteriaMet(context));

when(context.clusterHasCapabilities("GET", "/r", null, null)).thenReturn(Optional.of(TRUE));
when(context.clusterHasCapabilities("GET", "/r", null, null, false)).thenReturn(Optional.of(TRUE));
assertFalse(section.requiresCriteriaMet(context));

when(context.clusterHasCapabilities("GET", "/r", "p1", null)).thenReturn(Optional.of(TRUE));
when(context.clusterHasCapabilities("GET", "/r", "p1", null, false)).thenReturn(Optional.of(TRUE));
assertTrue(section.requiresCriteriaMet(context));
}

Expand Down

0 comments on commit 5da4f31

Please sign in to comment.