On Wed, Apr 19, 2023 at 2:41 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]>
>
Hi Xavier,
I have one comment down below other than that it looks good.
>
> ---
> v3: - fixed another pb->chassis not being cleared
> - avoid setting port down (and logging) if already in idl
> ---
> controller/binding.c | 20 ++++++-
> controller/binding.h | 4 ++
> controller/if-status.c | 102 +++++++++++++++++++++++++-----------
> controller/if-status.h | 1 +
> controller/ovn-controller.c | 1 +
> tests/system-ovn.at | 12 ++---
> 6 files changed, 99 insertions(+), 41 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 4e79c1c87..2aebae721 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -912,7 +912,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);
> @@ -3389,6 +3388,24 @@ binding_lport_delete(struct shash *binding_lports,
> binding_lport_destroy(b_lport);
> }
>
> +void
> +port_binding_set_down(struct ovsdb_idl_index *sbrec_port_binding_by_name,
> + const struct sbrec_chassis *chassis_rec,
> + const char *iface_id)
> +{
> + const struct sbrec_port_binding *pb = lport_lookup_by_name(
> + sbrec_port_binding_by_name, iface_id);
> +
> + if (!pb || sbrec_port_binding_is_deleted(pb)) {
>
I think this poses the same problem as the previous patch. This should be
probably
also solved by uuid lookup in the DB.
> + 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", iface_id);
> + set_pb_chassis_in_sbrec(pb, chassis_rec, false);
> + }
> +}
> +
> static void
> binding_lport_set_up(struct binding_lport *b_lport, bool sb_readonly)
> {
> @@ -3406,6 +3423,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 fdf59b813..8ba764f8c 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -204,6 +204,10 @@ 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(struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> + const struct sbrec_chassis *chassis_rec,
> + const char *iface_id);
> +
> /* 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 7caa65aca..aa0b95273 100644
> --- a/controller/if-status.c
> +++ b/controller/if-status.c
> @@ -16,6 +16,7 @@
> #include <config.h>
>
> #include "binding.h"
> +#include "lport.h"
> #include "if-status.h"
> #include "ofctrl-seqno.h"
> #include "simap.h"
> @@ -75,6 +76,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 +89,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,25 +142,34 @@ 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)
> */
>
> @@ -278,6 +293,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:
> @@ -306,9 +322,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:
> @@ -318,6 +334,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:
> @@ -338,9 +355,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:
> @@ -350,6 +367,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:
> @@ -403,6 +421,7 @@ void
> if_status_mgr_update(struct if_status_mgr *mgr,
> struct local_binding_data *binding_data,
> const struct sbrec_chassis *chassis_rec,
> + struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const struct ovsrec_interface_table *iface_table,
> bool ovs_readonly,
> bool sb_readonly)
> @@ -459,6 +478,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);
> @@ -506,6 +529,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(sbrec_port_binding_by_name, chassis_rec,
> + iface->id);
> + 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 6341b5dc6..922345e8e 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -36,6 +36,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,
> + struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> const struct ovsrec_interface_table
> *iface_table,
> bool ovs_readonly,
> bool sb_readonly);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 8ddc21f5e..909ef3823 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5243,6 +5243,7 @@ main(int argc, char *argv[])
> stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> time_msec());
> if_status_mgr_update(if_mgr, binding_data, chassis,
> + sbrec_port_binding_by_name,
> ovsrec_interface_table_get(
> ovs_idl_loop.idl),
> !ovs_idl_txn,
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index bf15d2aac..f697ebe87 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -10970,10 +10970,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
>
> @@ -11029,10 +11027,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
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales
--
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
