| Message ID | 20240208181719.224501-2-amusil@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Remove most of the hardcoded table numbers | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Ales, I have one comment below, but it's small enough it can be fixed during merge. Acked-by: Mark Michelson <mmichels@redhat.com> On 2/8/24 13:17, Ales Musil wrote: > There were some comments left with hardcoded numbers. Even if it > wouldn't break any test table shift/change it would just left the > comment outdated. > > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > tests/ovn-northd.at | 2 +- > tests/ovn.at | 8 ++++---- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 591ad5aad..bb5e1f958 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2150,7 +2150,7 @@ AT_CLEANUP > > # This test case tests that when a logical switch has load balancers associated > # (with VIPs configured), the below logical flow is added by ovn-northd. > -# table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]] = 1; next;) > +# (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]] = 1; next;) The other changes in this patch are formatted differently than this one. If this were modeled after the other changes, it would be # table=ls_out_pr_lb, priority=100... Should this comment be formatted the same as the others in this change? > # This test case is added for the BZ - > # https://bugzilla.redhat.com/show_bug.cgi?id=1849162 > # > diff --git a/tests/ovn.at b/tests/ovn.at > index 2cf335cf4..0af60f893 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -18372,8 +18372,8 @@ AT_CHECK([cat 2.packets], [0], [expout]) > > # There should be total of 9 flows present with conjunction action and 2 flows > # with conj match. Eg. > -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > +# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,ls_out_acl_action) > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) > @@ -18413,7 +18413,7 @@ AT_CHECK([cat 2.packets], [0], []) > # properly. > # There should be total of 6 flows present with conjunction action and 1 flow > # with conj match. Eg. > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2) > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2) > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2) > @@ -34296,7 +34296,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC > in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND > out_port_sec=OFTABLE_CHK_OUT_PORT_SEC > > -# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75 in hv1 and hv2 > +# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2 > > hv1_t${in_port_sec}_flows.expected > > hv1_t${in_port_sec_nd}_flows.expected > > hv1_t${out_port_sec}_flows.expected
On Fri, Feb 9, 2024 at 5:51 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Ales, > > I have one comment below, but it's small enough it can be fixed during > merge. > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 2/8/24 13:17, Ales Musil wrote: > > There were some comments left with hardcoded numbers. Even if it > > wouldn't break any test table shift/change it would just left the > > comment outdated. > > > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > tests/ovn-northd.at | 2 +- > > tests/ovn.at | 8 ++++---- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 591ad5aad..bb5e1f958 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2150,7 +2150,7 @@ AT_CLEANUP > > > > # This test case tests that when a logical switch has load balancers > associated > > # (with VIPs configured), the below logical flow is added by > ovn-northd. > > -# table=1 (ls_out_pre_lb ), priority=100 , match=(ip), > action=(reg0[[0]] = 1; next;) > > +# (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]] > = 1; next;) > > The other changes in this patch are formatted differently than this one. > If this were modeled after the other changes, it would be > > # table=ls_out_pr_lb, priority=100... > > Should this comment be formatted the same as the others in this change? > We can follow the same "convention" as for the others. > > # This test case is added for the BZ - > > # https://bugzilla.redhat.com/show_bug.cgi?id=1849162 > > # > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 2cf335cf4..0af60f893 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -18372,8 +18372,8 @@ AT_CHECK([cat 2.packets], [0], [expout]) > > > > # There should be total of 9 flows present with conjunction action and > 2 flows > > # with conj match. Eg. > > -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) > > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > > +# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1 > actions=resubmit(,ls_out_acl_action) > > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 > actions=drop > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 > actions=conjunction(2,2/2) > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 > actions=conjunction(2,2/2) > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 > actions=conjunction(2,2/2) > > @@ -18413,7 +18413,7 @@ AT_CHECK([cat 2.packets], [0], []) > > # properly. > > # There should be total of 6 flows present with conjunction action and > 1 flow > > # with conj match. Eg. > > -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop > > +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 > actions=drop > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 > actions=conjunction(4,2/2) > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 > actions=conjunction(4,2/2) > > # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 > actions=conjunction(4,2/2) > > @@ -34296,7 +34296,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC > > in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND > > out_port_sec=OFTABLE_CHK_OUT_PORT_SEC > > > > -# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75 > in hv1 and hv2 > > +# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, > OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2 > > > hv1_t${in_port_sec}_flows.expected > > > hv1_t${in_port_sec_nd}_flows.expected > > > hv1_t${out_port_sec}_flows.expected > > Thanks, Ales
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 591ad5aad..bb5e1f958 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2150,7 +2150,7 @@ AT_CLEANUP # This test case tests that when a logical switch has load balancers associated # (with VIPs configured), the below logical flow is added by ovn-northd. -# table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]] = 1; next;) +# (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]] = 1; next;) # This test case is added for the BZ - # https://bugzilla.redhat.com/show_bug.cgi?id=1849162 # diff --git a/tests/ovn.at b/tests/ovn.at index 2cf335cf4..0af60f893 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -18372,8 +18372,8 @@ AT_CHECK([cat 2.packets], [0], [expout]) # There should be total of 9 flows present with conjunction action and 2 flows # with conj match. Eg. -# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45) -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop +# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,ls_out_acl_action) +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2) @@ -18413,7 +18413,7 @@ AT_CHECK([cat 2.packets], [0], []) # properly. # There should be total of 6 flows present with conjunction action and 1 flow # with conj match. Eg. -# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop +# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2) # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2) @@ -34296,7 +34296,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND out_port_sec=OFTABLE_CHK_OUT_PORT_SEC -# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75 in hv1 and hv2 +# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2 > hv1_t${in_port_sec}_flows.expected > hv1_t${in_port_sec_nd}_flows.expected > hv1_t${out_port_sec}_flows.expected
There were some comments left with hardcoded numbers. Even if it wouldn't break any test table shift/change it would just left the comment outdated. Signed-off-by: Ales Musil <amusil@redhat.com> --- tests/ovn-northd.at | 2 +- tests/ovn.at | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)