From patchwork Mon Sep 7 10:10:45 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dumitru Ceara X-Patchwork-Id: 1358725 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=ieP8w77o; dkim-atps=neutral Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BlPF83MHxz9sSJ for ; Mon, 7 Sep 2020 20:11:07 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 66FAC87104; Mon, 7 Sep 2020 10:11:03 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jrf7cQS39Gxx; Mon, 7 Sep 2020 10:11:02 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 707D6870F0; Mon, 7 Sep 2020 10:11:02 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 5E19AC0052; Mon, 7 Sep 2020 10:11:02 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 26C43C0051 for ; Mon, 7 Sep 2020 10:11:01 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 037F5204EB for ; Mon, 7 Sep 2020 10:11:01 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nywHx8pQxwmn for ; Mon, 7 Sep 2020 10:10:59 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by silver.osuosl.org (Postfix) with ESMTPS id C62E7204E5 for ; Mon, 7 Sep 2020 10:10:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599473457; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:content-type:content-type; bh=SXVW3Yj/KRX3qBIp820vngAoHzmouj61UYDpK1WYT2c=; b=ieP8w77ocPfqNrXHZItaV8IELFb32qSsS04eaoulkTF3ZpJF1KIcyTUWvdLI1u48jfsmo6 jjPKnuwcVhL/AHZJjG9ps6b9oQwsbTL63sjj1HFmcdEIGYRN3O+l3RYSQ18ydXZHv92Bc1 VMNarkGId+TLoZste9/+MusyFjeomoU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-219-Syi3yBThMhSOa169aKSbjA-1; Mon, 07 Sep 2020 06:10:55 -0400 X-MC-Unique: Syi3yBThMhSOa169aKSbjA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CB84F64089 for ; Mon, 7 Sep 2020 10:10:54 +0000 (UTC) Received: from dceara.remote.csb (ovpn-113-208.ams2.redhat.com [10.36.113.208]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3C4527E301 for ; Mon, 7 Sep 2020 10:10:54 +0000 (UTC) From: Dumitru Ceara To: dev@openvswitch.org Date: Mon, 7 Sep 2020 12:10:45 +0200 Message-Id: <1599473445-29748-1-git-send-email-dceara@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dceara@redhat.com X-Mimecast-Spam-Score: 0.001 X-Mimecast-Originator: redhat.com Subject: [ovs-dev] [PATCH ovn branch-20.03] chassis: Fix the way encaps are updated for a chassis record. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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. Without this change, if the chassis-id changes while the SB connection is down, ovn-controller will fail to create the new record but nevertheless update its in-memory chassis-id. When the SB connection is restored ovn-controller tries to find the record corresponding to the chassis-id it stored in memory. This fails causing ovn-controller to try to insert a new record. But at this point a constraint violation is hit in the SB because the Encap records of the "stale" chassis still exist in the DB, along with the old chassis record. 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 (cherry-picked from master commit 94a32fca2d2b825fece0ef5b1873459bd9857dd3) --- controller/chassis.c | 60 ++++++++++++++++++++++++++++++------------------- tests/ovn-controller.at | 17 ++++++++++++++ 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/controller/chassis.c b/controller/chassis.c index 522893e..d525463 100644 --- a/controller/chassis.c +++ b/controller/chassis.c @@ -380,10 +380,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; @@ -456,6 +453,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. */ @@ -486,9 +496,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. */ static const struct sbrec_chassis * chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn, @@ -497,29 +508,31 @@ chassis_get_record(struct ovsdb_idl_txn *ovnsb_idl_txn, 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_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); - } } - } else { - 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) { + 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) { + return sbrec_chassis_insert(ovnsb_idl_txn); } } - return chassis_rec; + + return chassis; } /* Update a Chassis record based on the config in the ovs config. */ @@ -567,6 +580,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; } diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 63b2581..1c7aa58 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])