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 <sdani...@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.maxim...@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 <sdani...@redhat.com>
>> > Acked-by: Aaron Conole <acon...@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.
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
