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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
