Re: [ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check
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 wrote:
>
> On 5/24/23 13:03, Ales Musil wrote:
>> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov
>> 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
>>> ---
>>> 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
>>> @@ -,6 +,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
>>
>
> 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
Re: [ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check
On 5/24/23 13:03, Ales Musil wrote:
> On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov
> 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
>> ---
>> 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
>> @@ -,6 +,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
>
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
Re: [ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check
On Mon, May 22, 2023 at 11:18 PM Vladislav Odintsov
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
> ---
> 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
> @@ -,6 +,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
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA
amu...@redhat.comIM: amusil
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] controller: don't report UDP as unsupported proto in svc_check
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
---
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
@@ -,6 +,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
