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
---
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)) {
+ 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
