From patchwork Thu Aug 25 20:04:10 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gurucharan Shetty X-Patchwork-Id: 662922 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3sKwB06HCFz9sBc for ; Fri, 26 Aug 2016 06:04:22 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 55FE110547; Thu, 25 Aug 2016 13:04:21 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 22C69102F6 for ; Thu, 25 Aug 2016 13:04:20 -0700 (PDT) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 59AFF420486 for ; Thu, 25 Aug 2016 14:04:19 -0600 (MDT) X-ASG-Debug-ID: 1472155458-09eadd035110960001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar5.cudamail.com with ESMTP id 59HXcDXt5gp2AyCU (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Thu, 25 Aug 2016 14:04:18 -0600 (MDT) X-Barracuda-Envelope-From: guru@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO relay7-d.mail.gandi.net) (217.70.183.200) by mx1-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 25 Aug 2016 20:04:18 -0000 Received-SPF: pass (mx1-pf1.cudamail.com: SPF record at ovn.org designates 217.70.183.200 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.200 X-Barracuda-RBL-IP: 217.70.183.200 Received: from mfilter25-d.gandi.net (mfilter25-d.gandi.net [217.70.178.153]) by relay7-d.mail.gandi.net (Postfix) with ESMTP id 5B1CEAFF for ; Thu, 25 Aug 2016 22:04:16 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter25-d.gandi.net Received: from relay7-d.mail.gandi.net ([IPv6:::ffff:217.70.183.200]) by mfilter25-d.gandi.net (mfilter25-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id u7uPd1A8So8Q for ; Thu, 25 Aug 2016 22:04:13 +0200 (CEST) X-Originating-IP: 209.85.215.43 Received: from mail-lf0-f43.google.com (mail-lf0-f43.google.com [209.85.215.43]) (Authenticated sender: guru@ovn.org) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 598C49AB for ; Thu, 25 Aug 2016 22:04:12 +0200 (CEST) Received: by mail-lf0-f43.google.com with SMTP id f93so42460673lfi.2 for ; Thu, 25 Aug 2016 13:04:12 -0700 (PDT) X-Gm-Message-State: AE9vXwP2f21bPLL+GIIjN97Ol+zHR9bAYjw0HKaaos/GVekXyXeqTnDSy+MoJA6+2TOjsTE1XtW6WJhrVMM0UA== X-Received: by 10.25.142.203 with SMTP id q194mr2652234lfd.11.1472155452156; Thu, 25 Aug 2016 13:04:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.114.5.232 with HTTP; Thu, 25 Aug 2016 13:04:10 -0700 (PDT) In-Reply-To: <1472088625-19306-1-git-send-email-rmoats@us.ibm.com> References: <1472088625-19306-1-git-send-email-rmoats@us.ibm.com> X-CudaMail-Envelope-Sender: guru@ovn.org From: Guru Shetty Date: Thu, 25 Aug 2016 13:04:10 -0700 X-Gmail-Original-Message-ID: Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-824065579 X-CudaMail-DTE: 082516 X-CudaMail-Originating-IP: 217.70.183.200 To: Ryan Moats X-ASG-Orig-Subj: [##CM-E1-824065579##]Re: [ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1472155458 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Content-Filtered-By: Mailman/MimeDel 2.1.16 Cc: ovs dev Subject: Re: [ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On 24 August 2016 at 18:30, Ryan Moats wrote: > As [1] indicates, incremental processing hasn't resulted > in an improvement worth the complexity it has added. > This patch backs out all of the code specific to incremental > processing, along with the persisting of OF flows, > logical ports, multicast groups, all_lports, local and patched > datapaths > > [1] http://openvswitch.org/pipermail/dev/2016-August/078272.html > > Signed-off-by: Ryan Moats > Co-authored-by: Guru Shetty > I think you should remove the co-authored-by. I just gave a very silly test that I don't think warrants a co-authored-by. I think you should add the following incremental to the test to make it a little better. dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev diff --git a/tests/ovn.at b/tests/ovn.at index 30411d8..c161d07 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -5015,7 +5015,7 @@ for i in `seq 1 3`; do -- lsp-set-addresses bar$i "f0:00:0a:01:02:$i 172.16.1.$ip" done -OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-int table=0 | grep REG13]) +OVS_WAIT_UNTIL([test `ovs-ofctl dump-flows br-int table=0 | grep REG13 | wc -l` -eq 4]) OVN_CLEANUP([hv1]) As expected, the load-balancer tests now fail. But in addition to the zone allocation, there was another issue which was added as part of incremental processing fixes that was causing consistent test failures. The following incremental reverts it back to how it was before all the incremental processing changes (There is still a corner case, but I don't think it was caused by incremental processing, so I will send a separate fix for that). diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index 9fc8a93..647b4f0 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -253,8 +253,6 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, uint32_t conj_id_ofs = 1; const struct sbrec_logical_flow *lflow; - ovn_group_table_clear(group_table, false); - struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts); struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts); const struct sbrec_dhcp_options *dhcp_opt_row; diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c index 5c3125a..d8e111d 100644 --- a/ovn/controller/ofctrl.c +++ b/ovn/controller/ofctrl.c @@ -708,16 +708,6 @@ add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs) ^L /* group_table. */ -static struct group_info * -group_info_clone(struct group_info *source) -{ - struct group_info *clone = xmalloc(sizeof *clone); - ds_clone(&clone->group, &source->group); - clone->group_id = source->group_id; - clone->hmap_node.hash = source->hmap_node.hash; - return clone; -} - /* Finds and returns a group_info in 'existing_groups' whose key is identical * to 'target''s key, or NULL if there is none. */ static struct group_info * @@ -770,7 +760,10 @@ add_group_mod(const struct ofputil_group_mod *gm, struct ovs_list *msgs) * with ofctrl_add_flow(). * * Replaces the group table on the switch, if possible, by the contents of - * 'groups->desired_groups'. + * 'groups->desired_groups'. Regardless of whether the group table + * is updated, this deletes all the groups from the + * 'groups->desired_groups' and frees them. (The hmap itself isn't + * destroyed.) * * This should be called after ofctrl_run() within the main loop. */ void @@ -929,10 +922,13 @@ ofctrl_put(struct hmap *flow_table, int64_t nb_cfg) /* Move the contents of desired_groups to existing_groups. */ HMAP_FOR_EACH_SAFE(desired, next_group, hmap_node, &groups->desired_groups) { + hmap_remove(&groups->desired_groups, &desired->hmap_node); if (!ovn_group_lookup(&groups->existing_groups, desired)) { - struct group_info *clone = group_info_clone(desired); - hmap_insert(&groups->existing_groups, &clone->hmap_node, - clone->hmap_node.hash); + hmap_insert(&groups->existing_groups, &desired->hmap_node, + desired->hmap_node.hash); + } else { + ds_destroy(&desired->group); + free(desired); } } diff --git a/tests/system-ovn.at b/tests/system-ovn.at index e267384..b9da189 100755 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -373,10 +373,11 @@ for i in `seq 1 20`; do done dnl Each server should have at least one connection. -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1)], [0], [dnl -tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=/'], [0], [dnl +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=/'], [0], [dnl +tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=/'], [0], [dnl +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=172.16.1.2,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=,dport=),reply=(src=192.168.1.3,dst=192.168.1.2,sport=,dport=< -tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=192.168.1.4,dst=192.168.1.2,sport=,dport=< -tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=192.168.1.5,dst=192.168.1.2,sport=,dport=< +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.1) | \ +sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=192.168.1.3,dst=192.168.1.2,sport=,dport=< +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=192.168.1.4,dst=192.168.1.2,sport=,dport=< +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=192.168.1.5,dst=192.168.1.2,sport=,dport=< ]) dnl Test load-balancing that includes L4 ports in NAT. @@ -502,10 +506,11 @@ for i in `seq 1 20`; do done dnl Each server should have at least one connection. -AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2)], [0], [dnl -tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.3,dst=192.168.1.2,sport=,dport=< -tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.4,dst=192.168.1.2,sport=,dport=< -tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.5,dst=192.168.1.2,sport=,dport=< +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.2) | \ +sed -e 's/zone=[[0-9]]*/zone=/'], [0], [dnl +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.3,dst=192.168.1.2,sport=,dport=< +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.4,dst=192.168.1.2,sport=,dport=< +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=192.168.1.5,dst=192.168.1.2,sport=,dport=< ]) _______________________________________________ dev mailing list