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 <mmich...@redhat.com> Signed-off-by: Han Zhou <hzh...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
