add new flowHelper method: getFlowHistoryByAction #2791

andriidovhan · 2019-09-12T11:07:02Z

No description provided.

...ctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryAction.groovy

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy

...ctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryAction.groovy

rtretyak

It would be also great if you would try to asses performance degradation related to switching to 'history' approach compared to 'status' approach

...ctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryAction.groovy

rtretyak · 2019-09-20T10:53:16Z

...nctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryEvent.groovy

+
+    REROUTE_SUCCESS(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_UP),
+    REROUTE_FAIL(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_DOWN),
+    REROUTE_FAIL_PROTECTED_PATH(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.FLOW_WAS_VALIDATED_SUCCESSFULLY),


I am a bit confused how FLOW_REROUTING + FLOW_WAS_VALIDATED_SUCCESSFULLY is related to protected paths

I just copied it from flowHistory output after flowState was changed to DEGRADED

Why do you call it like that? It clearly means that flow is validated after reroute and I would agree with that, but the 'reroute fail for protected path' is just a special case of your test. The flow is validated, and you expect a degraded status in your exact special test, that's fine. But you cannot globally assume that 'rerouting + successfull validation' means 'reroute for protected path failed', that breaks laws of logic. Crocodiles are green, crocodiles are dangerous. Grass is green -> grass is dangerous?

andriidovhan

It would be also great if you would try to asses performance degradation related to switching to 'history' approach compared to 'status' approach

it will add around half a second to each test

	verifyFlowstate(ms)	getFlowStatus(ms)
1	4564	3815
2	3946	4312
3	4501	3774
4	3914	3788
5	3871	4288
6	3314	4304
7	3859	3825
8	3840	3752
9	4517	3817
10	4424	3275
11	4485	3755
average:	4112,272727	3882,272727

andriidovhan · 2019-09-20T13:03:59Z

...nctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryEvent.groovy

+
+    REROUTE_SUCCESS(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_UP),
+    REROUTE_FAIL(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_DOWN),
+    REROUTE_FAIL_PROTECTED_PATH(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.FLOW_WAS_VALIDATED_SUCCESSFULLY),


I just copied it from flowHistory output after flowState was changed to DEGRADED

rtretyak · 2019-09-23T11:31:08Z

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy

