| Message ID | 20220725104951.391619-3-amusil@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix IPv6 prefix delegation and the test flakiness | 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 | fail | github build: failed |
Hi Ales, I have one small finding below, but other than that, it looks good. I don't think you need to roll a new version to address this issue, so Acked-by: Mark Michelson <mmichels@redhat.com> On 7/25/22 06:49, Ales Musil wrote: > The local_datapaths are populated by iterating through > all available port bindings or new ones added during I-C. > The port bindings are not ordered, it might happen that > the order between engine runs are different. This can cause > the prefix delegation shash to be with NULL local_datapath. > Even in cases that the port binding is actually part of > the local_datapath. Store only port binding in the shash > and during pinctrl_run check if the stored port binding > is part of local_datapaths. Because pinctrl runs only > after the engine computation is complete we should have > complete hmap of all local_datapaths. > > Fixes: 647920b6 ("controller: incrementally create ipv6 prefix delegation port_binding list") > Reported-at: https://bugzilla.redhat.com/2108726 > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > controller/binding.c | 35 ++++++++--------------------------- > controller/ovn-controller.c | 8 ++++---- > controller/ovn-controller.h | 6 ------ > controller/pinctrl.c | 18 ++++++++++-------- > 4 files changed, 22 insertions(+), 45 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index ba803ae3e..19b28369f 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -509,35 +509,20 @@ static void > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > struct shash *ras_pd_map) > { > - struct pb_ld_binding *ras_pd = > - shash_find_and_delete(ras_pd_map, pb->logical_port); > - > - free(ras_pd); > + shash_find_and_delete(ras_pd_map, pb->logical_port); > } > > static void > update_active_pb_ras_pd(const struct sbrec_port_binding *pb, > - struct hmap *local_datapaths, > struct shash *map, const char *conf) > { > bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); > struct shash_node *iter = shash_find(map, pb->logical_port); > - struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; > > - if (iter && !ras_pd_conf) { > + if (!ras_pd_conf && iter) { > shash_delete(map, iter); > - free(ras_pd); > - return; > - } > - if (ras_pd_conf) { > - if (!ras_pd) { > - ras_pd = xzalloc(sizeof *ras_pd); > - ras_pd->pb = pb; > - shash_add(map, pb->logical_port, ras_pd); > - } > - ovs_assert(ras_pd); > - ras_pd->ld = get_local_datapath(local_datapaths, > - pb->datapath->tunnel_key); > + } else if (ras_pd_conf && !iter) { > + shash_add(map, pb->logical_port, pb); > } > } > > @@ -1735,11 +1720,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) > const struct sbrec_port_binding *pb; > SBREC_PORT_BINDING_TABLE_FOR_EACH (pb, > b_ctx_in->port_binding_table) { > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > - b_ctx_out->local_active_ports_ipv6_pd, > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > - b_ctx_out->local_active_ports_ras, > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > "ipv6_ra_send_periodic"); > > enum en_lport_type lport_type = get_lport_type(pb); > @@ -2629,12 +2612,10 @@ delete_done: > continue; > } > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > - b_ctx_out->local_active_ports_ipv6_pd, > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > - b_ctx_out->local_active_ports_ras, > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > "ipv6_ra_send_periodic"); > > enum en_lport_type lport_type = get_lport_type(pb); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 2e9138036..5449743e8 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1312,8 +1312,8 @@ en_runtime_data_cleanup(void *data) > sset_destroy(&rt_data->egress_ifaces); > smap_destroy(&rt_data->local_iface_ids); > local_datapaths_destroy(&rt_data->local_datapaths); > - shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd); > - shash_destroy_free_data(&rt_data->local_active_ports_ras); > + shash_destroy(&rt_data->local_active_ports_ipv6_pd); > + shash_destroy(&rt_data->local_active_ports_ras); > local_binding_data_destroy(&rt_data->lbinding_data); > } > > @@ -1425,8 +1425,8 @@ en_runtime_data_run(struct engine_node *node, void *data) > first_run = false; > } else { > local_datapaths_destroy(local_datapaths); > - shash_clear_free_data(local_active_ipv6_pd); > - shash_clear_free_data(local_active_ras); > + shash_clear(local_active_ipv6_pd); > + shash_clear(local_active_ras); > local_binding_data_destroy(&rt_data->lbinding_data); > sset_destroy(local_lports); > related_lports_destroy(&rt_data->related_lports); > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > index df28c62cb..3a0e95377 100644 > --- a/controller/ovn-controller.h > +++ b/controller/ovn-controller.h > @@ -45,10 +45,4 @@ const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, > > uint32_t get_tunnel_type(const char *name); > > -struct pb_ld_binding { > - const struct sbrec_port_binding *pb; > - const struct local_datapath *ld; > - struct hmap_node hmap_node; > -}; > - > #endif /* controller/ovn-controller.h */ > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 38e8590af..5ea54e7d5 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1292,18 +1292,20 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct shash *local_active_ports_ipv6_pd, > const struct sbrec_chassis *chassis, > - const struct sset *active_tunnels) > + const struct sset *active_tunnels, > + const struct hmap *local_datapaths) > OVS_REQUIRES(pinctrl_mutex) > { > bool changed = false; > > struct shash_node *iter; > SHASH_FOR_EACH (iter, local_active_ports_ipv6_pd) { > - const struct pb_ld_binding *pb_ipv6 = iter->data; > - const struct sbrec_port_binding *pb = pb_ipv6->pb; > + const struct sbrec_port_binding *pb = iter->data; > + const struct local_datapath *ld = > + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); > int j; > > - if (!pb_ipv6->ld) { > + if (!ld) { > continue; > } > > @@ -1354,7 +1356,7 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, > in6_generate_lla(ea, &ip6_addr); > } > > - changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, pb_ipv6->ld, > + changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ld, > ea, ip6_addr, > peer->tunnel_key, > peer->datapath->tunnel_key); > @@ -3538,7 +3540,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > local_active_ports_ipv6_pd, chassis, > - active_tunnels); > + active_tunnels, local_datapaths); > sync_dns_cache(dns_table); > controller_event_run(ovnsb_idl_txn, ce_table, chassis); > ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths, > @@ -3982,8 +3984,8 @@ prepare_ipv6_ras(const struct shash *local_active_ports_ras, > > bool changed = false; > SHASH_FOR_EACH (iter, local_active_ports_ras) { > - const struct pb_ld_binding *ras = iter->data; > - const struct sbrec_port_binding *pb = ras->pb; > + const struct sbrec_port_binding *pb = iter->data; > + VLOG_INFO("Pop %p", pb); I'm guessing you meant to delete this? :) > > const char *peer_s = smap_get(&pb->options, "peer"); > if (!peer_s) {
Hi Mark, thank you for the review. On Mon, Jul 25, 2022 at 6:09 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Ales, I have one small finding below, but other than that, it looks > good. I don't think you need to roll a new version to address this issue, > so > > Acked-by: Mark Michelson <mmichels@redhat.com> > > On 7/25/22 06:49, Ales Musil wrote: > > The local_datapaths are populated by iterating through > > all available port bindings or new ones added during I-C. > > The port bindings are not ordered, it might happen that > > the order between engine runs are different. This can cause > > the prefix delegation shash to be with NULL local_datapath. > > Even in cases that the port binding is actually part of > > the local_datapath. Store only port binding in the shash > > and during pinctrl_run check if the stored port binding > > is part of local_datapaths. Because pinctrl runs only > > after the engine computation is complete we should have > > complete hmap of all local_datapaths. > > > > Fixes: 647920b6 ("controller: incrementally create ipv6 prefix > delegation port_binding list") > > Reported-at: https://bugzilla.redhat.com/2108726 > > Signed-off-by: Ales Musil <amusil@redhat.com> > > --- > > controller/binding.c | 35 ++++++++--------------------------- > > controller/ovn-controller.c | 8 ++++---- > > controller/ovn-controller.h | 6 ------ > > controller/pinctrl.c | 18 ++++++++++-------- > > 4 files changed, 22 insertions(+), 45 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index ba803ae3e..19b28369f 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -509,35 +509,20 @@ static void > > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > struct shash *ras_pd_map) > > { > > - struct pb_ld_binding *ras_pd = > > - shash_find_and_delete(ras_pd_map, pb->logical_port); > > - > > - free(ras_pd); > > + shash_find_and_delete(ras_pd_map, pb->logical_port); > > } > > > > static void > > update_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > - struct hmap *local_datapaths, > > struct shash *map, const char *conf) > > { > > bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); > > struct shash_node *iter = shash_find(map, pb->logical_port); > > - struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; > > > > - if (iter && !ras_pd_conf) { > > + if (!ras_pd_conf && iter) { > > shash_delete(map, iter); > > - free(ras_pd); > > - return; > > - } > > - if (ras_pd_conf) { > > - if (!ras_pd) { > > - ras_pd = xzalloc(sizeof *ras_pd); > > - ras_pd->pb = pb; > > - shash_add(map, pb->logical_port, ras_pd); > > - } > > - ovs_assert(ras_pd); > > - ras_pd->ld = get_local_datapath(local_datapaths, > > - pb->datapath->tunnel_key); > > + } else if (ras_pd_conf && !iter) { > > + shash_add(map, pb->logical_port, pb); > > } > > } > > > > @@ -1735,11 +1720,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, > struct binding_ctx_out *b_ctx_out) > > const struct sbrec_port_binding *pb; > > SBREC_PORT_BINDING_TABLE_FOR_EACH (pb, > > b_ctx_in->port_binding_table) { > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ipv6_pd, > > + update_active_pb_ras_pd(pb, > b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ras, > > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > > "ipv6_ra_send_periodic"); > > > > enum en_lport_type lport_type = get_lport_type(pb); > > @@ -2629,12 +2612,10 @@ delete_done: > > continue; > > } > > > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ipv6_pd, > > + update_active_pb_ras_pd(pb, > b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > > > - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, > > - b_ctx_out->local_active_ports_ras, > > + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, > > "ipv6_ra_send_periodic"); > > > > enum en_lport_type lport_type = get_lport_type(pb); > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > index 2e9138036..5449743e8 100644 > > --- a/controller/ovn-controller.c > > +++ b/controller/ovn-controller.c > > @@ -1312,8 +1312,8 @@ en_runtime_data_cleanup(void *data) > > sset_destroy(&rt_data->egress_ifaces); > > smap_destroy(&rt_data->local_iface_ids); > > local_datapaths_destroy(&rt_data->local_datapaths); > > - shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd); > > - shash_destroy_free_data(&rt_data->local_active_ports_ras); > > + shash_destroy(&rt_data->local_active_ports_ipv6_pd); > > + shash_destroy(&rt_data->local_active_ports_ras); > > local_binding_data_destroy(&rt_data->lbinding_data); > > } > > > > @@ -1425,8 +1425,8 @@ en_runtime_data_run(struct engine_node *node, void > *data) > > first_run = false; > > } else { > > local_datapaths_destroy(local_datapaths); > > - shash_clear_free_data(local_active_ipv6_pd); > > - shash_clear_free_data(local_active_ras); > > + shash_clear(local_active_ipv6_pd); > > + shash_clear(local_active_ras); > > local_binding_data_destroy(&rt_data->lbinding_data); > > sset_destroy(local_lports); > > related_lports_destroy(&rt_data->related_lports); > > diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h > > index df28c62cb..3a0e95377 100644 > > --- a/controller/ovn-controller.h > > +++ b/controller/ovn-controller.h > > @@ -45,10 +45,4 @@ const struct ovsrec_bridge *get_bridge(const struct > ovsrec_bridge_table *, > > > > uint32_t get_tunnel_type(const char *name); > > > > -struct pb_ld_binding { > > - const struct sbrec_port_binding *pb; > > - const struct local_datapath *ld; > > - struct hmap_node hmap_node; > > -}; > > - > > #endif /* controller/ovn-controller.h */ > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 38e8590af..5ea54e7d5 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -1292,18 +1292,20 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > struct ovsdb_idl_index > *sbrec_port_binding_by_name, > > const struct shash *local_active_ports_ipv6_pd, > > const struct sbrec_chassis *chassis, > > - const struct sset *active_tunnels) > > + const struct sset *active_tunnels, > > + const struct hmap *local_datapaths) > > OVS_REQUIRES(pinctrl_mutex) > > { > > bool changed = false; > > > > struct shash_node *iter; > > SHASH_FOR_EACH (iter, local_active_ports_ipv6_pd) { > > - const struct pb_ld_binding *pb_ipv6 = iter->data; > > - const struct sbrec_port_binding *pb = pb_ipv6->pb; > > + const struct sbrec_port_binding *pb = iter->data; > > + const struct local_datapath *ld = > > + get_local_datapath(local_datapaths, > pb->datapath->tunnel_key); > > int j; > > > > - if (!pb_ipv6->ld) { > > + if (!ld) { > > continue; > > } > > > > @@ -1354,7 +1356,7 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > in6_generate_lla(ea, &ip6_addr); > > } > > > > - changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, pb_ipv6->ld, > > + changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ld, > > ea, ip6_addr, > > peer->tunnel_key, > > peer->datapath->tunnel_key); > > @@ -3538,7 +3540,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > prepare_ipv6_ras(local_active_ports_ras, > sbrec_port_binding_by_name); > > prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, > > local_active_ports_ipv6_pd, chassis, > > - active_tunnels); > > + active_tunnels, local_datapaths); > > sync_dns_cache(dns_table); > > controller_event_run(ovnsb_idl_txn, ce_table, chassis); > > ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths, > > @@ -3982,8 +3984,8 @@ prepare_ipv6_ras(const struct shash > *local_active_ports_ras, > > > > bool changed = false; > > SHASH_FOR_EACH (iter, local_active_ports_ras) { > > - const struct pb_ld_binding *ras = iter->data; > > - const struct sbrec_port_binding *pb = ras->pb; > > + const struct sbrec_port_binding *pb = iter->data; > > + VLOG_INFO("Pop %p", pb); > > I'm guessing you meant to delete this? :) > Oops, yes that shouldn't be there. > > > > > const char *peer_s = smap_get(&pb->options, "peer"); > > if (!peer_s) { > > Thanks, Ales
diff --git a/controller/binding.c b/controller/binding.c index ba803ae3e..19b28369f 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -509,35 +509,20 @@ static void delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, struct shash *ras_pd_map) { - struct pb_ld_binding *ras_pd = - shash_find_and_delete(ras_pd_map, pb->logical_port); - - free(ras_pd); + shash_find_and_delete(ras_pd_map, pb->logical_port); } static void update_active_pb_ras_pd(const struct sbrec_port_binding *pb, - struct hmap *local_datapaths, struct shash *map, const char *conf) { bool ras_pd_conf = smap_get_bool(&pb->options, conf, false); struct shash_node *iter = shash_find(map, pb->logical_port); - struct pb_ld_binding *ras_pd = iter ? iter->data : NULL; - if (iter && !ras_pd_conf) { + if (!ras_pd_conf && iter) { shash_delete(map, iter); - free(ras_pd); - return; - } - if (ras_pd_conf) { - if (!ras_pd) { - ras_pd = xzalloc(sizeof *ras_pd); - ras_pd->pb = pb; - shash_add(map, pb->logical_port, ras_pd); - } - ovs_assert(ras_pd); - ras_pd->ld = get_local_datapath(local_datapaths, - pb->datapath->tunnel_key); + } else if (ras_pd_conf && !iter) { + shash_add(map, pb->logical_port, pb); } } @@ -1735,11 +1720,9 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct binding_ctx_out *b_ctx_out) const struct sbrec_port_binding *pb; SBREC_PORT_BINDING_TABLE_FOR_EACH (pb, b_ctx_in->port_binding_table) { - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, - b_ctx_out->local_active_ports_ipv6_pd, + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, "ipv6_prefix_delegation"); - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, - b_ctx_out->local_active_ports_ras, + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, "ipv6_ra_send_periodic"); enum en_lport_type lport_type = get_lport_type(pb); @@ -2629,12 +2612,10 @@ delete_done: continue; } - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, - b_ctx_out->local_active_ports_ipv6_pd, + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, "ipv6_prefix_delegation"); - update_active_pb_ras_pd(pb, b_ctx_out->local_datapaths, - b_ctx_out->local_active_ports_ras, + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ras, "ipv6_ra_send_periodic"); enum en_lport_type lport_type = get_lport_type(pb); diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 2e9138036..5449743e8 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1312,8 +1312,8 @@ en_runtime_data_cleanup(void *data) sset_destroy(&rt_data->egress_ifaces); smap_destroy(&rt_data->local_iface_ids); local_datapaths_destroy(&rt_data->local_datapaths); - shash_destroy_free_data(&rt_data->local_active_ports_ipv6_pd); - shash_destroy_free_data(&rt_data->local_active_ports_ras); + shash_destroy(&rt_data->local_active_ports_ipv6_pd); + shash_destroy(&rt_data->local_active_ports_ras); local_binding_data_destroy(&rt_data->lbinding_data); } @@ -1425,8 +1425,8 @@ en_runtime_data_run(struct engine_node *node, void *data) first_run = false; } else { local_datapaths_destroy(local_datapaths); - shash_clear_free_data(local_active_ipv6_pd); - shash_clear_free_data(local_active_ras); + shash_clear(local_active_ipv6_pd); + shash_clear(local_active_ras); local_binding_data_destroy(&rt_data->lbinding_data); sset_destroy(local_lports); related_lports_destroy(&rt_data->related_lports); diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h index df28c62cb..3a0e95377 100644 --- a/controller/ovn-controller.h +++ b/controller/ovn-controller.h @@ -45,10 +45,4 @@ const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *, uint32_t get_tunnel_type(const char *name); -struct pb_ld_binding { - const struct sbrec_port_binding *pb; - const struct local_datapath *ld; - struct hmap_node hmap_node; -}; - #endif /* controller/ovn-controller.h */ diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 38e8590af..5ea54e7d5 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -1292,18 +1292,20 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct shash *local_active_ports_ipv6_pd, const struct sbrec_chassis *chassis, - const struct sset *active_tunnels) + const struct sset *active_tunnels, + const struct hmap *local_datapaths) OVS_REQUIRES(pinctrl_mutex) { bool changed = false; struct shash_node *iter; SHASH_FOR_EACH (iter, local_active_ports_ipv6_pd) { - const struct pb_ld_binding *pb_ipv6 = iter->data; - const struct sbrec_port_binding *pb = pb_ipv6->pb; + const struct sbrec_port_binding *pb = iter->data; + const struct local_datapath *ld = + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); int j; - if (!pb_ipv6->ld) { + if (!ld) { continue; } @@ -1354,7 +1356,7 @@ prepare_ipv6_prefixd(struct ovsdb_idl_txn *ovnsb_idl_txn, in6_generate_lla(ea, &ip6_addr); } - changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, pb_ipv6->ld, + changed |= fill_ipv6_prefix_state(ovnsb_idl_txn, ld, ea, ip6_addr, peer->tunnel_key, peer->datapath->tunnel_key); @@ -3538,7 +3540,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, prepare_ipv6_ras(local_active_ports_ras, sbrec_port_binding_by_name); prepare_ipv6_prefixd(ovnsb_idl_txn, sbrec_port_binding_by_name, local_active_ports_ipv6_pd, chassis, - active_tunnels); + active_tunnels, local_datapaths); sync_dns_cache(dns_table); controller_event_run(ovnsb_idl_txn, ce_table, chassis); ip_mcast_sync(ovnsb_idl_txn, chassis, local_datapaths, @@ -3982,8 +3984,8 @@ prepare_ipv6_ras(const struct shash *local_active_ports_ras, bool changed = false; SHASH_FOR_EACH (iter, local_active_ports_ras) { - const struct pb_ld_binding *ras = iter->data; - const struct sbrec_port_binding *pb = ras->pb; + const struct sbrec_port_binding *pb = iter->data; + VLOG_INFO("Pop %p", pb); const char *peer_s = smap_get(&pb->options, "peer"); if (!peer_s) {
The local_datapaths are populated by iterating through all available port bindings or new ones added during I-C. The port bindings are not ordered, it might happen that the order between engine runs are different. This can cause the prefix delegation shash to be with NULL local_datapath. Even in cases that the port binding is actually part of the local_datapath. Store only port binding in the shash and during pinctrl_run check if the stored port binding is part of local_datapaths. Because pinctrl runs only after the engine computation is complete we should have complete hmap of all local_datapaths. Fixes: 647920b6 ("controller: incrementally create ipv6 prefix delegation port_binding list") Reported-at: https://bugzilla.redhat.com/2108726 Signed-off-by: Ales Musil <amusil@redhat.com> --- controller/binding.c | 35 ++++++++--------------------------- controller/ovn-controller.c | 8 ++++---- controller/ovn-controller.h | 6 ------ controller/pinctrl.c | 18 ++++++++++-------- 4 files changed, 22 insertions(+), 45 deletions(-)