Hi Dumitru, I agree with the requested changes, such approach seems much better. I’ve reworked patch and just sent v2. Please, check it out:
https://patchwork.ozlabs.org/project/ovn/patch/20230530124157.1223591-1-odiv...@gmail.com/ > On 30 May 2023, at 12:44, Dumitru Ceara <dce...@redhat.com> wrote: > > On 5/24/23 13:03, Ales Musil wrote: >> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov <odiv...@gmail.com> >> wrote: >> >>> Depending on the udp service, it can reply with some udp data. >>> In that case ovn-controller will warn with next message: >>> >>> pinctrl(ovn_pinctrl0)|WARN|handle service check: Unsupported protocol - >>> [11] >>> >>> With this patch ovn-controller ignores UDP packets, which came to >>> pinctrl_handle_svc_check(). This is not something abnormal, so don't >>> write warnings. >>> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1913162 >>> Signed-off-by: Vladislav Odintsov <odiv...@gmail.com> >>> --- >>> controller/pinctrl.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >>> index b4be22020..f2e753cdb 100644 >>> --- a/controller/pinctrl.c >>> +++ b/controller/pinctrl.c >>> @@ -7777,6 +7777,13 @@ pinctrl_handle_svc_check(struct rconn *swconn, >>> const struct flow *ip_flow, >>> ip_proto = in_ip->ip6_nxt; >>> } >>> >>> + if (ip_proto == IPPROTO_UDP) { >>> + /* We should do nothing if we got UDP packet. >>> + * If service is running, it can respond with any UDP data, >>> + * so just ingore it. */ >>> + return; >>> + } >>> + >>> if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_ICMP && >>> ip_proto != IPPROTO_ICMPV6) { >>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); >>> -- >>> 2.36.1 >>> >>> _______________________________________________ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil <amu...@redhat.com> >> > > Hi, Vladislav, Ales, > > I think it might be better to just restrict the logical flow condition > to the type of traffic we actually want to handle in slow path (in > pinctrl). > > That is, this flow (and the others that match on ethernet destination > being equal to $svc_monitor_mac): > > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110, > "eth.dst == $svc_monitor_mac", > "handle_svc_check(inport);"); > > should probably be changed to explicitly match on the supported protocol > list, e.g.: > > ovn_lflow_add(lflows, od, S_SWITCH_IN_L2_LKUP, 110, > "eth.dst == $svc_monitor_mac && (tcp || icmp || icmp6)", > "handle_svc_check(inport);"); > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
