From patchwork Thu Apr 8 12:31:12 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1463779 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=2605:bc80:3010::137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4FGLGn4rWfz9sX3 for ; Thu, 8 Apr 2021 22:31:29 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 6A28D418CA; Thu, 8 Apr 2021 12:31:25 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3NRTVu4kEhaf; Thu, 8 Apr 2021 12:31:24 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp4.osuosl.org (Postfix) with ESMTP id 1B93B418D6; Thu, 8 Apr 2021 12:31:23 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 02F48C000C; Thu, 8 Apr 2021 12:31:23 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8ED93C000A for ; Thu, 8 Apr 2021 12:31:21 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 6F323418D6 for ; Thu, 8 Apr 2021 12:31:21 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id em3wKFXiOsCL for ; Thu, 8 Apr 2021 12:31:19 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by smtp4.osuosl.org (Postfix) with ESMTPS id 642BE418CA for ; Thu, 8 Apr 2021 12:31:19 +0000 (UTC) X-Originating-IP: 78.45.89.65 Received: from im-t490s.redhat.com (ip-78-45-89-65.net.upcbroadband.cz [78.45.89.65]) (Authenticated sender: i.maximets@ovn.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 75468FF80E; Thu, 8 Apr 2021 12:31:15 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Thu, 8 Apr 2021 14:31:12 +0200 Message-Id: <20210408123112.678123-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Cc: Dumitru Ceara , Ilya Maximets Subject: [ovs-dev] [PATCH ovn] ofctrl: Send all flow modifications in a bundle. 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: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" If some OF rules needs to be replaced due to logical flow changes, ovn-controller deletes old rules first and adds new ones later. In a complex scenario with big number of flows a lot of time can pass between these events leading to the dataplane downtime and packet loss. Also, while these changes are in progress, OVS will use incomplete pipelines that will also lead to packet drops. To avoid this, all flow modifications should be done atomically, so there will be always some version of OF rules installed that can handle dataplane traffic and it will be complete in terms of reflecting some consistent set of logical flows. Wrapping all flow modifications into atomic ordered bundle to achieve that. Reported-by: Dumitru Ceara Reported-at: https://bugzilla.redhat.com/1947398 Signed-off-by: Ilya Maximets Acked-by: Dumitru Ceara --- controller/ofctrl.c | 80 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 415d9b7e1..0b24d98b3 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -29,6 +29,7 @@ #include "openvswitch/list.h" #include "openvswitch/match.h" #include "openvswitch/ofp-actions.h" +#include "openvswitch/ofp-bundle.h" #include "openvswitch/ofp-flow.h" #include "openvswitch/ofp-group.h" #include "openvswitch/ofp-match.h" @@ -1567,10 +1568,22 @@ encode_flow_mod(struct ofputil_flow_mod *fm) } static void -add_flow_mod(struct ofputil_flow_mod *fm, struct ovs_list *msgs) +add_flow_mod(struct ofputil_flow_mod *fm, + struct ofputil_bundle_ctrl_msg *bc, + struct ovs_list *msgs) { struct ofpbuf *msg = encode_flow_mod(fm); - ovs_list_push_back(msgs, &msg->list_node); + struct ofputil_bundle_add_msg bam = { + .bundle_id = bc->bundle_id, + .flags = bc->flags, + .msg = msg->data, + }; + struct ofpbuf *bundle_msg; + + bundle_msg = ofputil_encode_bundle_add(OFP15_VERSION, &bam); + + ofpbuf_delete(msg); + ovs_list_push_back(msgs, &bundle_msg->list_node); } /* group_table. */ @@ -1752,7 +1765,9 @@ add_meter(struct ovn_extend_table_info *m_desired, } static void -installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs) +installed_flow_add(struct ovn_flow *d, + struct ofputil_bundle_ctrl_msg *bc, + struct ovs_list *msgs) { /* Send flow_mod to add flow. */ struct ofputil_flow_mod fm = { @@ -1764,11 +1779,12 @@ installed_flow_add(struct ovn_flow *d, struct ovs_list *msgs) .new_cookie = htonll(d->cookie), .command = OFPFC_ADD, }; - add_flow_mod(&fm, msgs); + add_flow_mod(&fm, bc, msgs); } static void installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, + struct ofputil_bundle_ctrl_msg *bc, struct ovs_list *msgs) { /* Update actions in installed flow. */ @@ -1787,7 +1803,7 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, /* Use OFPFC_ADD so that cookie can be updated. */ fm.command = OFPFC_ADD; } - add_flow_mod(&fm, msgs); + add_flow_mod(&fm, bc, msgs); /* Replace 'i''s actions and cookie by 'd''s. */ free(i->ofpacts); @@ -1797,7 +1813,9 @@ installed_flow_mod(struct ovn_flow *i, struct ovn_flow *d, } static void -installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) +installed_flow_del(struct ovn_flow *i, + struct ofputil_bundle_ctrl_msg *bc, + struct ovs_list *msgs) { struct ofputil_flow_mod fm = { .match = i->match, @@ -1805,11 +1823,12 @@ installed_flow_del(struct ovn_flow *i, struct ovs_list *msgs) .table_id = i->table_id, .command = OFPFC_DELETE_STRICT, }; - add_flow_mod(&fm, msgs); + add_flow_mod(&fm, bc, msgs); } static void update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, + struct ofputil_bundle_ctrl_msg *bc, struct ovs_list *msgs) { ovs_assert(ovs_list_is_empty(&flow_table->tracked_flows)); @@ -1823,7 +1842,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, if (!d) { /* Installed flow is no longer desirable. Delete it from the * switch and from installed_flows. */ - installed_flow_del(&i->flow, msgs); + installed_flow_del(&i->flow, bc, msgs); ovn_flow_log(&i->flow, "removing installed"); hmap_remove(&installed_flows, &i->match_hmap_node); @@ -1832,7 +1851,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, if (!ofpacts_equal(i->flow.ofpacts, i->flow.ofpacts_len, d->flow.ofpacts, d->flow.ofpacts_len) || i->flow.cookie != d->flow.cookie) { - installed_flow_mod(&i->flow, &d->flow, msgs); + installed_flow_mod(&i->flow, &d->flow, bc, msgs); ovn_flow_log(&i->flow, "updating installed"); } link_installed_to_desired(i, d); @@ -1847,7 +1866,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, i = installed_flow_lookup(&d->flow); if (!i) { ovn_flow_log(&d->flow, "adding installed"); - installed_flow_add(&d->flow, msgs); + installed_flow_add(&d->flow, bc, msgs); /* Copy 'd' from 'flow_table' to installed_flows. */ i = installed_flow_dup(d); @@ -1860,7 +1879,7 @@ update_installed_flows_by_compare(struct ovn_desired_flow_table *flow_table, * flow then modify the installed flow. */ if (link_installed_to_desired(i, d)) { - installed_flow_mod(&i->flow, &d->flow, msgs); + installed_flow_mod(&i->flow, &d->flow, bc, msgs); ovn_flow_log(&i->flow, "updating installed (conflict)"); } } @@ -1941,6 +1960,7 @@ merge_tracked_flows(struct ovn_desired_flow_table *flow_table) static void update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, + struct ofputil_bundle_ctrl_msg *bc, struct ovs_list *msgs) { merge_tracked_flows(flow_table); @@ -1956,7 +1976,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, struct desired_flow *d = installed_flow_get_active(i); if (!d) { - installed_flow_del(&i->flow, msgs); + installed_flow_del(&i->flow, bc, msgs); ovn_flow_log(&i->flow, "removing installed (tracked)"); hmap_remove(&installed_flows, &i->match_hmap_node); @@ -1966,7 +1986,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, * installed flow, so update the OVS flow for the new * active flow (at least the cookie will be different, * even if the actions are the same). */ - installed_flow_mod(&i->flow, &d->flow, msgs); + installed_flow_mod(&i->flow, &d->flow, bc, msgs); ovn_flow_log(&i->flow, "updating installed (tracked)"); } } @@ -1976,7 +1996,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, struct installed_flow *i = installed_flow_lookup(&f->flow); if (!i) { /* Adding a new flow. */ - installed_flow_add(&f->flow, msgs); + installed_flow_add(&f->flow, bc, msgs); ovn_flow_log(&f->flow, "adding installed (tracked)"); /* Copy 'f' from 'flow_table' to installed_flows. */ @@ -1987,7 +2007,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, } else if (installed_flow_get_active(i) == f) { /* The installed flow is installed for f, but f has change * tracked, so it must have been modified. */ - installed_flow_mod(&i->flow, &f->flow, msgs); + installed_flow_mod(&i->flow, &f->flow, bc, msgs); ovn_flow_log(&i->flow, "updating installed (tracked)"); } else if (!f->installed_flow) { /* Adding a new flow that conflicts with an existing installed @@ -1996,7 +2016,7 @@ update_installed_flows_by_track(struct ovn_desired_flow_table *flow_table, * then modify the installed flow. */ if (link_installed_to_desired(i, f)) { - installed_flow_mod(&i->flow, &f->flow, msgs); + installed_flow_mod(&i->flow, &f->flow, bc, msgs); ovn_flow_log(&i->flow, "updating installed (tracked conflict)"); } @@ -2126,10 +2146,34 @@ ofctrl_put(struct ovn_desired_flow_table *flow_table, } } + /* Add all flow updates into a bundle. */ + static int bundle_id = 0; + struct ofputil_bundle_ctrl_msg bc = { + .bundle_id = bundle_id++, + .flags = OFPBF_ORDERED | OFPBF_ATOMIC, + }; + struct ofpbuf *bundle_open, *bundle_commit; + + /* Open a new bundle. */ + bc.type = OFPBCT_OPEN_REQUEST; + bundle_open = ofputil_encode_bundle_ctrl_request(OFP15_VERSION, &bc); + ovs_list_push_back(&msgs, &bundle_open->list_node); + if (flow_table->change_tracked) { - update_installed_flows_by_track(flow_table, &msgs); + update_installed_flows_by_track(flow_table, &bc, &msgs); + } else { + update_installed_flows_by_compare(flow_table, &bc, &msgs); + } + + if (ovs_list_back(&msgs) == &bundle_open->list_node) { + /* No flow updates. Removing the bundle open request. */ + ovs_list_pop_back(&msgs); + ofpbuf_delete(bundle_open); } else { - update_installed_flows_by_compare(flow_table, &msgs); + /* Committing the bundle. */ + bc.type = OFPBCT_COMMIT_REQUEST; + bundle_commit = ofputil_encode_bundle_ctrl_request(OFP15_VERSION, &bc); + ovs_list_push_back(&msgs, &bundle_commit->list_node); } /* Iterate through the installed groups from previous runs. If they