Skip to content

Commit

Permalink
[grid] Simplifying DefaultSlotSelector logic
Browse files Browse the repository at this point in the history 
The idea has always been to keep as available
as possible the Nodes with diverse configurations.
Such as the ones supporting different browsers,
like a Node supporting Chrome, Firefox and Safari.

But if we have a Node that only supports Chrome,
we'd like to give as many Chrome sessions as possible
to keep the one with Safari as available as possible,
so when a Safari session request comes in, it can be
served right away.

The previous logic had that in mind, but the
implementation was rather complex. This simplified
version orders Nodes by the number of browsers they
support. Therefore, it would offer "specialized"
Nodes first (e.g. only supporting one browser), and
the more diverse ones at the bottom.
  • Loading branch information
@diemol
diemol committed Dec 24, 2020
1 parent b4b7674 commit 4a3f3ef
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 221 deletions.
8 changes: 4 additions & 4 deletions java/server/src/org/openqa/selenium/grid/distributor/Distributor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ protected Distributor(
.with(new SpanDecorator(tracer, req -> "distributor.status")));
}

public Either<SessionNotCreatedException, CreateSessionResponse> newSession(
HttpRequest request) throws SessionNotCreatedException {
public Either<SessionNotCreatedException, CreateSessionResponse> newSession(HttpRequest request)
throws SessionNotCreatedException {

Span span = newSpanAsChildOf(tracer, request, "distributor.create_session_response");
Map<String, EventAttributeValue> attributeMap = new HashMap<>();
Expand Down Expand Up @@ -199,7 +199,7 @@ public Either<SessionNotCreatedException, CreateSessionResponse> newSession(

if (!hostsWithCaps) {
String errorMessage = String.format(
"No host supports the capabilities required: %s",
"No Node supports the required capabilities: %s",
payload.stream().map(Capabilities::toString).collect(Collectors.joining(", ")));
SessionNotCreatedException exception = new SessionNotCreatedException(errorMessage);
span.setAttribute(AttributeKey.ERROR.getKey(), true);
Expand All @@ -212,7 +212,7 @@ public Either<SessionNotCreatedException, CreateSessionResponse> newSession(
return Either.left(exception);
}

// Find a host that supports the capabilities present in the new session
// Find a Node that supports the capabilities present in the new session
Set<SlotId> slotIds = slotSelector.selectSlot(firstRequest.getCapabilities(), model);
if (!slotIds.isEmpty()) {
selected = Optional.of(reserve(slotIds.iterator().next(), firstRequest));
Expand Down
131 changes: 17 additions & 114 deletions java/server/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,38 +24,27 @@
import org.openqa.selenium.grid.data.SlotId;

import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static com.google.common.collect.ImmutableSet.toImmutableSet;

public class DefaultSlotSelector implements SlotSelector {

private static final Logger LOG = Logger.getLogger(DefaultSlotSelector.class.getName());

@Override
public Set<SlotId> selectSlot(Capabilities capabilities, Set<NodeStatus> nodes) {
Stream<NodeStatus> firstRound = nodes.stream()
// Find a node that supports this kind of thing
.filter(node -> node.hasCapacity(capabilities));

// of the nodes that survived the first round, separate into buckets and prioritize by browser "rarity"
Stream<NodeStatus> prioritizedNodes = getPrioritizedNodeStream(firstRound, capabilities);

//Take the further-filtered Stream and prioritize by load, then by session age

return prioritizedNodes
// First, filter the Nodes that support the required capabilities. Then, the filtered Nodes
// get ordered in ascendant order by the number of browsers they support.
// With this, Nodes with diverse configurations (supporting many browsers, e.g. Chrome,
// Firefox, Safari) are placed at the bottom so they have more availability when a session
// requests a browser supported only by a few Nodes (e.g. Safari only supported on macOS
// Nodes).
// After that, Nodes are ordered by their load, last session creation, and their id.
return nodes.stream()
.filter(node -> node.hasCapacity(capabilities))
.sorted(
Comparator.comparingLong(this::getNumberOfSupportedBrowsers)
// Now sort by node which has the lowest load (natural ordering)
Comparator.comparingDouble(NodeStatus::getLoad)
.thenComparingDouble(NodeStatus::getLoad)
// Then last session created (oldest first), so natural ordering again
.thenComparingLong(NodeStatus::getLastSessionCreated)
// And use the node id as a tie-breaker.
Expand All @@ -67,99 +56,13 @@ public Set<SlotId> selectSlot(Capabilities capabilities, Set<NodeStatus> nodes)
.collect(toImmutableSet());
}

/**
* Takes a Stream of NodeStatus, along with the Capabilities of the current request, and prioritizes the
* request by removing NodeStatus that offer Capabilities that are more rare. e.g. if there are only a
* couple Edge nodes, but a lot of Chrome nodes, the Edge nodes should be removed from
* consideration when Chrome is requested. This does not currently take the amount of load on the
* server into consideration--it only checks for availability, not how much availability
*
* @param nodes Stream of nodestatus attached to the Distributor (assume it's filtered for only those that offer these Capabilities)
* @param capabilities Passing in the whole Capabilities object will allow us to prioritize more than just browser
* @return Stream of distinct NodeStatus with the more rare Capabilities removed
*/
@VisibleForTesting
Stream<NodeStatus> getPrioritizedNodeStream(Stream<NodeStatus> nodes, Capabilities capabilities) {
//TODO for the moment, we're not going to operate on the Stream that was passed in--we need to
// alter and futz with the contents, so the stream isn't the right place to operate. This
// will likely be optimized back into the algo, but not yet
Set<NodeStatus> filteredNodeSet = nodes.collect(Collectors.toSet());

//A "bucket" is a list of nodes that can use a particular browser. The "edge" bucket is the
// complete list of nodes that support "edge". By separating nodes into buckets, we will
// know which browsers have fewer nodes available for the browsers we're not interested in, and
// can prioritize based on the browsers that have more availability
Map<String, Set<NodeStatus>> nodeBuckets = sortNodesToBucketsByBrowser(filteredNodeSet);

//First, check to see if all buckets are the same size. If they are, just send back the full list of nodes
// (i.e. the nodes are all "balanced" with regard to browser priority)
if (allBucketsSameSize(nodeBuckets)) {
return nodeBuckets.values().stream().distinct().flatMap(Set::stream);
}

//Then, starting with the smallest bucket that isn't the current browser being prioritized,
// remove all nodes in that bucket from consideration, then rebuild the buckets. Then do the
// "same size" check again, and keep doing this until either a) there is only one bucket, or b)
// all buckets are the same size

//Note: there should never be a case where a bucket will have *more* nodes available for the
// given browser than the one being requested. The first filter in this check looks for
// "equal", not "equal-or-greater" as a result of this assumption

//There might be unforeseen cases that challenge this assumption. TODO Create unit tests to prove it

//TODO a List of Map.Entry is silly. whatever this structure needs to be needs to be returned by
// the sortHostsToBucketsByBrowser method in a way that we don't have to sort it separately like this
final List<Map.Entry<String, Set<NodeStatus>>> sorted = nodeBuckets.entrySet().stream().sorted(
Comparator.comparingInt(v -> v.getValue().size())
).collect(Collectors.toList());

// Until the buckets are the same size, keep removing nodes that have more "rare" browser capabilities
Map<String, Set<NodeStatus>> newNodeBuckets;
for (Map.Entry<String, Set<NodeStatus>> entry : sorted) {
//Don't examine the bucket containing the browser in question--we're prioritizing the other browsers
//TODO This shouldn't be necessary, because if the list is sorted by size, this won't be possible until
// they're all the same size. Create a unit test to prove it
if (entry.getKey().equals(capabilities.getBrowserName())) {
continue;
}

//Remove all nodes from this bucket from the full set of eligible nodes
final Set<NodeStatus> filteredNodes = filteredNodeSet.stream()
.filter(node -> !entry.getValue().contains(node))
.collect(Collectors.toSet());

//Rebuild the buckets by browser
newNodeBuckets = sortNodesToBucketsByBrowser(filteredNodes);

//Check the bucket sizes--if they're the same, then we're done
if (allBucketsSameSize(newNodeBuckets)) {
LOG.fine("Nodes have been balanced according to browser priority");
return newNodeBuckets.values().stream().distinct().flatMap(Set::stream);
}
}

return nodeBuckets.values().stream().distinct().flatMap(Set::stream);
}

Map<String, Set<NodeStatus>> sortNodesToBucketsByBrowser(Set<NodeStatus> nodes) {
//Make a hash of browserType -> list of nodes that support it
Map<String, Set<NodeStatus>> buckets = new HashMap<>();

for (NodeStatus node : nodes) {
for (Slot slot : node.getSlots()) {
String name = Optional.ofNullable(slot.getStereotype().getBrowserName()).orElse("");
buckets.computeIfAbsent(name, n -> new LinkedHashSet<>()).add(node);
}
}

return buckets;
long getNumberOfSupportedBrowsers(NodeStatus nodeStatus) {
return nodeStatus.getSlots()
.stream()
.map(slot -> slot.getStereotype().getBrowserName().toLowerCase())
.distinct()
.count();
}

@VisibleForTesting
boolean allBucketsSameSize(Map<String, Set<NodeStatus>> buckets) {
Set<Integer> intSet = new HashSet<>();
buckets.values().forEach(bucket -> intSet.add(bucket.size()));
return intSet.size() == 1;
}
}
Loading

0 comments on commit 4a3f3ef

Please sign in to comment.