[ovs-dev] [PATCH ovn] chassis.c: Add comment to SB DB transaction only when needed.
Dumitru Ceara
dceara at redhat.com
Mon Jun 29 11:34:19 UTC 2020
On 6/29/20 9:35 AM, Numan Siddique wrote:
>
>
> On Thu, Jun 25, 2020 at 1:28 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>
> The chassis_run() function incorrectly adds a "ovn-controller:
> registering chassis" comment to every SB transaction. This should be
> done
> only when the chassis record is created or updated. If nothing
> changes in
> the chassis record we shouldn't add useless extra information to the
> transaction request.
>
> Reported-by: Daniel Alvarez <dalvarez at redhat.com
> <mailto:dalvarez at redhat.com>>
> Reported-at: https://bugzilla.redhat.com/1850511
> Fixes: 5344f24ecb1a ("ovn-controller: Refactor chassis.c to abstract
> the string parsing")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>>
> ---
> controller/chassis.c | 64
> +++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index d619361..33a1e18 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -489,53 +489,64 @@ chassis_get_stale_record(const struct
> sbrec_chassis_table *chassis_table,
> * Otherwise (i.e., first time we create the record) 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.
> + *
> + * 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 const struct sbrec_chassis *
> +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 char *chassis_id,
> + const struct sbrec_chassis **chassis_rec)
> {
> - const struct sbrec_chassis *chassis_rec;
> -
> if (chassis_info_id_inited(&chassis_state)) {
> - chassis_rec = chassis_lookup_by_name(sbrec_chassis_by_name,
> -
> chassis_info_id(&chassis_state));
> - if (!chassis_rec) {
> + *chassis_rec = chassis_lookup_by_name(sbrec_chassis_by_name,
> +
> chassis_info_id(&chassis_state));
> + if (!(*chassis_rec)) {
> VLOG_DBG("Could not find Chassis, will create it"
> ": stored (%s) ovs (%s)",
> chassis_info_id(&chassis_state), chassis_id);
> if (ovnsb_idl_txn) {
> /* Recreate the chassis record. */
> - chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> + *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> + return false;
> }
> }
> } else {
> - chassis_rec =
> + *chassis_rec =
> chassis_get_stale_record(chassis_table, ovs_cfg,
> chassis_id);
>
> - if (!chassis_rec && ovnsb_idl_txn) {
> - chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> + if (!(*chassis_rec) && ovnsb_idl_txn) {
> + *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> + return false;
> }
> }
> - return chassis_rec;
> + return true;
> }
>
> -/* Update a Chassis record based on the config in the ovs config. */
> -static void
> +/* Update a Chassis record based on the config in the ovs config.
> + * Returns true if 'chassis_rec' was updated, false otherwise.
> + */
> +static bool
> chassis_update(const struct sbrec_chassis *chassis_rec,
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> const struct ovs_chassis_cfg *ovs_cfg,
> const char *chassis_id,
> const struct sset *transport_zones)
> {
> + bool updated = false;
> +
> if (strcmp(chassis_id, chassis_rec->name)) {
> sbrec_chassis_set_name(chassis_rec, chassis_id);
> + updated = true;
> }
>
> if (strcmp(ovs_cfg->hostname, chassis_rec->hostname)) {
> sbrec_chassis_set_hostname(chassis_rec, ovs_cfg->hostname);
> + updated = true;
> }
>
> if (chassis_other_config_changed(ovs_cfg->bridge_mappings,
> @@ -561,6 +572,7 @@ chassis_update(const struct sbrec_chassis
> *chassis_rec,
> * systems, this behavior should be removed in the future. */
> sbrec_chassis_set_external_ids(chassis_rec, &other_config);
> smap_destroy(&other_config);
> + updated = true;
> }
>
> update_chassis_transport_zones(transport_zones, chassis_rec);
> @@ -571,7 +583,7 @@ chassis_update(const struct sbrec_chassis
> *chassis_rec,
> &ovs_cfg->encap_ip_set,
> ovs_cfg->encap_csum,
> chassis_rec);
> if (!tunnels_changed) {
> - return;
> + return updated;
> }
>
> struct sbrec_encap **encaps;
> @@ -583,6 +595,7 @@ chassis_update(const struct sbrec_chassis
> *chassis_rec,
> ovs_cfg->encap_csum, &n_encap);
> sbrec_chassis_set_encaps(chassis_rec, encaps, n_encap);
> free(encaps);
> + return true;
> }
>
> /* Returns this chassis's Chassis record, if it is available. */
> @@ -603,21 +616,26 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> return NULL;
> }
>
> - const struct sbrec_chassis *chassis_rec =
> - chassis_get_record(ovnsb_idl_txn, sbrec_chassis_by_name,
> - chassis_table, &ovs_cfg, chassis_id);
> + 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);
>
> /* 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) {
> - chassis_update(chassis_rec, ovnsb_idl_txn, &ovs_cfg,
> chassis_id,
> - transport_zones);
> + bool updated = chassis_update(chassis_rec, ovnsb_idl_txn,
> &ovs_cfg,
> + chassis_id, transport_zones);
> +
> chassis_info_set_id(&chassis_state, chassis_id);
> - ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
> - "ovn-controller: registering
> chassis '%s'",
> - chassis_id);
> + if (!existed || updated) {
> + ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
> + "ovn-controller: registering "
> + "chassis '%s'",
> + chassis_id);
>
>
> Hi Dumitru,
>
> The patch LGTM.
>
> To make the comment more accurate, how about the below ?
>
> ovsdb_idl_txn_add_comment(ovnsb_idl_txn,
> "ovn-controller: %s "
> "chassis '%s'",
> !existed ? "registering" :
> "updating",
> chassis_id);
>
>
> What do you think ?
>
> Thanks
> Numan
Hi Numan,
Thanks for the review. I sent a v2 with your suggested change:
https://patchwork.ozlabs.org/project/openvswitch/patch/1593430322-7726-1-git-send-email-dceara@redhat.com/
Thanks,
Dumitru
>
> + }
> }
>
> ovs_chassis_cfg_destroy(&ovs_cfg);
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org <mailto:dev at openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list