From patchwork Thu Aug 25 20:11:42 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gurucharan Shetty X-Patchwork-Id: 662925 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 3sKwLf62D1z9s65 for ; Fri, 26 Aug 2016 06:11:54 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id E822B108CB; Thu, 25 Aug 2016 13:11:53 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id 67D4B10873 for ; Thu, 25 Aug 2016 13:11:52 -0700 (PDT) Received: from bar6.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id F058B16259E for ; Thu, 25 Aug 2016 14:11:51 -0600 (MDT) X-ASG-Debug-ID: 1472155910-0b3237564e41590001-byXFYA Received: from mx3-pf1.cudamail.com ([192.168.14.2]) by bar6.cudamail.com with ESMTP id h8I1T5FErwQRsu78 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 25 Aug 2016 14:11:50 -0600 (MDT) X-Barracuda-Envelope-From: guru@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.2 Received: from unknown (HELO relay2-d.mail.gandi.net) (217.70.183.194) by mx3-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 25 Aug 2016 20:11:50 -0000 Received-SPF: pass (mx3-pf1.cudamail.com: SPF record at ovn.org designates 217.70.183.194 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.194 X-Barracuda-RBL-IP: 217.70.183.194 Received: from mfilter29-d.gandi.net (mfilter29-d.gandi.net [217.70.178.160]) by relay2-d.mail.gandi.net (Postfix) with ESMTP id AD41CC5A43 for ; Thu, 25 Aug 2016 22:11:47 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter29-d.gandi.net Received: from relay2-d.mail.gandi.net ([IPv6:::ffff:217.70.183.194]) by mfilter29-d.gandi.net (mfilter29-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id QeNjOzvXKPiA for ; Thu, 25 Aug 2016 22:11:44 +0200 (CEST) X-Originating-IP: 209.85.215.51 Received: from mail-lf0-f51.google.com (mail-lf0-f51.google.com [209.85.215.51]) (Authenticated sender: guru@ovn.org) by relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 8B513C5A5A for ; Thu, 25 Aug 2016 22:11:44 +0200 (CEST) Received: by mail-lf0-f51.google.com with SMTP id f93so42593591lfi.2 for ; Thu, 25 Aug 2016 13:11:44 -0700 (PDT) X-Gm-Message-State: AE9vXwNg9XlmYgmw2C71GNQPObZ4htLvudRztGb2aqNWg3sOb9HAd+RSrX/Eaz7qgQVnJq7x2OshZv/qGlk8eg== X-Received: by 10.25.82.9 with SMTP id g9mr3113115lfb.228.1472155903497; Thu, 25 Aug 2016 13:11:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.114.5.232 with HTTP; Thu, 25 Aug 2016 13:11:42 -0700 (PDT) In-Reply-To: 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:11:42 -0700 X-Gmail-Original-Message-ID: Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-V1-824045330 X-CudaMail-DTE: 082516 X-CudaMail-Originating-IP: 217.70.183.194 To: Ryan Moats X-ASG-Orig-Subj: [##CM-V1-824045330##]Re: [ovs-dev] [PATCH v4] ovn-controller: Back out incremental processing X-Barracuda-Connect: UNKNOWN[192.168.14.2] X-Barracuda-Start-Time: 1472155910 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA 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 25 August 2016 at 13:04, Guru Shetty wrote: > > > 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. > > 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). > Ignore the incremental below this in the previous mail. It got cut. I am pasting a fresh one here: dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev 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=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) +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=172.16.1.2,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.1,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) ]) dnl Should work with the virtual IP 30.0.0.3 address through NAT @@ -386,10 +387,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.3)], [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=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(30.0.0.3) | \ +sed -e 's/zone=[[0-9]]*/zone=/'], [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=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.3,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) ]) dnl Test load-balancing that includes L4 ports in NAT. @@ -399,10 +401,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=172.16.1.2,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) -tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) +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=172.16.1.2,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=172.16.1.3,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) +tcp,orig=(src=192.168.1.2,dst=30.0.0.2,sport=,dport=),reply=(src=172.16.1.4,dst=192.168.1.2,sport=,dport=),zone=,protoinfo=(state=) ]) @@ -489,10 +492,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=192.168.1.3,dst=192.168.1.2,sport=,dport=),protoinfo=(state=) -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=),protoinfo=(state=) -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=),protoinfo=(state=) +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=),zone=,protoinfo=(state=) +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=),zone=,protoinfo=(state=) +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=),zone=,protoinfo=(state=) ]) 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=),protoinfo=(state=) -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=),protoinfo=(state=) -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=),protoinfo=(state=) +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=),zone=,protoinfo=(state=) +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=),zone=,protoinfo=(state=) +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=),zone=,protoinfo=(state=) ]) _______________________________________________ dev mailing list