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/[email protected]/ > On 30 May 2023, at 12:44, Dumitru Ceara <[email protected]> wrote: > > On 5/24/23 13:03, Ales Musil wrote: >> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov <[email protected]> >> 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 <[email protected]> >>> --- >>> 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 >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil <[email protected]> >> > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
