[ovs-dev] [PATCH ovn 1/2] chassis: Fix the way encaps are updated for a chassis record.

Numan Siddique numans at ovn.org
Thu Sep 3 09:57:27 UTC 2020


On Thu, Aug 27, 2020 at 7:32 PM Dumitru Ceara <dceara at redhat.com> wrote:

> ovn-controller always stores the last configured system-id/chassis-id in
> memory regardless if the connection to the SB is up or down. This is OK
> as long as the change can be committed successfully when the SB DB
> connection comes back up.
>
> This commit changes the way we search for a "stale" chassis record in the
> SB to cover the above mentioned case. Also, in such cases there's no need
> to recreate the Encaps, it's sufficient to update the chassis_name field.
>
> Fixes: 5344f24ecb1a ("ovn-controller: Refactor chassis.c to abstract the
> string parsing")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>


Hi Dumitru,

Thanks for the patch. Below are few observations when I tested with this
patch and without

   1.  In a sandbox environment, when I change the external_ids:system-id,
the chassis row for the
     ovn-controller never gets updated.  I observed with this patch and
without this patch. I see the below error
    message

   ******
  2020-09-03T09:19:53.654Z|00032|ovsdb_idl|WARN|transaction error:
{"details":"RBAC rules for client \"chassis-1\" role \"ovn-controller\"
prohibit row insertion into table \"Chassis_Private\".","error":"permission
error"}
2020-09-03T09:19:53.654Z|00033|ovsdb_idl|WARN|transaction error:
{"details":"RBAC rules for client \"chassis-1\" role \"ovn-controller\"
prohibit row insertion into table \"Chassis_Private\".","error":"permission
error"}
*****
     When I use a unix socket for SB DB connection, I don't see this issue.
May be its to do with the RBAC permissions. Its not related to your patch.
But thought of listing it here.

  2. When I change the external_ids:system-id name, the chassis_private row
is leaked. Again this is not related to your patch. But something we should
fix ?

  3. with this patch, I ran the below commands
      - ovs-vsctl set open . external_ids:system-id=ch-1  -- > the chassis
row is recreated with ch-1
      -  ovs-vsctl set open . external_ids:system-id=ch-2  -- > the chassis
row is recreated with ch-2
      - ovs-vsctl set open . external_ids:system-id=ch-1 -- > the chassis
row is still ch-2.
     - ovs-vsctl set open . external_ids:system-id=ch-3  -- > the chassis
row is recreated with ch-3
     - ovs-vsctl set open . external_ids:system-id=ch-2 -- > the chassis
row is still ch-3.

    Something is not correct when the old system-id is restored.

I think it's better to address (3) in this patch. I think (1) and (2) can
be addressed in the same patch series or as a separate series.

What do you think ?

Thanks
Numan





> ---
> NOTE: this patch should be backported down to branch-20.03.
> ---
>  controller/chassis.c    |   60
> ++++++++++++++++++++++++++++++-----------------
>  tests/ovn-controller.at |   17 +++++++++++++
>  2 files changed, 55 insertions(+), 22 deletions(-)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index d392d4f..97120a9 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -399,10 +399,7 @@ chassis_tunnels_changed(const struct sset
> *encap_type_set,
>  {
>      size_t encap_type_count = 0;
>
> -    for (int i = 0; i < chassis_rec->n_encaps; i++) {
> -        if (strcmp(chassis_rec->name,
> chassis_rec->encaps[i]->chassis_name)) {
> -            return true;
> -        }
> +    for (size_t i = 0; i < chassis_rec->n_encaps; i++) {
>
>          if (!sset_contains(encap_type_set, chassis_rec->encaps[i]->type))
> {
>              return true;
> @@ -475,6 +472,19 @@ chassis_build_encaps(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  }
>
>  /*
> + * 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.
>   */
> @@ -505,9 +515,10 @@ chassis_get_stale_record(const struct
> sbrec_chassis_table *chassis_table,
>  /* 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) 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.
> + * 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.
>   *
>   * Sets '*chassis_rec' to point to the local chassis record.
>   * Returns true if this record was already in the database, false if it
> was
> @@ -521,28 +532,32 @@ chassis_get_record(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>                     const char *chassis_id,
>                     const struct sbrec_chassis **chassis_rec)
>  {
> +    const struct sbrec_chassis *chassis = NULL;
> +
>      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)) {
> -            VLOG_DBG("Could not find Chassis, will create it"
> -                     ": stored (%s) ovs (%s)",
> +        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 (ovnsb_idl_txn) {
> -                /* Recreate the chassis record.  */
> -                *chassis_rec = sbrec_chassis_insert(ovnsb_idl_txn);
> -                return false;
> -            }
>          }
> -    } else {
> -        *chassis_rec =
> -            chassis_get_stale_record(chassis_table, ovs_cfg, chassis_id);
> +    }
>
> -        if (!(*chassis_rec) && ovnsb_idl_txn) {
> +    if (!chassis) {
> +        chassis = chassis_get_stale_record(chassis_table, ovs_cfg,
> chassis_id);
> +    }
> +
> +    if (!chassis) {
> +        /* Recreate 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);
> -            return false;
>          }
> +        return false;
>      }
> +
> +    *chassis_rec = chassis;
>      return true;
>  }
>
> @@ -604,6 +619,7 @@ 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;
>      }
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 1b96934..f2faf1f 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -196,6 +196,23 @@ OVS_WAIT_UNTIL([
>      test "${sysid}" = "${chassis_id}"
>  ])
>
> +# 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}"
> +])
> +
>  # Gracefully terminate daemons
>  OVN_CLEANUP_SBOX([hv])
>  OVN_CLEANUP_VSWITCH([main])
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list