Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functional/preemptive flow remove #1425

Merged
merged 2 commits into from Oct 11, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 11 additions & 4 deletions services/src/atdd-staging/src/main/resources/log4j2.xml
Expand Up @@ -4,16 +4,23 @@
<Console name="STDOUT" target="SYSTEM_OUT">
<PatternLayout pattern="%d{ISO8601} %-5p %c{1}:%L - %m%n"/>
</Console>
<File name="File" fileName="target/logs/request_logs.log">
<File name="RequestLogsFile" fileName="target/logs/request_logs.log">
<PatternLayout pattern="%d{ISO8601} %-5p %c{1}:%L - %m%n"/>
</File>
<File name="KildaLogFile" fileName="target/logs/logs.log">
<PatternLayout pattern="%d{ISO8601} %-5p %c{1}:%L - %m%n"/>
</File>
</Appenders>
<Loggers>
<Logger name="org.openkilda" level="DEBUG" additivity="false">
<AppenderRef ref="KildaLogFile" level="DEBUG"/>
<AppenderRef ref="STDOUT" level="INFO"/>
</Logger>
<Logger name="org.openkilda.testing.tools.LoggingRequestInterceptor" level="DEBUG" additivity="false">
<AppenderRef ref="File"/>
<AppenderRef ref="RequestLogsFile" level="DEBUG"/>
</Logger>
<Root level="INFO">
<AppenderRef ref="STDOUT"/>
<Root>
<AppenderRef ref="STDOUT" level="ERROR"/>
</Root>
</Loggers>
</Configuration>
Expand Up @@ -5,6 +5,7 @@ import org.openkilda.messaging.payload.flow.FlowPathPayload
import org.openkilda.northbound.dto.links.LinkPropsDto
import org.openkilda.testing.model.topology.TopologyDefinition
import org.openkilda.testing.model.topology.TopologyDefinition.Isl
import org.openkilda.testing.model.topology.TopologyDefinition.Switch
import org.openkilda.testing.service.northbound.NorthboundService
import org.openkilda.testing.tools.IslUtils

Expand Down Expand Up @@ -97,6 +98,13 @@ class PathHelper {
return pathNodes
}

/**
* Get list of Switches involved in given path.
*/
List<Switch> getInvolvedSwitches(List<PathNode> path) {
return getInvolvedIsls(path).collect { [it.srcSwitch, it.dstSwitch] }.flatten().unique()
}

