| Message ID | 20230522211815.1201754-1-odivlad@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] controller: don't report UDP as unsupported proto in svc_check | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov <odivlad@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 <odivlad@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On 5/24/23 13:03, Ales Musil wrote: > On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov <odivlad@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 <odivlad@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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@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
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-odivlad@gmail.com/ > On 30 May 2023, at 12:44, Dumitru Ceara <dceara@redhat.com> wrote: > > On 5/24/23 13:03, Ales Musil wrote: >> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov <odivlad@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 <odivlad@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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Looks good to me, thanks. >> >> Acked-by: Ales Musil <amusil@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
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);
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 <odivlad@gmail.com> --- controller/pinctrl.c | 7 +++++++ 1 file changed, 7 insertions(+)