[ovs-dev] [PATCH ovn] chassis: Do not try to guess system-id changes.
Numan Siddique
numans at ovn.org
Fri Dec 11 11:53:13 UTC 2020
On Thu, Dec 10, 2020 at 3:28 AM Mark Michelson <mmichels at redhat.com> wrote:
>
> Acked-by: Mark Michelson <mmichels at redhat.com>
Thanks Dumitru and Mark.
I applied this patch to the main branch and backported to branch-20.12.
Can you please submit backport patches for the remaining branches.
It doesn't apply cleanly.
Thanks
Numan
>
> On 12/8/20 2:21 PM, Dumitru Ceara wrote:
> > When the OVS system-id changes ovn-controller needs external (CMS) help
> > in order to update its own Chassis/Chassis_private records, i.e., the
> > CMS has to ensure that either ovn-controller is stopped (so that
> > ovn-controller cleans up its old Chassis/Chassis_private records) or
> > that after the system-id is changed, the stale Chassis/Chassis_private
> > records are destroyed externally.
> >
> > This patch reverts the previous efforts to have ovn-controller reuse
> > stale Chassis records and documents how the system-id change operation
> > needs to be executed. The main problem with reusing stale records is
> > that there's no safe way to make it work when RBAC is enabled.
> >
> > Suggestedy-by: Han Zhou <hzhou at ovn.org>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374608.html
> > Fixes: f26c4a530bca ("ovn-controller: Fix chassis ovn-sbdb record init")
> > Fixes: 4465f553ee70 ("ovn-controller: Update stale chassis entry at init")
> > Fixes: 94a32fca2d2b ("chassis: Fix the way encaps are updated for a chassis record.")
> > Fixes: dce1af31b550 ("chassis: Fix chassis_private record updates when the system-id changes.")
> > Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > ---
> > controller/chassis.c | 177 ++++------------------------------------
> > controller/chassis.h | 3 -
> > controller/ovn-controller.8.xml | 7 +-
> > controller/ovn-controller.c | 21 +++--
> > northd/ovn-northd.c | 6 +-
> > tests/ovn-controller.at | 44 +++-------
> > 6 files changed, 47 insertions(+), 211 deletions(-)
> >
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 7748fb9..b4d4b0e 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -37,44 +37,6 @@ VLOG_DEFINE_THIS_MODULE(chassis);
> > #endif /* HOST_NAME_MAX */
> >
> > /*
> > - * Structure to hold chassis specific state (currently just chassis-id)
> > - * to avoid database lookups when changes happen while the controller is
> > - * running.
> > - */
> > -struct chassis_info {
> > - /* Last ID we initialized the Chassis SB record with. */
> > - struct ds id;
> > -
> > - /* True if Chassis SB record is initialized, false otherwise. */
> > - uint32_t id_inited : 1;
> > -};
> > -
> > -static struct chassis_info chassis_state = {
> > - .id = DS_EMPTY_INITIALIZER,
> > - .id_inited = false,
> > -};
> > -
> > -static void
> > -chassis_info_set_id(struct chassis_info *info, const char *id)
> > -{
> > - ds_clear(&info->id);
> > - ds_put_cstr(&info->id, id);
> > - info->id_inited = true;
> > -}
> > -
> > -static bool
> > -chassis_info_id_inited(const struct chassis_info *info)
> > -{
> > - return info->id_inited;
> > -}
> > -
> > -static const char *
> > -chassis_info_id(const struct chassis_info *info)
> > -{
> > - return ds_cstr_ro(&info->id);
> > -}
> > -
> > -/*
> > * Structure for storing the chassis config parsed from the ovs table.
> > */
> > struct ovs_chassis_cfg {
> > @@ -420,6 +382,9 @@ chassis_tunnels_changed(const struct sset *encap_type_set,
> > bool changed = false;
> >
> > for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
> > + if (strcmp(chassis_rec->name, chassis_rec->encaps[i]->chassis_name)) {
> > + return true;
> > + }
> >
> > if (!sset_contains(encap_type_set, chassis_rec->encaps[i]->type)) {
> > changed = true;
> > @@ -500,54 +465,11 @@ chassis_build_encaps(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > return encaps;
> > }
> >
> > -/*
> > - * Updates encaps for a given chassis. This can happen when the chassis
> > - * name has changed. Also, the only thing we support updating is the
> > - * chassis_name. For other changes the encaps will be recreated.
> > - */
> > -static void
> > -chassis_update_encaps(const struct sbrec_chassis *chassis)
> > -{
> > - for (size_t i = 0; i < chassis->n_encaps; i++) {
> > - sbrec_encap_set_chassis_name(chassis->encaps[i], chassis->name);
> > - }
> > -}
> > -
> > -/*
> > - * Returns a pointer to a chassis record from 'chassis_table' that
> > - * matches at least one tunnel config.
> > - */
> > -static const struct sbrec_chassis *
> > -chassis_get_stale_record(const struct sbrec_chassis_table *chassis_table,
> > - const struct ovs_chassis_cfg *ovs_cfg,
> > - const char *chassis_id)
> > -{
> > - const struct sbrec_chassis *chassis_rec;
> > -
> > - SBREC_CHASSIS_TABLE_FOR_EACH (chassis_rec, chassis_table) {
> > - for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
> > - if (sset_contains(&ovs_cfg->encap_type_set,
> > - chassis_rec->encaps[i]->type) &&
> > - sset_contains(&ovs_cfg->encap_ip_set,
> > - chassis_rec->encaps[i]->ip)) {
> > - return chassis_rec;
> > - }
> > - if (strcmp(chassis_rec->name, chassis_id) == 0) {
> > - return chassis_rec;
> > - }
> > - }
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > /* 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) then we check if there's a stale record from a previous
> > - * controller run that didn't end gracefully and reuse it. If not then we
> > - * create a new record.
> > + * 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
> > @@ -556,33 +478,16 @@ chassis_get_stale_record(const struct sbrec_chassis_table *chassis_table,
> > static bool
> > chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct ovsdb_idl_index *sbrec_chassis_by_name,
> > - const struct sbrec_chassis_table *chassis_table,
> > - const struct ovs_chassis_cfg *ovs_cfg,
> > const char *chassis_id,
> > const struct sbrec_chassis **chassis_rec)
> > {
> > - const struct sbrec_chassis *chassis = NULL;
> > -
> > - if (chassis_info_id_inited(&chassis_state)) {
> > - chassis = chassis_lookup_by_name(sbrec_chassis_by_name,
> > - chassis_info_id(&chassis_state));
> > - if (!chassis) {
> > - VLOG_DBG("Could not find Chassis, will check if the id changed: "
> > - "stored (%s) ovs (%s)",
> > - chassis_info_id(&chassis_state), chassis_id);
> > - }
> > - }
> > -
> > - if (!chassis) {
> > - chassis = chassis_get_stale_record(chassis_table, ovs_cfg, chassis_id);
> > - }
> > + const struct sbrec_chassis *chassis =
> > + chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
> >
> > - if (!chassis) {
> > - /* Recreate the chassis record. */
> > + if (!chassis && ovnsb_idl_txn) {
> > + /* Create the chassis record. */
> > VLOG_DBG("Could not find Chassis, will create it: %s", chassis_id);
> > - if (ovnsb_idl_txn) {
> > - *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> > - }
> > + *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> > return false;
> > }
> >
> > @@ -650,7 +555,6 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
> > &ovs_cfg->encap_ip_set, ovs_cfg->encap_csum,
> > chassis_rec);
> > if (!tunnels_changed) {
> > - chassis_update_encaps(chassis_rec);
> > return updated;
> > }
> >
> > @@ -666,33 +570,11 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
> > return true;
> > }
> >
> > -/*
> > - * Returns a pointer to a chassis_private record from 'chassis_pvt_table' that
> > - * matches the chassis record.
> > - */
> > -static const struct sbrec_chassis_private *
> > -chassis_private_get_stale_record(
> > - const struct sbrec_chassis_private_table *chassis_pvt_table,
> > - const struct sbrec_chassis *chassis)
> > -{
> > - const struct sbrec_chassis_private *chassis_pvt_rec;
> > -
> > - SBREC_CHASSIS_PRIVATE_TABLE_FOR_EACH (chassis_pvt_rec, chassis_pvt_table) {
> > - if (chassis_pvt_rec->chassis == chassis) {
> > - return chassis_pvt_rec;
> > - }
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > /* 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) then we check if there's a stale record from a previous
> > - * controller run that didn't end gracefully and reuse it. If not then we
> > - * create a new record.
> > + * system-id changed) we create a new record.
> > *
> > * Returns the local chassis record.
> > */
> > @@ -700,21 +582,11 @@ 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 struct sbrec_chassis_private_table *chassis_pvt_table,
> > - const struct sbrec_chassis *chassis)
> > + const char *chassis_id)
> > {
> > - const struct sbrec_chassis_private *chassis_p = NULL;
> > -
> > - if (chassis_info_id_inited(&chassis_state)) {
> > - chassis_p =
> > + const struct sbrec_chassis_private *chassis_p =
> > chassis_private_lookup_by_name(sbrec_chassis_pvt_by_name,
> > - chassis_info_id(&chassis_state));
> > - }
> > -
> > - if (!chassis_p) {
> > - chassis_p = chassis_private_get_stale_record(chassis_pvt_table,
> > - chassis);
> > - }
> > + chassis_id);
> >
> > if (!chassis_p && ovnsb_idl_txn) {
> > return sbrec_chassis_private_insert(ovnsb_idl_txn);
> > @@ -743,8 +615,6 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct ovsdb_idl_index *sbrec_chassis_by_name,
> > struct ovsdb_idl_index *sbrec_chassis_private_by_name,
> > const struct ovsrec_open_vswitch_table *ovs_table,
> > - const struct sbrec_chassis_table *chassis_table,
> > - const struct sbrec_chassis_private_table *chassis_pvt_table,
> > const char *chassis_id,
> > const struct ovsrec_bridge *br_int,
> > const struct sset *transport_zones,
> > @@ -762,8 +632,7 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >
> > const struct sbrec_chassis *chassis_rec = NULL;
> > bool existed = chassis_get_record(ovnsb_idl_txn, sbrec_chassis_by_name,
> > - chassis_table, &ovs_cfg, chassis_id,
> > - &chassis_rec);
> > + chassis_id, &chassis_rec);
> >
> > /* 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
> > @@ -783,13 +652,10 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > *chassis_private =
> > chassis_private_get_record(ovnsb_idl_txn,
> > sbrec_chassis_private_by_name,
> > - chassis_pvt_table, chassis_rec);
> > -
> > + chassis_id);
> > if (*chassis_private) {
> > chassis_private_update(*chassis_private, chassis_rec, chassis_id);
> > }
> > -
> > - chassis_info_set_id(&chassis_state, chassis_id);
> > }
> >
> > ovs_chassis_cfg_destroy(&ovs_cfg);
> > @@ -865,16 +731,3 @@ chassis_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > }
> > return false;
> > }
> > -
> > -/*
> > - * Returns the last initialized chassis-id.
> > - */
> > -const char *
> > -chassis_get_id(void)
> > -{
> > - if (chassis_info_id_inited(&chassis_state)) {
> > - return chassis_info_id(&chassis_state);
> > - }
> > -
> > - return NULL;
> > -}
> > diff --git a/controller/chassis.h b/controller/chassis.h
> > index 220f726..18b45a1 100644
> > --- a/controller/chassis.h
> > +++ b/controller/chassis.h
> > @@ -37,8 +37,6 @@ const struct sbrec_chassis *chassis_run(
> > struct ovsdb_idl_index *sbrec_chassis_by_name,
> > struct ovsdb_idl_index *sbrec_chassis_private_by_name,
> > const struct ovsrec_open_vswitch_table *,
> > - const struct sbrec_chassis_table *,
> > - const struct sbrec_chassis_private_table *,
> > const char *chassis_id, const struct ovsrec_bridge *br_int,
> > const struct sset *transport_zones,
> > const struct sbrec_chassis_private **chassis_private);
> > @@ -48,7 +46,6 @@ bool chassis_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > bool chassis_get_mac(const struct sbrec_chassis *chassis,
> > const char *bridge_mapping,
> > struct eth_addr *chassis_mac);
> > -const char *chassis_get_id(void);
> > const char * get_chassis_mac_mappings(const struct smap *ext_ids);
> >
> >
> > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
> > index 6f3d388..29833c7 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -68,7 +68,12 @@
> > </p>
> > <dl>
> > <dt><code>external_ids:system-id</code></dt>
> > - <dd>The chassis name to use in the Chassis table.</dd>
> > + <dd>The chassis name to use in the Chassis table. Changing the
> > + <code>system-id</code> while <code>ovn-controller</code> is running is
> > + not directly supported. Users have two options: either first
> > + gracefully stop <code>ovn-controller</code> or manually delete the
> > + stale <code>Chassis</code> and <code>Chassis_Private</code> records
> > + after changing the <code>system-id</code>.</dd>
> >
> > <dt><code>external_ids:hostname</code></dt>
> > <dd>The hostname to use in the Chassis table.</dd>
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 90a0d9c..b5f0c13 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1777,7 +1777,7 @@ static void init_physical_ctx(struct engine_node *node,
> > (struct ovsrec_bridge_table *)EN_OVSDB_GET(
> > engine_get_input("OVS_bridge", node));
> > const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> > - const char *chassis_id = chassis_get_id();
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > const struct sbrec_chassis *chassis = NULL;
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > @@ -1867,7 +1867,11 @@ static void init_lflow_ctx(struct engine_node *node,
> > (struct sbrec_load_balancer_table *)EN_OVSDB_GET(
> > engine_get_input("SB_load_balancer", node));
> >
> > - const char *chassis_id = chassis_get_id();
> > + struct ovsrec_open_vswitch_table *ovs_table =
> > + (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
> > + engine_get_input("OVS_open_vswitch", node));
> > +
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > const struct sbrec_chassis *chassis = NULL;
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > @@ -1961,7 +1965,7 @@ en_flow_output_run(struct engine_node *node, void *data)
> > (struct ovsrec_bridge_table *)EN_OVSDB_GET(
> > engine_get_input("OVS_bridge", node));
> > const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> > - const char *chassis_id = chassis_get_id();
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> >
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > @@ -2145,7 +2149,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data,
> > (struct ovsrec_bridge_table *)EN_OVSDB_GET(
> > engine_get_input("OVS_bridge", node));
> > const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
> > - const char *chassis_id = chassis_get_id();
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> >
> > struct ovsdb_idl_index *sbrec_chassis_by_name =
> > engine_ovsdb_node_get_index(
> > @@ -2770,18 +2774,13 @@ main(int argc, char *argv[])
> > get_transport_zones(ovsrec_open_vswitch_table_get(
> > ovs_idl_loop.idl)), ",");
> >
> > - const struct sbrec_chassis_table *chassis_table =
> > - sbrec_chassis_table_get(ovnsb_idl_loop.idl);
> > - const struct sbrec_chassis_private_table *chassis_pvt_table =
> > - sbrec_chassis_private_table_get(ovnsb_idl_loop.idl);
> > const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > const struct sbrec_chassis *chassis = NULL;
> > const struct sbrec_chassis_private *chassis_private = NULL;
> > if (chassis_id) {
> > chassis = chassis_run(ovnsb_idl_txn, sbrec_chassis_by_name,
> > sbrec_chassis_private_by_name,
> > - ovs_table, chassis_table,
> > - chassis_pvt_table, chassis_id,
> > + ovs_table, chassis_id,
> > br_int, &transport_zones,
> > &chassis_private);
> > }
> > @@ -3020,7 +3019,7 @@ loop_done:
> >
> > const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> > ovs_table);
> > - const char *chassis_id = chassis_get_id();
> > + const char *chassis_id = get_ovs_chassis_id(ovs_table);
> > const struct sbrec_chassis *chassis
> > = (chassis_id
> > ? chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id)
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 9572423..8dc41ab 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -12637,17 +12637,17 @@ static const char *rbac_chassis_auth[] =
> > {"name"};
> > static const char *rbac_chassis_update[] =
> > {"nb_cfg", "external_ids", "encaps", "vtep_logical_switches",
> > - "other_config", "name"};
> > + "other_config"};
> >
> > static const char *rbac_chassis_private_auth[] =
> > {"name"};
> > static const char *rbac_chassis_private_update[] =
> > - {"nb_cfg", "nb_cfg_timestamp", "chassis", "name"};
> > + {"nb_cfg", "nb_cfg_timestamp", "chassis"};
> >
> > static const char *rbac_encap_auth[] =
> > {"chassis_name"};
> > static const char *rbac_encap_update[] =
> > - {"type", "options", "ip", "chassis_name"};
> > + {"type", "options", "ip"};
> >
> > static const char *rbac_port_binding_auth[] =
> > {""};
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index a1f35fb..1b46799 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -187,45 +187,27 @@ OVS_WAIT_UNTIL([
> > test "${expected_iface_types}" = "${chassis_iface_types}"
> > ])
> >
> > -# Change the value of external_ids:system-id and make sure it's mirrored
> > -# in the Chassis record in the OVN_Southbound database.
> > +# Change the value of external_ids:system-id.
> > +# This requires operator intervention and removal of the stale chassis and
> > +# chassis_private records. Until that happens ovn-controller fails to
> > +# create the records due to constraint violation on the Encap table.
> > sysid=${sysid}-foo
> > ovs-vsctl set Open_vSwitch . external-ids:system-id="${sysid}"
> > +
> > OVS_WAIT_UNTIL([
> > - chassis_id=$(ovn-sbctl get Chassis "${sysid}" name)
> > - test "${sysid}" = "${chassis_id}"
> > -])
> > -OVS_WAIT_UNTIL([
> > - chassis_id=$(ovn-sbctl get Chassis_Private "${sysid}" name)
> > - test "${sysid}" = "${chassis_id}"
> > + grep -q 'Transaction causes multiple rows in \\"Encap\\" table to have identical values (geneve and \\"192.168.0.1\\") for index on columns \\"type\\" and \\"ip\\".' hv/ovn-controller.log
> > ])
> >
> > -# Only one Chassis_Private record should exist.
> > -wait_row_count Chassis_Private 1
> > +# Destroy the stale entries manually and ovn-controller should now be able
> > +# to create new ones.
> > +check ovn-sbctl destroy chassis_private . -- destroy chassis .
> >
> > -# Simulate system-id changing while ovn-controller is disconnected from the
> > -# SB.
> > -valid_remote=$(ovs-vsctl get Open_vSwitch . external_ids:ovn-remote)
> > -invalid_remote=tcp:0.0.0.0:4242
> > -ovs-vsctl set Open_vSwitch . external_ids:ovn-remote=${invalid_remote}
> > -expected_state="not connected"
> > -OVS_WAIT_UNTIL([
> > - test "${expected_state}" = "$(ovn-appctl -t ovn-controller connection-status)"
> > -])
> > -sysid=${sysid}-bar
> > -ovs-vsctl set Open_vSwitch . external-ids:system-id="${sysid}"
> > -ovs-vsctl set Open_vSwitch . external_ids:ovn-remote=${valid_remote}
> > -OVS_WAIT_UNTIL([
> > - chassis_id=$(ovn-sbctl get Chassis "${sysid}" name)
> > - test "${sysid}" = "${chassis_id}"
> > -])
> > -OVS_WAIT_UNTIL([
> > - chassis_id=$(ovn-sbctl get Chassis_Private "${sysid}" name)
> > - test "${sysid}" = "${chassis_id}"
> > -])
> > +wait_row_count Chassis_Private 1 name=${sysid}
> > +wait_row_count Chassis 1 name=${sysid}
> >
> > -# Only one Chassis_Private record should exist.
> > +# Only one Chassis_Private/Chassis record should exist.
> > wait_row_count Chassis_Private 1
> > +wait_row_count Chassis 1
> >
> > # Gracefully terminate daemons
> > OVN_CLEANUP_SBOX([hv])
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list