On 5/5/23 11:21, Ales Musil wrote: > On Fri, Apr 28, 2023 at 1:59 PM Xavier Simonart <[email protected]> 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 <[email protected]> >> >> --- >> 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 <[email protected]> >
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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
