[ovs-dev] [PATCH v5 15/20] ovn-controller: Incremental processing for address-set changes.

Han Zhou zhouhan at gmail.com
Mon Aug 13 17:48:14 UTC 2018


When the content of an address set changes, ovn-controller will
not recompute all flows but only the ones related to the changed
address-set. The performance test result is discussed at [1].

[1] https://mail.openvswitch.org/pipermail/ovs-discuss/2018-June/046880.html

Tested-by: Mark Michelson <mmichels at redhat.com>
Signed-off-by: Han Zhou <hzhou8 at ebay.com>
---
 ovn/controller/lflow.c          | 107 +++++++++++++++++++++++++
 ovn/controller/lflow.h          |  22 +++++
 ovn/controller/ovn-controller.c | 172 +++++++++++++++++++++++++++++++++++++++-
 tests/ovn.at                    |  74 +++++++++++++++++
 4 files changed, 373 insertions(+), 2 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index e13ee69..f3d7b44 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -413,6 +413,113 @@ lflow_handle_changed_flows(
     return ret;
 }
 
+bool
+lflow_handle_changed_ref(
+    enum ref_type ref_type,
+    const char *ref_name,
+    struct ovsdb_idl_index *sbrec_chassis_by_name,
+    struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
+    struct ovsdb_idl_index *sbrec_port_binding_by_name,
+    const struct sbrec_dhcp_options_table *dhcp_options_table,
+    const struct sbrec_dhcpv6_options_table *dhcpv6_options_table,
+    const struct sbrec_logical_flow_table *logical_flow_table,
+    const struct hmap *local_datapaths,
+    const struct sbrec_chassis *chassis,
+    const struct shash *addr_sets,
+    const struct shash *port_groups,
+    const struct sset *active_tunnels,
+    const struct sset *local_lport_ids,
+    struct ovn_desired_flow_table *flow_table,
+    struct ovn_extend_table *group_table,
+    struct ovn_extend_table *meter_table,
+    struct lflow_resource_ref *lfrr,
+    uint32_t *conj_id_ofs,
+    bool *changed)
+{
+    struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table,
+                                                   ref_type, ref_name);
+    if (!rlfn) {
+        *changed = false;
+        return true;
+    }
+    VLOG_DBG("Handle changed lflow reference for resource type: %d,"
+             " name: %s.", ref_type, ref_name);
+    *changed = false;
+    bool ret = true;
+
+    hmap_remove(&lfrr->ref_lflow_table, &rlfn->node);
+
+    struct lflow_ref_list_node *lrln, *next;
+    /* Detach the rlfn->ref_lflow_head nodes from the lfrr table and clean
+     * up all other nodes related to the lflows that uses the resource,
+     * so that the old nodes won't interfere with updating the lfrr table
+     * when reparsing the lflows. */
+    LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
+        ovs_list_remove(&lrln->lflow_list);
+        lflow_resource_destroy_lflow(lfrr, &lrln->lflow_uuid);
+    }
+
+    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
+    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
+    const struct sbrec_dhcp_options *dhcp_opt_row;
+    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row, dhcp_options_table) {
+        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
+                     dhcp_opt_row->type);
+    }
+
+    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
+    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH(dhcpv6_opt_row, dhcpv6_options_table) {
+       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code,
+                    dhcpv6_opt_row->type);
+    }
+
+    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
+    nd_ra_opts_init(&nd_ra_opts);
+
+    /* Re-parse the related lflows. */
+    LIST_FOR_EACH (lrln, ref_list, &rlfn->ref_lflow_head) {
+        const struct sbrec_logical_flow *lflow =
+            sbrec_logical_flow_table_get_for_uuid(logical_flow_table,
+                                                  &lrln->lflow_uuid);
+        if (!lflow) {
+            VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
+                     " name: %s - not found.",
+                     UUID_ARGS(&lrln->lflow_uuid),
+                     ref_type, ref_name);
+            continue;
+        }
+        VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %d,"
+                 " name: %s.",
+                 UUID_ARGS(&lrln->lflow_uuid),
+                 ref_type, ref_name);
+        ofctrl_remove_flows(flow_table, &lrln->lflow_uuid);
+        if (!consider_logical_flow(sbrec_chassis_by_name,
+                                   sbrec_multicast_group_by_name_datapath,
+                                   sbrec_port_binding_by_name,
+                                   lflow, local_datapaths,
+                                   chassis, &dhcp_opts, &dhcpv6_opts,
+                                   &nd_ra_opts, addr_sets, port_groups,
+                                   active_tunnels, local_lport_ids,
+                                   flow_table, group_table, meter_table,
+                                   lfrr, conj_id_ofs)) {
+            ret = false;
+            break;
+        }
+        *changed = true;
+    }
+
+    LIST_FOR_EACH_SAFE (lrln, next, ref_list, &rlfn->ref_lflow_head) {
+        ovs_list_remove(&lrln->ref_list);
+        free(lrln);
+    }
+    free(rlfn);
+
+    dhcp_opts_destroy(&dhcp_opts);
+    dhcp_opts_destroy(&dhcpv6_opts);
+    nd_ra_opts_destroy(&nd_ra_opts);
+    return ret;
+}
+
 static bool
 update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
 {
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index bb5949b..01dda1d 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -154,6 +154,28 @@ bool lflow_handle_changed_flows(
     struct lflow_resource_ref *,
     uint32_t *conj_id_ofs);
 
+bool lflow_handle_changed_ref(
+    enum ref_type,
+    const char *ref_name,
+    struct ovsdb_idl_index *sbrec_chassis_by_name,
+    struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath,
+    struct ovsdb_idl_index *sbrec_port_binding_by_name,
+    const struct sbrec_dhcp_options_table *,
+    const struct sbrec_dhcpv6_options_table *,
+    const struct sbrec_logical_flow_table *,
+    const struct hmap *local_datapaths,
+    const struct sbrec_chassis *,
+    const struct shash *addr_sets,
+    const struct shash *port_groups,
+    const struct sset *active_tunnels,
+    const struct sset *local_lport_ids,
+    struct ovn_desired_flow_table *,
+    struct ovn_extend_table *group_table,
+    struct ovn_extend_table *meter_table,
+    struct lflow_resource_ref *,
+    uint32_t *conj_id_ofs,
+    bool *changed);
+
 void lflow_destroy(void);
 
 #endif /* ovn/lflow.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 261c087..0e445df 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -295,6 +295,29 @@ addr_sets_init(const struct sbrec_address_set_table *address_set_table,
     }
 }
 
