Skip to content

Commit 8d0a81e

Browse files
VietND96VietND96
authored
[grid] Fix regression Distributor rejecting requests when nodes have supported caps but no free slots (#16327)
1 parent f00747c commit 8d0a81e

File tree

4 files changed

+175
-19
lines changed

4 files changed

+175
-19
lines changed

‎java/src/org/openqa/selenium/grid/distributor/NodeRegistry.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,14 @@ public interface NodeRegistry extends HasReadyState, Closeable {
8585
DistributorStatus getStatus();
8686

8787
/**
88-
* Gets all available nodes that are not DOWN or DRAINING.
88+
* Get all nodes that are UP.
89+
*
90+
* @return Set of UP node statuses.
91+
*/
92+
Set<NodeStatus> getUpNodes();
93+
94+
/**
95+
* Gets all available nodes that are not DOWN or DRAINING and has free slots.
8996
*
9097
* @return Set of available node statuses.
9198
*/

‎java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,8 @@ private SlotId reserveSlot(RequestId requestId, Capabilities caps) {
466466
}
467467

468468
private boolean isNotSupported(Capabilities caps) {
469-
return getAvailableNodes().stream().noneMatch(node -> node.hasCapability(caps, slotMatcher));
469+
return nodeRegistry.getUpNodes().stream()
470+
.noneMatch(node -> node.hasCapability(caps, slotMatcher));
470471
}
471472

472473
private boolean reserve(SlotId id) {
@@ -560,21 +561,28 @@ public void run() {
560561
}
561562

562563
private void checkMatchingSlot(List<SessionRequestCapability> sessionRequests) {
563-
for (SessionRequestCapability request : sessionRequests) {
564-
long unmatchableCount =
565-
request.getDesiredCapabilities().stream()
566-
.filter(LocalDistributor.this::isNotSupported)
567-
.count();
568-
569-
if (unmatchableCount == request.getDesiredCapabilities().size()) {
570-
LOG.info(
571-
"No nodes support the capabilities in the request: "
572-
+ request.getDesiredCapabilities());
573-
SessionNotCreatedException exception =
574-
new SessionNotCreatedException("No nodes support the capabilities in the request");
575-
sessionQueue.complete(request.getRequestId(), Either.left(exception));
576-
}
577-
}
564+
// Get UP nodes once to avoid lock & loop over multiple requests
565+
Set<NodeStatus> upNodes = nodeRegistry.getUpNodes();
566+
// Filter and reject only requests where NO capabilities are supported by ANY UP node
567+
// This prevents rejecting requests when nodes support capabilities but are just busy
568+
sessionRequests.stream()
569+
.filter(
570+
request ->
571+
request.getDesiredCapabilities().stream()
572+
.noneMatch(
573+
caps ->
574+
upNodes.stream()
575+
.anyMatch(node -> node.hasCapability(caps, slotMatcher))))
576+
.forEach(
577+
request -> {
578+
LOG.info(
579+
"No nodes support the capabilities in the request: "
580+
+ request.getDesiredCapabilities());
581+
SessionNotCreatedException exception =
582+
new SessionNotCreatedException(
583+
"No nodes support the capabilities in the request");
584+
sessionQueue.complete(request.getRequestId(), Either.left(exception));
585+
});
578586
}
579587

580588
private void handleNewSessionRequest(SessionRequest sessionRequest) {

‎java/src/org/openqa/selenium/grid/distributor/local/LocalNodeRegistry.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,12 +357,19 @@ public DistributorStatus getStatus() {
357357

358358
@Override
359359
public Set<NodeStatus> getAvailableNodes() {
360+
// Filter nodes are UP and have capacity (available slots)
361+
return getUpNodes().stream()
362+
.filter(NodeStatus::hasCapacity)
363+
.collect(ImmutableSet.toImmutableSet());
364+
}
365+
366+
@Override
367+
public Set<NodeStatus> getUpNodes() {
360368
Lock readLock = this.lock.readLock();
361369
readLock.lock();
362370
try {
363371
return model.getSnapshot().stream()
364-
// Filter nodes are UP and have capacity (available slots)
365-
.filter(node -> UP.equals(node.getAvailability()) && node.hasCapacity())
372+
.filter(node -> UP.equals(node.getAvailability()))
366373
.collect(ImmutableSet.toImmutableSet());
367374
} finally {
368375
readLock.unlock();

‎java/test/org/openqa/selenium/grid/distributor/local/LocalDistributorTest.java‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -777,6 +777,140 @@ void shouldReduceRedundantSlotChecks() throws URISyntaxException {
777777
}
778778
}
779779

780+
@Test
781+
void shouldNotRejectRequestsWhenNodesHaveCapabilityButNoFreeSlots() throws URISyntaxException {
782+
// Create a distributor with rejectUnsupportedCaps enabled
783+
NewSessionQueue queue =
784+
new LocalNewSessionQueue(
785+
tracer,
786+
new DefaultSlotMatcher(),
787+
Duration.ofSeconds(2),
788+
Duration.ofSeconds(2),
789+
Duration.ofSeconds(1),
790+
registrationSecret,
791+
5);
792+
LocalDistributor distributor =
793+
new LocalDistributor(
794+
tracer,
795+
bus,
796+
new PassthroughHttpClient.Factory(localNode),
797+
new LocalSessionMap(tracer, bus),
798+
queue,
799+
new DefaultSlotSelector(),
800+
registrationSecret,
801+
Duration.ofMinutes(5),
802+
true, // Enable rejectUnsupportedCaps
803+
Duration.ofSeconds(5),
804+
newSessionThreadPoolSize,
805+
new DefaultSlotMatcher(),
806+
Duration.ofSeconds(30));
807+
808+
// Create a node that supports Chrome with single slot
809+
URI nodeUri = new URI("http://example:1234");
810+
Node node =
811+
LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret)
812+
.add(
813+
new ImmutableCapabilities("browserName", "chrome"),
814+
new TestSessionFactory(
815+
(id, c) ->
816+
new Session(id, nodeUri, new ImmutableCapabilities(), c, Instant.now())))
817+
.maximumConcurrentSessions(1)
818+
.build();
819+
distributor.add(node);
820+
821+
// Occupy the node's only slot
822+
SessionRequest sessionRequest =
823+
new SessionRequest(
824+
new RequestId(UUID.randomUUID()),
825+
Instant.now(),
826+
Set.of(W3C),
827+
Set.of(new ImmutableCapabilities("browserName", "chrome")),
828+
Map.of(),
829+
Map.of());
830+
Either<SessionNotCreatedException, CreateSessionResponse> result =
831+
distributor.newSession(sessionRequest);
832+
assertThat(result.isRight()).isTrue(); // Session created successfully
833+
834+
// Verify node has no available capacity but still supports Chrome
835+
assertThat(distributor.getAvailableNodes()).isEmpty(); // No available nodes
836+
837+
// Test that the distributor status shows the node is still UP and supports Chrome
838+
// even though it has no available capacity
839+
DistributorStatus status = distributor.getStatus();
840+
Set<NodeStatus> allNodes = status.getNodes();
841+
assertThat(allNodes).hasSize(1);
842+
843+
NodeStatus nodeStatus = allNodes.iterator().next();
844+
assertThat(nodeStatus.getAvailability()).isEqualTo(UP);
845+
846+
// Verify the node still supports Chrome capability even with no free slots
847+
boolean supportsChrome =
848+
nodeStatus.hasCapability(
849+
new ImmutableCapabilities("browserName", "chrome"), new DefaultSlotMatcher());
850+
assertThat(supportsChrome).isTrue();
851+
852+
// Verify the node has no capacity (all slots occupied)
853+
assertThat(nodeStatus.hasCapacity()).isFalse();
854+
}
855+
856+
@Test
857+
void shouldRejectRequestsWhenNoNodesHaveCapability() throws URISyntaxException {
858+
// Create a distributor with rejectUnsupportedCaps enabled
859+
NewSessionQueue queue =
860+
new LocalNewSessionQueue(
861+
tracer,
862+
new DefaultSlotMatcher(),
863+
Duration.ofSeconds(2),
864+
Duration.ofSeconds(2),
865+
Duration.ofSeconds(1),
866+
registrationSecret,
867+
5);
868+
LocalDistributor distributor =
869+
new LocalDistributor(
870+
tracer,
871+
bus,
872+
new PassthroughHttpClient.Factory(localNode),
873+
new LocalSessionMap(tracer, bus),
874+
queue,
875+
new DefaultSlotSelector(),
876+
registrationSecret,
877+
Duration.ofMinutes(5),
878+
true, // Enable rejectUnsupportedCaps
879+
Duration.ofSeconds(5),
880+
newSessionThreadPoolSize,
881+
new DefaultSlotMatcher(),
882+
Duration.ofSeconds(30));
883+
884+
// Create a node that only supports Chrome
885+
URI nodeUri = new URI("http://example:1234");
886+
Node node =
887+
LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret)
888+
.add(
889+
new ImmutableCapabilities("browserName", "chrome"),
890+
new TestSessionFactory(
891+
(id, c) ->
892+
new Session(id, nodeUri, new ImmutableCapabilities(), c, Instant.now())))
893+
.build();
894+
distributor.add(node);
895+
896+
// Add a Firefox request to the queue (unsupported capability)
897+
SessionRequest unsupportedRequest =
898+
new SessionRequest(
899+
new RequestId(UUID.randomUUID()),
900+
Instant.now(),
901+
Set.of(W3C),
902+
Set.of(new ImmutableCapabilities("browserName", "firefox")),
903+
Map.of(),
904+
Map.of());
905+
queue.addToQueue(unsupportedRequest);
906+
907+
// Wait for checkMatchingSlot to run and reject the request
908+
wait.until(obj -> queue.getQueueContents().isEmpty());
909+
910+
// The request should be rejected and removed from queue
911+
assertThat(queue.getQueueContents()).isEmpty();
912+
}
913+
780914
@Test
781915
void shouldHandleAllNodesFullyOccupied() throws URISyntaxException {
782916
// Create a distributor

0 commit comments

Comments
 (0)