On Thu, Aug 27, 2020 at 7:32 PM Dumitru Ceara <[email protected]> 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 <[email protected]>
>
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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