+static void
+addr_sets_update(const struct sbrec_address_set_table *address_set_table,
+                 struct shash *addr_sets, struct sset *new,
+                 struct sset *deleted, struct sset *updated)
+{
+    const struct sbrec_address_set *as;
+    SBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (as, address_set_table) {
+        if (sbrec_address_set_is_deleted(as)) {
+            expr_const_sets_remove(addr_sets, as->name);
+            sset_add(deleted, as->name);
+        } else {
+            expr_const_sets_add(addr_sets, as->name,
+                                (const char *const *) as->addresses,
+                                as->n_addresses, true);
+            if (sbrec_address_set_is_new(as)) {
+                sset_add(new, as->name);
+            } else {
+                sset_add(updated, as->name);
+            }
+        }
+    }
+}
+
 /* Iterate port groups in the southbound database.  Create and update the
  * corresponding symtab entries as necessary. */
 static void
@@ -606,6 +629,7 @@ const char *ovs_engine_node_names[] = {
 
 struct ed_type_addr_sets {
     struct shash addr_sets;
+    bool change_tracked;
     struct sset new;
     struct sset deleted;
     struct sset updated;
@@ -616,6 +640,7 @@ en_addr_sets_init(struct engine_node *node)
 {
     struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
     shash_init(&as->addr_sets);
+    as->change_tracked = false;
     sset_init(&as->new);
     sset_init(&as->deleted);
     sset_init(&as->updated);
@@ -648,7 +673,32 @@ en_addr_sets_run(struct engine_node *node)
 
     addr_sets_init(as_table, &as->addr_sets);
 
+    as->change_tracked = false;
+    node->changed = true;
+}
+
+static bool
+addr_sets_sb_address_set_handler(struct engine_node *node)
+{
+    struct ed_type_addr_sets *as = (struct ed_type_addr_sets *)node->data;
+
+    sset_clear(&as->new);
+    sset_clear(&as->deleted);
+    sset_clear(&as->updated);
+
+    struct sbrec_address_set_table *as_table =
+        (struct sbrec_address_set_table *)EN_OVSDB_GET(
+            engine_get_input("SB_address_set", node));
+
+    addr_sets_update(as_table, &as->addr_sets, &as->new,
+                     &as->deleted, &as->updated);
+
+    node->changed = !sset_is_empty(&as->new) || !sset_is_empty(&as->deleted)
+                    || !sset_is_empty(&as->updated);
+
+    as->change_tracked = true;
     node->changed = true;
+    return true;
 }
 
 struct ed_type_runtime_data {
@@ -1279,6 +1329,124 @@ flow_output_sb_multicast_group_handler(struct engine_node *node)
 
 }
 
+static bool
+flow_output_addr_sets_handler(struct engine_node *node)
+{
+    struct ed_type_runtime_data *data =
+        (struct ed_type_runtime_data *)engine_get_input(
+                "runtime_data", node)->data;
+    struct hmap *local_datapaths = &data->local_datapaths;
+    struct sset *local_lport_ids = &data->local_lport_ids;
+    struct sset *active_tunnels = &data->active_tunnels;
+    struct shash *port_groups = &data->port_groups;
+    struct ed_type_addr_sets *as_data =
+        (struct ed_type_addr_sets *)engine_get_input("addr_sets", node)->data;
+
+    /* XXX: The change_tracked check may be added to inc-proc framework. */
+    if (!as_data->change_tracked) {
+        return false;
+    }
+    struct shash *addr_sets = &as_data->addr_sets;
+
+    struct ovsrec_open_vswitch_table *ovs_table =
+        (struct ovsrec_open_vswitch_table *)EN_OVSDB_GET(
+            engine_get_input("OVS_open_vswitch", node));
+    struct ovsrec_bridge_table *bridge_table =
+        (struct ovsrec_bridge_table *)EN_OVSDB_GET(
+            engine_get_input("OVS_bridge", node));
+    const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
+    const char *chassis_id = get_chassis_id(ovs_table);
+
+    struct ovsdb_idl_index *sbrec_chassis_by_name =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_chassis", node),
+                "name");
+    const struct sbrec_chassis *chassis = NULL;
+    if (chassis_id) {
+        chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
+    }
+
+    ovs_assert(br_int && chassis);
+
+    struct ed_type_flow_output *fo =
+        (struct ed_type_flow_output *)node->data;
+    struct ovn_desired_flow_table *flow_table = &fo->flow_table;
+    struct ovn_extend_table *group_table = &fo->group_table;
+    struct ovn_extend_table *meter_table = &fo->meter_table;
+    uint32_t *conj_id_ofs = &fo->conj_id_ofs;
+    struct lflow_resource_ref *lfrr = &fo->lflow_resource_ref;
+
+    struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_multicast_group", node),
+                "name_datapath");
+
+    struct ovsdb_idl_index *sbrec_port_binding_by_name =
+        engine_ovsdb_node_get_index(
+                engine_get_input("SB_port_binding", node),
+                "name");
+
+    struct sbrec_dhcp_options_table *dhcp_table =
+        (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
+            engine_get_input("SB_dhcp_options", node));
+
+    struct sbrec_dhcpv6_options_table *dhcpv6_table =
+        (struct sbrec_dhcpv6_options_table *)EN_OVSDB_GET(
+            engine_get_input("SB_dhcpv6_options", node));
+
+    struct sbrec_logical_flow_table *logical_flow_table =
+        (struct sbrec_logical_flow_table *)EN_OVSDB_GET(
+            engine_get_input("SB_logical_flow", node));
+
+    bool changed;
+    const char *as;
+
+    SSET_FOR_EACH (as, &as_data->deleted) {
+        if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, as,
+                    sbrec_chassis_by_name,
+                    sbrec_multicast_group_by_name_datapath,
+                    sbrec_port_binding_by_name,dhcp_table,
+                    dhcpv6_table, logical_flow_table,
+                    local_datapaths, chassis, addr_sets,
+                    port_groups, active_tunnels, local_lport_ids,
+                    flow_table, group_table, meter_table, lfrr,
+                    conj_id_ofs, &changed)) {
+            return false;
+        }
+        node->changed = changed || node->changed;
+    }
+    SSET_FOR_EACH (as, &as_data->updated) {
+        if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, as,
+                    sbrec_chassis_by_name,
+                    sbrec_multicast_group_by_name_datapath,
+                    sbrec_port_binding_by_name,dhcp_table,
+                    dhcpv6_table, logical_flow_table,
+                    local_datapaths, chassis, addr_sets,
+                    port_groups, active_tunnels, local_lport_ids,
+                    flow_table, group_table, meter_table, lfrr,
+                    conj_id_ofs, &changed)) {
+            return false;
+        }
+        node->changed = changed || node->changed;
+    }
+    SSET_FOR_EACH (as, &as_data->new) {
+        if (!lflow_handle_changed_ref(REF_TYPE_ADDRSET, as,
+                    sbrec_chassis_by_name,
+                    sbrec_multicast_group_by_name_datapath,
+                    sbrec_port_binding_by_name,dhcp_table,
+                    dhcpv6_table, logical_flow_table,
+                    local_datapaths, chassis, addr_sets,
+                    port_groups, active_tunnels, local_lport_ids,
+                    flow_table, group_table, meter_table, lfrr,
+                    conj_id_ofs, &changed)) {
+            return false;
+        }
+        node->changed = changed || node->changed;
+    }
+
+    return true;
+}
+
 struct ovn_controller_exit_args {
     bool *exiting;
     bool *restart;
@@ -1387,9 +1555,9 @@ main(int argc, char *argv[])
 
     /* Add dependencies between inc-proc-engine nodes. */
 
-    engine_add_input(&en_addr_sets, &en_sb_address_set, NULL);
+    engine_add_input(&en_addr_sets, &en_sb_address_set, addr_sets_sb_address_set_handler);
 
-    engine_add_input(&en_flow_output, &en_addr_sets, NULL);
+    engine_add_input(&en_flow_output, &en_addr_sets, flow_output_addr_sets_handler);
     engine_add_input(&en_flow_output, &en_runtime_data, NULL);
     engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 70c6c50..49bdf99 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10975,5 +10975,79 @@ OVN_CHECK_PACKETS([hv2/vif1-tx.pcap], [expected2])
 as hv2 start_daemon ovn-controller
 
 OVN_CLEANUP([hv1],[hv2])
+AT_CLEANUP
+
+AT_SETUP([ovn -- Address Set Incremental Processing])
+AT_KEYWORDS([ovn_as_inc])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+net_add n1
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.10
 
+ovn-nbctl ls-add ls1
+for i in 1 2; do
+    ovn-nbctl lsp-add ls1 lp$i \
+        -- lsp-set-addresses lp$i "f0:00:00:00:00:0$i 192.168.1.$i"
+    as hv1 ovs-vsctl \
+        -- add-port br-int vif$i \
+        -- set Interface vif$i \
+            external-ids:iface-id=lp$i
+done
+
+for i in 1 2 3; do
+    as1_uuid=`ovn-nbctl --wait=hv create addr name=as1`
+    as2_uuid=`ovn-nbctl --wait=hv create addr name=as2`
+    ovn-nbctl --wait=hv acl-add ls1 to-lport 200 \
+            'outport=="lp1" && ip4 && ip4.src == {$as1, $as2}' allow-related
+    ovn-nbctl --wait=hv set addr as1 addresses="10.1.2.10"
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.10"], [0], [ignore])
+
+    # Update address set as1
+    ovn-nbctl --wait=hv set addr as1 addresses="10.1.2.10 10.1.2.11"
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.11"], [0], [ignore])
+
+    # Update address set as2
+    ovn-nbctl --wait=hv set addr as2 addresses="10.1.2.12 10.1.2.13"
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [0], [ignore])
+
+    # Add another ACL referencing as1
+    n_flows_before=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
+    ovn-nbctl --wait=hv acl-add ls1 to-lport 200 \
+            'outport=="lp2" && ip4 && ip4.src == $as1' allow-related
+    n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
+    AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after], [0], [ignore])
+
+    # Remove an ACL
+    ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \
+            'outport=="lp2" && ip4 && ip4.src == $as1'
+    n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc -l`
+    AT_CHECK([test $n_flows_before = $n_flows_after], [0], [ignore])
+
+    # Remove as1 while it is still used by an ACL, the lflows should be reparsed and
+    # parsing should fail.
+    echo "before del as1"
+    ovn-nbctl list addr | grep as1
+    ovn-nbctl --wait=hv destroy addr $as1_uuid
+    echo "after del as1"
+    ovn-nbctl list addr | grep as1
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.10"], [1], [ignore])
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [1], [ignore])
+
+    # Recreate as1
+    as1_uuid=`ovn-nbctl --wait=hv create addr name=as1`
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [0], [ignore])
+
+    # Remove ACLs and address sets
+    ovn-nbctl --wait=hv destroy addr $as1_uuid -- destroy addr $as2_uuid
+    AT_CHECK([ovs-ofctl dump-flows br-int | grep "10.1.2.12"], [1], [ignore])
+
+    ovn-nbctl --wait=hv acl-del ls1
+done
+
+# Gracefully terminate daemons
+OVN_CLEANUP([hv1])
 AT_CLEANUP
-- 
2.1.0



More information about the dev mailing list