| Message ID | 20210723145841.2229447-2-sdaniele@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | flow dumps containing igmp match cannot be restored | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot | success | github build: passed |
On Fri, 23 Jul 2021 10:58:40 -0400 Salvatore Daniele <sdaniele@redhat.com> wrote: > match.c generates the keyword "igmp", which is not supported in ofp-parse. > This means that flow dumps containing 'igmp' can not be restored. > > Removing the 'igmp' keyword entirely could break existing scripts in stable > branches, so this patch creates a workaround within ovs-save by converting any > instances of "igmp" within $bridge.flows.dump into "ip, nw_proto=2". > > Signed-off-by: Salvatore Daniele <sdaniele@redhat.com> > Acked-by: Aaron Conole <aconole@redhat.com> Acked-by: Timothy Redaelli <tredaelli@redhat.com>
On 7/23/21 4:58 PM, Salvatore Daniele wrote: > match.c generates the keyword "igmp", which is not supported in ofp-parse. > This means that flow dumps containing 'igmp' can not be restored. > > Removing the 'igmp' keyword entirely could break existing scripts in stable > branches, so this patch creates a workaround within ovs-save by converting any > instances of "igmp" within $bridge.flows.dump into "ip, nw_proto=2". > > Signed-off-by: Salvatore Daniele <sdaniele@redhat.com> > Acked-by: Aaron Conole <aconole@redhat.com> > --- > utilities/ovs-save | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/utilities/ovs-save b/utilities/ovs-save > index 27ce3a9aa..23cb0d9d9 100755 > --- a/utilities/ovs-save > +++ b/utilities/ovs-save > @@ -150,7 +150,8 @@ save_flows () { > ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \ > sed -e '/NXST_FLOW/d' \ > -e '/OFPST_FLOW/d' \ > - -e 's/\(idle\|hard\)_age=[^,]*,//g' > \ > + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ > + -e 's/igmp/ip,nw_proto=2/g' > \ > "$workdir/$bridge.flows.dump" > done > echo "rm -rf \"$workdir\"" > Hmm. What about 'igmp_type' and 'igmp_code'? I don't think that OVS will be able to parse them. Should we replace them with generic 'tp_src' and 'tp_dst' ? Will that work? Best regards, Ilya Maximets.
Good point, OVS is unable to parse them. I don't see anywhere in the OVS code that relies on these fields being printed. I could replace them with the default 'tp_src/dst' in this workaround, so as not to break any scripts. That being said, I am not sure how these fields would currently even end up in a flow-dump so perhaps that would be redundant. igmp* fields can not be added explicitly (add-flow ... igmp_type=1) the way you could any other l3 field/type since they can't be parsed. Additionally, it would seem ofputil_normalize_match__() does not recognize 'igmp/ip,nw_proto=2' as an L3 protocol. Setting "tp_src" should work to insert a flow with 'igmp_type', as tp_src is changed to igmp_type internally, but a flow cannot match on an L3 field without saying what L3 protocol is in use, and since 'igmp' is not recognized as such, the flow is normalized removing the igmp_type field. $ ovs-ofctl add-flow br1 "ip,nw_proto=2,tp_src=1, action=drop" 2021-08-05T15:16:48Z|00001|ofp_match|INFO|normalization changed ofp_match, details: 2021-08-05T15:16:48Z|00002|ofp_match|INFO| pre: igmp,igmp_type=1 2021-08-05T15:16:48Z|00003|ofp_match|INFO|post: igmp I could fix this issue in ofputil_normalize_match__() in this patch so igmp packets will behave like any other L3 proto, which would then require me to update the workaround. However, if the goal of this first patch is to avoid breaking existing scripts, perhaps it would make sense to leave the ofputil_normalize_match__() as is? I could then fix the normalization behavior in the second patch in addition to removing 'igmp' and 'igmp fields' from match.c. What do you think? On Wed, Aug 4, 2021 at 1:51 PM Ilya Maximets <i.maximets@ovn.org> wrote: > On 7/23/21 4:58 PM, Salvatore Daniele wrote: > > match.c generates the keyword "igmp", which is not supported in > ofp-parse. > > This means that flow dumps containing 'igmp' can not be restored. > > > > Removing the 'igmp' keyword entirely could break existing scripts in > stable > > branches, so this patch creates a workaround within ovs-save by > converting any > > instances of "igmp" within $bridge.flows.dump into "ip, nw_proto=2". > > > > Signed-off-by: Salvatore Daniele <sdaniele@redhat.com> > > Acked-by: Aaron Conole <aconole@redhat.com> > > --- > > utilities/ovs-save | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/utilities/ovs-save b/utilities/ovs-save > > index 27ce3a9aa..23cb0d9d9 100755 > > --- a/utilities/ovs-save > > +++ b/utilities/ovs-save > > @@ -150,7 +150,8 @@ save_flows () { > > ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats > "$bridge" | \ > > sed -e '/NXST_FLOW/d' \ > > -e '/OFPST_FLOW/d' \ > > - -e 's/\(idle\|hard\)_age=[^,]*,//g' > \ > > + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ > > + -e 's/igmp/ip,nw_proto=2/g' > \ > > "$workdir/$bridge.flows.dump" > > done > > echo "rm -rf \"$workdir\"" > > > > Hmm. What about 'igmp_type' and 'igmp_code'? I don't think that OVS > will be able to parse them. Should we replace them with generic > 'tp_src' and 'tp_dst' ? Will that work? > > Best regards, Ilya Maximets. > >
Just a friendly ping to bring this patch back to the queue. Perhaps addressing igmp in ofputil_normalize_match is outside the scope of this patch set, however, I do think the workaround in ovs-save will be needed to allow flow-dumps containing 'igmp' to be restored. Open to any thoughts! On Thu, Aug 5, 2021 at 11:58 AM Salvatore Daniele <sdaniele@redhat.com> wrote: > > Good point, OVS is unable to parse them. I don't see anywhere in the OVS code that relies on these fields being printed. > > I could replace them with the default 'tp_src/dst' in this workaround, so as not to break any scripts. > > That being said, I am not sure how these fields would currently even end up in a flow-dump so perhaps that would be redundant. igmp* fields can not be added explicitly (add-flow ... igmp_type=1) the way you could any other l3 field/type since they can't be parsed. > > Additionally, it would seem ofputil_normalize_match__() does not recognize 'igmp/ip,nw_proto=2' as an L3 protocol. > Setting "tp_src" should work to insert a flow with 'igmp_type', as tp_src is changed to igmp_type internally, but a flow cannot match on an L3 field without saying what L3 protocol is in use, and since 'igmp' is not recognized as such, the flow is normalized removing the igmp_type field. > > $ ovs-ofctl add-flow br1 "ip,nw_proto=2,tp_src=1, action=drop" > 2021-08-05T15:16:48Z|00001|ofp_match|INFO|normalization changed ofp_match, details: > 2021-08-05T15:16:48Z|00002|ofp_match|INFO| pre: igmp,igmp_type=1 > 2021-08-05T15:16:48Z|00003|ofp_match|INFO|post: igmp > > I could fix this issue in ofputil_normalize_match__() in this patch so igmp packets will behave like any other L3 proto, which would then require me to update the workaround. However, if the goal of this first patch is to avoid breaking existing scripts, perhaps it would make sense to leave the ofputil_normalize_match__() as is? > > I could then fix the normalization behavior in the second patch in addition to removing 'igmp' and 'igmp fields' from match.c. > > What do you think? > > On Wed, Aug 4, 2021 at 1:51 PM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 7/23/21 4:58 PM, Salvatore Daniele wrote: >> > match.c generates the keyword "igmp", which is not supported in ofp-parse. >> > This means that flow dumps containing 'igmp' can not be restored. >> > >> > Removing the 'igmp' keyword entirely could break existing scripts in stable >> > branches, so this patch creates a workaround within ovs-save by converting any >> > instances of "igmp" within $bridge.flows.dump into "ip, nw_proto=2". >> > >> > Signed-off-by: Salvatore Daniele <sdaniele@redhat.com> >> > Acked-by: Aaron Conole <aconole@redhat.com> >> > --- >> > utilities/ovs-save | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/utilities/ovs-save b/utilities/ovs-save >> > index 27ce3a9aa..23cb0d9d9 100755 >> > --- a/utilities/ovs-save >> > +++ b/utilities/ovs-save >> > @@ -150,7 +150,8 @@ save_flows () { >> > ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \ >> > sed -e '/NXST_FLOW/d' \ >> > -e '/OFPST_FLOW/d' \ >> > - -e 's/\(idle\|hard\)_age=[^,]*,//g' > \ >> > + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ >> > + -e 's/igmp/ip,nw_proto=2/g' > \ >> > "$workdir/$bridge.flows.dump" >> > done >> > echo "rm -rf \"$workdir\"" >> > >> >> Hmm. What about 'igmp_type' and 'igmp_code'? I don't think that OVS >> will be able to parse them. Should we replace them with generic >> 'tp_src' and 'tp_dst' ? Will that work? >> >> Best regards, Ilya Maximets. >>
diff --git a/utilities/ovs-save b/utilities/ovs-save index 27ce3a9aa..23cb0d9d9 100755 --- a/utilities/ovs-save +++ b/utilities/ovs-save @@ -150,7 +150,8 @@ save_flows () { ovs-ofctl -O $ofp_version dump-flows --no-names --no-stats "$bridge" | \ sed -e '/NXST_FLOW/d' \ -e '/OFPST_FLOW/d' \ - -e 's/\(idle\|hard\)_age=[^,]*,//g' > \ + -e 's/\(idle\|hard\)_age=[^,]*,//g' \ + -e 's/igmp/ip,nw_proto=2/g' > \ "$workdir/$bridge.flows.dump" done echo "rm -rf \"$workdir\""