5390: [TEST]: Replace switch validation with switch synchronization w…

…here possible Implements #5390 * Added methods synchronizeAndCollectFixedDiscrepancies(), validateAndCollectFoundDiscrepancies() into SwitchHelper * Replaced validation calls with synchronization ones where possible (where it doesn't break the test's logic) * Replaced synchronous validation/synchronization calls with asynchronous ones * Replaced direct northbound calls (validate/synchronize) with SwitchHelper methods
telstra · Feb 1, 2024 · 4e74860 · 4e74860
1 parent 4e43c7b
commit 4e74860
Show file tree

Hide file tree

Showing 50 changed files with 573 additions and 1,068 deletions.
diff --git a/...nal-tests/src/main/groovy/org/openkilda/functionaltests/extension/env/EnvExtension.groovy b/...nal-tests/src/main/groovy/org/openkilda/functionaltests/extension/env/EnvExtension.groovy
@@ -1,7 +1,6 @@
 package org.openkilda.functionaltests.extension.env
 
 import static groovyx.gpars.GParsExecutorsPool.withPool
-import static org.openkilda.model.MeterId.MAX_SYSTEM_RULE_METER_ID
 import static org.openkilda.testing.Constants.SWITCHES_ACTIVATION_TIME
 import static org.openkilda.testing.Constants.TOPOLOGY_DISCOVERING_TIME
 import static org.openkilda.testing.Constants.WAIT_OFFSET
@@ -15,7 +14,6 @@ import org.openkilda.functionaltests.helpers.Wrappers
 import org.openkilda.messaging.info.event.IslChangeType
 import org.openkilda.messaging.info.event.SwitchChangeType
 import org.openkilda.messaging.model.system.FeatureTogglesDto
-import org.openkilda.messaging.model.system.KildaConfigurationDto
 import org.openkilda.model.FlowEncapsulationType
 import org.openkilda.model.SwitchFeature
 import org.openkilda.northbound.dto.v1.links.LinkParametersDto
