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]>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
