[ovs-dev] [PATCH ovn] controller: do not mark bfd and ipv6_pd msgs as local-only

Dumitru Ceara dceara at redhat.com
Mon Oct 11 07:32:38 UTC 2021


On 10/9/21 1:05 AM, Ihar Hrachyshka wrote:
> This is correct, thank you for fixing it.
> 
> One comment and one question:
> 
> 1) While the referred patch that suppressed LOCAL_ONLY traffic exposed
> the issue, it was always a bug, though affecting a tiny minority of
> scenarios.
> 
> For example for PD, one can probably imagine a scenario where PD server
> is not located in a provider network but is hosted on a VIF that is
> attached to a "public" logical switch (no localnet ports!). This VIF may
> be located on a different chassis, in which case, even without the
> suppressing patch, PD requests wouldn't reach PD server. On the other
> hand, they would reach the server if its VIF would happen to land on the
> same chassis. A similar scenario may be envisioned for BFD, where router
> ports are distributed across multiple chassis.

I agree, the "Fixes" tag looks wrong.  I think it should be:

Fixes: e3a398e9146e ("controller: Add ipv6 prefix delegation state machine")
Fixes: 365a8a696afb ("bfd: support demand mode on rx side.")

> 
> 2) I see you covered all pinctrl handlers that were setting the
> LOCAL_ONLY bit via MLF_LOCAL_ONLY_BIT macro. I've noticed that there are
> several other handlers that use MLF_LOCAL_ONLY macro instead, with (as
> far as I can see) exactly the same effect (any idea why we have both?).
> An example of that would be e.g. ip_mcast_querier_send_igmp. Do we
> believe that other packet injections are meant to stay inside the same
> chassis, or is it just something you missed / left out of scope for this
> patch?

The other use cases are, AFAICT:

- IGMP/MLD querier
- service monitor

The multicast queriers use per logical switch source mac address, and
are generated by ovn-controller on each hypervisor where the logical
switch has bound ports.  As far as I see it's similar for service
monitor.  So using MLF_LOCAL_ONLY seems correct to me.

> 
> Thanks again,
> 
> Ihar

Regards,
Dumitru

> 
> On 10/8/21 10:54 AM, Lorenzo Bianconi wrote:
>> Do not mark BFD and IPv6 Prefix Delegation messages as local only since
>> they will be sent through a localnet port.
>>
>> Fixes: 14c6dac51dc8 ("Suppress LOCAL_ONLY traffic for localnet ports")
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>
>> ---
>>   controller/pinctrl.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index cc3edaaf4..7d01b18a3 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -1108,7 +1108,6 @@ ipv6_prefixd_send(struct rconn *swconn, struct
>> ipv6_prefixd_state *pfd)
>>       uint32_t port_key = pfd->port_key;
>>       put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>>       put_load(port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
>> -    put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY_BIT, 1, &ofpacts);
>>       struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>>       resubmit->in_port = OFPP_CONTROLLER;
>>       resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
>> @@ -6815,7 +6814,6 @@ pinctrl_send_bfd_tx_msg(struct rconn *swconn,
>> struct bfd_entry *entry,
>>       uint32_t port_key = entry->port_key;
>>       put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>>       put_load(port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
>> -    put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY_BIT, 1, &ofpacts);
>>       struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>>       resubmit->in_port = OFPP_CONTROLLER;
>>       resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list