/**
* Get total cost of all ISLs that are involved in a given path.
*
Expand Down
Expand Up @@ -62,11 +62,11 @@ class BandwidthSpec extends BaseSpecification {
and: "Available bandwidth on ISLs is changed in accordance with flow maximum bandwidth"
def linksAfterFlow = northboundService.getAllLinks()
def flowPath = PathHelper.convert(northboundService.getFlowPath(flow.id))
pathHelper.getInvolvedIsls(flowPath).every { link ->
[link, islUtils.reverseIsl(link)].every {
pathHelper.getInvolvedIsls(flowPath).each { link ->
[link, islUtils.reverseIsl(link)].each {
def bwBeforeFlow = islUtils.getIslInfo(linksBeforeFlow, it).get().availableBandwidth
def bwAfterFlow = islUtils.getIslInfo(linksAfterFlow, it).get().availableBandwidth
bwAfterFlow == bwBeforeFlow - maximumBandwidth
assert bwAfterFlow == bwBeforeFlow - maximumBandwidth
}
}

Expand Down
Expand Up @@ -4,22 +4,33 @@ import static org.openkilda.testing.Constants.WAIT_OFFSET

import org.openkilda.functionaltests.BaseSpecification
import org.openkilda.functionaltests.helpers.FlowHelper
import org.openkilda.functionaltests.helpers.PathHelper
import org.openkilda.functionaltests.helpers.Wrappers
import org.openkilda.messaging.payload.flow.FlowState
import org.openkilda.testing.model.topology.TopologyDefinition
import org.openkilda.testing.model.topology.TopologyDefinition.Switch
import org.openkilda.testing.service.northbound.NorthboundService
import org.openkilda.testing.service.topology.TopologyEngineService

import org.springframework.beans.factory.annotation.Autowired
import org.springframework.beans.factory.annotation.Value
import spock.lang.Unroll

class FlowsSpec extends BaseSpecification {

@Value('${reroute.delay}')
int rerouteDelay

@Autowired
TopologyDefinition topology
@Autowired
FlowHelper flowHelper
@Autowired
NorthboundService northboundService
@Autowired
TopologyEngineService topologyEngineService
@Autowired
PathHelper pathHelper

@Unroll
def "Able to create a single-switch flow for switch with #sw.ofVersion"() {
Expand Down Expand Up @@ -67,4 +78,26 @@ class FlowsSpec extends BaseSpecification {
cleanup:
flow && northboundService.deleteFlow(flow.id)
}

def "Removing flow while it is still in progress of being set up should not cause rule discrepancies"() {
given: "A potential flow"
def (Switch srcSwitch, Switch dstSwitch) = topology.activeSwitches
def flow = flowHelper.randomFlow(srcSwitch, dstSwitch)
def paths = topologyEngineService.getPaths(srcSwitch.dpId, dstSwitch.dpId)*.path
def switches = pathHelper.getInvolvedSwitches(paths.min { pathHelper.getCost(it) })

when: "Init creation of new flow"
northboundService.addFlow(flow)

and: "Immediately remove it"
northboundService.deleteFlow(flow.id)

then: "All related switches have no discrepancies in rules"
Wrappers.wait(WAIT_OFFSET) {
switches.every {
def rules = northboundService.validateSwitchRules(it.dpId)
rules.missingRules.empty && rules.excessRules.empty
}
}
}
}
Expand Up @@ -294,6 +294,37 @@ class ThrottlingRerouteSpec extends BaseSpecification {
db.countFlows() == 0
}

def "Auto-reroute on the flow which is already deleted should not revive the flow"() {
given: "A flow"
def (Switch srcSwitch, Switch dstSwitch) = topology.activeSwitches
def flow = flowHelper.randomFlow(srcSwitch, dstSwitch)
northboundService.addFlow(flow)
assert Wrappers.wait(WAIT_OFFSET) { northboundService.getFlowStatus(flow.id).status == FlowState.UP }
def path = PathHelper.convert(northboundService.getFlowPath(flow.id))

when: "Remove the flow"
northboundService.deleteFlow(flow.id)

and: "Immediately break the path to init all flows on that path to reroute"
def islToBreak = pathHelper.getInvolvedIsls(path).first()
northboundService.portDown(islToBreak.srcSwitch.dpId, islToBreak.srcPort)

then: "Flow is not present in the system after reroute timeout"
TimeUnit.SECONDS.sleep(rerouteDelay + WAIT_OFFSET)
!northboundService.getAllFlows().find { it.id == flow.id }
northboundService.getAllLinks().every { it.availableBandwidth == it.speed }

and: "No rule discrepancies observed"
topology.activeSwitches.every {
def rules = northboundService.validateSwitchRules(it.dpId)
rules.missingRules.empty && rules.excessRules.empty
}

and: "Bring port back up"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to additionally ensure that port gets back up (in cleanup or something)

Copy link
Collaborator Author

@rtretyak rtretyak Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to make sure that system was reverted to original state and next tests are allowed to run I do Wrappers.wait(WAIT_OFFSET) { northboundService.getAllLinks().every { it.state != IslChangeType.FAILED } } on line 325. In ensures that the port is up and the related link was again discovered.
Verifying that 'port up' operation made the port to be up is not the goal of this test. Port up/down functionality is tested in a separate spec

northboundService.portUp(islToBreak.srcSwitch.dpId, islToBreak.srcPort)
Wrappers.wait(WAIT_OFFSET) { northboundService.getAllLinks().every { it.state != IslChangeType.FAILED } }
}

def cleanup() {
northboundService.deleteLinkProps(northboundService.getAllLinkProps())
db.resetCosts()
Expand Down
Expand Up @@ -255,7 +255,7 @@ public static Isl factory(
@Override
public String toString() {
return String.format("%s-%s -> %s-%s", srcSwitch.dpId.toString(), srcPort,
dstSwitch.dpId.toString(), dstPort);
dstSwitch != null ? dstSwitch.dpId.toString() : "null", dstSwitch != null ? dstPort : "null");
}
}

Expand Down