Skip to content

Commit

Permalink
Merge branch 'hotfix-1.126.1'
Browse files Browse the repository at this point in the history
  • Loading branch information
@niksv
niksv committed Dec 12, 2022
2 parents 7bfb19d + abf312d commit 01f0867
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 11 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
# Changelog

## v1.126.1 (12/12/2022)

### Bug Fixes:
- [#5011](https://github.com/telstra/open-kilda/pull/5011) Improved PCE performance (Issue: [#5010](https://github.com/telstra/open-kilda/issues/5010))

For the complete list of changes, check out [the commit log](https://github.com/telstra/open-kilda/compare/v1.126.0...v1.126.1).

### Affected Components:
flow-hs

---

## v1.126.0 (21/11/2022)

### Features:
Expand Down
65 changes: 54 additions & 11 deletions ...ava/kilda-pce/src/main/java/org/openkilda/pce/finder/BestWeightAndShortestPathFinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,10 @@ private List<Edge> getPath(Node start, Node end, WeightFunction weightFunction)

while (!toVisit.isEmpty()) {
SearchNode current = toVisit.pop();
log.trace("Going to visit node {} with weight {}.",
current.dstSw, current.getParentWeight().getTotalWeight());
if (log.isTraceEnabled()) {
log.trace("Going to visit node {} with weight {}.",
current.dstSw, current.getParentWeight().getTotalWeight());
}

if (isContainHardDiversityPenalties(current.parentWeight)) {
// Have not to use path with hard diversity penalties
Expand All @@ -386,7 +388,9 @@ private List<Edge> getPath(Node start, Node end, WeightFunction weightFunction)
bestPath = current;
}
// We found dest, no need to keep processing
log.trace("Found destination using {} with path {}", current.dstSw, current.parentPath);
if (log.isTraceEnabled()) {
log.trace("Found destination using {} with path {}", current.dstSw, current.parentPath);
}
continue;
}

Expand All @@ -398,14 +402,18 @@ private List<Edge> getPath(Node start, Node end, WeightFunction weightFunction)

// Stop processing entirely if we've gone too far, or over bestWeight
if (current.allowedDepth <= 0 || current.parentWeight.compareTo(bestWeight) > 0) {
log.trace("Skip node {} processing", current.dstSw);
if (log.isTraceEnabled()) {
log.trace("Skip node {} processing", current.dstSw);
}
continue;
}

// Either this is the first time, or this one has less weight .. either way, this node should
// be the one in the visited list
visited.put(current.dstSw, current);
log.trace("Save new path to node {} and process it's outgoing links", current.dstSw);
if (log.isTraceEnabled()) {
log.trace("Save new path to node {} and process it's outgoing links", current.dstSw);
}

// At this stage .. haven't found END, haven't gone too deep, and we are not over weight.
// So, add the outbound isls.
Expand Down Expand Up @@ -446,11 +454,14 @@ private SearchNode getDesiredPath(Node start, Node end, WeightFunction weightFun
while (!toVisit.isEmpty()) {
SearchNode current = toVisit.pop();
PathWeight currentPathWeight = current.getParentWeight();
log.trace("Going to visit node {} with weight {}.", current.dstSw, currentPathWeight);

if (log.isTraceEnabled()) {
log.trace("Going to visit node {} with weight {}.", current.dstSw, currentPathWeight);
}
// Leave if the path contains this node
if (current.containsSwitch(current.dstSw.getSwitchId())) {
log.trace("Skip node {} already in the path", current.dstSw);
if (log.isTraceEnabled()) {
log.trace("Skip node {} already in the path", current.dstSw);
}
continue;
}

Expand All @@ -470,23 +481,39 @@ private SearchNode getDesiredPath(Node start, Node end, WeightFunction weightFun

if (desiredPath == null) {
desiredPath = current;
log.trace("Found first path to node {} with base weight {} and penalties {}",
current.dstSw, current.parentWeight.getBaseWeight(),
current.parentWeight.getPenaltiesWeight());
continue;
}

if (currentPathWeight.getPenaltiesWeight() < desiredPath.parentWeight.getPenaltiesWeight()) {
if (log.isTraceEnabled()) {
log.trace("Found path with best penalties {} instead of {} penalties. Base weight is {}",
currentPathWeight.getPenaltiesWeight(),
desiredPath.parentWeight.getPenaltiesWeight(),
current.parentWeight.getBaseWeight());
}
desiredPath = current;
continue;
}

if (currentPathWeight.getPenaltiesWeight() == desiredPath.parentWeight.getPenaltiesWeight()
&& currentPathWeight.getBaseWeight() > desiredPath.parentWeight.getBaseWeight()) {
if (log.isTraceEnabled()) {
log.trace("Found path with same penalties {}, but different weight {} instead of {}",
currentPathWeight.getPenaltiesWeight(), currentPathWeight.getBaseWeight(),
desiredPath.parentWeight.getBaseWeight());
}
desiredPath = current;
continue;
}
}

// We found dest, no need to keep processing
log.trace("Found destination using {} with path {}", current.dstSw, current.parentPath);
if (log.isTraceEnabled()) {
log.trace("Found destination using {} with path {}", current.dstSw, current.parentPath);
}
continue;
}

Expand All @@ -502,19 +529,26 @@ private SearchNode getDesiredPath(Node start, Node end, WeightFunction weightFun

// Should check with penalties too
if (currentPathWeight.getTotalWeight() > prior.getParentWeight().getTotalWeight()) {
log.trace("Skip node {} processing", current.dstSw);
if (log.isTraceEnabled()) {
log.trace("Skip node {} processing", current.dstSw);
}
continue;
}
}

// Either this is the first time, or this one has less weight .. either way, this node should
// be the one in the visited list
visited.put(current.dstSw, current);
log.trace("Save new path to node {} and process it's outgoing links", current.dstSw);
if (log.isTraceEnabled()) {
log.trace("Save new path to node {} and process it's outgoing links", current.dstSw);
}

// At this stage .. haven't found END, haven't gone too deep, and we are not over weight.
// So, add the outbound isls.
PathWeight desiredWeight = desiredPath == null ? null : desiredPath.getParentWeight();
current.dstSw.getOutgoingLinks().stream()
// We can skip path if its penalties are higher than bestPath penalties
.filter(edge -> filterEdgeByPenalties(edge, current.parentWeight, desiredWeight, weightFunction))
// should firstly process edges with big weights to guarantee they will not be skipped.
// See unit test shouldUseSlowLinkInsidePath.
.sorted(Comparator.comparing(weightFunction).reversed()
Expand All @@ -525,6 +559,15 @@ private SearchNode getDesiredPath(Node start, Node end, WeightFunction weightFun
return desiredPath;
}

private static boolean filterEdgeByPenalties(
Edge edge, PathWeight currentWeight, PathWeight desiredWeight, WeightFunction weightFunction) {
if (desiredWeight == null) {
return true;
}
PathWeight nextPathWeight = currentWeight.add(weightFunction.apply(edge));
return nextPathWeight.getPenaltiesWeight() <= desiredWeight.getPenaltiesWeight();
}

/**
* This is generally called after getPath() to find the path back. The path back could be asymmetric, but this will
* increase the odds that we return the symmetric path if it exists. The hint will be used to determine if it
Expand Down
25 changes: 25 additions & 0 deletions src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerBaseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -956,4 +956,29 @@ private FlowPath buildFlowPath(String pathIdAsString, int... ids) {
.segments(segments)
.build();
}

protected void addLink(
AvailableNetwork network, SwitchId srcSwitchId, SwitchId dstSwitchId, int srcPort, int dstPort,
long latency, int affinityCounter) {
Edge edge = Edge.builder()
.srcSwitch(network.getOrAddNode(srcSwitchId, null))
.srcPort(srcPort)
.destSwitch(network.getOrAddNode(dstSwitchId, null))
.destPort(dstPort)
.latency(latency)
.cost(1)
.availableBandwidth(500000)
.underMaintenance(false)
.unstable(false)
.affinityGroupUseCounter(affinityCounter)
.build();
network.addEdge(edge);
}

protected void addBidirectionalLink(
AvailableNetwork network, SwitchId firstSwitch, SwitchId secondSwitch, int srcPort, int dstPort,
long latency, int affinityCounter) {
addLink(network, firstSwitch, secondSwitch, srcPort, dstPort, latency, affinityCounter);
addLink(network, secondSwitch, firstSwitch, dstPort, srcPort, latency, affinityCounter);
}
}
71 changes: 71 additions & 0 deletions ...a-pce/src/test/java/org/openkilda/pce/impl/MaxLatencyPathComputationStrategyBaseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,68 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.openkilda.model.Flow;
import org.openkilda.model.FlowEncapsulationType;
import org.openkilda.model.IslStatus;
import org.openkilda.model.PathComputationStrategy;
import org.openkilda.model.Switch;
import org.openkilda.model.SwitchId;
import org.openkilda.pce.AvailableNetworkFactory;
import org.openkilda.pce.GetPathsResult;
import org.openkilda.pce.Path;
import org.openkilda.pce.PathComputer;
import org.openkilda.pce.exception.RecoverableException;
import org.openkilda.pce.exception.UnroutableFlowException;
import org.openkilda.pce.finder.BestWeightAndShortestPathFinder;
import org.openkilda.pce.model.Node;

import org.junit.Test;

import java.time.Duration;
import java.util.ArrayList;
import java.util.List;

public class MaxLatencyPathComputationStrategyBaseTest extends InMemoryPathComputerBaseTest {

@Test(timeout = 10_000)
public void shouldFindPathInHugeNetworkWithAffinity() throws UnroutableFlowException, RecoverableException {
SwitchId srcSwitchId = new SwitchId(1);
SwitchId dstSwitchId = new SwitchId(49);
AvailableNetwork network = buildHugeNetworkWithAffinity(7);
AvailableNetworkFactory mockFactory = mock(AvailableNetworkFactory.class);
when(mockFactory.getAvailableNetwork(any(Flow.class), eq(new ArrayList<>())))
.thenReturn(network);

BestWeightAndShortestPathFinder pathFinder = new BestWeightAndShortestPathFinder(15);

PathComputer pathComputer = new InMemoryPathComputer(mockFactory, pathFinder, config);
Flow flow = Flow.builder()
.flowId(TEST_FLOW_ID)
.srcSwitch(Switch.builder().switchId(new SwitchId(1)).build())
.destSwitch(Switch.builder().switchId(new SwitchId(49)).build())
.encapsulationType(FlowEncapsulationType.TRANSIT_VLAN)
.pathComputationStrategy(PathComputationStrategy.MAX_LATENCY)
.maxLatency(175_000_000L)
.bandwidth(10000)
.ignoreBandwidth(false)
.build();
GetPathsResult result = pathComputer.getPath(flow);

assertEquals(6, result.getForward().getSegments().size());
assertEquals(srcSwitchId, result.getForward().getSegments().get(0).getSrcSwitchId());
assertEquals(new SwitchId(9), result.getForward().getSegments().get(1).getSrcSwitchId());
assertEquals(new SwitchId(17), result.getForward().getSegments().get(2).getSrcSwitchId());
assertEquals(new SwitchId(25), result.getForward().getSegments().get(3).getSrcSwitchId());
assertEquals(new SwitchId(33), result.getForward().getSegments().get(4).getSrcSwitchId());
assertEquals(new SwitchId(41), result.getForward().getSegments().get(5).getSrcSwitchId());
assertEquals(dstSwitchId, result.getForward().getSegments().get(5).getDestSwitchId());
}

/**
* Special case: flow with MAX_LATENCY strategy and 'max-latency' set to 0 should pick path with least latency.
*/
Expand Down Expand Up @@ -321,4 +362,34 @@ private void createThreeWaysTopo() {
createBiIsl(nodeD, nodeE, IslStatus.ACTIVE, IslStatus.ACTIVE, 10, 1000, 6, 30L);
}

/**
* Creates topology where each not connected to each node from previous row.
* All edges, except diagonal edges from top left to bottom right has affinityCounter = 1.
* Main diagonal has affinityCounter = 0.
*/
private AvailableNetwork buildHugeNetworkWithAffinity(int size) {
long latency = 1000000L;
AvailableNetwork network = new AvailableNetwork();
Node[][] nodeField = new Node[size][size];

int switchCount = 1;
int portCount = 1;
for (int line = 0; line < size; line++) {
for (int row = 0; row < size; row++) {
nodeField[line][row] = network.getOrAddNode(new SwitchId(switchCount++), null);
if (line == 0) {
continue;
}

for (int i = 0; i < size; i++) {
int affinityCounter = row == line ? 0 : 1;
addBidirectionalLink(
network, nodeField[line][row].getSwitchId(), nodeField[line - 1][i].getSwitchId(),
portCount++, portCount++, latency++, affinityCounter);
}

}
}
return network;
}
}

0 comments on commit 01f0867

Please sign in to comment.