| Message ID | 2e7c714a8543135ac19fd8a1a2c58bb691e78e0c.1732630355.git.felix.huettner@stackit.cloud |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN Fabric integration | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 11/26/24 3:37 PM, Felix Huettner via dev wrote: > We already parse the networks of a port to the ovn_port struct, so there > is no need to reference northbound. > This is a prerequisite for later patches that use derived router ports. > > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > --- Hi Felix, > northd/northd.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 0fe15ac59..32a7c8509 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3139,10 +3139,25 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > sbrec_port_binding_set_parent_port(op->sb, NULL); > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > + const struct lport_addresses *networks; > + if (op->primary_port) { > + networks = &op->primary_port->lrp_networks; > + } else { > + networks = &op->lrp_networks; > + } > struct ds s = DS_EMPTY_INITIALIZER; > - ds_put_cstr(&s, op->nbrp->mac); > - for (int i = 0; i < op->nbrp->n_networks; ++i) { > - ds_put_format(&s, " %s", op->nbrp->networks[i]); > + ds_put_cstr(&s, networks->ea_s); > + for (int i = 0; i < networks->n_ipv4_addrs; ++i) { Nit: size_t Nit: ++i is not really needed > + struct ipv4_netaddr addr = networks->ipv4_addrs[i]; > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); > + } > + /* We do not need the ipv6 LLA. Since it is last in the list we just > + * skip it. */ Should we instead use in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr) ? If this has the potential of breaking things (e.g., when users explicitly set a LLA into the LRP.networks - although, is that even supported and OK?) we could add a helper to ovn-util.[hc]: lrp_networks_to_string(const struct lport_addresses *, struct ds *) Like that we don't make assumptions in northd.c about how addresses are stored by the ovn-util module. > + if (networks->n_ipv6_addrs > 1) { > + for (int i = 0; i < networks->n_ipv6_addrs - 1; ++i) { Same nits about size_t and ++i. > + struct ipv6_netaddr addr = networks->ipv6_addrs[i]; > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); > + } > } > const char *addresses = ds_cstr(&s); > sbrec_port_binding_set_mac(op->sb, &addresses, 1); Regards, Dumitru
On Mon, Dec 02, 2024 at 10:43:50AM +0100, Dumitru Ceara wrote: > On 11/26/24 3:37 PM, Felix Huettner via dev wrote: > > We already parse the networks of a port to the ovn_port struct, so there > > is no need to reference northbound. > > This is a prerequisite for later patches that use derived router ports. > > > > Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud> > > --- > > Hi Felix, Hi Dumitru, thanks for the review. Smaller nits will be fixed in the next version. > > > northd/northd.c | 21 ++++++++++++++++++--- > > 1 file changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 0fe15ac59..32a7c8509 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -3139,10 +3139,25 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, > > sbrec_port_binding_set_parent_port(op->sb, NULL); > > sbrec_port_binding_set_tag(op->sb, NULL, 0); > > > > + const struct lport_addresses *networks; > > + if (op->primary_port) { > > + networks = &op->primary_port->lrp_networks; > > + } else { > > + networks = &op->lrp_networks; > > + } > > struct ds s = DS_EMPTY_INITIALIZER; > > - ds_put_cstr(&s, op->nbrp->mac); > > - for (int i = 0; i < op->nbrp->n_networks; ++i) { > > - ds_put_format(&s, " %s", op->nbrp->networks[i]); > > + ds_put_cstr(&s, networks->ea_s); > > + for (int i = 0; i < networks->n_ipv4_addrs; ++i) { > > Nit: size_t > Nit: ++i is not really needed > > > + struct ipv4_netaddr addr = networks->ipv4_addrs[i]; > > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); > > + } > > + /* We do not need the ipv6 LLA. Since it is last in the list we just > > + * skip it. */ > > Should we instead use > in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr) > ? > > If this has the potential of breaking things (e.g., when users > explicitly set a LLA into the LRP.networks - although, is that even > supported and OK?) we could add a helper to ovn-util.[hc]: > > lrp_networks_to_string(const struct lport_addresses *, struct ds *) > > Like that we don't make assumptions in northd.c about how addresses are > stored by the ovn-util module. I took a look into why there is even the need to ignore the generated ipv6 address in the first place. It would seem to me more logical if the address list contained all addresses a given port_binding has, instead of ignoring one. However i saw multiple places (e.g. bfd_monitor_run in pinctrl.c) that check if there is any ipv6 address and if yes use the first one. So for a previous ipv4 only port adding now this generated v6 address would cause a behaviour change. I am extremely uncertain if this would be breaking in any way. I will therefor add the helpers to ovn-util as you suggested. Thanks a lot Felix > > > + if (networks->n_ipv6_addrs > 1) { > > + for (int i = 0; i < networks->n_ipv6_addrs - 1; ++i) { > > Same nits about size_t and ++i. > > > + struct ipv6_netaddr addr = networks->ipv6_addrs[i]; > > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); > > + } > > } > > const char *addresses = ds_cstr(&s); > > sbrec_port_binding_set_mac(op->sb, &addresses, 1); > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 12/5/24 8:20 AM, Felix Huettner wrote: >>> + struct ipv4_netaddr addr = networks->ipv4_addrs[i]; >>> + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); >>> + } >>> + /* We do not need the ipv6 LLA. Since it is last in the list we just >>> + * skip it. */ >> Should we instead use >> in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr) >> ? >> >> If this has the potential of breaking things (e.g., when users >> explicitly set a LLA into the LRP.networks - although, is that even >> supported and OK?) we could add a helper to ovn-util.[hc]: >> >> lrp_networks_to_string(const struct lport_addresses *, struct ds *) >> >> Like that we don't make assumptions in northd.c about how addresses are >> stored by the ovn-util module. > I took a look into why there is even the need to ignore the generated > ipv6 address in the first place. It would seem to me more logical if the > address list contained all addresses a given port_binding has, instead > of ignoring one. > However i saw multiple places (e.g. bfd_monitor_run in pinctrl.c) that > check if there is any ipv6 address and if yes use the first one. > So for a previous ipv4 only port adding now this generated v6 address > would cause a behaviour change. > I am extremely uncertain if this would be breaking in any way. > > I will therefor add the helpers to ovn-util as you suggested. > Sounds good, thanks!
diff --git a/northd/northd.c b/northd/northd.c index 0fe15ac59..32a7c8509 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3139,10 +3139,25 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn, sbrec_port_binding_set_parent_port(op->sb, NULL); sbrec_port_binding_set_tag(op->sb, NULL, 0); + const struct lport_addresses *networks; + if (op->primary_port) { + networks = &op->primary_port->lrp_networks; + } else { + networks = &op->lrp_networks; + } struct ds s = DS_EMPTY_INITIALIZER; - ds_put_cstr(&s, op->nbrp->mac); - for (int i = 0; i < op->nbrp->n_networks; ++i) { - ds_put_format(&s, " %s", op->nbrp->networks[i]); + ds_put_cstr(&s, networks->ea_s); + for (int i = 0; i < networks->n_ipv4_addrs; ++i) { + struct ipv4_netaddr addr = networks->ipv4_addrs[i]; + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); + } + /* We do not need the ipv6 LLA. Since it is last in the list we just + * skip it. */ + if (networks->n_ipv6_addrs > 1) { + for (int i = 0; i < networks->n_ipv6_addrs - 1; ++i) { + struct ipv6_netaddr addr = networks->ipv6_addrs[i]; + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen); + } } const char *addresses = ds_cstr(&s); sbrec_port_binding_set_mac(op->sb, &addresses, 1);