Hi Dumitru, thanks for the review.
On 5/24/22 07:17, Dumitru Ceara wrote:
On 5/23/22 20:43, Mark Michelson wrote:
The "vtep: 3 HVs, 1 VIFs/HV, 1 GW, 1 LS" test fails consistently on PPC.
The test expects a flow with the following to be present:
is_chassis_resident("cr-lrp1") || is_chassis_resident("cr-lrp2")
However, on PPC, the test fails because the flow has the following
instead:
is_chassis_resident("cr-lrp2") || is_chassis_resident("cr-lrp1")
In other words, the fields are reversed. This is because the ordering of
the ports in ovn-northd can differ depending on architecture. This is a
case where the northd code is correct, but the test is overly
restrictive.
To fix this, the test has been broken into two parts. First, we check
the overall flow structure to ensure that there are two
"is_chassis_resident" fields present. Then we can break that flow down
further and sort the fields so that they will be in a predictable order.
Signed-off-by: Mark Michelson <[email protected]>
---
Hi Mark,
tests/ovn.at | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/tests/ovn.at b/tests/ovn.at
index 4461b840e..80ede049d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -4269,12 +4269,25 @@
response=${sha}${lrpmac}08060001080006040002${lrpmac}${tpa}${sha}${spa}
# we expect arp reply packet on hv3
echo $response >> 3.expected
-AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed 's/table=../table=??/g'], [0], [dnl
+# First ensure basic flow contents are as we expect.
+AT_CHECK([ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | sort | sed
's/table=../table=??/g' | sed
's/is_chassis_resident([[^)]]*)/is_chassis_resident("??")/g'], [0], [dnl
table=??(ls_in_check_port_sec), priority=70 , match=(inport ==
"lp-vtep"), action=(reg0[[14]] = 1; next(pipeline=ingress, table=??);)
table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1),
action=(next(pipeline=ingress, table=??);)
- table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 &&
(is_chassis_resident("cr-lrp1") || is_chassis_resident("cr-lrp2"))), action=(next;)
+ table=??(ls_in_hairpin ), priority=2000 , match=(reg0[[14]] == 1 &&
(is_chassis_resident("??") || is_chassis_resident("??"))), action=(next;)
])
+# We've ensured that the expected hairpin flows are present
+# and that the expected number of "is_chassis_resident" fields are in
+# the flow. Now we need to ensure the contents are correct.
+# Unfortunately, the order of the "is_chassis_resident" fields is
+# unpredictable. Therefore we sort them so the order is predictable.
+actual_chassis=$(ovn-sbctl lflow-list lsw0 | grep 'reg0[\[14\]]' | grep
'priority=2000' | grep -o 'is_chassis_resident([[^)]]*)' | sort)
This looks a bit weird, shouldn't the first grep be 'reg0\[[14\]]'?
Surprisingly, your version seems to work too.
My version is a copy of the existing grep in the AT_CHECK above. I
believe it works because it's not important which of the square brackets
is escaped, so long as one opening and one closing bracket is escaped.
To avoid this kind of confusion can we change it to the following?
actual_chassis=$(ovn-sbctl lflow-list lsw0 | grep ls_in_hairpin | grep -o
'is_chassis_resident([[^)]]*)' | sort)
In any case, I don't have a strong preference, any of the two above
will do, and if you change this at apply time please also add:
Acked-by: Dumitru Ceara <[email protected]>
I opted to grep on the table name as you have suggested. I also kept the
priority match just in case any other flows with "is_chassis_resident"
are added to the ls_in_hairpin table in the future.
I pushed this to main and branch-22.06
+
+expected_chassis='is_chassis_resident("cr-lrp1")
+is_chassis_resident("cr-lrp2")'
+
+check test "$expected_chassis" = "$actual_chassis"
+
# dump information with counters
echo "------ OVN dump ------"
ovn-nbctl show
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
