On 5/11/20 11:46 AM, [email protected] wrote:
> From: Numan Siddique <[email protected]>
>
> This patch handles ct zone changes and OVS interface changes incrementally
> in the flow output stage.
>
> Any changes to ct zone can be handled by running physical_run() instead of
> running
> flow_output_run(). And any changes to OVS interfaces can be either handled
> incrementally (for OVS interfaces representing VIFs) or just running
> physical_run() (for tunnel and other types of interfaces).
>
> To better handle this, a new engine node 'physical_flow_changes' is added
> which
> handles changes to ct zone and OVS interfaces.
>
> Signed-off-by: Numan Siddique <[email protected]>
> ---
> controller/binding.c | 22 ---------
> controller/lflow.c | 13 ++++++
> controller/lflow.h | 2 +
> controller/ovn-controller.c | 92 ++++++++++++++++++++++++++++++++++++-
> controller/ovn-controller.h | 22 +++++++++
> controller/physical.c | 51 ++++++++++++++++++++
> controller/physical.h | 5 +-
> 7 files changed, 182 insertions(+), 25 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 662424518..3821edc03 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -499,22 +499,6 @@ remove_local_lport_ids(const struct sbrec_port_binding
> *binding_rec,
> sset_find_and_delete(local_lport_ids, buf);
> }
>
> -enum local_binding_type {
> - BT_VIF,
> - BT_CHILD,
> - BT_VIRTUAL
> -};
> -
> -struct local_binding {
> - struct ovs_list node; /* In parent if any. */
> - char *name;
> - enum local_binding_type type;
> - const struct ovsrec_interface *iface;
> - const struct sbrec_port_binding *pb;
> -
> - struct ovs_list children;
> -};
> -
> static struct local_binding *
> local_binding_create(const char *name, const struct ovsrec_interface *iface,
> const struct sbrec_port_binding *pb,
> @@ -535,12 +519,6 @@ local_binding_add(struct shash *local_bindings, struct
> local_binding *lbinding)
> shash_add(local_bindings, lbinding->name, lbinding);
> }
>
> -static struct local_binding *
> -local_binding_find(struct shash *local_bindings, const char *name)
> -{
> - return shash_find_data(local_bindings, name);
> -}
> -
Hi Numan,
I think if struct local_binding is to be shared between units we should
probably decide on a single place where the structure is defined and all
it's related API functions should be declared there.
Now we have "struct local_binding" and local_binding_find() in
ovn-controller.h and local_bindings_destroy() in binding.h.
> static void
> local_binding_destroy(struct local_binding *lbinding)
> {
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 01214a3a6..512258cd3 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -847,3 +847,16 @@ lflow_destroy(void)
> expr_symtab_destroy(&symtab);
> shash_destroy(&symtab);
> }
> +
> +bool
> +lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *pb_table)
> +{
> + const struct sbrec_port_binding *binding;
> + SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (binding, pb_table) {
> + if (!strcmp(binding->type, "remote")) {
> + return false;
> + }
> + }
> +
> + return true;
> +}
Shouldn't lflow_evaluate_pb_changes() be part of a separate patch?
> diff --git a/controller/lflow.h b/controller/lflow.h
> index f02f709d7..fa54d4de2 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -48,6 +48,7 @@ struct sbrec_dhcp_options_table;
> struct sbrec_dhcpv6_options_table;
> struct sbrec_logical_flow_table;
> struct sbrec_mac_binding_table;
> +struct sbrec_port_binding_table;
> struct simap;
> struct sset;
> struct uuid;
> @@ -153,6 +154,7 @@ void lflow_handle_changed_neighbors(
> const struct hmap *local_datapaths,
> struct ovn_desired_flow_table *);
>
> +bool lflow_evaluate_pb_changes(const struct sbrec_port_binding_table *);
> void lflow_destroy(void);
>
> void lflow_expr_destroy(struct hmap *lflow_expr_cache);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index f94cd16d1..4eee9a2c9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1369,8 +1369,13 @@ static void init_physical_ctx(struct engine_node *node,
>
> ovs_assert(br_int && chassis);
>
> + struct ovsrec_interface_table *iface_table =
> + (struct ovsrec_interface_table *)EN_OVSDB_GET(
> + engine_get_input("OVS_interface",
> + engine_get_input("physical_flow_changes", node)));
> struct ed_type_ct_zones *ct_zones_data =
> - engine_get_input_data("ct_zones", node);
> + engine_get_input_data("ct_zones",
> + engine_get_input("physical_flow_changes", node));
> struct simap *ct_zones = &ct_zones_data->current;
>
> p_ctx->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
> @@ -1378,12 +1383,14 @@ static void init_physical_ctx(struct engine_node
> *node,
> p_ctx->mc_group_table = multicast_group_table;
> p_ctx->br_int = br_int;
> p_ctx->chassis_table = chassis_table;
> + p_ctx->iface_table = iface_table;
> p_ctx->chassis = chassis;
> p_ctx->active_tunnels = &rt_data->active_tunnels;
> p_ctx->local_datapaths = &rt_data->local_datapaths;
> p_ctx->local_lports = &rt_data->local_lports;
> p_ctx->ct_zones = ct_zones;
> p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
> + p_ctx->local_bindings = &rt_data->local_bindings;
> }
>
> static void init_lflow_ctx(struct engine_node *node,
> @@ -1646,6 +1653,8 @@ flow_output_sb_port_binding_handler(struct engine_node
> *node, void *data)
> * always logical router port, and any change to logical router port
> * would just trigger recompute.
> *
> + * - When a port binding of type 'remote' is updated by ovn-ic.
> + *
> * Although there is no correctness issue so far (except the unusual ACL
> * use case, which doesn't seem to be a real problem), it might be better
> * to handle this more gracefully, without the need to consider these
> @@ -1656,6 +1665,14 @@ flow_output_sb_port_binding_handler(struct engine_node
> *node, void *data)
> struct physical_ctx p_ctx;
> init_physical_ctx(node, rt_data, &p_ctx);
>
> + if (!lflow_evaluate_pb_changes(p_ctx.port_binding_table)) {
> + /* If this returns false, it means there is an impact on
> + * the logical flow processing because of some changes in
> + * port bindings. Return false so that recompute is triggered
> + * for this stage. */
> + return false;
> + }
> +
> physical_handle_port_binding_changes(&p_ctx, flow_table);
>
> engine_set_node_state(node, EN_UPDATED);
> @@ -1791,6 +1808,70 @@ flow_output_port_groups_handler(struct engine_node
> *node, void *data)
> return _flow_output_resource_ref_handler(node, data, REF_TYPE_PORTGROUP);
> }
>
> +struct ed_type_physical_flow_changes {
> + bool need_physical_run;
> + bool ovs_ifaces_changed;
> +};
> +
> +static bool
> +flow_output_physical_flow_changes_handler(struct engine_node *node, void
> *data)
> +{
> + struct ed_type_physical_flow_changes *pfc =
> + engine_get_input_data("physical_flow_changes", node);
> + struct ed_type_runtime_data *rt_data =
> + engine_get_input_data("runtime_data", node);
> +
> + struct ed_type_flow_output *fo = data;
> + struct physical_ctx p_ctx;
> + init_physical_ctx(node, rt_data, &p_ctx);
> +
> + engine_set_node_state(node, EN_UPDATED);
> + if (pfc->need_physical_run) {
> + pfc->need_physical_run = false;
> + physical_run(&p_ctx, &fo->flow_table);
> + return true;
> + }
> +
> + if (pfc->ovs_ifaces_changed) {
> + pfc->ovs_ifaces_changed = false;
> + return physical_handle_ovs_iface_changes(&p_ctx, &fo->flow_table);
> + }
> +
> + return true;
> +}
> +
> +static void *
> +en_physical_flow_changes_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct ed_type_physical_flow_changes *data = xzalloc(sizeof *data);
> + return data;
> +}
> +
> +static void
> +en_physical_flow_changes_cleanup(void *data OVS_UNUSED)
> +{
> +
> +}
> +
> +static void
> +en_physical_flow_changes_run(struct engine_node *node, void *data OVS_UNUSED)
> +{
> + struct ed_type_physical_flow_changes *pfc = data;
> + pfc->need_physical_run = true;
> + engine_set_node_state(node, EN_UPDATED);
> +}
> +
> +static bool
> +physical_flow_changes_ovs_iface_handler(struct engine_node *node OVS_UNUSED,
> + void *data OVS_UNUSED)
> +{
> + struct ed_type_physical_flow_changes *pfc = data;
> + pfc->ovs_ifaces_changed = true;
> + engine_set_node_state(node, EN_UPDATED);
> + return true;
> +}
> +
> struct ovn_controller_exit_args {
> bool *exiting;
> bool *restart;
> @@ -1913,6 +1994,7 @@ main(int argc, char *argv[])
> ENGINE_NODE(runtime_data, "runtime_data");
> ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> + ENGINE_NODE(physical_flow_changes, "physical_flow_changes");
> ENGINE_NODE(flow_output, "flow_output");
> ENGINE_NODE(addr_sets, "addr_sets");
> ENGINE_NODE(port_groups, "port_groups");
> @@ -1932,13 +2014,19 @@ main(int argc, char *argv[])
> engine_add_input(&en_port_groups, &en_sb_port_group,
> port_groups_sb_port_group_handler);
>
> + engine_add_input(&en_physical_flow_changes, &en_ct_zones,
> + NULL);
> + engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
> + physical_flow_changes_ovs_iface_handler);
> +
> engine_add_input(&en_flow_output, &en_addr_sets,
> flow_output_addr_sets_handler);
> engine_add_input(&en_flow_output, &en_port_groups,
> flow_output_port_groups_handler);
> engine_add_input(&en_flow_output, &en_runtime_data, NULL);
> - engine_add_input(&en_flow_output, &en_ct_zones, NULL);
> engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
> + engine_add_input(&en_flow_output, &en_physical_flow_changes,
> + flow_output_physical_flow_changes_handler);
>
> engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL);
> engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> index 5d9466880..40c358cef 100644
> --- a/controller/ovn-controller.h
> +++ b/controller/ovn-controller.h
> @@ -72,6 +72,28 @@ struct local_datapath {
> struct local_datapath *get_local_datapath(const struct hmap *,
> uint32_t tunnel_key);
>
> +enum local_binding_type {
> + BT_VIF,
> + BT_CHILD,
> + BT_VIRTUAL
> +};
> +
> +struct local_binding {
> + struct ovs_list node; /* In parent if any. */
> + char *name;
> + enum local_binding_type type;
> + const struct ovsrec_interface *iface;
> + const struct sbrec_port_binding *pb;
> +
> + struct ovs_list children;
> +};
> +
> +static inline struct local_binding *
> +local_binding_find(struct shash *local_bindings, const char *name)
> +{
> + return shash_find_data(local_bindings, name);
> +}
> +
> const struct ovsrec_bridge *get_bridge(const struct ovsrec_bridge_table *,
> const char *br_name);
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 144aeb7bd..03eb677d1 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1780,6 +1780,57 @@ physical_run(struct physical_ctx *p_ctx,
> simap_destroy(&new_tunnel_to_ofport);
> }
>
> +bool
> +physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
> + struct ovn_desired_flow_table *flow_table)
> +{
> + const struct ovsrec_interface *iface_rec;
> + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, p_ctx->iface_table) {
> + if (!strcmp(iface_rec->type, "geneve") ||
> + !strcmp(iface_rec->type, "patch")) {
> + return false;
> + }
> + }
> +
> + struct ofpbuf ofpacts;
> + ofpbuf_init(&ofpacts, 0);
> +
> + OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, p_ctx->iface_table) {
> + const char *iface_id = smap_get(&iface_rec->external_ids,
> "iface-id");
> + if (!iface_id) {
> + continue;
> + }
> +
> + const struct local_binding *lb =
> + local_binding_find(p_ctx->local_bindings, iface_id);
> +
> + if (!lb || !lb->pb) {
> + continue;
> + }
> +
> + int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
> + if (ovsrec_interface_is_deleted(iface_rec)) {
> + ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
> + simap_find_and_delete(&localvif_to_ofport, iface_id);
If I understand correctly we should always be able to find this iface_id
in localvif_to_ofport, right?
Should we log an error when it's not there? Or assert?
Thanks,
Dumitru
> + } else {
> + if (!ovsrec_interface_is_new(iface_rec)) {
> + ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
> + }
> +
> + simap_put(&localvif_to_ofport, iface_id, ofport);
> + consider_port_binding(p_ctx->sbrec_port_binding_by_name,
> + p_ctx->mff_ovn_geneve, p_ctx->ct_zones,
> + p_ctx->active_tunnels,
> + p_ctx->local_datapaths,
> + lb->pb, p_ctx->chassis,
> + flow_table, &ofpacts);
> + }
> + }
> +
> + ofpbuf_uninit(&ofpacts);
> + return true;
> +}
> +
> bool
> get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t
> *ofport)
> {
> diff --git a/controller/physical.h b/controller/physical.h
> index dadf84ea1..9ca34436a 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -49,11 +49,13 @@ struct physical_ctx {
> const struct ovsrec_bridge *br_int;
> const struct sbrec_chassis_table *chassis_table;
> const struct sbrec_chassis *chassis;
> + const struct ovsrec_interface_table *iface_table;
> const struct sset *active_tunnels;
> struct hmap *local_datapaths;
> struct sset *local_lports;
> const struct simap *ct_zones;
> enum mf_field_id mff_ovn_geneve;
> + struct shash *local_bindings;
> };
>
> void physical_register_ovs_idl(struct ovsdb_idl *);
> @@ -63,7 +65,8 @@ void physical_handle_port_binding_changes(struct
> physical_ctx *,
> struct ovn_desired_flow_table *);
> void physical_handle_mc_group_changes(struct physical_ctx *,
> struct ovn_desired_flow_table *);
> -
> +bool physical_handle_ovs_iface_changes(struct physical_ctx *,
> + struct ovn_desired_flow_table *);
> bool get_tunnel_ofport(const char *chassis_name, char *encap_ip,
> ofp_port_t *ofport);
> #endif /* controller/physical.h */
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
