[ovs-dev] [PATCH v4 1/2] ovs-save: Save igmp flows in ofp_parse syntax

Salvatore Daniele sdaniele at redhat.com
Thu Oct 7 18:00:44 UTC 2021


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 at 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 at 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 at redhat.com>
>> > Acked-by: Aaron Conole <aconole at 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.
>>



More information about the dev mailing list