| Message ID | 20230428115929.2208346-2-xsimonar@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v4,1/2] ovn-controller: fixed ovn-installed not always properly added or removed. | 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 |
On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <xsimonar@redhat.com> wrote: > When interface was unbound, the port was not always set down and the > port_binding->chassis not always removed. > > Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB > Port_Binding updates.") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150905 > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > --- > v3: - fixed another pb->chassis not being cleared > - avoid setting port down (and logging) if already in idl > v4: - handled Ales' comments: avoid using is_deleted outside of tracked > loops > --- > controller/binding.c | 20 ++++++- > controller/binding.h | 5 ++ > controller/if-status.c | 103 +++++++++++++++++++++++++----------- > controller/if-status.h | 1 + > controller/ovn-controller.c | 2 + > tests/system-ovn.at | 12 ++--- > 6 files changed, 102 insertions(+), 41 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 357e77609..bd810f669 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -899,7 +899,6 @@ local_binding_set_down(struct shash *local_bindings, > const char *pb_name, > > if (!sb_readonly && b_lport && b_lport->pb->n_up && > b_lport->pb->up[0] && > (!b_lport->pb->chassis || b_lport->pb->chassis == > chassis_rec)) { > - VLOG_INFO("Setting lport %s down in Southbound", pb_name); > binding_lport_set_down(b_lport, sb_readonly); > LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { > binding_lport_set_down(b_lport, sb_readonly); > @@ -3376,6 +3375,24 @@ binding_lport_delete(struct shash *binding_lports, > binding_lport_destroy(b_lport); > } > > +void > +port_binding_set_down(const struct sbrec_chassis *chassis_rec, > + const struct sbrec_port_binding_table *pb_table, > + const char *iface_id, > + const struct uuid *pb_uuid) > +{ > + const struct sbrec_port_binding *pb = > + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); > + if (!pb) { > + VLOG_DBG("port_binding already deleted for %s", iface_id); > + } else if (pb->n_up && pb->up[0]) { > + bool up = false; > + sbrec_port_binding_set_up(pb, &up, 1); > + VLOG_INFO("Setting lport %s down in Southbound", > pb->logical_port); > + set_pb_chassis_in_sbrec(pb, chassis_rec, false); > + } > +} > + > static void > binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) > { > @@ -3393,6 +3410,7 @@ binding_lport_set_down(struct binding_lport > *b_lport, bool sb_readonly) > if (sb_readonly || !b_lport || !b_lport->pb->n_up || > !b_lport->pb->up[0]) { > return; > } > + VLOG_INFO("Setting lport %s down in Southbound", b_lport->name); > > bool up = false; > sbrec_port_binding_set_up(b_lport->pb, &up, 1); > diff --git a/controller/binding.h b/controller/binding.h > index 0b9c6994f..5b73c6a4b 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -206,6 +206,11 @@ void set_pb_chassis_in_sbrec(const struct > sbrec_port_binding *pb, > void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *, > const struct uuid *); > > +void port_binding_set_down(const struct sbrec_chassis *chassis_rec, > + const struct sbrec_port_binding_table > *pb_table, > + const char *iface_id, > + const struct uuid *pb_uuid); > + > /* Corresponds to each Port_Binding.type. */ > enum en_lport_type { > LP_UNKNOWN, > diff --git a/controller/if-status.c b/controller/if-status.c > index ac36a9fb9..8503e5daa 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -75,6 +75,9 @@ enum if_state { > OIF_INSTALLED, /* Interface flows programmed in OVS and binding > * marked "up" in the binding module. > */ > + OIF_UPDATE_PORT, /* Logical ports need to be set down, and > pb->chassis > + * removed. > + */ > OIF_MAX, > }; > > @@ -85,18 +88,20 @@ static const char *if_state_names[] = { > [OIF_MARK_UP] = "MARK_UP", > [OIF_MARK_DOWN] = "MARK_DOWN", > [OIF_INSTALLED] = "INSTALLED", > + [OIF_UPDATE_PORT] = "UPDATE_PORT", > }; > > /* > * +----------------------+ > * +---> | | > - * | +-> | NULL | > <--------------------------------------+++-+ > - * | | +----------------------+ > | > - * | | ^ release_iface | claim_iface() > | > - * | | | V - sbrec_update_chassis(if sb is rw) > | > - * | | +----------------------+ > | > - * | | | | > <----------------------------------------+ | > - * | | | CLAIMED | > <--------------------------------------+ | | > + * | +-> | NULL | > + * | | +----------------------+ > + * | | ^ release_iface | claim_iface() > + * | | | V - sbrec_update_chassis(if sb is rw) > + * | | +----------------------+ > + * | | | | > <------------------------------------------+ > + * | | | CLAIMED | > <----------------------------------------+ | > + * | | | | > <--------------------------------------+ | | > * | | +----------------------+ > | | | > * | | | V ^ > | | | > * | | | | | handle_claims() > | | | > @@ -136,31 +141,41 @@ static const char *if_state_names[] = { > * | V V > | | | > * | +----------------------+ > | | | > * | | | mgr_run() > | | | > - * +-- | MARK_UP | - set port up in sb > | | | > - * | | - set ovn-installed in ovs > | | | > - * | | mgr_update() > | | | > - * +----------------------+ - sbrec_update_chassis if needed > | | | > - * | > | | | > - * | mgr_update(rcvd port up / ovn_installed & chassis set) > | | | > - * V > | | | > - * +----------------------+ > | | | > - * | INSTALLED | ------------> claim_iface > ---------------+ | | > - * +----------------------+ > | | > - * | > | | > - * | release_iface > | | > - * V > | | > - * +----------------------+ > | | > - * | | ------------> claim_iface > -----------------+ | > - * | MARK_DOWN | ------> mgr_update(rcvd port down) > ----------+ > - * | | mgr_run() > - * | | - set port down in sb > - * | | mgr_update() > + * +---| MARK_UP | - set port up in sb > | | | > + * | | | - set ovn-installed in ovs > | | | > + * | | | mgr_update() > | | | > + * | +----------------------+ - sbrec_update_chassis if needed > | | | > + * | | > | | | > + * | | mgr_update(rcvd port up / ovn_installed & chassis set) > | | | > + * | V > | | | > + * | +----------------------+ > | | | > + * | | INSTALLED | ------------> claim_iface > ---------------+ | | > + * | +----------------------+ > | | > + * | | > | | > + * | | release_iface > | | > + * |mgr_update( | > | | > + * | rcvd port down) | > | | > + * | V > | | > + * | +----------------------+ > | | > + * | | | ------------> claim_iface > -----------------+ | > + * +---+ MARK_DOWN | mgr_run() > | > + * | | | - set port down in sb > | > + * | | | mgr_update(sb is rw) > | > + * | +----------------------+ - sbrec_update_chassis(NULL) > | > + * | | > | > + * | | mgr_update(local binding not found) > | > + * | | > | > + * | V > | > + * | +----------------------+ > | > + * | | | ------------> claim_iface > -------------------+ > + * +---+ UPDATE_PORT | mgr_run() > * +----------------------+ - sbrec_update_chassis(NULL) > */ > > > struct ovs_iface { > char *id; /* Extracted from OVS external_ids.iface_id. > */ > + struct uuid pb_uuid; /* Port_binding uuid */ > enum if_state state; /* State of the interface in the state > machine. */ > uint32_t install_seqno; /* Seqno at which this interface is expected > to > * be fully programmed in OVS. Only used in > state > @@ -266,6 +281,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, > iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED); > } > > + memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); > if (!sb_readonly) { > set_pb_chassis_in_sbrec(pb, chassis_rec, true); > } > @@ -279,6 +295,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, > break; > case OIF_INSTALLED: > case OIF_MARK_DOWN: > + case OIF_UPDATE_PORT: > ovs_iface_set_state(mgr, iface, OIF_CLAIMED); > break; > case OIF_MAX: > @@ -307,9 +324,9 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, > const char *iface_id) > switch (iface->state) { > case OIF_CLAIMED: > case OIF_INSTALL_FLOWS: > - /* Not yet fully installed interfaces can be safely deleted. */ > - ovs_iface_destroy(mgr, iface); > - break; > + /* Not yet fully installed interfaces: > + * pb->chassis still need to be deleted. > + */ > case OIF_REM_OLD_OVN_INST: > case OIF_MARK_UP: > case OIF_INSTALLED: > @@ -319,6 +336,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, > const char *iface_id) > ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); > break; > case OIF_MARK_DOWN: > + case OIF_UPDATE_PORT: > /* Nothing to do here. */ > break; > case OIF_MAX: > @@ -339,9 +357,9 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, > const char *iface_id) > switch (iface->state) { > case OIF_CLAIMED: > case OIF_INSTALL_FLOWS: > - /* Not yet fully installed interfaces can be safely deleted. */ > - ovs_iface_destroy(mgr, iface); > - break; > + /* Not yet fully installed interfaces: > + * pb->chassis still need to be deleted. > + */ > case OIF_REM_OLD_OVN_INST: > case OIF_MARK_UP: > case OIF_INSTALLED: > @@ -351,6 +369,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, > const char *iface_id) > ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); > break; > case OIF_MARK_DOWN: > + case OIF_UPDATE_PORT: > /* Nothing to do here. */ > break; > case OIF_MAX: > @@ -405,6 +424,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, > struct local_binding_data *binding_data, > const struct sbrec_chassis *chassis_rec, > const struct ovsrec_interface_table *iface_table, > + const struct sbrec_port_binding_table *pb_table, > bool ovs_readonly, > bool sb_readonly) > { > @@ -460,6 +480,10 @@ if_status_mgr_update(struct if_status_mgr *mgr, > HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { > struct ovs_iface *iface = node->data; > > + if (!local_binding_find(bindings, iface->id)) { > + ovs_iface_set_state(mgr, iface, OIF_UPDATE_PORT); > + continue; > + } > if (!sb_readonly) { > local_binding_set_pb(bindings, iface->id, chassis_rec, > NULL, false); > @@ -507,6 +531,21 @@ if_status_mgr_update(struct if_status_mgr *mgr, > } > } > > + if (!sb_readonly) { > + HMAPX_FOR_EACH_SAFE (node, > &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { > + struct ovs_iface *iface = node->data; > + port_binding_set_down(chassis_rec, pb_table, iface->id, > + &iface->pb_uuid); > + ovs_iface_destroy(mgr, node->data); > + } > + } else { > + HMAPX_FOR_EACH_SAFE (node, > &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { > + struct ovs_iface *iface = node->data; > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_INFO_RL(&rl, "Not setting lport %s down as sb is > readonly", > + iface->id); > + } > + } > /* Register for a notification about flows being installed in OVS for > all > * newly claimed interfaces for which pb->chassis has been updated. > * Request a seqno update when the flows for new interfaces have been > diff --git a/controller/if-status.h b/controller/if-status.h > index 203764946..8ba80acd9 100644 > --- a/controller/if-status.h > +++ b/controller/if-status.h > @@ -37,6 +37,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, > const char *iface_id); > void if_status_mgr_update(struct if_status_mgr *, struct > local_binding_data *, > const struct sbrec_chassis *chassis, > const struct ovsrec_interface_table > *iface_table, > + const struct sbrec_port_binding_table *pb_table, > bool ovs_readonly, > bool sb_readonly); > void if_status_mgr_run(struct if_status_mgr *mgr, struct > local_binding_data *, > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 2d8fee4d6..de90025f0 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -5227,6 +5227,8 @@ main(int argc, char *argv[]) > if_status_mgr_update(if_mgr, binding_data, chassis, > ovsrec_interface_table_get( > ovs_idl_loop.idl), > + sbrec_port_binding_table_get( > + ovnsb_idl_loop.idl), > !ovs_idl_txn, > !ovnsb_idl_txn); > stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index cae44edee..d7b889a96 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10881,10 +10881,8 @@ sleep_controller > wake_up_sb > wake_up_controller > check_ovn_uninstalled > -# Port_down not always set on iface-id removal > -# check_ports_down > -# Port_Binding(chassis) not always removed on iface-id removal > -# check_ports_unbound > +check_ports_down > +check_ports_unbound > add_iface_ids > check ovn-nbctl --wait=hv sync > > @@ -10940,10 +10938,8 @@ sleep_controller > wake_up_sb > wake_up_controller > check_ovn_uninstalled > -# Port_down not always set on Interface removal > -# check_ports_down > -# Port_Binding(chassis) not always removed on Interface removal > -# check_ports_unbound > +check_ports_down > +check_ports_unbound > add_ovs_interfaces > check ovn-nbctl --wait=hv sync > > -- > 2.31.1 > > Looks good to me, thanks. Reviewed-by: Ales Musil <amusil@redhat.com>
On 5/5/23 11:21, Ales Musil wrote: > On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <xsimonar@redhat.com> wrote: > >> When interface was unbound, the port was not always set down and the >> port_binding->chassis not always removed. >> >> Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB >> Port_Binding updates.") >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150905 >> >> Signed-off-by: Xavier Simonart <xsimonar@redhat.com> >> >> --- >> v3: - fixed another pb->chassis not being cleared >> - avoid setting port down (and logging) if already in idl >> v4: - handled Ales' comments: avoid using is_deleted outside of tracked >> loops >> --- >> controller/binding.c | 20 ++++++- >> controller/binding.h | 5 ++ >> controller/if-status.c | 103 +++++++++++++++++++++++++----------- >> controller/if-status.h | 1 + >> controller/ovn-controller.c | 2 + >> tests/system-ovn.at | 12 ++--- >> 6 files changed, 102 insertions(+), 41 deletions(-) >> >> diff --git a/controller/binding.c b/controller/binding.c >> index 357e77609..bd810f669 100644 >> --- a/controller/binding.c >> +++ b/controller/binding.c >> @@ -899,7 +899,6 @@ local_binding_set_down(struct shash *local_bindings, >> const char *pb_name, >> >> if (!sb_readonly && b_lport && b_lport->pb->n_up && >> b_lport->pb->up[0] && >> (!b_lport->pb->chassis || b_lport->pb->chassis == >> chassis_rec)) { >> - VLOG_INFO("Setting lport %s down in Southbound", pb_name); >> binding_lport_set_down(b_lport, sb_readonly); >> LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { >> binding_lport_set_down(b_lport, sb_readonly); >> @@ -3376,6 +3375,24 @@ binding_lport_delete(struct shash *binding_lports, >> binding_lport_destroy(b_lport); >> } >> >> +void >> +port_binding_set_down(const struct sbrec_chassis *chassis_rec, >> + const struct sbrec_port_binding_table *pb_table, >> + const char *iface_id, >> + const struct uuid *pb_uuid) >> +{ >> + const struct sbrec_port_binding *pb = >> + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); >> + if (!pb) { >> + VLOG_DBG("port_binding already deleted for %s", iface_id); >> + } else if (pb->n_up && pb->up[0]) { >> + bool up = false; >> + sbrec_port_binding_set_up(pb, &up, 1); >> + VLOG_INFO("Setting lport %s down in Southbound", >> pb->logical_port); >> + set_pb_chassis_in_sbrec(pb, chassis_rec, false); >> + } >> +} >> + >> static void >> binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) >> { >> @@ -3393,6 +3410,7 @@ binding_lport_set_down(struct binding_lport >> *b_lport, bool sb_readonly) >> if (sb_readonly || !b_lport || !b_lport->pb->n_up || >> !b_lport->pb->up[0]) { >> return; >> } >> + VLOG_INFO("Setting lport %s down in Southbound", b_lport->name); >> >> bool up = false; >> sbrec_port_binding_set_up(b_lport->pb, &up, 1); >> diff --git a/controller/binding.h b/controller/binding.h >> index 0b9c6994f..5b73c6a4b 100644 >> --- a/controller/binding.h >> +++ b/controller/binding.h >> @@ -206,6 +206,11 @@ void set_pb_chassis_in_sbrec(const struct >> sbrec_port_binding *pb, >> void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *, >> const struct uuid *); >> >> +void port_binding_set_down(const struct sbrec_chassis *chassis_rec, >> + const struct sbrec_port_binding_table >> *pb_table, >> + const char *iface_id, >> + const struct uuid *pb_uuid); >> + >> /* Corresponds to each Port_Binding.type. */ >> enum en_lport_type { >> LP_UNKNOWN, >> diff --git a/controller/if-status.c b/controller/if-status.c >> index ac36a9fb9..8503e5daa 100644 >> --- a/controller/if-status.c >> +++ b/controller/if-status.c >> @@ -75,6 +75,9 @@ enum if_state { >> OIF_INSTALLED, /* Interface flows programmed in OVS and binding >> * marked "up" in the binding module. >> */ >> + OIF_UPDATE_PORT, /* Logical ports need to be set down, and >> pb->chassis >> + * removed. >> + */ >> OIF_MAX, >> }; >> >> @@ -85,18 +88,20 @@ static const char *if_state_names[] = { >> [OIF_MARK_UP] = "MARK_UP", >> [OIF_MARK_DOWN] = "MARK_DOWN", >> [OIF_INSTALLED] = "INSTALLED", >> + [OIF_UPDATE_PORT] = "UPDATE_PORT", >> }; >> >> /* >> * +----------------------+ >> * +---> | | >> - * | +-> | NULL | >> <--------------------------------------+++-+ >> - * | | +----------------------+ >> | >> - * | | ^ release_iface | claim_iface() >> | >> - * | | | V - sbrec_update_chassis(if sb is rw) >> | >> - * | | +----------------------+ >> | >> - * | | | | >> <----------------------------------------+ | >> - * | | | CLAIMED | >> <--------------------------------------+ | | >> + * | +-> | NULL | >> + * | | +----------------------+ >> + * | | ^ release_iface | claim_iface() >> + * | | | V - sbrec_update_chassis(if sb is rw) >> + * | | +----------------------+ >> + * | | | | >> <------------------------------------------+ >> + * | | | CLAIMED | >> <----------------------------------------+ | >> + * | | | | >> <--------------------------------------+ | | >> * | | +----------------------+ >> | | | >> * | | | V ^ >> | | | >> * | | | | | handle_claims() >> | | | >> @@ -136,31 +141,41 @@ static const char *if_state_names[] = { >> * | V V >> | | | >> * | +----------------------+ >> | | | >> * | | | mgr_run() >> | | | >> - * +-- | MARK_UP | - set port up in sb >> | | | >> - * | | - set ovn-installed in ovs >> | | | >> - * | | mgr_update() >> | | | >> - * +----------------------+ - sbrec_update_chassis if needed >> | | | >> - * | >> | | | >> - * | mgr_update(rcvd port up / ovn_installed & chassis set) >> | | | >> - * V >> | | | >> - * +----------------------+ >> | | | >> - * | INSTALLED | ------------> claim_iface >> ---------------+ | | >> - * +----------------------+ >> | | >> - * | >> | | >> - * | release_iface >> | | >> - * V >> | | >> - * +----------------------+ >> | | >> - * | | ------------> claim_iface >> -----------------+ | >> - * | MARK_DOWN | ------> mgr_update(rcvd port down) >> ----------+ >> - * | | mgr_run() >> - * | | - set port down in sb >> - * | | mgr_update() >> + * +---| MARK_UP | - set port up in sb >> | | | >> + * | | | - set ovn-installed in ovs >> | | | >> + * | | | mgr_update() >> | | | >> + * | +----------------------+ - sbrec_update_chassis if needed >> | | | >> + * | | >> | | | >> + * | | mgr_update(rcvd port up / ovn_installed & chassis set) >> | | | >> + * | V >> | | | >> + * | +----------------------+ >> | | | >> + * | | INSTALLED | ------------> claim_iface >> ---------------+ | | >> + * | +----------------------+ >> | | >> + * | | >> | | >> + * | | release_iface >> | | >> + * |mgr_update( | >> | | >> + * | rcvd port down) | >> | | >> + * | V >> | | >> + * | +----------------------+ >> | | >> + * | | | ------------> claim_iface >> -----------------+ | >> + * +---+ MARK_DOWN | mgr_run() >> | >> + * | | | - set port down in sb >> | >> + * | | | mgr_update(sb is rw) >> | >> + * | +----------------------+ - sbrec_update_chassis(NULL) >> | >> + * | | >> | >> + * | | mgr_update(local binding not found) >> | >> + * | | >> | >> + * | V >> | >> + * | +----------------------+ >> | >> + * | | | ------------> claim_iface >> -------------------+ >> + * +---+ UPDATE_PORT | mgr_run() >> * +----------------------+ - sbrec_update_chassis(NULL) >> */ >> >> >> struct ovs_iface { >> char *id; /* Extracted from OVS external_ids.iface_id. >> */ >> + struct uuid pb_uuid; /* Port_binding uuid */ >> enum if_state state; /* State of the interface in the state >> machine. */ >> uint32_t install_seqno; /* Seqno at which this interface is expected >> to >> * be fully programmed in OVS. Only used in >> state >> @@ -266,6 +281,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, >> iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED); >> } >> >> + memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); >> if (!sb_readonly) { >> set_pb_chassis_in_sbrec(pb, chassis_rec, true); >> } >> @@ -279,6 +295,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, >> break; >> case OIF_INSTALLED: >> case OIF_MARK_DOWN: >> + case OIF_UPDATE_PORT: >> ovs_iface_set_state(mgr, iface, OIF_CLAIMED); >> break; >> case OIF_MAX: >> @@ -307,9 +324,9 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, >> const char *iface_id) >> switch (iface->state) { >> case OIF_CLAIMED: >> case OIF_INSTALL_FLOWS: >> - /* Not yet fully installed interfaces can be safely deleted. */ >> - ovs_iface_destroy(mgr, iface); >> - break; >> + /* Not yet fully installed interfaces: >> + * pb->chassis still need to be deleted. >> + */ >> case OIF_REM_OLD_OVN_INST: >> case OIF_MARK_UP: >> case OIF_INSTALLED: >> @@ -319,6 +336,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, >> const char *iface_id) >> ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); >> break; >> case OIF_MARK_DOWN: >> + case OIF_UPDATE_PORT: >> /* Nothing to do here. */ >> break; >> case OIF_MAX: >> @@ -339,9 +357,9 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, >> const char *iface_id) >> switch (iface->state) { >> case OIF_CLAIMED: >> case OIF_INSTALL_FLOWS: >> - /* Not yet fully installed interfaces can be safely deleted. */ >> - ovs_iface_destroy(mgr, iface); >> - break; >> + /* Not yet fully installed interfaces: >> + * pb->chassis still need to be deleted. >> + */ >> case OIF_REM_OLD_OVN_INST: >> case OIF_MARK_UP: >> case OIF_INSTALLED: >> @@ -351,6 +369,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, >> const char *iface_id) >> ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); >> break; >> case OIF_MARK_DOWN: >> + case OIF_UPDATE_PORT: >> /* Nothing to do here. */ >> break; >> case OIF_MAX: >> @@ -405,6 +424,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> struct local_binding_data *binding_data, >> const struct sbrec_chassis *chassis_rec, >> const struct ovsrec_interface_table *iface_table, >> + const struct sbrec_port_binding_table *pb_table, >> bool ovs_readonly, >> bool sb_readonly) >> { >> @@ -460,6 +480,10 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { >> struct ovs_iface *iface = node->data; >> >> + if (!local_binding_find(bindings, iface->id)) { >> + ovs_iface_set_state(mgr, iface, OIF_UPDATE_PORT); >> + continue; >> + } >> if (!sb_readonly) { >> local_binding_set_pb(bindings, iface->id, chassis_rec, >> NULL, false); >> @@ -507,6 +531,21 @@ if_status_mgr_update(struct if_status_mgr *mgr, >> } >> } >> >> + if (!sb_readonly) { >> + HMAPX_FOR_EACH_SAFE (node, >> &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { >> + struct ovs_iface *iface = node->data; >> + port_binding_set_down(chassis_rec, pb_table, iface->id, >> + &iface->pb_uuid); >> + ovs_iface_destroy(mgr, node->data); >> + } >> + } else { >> + HMAPX_FOR_EACH_SAFE (node, >> &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { >> + struct ovs_iface *iface = node->data; >> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> + VLOG_INFO_RL(&rl, "Not setting lport %s down as sb is >> readonly", >> + iface->id); >> + } >> + } >> /* Register for a notification about flows being installed in OVS for >> all >> * newly claimed interfaces for which pb->chassis has been updated. >> * Request a seqno update when the flows for new interfaces have been >> diff --git a/controller/if-status.h b/controller/if-status.h >> index 203764946..8ba80acd9 100644 >> --- a/controller/if-status.h >> +++ b/controller/if-status.h >> @@ -37,6 +37,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, >> const char *iface_id); >> void if_status_mgr_update(struct if_status_mgr *, struct >> local_binding_data *, >> const struct sbrec_chassis *chassis, >> const struct ovsrec_interface_table >> *iface_table, >> + const struct sbrec_port_binding_table *pb_table, >> bool ovs_readonly, >> bool sb_readonly); >> void if_status_mgr_run(struct if_status_mgr *mgr, struct >> local_binding_data *, >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 2d8fee4d6..de90025f0 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -5227,6 +5227,8 @@ main(int argc, char *argv[]) >> if_status_mgr_update(if_mgr, binding_data, chassis, >> ovsrec_interface_table_get( >> ovs_idl_loop.idl), >> + sbrec_port_binding_table_get( >> + ovnsb_idl_loop.idl), >> !ovs_idl_txn, >> !ovnsb_idl_txn); >> stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, >> diff --git a/tests/system-ovn.at b/tests/system-ovn.at >> index cae44edee..d7b889a96 100644 >> --- a/tests/system-ovn.at >> +++ b/tests/system-ovn.at >> @@ -10881,10 +10881,8 @@ sleep_controller >> wake_up_sb >> wake_up_controller >> check_ovn_uninstalled >> -# Port_down not always set on iface-id removal >> -# check_ports_down >> -# Port_Binding(chassis) not always removed on iface-id removal >> -# check_ports_unbound >> +check_ports_down >> +check_ports_unbound >> add_iface_ids >> check ovn-nbctl --wait=hv sync >> >> @@ -10940,10 +10938,8 @@ sleep_controller >> wake_up_sb >> wake_up_controller >> check_ovn_uninstalled >> -# Port_down not always set on Interface removal >> -# check_ports_down >> -# Port_Binding(chassis) not always removed on Interface removal >> -# check_ports_unbound >> +check_ports_down >> +check_ports_unbound >> add_ovs_interfaces >> check ovn-nbctl --wait=hv sync >> >> -- >> 2.31.1 >> >> > Looks good to me, thanks. > > Reviewed-by: Ales Musil <amusil@redhat.com> > Thanks, Xavier and Ales! I applied this to the main branch and backported it to stable branches down to 22.09. I have the same mention about older branches, could you please prepare a custom backport if you have time? Regards, Dumitru
diff --git a/controller/binding.c b/controller/binding.c index 357e77609..bd810f669 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -899,7 +899,6 @@ local_binding_set_down(struct shash *local_bindings, const char *pb_name, if (!sb_readonly && b_lport && b_lport->pb->n_up && b_lport->pb->up[0] && (!b_lport->pb->chassis || b_lport->pb->chassis == chassis_rec)) { - VLOG_INFO("Setting lport %s down in Southbound", pb_name); binding_lport_set_down(b_lport, sb_readonly); LIST_FOR_EACH (b_lport, list_node, &lbinding->binding_lports) { binding_lport_set_down(b_lport, sb_readonly); @@ -3376,6 +3375,24 @@ binding_lport_delete(struct shash *binding_lports, binding_lport_destroy(b_lport); } +void +port_binding_set_down(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid) +{ + const struct sbrec_port_binding *pb = + sbrec_port_binding_table_get_for_uuid(pb_table, pb_uuid); + if (!pb) { + VLOG_DBG("port_binding already deleted for %s", iface_id); + } else if (pb->n_up && pb->up[0]) { + bool up = false; + sbrec_port_binding_set_up(pb, &up, 1); + VLOG_INFO("Setting lport %s down in Southbound", pb->logical_port); + set_pb_chassis_in_sbrec(pb, chassis_rec, false); + } +} + static void binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly) { @@ -3393,6 +3410,7 @@ binding_lport_set_down(struct binding_lport *b_lport, bool sb_readonly) if (sb_readonly || !b_lport || !b_lport->pb->n_up || !b_lport->pb->up[0]) { return; } + VLOG_INFO("Setting lport %s down in Southbound", b_lport->name); bool up = false; sbrec_port_binding_set_up(b_lport->pb, &up, 1); diff --git a/controller/binding.h b/controller/binding.h index 0b9c6994f..5b73c6a4b 100644 --- a/controller/binding.h +++ b/controller/binding.h @@ -206,6 +206,11 @@ void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb, void remove_ovn_installed_for_uuid(const struct ovsrec_interface_table *, const struct uuid *); +void port_binding_set_down(const struct sbrec_chassis *chassis_rec, + const struct sbrec_port_binding_table *pb_table, + const char *iface_id, + const struct uuid *pb_uuid); + /* Corresponds to each Port_Binding.type. */ enum en_lport_type { LP_UNKNOWN, diff --git a/controller/if-status.c b/controller/if-status.c index ac36a9fb9..8503e5daa 100644 --- a/controller/if-status.c +++ b/controller/if-status.c @@ -75,6 +75,9 @@ enum if_state { OIF_INSTALLED, /* Interface flows programmed in OVS and binding * marked "up" in the binding module. */ + OIF_UPDATE_PORT, /* Logical ports need to be set down, and pb->chassis + * removed. + */ OIF_MAX, }; @@ -85,18 +88,20 @@ static const char *if_state_names[] = { [OIF_MARK_UP] = "MARK_UP", [OIF_MARK_DOWN] = "MARK_DOWN", [OIF_INSTALLED] = "INSTALLED", + [OIF_UPDATE_PORT] = "UPDATE_PORT", }; /* * +----------------------+ * +---> | | - * | +-> | NULL | <--------------------------------------+++-+ - * | | +----------------------+ | - * | | ^ release_iface | claim_iface() | - * | | | V - sbrec_update_chassis(if sb is rw) | - * | | +----------------------+ | - * | | | | <----------------------------------------+ | - * | | | CLAIMED | <--------------------------------------+ | | + * | +-> | NULL | + * | | +----------------------+ + * | | ^ release_iface | claim_iface() + * | | | V - sbrec_update_chassis(if sb is rw) + * | | +----------------------+ + * | | | | <------------------------------------------+ + * | | | CLAIMED | <----------------------------------------+ | + * | | | | <--------------------------------------+ | | * | | +----------------------+ | | | * | | | V ^ | | | * | | | | | handle_claims() | | | @@ -136,31 +141,41 @@ static const char *if_state_names[] = { * | V V | | | * | +----------------------+ | | | * | | | mgr_run() | | | - * +-- | MARK_UP | - set port up in sb | | | - * | | - set ovn-installed in ovs | | | - * | | mgr_update() | | | - * +----------------------+ - sbrec_update_chassis if needed | | | - * | | | | - * | mgr_update(rcvd port up / ovn_installed & chassis set) | | | - * V | | | - * +----------------------+ | | | - * | INSTALLED | ------------> claim_iface ---------------+ | | - * +----------------------+ | | - * | | | - * | release_iface | | - * V | | - * +----------------------+ | | - * | | ------------> claim_iface -----------------+ | - * | MARK_DOWN | ------> mgr_update(rcvd port down) ----------+ - * | | mgr_run() - * | | - set port down in sb - * | | mgr_update() + * +---| MARK_UP | - set port up in sb | | | + * | | | - set ovn-installed in ovs | | | + * | | | mgr_update() | | | + * | +----------------------+ - sbrec_update_chassis if needed | | | + * | | | | | + * | | mgr_update(rcvd port up / ovn_installed & chassis set) | | | + * | V | | | + * | +----------------------+ | | | + * | | INSTALLED | ------------> claim_iface ---------------+ | | + * | +----------------------+ | | + * | | | | + * | | release_iface | | + * |mgr_update( | | | + * | rcvd port down) | | | + * | V | | + * | +----------------------+ | | + * | | | ------------> claim_iface -----------------+ | + * +---+ MARK_DOWN | mgr_run() | + * | | | - set port down in sb | + * | | | mgr_update(sb is rw) | + * | +----------------------+ - sbrec_update_chassis(NULL) | + * | | | + * | | mgr_update(local binding not found) | + * | | | + * | V | + * | +----------------------+ | + * | | | ------------> claim_iface -------------------+ + * +---+ UPDATE_PORT | mgr_run() * +----------------------+ - sbrec_update_chassis(NULL) */ struct ovs_iface { char *id; /* Extracted from OVS external_ids.iface_id. */ + struct uuid pb_uuid; /* Port_binding uuid */ enum if_state state; /* State of the interface in the state machine. */ uint32_t install_seqno; /* Seqno at which this interface is expected to * be fully programmed in OVS. Only used in state @@ -266,6 +281,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, iface = ovs_iface_create(mgr, iface_id, OIF_CLAIMED); } + memcpy(&iface->pb_uuid, &pb->header_.uuid, sizeof(iface->pb_uuid)); if (!sb_readonly) { set_pb_chassis_in_sbrec(pb, chassis_rec, true); } @@ -279,6 +295,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr, break; case OIF_INSTALLED: case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: ovs_iface_set_state(mgr, iface, OIF_CLAIMED); break; case OIF_MAX: @@ -307,9 +324,9 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) switch (iface->state) { case OIF_CLAIMED: case OIF_INSTALL_FLOWS: - /* Not yet fully installed interfaces can be safely deleted. */ - ovs_iface_destroy(mgr, iface); - break; + /* Not yet fully installed interfaces: + * pb->chassis still need to be deleted. + */ case OIF_REM_OLD_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: @@ -319,6 +336,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr, const char *iface_id) ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); break; case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: /* Nothing to do here. */ break; case OIF_MAX: @@ -339,9 +357,9 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) switch (iface->state) { case OIF_CLAIMED: case OIF_INSTALL_FLOWS: - /* Not yet fully installed interfaces can be safely deleted. */ - ovs_iface_destroy(mgr, iface); - break; + /* Not yet fully installed interfaces: + * pb->chassis still need to be deleted. + */ case OIF_REM_OLD_OVN_INST: case OIF_MARK_UP: case OIF_INSTALLED: @@ -351,6 +369,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const char *iface_id) ovs_iface_set_state(mgr, iface, OIF_MARK_DOWN); break; case OIF_MARK_DOWN: + case OIF_UPDATE_PORT: /* Nothing to do here. */ break; case OIF_MAX: @@ -405,6 +424,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, struct local_binding_data *binding_data, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool ovs_readonly, bool sb_readonly) { @@ -460,6 +480,10 @@ if_status_mgr_update(struct if_status_mgr *mgr, HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_MARK_DOWN]) { struct ovs_iface *iface = node->data; + if (!local_binding_find(bindings, iface->id)) { + ovs_iface_set_state(mgr, iface, OIF_UPDATE_PORT); + continue; + } if (!sb_readonly) { local_binding_set_pb(bindings, iface->id, chassis_rec, NULL, false); @@ -507,6 +531,21 @@ if_status_mgr_update(struct if_status_mgr *mgr, } } + if (!sb_readonly) { + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { + struct ovs_iface *iface = node->data; + port_binding_set_down(chassis_rec, pb_table, iface->id, + &iface->pb_uuid); + ovs_iface_destroy(mgr, node->data); + } + } else { + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) { + struct ovs_iface *iface = node->data; + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_INFO_RL(&rl, "Not setting lport %s down as sb is readonly", + iface->id); + } + } /* Register for a notification about flows being installed in OVS for all * newly claimed interfaces for which pb->chassis has been updated. * Request a seqno update when the flows for new interfaces have been diff --git a/controller/if-status.h b/controller/if-status.h index 203764946..8ba80acd9 100644 --- a/controller/if-status.h +++ b/controller/if-status.h @@ -37,6 +37,7 @@ void if_status_mgr_delete_iface(struct if_status_mgr *, const char *iface_id); void if_status_mgr_update(struct if_status_mgr *, struct local_binding_data *, const struct sbrec_chassis *chassis, const struct ovsrec_interface_table *iface_table, + const struct sbrec_port_binding_table *pb_table, bool ovs_readonly, bool sb_readonly); void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data *, diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 2d8fee4d6..de90025f0 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -5227,6 +5227,8 @@ main(int argc, char *argv[]) if_status_mgr_update(if_mgr, binding_data, chassis, ovsrec_interface_table_get( ovs_idl_loop.idl), + sbrec_port_binding_table_get( + ovnsb_idl_loop.idl), !ovs_idl_txn, !ovnsb_idl_txn); stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, diff --git a/tests/system-ovn.at b/tests/system-ovn.at index cae44edee..d7b889a96 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -10881,10 +10881,8 @@ sleep_controller wake_up_sb wake_up_controller check_ovn_uninstalled -# Port_down not always set on iface-id removal -# check_ports_down -# Port_Binding(chassis) not always removed on iface-id removal -# check_ports_unbound +check_ports_down +check_ports_unbound add_iface_ids check ovn-nbctl --wait=hv sync @@ -10940,10 +10938,8 @@ sleep_controller wake_up_sb wake_up_controller check_ovn_uninstalled -# Port_down not always set on Interface removal -# check_ports_down -# Port_Binding(chassis) not always removed on Interface removal -# check_ports_unbound +check_ports_down +check_ports_unbound add_ovs_interfaces check ovn-nbctl --wait=hv sync
When interface was unbound, the port was not always set down and the port_binding->chassis not always removed. Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB Port_Binding updates.") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2150905 Signed-off-by: Xavier Simonart <xsimonar@redhat.com> --- v3: - fixed another pb->chassis not being cleared - avoid setting port down (and logging) if already in idl v4: - handled Ales' comments: avoid using is_deleted outside of tracked loops --- controller/binding.c | 20 ++++++- controller/binding.h | 5 ++ controller/if-status.c | 103 +++++++++++++++++++++++++----------- controller/if-status.h | 1 + controller/ovn-controller.c | 2 + tests/system-ovn.at | 12 ++--- 6 files changed, 102 insertions(+), 41 deletions(-)