[ovs-dev] [PATCH v2 ovn 2/2] chassis: Fix chassis_private record updates when the system-id changes.
Numan Siddique
numans at ovn.org
Fri Sep 4 13:52:01 UTC 2020
On Thu, Sep 3, 2020 at 8:34 PM Dumitru Ceara <dceara at redhat.com> wrote:
> Also:
> - Change conditional monitoring for Chassis_Private to use the chassis uuid
> instead of chassis name. Using the chassis->name field does not work
> because this is the old value of the field and would cause ovsdb-server
> to inform ovn-controller that the updated Chassis_Private record was
> "deleted" because it doesn't match the monitor condition anymore.
> - Allow ovn-sbctl to access Chassis_Private records by name.
>
> Reported-at: https://bugzilla.redhat.com/1873032
> Reported-by: Ying Xu <yinxu at redhat.com>
> CC: Han Zhou <hzhou at ovn.org>
> Fixes: 4adc10f58127 ("Avoid nb_cfg update notification flooding")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>
> ---
> v2:
> - Fix the monitor condition update reported by Numan.
>
Thanks Dumitru. I applied both the patches to master.
Numan
> ---
> controller/chassis.c | 92
> ++++++++++++++++++++++++++++++++++++++-----
> controller/chassis.h | 2 +
> controller/ovn-controller.c | 13 ++++--
> tests/ovn-controller.at | 18 ++++++++
> utilities/ovn-sbctl.c | 3 +
> 5 files changed, 112 insertions(+), 16 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index 97120a9..45e018e 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -635,6 +635,77 @@ 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.
> + *
> + * 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 struct sbrec_chassis_private_table *chassis_pvt_table,
> + const struct sbrec_chassis *chassis)
> +{
> + const struct sbrec_chassis_private *chassis_p = NULL;
> +
> + if (chassis_info_id_inited(&chassis_state)) {
> + 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);
> + }
> +
> + if (!chassis_p && ovnsb_idl_txn) {
> + 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,
> + const char *chassis_id)
> +{
> + if (!chassis_pvt->name || strcmp(chassis_pvt->name, chassis_id)) {
> + sbrec_chassis_private_set_name(chassis_pvt, chassis_id);
> + }
> +
> + if (chassis_pvt->chassis != chassis) {
> + sbrec_chassis_private_set_chassis(chassis_pvt, chassis);
> + }
> +}
> +
> /* Returns this chassis's Chassis record, if it is available. */
> const struct sbrec_chassis *
> chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> @@ -642,6 +713,7 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> 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,
> @@ -670,7 +742,6 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> bool updated = chassis_update(chassis_rec, ovnsb_idl_txn,
> &ovs_cfg,
> chassis_id, transport_zones);
>
> - chassis_info_set_id(&chassis_state, chassis_id);
> if (!existed || updated) {
> ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
> "ovn-controller: %s chassis '%s'",
> @@ -678,17 +749,16 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> chassis_id);
> }
>
> - const struct sbrec_chassis_private *chassis_private_rec =
> - chassis_private_lookup_by_name(sbrec_chassis_private_by_name,
> - chassis_id);
> - if (!chassis_private_rec && ovnsb_idl_txn) {
> - chassis_private_rec =
> sbrec_chassis_private_insert(ovnsb_idl_txn);
> - sbrec_chassis_private_set_name(chassis_private_rec,
> - chassis_id);
> - sbrec_chassis_private_set_chassis(chassis_private_rec,
> - chassis_rec);
> + *chassis_private =
> + chassis_private_get_record(ovnsb_idl_txn,
> + sbrec_chassis_private_by_name,
> + chassis_pvt_table, chassis_rec);
> +
> + if (*chassis_private) {
> + chassis_private_update(*chassis_private, chassis_rec,
> chassis_id);
> }
> - *chassis_private = chassis_private_rec;
> +
> + chassis_info_set_id(&chassis_state, chassis_id);
> }
>
> ovs_chassis_cfg_destroy(&ovs_cfg);
> diff --git a/controller/chassis.h b/controller/chassis.h
> index 81055b4..220f726 100644
> --- a/controller/chassis.h
> +++ b/controller/chassis.h
> @@ -26,6 +26,7 @@ struct ovsrec_bridge;
> struct ovsrec_open_vswitch_table;
> struct sbrec_chassis;
> struct sbrec_chassis_table;
> +struct sbrec_chassis_private_table;
> struct sset;
> struct eth_addr;
> struct smap;
> @@ -37,6 +38,7 @@ const struct sbrec_chassis *chassis_run(
> 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);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 874087c..67e06f1 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -179,7 +179,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> * chassis */
> sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ,
> "chassisredirect");
> sbrec_port_binding_add_clause_type(&pb, OVSDB_F_EQ, "external");
> - if (chassis) {
> + if (chassis && !sbrec_chassis_is_new(chassis)) {
> /* This should be mostly redundant with the other clauses for port
> * bindings, but it allows us to catch any ports that are
> assigned to
> * us but should not be. That way, we can clear their chassis
> @@ -202,9 +202,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
> sbrec_igmp_group_add_clause_chassis(&igmp, OVSDB_F_EQ,
> &chassis->header_.uuid);
>
> - /* Monitors Chassis_Private record for current chassis only */
> - sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ,
> - chassis->name);
> + /* Monitors Chassis_Private record for current chassis only. */
> + sbrec_chassis_private_add_clause_chassis(&chprv, OVSDB_F_EQ,
> + &chassis->header_.uuid);
> } else {
> /* During initialization, we monitor all records in
> Chassis_Private so
> * that we don't try to recreate existing ones. */
> @@ -2425,6 +2425,8 @@ main(int argc, char *argv[])
> 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 struct ovsrec_bridge *br_int =
> process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> const char *chassis_id = get_ovs_chassis_id(ovs_table);
> @@ -2433,7 +2435,8 @@ main(int argc, char *argv[])
> if (chassis_id) {
> chassis = chassis_run(ovnsb_idl_txn,
> sbrec_chassis_by_name,
> sbrec_chassis_private_by_name,
> - ovs_table, chassis_table,
> chassis_id,
> + ovs_table, chassis_table,
> + chassis_pvt_table, chassis_id,
> br_int, &transport_zones,
> &chassis_private);
> }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index f2faf1f..812946b 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -195,6 +195,15 @@ 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}"
> +])
> +
> +# Only one Chassis_Private record should exist.
> +OVS_WAIT_UNTIL([
> + test $(ovn-sbctl --columns _uuid list chassis_private | wc -l) -eq 1
> +])
>
> # Simulate system-id changing while ovn-controller is disconnected from
> the
> # SB.
> @@ -212,6 +221,15 @@ 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}"
> +])
> +
> +# Only one Chassis_Private record should exist.
> +OVS_WAIT_UNTIL([
> + test $(ovn-sbctl --columns _uuid list chassis_private | wc -l) -eq 1
> +])
>
> # Gracefully terminate daemons
> OVN_CLEANUP_SBOX([hv])
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 04e082c..d3eec91 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -1391,6 +1391,9 @@ cmd_set_ssl(struct ctl_context *ctx)
> static const struct ctl_table_class tables[SBREC_N_TABLES] = {
> [SBREC_TABLE_CHASSIS].row_ids[0] = {&sbrec_chassis_col_name, NULL,
> NULL},
>
> + [SBREC_TABLE_CHASSIS_PRIVATE].row_ids[0]
> + = {&sbrec_chassis_private_col_name, NULL, NULL},
> +
> [SBREC_TABLE_DATAPATH_BINDING].row_ids
> = {{&sbrec_datapath_binding_col_external_ids, "name", NULL},
> {&sbrec_datapath_binding_col_external_ids, "name2", NULL},
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list