[ovs-dev] [PATCH v5 0/2] flow dumps containing igmp match cannot be restored
i.maximets at ovn.org
Mon Nov 29 13:01:11 UTC 2021
On 11/4/21 20:54, Salvatore Daniele wrote:
> Hi Ilya,
> Per your feedback on v4, I accounted for igmp_type/code, both in the
> work-around in 1/2, and removing it from match.c entirely in 2/2.
> I was not sure the best way to proceed regarding the issue I had
> mentioned on v4 where ofputil_normalize_match__() fails to recognize
> 'igmp/ip,nw_proto=2' as an L3 protocol and normalizes igmp flows,
> removing igmp_type/code.
> However, I feel removing the fields as you suggested should solve the
> problem regardless.
> Curious to hear your thoughts at your convenience!
Hi. Thanks for v5 and sorry for it taking so long for me to reply.
Patches LGTM at the first glance. I'll run a couple of tests with
them and apply if everything will look fine.
For the matching issue. tp_src field in the OpenFlow structure is
defined as "TCP/UDP/SCTP source port/ICMP type", so it's not intended
to be used for IGMP packets. Matching on IGMP fields also not supported
by the OpenFlow specification. So, it seems correct that OVS rejects
this kind of matching requests. Same for tp_dst.
Best regards, Ilya Maximets.
> On Thu, Nov 4, 2021 at 3:39 PM Salvatore Daniele <sdaniele at redhat.com> wrote:
>> match_format() prints the keyword "igmp" for flows with the field
>> "ip,nw_proto=2". ofp_parse_protocol does not accept this value, or
>> the values igmp_type or igmp_code.
>> This results in flow dump restoration failing when the ovs-save script
>> is used by "ovs-ctl restart" on a dump of flows containing this match.
>> However, removing the "igmp" keyword entirely could break existing
>> scripts in stable branches.
>> The first patch addresses this issue by providing a workaround within
>> ovs-save to preserve the 'igmp' keywords while allowing flows to be
>> restored. This change would be backported to all stable branches.
>> The second patch removes the 'igmp' outputs entirely, replacing it with
>> 'ip,nw_proto=2'. This has been added to NEWS, and would be applied
>> in master branch only.
>> While it might make sense to eventually remove the ovs-save workaround,
>> it remains in this second patch to ensure flows can be restored when
>> upgrading ovs-vswitchd from older versions.
>> - Handle igmp_type/code in workaround and remove them from match.c in
>> - Include the ovs-save workaround in both patch 1/2 and 2/2 to address
>> upgrade case discussed in v3
>> - Fixed typos to be inline with DCO policy
>> - Rebased ontop of latest master
>> - Address comments made on v1 with regard to a work around for stable
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1982743
>> Signed-off-by: Salvatore Daniele <sdaniele at redhat.com>
>> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
>> Co-authored-by: Adrian Moreno <amorenoz at redhat.com>
>> Adrian Moreno (1):
>> Match: Do not print "igmp" match keyword
>> Salvatore Daniele (1):
>> ovs-save: Save igmp flows in ofp_parse syntax
>> NEWS | 2 ++
>> lib/match.c | 6 ------
>> tests/ovs-ofctl.at | 6 ++++++
>> utilities/ovs-save | 5 ++++-
>> 4 files changed, 12 insertions(+), 7 deletions(-)
More information about the dev