On 1/4/23 09:32, Xavier Simonart 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,
> controller/binding.c | 2 +-
> controller/if-status.c | 44 +++++++++++++++++++++++++++++++++++++
> controller/if-status.h | 4 ++++
> controller/ovn-controller.c | 12 ++++++++++
> tests/ovn.at | 12 ++++------
> 5 files changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index d85a17730..eb8d580c8 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -894,7 +894,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);
> @@ -3384,6 +3383,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/if-status.c b/controller/if-status.c
> index c008aa79e..26535c9e8 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,6 +89,7 @@ static const char *if_state_names[] = {
> [OIF_MARK_UP] = "MARK_UP",
> [OIF_MARK_DOWN] = "MARK_DOWN",
> [OIF_INSTALLED] = "INSTALLED",
> + [OIF_UPDATE_PORT] = "UPDATE_PORT",
> };
>
> /*
> @@ -264,6 +269,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:
> @@ -304,6 +310,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:
> @@ -336,6 +343,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:
> @@ -344,6 +352,36 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
> const char *iface_id)
> }
> }
>
> +void
> +if_status_handle_lports(struct if_status_mgr *mgr,
> + const struct sbrec_chassis *chassis_rec,
> + struct ovsdb_idl_index *sbrec_port_binding_by_name,
> + bool sb_readonly)
> +{
> + if (sb_readonly) {
> + return;
> + }
> +
> + struct hmapx_node *node;
> +
> + HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) {
> + struct ovs_iface *iface = node->data;
> + const struct sbrec_port_binding *pb = lport_lookup_by_name(
> + sbrec_port_binding_by_name, iface->id);
> +
> + if ((pb == NULL) || sbrec_port_binding_is_deleted(pb)) {
Nit: s/(pb == NULL)/!pb/
> + VLOG_DBG("Removing lport %s from list of ports to set down",
> + iface->id);
> + } else {
> + bool up = false;
> + sbrec_port_binding_set_up(pb, &up, 1);
> + VLOG_INFO("Setting lport %s down in Southbound", iface->id);
We really avoided changing IDL records in if-status.c until now. It
makes it more contained. I think all port binding updates should happen
in binding.c
Maybe we need to wrap this whole if/else into a new function exposed by
binding.c
> + set_pb_chassis_in_sbrec(pb, chassis_rec, false);
> + }
> + ovs_iface_destroy(mgr, node->data);
> + }
> +}
> +
> bool
> if_status_handle_claims(struct if_status_mgr *mgr,
> struct local_binding_data *binding_data,
> @@ -424,6 +462,12 @@ 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) && sb_readonly) {
> + VLOG_DBG("%s not found in local_bindings and sb read only => "
> + "port down delayed", 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);
> diff --git a/controller/if-status.h b/controller/if-status.h
> index 5bd187a25..593597af7 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -48,5 +48,9 @@ bool if_status_handle_claims(struct if_status_mgr *mgr,
> const struct sbrec_chassis *chassis_rec,
> struct hmap *tracked_datapath,
> bool sb_readonly);
> +void if_status_handle_lports(struct if_status_mgr *mgr,
> + const struct sbrec_chassis *chassis_rec,
> + struct ovsdb_idl_index
> *sbrec_port_binding_by_name,
> + bool sb_readonly);
>
> # endif /* controller/if-status.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 814a88117..4094eb56e 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1603,6 +1603,11 @@ runtime_data_sb_ro_handler(struct engine_node *node,
> void *data)
> engine_get_input("SB_chassis", node),
> "name");
>
> + struct ovsdb_idl_index *sbrec_port_binding_by_name =
> + engine_ovsdb_node_get_index(
> + engine_get_input("SB_port_binding", node),
> + "name");
> +
> if (chassis_id) {
> chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
> }
> @@ -1620,6 +1625,13 @@ runtime_data_sb_ro_handler(struct engine_node *node,
> void *data)
> engine_set_node_state(node, EN_UPDATED);
> rt_data->tracked = true;
> }
> +
> + if (sbrec_port_binding_by_name) {
> + if_status_handle_lports(ctrl_ctx->if_mgr,
> + chassis,
> + sbrec_port_binding_by_name,
> + sb_readonly);
> + }
It seems a bit strange to me that we only do this during the incremental
processing stage. Why can't we do this inside if_status_mgr_update()?
> }
> return true;
> }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index a0378a728..a849a483d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -34403,10 +34403,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
>
> @@ -34443,10 +34441,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
>
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
