| Message ID | 866dfb4e485a76a66503f05e82927bf721c9a455.1633704067.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] controller: do not mark bfd and ipv6_pd msgs as local-only | 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 |
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. 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? Thanks again, Ihar 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@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;
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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> 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.") ack, do I need to repost to fix them? Regards, Lorenzo > > > > > 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@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 10/11/21 12:01 PM, Lorenzo Bianconi wrote: >> 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.") > > ack, do I need to repost to fix them? > I guess it's up to the maintainers; in any case if you plan to repost then feel free to include my ack: Acked-by: Dumitru Ceara <dceara@redhat.com> Regards, Dumitru
On 10/11/21 6:14 AM, Dumitru Ceara wrote: > On 10/11/21 12:01 PM, Lorenzo Bianconi wrote: >>> 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.") >> >> ack, do I need to repost to fix them? >> > > I guess it's up to the maintainers; in any case if you plan to repost > then feel free to include my ack: > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Regards, > Dumitru > Thanks everyone, I updated the "Fixes" tags as suggested earlier in the thread and pushed the changes to main and branch-21.09. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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;
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@redhat.com> --- controller/pinctrl.c | 2 -- 1 file changed, 2 deletions(-)