From b88b24375ccf189dbb4765edca970ed73ea16ff9 Mon Sep 17 00:00:00 2001 From: Sergey Nikitin Date: Sun, 11 Dec 2022 18:42:29 +0400 Subject: [PATCH 1/2] Improved PCE performance Do not use path if its penalties higher than best path penalties Closes #5010 --- .../BestWeightAndShortestPathFinder.java | 65 ++++++++++++++--- .../impl/InMemoryPathComputerBaseTest.java | 25 +++++++ ...atencyPathComputationStrategyBaseTest.java | 71 +++++++++++++++++++ 3 files changed, 150 insertions(+), 11 deletions(-) diff --git a/src-java/kilda-pce/src/main/java/org/openkilda/pce/finder/BestWeightAndShortestPathFinder.java b/src-java/kilda-pce/src/main/java/org/openkilda/pce/finder/BestWeightAndShortestPathFinder.java index d170d58edeb..51d31cf711e 100644 --- a/src-java/kilda-pce/src/main/java/org/openkilda/pce/finder/BestWeightAndShortestPathFinder.java +++ b/src-java/kilda-pce/src/main/java/org/openkilda/pce/finder/BestWeightAndShortestPathFinder.java @@ -368,8 +368,10 @@ private List 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 @@ -386,7 +388,9 @@ private List 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; } @@ -398,14 +402,18 @@ private List 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. @@ -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; } @@ -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; } @@ -502,7 +529,9 @@ 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; } } @@ -510,11 +539,16 @@ private SearchNode getDesiredPath(Node start, Node end, WeightFunction weightFun // 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() @@ -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 diff --git a/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerBaseTest.java b/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerBaseTest.java index 4cce55a01fa..74518e2f162 100644 --- a/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerBaseTest.java +++ b/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/InMemoryPathComputerBaseTest.java @@ -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); + } } diff --git a/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/MaxLatencyPathComputationStrategyBaseTest.java b/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/MaxLatencyPathComputationStrategyBaseTest.java index 7885c1ea7b6..b971f976afe 100644 --- a/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/MaxLatencyPathComputationStrategyBaseTest.java +++ b/src-java/kilda-pce/src/test/java/org/openkilda/pce/impl/MaxLatencyPathComputationStrategyBaseTest.java @@ -25,6 +25,10 @@ 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; @@ -32,20 +36,57 @@ 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. */ @@ -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; + } } From abf312d86116f8320314bc60f815bce6c6ca8e21 Mon Sep 17 00:00:00 2001 From: Sergey Nikitin Date: Mon, 12 Dec 2022 05:21:45 +0400 Subject: [PATCH 2/2] Updated CHANGELOG.md (hotfix-1.126.1) --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b657215da1..ce3a32795fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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: