Message ID | 1499669501-80363-3-git-send-email-jpettit@ovn.org |
---|---|
State | Superseded |
Headers | show |
On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote: > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > tests/ofproto.at | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tests/ofproto.at b/tests/ofproto.at > index 9e6acfad653d..c7ea31a77ce9 100644 > --- a/tests/ofproto.at > +++ b/tests/ofproto.at > @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172 > ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234 > ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234 > if test X"$1" = X"OFPRR_DELETE"; then shift; > - echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=gropu_delete table_id=0" > + echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=group_delete table_id=0" Why didn't this cause a test failure? (Is this code never actually executed?)
> On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote: >> Signed-off-by: Justin Pettit <jpettit@ovn.org> >> --- >> tests/ofproto.at | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tests/ofproto.at b/tests/ofproto.at >> index 9e6acfad653d..c7ea31a77ce9 100644 >> --- a/tests/ofproto.at >> +++ b/tests/ofproto.at >> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172 >> ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234 >> ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234 >> if test X"$1" = X"OFPRR_DELETE"; then shift; >> - echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=gropu_delete table_id=0" >> + echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=group_delete table_id=0" > > Why didn't this cause a test failure? (Is this code never actually > executed?) Yes, there were multiple errors in the test that prevented it from running. It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete". The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3. Do you think we should add support for 1.3 or just remove that bit of test code? Here's the commit that introduced that test: https://github.com/openvswitch/ovs/commit/cc40d06bf5ca3 --Justin
On Sat, Jul 15, 2017 at 03:02:18PM -0700, Justin Pettit wrote: > > > On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote: > >> Signed-off-by: Justin Pettit <jpettit@ovn.org> > >> --- > >> tests/ofproto.at | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tests/ofproto.at b/tests/ofproto.at > >> index 9e6acfad653d..c7ea31a77ce9 100644 > >> --- a/tests/ofproto.at > >> +++ b/tests/ofproto.at > >> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172 > >> ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234 > >> ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234 > >> if test X"$1" = X"OFPRR_DELETE"; then shift; > >> - echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=gropu_delete table_id=0" > >> + echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=group_delete table_id=0" > > > > Why didn't this cause a test failure? (Is this code never actually > > executed?) > > Yes, there were multiple errors in the test that prevented it from running. It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete". The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3. > > Do you think we should add support for 1.3 or just remove that bit of test code? I think it would be better to support OF1.3. I don't know of a reason that it's particularly hard.
> On Jul 15, 2017, at 3:21 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Sat, Jul 15, 2017 at 03:02:18PM -0700, Justin Pettit wrote: >> >>> On Jul 14, 2017, at 12:59 PM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Sun, Jul 09, 2017 at 11:51:40PM -0700, Justin Pettit wrote: >>>> Signed-off-by: Justin Pettit <jpettit@ovn.org> >>>> --- >>>> tests/ofproto.at | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/tests/ofproto.at b/tests/ofproto.at >>>> index 9e6acfad653d..c7ea31a77ce9 100644 >>>> --- a/tests/ofproto.at >>>> +++ b/tests/ofproto.at >>>> @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172 >>>> ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234 >>>> ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234 >>>> if test X"$1" = X"OFPRR_DELETE"; then shift; >>>> - echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=gropu_delete table_id=0" >>>> + echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=group_delete table_id=0" >>> >>> Why didn't this cause a test failure? (Is this code never actually >>> executed?) >> >> Yes, there were multiple errors in the test that prevented it from running. It's easy to correct them, but our support for OpenFlow 1.3 doesn't include generating a flow removed due to the "group delete". The OpenFlow 1.3 specification defines such a reason but we seem to break things into "OpenFlow 1.4+" or "Open Flow 1.0". so we don't generate it for 1.3. >> >> Do you think we should add support for 1.3 or just remove that bit of test code? > > I think it would be better to support OF1.3. I don't know of a reason > that it's particularly hard. I sent out a patch to add support: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335724.html --Justin
diff --git a/tests/ofproto.at b/tests/ofproto.at index 9e6acfad653d..c7ea31a77ce9 100644 --- a/tests/ofproto.at +++ b/tests/ofproto.at @@ -3430,7 +3430,7 @@ udp,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172 ovs-ofctl -O OpenFlow13 add-flow br0 send_flow_rem,actions=group:1234 ovs-ofctl -O OpenFlow13 --strict del-groups br0 group_id=1234 if test X"$1" = X"OFPRR_DELETE"; then shift; - echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=gropu_delete table_id=0" + echo >>expout "OFPT_FLOW_REMOVED (OF1.3): reason=group_delete table_id=0" fi AT_FAIL_IF([test X"$1" != X])
Signed-off-by: Justin Pettit <jpettit@ovn.org> --- tests/ofproto.at | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)