@@ -235,22 +233,7 @@ class EnvExtension extends AbstractGlobalExtension implements SpringContextListe
     }
 
     Closure noExcessRulesMeters = { TopologyDefinition topologyDefinition ->
-        def excessRulesAssertions = new SoftAssertions()
-        withPool {
-            (topologyDefinition.activeSwitches).eachParallel { sw ->
-                def rules = northbound.validateSwitchRules(sw.dpId)
-                excessRulesAssertions.checkSucceeds { assert rules.excessRules.empty, sw }
-                excessRulesAssertions.checkSucceeds { assert rules.missingRules.empty, sw }
-                if (!sw.virtual && sw.ofVersion != "OF_12") {
-                    excessRulesAssertions.checkSucceeds {
-                        assert northbound.getAllMeters(sw.dpId).meterEntries.findAll {
-                            it.meterId > MAX_SYSTEM_RULE_METER_ID
-                        }.isEmpty(), "Switch has meters above system max ones"
-                    }
-                }
-            }
-            excessRulesAssertions.verify()
-        }
+        switchHelper.synchronizeAndCollectFixedDiscrepancies(topologyDefinition.activeSwitches*.getDpId())
     }
 
     Closure allSwitchesConnectedToExpectedRegion = { TopologyDefinition topologyDefinition ->

diff --git a/...unctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/SwitchHelper.groovy b/...unctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/SwitchHelper.groovy
@@ -184,11 +184,6 @@ class SwitchHelper {
         database.get().getSwitch(sw.dpId).features
     }
 
-    static void synchronize(Switch sw) {
-        northbound.get().synchronizeSwitch(sw.dpId, true)
-        assert northboundV2.get().validateSwitch(sw.dpId).asExpected
-    }
-
     static List<Long> getDefaultCookies(Switch sw) {
         def swProps = northbound.get().getSwitchProperties(sw.dpId)
         def multiTableRules = []
@@ -564,18 +559,6 @@ class SwitchHelper {
         }
     }
 
-    /**
-     * Verifies that specified logical port sections in the validation response are empty.
-     */
-    static void verifyLogicalPortsSectionsAreEmpty(SwitchValidationExtendedResult switchValidateInfo,
-                                                   List<String> sections = ["missing", "excess", "misconfigured"]) {
-        def assertions = new SoftAssertions()
-        sections.each { String section ->
-            assertions.checkSucceeds { assert switchValidateInfo.logicalPorts."$section".empty }
-        }
-        assertions.verify()
-    }
-
     static SwitchProperties getDummyServer42Props() {
         return new SwitchProperties(true, 33, "00:00:00:00:00:00", 1, null)
     }
@@ -653,12 +636,6 @@ class SwitchHelper {
         reviveSwitch(sw, flResourceAddress, false)
     }
 
-    static void removeExcessRules(List<SwitchId> switches) {
-        withPool {
-            switches.eachParallel {northbound.get().synchronizeSwitch(it, true)}
-        }
-    }
-
     static void verifySectionInSwitchValidationInfo(SwitchValidationV2ExtendedResult switchValidateInfo,
                                                     List<String> sections = ["groups", "meters", "logical_ports", "rules"]) {
         sections.each { String section ->
@@ -678,32 +655,81 @@ class SwitchHelper {
         assert result == switchValidateInfo.asExpected
     }
 
-    static SwitchSyncResult synchronize(SwitchId switchId) {
-        return northbound.get().synchronizeSwitch(switchId, true)
+    static SwitchSyncResult synchronize(SwitchId switchId, boolean removeExcess=true) {
+        return northbound.get().synchronizeSwitch(switchId, removeExcess)
     }
 
-    static void synchronize(List<SwitchId> switchesToSynchronize) {
-        def synchronizationResult = withPool {
-            switchesToSynchronize.collectParallel { synchronize(it) }
+    /**
+     * Synchronizes each switch from the list and returns a map of SwitchSyncResults, where the key is
+     * SwitchId and the value is result of synchronization if there were entries which had to be fixed.
+     * I.e. if all the switches were in expected state, then empty list is returned. If there were only
+     * two switches in unexpected state, than resulting map will have only two entries, etc.
+     * @param switchesToSynchronize SwitchIds which should be synchronized
+     * @return Map of SwitchIds and SwitchSyncResults for switches which weren't in expected state before
+     * the synchronization
+     */
+    static Map<SwitchId, SwitchSyncResult> synchronizeAndCollectFixedDiscrepancies(List<SwitchId> switchesToSynchronize) {
+        return withPool {
+            switchesToSynchronize.collectParallel { [it, northbound.get().synchronizeSwitch(it, true)] }
+            .collectEntries { [(it[0]): it[1]] }
                     .findAll {
-                        !(it.getRules().getExcess().isEmpty() ||
-                                it.getRules().getMisconfigured().isEmpty() ||
-                                it.getRules().getMissing().isEmpty())
+                        [it.getValue().getRules().getMissing(),
+                        it.getValue().getRules().getMisconfigured(),
+                        it.getValue().getRules().getExcess(),
+                        it.getValue().getMeters().getMissing(),
+                        it.getValue().getMeters().getMisconfigured(),
+                        it.getValue().getMeters().getExcess()].any {!it.isEmpty()}
                     }
         }
-        assert synchronizationResult.isEmpty()
+    }
+
+    /**
+     * Synchronizes the switch and returns an optional SwitchSyncResult if the switch was in an unexpected state
+     * before the synchronization.
+     * @param switchToSynchronize SwitchId to synchronize
+     * @return optional SwitchSyncResult if the switch was in an unexpected state
+     * before the synchronization
+     */
+    static Optional<SwitchSyncResult> synchronizeAndCollectFixedDiscrepancies(SwitchId switchToSynchronize) {
+        return Optional.ofNullable(synchronizeAndCollectFixedDiscrepancies([switchToSynchronize]).get(switchToSynchronize))
+    }
+
+    /**
+     * Alias for the method for the same name but accepts Set instead of List
+     * @param switchesToSynchronize
+     * @return
+     */
+    static Map<SwitchId, SwitchSyncResult> synchronizeAndCollectFixedDiscrepancies(Set<SwitchId> switchesToSynchronize) {
+        return synchronizeAndCollectFixedDiscrepancies(switchesToSynchronize as List)
+    }
+
+    static Map<SwitchId, SwitchValidationV2ExtendedResult> validateAndCollectFoundDiscrepancies(List<SwitchId> switchesToValidate) {
+        return withPool {
+            switchesToValidate.collectParallel { [it, northboundV2.get().validateSwitch(it)] }
+                    .collectEntries { [(it[0]): it[1]] }
+                    .findAll { !it.getValue().isAsExpected() }
+        }
+    }
+
+    static Optional<SwitchValidationV2ExtendedResult> validateAndCollectFoundDiscrepancies(SwitchId switchToValidate) {
+        return Optional.ofNullable(validateAndCollectFoundDiscrepancies([switchToValidate]).get(switchToValidate))
     }
 
     static void synchronizeAndValidateRulesInstallation(Switch srcSwitch, Switch dstSwitch) {
-        synchronize([srcSwitch.dpId, dstSwitch.dpId])
+        synchronizeAndCollectFixedDiscrepancies([srcSwitch.dpId, dstSwitch.dpId])
         [srcSwitch, dstSwitch].each { sw ->
             Wrappers.wait(RULES_INSTALLATION_TIME) {
                 validate(sw.dpId).verifyRuleSectionsAreEmpty()
             }
         }
     }
-    static SwitchValidationV2ExtendedResult validate(SwitchId switchId) {
-        return northboundV2.get().validateSwitch(switchId)
+
+    static SwitchValidationV2ExtendedResult validate(SwitchId switchId, String include = null, String exclude = null) {
+        return northboundV2.get().validateSwitch(switchId, include, exclude)
+    }
+
+    static SwitchValidationExtendedResult validateV1(SwitchId switchId) {
+        return northbound.get().validateSwitch(switchId)
     }
 
     static List<SwitchValidationV2ExtendedResult> validate(List<SwitchId> switchesToValidate) {

diff --git a/...ional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/model/SwitchPair.groovy b/...ional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/model/SwitchPair.groovy
@@ -46,6 +46,11 @@ class SwitchPair {
         return pathsFromApi.collect {new Path(it, topologyDefinition)}
     }
 
+    @JsonIgnore
+    List<Switch> toList() {
+        return [src, dst]
+    }
+
     @JsonIgnore
     SwitchPair getReversed() {
         new SwitchPair(src: dst,

diff --git a/...g/functional-tests/src/test/groovy/org/openkilda/functionaltests/BaseSpecification.groovy b/...g/functional-tests/src/test/groovy/org/openkilda/functionaltests/BaseSpecification.groovy
@@ -115,12 +115,6 @@ class BaseSpecification extends Specification {
                 "but current active profile is '${this.profile}'")
     }
 
-    void verifySwitchRules(SwitchId switchId) {
-        def rules = northbound.validateSwitchRules(switchId)
-        assert rules.excessRules.empty
-        assert rules.missingRules.empty
-    }
-
     // this cleanup section should be removed after fixing the issue https://github.com/telstra/open-kilda/issues/5480
     void deleteAnyFlowsLeftoversIssue5480() {
         Wrappers.benchmark("Deleting flows leftovers") {

diff --git a/...al-tests/src/test/groovy/org/openkilda/functionaltests/listeners/DoCleanupListener.groovy b/...al-tests/src/test/groovy/org/openkilda/functionaltests/listeners/DoCleanupListener.groovy
@@ -1,6 +1,7 @@
 package org.openkilda.functionaltests.listeners
 
-import static groovyx.gpars.GParsPool.withPool
+import org.openkilda.functionaltests.helpers.SwitchHelper
+
 import static org.openkilda.functionaltests.extension.tags.Tag.ISL_PROPS_DB_RESET
 import static org.openkilda.functionaltests.extension.tags.Tag.ISL_RECOVER_ON_FAIL
 import static org.openkilda.functionaltests.helpers.Wrappers.wait
@@ -33,18 +34,17 @@ class DoCleanupListener extends AbstractSpringListener {
     @Autowired
     PortAntiflapHelper antiflap
 
+    @Autowired
+    SwitchHelper switchHelper
+
 
     @Override
     void error(ErrorInfo error) {
         context && context.autowireCapableBeanFactory.autowireBean(this)
         def thrown = error.exception
         if (thrown instanceof AssertionError) {
             if (thrown.getMessage() && thrown.getMessage().contains("SwitchValidationExtendedResult(")) {
-                withPool {
-                    topology.activeSwitches.eachParallel { sw ->
-                        northbound.synchronizeSwitch(sw.dpId, true)
-                    }
-                }
+                switchHelper.synchronizeAndCollectFixedDiscrepancies(topology.activeSwitches*.dpId)
             }
         }
         if (error.method.name && ISL_PROPS_DB_RESET in error.method.getAnnotation(Tags)?.value()) {

diff --git a/...ests/src/test/groovy/org/openkilda/functionaltests/spec/flows/ConnectedDevicesSpec.groovy b/...ests/src/test/groovy/org/openkilda/functionaltests/spec/flows/ConnectedDevicesSpec.groovy
@@ -5,7 +5,6 @@ import static org.openkilda.functionaltests.extension.tags.Tag.HARDWARE
 import static org.openkilda.functionaltests.extension.tags.Tag.SMOKE
 import static org.openkilda.functionaltests.extension.tags.Tag.SMOKE_SWITCHES
 import static org.openkilda.functionaltests.extension.tags.Tag.TOPOLOGY_DEPENDENT
-import static org.openkilda.functionaltests.helpers.Wrappers.wait
 import static org.openkilda.model.MeterId.createMeterIdForDefaultRule
 import static org.openkilda.model.SwitchFeature.KILDA_OVS_PUSH_POP_MATCH_VXLAN
 import static org.openkilda.model.cookie.Cookie.LLDP_INGRESS_COOKIE
@@ -1234,9 +1233,8 @@ srcDevices=#newSrcEnabled, dstDevices=#newDstEnabled"() {
         flowHelperV2.updateFlow(flow.flowId, flow)
 
         then: "Check excess rules are not registered on device"
-        wait(WAIT_OFFSET) {
-             verifySwitchRules(sw.dpId)
-        }
+        !switchHelper.synchronizeAndCollectFixedDiscrepancies(sw.dpId).isPresent()
+
         cleanup: "Remove created flow and registered devices, revert switch props"
         flow && flowHelperV2.deleteFlow(flow.flowId)
         sw && database.removeConnectedDevices(sw.dpId)
@@ -1424,15 +1422,9 @@ srcDevices=#newSrcEnabled, dstDevices=#newDstEnabled"() {
     }
 
     private void validateFlowAndSwitches(Flow flow) {
-        northbound.validateFlow(flow.flowId).each { assert it.asExpected }
-        [flow.srcSwitch, flow.destSwitch].each {
-            def validation = northbound.validateSwitch(it.switchId)
-            validation.verifyRuleSectionsAreEmpty(["missing", "excess", "misconfigured"])
-            validation.verifyHexRuleSectionsAreEmpty(["missingHex", "excessHex", "misconfiguredHex"])
-            if (it.ofVersion != "OF_12") {
-                validation.verifyMeterSectionsAreEmpty(["missing", "misconfigured", "excess"])
-            }
-        }
+        assert northbound.validateFlow(flow.flowId).each { assert it.asExpected }
+        assert switchHelper.synchronizeAndCollectFixedDiscrepancies([flow.srcSwitch.getSwitchId(), flow.destSwitch.getSwitchId()])
+                .isEmpty()
     }
 
     private void waitForSrcDevicesInputRules(Flow flow) {