+     * @param event sequence of flow history actions
+     * @param state expected flow state
+     */
+    void verifyFlowState(String flowId, Long timeFrom, FlowHistoryEvent event, state) {


Please add a strict type for state

rtretyak · 2019-09-23T11:32:06Z

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy

+     * Check that flow state is changed correctly after corresponding flow history action.
+     *
+     * @param flowId
+     * @param timeFrom history will requested from this timestamp


rtretyak · 2019-09-23T11:33:10Z

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy

+        def flowHistory = northbound.getFlowHistory(flowId, timeFrom, timeTo).findAll {
+            it.action == event.initAction.toString()
+        }
+        assert flowHistory.size() == 1


are you sure that you want to limit your method with this condition? What if there were more actions on the flow? I'm not insisting, but for me it kind of limits where it can be used or forces to use the 'from' timestamp. that's fine I guess, but requires a lot of 'timestamp' vars for long tests

rtretyak · 2019-09-23T11:35:25Z

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy

+     * @param state expected flow state
+     */
+    void verifyFlowState(String flowId, Long timeFrom, FlowHistoryEvent event, state) {
+        def timeTo = timeFrom ? System.currentTimeSeconds() : null


well, this is kind of tricky. timeTo based on timeFrom, why would you do this? Very strange and not intuitive approach

rtretyak · 2019-09-23T11:48:19Z

...nctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryEvent.groovy

+
+    REROUTE_SUCCESS(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_UP),
+    REROUTE_FAIL(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.SET_THE_FLOW_STATUS_TO_DOWN),
+    REROUTE_FAIL_PROTECTED_PATH(FlowHistoryAction.FLOW_REROUTING, FlowHistoryAction.FLOW_WAS_VALIDATED_SUCCESSFULLY),


Why do you call it like that? It clearly means that flow is validated after reroute and I would agree with that, but the 'reroute fail for protected path' is just a special case of your test. The flow is validated, and you expect a degraded status in your exact special test, that's fine. But you cannot globally assume that 'rerouting + successfull validation' means 'reroute for protected path failed', that breaks laws of logic. Crocodiles are green, crocodiles are dangerous. Grass is green -> grass is dangerous?

rtretyak · 2019-09-23T11:51:00Z

...onal-tests/src/test/groovy/org/openkilda/functionaltests/spec/flows/ProtectedPathSpec.groovy

@@ -893,17 +893,22 @@ class ProtectedPathSpec extends HealthCheckSpecification {
        antiflap.portDown(protectedIsls[0].dstSwitch.dpId, protectedIsls[0].dstPort)

        then: "Flow state is changed to DEGRADED"
-        Wrappers.wait(WAIT_OFFSET) { assert northbound.getFlowStatus(flow.id).status == FlowState.DEGRADED }
+        Wrappers.wait(WAIT_OFFSET) {
+            flowHelper.verifyFlowState(flow.id, FlowHistoryEvent.REROUTE_FAIL_PROTECTED_PATH, FlowState.DEGRADED)


This is literally false expression. You are not verifying that the reroute failed for protected path, you are just checking that a successful validation happened.

andriidovhan · 2019-12-06T08:42:43Z

I am going to rework it at all
So I will create a new pr

andriidovhan added area/testing On Review improvement labels Sep 12, 2019

andriidovhan requested a review from rtretyak September 12, 2019 11:07

rtretyak reviewed Sep 12, 2019

View reviewed changes

...ctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryAction.groovy Show resolved Hide resolved

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy Outdated Show resolved Hide resolved

andriidovhan force-pushed the test/add-helper-getFlowHistoryByAction branch from 2c8e0c1 to 12ec9d8 Compare September 12, 2019 13:11

andriidovhan requested a review from rtretyak September 12, 2019 13:13

andriidovhan force-pushed the test/add-helper-getFlowHistoryByAction branch from 12ec9d8 to 2560567 Compare September 16, 2019 07:31

rtretyak reviewed Sep 16, 2019

View reviewed changes

...src/functional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHelper.groovy Show resolved Hide resolved

...ctional-tests/src/main/groovy/org/openkilda/functionaltests/helpers/FlowHistoryAction.groovy Outdated Show resolved Hide resolved

andriidovhan force-pushed the test/add-helper-getFlowHistoryByAction branch from 2560567 to 3f1dee7 Compare September 19, 2019 12:12

andriidovhan requested a review from rtretyak September 19, 2019 12:12

add new helper: verifyFlowState

0b0a690

andriidovhan force-pushed the test/add-helper-getFlowHistoryByAction branch from 3f1dee7 to 0b0a690 Compare September 20, 2019 10:46

rtretyak reviewed Sep 20, 2019

View reviewed changes

andriidovhan commented Sep 20, 2019

View reviewed changes

andriidovhan requested a review from rtretyak September 20, 2019 13:10

rtretyak suggested changes Sep 23, 2019

View reviewed changes

andriidovhan removed the On Review label Oct 11, 2019

andriidovhan closed this Dec 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new flowHelper method: getFlowHistoryByAction #2791

add new flowHelper method: getFlowHistoryByAction #2791

andriidovhan commented Sep 12, 2019

rtretyak left a comment

rtretyak Sep 20, 2019

andriidovhan Sep 20, 2019

rtretyak Sep 23, 2019

andriidovhan left a comment

andriidovhan Sep 20, 2019

rtretyak Sep 23, 2019

rtretyak Sep 23, 2019

rtretyak Sep 23, 2019

rtretyak Sep 23, 2019

rtretyak Sep 23, 2019

rtretyak Sep 23, 2019

andriidovhan commented Dec 6, 2019

add new flowHelper method: getFlowHistoryByAction #2791

add new flowHelper method: getFlowHistoryByAction #2791

Conversation

andriidovhan commented Sep 12, 2019

rtretyak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andriidovhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andriidovhan commented Dec 6, 2019