[ovs-dev] [PATCH v3 2/2] Match: Do not print "igmp" match keyword

[Salvatore Daniele]

Thank you for the feedback Aaron, I hadn't considered that. I will send a
v4 without reverting the workaround.

On Thu, Jul 22, 2021 at 1:25 PM Aaron Conole <aconole at redhat.com> wrote:

> Ilya Maximets <i.maximets at ovn.org> writes:
>
> > On 7/22/21 4:15 PM, Aaron Conole wrote:
> >> Salvatore Daniele <sdaniele at redhat.com> writes:
> >>
> >>> From: Adrian Moreno <amorenoz at redhat.com>
> >>>
> >>> The match keyword "igmp" is not supported in ofp-parse, which means
> >>> that flow dumps cannot be restored. Previously a workaround was
> >>> added to ovs-save to avoid changing output in stable branches.
> >>>
> >>> This patch removes that work around, and instead prints the igmp
> >>> match in the accepted format (ip,nw_proto=2). Tests are added, and
> >>> NEWS is updated to reflect this change.
> >>>
> >>> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
> >>> Signed-off-by: Salvatore Daniele <sdaniele at redhat.com>
> >>> Co-authored-by: Salvatore Daniele <sdaniele at redhat.com>
> >>> Acked-by: Flavio Leitner <fbl at sysclose.org>
> >>> ---
> >>
> >> It's strange to me to see a fix introduced in 1/2 and then backed out
> >> completely in 2/2.  I think it makes sense not to revert it.  Here's
> >> an example:
> >>
> >> Suppose I have an older ovs-vswitchd running, and I upgrade.  This will
> >> replace the ovs-save script.  With 1/2 and no revert from 2/2, I'll get
> >> good behavior (we will replace igmp with ip,nw_proto=2).  With the
> >> current patch (revert 1/2), I will fall back into having 'igmp' in the
> >> flow output that ofp-parse cannot support.
> >
> > That's a good point.  We should keep the workaround for a while, I
> suppose.
> > We should be able to remove it in 2.18 release time frame.  This way will
> > have normal upgrades between 2 LTS versions 2.13 and 2.17.  2.17 will not
> > have the issue and upgrades from it to later versions will not need a
> > workaround.
> >
> >>
> >> Maybe sometime in the future it might make sense to remove the
> >> replacement, but for now it helps with upgrade case.
> >>
> >> Alternatively, maybe we should add support to ofp-parse for 'igmp'
> >> keyword.
> >
> > We discussed that a few times:
> >
> https://patchwork.ozlabs.org/project/openvswitch/patch/20201102232805.1960103-1-blp@ovn.org/
> >
> https://patchwork.ozlabs.org/project/openvswitch/patch/20210630154354.490671-2-amorenoz@redhat.com/
> >
> > I don't think it's a good thing to do.
>
> ACK!
>
> > Best regards, Ilya Maximets.
>
>