OVN checks whether ovn-installed is already present (in OVS) before updating it.
This might cause ovn-installed related issues in the following case:
- (1) ovn-installed is present
- (2) we claim the interface
- (3) we update ovs, removing ovn-installed and start installing flows
- (4) (next loop), after flows installed, we check if ovn-installed is absent,
and if yes, we update OVS with ovn-installed.
However, in step4, if OVS is still busy from step 3, ovn-installed is read as
present; hence OVN controller does not update it and move to INSTALLED state.
ovn-installed was also not properly deleted on port or binding removal.
Note that port->up is not properly removed on external_ids:iface-id removal when
sb is read-only. This will be fixed in a further patch.
Fixes: a7c7d4519e50 ("controller: avoid recomputes triggered by SBDB
Port_Binding updates.")
Signed-off-by: Xavier Simonart <[email protected]>
v2: - handled Dumitru's comments
- rebased on main
- updated state machine diagram as commented in v1 commit message
- remove ovn-installed on port deletion or external_ids:iface-id removal.
- added test case
---
controller/binding.c | 59 +++++++++-
controller/binding.h | 6 +
controller/if-status.c | 113 +++++++++++++-----
tests/ovn.at | 257 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 403 insertions(+), 32 deletions(-)
diff --git a/controller/binding.c b/controller/binding.c
index 5df62baef..d85a17730 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -746,6 +746,19 @@ local_binding_get_lport_ofport(const struct shash
*local_bindings,
u16_to_ofp(lbinding->iface->ofport[0]) : 0;
}
+bool
+local_binding_is_ovn_installed(struct shash *local_bindings,
+ const char *pb_name)
+{
+ struct local_binding *lbinding =
+ local_binding_find(local_bindings, pb_name);
+ if (lbinding && lbinding->iface) {
+ return smap_get_bool(&lbinding->iface->external_ids,
+ OVN_INSTALLED_EXT_ID, false);
+ }
+ return false;
+}
+
bool
local_binding_is_up(struct shash *local_bindings, const char *pb_name,
const struct sbrec_chassis *chassis_rec)
@@ -783,6 +796,7 @@ local_binding_is_down(struct shash *local_bindings, const
char *pb_name,
} else if (b_lport->pb->chassis) {
VLOG_DBG("lport %s already claimed by other chassis",
b_lport->pb->logical_port);
+ return true;
}
}
@@ -834,6 +848,33 @@ local_binding_set_up(struct shash *local_bindings, const
char *pb_name,
}
}
+static void
+remove_ovn_installed(struct local_binding *lbinding, const char *pb_name)
+{
+ if (lbinding && lbinding->iface &&
+ smap_get_bool(&lbinding->iface->external_ids,
+ OVN_INSTALLED_EXT_ID, false)) {
+ /* If iface has been deleted, do not try to delete a key from it */
+ if (!ovsrec_interface_is_deleted(lbinding->iface)) {
+ VLOG_INFO("Removing lport %s ovn-installed in OVS", pb_name);
+ ovsrec_interface_update_external_ids_delkey(lbinding->iface,
+ OVN_INSTALLED_EXT_ID);
+ }
+ }
+}
+
+void
+local_binding_remove_ovn_installed(struct shash *local_bindings,
+ const char *pb_name, bool ovs_readonly)
+{
+ if (ovs_readonly) {
+ return;
+ }
+ struct local_binding *lbinding =
+ local_binding_find(local_bindings, pb_name);
+ remove_ovn_installed(lbinding, pb_name);
+}
+
void
local_binding_set_down(struct shash *local_bindings, const char *pb_name,
const struct sbrec_chassis *chassis_rec,
@@ -1239,7 +1280,9 @@ claim_lport(const struct sbrec_port_binding *pb,
return false;
}
} else {
- if (pb->n_up && !pb->up[0]) {
+ if ((pb->n_up && !pb->up[0]) ||
+ !smap_get_bool(&iface_rec->external_ids,
+ OVN_INSTALLED_EXT_ID, false)) {
if_status_mgr_claim_iface(if_mgr, pb, chassis_rec,
sb_readonly);
}
@@ -1464,9 +1507,11 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
const char *requested_chassis_option = smap_get(
&pb->options, "requested-chassis");
VLOG_INFO_RL(&rl,
- "Not claiming lport %s, chassis %s requested-chassis %s",
+ "Not claiming lport %s, chassis %s requested-chassis %s "
+ "pb->chassis %s",
pb->logical_port, b_ctx_in->chassis_rec->name,
- requested_chassis_option ? requested_chassis_option : "[]");
+ requested_chassis_option ? requested_chassis_option : "[]",
+ pb->chassis ? pb->chassis->name: "");
}
}
@@ -2288,6 +2333,9 @@ consider_iface_release(const struct ovsrec_interface
*iface_rec,
return false;
}
}
+ if (b_ctx_in->ovs_idl_txn) {
+ remove_ovn_installed(lbinding, iface_id);
+ }
} else if (lbinding && b_lport && b_lport->type == LP_LOCALPORT) {
/* lbinding is associated with a localport. Remove it from the
@@ -2558,6 +2606,7 @@ handle_deleted_lport(const struct sbrec_port_binding *pb,
if (ld) {
remove_pb_from_local_datapath(pb,
b_ctx_out, ld);
+ if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
return;
}
@@ -2581,6 +2630,7 @@ handle_deleted_lport(const struct sbrec_port_binding *pb,
remove_pb_from_local_datapath(pb, b_ctx_out,
ld);
}
+ if_status_mgr_release_iface(b_ctx_out->if_mgr, pb->logical_port);
}
}
@@ -2627,6 +2677,9 @@ handle_deleted_vif_lport(const struct sbrec_port_binding
*pb,
}
handle_deleted_lport(pb, b_ctx_in, b_ctx_out);
+ if (b_ctx_in->ovs_idl_txn) {
+ remove_ovn_installed(lbinding, pb->logical_port);
+ }
return true;
}
diff --git a/controller/binding.h b/controller/binding.h
index 6c3a98b02..72b4a35b6 100644
--- a/controller/binding.h
+++ b/controller/binding.h
@@ -159,6 +159,12 @@ bool local_binding_is_up(struct shash *local_bindings,
const char *pb_name,
bool local_binding_is_down(struct shash *local_bindings, const char *pb_name,
const struct sbrec_chassis *);
+bool local_binding_is_ovn_installed(struct shash *local_bindings,
+ const char *pb_name);
+void local_binding_remove_ovn_installed(struct shash *local_bindings,
+ const char *pb_name,
+ bool ovs_readonly);
+
void local_binding_set_up(struct shash *local_bindings, const char *pb_name,
const struct sbrec_chassis *chassis_rec,
const char *ts_now_str, bool sb_readonly,
diff --git a/controller/if-status.c b/controller/if-status.c
index d1c14ac30..c008aa79e 100644
--- a/controller/if-status.c
+++ b/controller/if-status.c
@@ -54,32 +54,37 @@ VLOG_DEFINE_THIS_MODULE(if_status);
*/
enum if_state {
- OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not yet
- initiated. */
- OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to
- * SB (but update notification not confirmed, so the
- * update may be resent in any of the following states)
- * and for which flows are still being installed.
- */
- OIF_MARK_UP, /* Interface with flows successfully installed in OVS
- * but not yet marked "up" in the binding module (in
- * SB and OVS databases).
- */
- OIF_MARK_DOWN, /* Released interface but not yet marked "down" in the
- * binding module (in SB and/or OVS databases).
- */
- OIF_INSTALLED, /* Interface flows programmed in OVS and binding marked
- * "up" in the binding module.
- */
+ OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not yet
+ initiated. */
+ OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent to
+ * SB (but update notification not confirmed, so the
+ * update may be resent in any of the following
+ * states and for which flows are still being
+ * installed.
+ */
+ OIF_REMOVE_OVN_INST, /* Interface with flows successfully installed in OVS,
+ * but with ovn-installed still in OVSDB.
+ */
+ OIF_MARK_UP, /* Interface with flows successfully installed in OVS
+ * but not yet marked "up" in the binding module (in
+ * SB and OVS databases).
+ */
+ OIF_MARK_DOWN, /* Released interface but not yet marked "down" in the
+ * binding module (in SB and/or OVS databases).
+ */
+ OIF_INSTALLED, /* Interface flows programmed in OVS and binding
+ * marked "up" in the binding module.
+ */
OIF_MAX,
};
static const char *if_state_names[] = {
- [OIF_CLAIMED] = "CLAIMED",
- [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
- [OIF_MARK_UP] = "MARK_UP",
- [OIF_MARK_DOWN] = "MARK_DOWN",
- [OIF_INSTALLED] = "INSTALLED",
+ [OIF_CLAIMED] = "CLAIMED",
+ [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
+ [OIF_REMOVE_OVN_INST] = "REMOVE_OVN_INST",
+ [OIF_MARK_UP] = "MARK_UP",
+ [OIF_MARK_DOWN] = "MARK_DOWN",
+ [OIF_INSTALLED] = "INSTALLED",
};
/*
@@ -109,11 +114,26 @@ static const char *if_state_names[] = {
* | | | - remove ovn-installed from ovsdb | | |
* | | | mgr_update() | | |
* | +----------------------+ - sbrec_update_chassis if needed | | |
- * | | | | |
- * | | mgr_run(seqno rcvd) | | |
- * | | - set port up in sb | | |
- * | release_iface | - set ovn-installed in ovs | | |
- * | V | | |
+ * | | | | | |
+ * | | +----------------------------------------+ | | |
+ * | | | | | |
+ * | | mgr_run(seqno rcvd, ovn-installed present) | | | |
+ * | V | | | |
+ * | +--------------------+ | | | |
+ * | | | mgr_run() | | | |
+ * +--- | REMOVE_OVN_INST | - remove ovn-installed in ovs | | | |
+ * | +--------------------+ | | | |
+ * | | | | | |
+ * | | | | | |
+ * | | mgr_update( ovn_installed not present) | | | |
+ * | | | | | |
+ * | | +-------------------------------------------+ | | |
+ * | | | | | |
+ * | | | mgr_run(seqno rcvd, ovn-installed not present) | | |
+ * | | | - set port up in sb | | |
+ * | | | - set ovn-installed in ovs | | |
+ * |release_iface | | | | |
+ * | V V | | |
* | +----------------------+ | | |
* | | | mgr_run() | | |
* +-- | MARK_UP | - set port up in sb | | |
@@ -238,6 +258,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
switch (iface->state) {
case OIF_CLAIMED:
case OIF_INSTALL_FLOWS:
+ case OIF_REMOVE_OVN_INST:
case OIF_MARK_UP:
/* Nothing to do here. */
break;
@@ -274,6 +295,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
const char *iface_id)
/* Not yet fully installed interfaces can be safely deleted. */
ovs_iface_destroy(mgr, iface);
break;
+ case OIF_REMOVE_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
/* Properly mark interfaces "down" if their flows were already
@@ -305,6 +327,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr, const
char *iface_id)
/* Not yet fully installed interfaces can be safely deleted. */
ovs_iface_destroy(mgr, iface);
break;
+ case OIF_REMOVE_OVN_INST:
case OIF_MARK_UP:
case OIF_INSTALLED:
/* Properly mark interfaces "down" if their flows were already
@@ -359,6 +382,17 @@ if_status_mgr_update(struct if_status_mgr *mgr,
struct shash *bindings = &binding_data->bindings;
struct hmapx_node *node;
+ /* Move all interfaces that have been confirmed without ovn-installed,
+ * from OIF_REMOVE_OVN_INST to OIF_MARK_UP.
+ */
+ HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) {
+ struct ovs_iface *iface = node->data;
+
+ if (!local_binding_is_ovn_installed(bindings, iface->id)) {
+ ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
+ }
+ }
+
/* Interfaces in OIF_MARK_UP/INSTALL_FLOWS state have already set their
* pb->chassis. However, the update might still be in fly (confirmation
* not received yet) or pb->chassis was overwitten by another chassis.
@@ -471,7 +505,19 @@ if_status_mgr_run(struct if_status_mgr *mgr,
iface->install_seqno)) {
continue;
}
- ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
+ /* Wait for ovn-installed to be absent before moving to MARK_UP state.
+ * Most of the times ovn-installed is already absent and hence we will
+ * not have to wait.
+ * If there is no binding_data, we can't determine if ovn-installed is
+ * present or not; hence also go to the OIF_REMOVE_OVN_INST state.
+ */
+ if (!binding_data ||
+ local_binding_is_ovn_installed(&binding_data->bindings,
+ iface->id)) {
+ ovs_iface_set_state(mgr, iface, OIF_REMOVE_OVN_INST);
+ } else {
+ ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
+ }
}
ofctrl_acked_seqnos_destroy(acked_seqnos);
@@ -558,7 +604,16 @@ if_status_mgr_update_bindings(struct if_status_mgr *mgr,
sb_readonly, ovs_readonly);
}
- /* Notifiy the binding module to set "up" all bindings that have had
+ /* Notify the binding module to remove "ovn-installed" for all bindings
+ * in the OIF_REMOVE_OVN_INST state.
+ */
+ HMAPX_FOR_EACH (node, &mgr->ifaces_per_state[OIF_REMOVE_OVN_INST]) {
+ struct ovs_iface *iface = node->data;
+
+ local_binding_remove_ovn_installed(bindings, iface->id, ovs_readonly);
+ }
+
+ /* Notify the binding module to set "up" all bindings that have had
* their flows installed but are not yet marked "up" in the binding
* module.
*/
diff --git a/tests/ovn.at b/tests/ovn.at
index d2163d87d..a0378a728 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -34248,3 +34248,260 @@ AT_CHECK([ovs-ofctl dump-flows br-int | grep table=68
| ofctl_strip_all], [0], [
OVN_CLEANUP([hv1])
AT_CLEANUP
])
+
+# This tests port->up/down and ovn-installed after adding and removing Ports
and Interfaces.
+# 3 Conditions x 3 tests:
+# - 3 Conditions:
+# - In normal conditions
+# - Remove interface while starting and stopping SB and Controller
+# - Remove and add back interface while starting and stopping SB and
Controller
+# - 3 tests:
+# - Add/Remove Logical Port
+# - Add/Remove iface-id
+# - Add/Remove Interface
+# Each tests/conditions checks for
+# - Port_binding->chassis
+# - Port up or down
+# - ovn-installed
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-installed and ports up or down])
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+ovn_attach n1 br-phys 192.168.0.1 24 geneve
+
+check ovn-nbctl ls-add ls1
+check ovn-nbctl set Logical_Switch ls1 other_config:subnet=10.1.0.0/16
+
+check ovn-nbctl --wait=hv sync
+
+add_logical_ports() {
+ echo Adding logical ports
+ check ovn-nbctl lsp-add ls1 lsp1
+ check ovn-nbctl lsp-add ls1 lsp2
+}
+
+remove_logical_ports() {
+ echo Removing logical ports
+ check ovn-nbctl lsp-del lsp1
+ check ovn-nbctl lsp-del lsp2
+}
+
+add_ovs_interfaces() {
+ echo Adding interfaces
+ ovs-vsctl --no-wait -- add-port br-int vif1 \
+ -- set Interface vif1 external_ids:iface-id=lsp1
+ ovs-vsctl --no-wait -- add-port br-int vif2 \
+ -- set Interface vif2 external_ids:iface-id=lsp2
+}
+remove_ovs_interfaces() {
+ echo Removing interfaces
+ check ovs-vsctl --no-wait -- del-port vif1
+ check ovs-vsctl --no-wait -- del-port vif2
+}
+add_iface_ids() {
+ echo Adding back iface-id
+ ovs-vsctl --no-wait -- set Interface vif1 external_ids:iface-id=lsp1
+ ovs-vsctl --no-wait -- set Interface vif2 external_ids:iface-id=lsp2
+}
+remove_iface_ids() {
+ echo Removing iface-id
+ check ovs-vsctl remove Interface vif1 external_ids iface-id
+ check ovs-vsctl remove Interface vif2 external_ids iface-id
+}
+wait_for_local_bindings() {
+ OVS_WAIT_UNTIL(
+ [test `ovs-appctl -t ovn-controller debug/dump-local-bindings | grep
interface | wc -l` -eq 2],
+ [kill -CONT $(cat ovn-sb/ovsdb-server.pid)]
+ )
+}
+sleep_sb() {
+ echo SB going to sleep
+ AT_CHECK([kill -STOP $(cat ovn-sb/ovsdb-server.pid)])
+}
+wake_up_sb() {
+ echo SB waking up
+ AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)])
+}
+sleep_controller() {
+ echo Controller going to sleep
+ ovn-appctl debug/pause
+ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) =
"xpaused"])
+}
+wake_up_controller() {
+ echo Controller waking up
+ ovn-appctl debug/resume
+}
+ensure_controller_run() {
+# We want to make sure controller could run at least one full loop.
+# We can't use wait=hv as sb might be sleeping.
+# Use 2 ovn-appctl to guarentee that ovn-controller run the full loop, and not
just the unixctl handling
+ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) =
"xrunning"])
+ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) =
"xrunning"])
+}
+sleep_ovsdb() {
+ echo OVSDB going to sleep
+ AT_CHECK([kill -STOP $(cat hv1/ovsdb-server.pid)])
+}
+wake_up_ovsdb() {
+ echo OVSDB waking up
+ AT_CHECK([kill -CONT $(cat hv1/ovsdb-server.pid)])
+}
+check_ovn_installed() {
+ OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface vif1
external_ids:ovn-installed` = '"true"'])
+ OVS_WAIT_UNTIL([test `as hv1 ovs-vsctl get Interface vif2
external_ids:ovn-installed` = '"true"'])
+}
+check_ovn_uninstalled() {
+ OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface vif1
external_ids:ovn-installed` = x])
+ OVS_WAIT_UNTIL([test x`as hv1 ovs-vsctl get Interface vif2
external_ids:ovn-installed` = x])
+}
+check_ports_up() {
+ OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'true'])
+ OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'true'])
+}
+check_ports_down() {
+ OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp1 up` = 'false'])
+ OVS_WAIT_UNTIL([test `ovn-sbctl get Port_Binding lsp2 up` = 'false'])
+}
+
+check_ports_bound() {
+ ch=$(fetch_column Chassis _uuid name=hv1)
+ wait_row_count Port_Binding 1 logical_port=lsp1 chassis=$ch
+ wait_row_count Port_Binding 1 logical_port=lsp2 chassis=$ch
+}
+check_ports_unbound() {
+ wait_column "" Port_Binding chassis logical_port=lsp1
+ wait_column "" Port_Binding chassis logical_port=lsp2
+}
+add_logical_ports
+add_ovs_interfaces
+wait_for_local_bindings
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+############################################################
+################### Add/Remove iface-id ####################
+############################################################
+AS_BOX(["iface-id removal and added back (no sleeping sb or controller)"])
+remove_iface_ids
+check_ovn_uninstalled
+check_ports_down
+check_ports_unbound
+add_iface_ids
+check_ovn_installed
+check_ports_up
+check_ports_bound
+
+AS_BOX(["iface-id removal"])
+sleep_sb
+remove_iface_ids
+ensure_controller_run
+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
+add_iface_ids
+check ovn-nbctl --wait=hv sync
+
+AS_BOX(["iface-id removal and added back"])
+sleep_sb
+remove_iface_ids
+ensure_controller_run
+sleep_controller
+add_iface_ids
+wake_up_sb
+wake_up_controller
+check_ovn_installed
+check_ports_up
+check_ports_bound
+############################################################
+###################### Add/Remove Interface ################
+############################################################
+AS_BOX(["Interface removal and added back (no sleeping sb or controller)"])
+remove_ovs_interfaces
+check_ovn_uninstalled
+check_ports_down
+check_ports_unbound
+add_ovs_interfaces
+check_ovn_installed
+check_ports_up
+check_ports_bound
+check ovn-nbctl --wait=hv sync
+
+AS_BOX(["Interface removal"])
+sleep_sb
+remove_ovs_interfaces
+ensure_controller_run
+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
+add_ovs_interfaces
+check ovn-nbctl --wait=hv sync
+
+AS_BOX(["Interface removal and added back"])
+sleep_sb
+remove_ovs_interfaces
+ensure_controller_run
+sleep_controller
+add_ovs_interfaces
+wake_up_sb
+wake_up_controller
+check_ovn_installed
+check_ports_up
+check_ports_bound
+check ovn-nbctl --wait=hv sync
+############################################################
+###################### Add/Remove Logical Port #############
+############################################################
+AS_BOX(["Logical port removal and added back (no sleeping sb or controller)"])
+remove_logical_ports
+check_ovn_uninstalled
+check_ports_unbound
+sleep_ovsdb
+add_logical_ports
+ensure_controller_run
+wake_up_ovsdb
+check_ovn_installed
+check_ports_up
+check_ports_bound
+check ovn-nbctl --wait=hv sync
+
+AS_BOX(["Logical port removal"])
+sleep_sb
+remove_logical_ports
+ensure_controller_run
+sleep_controller
+wake_up_sb
+wake_up_controller
+check_ovn_uninstalled
+check_ports_unbound
+add_logical_ports
+check ovn-nbctl --wait=hv sync
+
+AS_BOX(["Logical port removal and added back"])
+sleep_sb
+remove_logical_ports
+ensure_controller_run
+sleep_controller
+add_logical_ports
+wake_up_sb
+wake_up_controller
+check_ovn_installed
+check_ports_up
+check_ports_bound
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
--
2.31.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
