This change makes it so that if a chassis has no encaps configured, the
entire Chassis record is deleted.
If a chassis hv1 has encap A, and then encap A is removed from hv1,
chassis hv2 should be able to use encap A. But this is not possible if
the Encap record is still present in the sb db.
The reasoning for deleting the entire Chassis record rather than just
its Encap is that a chassis is already considered misconfigured if it
has no encaps set. We could allow encap-less chassis, but we already
have the behavior established of deleting chassis with no valid encaps
(if there are duplicates) in chassis_update(), and allowing 0 encaps
would also require a schema change and checking for n_encaps>0 in many
places.
So the easiest solution is just to remove the chassis record altogether.
When encaps are readded, the chassis record will be recreated.
Fixes: c2dd8593c8e8 ("controller: Prevent other chassis with same encap IP.")
Assisted-by: Claude Opus 5, Claude Code
Reported-at: https://redhat.atlassian.net/browse/FDP-1945
Signed-off-by: Rosemarie O'Riorden <[email protected]>
---
v2:
- Make chassis_parse_ovs_config() return a bool rather than an enum.
Move "if (!cfg)" check outside of the function.
- Make sure *chassis_private is NULL if the record is deleted. Added Fixes
tag because this was a previously introduced bug.
- Refactor chassis_run() so records are inserted at the beginning. And bail
immediately if !ovnsb_idl_txn.
- Delete unused chassis_get_record() and chassis_private_get_record().
---
controller/chassis.c | 167 +++++++++++++++++-----------------------
tests/ovn-controller.at | 52 +++++++++++++
2 files changed, 122 insertions(+), 97 deletions(-)
diff --git a/controller/chassis.c b/controller/chassis.c
index b82683064..c129a829a 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -303,21 +303,17 @@ chassis_parse_ovs_iface_types(char **iface_types, size_t
n_iface_types,
}
/*
- * Parse the 'ovs_table' entry and populate 'ovs_cfg'.
+ * Parse the 'ovs_table' entry and populate 'ovs_cfg'. Returns true on
+ * success. Returns false if 'cfg' has no encap type and/or ip configured,
+ * in which case the caller should delete the chassis.
*/
static bool
chassis_parse_ovs_config(const struct ovsrec_open_vswitch_table *ovs_table,
+ const struct ovsrec_open_vswitch *cfg,
const struct ovsrec_bridge *br_int,
struct ovs_chassis_cfg *ovs_cfg)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
- const struct ovsrec_open_vswitch *cfg =
- ovsrec_open_vswitch_table_first(ovs_table);
-
- if (!cfg) {
- VLOG_INFO("No Open_vSwitch row defined.");
- return false;
- }
const char *chassis_id = get_ovs_chassis_id(ovs_table);
const char *encap_type =
@@ -328,7 +324,8 @@ chassis_parse_ovs_config(const struct
ovsrec_open_vswitch_table *ovs_table,
get_chassis_external_id_value(&cfg->external_ids, chassis_id,
"ovn-encap-ip", NULL);
if (!encap_type || !encap_ips) {
- VLOG_INFO_RL(&rl, "Need to specify an encap type and ip");
+ VLOG_WARN_RL(&rl, "No encap type and/or ip on chassis '%s'. "
+ "Deleting chassis.", chassis_id);
return false;
}
@@ -801,36 +798,6 @@ remove_unsupported_options(const struct sbrec_chassis
*chassis_rec,
sset_destroy(&supported_options);
}
-/* If this is a chassis config update after we initialized the record once
- * then we should always be able to find it with the ID we saved in
- * chassis_state.
- * Otherwise (i.e., first time we create the record or if the system-id
- * changed) we create a new record.
- *
- * Sets '*chassis_rec' to point to the local chassis record.
- * Returns true if this record was already in the database, false if it was
- * just inserted.
- */
-static bool
-chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn,
- struct ovsdb_idl_index *sbrec_chassis_by_name,
- const char *chassis_id,
- const struct sbrec_chassis **chassis_rec)
-{
- const struct sbrec_chassis *chassis =
- chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
-
- if (!chassis && ovnsb_idl_txn) {
- /* Create the chassis record. */
- VLOG_DBG("Could not find Chassis, will create it: %s", chassis_id);
- *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
- return false;
- }
-
- *chassis_rec = chassis;
- return true;
-}
-
/* Update a Chassis record based on the config in the ovs config.
* Returns CHASSIS_UPDATED if 'chassis_rec' was updated, CHASSIS_NEED_DELETE if
* another chassis exists with an encap with same IP and type as 'chassis', and
@@ -901,31 +868,6 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
return CHASSIS_UPDATED;
}
-/* If this is a chassis_private config update after we initialized the record
- * once then we should always be able to find it with the ID we saved in
- * chassis_state.
- * Otherwise (i.e., first time we created the chassis record or if the
- * system-id changed) we create a new record.
- *
- * Returns the local chassis record.
- */
-static const struct sbrec_chassis_private *
-chassis_private_get_record(
- struct ovsdb_idl_txn *ovnsb_idl_txn,
- struct ovsdb_idl_index *sbrec_chassis_pvt_by_name,
- const char *chassis_id)
-{
- const struct sbrec_chassis_private *chassis_p =
- chassis_private_lookup_by_name(sbrec_chassis_pvt_by_name,
- chassis_id);
-
- if (!chassis_p) {
- return sbrec_chassis_private_insert(ovnsb_idl_txn);
- }
-
- return chassis_p;
-}
-
static void
chassis_private_update(const struct sbrec_chassis_private *chassis_pvt,
const struct sbrec_chassis *chassis,
@@ -956,52 +898,83 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
*chassis_private = NULL;
+ if (!ovnsb_idl_txn) {
+ return NULL;
+ }
+
/* Get the chassis config from the ovs table. */
- ovs_chassis_cfg_init(&ovs_cfg);
- if (!chassis_parse_ovs_config(ovs_table, br_int, &ovs_cfg)) {
+ const struct ovsrec_open_vswitch *cfg =
+ ovsrec_open_vswitch_table_first(ovs_table);
+
+ if (!cfg) {
+ VLOG_INFO("No Open_vSwitch row defined.");
return NULL;
}
- const struct sbrec_chassis *chassis_rec = NULL;
- bool existed = chassis_get_record(ovnsb_idl_txn, sbrec_chassis_by_name,
- chassis_id, &chassis_rec);
+ ovs_chassis_cfg_init(&ovs_cfg);
+ bool parse_status = chassis_parse_ovs_config(ovs_table, cfg,
+ br_int, &ovs_cfg);
+
+ /* Look up any existing records for this chassis. */
+ const struct sbrec_chassis *chassis_rec =
+ chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
+ const struct sbrec_chassis_private *chassis_pvt_rec =
+ chassis_private_lookup_by_name(sbrec_chassis_private_by_name,
+ chassis_id);
+ bool existed = chassis_rec != NULL;
+
+ /* If no encaps are configured, delete the Chassis record. */
+ if (!parse_status) {
+ if (chassis_rec) {
+ sbrec_chassis_delete(chassis_rec);
+ }
+ if (chassis_pvt_rec) {
+ sbrec_chassis_private_delete(chassis_pvt_rec);
+ }
+ goto exit;
+ }
+
+ /* Insert a new record if needed. */
+ if (!chassis_rec) {
+ chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
+ }
+ if (!chassis_rec) {
+ return NULL;
+ }
+ if (!chassis_pvt_rec) {
+ chassis_pvt_rec = sbrec_chassis_private_insert(ovnsb_idl_txn);
+ }
/* If we found (or created) a record, update it with the correct config
* and store the current chassis_id for fast lookup in case it gets
* modified in the ovs table.
*/
- if (chassis_rec && ovnsb_idl_txn) {
- enum chassis_update_status update_status =
- chassis_update(chassis_rec, ovnsb_idl_txn, &ovs_cfg,
- chassis_id,
- transport_zones, sbrec_encaps_index_by_ip_and_type);
-
- *chassis_private = chassis_private_get_record(ovnsb_idl_txn,
- sbrec_chassis_private_by_name, chassis_id);
-
- if (update_status == CHASSIS_NEED_DELETE) {
- sbrec_chassis_delete(chassis_rec);
- if (*chassis_private) {
- sbrec_chassis_private_delete(*chassis_private);
- }
-
- ovs_chassis_cfg_destroy(&ovs_cfg);
- return NULL;
- }
+ enum chassis_update_status update_status =
+ chassis_update(chassis_rec, ovnsb_idl_txn, &ovs_cfg, chassis_id,
+ transport_zones, sbrec_encaps_index_by_ip_and_type);
+
+ /* Delete the chassis if another chassis already has the same encap. */
+ if (update_status == CHASSIS_NEED_DELETE) {
+ sbrec_chassis_delete(chassis_rec);
+ sbrec_chassis_private_delete(chassis_pvt_rec);
+ goto exit;
+ }
- if (!existed || update_status == CHASSIS_UPDATED) {
- ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
- "ovn-controller: %s chassis '%s'",
- !existed ? "registering" : "updating",
- chassis_id);
- }
- if (*chassis_private) {
- chassis_private_update(*chassis_private, chassis_rec, chassis_id);
- }
+ if (!existed || update_status == CHASSIS_UPDATED) {
+ ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
+ "ovn-controller: %s chassis '%s'",
+ !existed ? "registering" : "updating",
+ chassis_id);
}
+ chassis_private_update(chassis_pvt_rec, chassis_rec, chassis_id);
+ *chassis_private = chassis_pvt_rec;
+
+ ovs_chassis_cfg_destroy(&ovs_cfg);
+ return chassis_rec;
+exit:
ovs_chassis_cfg_destroy(&ovs_cfg);
- return existed ? chassis_rec : NULL;
+ return NULL;
}
bool
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index d600ba2cd..56010fe9b 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -4520,3 +4520,55 @@ done
OVN_CLEANUP([gw1],[gw3],[hv2])
AT_CLEANUP
])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([ovn-controller - remove encap from chassis add to other chassis])
+AT_KEYWORDS([ovn])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1 check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.11
+
+sim_add hv2
+as hv2 check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.12
+
+as hv2
+OVS_WAIT_FOR_OUTPUT([ovs-vsctl find Interface type=geneve | grep -c _uuid],
[0], [1
+])
+
+as hv1
+OVS_WAIT_FOR_OUTPUT([ovs-vsctl find Interface type=geneve | grep -c _uuid],
[0], [1
+])
+wait_row_count Encap 4
+wait_row_count Chassis 2
+
+# Remove hv1's encap.
+as hv1 check ovs-vsctl remove Open_vSwitch . external-ids
ovn-encap-ip="192.168.0.11"
+check ovn-nbctl --wait=sb sync
+
+# Make sure that hv1's chassis rec was deleted from sb. And of course with
+# that its encaps too.
+OVS_WAIT_UNTIL([test $(ovn-sbctl --bare find Encap chassis_name=hv1
ip="192.168.0.11" | wc -l) -eq 0])
+wait_row_count Encap 0 chassis_name="hv1" ip="192.168.0.11"
+wait_row_count Chassis 0 name="hv1"
+OVS_WAIT_UNTIL([grep -q "No encap type and/or ip on chassis 'hv1'. Deleting
chassis." hv1/ovn-controller.log
+])
+
+# Assign hv1's old encap ip to hv2. Verify it worked.
+as hv2 check ovs-vsctl set Open_vSwitch .
external-ids:ovn-encap-ip="192.168.0.11"
+wait_row_count Encap 0 chassis_name="hv2" ip="192.168.0.12"
+wait_row_count Encap 2 chassis_name="hv2" ip="192.168.0.11"
+
+# Now give hv1 a new different encap ip. Verify the chassis is back.
+as hv1 check ovs-vsctl set Open_vSwitch .
external-ids:ovn-encap-ip="192.168.0.13"
+check ovn-nbctl --wait=hv sync
+wait_row_count Encap 2 chassis_name="hv1" ip="192.168.0.13"
+wait_row_count Chassis 1 name="hv1"
+
+OVN_CLEANUP([hv2], [hv1
+/No encap type and\/or ip on chassis 'hv1'. Deleting chassis/d])
+AT_CLEANUP
+])
--
2.56.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
