On 11/16/22 22:33, Mark Michelson wrote:
> Hi Dumitru, I have a few comments down below
>
> On 11/4/22 18:11, Dumitru Ceara wrote:
>> Expand SB.Template_Var records in two stages:
>> 1. first expand them to local values in match/action strings
>> 2. then reparse the expanded strings
>>
>> For the case when a lflow references a Chassis_Template_Var
>> also track references (similar to the ones maintained for
>> multicast groups, address sets, port_groups, port bindings).
>>
>> Signed-off-by: Dumitru Ceara <[email protected]>
>> ---
>> V2:
>> - Fix GCC build due to missing newline.
>> - Handle SB table rename Template_Var -> Chassis_Template_Var.
>> - Address Han's comments:
>> - Add new function to parse lflow actions.
>> - Move xstrdup inside lexer_parse_template_string() and execute it
>> only
>> if needed.
>> - Match template vars only by chassis name.
>> - Change local_templates table to a plain smap.
>> - Some indentation updates.
>> - Use NULL template variable change handlers for chassis/Open_vSwitch
>> changes.
>> - Added more tests.
>> - Fix tracking of template references in lflow actions.
>> ---
>> controller/lflow.c | 136 ++++++++++++++------
>> controller/lflow.h | 1
>> controller/ofctrl.c | 11 +-
>> controller/ofctrl.h | 3
>> controller/ovn-controller.c | 289
>> +++++++++++++++++++++++++++++++++++++++++++
>> include/ovn/expr.h | 4 -
>> include/ovn/lex.h | 15 ++
>> lib/actions.c | 9 +
>> lib/expr.c | 18 ++-
>> lib/lex.c | 57 ++++++++
>> lib/objdep.c | 1
>> lib/objdep.h | 1
>> tests/ovn-controller.at | 50 +++++++
>> tests/ovn.at | 80 ++++++++++++
>> tests/test-ovn.c | 17 ++-
>> utilities/ovn-trace.c | 33 ++++-
>> 16 files changed, 658 insertions(+), 67 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index d4434bdee8..fc4371d0df 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -81,6 +81,8 @@ convert_match_to_expr(const struct
>> sbrec_logical_flow *,
>> const struct local_datapath *ldp,
>> struct expr **prereqs, const struct shash
>> *addr_sets,
>> const struct shash *port_groups,
>> + const struct smap *template_vars,
>> + struct sset *template_vars_ref,
>> struct objdep_mgr *, bool *pg_addr_set_ref);
>> static void
>> add_matches_to_flow_table(const struct sbrec_logical_flow *,
>> @@ -297,6 +299,43 @@ as_info_from_expr_const(const char *as_name,
>> const union expr_constant *c,
>> return true;
>> }
>> +static bool
>> +lflow_parse_actions(const struct sbrec_logical_flow *lflow,
>> + const struct lflow_ctx_in *l_ctx_in,
>> + struct sset *template_vars_ref,
>> + struct ofpbuf *ovnacts_out,
>> + struct expr **prereqs_out)
>> +{
>> + bool ingress = !strcmp(lflow->pipeline, "ingress");
>> + struct ovnact_parse_params pp = {
>> + .symtab = &symtab,
>> + .dhcp_opts = l_ctx_in->dhcp_opts,
>> + .dhcpv6_opts = l_ctx_in->dhcpv6_opts,
>> + .nd_ra_opts = l_ctx_in->nd_ra_opts,
>> + .controller_event_opts = l_ctx_in->controller_event_opts,
>> +
>> + .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>> + .n_tables = LOG_PIPELINE_LEN,
>> + .cur_ltable = lflow->table_id,
>> + };
>> +
>> + char *actions_expanded_s = NULL;
>> + const char *actions_s =
>> + lexer_parse_template_string(lflow->actions,
>> l_ctx_in->template_vars,
>> + template_vars_ref,
>> &actions_expanded_s);
>> + char *error = ovnacts_parse_string(actions_s, &pp,
>> + ovnacts_out, prereqs_out);
>> + free(actions_expanded_s);
>> + if (error) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> + VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
>> + lflow->actions, error);
>> + free(error);
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> /* Parses the lflow regarding the changed address set 'as_name', and
>> generates
>> * ovs flows for the newly added addresses in 'as_diff_added' only.
>> It is
>> * similar to consider_logical_flow__, with the below differences:
>> @@ -347,27 +386,14 @@ consider_lflow_for_added_as_ips__(
>> uint64_t ovnacts_stub[1024 / 8];
>> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
>> - struct ovnact_parse_params pp = {
>> - .symtab = &symtab,
>> - .dhcp_opts = l_ctx_in->dhcp_opts,
>> - .dhcpv6_opts = l_ctx_in->dhcpv6_opts,
>> - .nd_ra_opts = l_ctx_in->nd_ra_opts,
>> - .controller_event_opts = l_ctx_in->controller_event_opts,
>> - .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>> - .n_tables = LOG_PIPELINE_LEN,
>> - .cur_ltable = lflow->table_id,
>> - };
>> + struct sset template_vars_ref =
>> SSET_INITIALIZER(&template_vars_ref);
>> struct expr *prereqs = NULL;
>> - char *error;
>> - error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts,
>> &prereqs);
>> - if (error) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> - VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
>> - lflow->actions, error);
>> - free(error);
>> + if (!lflow_parse_actions(lflow, l_ctx_in, &template_vars_ref,
>> + &ovnacts, &prereqs)) {
>> ovnacts_free(ovnacts.data, ovnacts.size);
>> ofpbuf_uninit(&ovnacts);
>> + sset_destroy(&template_vars_ref);
>> return true;
>> }
>> @@ -431,6 +457,8 @@ consider_lflow_for_added_as_ips__(
>> struct expr *expr = convert_match_to_expr(lflow, ldp, &prereqs,
>> l_ctx_in->addr_sets,
>> l_ctx_in->port_groups,
>> + l_ctx_in->template_vars,
>> + &template_vars_ref,
>>
>> l_ctx_out->lflow_deps_mgr, NULL);
>> shash_replace((struct shash *)l_ctx_in->addr_sets, as_name,
>> real_as);
>> if (new_fake_as) {
>> @@ -502,6 +530,14 @@ done:
>> ofpbuf_uninit(&ovnacts);
>> expr_destroy(expr);
>> expr_matches_destroy(&matches);
>> +
>> + const char *tv_name;
>> + SSET_FOR_EACH (tv_name, &template_vars_ref) {
>> + objdep_mgr_add(l_ctx_out->lflow_deps_mgr, OBJDEP_TYPE_TEMPLATE,
>> + tv_name, &lflow->header_.uuid);
>> + }
>
> The loop above seems to be an inlined implementation of
> store_lflow_template_refs(). I think you should just call that function
> instead.
>
You're right. I factored out the function at some point and I failed to
use it in all applicable places. Fixed in v3.
>> + sset_destroy(&template_vars_ref);
>> +
>> return handled;
>> }
>> @@ -913,6 +949,8 @@ convert_match_to_expr(const struct
>> sbrec_logical_flow *lflow,
>> struct expr **prereqs,
>> const struct shash *addr_sets,
>> const struct shash *port_groups,
>> + const struct smap *template_vars,
>> + struct sset *template_vars_ref,
>> struct objdep_mgr *mgr,
>> bool *pg_addr_set_ref)
>> {
>> @@ -920,11 +958,18 @@ convert_match_to_expr(const struct
>> sbrec_logical_flow *lflow,
>> struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
>> char *error = NULL;
>> - struct expr *e = expr_parse_string(lflow->match, &symtab,
>> addr_sets,
>> + char *match_expanded_s = NULL;
>> + const char *match_s = lexer_parse_template_string(lflow->match,
>> + template_vars,
>> + template_vars_ref,
>> +
>> &match_expanded_s);
>> + struct expr *e = expr_parse_string(match_s, &symtab, addr_sets,
>> port_groups, &addr_sets_ref,
>> &port_groups_ref,
>> ldp->datapath->tunnel_key,
>> &error);
>> + free(match_expanded_s);
>> +
>> struct shash_node *addr_sets_ref_node;
>> SHASH_FOR_EACH (addr_sets_ref_node, &addr_sets_ref) {
>> objdep_mgr_add_with_refcount(mgr, OBJDEP_TYPE_ADDRSET,
>> @@ -963,6 +1008,18 @@ convert_match_to_expr(const struct
>> sbrec_logical_flow *lflow,
>> return expr_simplify(e);
>> }
>> +static void
>> +store_lflow_template_refs(struct objdep_mgr *lflow_deps_mgr,
>> + const struct sset *template_vars_ref,
>> + const struct sbrec_logical_flow *lflow)
>> +{
>> + const char *tv_name;
>> + SSET_FOR_EACH (tv_name, template_vars_ref) {
>> + objdep_mgr_add(lflow_deps_mgr, OBJDEP_TYPE_TEMPLATE, tv_name,
>> + &lflow->header_.uuid);
>> + }
>> +}
>> +
>> static void
>> consider_logical_flow__(const struct sbrec_logical_flow *lflow,
>> const struct sbrec_datapath_binding *dp,
>> @@ -1015,28 +1072,16 @@ consider_logical_flow__(const struct
>> sbrec_logical_flow *lflow,
>> * XXX Deny changes to 'outport' in egress pipeline. */
>> uint64_t ovnacts_stub[1024 / 8];
>> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
>> - struct ovnact_parse_params pp = {
>> - .symtab = &symtab,
>> - .dhcp_opts = l_ctx_in->dhcp_opts,
>> - .dhcpv6_opts = l_ctx_in->dhcpv6_opts,
>> - .nd_ra_opts = l_ctx_in->nd_ra_opts,
>> - .controller_event_opts = l_ctx_in->controller_event_opts,
>> -
>> - .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>> - .n_tables = LOG_PIPELINE_LEN,
>> - .cur_ltable = lflow->table_id,
>> - };
>> + struct sset template_vars_ref =
>> SSET_INITIALIZER(&template_vars_ref);
>> struct expr *prereqs = NULL;
>> - char *error;
>> - error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts,
>> &prereqs);
>> - if (error) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> - VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
>> - lflow->actions, error);
>> - free(error);
>> + if (!lflow_parse_actions(lflow, l_ctx_in, &template_vars_ref,
>> + &ovnacts, &prereqs)) {
>> ovnacts_free(ovnacts.data, ovnacts.size);
>> ofpbuf_uninit(&ovnacts);
>> + store_lflow_template_refs(l_ctx_out->lflow_deps_mgr,
>> + &template_vars_ref, lflow);
>
> Can you explain why we call store_lflow_template_refs() in the case
> where lflow_parse_actions() fails? This is different from the behavior
> in consider_lflow_for_added_as_ips__(), where
> store_lflow_template_refs() is only called when lflow_parse_actions()
> succeeds.
>
It should be called in case of failures too. It's for the case when an
address set change is processed incrementally and the added value is a
template. If that template is not yet defined we still need to record
the relation between the flow using the address set and the template.
I added the missing call in v3.
>> + sset_destroy(&template_vars_ref);
>> return;
>> }
>> @@ -1087,6 +1132,8 @@ consider_logical_flow__(const struct
>> sbrec_logical_flow *lflow,
>> case LCACHE_T_NONE:
>> expr = convert_match_to_expr(lflow, ldp, &prereqs,
>> l_ctx_in->addr_sets,
>> l_ctx_in->port_groups,
>> + l_ctx_in->template_vars,
>> + &template_vars_ref,
>> l_ctx_out->lflow_deps_mgr,
>> &pg_addr_set_ref);
>> if (!expr) {
>> @@ -1101,11 +1148,13 @@ consider_logical_flow__(const struct
>> sbrec_logical_flow *lflow,
>> }
>> /* If caching is enabled and this is a not cached expr that
>> doesn't refer
>> - * to address sets or port groups, save it to potentially cache
>> it later.
>> + * to address sets, port groups, or template variables, save it to
>> + * potentially cache it later.
>> */
>> if (lcv_type == LCACHE_T_NONE
>> && lflow_cache_is_enabled(l_ctx_out->lflow_cache)
>> - && !pg_addr_set_ref) {
>> + && !pg_addr_set_ref
>> + && sset_is_empty(&template_vars_ref)) {
>> cached_expr = expr_clone(expr);
>> }
>> @@ -1190,6 +1239,10 @@ done:
>> expr_destroy(cached_expr);
>> expr_matches_destroy(matches);
>> free(matches);
>> +
>> + store_lflow_template_refs(l_ctx_out->lflow_deps_mgr,
>> + &template_vars_ref, lflow);
>> + sset_destroy(&template_vars_ref);
>> }
>> static void
>> @@ -1953,8 +2006,7 @@ add_lb_ct_snat_hairpin_flows(struct
>> ovn_controller_lb *lb,
>> static void
>> consider_lb_hairpin_flows(const struct sbrec_load_balancer *sbrec_lb,
>> - const struct hmap *local_datapaths,
>> - bool use_ct_mark,
>> + const struct hmap *local_datapaths, bool
>> use_ct_mark,
>
> Nit: this doesn't have anything to do with chassis template variables
> and should probably be left out of this patch. If you want to change
> this, it probably should be done as a follow-up patch of its own.
>
Ack.
>> struct ovn_desired_flow_table *flow_table,
>> struct simap *ids)
>> {
>> @@ -2043,8 +2095,8 @@ add_lb_hairpin_flows(const struct
>> sbrec_load_balancer_table *lb_table,
>> ovs_assert(id_pool_alloc_id(pool, &id));
>> simap_put(ids, lb->name, id);
>> }
>> - consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark,
>> - flow_table, ids);
>> + consider_lb_hairpin_flows(lb, local_datapaths, use_ct_mark,
>> flow_table,
>> + ids);
>
> Nit: Same as my previous comment.
>
Ack.
>> }
>> }
>> diff --git a/controller/lflow.h b/controller/lflow.h
>> index 3f369ebfb6..9a7079f99e 100644
>> --- a/controller/lflow.h
>> +++ b/controller/lflow.h
>> @@ -112,6 +112,7 @@ struct lflow_ctx_in {
>> const struct hmap *dhcp_opts;
>> const struct hmap *dhcpv6_opts;
>> const struct controller_event_options *controller_event_opts;
>> + const struct smap *template_vars;
>> bool lb_hairpin_use_ct_mark;
>> };
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index c779912583..f92e2017ce 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -2873,7 +2873,8 @@ ofctrl_lookup_port(const void *br_int_, const
>> char *port_name,
>> char *
>> ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char
>> *flow_s,
>> const struct shash *addr_sets,
>> - const struct shash *port_groups)
>> + const struct shash *port_groups,
>> + const struct smap *template_vars)
>> {
>> int version = rconn_get_version(swconn);
>> if (version < 0) {
>> @@ -2881,9 +2882,15 @@ ofctrl_inject_pkt(const struct ovsrec_bridge
>> *br_int, const char *flow_s,
>> }
>> struct flow uflow;
>> - char *error = expr_parse_microflow(flow_s, &symtab, addr_sets,
>> + char *flow_expanded_s = NULL;
>> + const char *flow_exp_s = lexer_parse_template_string(flow_s,
>> + template_vars,
>> + NULL,
>> +
>> &flow_expanded_s);
>> + char *error = expr_parse_microflow(flow_exp_s, &symtab, addr_sets,
>> port_groups, ofctrl_lookup_port,
>> br_int, &uflow);
>> + free(flow_expanded_s);
>> if (error) {
>> return error;
>> }
>> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
>> index 71d3f5838a..3968245126 100644
>> --- a/controller/ofctrl.h
>> +++ b/controller/ofctrl.h
>> @@ -71,7 +71,8 @@ void ofctrl_ct_flush_zone(uint16_t zone_id);
>> char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int,
>> const char *flow_s, const struct shash
>> *addr_sets,
>> - const struct shash *port_groups);
>> + const struct shash *port_groups,
>> + const struct smap *template_vars);
>> /* Flow table interfaces to the rest of ovn-controller. */
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index f62303e3cb..c374bd0f33 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -91,6 +91,7 @@ static unixctl_cb_func debug_pause_execution;
>> static unixctl_cb_func debug_resume_execution;
>> static unixctl_cb_func debug_status_execution;
>> static unixctl_cb_func debug_dump_local_bindings;
>> +static unixctl_cb_func debug_dump_local_template_vars;
>> static unixctl_cb_func debug_dump_lflow_conj_ids;
>> static unixctl_cb_func lflow_cache_flush_cmd;
>> static unixctl_cb_func lflow_cache_show_stats_cmd;
>> @@ -170,6 +171,8 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>> *
>> * Monitor IGMP_Groups for local chassis.
>> *
>> + * Monitor Template_Var for local chassis.
>> + *
>> * We always monitor patch ports because they allow us to see
>> the linkages
>> * between related logical datapaths. That way, when we know
>> that we have
>> * a VIF on a particular logical switch, we immediately know to
>> monitor all
>> @@ -184,6 +187,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>> struct ovsdb_idl_condition ip_mcast =
>> OVSDB_IDL_CONDITION_INIT(&ip_mcast);
>> struct ovsdb_idl_condition igmp = OVSDB_IDL_CONDITION_INIT(&igmp);
>> struct ovsdb_idl_condition chprv =
>> OVSDB_IDL_CONDITION_INIT(&chprv);
>> + struct ovsdb_idl_condition tv = OVSDB_IDL_CONDITION_INIT(&tv);
>> /* Always monitor all logical datapath groups. Otherwise, DPG
>> updates may
>> * be received *after* the lflows using it are seen by
>> ovn-controller.
>> @@ -201,6 +205,7 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>> ovsdb_idl_condition_add_clause_true(&ip_mcast);
>> ovsdb_idl_condition_add_clause_true(&igmp);
>> ovsdb_idl_condition_add_clause_true(&chprv);
>> + ovsdb_idl_condition_add_clause_true(&tv);
>> goto out;
>> }
>> @@ -239,6 +244,9 @@ update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
>> /* Monitors Chassis_Private record for current chassis only. */
>> sbrec_chassis_private_add_clause_name(&chprv, OVSDB_F_EQ,
>> chassis->name);
>> +
>> + sbrec_chassis_template_var_add_clause_chassis(&tv, OVSDB_F_EQ,
>> + chassis->name);
>> } else {
>> /* During initialization, we monitor all records in
>> Chassis_Private so
>> * that we don't try to recreate existing ones. */
>> @@ -290,6 +298,7 @@ out:;
>> sbrec_ip_multicast_set_condition(ovnsb_idl, &ip_mcast),
>> sbrec_igmp_group_set_condition(ovnsb_idl, &igmp),
>> sbrec_chassis_private_set_condition(ovnsb_idl, &chprv),
>> + sbrec_chassis_template_var_set_condition(ovnsb_idl, &tv),
>> };
>> unsigned int expected_cond_seqno = 0;
>> @@ -307,6 +316,7 @@ out:;
>> ovsdb_idl_condition_destroy(&ip_mcast);
>> ovsdb_idl_condition_destroy(&igmp);
>> ovsdb_idl_condition_destroy(&chprv);
>> + ovsdb_idl_condition_destroy(&tv);
>> return expected_cond_seqno;
>> }
>> @@ -986,7 +996,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>> SB_NODE(load_balancer, "load_balancer") \
>> SB_NODE(fdb, "fdb") \
>> SB_NODE(meter, "meter") \
>> - SB_NODE(static_mac_binding, "static_mac_binding")
>> + SB_NODE(static_mac_binding, "static_mac_binding") \
>> + SB_NODE(chassis_template_var, "chassis_template_var")
>> enum sb_engine_node {
>> #define SB_NODE(NAME, NAME_STR) SB_##NAME,
>> @@ -1648,6 +1659,184 @@
>> runtime_data_sb_datapath_binding_handler(struct engine_node *node
>> OVS_UNUSED,
>> return true;
>> }
>> +static void
>> +local_templates_to_string(const struct smap *local_templates,
>> + struct ds *out_data)
>> +{
>> + const struct smap_node **nodes = smap_sort(local_templates);
>> +
>> + for (size_t i = 0; i < smap_count(local_templates); i++) {
>> + const struct smap_node *node = nodes[i];
>> +
>> + ds_put_format(out_data, "name: '%s' value: '%s'\n",
>> + node->key, node->value);
>> + }
>> + free(nodes);
>> +}
>> +
>> +struct ed_type_template_vars {
>> + struct smap local_templates;
>> +
>> + bool change_tracked;
>> + struct sset new;
>> + struct sset deleted;
>> + struct sset updated;
>> +};
>> +
>> +static void
>> +template_vars_init(struct ovsdb_idl_index *tv_index_by_chassis,
>> + const struct sbrec_chassis *chassis,
>> + struct smap *local_templates)
>> +{
>> + const struct sbrec_chassis_template_var *tv;
>> + struct sbrec_chassis_template_var *tv_key =
>> + sbrec_chassis_template_var_index_init_row(tv_index_by_chassis);
>> + sbrec_chassis_template_var_index_set_chassis(tv_key, chassis->name);
>> +
>> + tv = sbrec_chassis_template_var_index_find(tv_index_by_chassis,
>> tv_key);
>> + if (tv) {
>> + smap_destroy(local_templates);
>> + smap_clone(local_templates, &tv->variables);
>> + }
>> +
>> + sbrec_chassis_template_var_index_destroy_row(tv_key);
>> +}
>> +
>> +static void
>> +template_vars_update(const struct sbrec_chassis_template_var_table
>> *tv_table,
>> + const struct sbrec_chassis *chassis,
>> + struct smap *local_templates, struct sset *new,
>> + struct sset *deleted, struct sset *updated)
>> +{
>> + const struct sbrec_chassis_template_var *tv;
>> + struct smap_node *node;
>> + SBREC_CHASSIS_TEMPLATE_VAR_TABLE_FOR_EACH_TRACKED (tv, tv_table) {
>> + if (strcmp(tv->chassis, chassis->name)) {
>> + continue;
>> + }
>> +
>> + if (sbrec_chassis_template_var_is_deleted(tv)) {
>> + SMAP_FOR_EACH (node, local_templates) {
>> + sset_add(deleted, node->key);
>> + }
>> + } else if (sbrec_chassis_template_var_is_new(tv)) {
>> + ovs_assert(smap_count(local_templates) == 0);
>> + SMAP_FOR_EACH (node, &tv->variables) {
>> + sset_add(new, node->key);
>> + }
>> + } else {
>> + SMAP_FOR_EACH (node, &tv->variables) {
>> + struct smap_node *old = smap_get_node(local_templates,
>> + node->key);
>> + if (old) {
>> + if (strcmp(old->value, node->value)) {
>> + sset_add(updated, node->key);
>> + }
>> + } else {
>> + sset_add(new, node->key);
>> + }
>> + }
>> + SMAP_FOR_EACH (node, local_templates) {
>> + sset_add (deleted, node->key);
>> + }
>> + }
>> +
>> + smap_destroy(local_templates);
>> + smap_clone(local_templates, &tv->variables);
>> + return;
>> + }
>> +}
>> +
>> +static void *
>> +en_template_vars_init(struct engine_node *node OVS_UNUSED,
>> + struct engine_arg *arg OVS_UNUSED)
>> +{
>> + struct ed_type_template_vars *tv_data = xzalloc(sizeof *tv_data);
>> + smap_init(&tv_data->local_templates);
>> + tv_data->change_tracked = false;
>> + sset_init(&tv_data->new);
>> + sset_init(&tv_data->deleted);
>> + sset_init(&tv_data->updated);
>> + return tv_data;
>> +}
>> +
>> +static void
>> +en_template_vars_run(struct engine_node *node, void *data)
>> +{
>> + struct ed_type_template_vars *tv_data = data;
>> +
>> + const struct ovsrec_open_vswitch_table *ovs_table =
>> + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
>> + 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 =
>> + chassis_lookup_by_name(sbrec_chassis_by_name,
>> + get_ovs_chassis_id(ovs_table));
>> + struct ovsdb_idl_index
>> *sbrec_chassis_template_var_index_by_chassis =
>> + engine_ovsdb_node_get_index(
>> + engine_get_input("SB_chassis_template_var", node),
>> + "chassis");
>> +
>> + smap_clear(&tv_data->local_templates);
>> + template_vars_init(sbrec_chassis_template_var_index_by_chassis,
>> + chassis, &tv_data->local_templates);
>> + engine_set_node_state(node, EN_UPDATED);
>> +}
>> +
>> +static bool
>> +template_vars_sb_chassis_template_var_handler(struct engine_node *node,
>> + void *data)
>> +{
>> + struct ed_type_template_vars *tv_data = data;
>> +
>> + const struct sbrec_chassis_template_var_table *tv_table =
>> + EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", node));
>> + const struct ovsrec_open_vswitch_table *ovs_table =
>> + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
>> + 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 =
>> + chassis_lookup_by_name(sbrec_chassis_by_name,
>> + get_ovs_chassis_id(ovs_table));
>> +
>> + template_vars_update(tv_table, chassis, &tv_data->local_templates,
>> + &tv_data->new, &tv_data->deleted,
>> &tv_data->updated);
>> +
>> + if (!sset_is_empty(&tv_data->new) ||
>> !sset_is_empty(&tv_data->deleted) ||
>> + !sset_is_empty(&tv_data->updated)) {
>> + engine_set_node_state(node, EN_UPDATED);
>> + } else {
>> + engine_set_node_state(node, EN_UNCHANGED);
>> + }
>> +
>> + tv_data->change_tracked = true;
>> + return true;
>> +}
>> +
>> +static void
>> +en_template_vars_clear_tracked_data(void *data)
>> +{
>> + struct ed_type_template_vars *tv_data = data;
>> +
>> + sset_clear(&tv_data->new);
>> + sset_clear(&tv_data->deleted);
>> + sset_clear(&tv_data->updated);
>> + tv_data->change_tracked = false;
>> +}
>> +
>> +static void
>> +en_template_vars_cleanup(void *data)
>> +{
>> + struct ed_type_template_vars *tv_data = data;
>> +
>> + smap_destroy(&tv_data->local_templates);
>> + sset_destroy(&tv_data->new);
>> + sset_destroy(&tv_data->deleted);
>> + sset_destroy(&tv_data->updated);
>> +}
>> +
>> struct ed_type_addr_sets {
>> struct shash addr_sets;
>> bool change_tracked;
>> @@ -2702,6 +2891,9 @@ init_lflow_ctx(struct engine_node *node,
>> struct ed_type_dhcp_options *dhcp_opts =
>> engine_get_input_data("dhcp_options", node);
>> + struct ed_type_template_vars *template_vars =
>> + engine_get_input_data("template_vars", node);
>> +
>> l_ctx_in->sbrec_multicast_group_by_name_datapath =
>> sbrec_mc_group_by_name_dp;
>> l_ctx_in->sbrec_logical_flow_by_logical_datapath =
>> @@ -2734,6 +2926,7 @@ init_lflow_ctx(struct engine_node *node,
>> l_ctx_in->dhcp_opts = &dhcp_opts->v4_opts;
>> l_ctx_in->dhcpv6_opts = &dhcp_opts->v6_opts;
>> l_ctx_in->controller_event_opts = &fo->controller_event_opts;
>> + l_ctx_in->template_vars = &template_vars->local_templates;
>> l_ctx_out->flow_table = &fo->flow_table;
>> l_ctx_out->group_table = &fo->group_table;
>> @@ -3060,6 +3253,64 @@ lflow_output_port_groups_handler(struct
>> engine_node *node, void *data)
>> return true;
>> }
>> +static bool
>> +lflow_output_template_vars_handler(struct engine_node *node, void *data)
>> +{
>> + struct ed_type_template_vars *tv_data =
>> + engine_get_input_data("template_vars", node);
>> +
>> + struct ed_type_lflow_output *fo = data;
>> + struct lflow_ctx_out l_ctx_out;
>> + struct lflow_ctx_in l_ctx_in;
>> + init_lflow_ctx(node, fo, &l_ctx_in, &l_ctx_out);
>> +
>> + const char *res_name;
>> + bool changed;
>> +
>> + if (!tv_data->change_tracked) {
>> + return false;
>> + }
>> +
>> + SSET_FOR_EACH (res_name, &tv_data->deleted) {
>> + if (!objdep_mgr_handle_change(l_ctx_out.lflow_deps_mgr,
>> + OBJDEP_TYPE_TEMPLATE,
>> + res_name,
>> lflow_handle_changed_ref,
>> + l_ctx_out.lflows_processed,
>> + &l_ctx_in, &l_ctx_out,
>> &changed)) {
>> + return false;
>> + }
>> + if (changed) {
>> + engine_set_node_state(node, EN_UPDATED);
>> + }
>> + }
>> + SSET_FOR_EACH (res_name, &tv_data->updated) {
>> + if (!objdep_mgr_handle_change(l_ctx_out.lflow_deps_mgr,
>> + OBJDEP_TYPE_TEMPLATE,
>> + res_name,
>> lflow_handle_changed_ref,
>> + l_ctx_out.lflows_processed,
>> + &l_ctx_in, &l_ctx_out,
>> &changed)) {
>> + return false;
>> + }
>> + if (changed) {
>> + engine_set_node_state(node, EN_UPDATED);
>> + }
>> + }
>> + SSET_FOR_EACH (res_name, &tv_data->new) {
>> + if (!objdep_mgr_handle_change(l_ctx_out.lflow_deps_mgr,
>> + OBJDEP_TYPE_TEMPLATE,
>> + res_name,
>> lflow_handle_changed_ref,
>> + l_ctx_out.lflows_processed,
>> + &l_ctx_in, &l_ctx_out,
>> &changed)) {
>> + return false;
>> + }
>> + if (changed) {
>> + engine_set_node_state(node, EN_UPDATED);
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> static bool
>> lflow_output_runtime_data_handler(struct engine_node *node,
>> void *data OVS_UNUSED)
>> @@ -3646,6 +3897,9 @@ main(int argc, char *argv[])
>> struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath
>> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>>
>> &sbrec_static_mac_binding_col_datapath);
>> + struct ovsdb_idl_index *sbrec_chassis_template_var_index_by_chassis
>> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>> +
>> &sbrec_chassis_template_var_col_chassis);
>> ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
>> @@ -3707,6 +3961,7 @@ main(int argc, char *argv[])
>> /* Define inc-proc-engine nodes. */
>> ENGINE_NODE(sb_ro, "sb_ro");
>> + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(template_vars, "template_vars");
>> ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
>> ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow,
>> "ovs_interface_shadow");
>> @@ -3733,6 +3988,10 @@ main(int argc, char *argv[])
>> #undef OVS_NODE
>> /* Add dependencies between inc-proc-engine nodes. */
>> + engine_add_input(&en_template_vars, &en_ovs_open_vswitch, NULL);
>> + engine_add_input(&en_template_vars, &en_sb_chassis, NULL);
>> + engine_add_input(&en_template_vars, &en_sb_chassis_template_var,
>> + template_vars_sb_chassis_template_var_handler);
>> engine_add_input(&en_addr_sets, &en_sb_address_set,
>> addr_sets_sb_address_set_handler);
>> @@ -3800,6 +4059,8 @@ main(int argc, char *argv[])
>> lflow_output_addr_sets_handler);
>> engine_add_input(&en_lflow_output, &en_port_groups,
>> lflow_output_port_groups_handler);
>> + engine_add_input(&en_lflow_output, &en_template_vars,
>> + lflow_output_template_vars_handler);
>> engine_add_input(&en_lflow_output, &en_runtime_data,
>> lflow_output_runtime_data_handler);
>> engine_add_input(&en_lflow_output, &en_non_vif_data,
>> @@ -3908,6 +4169,8 @@ main(int argc, char *argv[])
>> sbrec_mac_binding_by_datapath);
>> engine_ovsdb_node_add_index(&en_sb_static_mac_binding, "datapath",
>> sbrec_static_mac_binding_by_datapath);
>> + engine_ovsdb_node_add_index(&en_sb_chassis_template_var, "chassis",
>> +
>> sbrec_chassis_template_var_index_by_chassis);
>> struct ed_type_lflow_output *lflow_output_data =
>> engine_get_internal_data(&en_lflow_output);
>> @@ -3917,6 +4180,8 @@ main(int argc, char *argv[])
>> engine_get_internal_data(&en_ct_zones);
>> struct ed_type_runtime_data *runtime_data =
>> engine_get_internal_data(&en_runtime_data);
>> + struct ed_type_template_vars *template_vars_data =
>> + engine_get_internal_data(&en_template_vars);
>> ofctrl_init(&lflow_output_data->group_table,
>> &lflow_output_data->meter_table,
>> @@ -3978,6 +4243,10 @@ main(int argc, char *argv[])
>> debug_dump_lflow_conj_ids,
>> &lflow_output_data->conj_ids);
>> + unixctl_command_register("debug/dump-local-template-vars", "",
>> 0, 0,
>> + debug_dump_local_template_vars,
>> + &template_vars_data->local_templates);
>> +
>> unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>> debug_ignore_startup_delay, NULL);
>> @@ -4354,9 +4623,12 @@ main(int argc, char *argv[])
>> engine_get_data(&en_addr_sets);
>> struct ed_type_port_groups *pg_data =
>> engine_get_data(&en_port_groups);
>> - if (br_int && chassis && as_data && pg_data) {
>> + struct ed_type_template_vars *tv_data =
>> + engine_get_data(&en_template_vars);
>> + if (br_int && chassis && as_data && pg_data &&
>> tv_data) {
>> char *error = ofctrl_inject_pkt(br_int,
>> pending_pkt.flow_s,
>> - &as_data->addr_sets,
>> &pg_data->port_groups_cs_local);
>> + &as_data->addr_sets,
>> &pg_data->port_groups_cs_local,
>> + &tv_data->local_templates);
>> if (error) {
>>
>> unixctl_command_reply_error(pending_pkt.conn, error);
>> free(error);
>> @@ -4825,6 +5097,17 @@ debug_dump_lflow_conj_ids(struct unixctl_conn
>> *conn, int argc OVS_UNUSED,
>> ds_destroy(&conj_ids_dump);
>> }
>> +static void
>> +debug_dump_local_template_vars(struct unixctl_conn *conn, int argc
>> OVS_UNUSED,
>> + const char *argv[] OVS_UNUSED, void
>> *local_vars)
>> +{
>> + struct ds tv_str = DS_EMPTY_INITIALIZER;
>> + ds_put_cstr(&tv_str, "Local template vars:\n");
>> + local_templates_to_string(local_vars, &tv_str);
>> + unixctl_command_reply(conn, ds_cstr(&tv_str));
>> + ds_destroy(&tv_str);
>> +}
>> +
>> static void
>> debug_ignore_startup_delay(struct unixctl_conn *conn, int argc
>> OVS_UNUSED,
>> const char *argv[] OVS_UNUSED, void *arg
>> OVS_UNUSED)
>> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
>> index 3b141b034c..80d95ff675 100644
>> --- a/include/ovn/expr.h
>> +++ b/include/ovn/expr.h
>> @@ -59,6 +59,7 @@
>> #include "openvswitch/match.h"
>> #include "openvswitch/meta-flow.h"
>> #include "logical-fields.h"
>> +#include "smap.h"
>> struct ds;
>> struct expr;
>> @@ -521,7 +522,8 @@ union expr_constant {
>> char *string;
>> };
>> -bool expr_constant_parse(struct lexer *, const struct expr_field *,
>> +bool expr_constant_parse(struct lexer *,
>> + const struct expr_field *,
>> union expr_constant *);
>> void expr_constant_format(const union expr_constant *,
>> enum expr_constant_type, struct ds *);
>> diff --git a/include/ovn/lex.h b/include/ovn/lex.h
>> index ecb7ace243..516bbc2bd8 100644
>> --- a/include/ovn/lex.h
>> +++ b/include/ovn/lex.h
>> @@ -23,6 +23,8 @@
>> * This is a simple lexical analyzer (or tokenizer) for OVN match
>> expressions
>> * and ACLs. */
>> +#include "smap.h"
>> +#include "sset.h"
>> #include "openvswitch/meta-flow.h"
>> struct ds;
>> @@ -37,7 +39,8 @@ enum lex_type {
>> LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or
>> 01:02:03:04:05 */
>> LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127
>> or... */
>> LEX_T_MACRO, /* $NAME */
>> - LEX_T_PORT_GROUP, /* @NAME */
>> + LEX_T_PORT_GROUP, /* @NAME */
>> + LEX_T_TEMPLATE, /* ^NAME */
>> LEX_T_ERROR, /* invalid input */
>> /* Bare tokens. */
>> @@ -86,9 +89,9 @@ struct lex_token {
>> /* One of LEX_*. */
>> enum lex_type type;
>> - /* Meaningful for LEX_T_ID, LEX_T_STRING, LEX_T_ERROR,
>> LEX_T_MACRO only.
>> - * For these token types, 's' may point to 'buffer'; otherwise,
>> it points
>> - * to malloc()ed memory owned by the token.
>> + /* Meaningful for LEX_T_ID, LEX_T_STRING, LEX_T_ERROR, LEX_T_MACRO,
>> + * LEX_T_TEMPLATE only. For these token types, 's' may point to
>> 'buffer';
>> + * otherwise, it points to malloc()ed memory owned by the token.
>> *
>> * Must be NULL for other token types.
>> *
>> @@ -151,4 +154,8 @@ void lexer_syntax_error(struct lexer *, const char
>> *message, ...)
>> char *lexer_steal_error(struct lexer *);
>> +const char *lexer_parse_template_string(const char *s,
>> + const struct smap
>> *template_vars,
>> + struct sset *template_vars_ref,
>> + char **out_expanded);
>> #endif /* ovn/lex.h */
>> diff --git a/lib/actions.c b/lib/actions.c
>> index adbb42db4f..d7d4868835 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -258,8 +258,8 @@ add_prerequisite(struct action_context *ctx, const
>> char *prerequisite)
>> struct expr *expr;
>> char *error;
>> - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL,
>> NULL,
>> - NULL, NULL, 0, &error);
>> + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL,
>> NULL, NULL,
>> + NULL, 0, &error);
>> ovs_assert(!error);
>> ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr);
>> }
>> @@ -4695,6 +4695,11 @@ parse_set_action(struct action_context *ctx)
>> static bool
>> parse_action(struct action_context *ctx)
>> {
>> + if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>> + lexer_error(ctx->lexer, "Unexpanded template.");
>> + return false;
>> + }
>> +
>> if (ctx->lexer->token.type != LEX_T_ID) {
>> lexer_syntax_error(ctx->lexer, NULL);
>> return false;
>> diff --git a/lib/expr.c b/lib/expr.c
>> index d1f9d28ca6..43f44771cf 100644
>> --- a/lib/expr.c
>> +++ b/lib/expr.c
>> @@ -695,7 +695,9 @@ parse_field(struct expr_context *ctx, struct
>> expr_field *f)
>> return false;
>> }
>> - symbol = shash_find_data(ctx->symtab, ctx->lexer->token.s);
>> + symbol = ctx->symtab
>> + ? shash_find_data(ctx->symtab, ctx->lexer->token.s)
>> + : NULL;
>> if (!symbol) {
>> lexer_syntax_error(ctx->lexer, "expecting field name");
>> return false;
>> @@ -894,7 +896,10 @@ parse_constant(struct expr_context *ctx, struct
>> expr_constant_set *cs,
>> cs->as_name = NULL;
>> }
>> - if (ctx->lexer->token.type == LEX_T_STRING) {
>> + if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>> + lexer_error(ctx->lexer, "Unexpanded template.");
>> + return false;
>> + } else if (ctx->lexer->token.type == LEX_T_STRING) {
>> if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) {
>> return false;
>> }
>> @@ -978,7 +983,9 @@ expr_constant_parse(struct lexer *lexer, const
>> struct expr_field *f,
>> return false;
>> }
>> - struct expr_context ctx = { .lexer = lexer };
>> + struct expr_context ctx = {
>> + .lexer = lexer,
>> + };
>> struct expr_constant_set cs;
>> memset(&cs, 0, sizeof cs);
>> @@ -1332,7 +1339,10 @@ expr_parse_primary(struct expr_context *ctx,
>> bool *atomic)
>> return e;
>> }
>> - if (ctx->lexer->token.type == LEX_T_ID) {
>> + if (ctx->lexer->token.type == LEX_T_TEMPLATE) {
>> + lexer_error(ctx->lexer, "Unexpanded template.");
>> + return NULL;
>> + } else if (ctx->lexer->token.type == LEX_T_ID) {
>> struct expr_field f;
>> enum expr_relop r;
>> struct expr_constant_set c;
>> diff --git a/lib/lex.c b/lib/lex.c
>> index c84d52aa8d..7b44bc3537 100644
>> --- a/lib/lex.c
>> +++ b/lib/lex.c
>> @@ -235,6 +235,10 @@ lex_token_format(const struct lex_token *token,
>> struct ds *s)
>> ds_put_format(s, "@%s", token->s);
>> break;
>> + case LEX_T_TEMPLATE:
>> + ds_put_format(s, "^%s", token->s);
>> + break;
>> +
>> case LEX_T_LPAREN:
>> ds_put_cstr(s, "(");
>> break;
>> @@ -588,6 +592,18 @@ lex_parse_port_group(const char *p, struct
>> lex_token *token)
>> return lex_parse_id(p, LEX_T_PORT_GROUP, token);
>> }
>> +static const char *
>> +lex_parse_template(const char *p, struct lex_token *token)
>> +{
>> + p++;
>> + if (!lex_is_id1(*p)) {
>> + lex_error(token, "`^' must be followed by a valid identifier.");
>> + return p;
>> + }
>> +
>> + return lex_parse_id(p, LEX_T_TEMPLATE, token);
>> +}
>> +
>> /* Initializes 'token' and parses the first token from the beginning of
>> * null-terminated string 'p' into 'token'. Stores a pointer to the
>> start of
>> * the token (after skipping white space and comments, if any) into
>> '*startp'.
>> @@ -766,6 +782,10 @@ next:
>> p = lex_parse_port_group(p, token);
>> break;
>> + case '^':
>> + p = lex_parse_template(p, token);
>> + break;
>> +
>> case ':':
>> if (p[1] != ':') {
>> token->type = LEX_T_COLON;
>> @@ -1031,3 +1051,40 @@ lexer_steal_error(struct lexer *lexer)
>> lexer->error = NULL;
>> return error;
>> }
>> +
>> +/* Takes ownership of 's' and expands all templates that are encountered
>> + * in the contents of 's', if possible. Adds the encountered
>> template names
>> + * to 'template_vars_ref'.
>> + */
>> +const char *
>> +lexer_parse_template_string(const char *s, const struct smap
>> *template_vars,
>> + struct sset *template_vars_ref,
>> + char **out_expanded)
>> +{
>> + /* No '^' means no templates. */
>> + if (!strchr(s, '^')) {
>> + *out_expanded = NULL;
>> + return s;
>> + }
>> +
>> + struct ds expanded = DS_EMPTY_INITIALIZER;
>> +
>> + struct lexer lexer;
>> + lexer_init(&lexer, s);
>> +
>> + while (lexer_get(&lexer) != LEX_T_END) {
>> + if (lexer.token.type == LEX_T_TEMPLATE) {
>> + ds_put_cstr(&expanded, smap_get_def(template_vars,
>> lexer.token.s,
>> + lexer.token.s));
>> + if (template_vars_ref) {
>> + sset_add(template_vars_ref, lexer.token.s);
>> + }
>> + } else {
>> + lex_token_format(&lexer.token, &expanded);
>> + }
>> + }
>> +
>> + lexer_destroy(&lexer);
>> + *out_expanded = ds_steal_cstr(&expanded);
>> + return *out_expanded;
>
> It seems odd to me that lexer_parse_template_string() sets the out
> parameter but also gives the same pointer as a return value. I could
> foresee a situation where someone makes the mistake of thinking these
> are two distinct values and causes chaos as a result (e.g. free the
> output parameter and then try to use the return value after). IMO, this
> should follow the same pattern that other parsers in OVN use. That is,
> it should use the output parameter for the desired outcome and return an
> error string in the case something unexpected occurs.
>
You're right, this isn't great. I'm not sure how to best implement this
however. I went with something a bit different in v3 (a new lex_str
type wrapping in-heap or on-stack allocated lexer strings). I guess we
should probably continue the discussion there.
>> +}
>> diff --git a/lib/objdep.c b/lib/objdep.c
>> index 092d4af261..06cf126f12 100644
>> --- a/lib/objdep.c
>> +++ b/lib/objdep.c
>> @@ -245,6 +245,7 @@ objdep_type_name(enum objdep_type type)
>> [OBJDEP_TYPE_PORTGROUP] = "Port_Group",
>> [OBJDEP_TYPE_PORTBINDING] = "Port_Binding",
>> [OBJDEP_TYPE_MC_GROUP] = "Multicast_Group",
>> + [OBJDEP_TYPE_TEMPLATE] = "Template",
>> };
>> ovs_assert(type < OBJDEP_TYPE_MAX);
>> diff --git a/lib/objdep.h b/lib/objdep.h
>> index 50c7b01ef1..1ea781947c 100644
>> --- a/lib/objdep.h
>> +++ b/lib/objdep.h
>> @@ -26,6 +26,7 @@ enum objdep_type {
>> OBJDEP_TYPE_PORTGROUP,
>> OBJDEP_TYPE_PORTBINDING,
>> OBJDEP_TYPE_MC_GROUP,
>> + OBJDEP_TYPE_TEMPLATE,
>> OBJDEP_TYPE_MAX,
>> };
>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
>> index 3c3fb31c7c..5fb6c65eba 100644
>> --- a/tests/ovn-controller.at
>> +++ b/tests/ovn-controller.at
>> @@ -2337,3 +2337,53 @@ done
>> AT_CHECK([grep "deleted interface patch" hv1/ovs-vswitchd.log], [1],
>> [ignore])
>> OVN_CLEANUP([hv1])
>> AT_CLEANUP
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([ovn-controller - Local Chassis_Template_Var updates])
>> +AT_KEYWORDS([templates])
>> +ovn_start
>> +
>> +net_add n1
>> +sim_add hv
>> +as hv
>> +ovs-vsctl \
>> + -- add-br br-phys \
>> + -- add-br br-eth0 \
>> + -- add-br br-eth1 \
>> + -- add-br br-eth2
>> +ovn_attach n1 br-phys 192.168.0.1
>> +
>> +m4_define([GET_LOCAL_TEMPLATE_VARS],
>> + [ovn-appctl debug/dump-local-template-vars | grep -v 'Local
>> template vars'])
>> +
>> +dnl Expect no local vars initially.
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], [])
>> +
>> +AT_CHECK([ovn-nbctl --wait=hv create Chassis_Template_Var
>> chassis="hv"], [0], [ignore])
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], [])
>> +
>> +dnl Expect new vars to be handled properly.
>> +check ovn-nbctl --wait=hv set Chassis_Template_Var hv
>> variables:foo=foo-hv variables:bar=bar-hv
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [0], [dnl
>> +name: 'bar' value: 'bar-hv'
>> +name: 'foo' value: 'foo-hv'
>> +])
>> +
>> +dnl Expect var updates to be handled properly.
>> +check ovn-nbctl --wait=hv set Chassis_Template_Var hv
>> variables:bar=bar-new-hv
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [0], [dnl
>> +name: 'bar' value: 'bar-new-hv'
>> +name: 'foo' value: 'foo-hv'
>> +])
>> +
>> +dnl Expect var deletions to be handled properly.
>> +check ovn-nbctl --wait=hv remove Chassis_Template_Var hv variables bar
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [0], [dnl
>> +name: 'foo' value: 'foo-hv'
>> +])
>> +
>> +check ovn-nbctl --wait=hv remove Chassis_Template_Var hv variables foo
>> +AT_CHECK([GET_LOCAL_TEMPLATE_VARS], [1], [])
>> +
>> +AT_CLEANUP
>> +])
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index f8b8db4df8..2a59235c59 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -259,7 +259,7 @@ fe:x => error("Invalid numeric constant.")
>> & => error("`&' is only valid as part of `&&'.")
>> | => error("`|' is only valid as part of `||'.")
>> -^ => error("Invalid character `^' in input.")
>> +^ => error("`^' must be followed by a valid identifier.")
>> ])
>> AT_CAPTURE_FILE([input.txt])
>> sed 's/ =>.*//' test-cases.txt > input.txt
>> @@ -32975,3 +32975,81 @@ check ovn-nbctl --wait=hv sync
>> OVN_CLEANUP([hv1])
>> AT_CLEANUP
>> ])
>> +
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([Logical flows with Chassis_Template_Var references])
>> +AT_KEYWORDS([templates])
>> +ovn_start
>> +net_add n1
>> +
>> +sim_add hv1
>> +as hv1
>> +ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +
>> +check ovn-nbctl ls-add sw
>> +
>> +dnl Use --wait=sb to ensure lsp1 getting a tunnel_key before lsp2.
>> +check ovn-nbctl --wait=sb lsp-add sw lsp1
>> +check ovn-nbctl --wait=sb lsp-add sw lsp2
>> +
>> +AT_CHECK([ovn-nbctl create Chassis_Template_Var chassis=hv1], [0],
>> [ignore])
>> +
>> +check ovn-nbctl pg-add pg1 lsp1 lsp2
>> +AT_CHECK([ovn-nbctl create address_set name=as1
>> addresses=\"1.1.1.1\",\"1.1.1.2\"], [0], [ignore])
>> +
>> +dnl Create an ACL that uses an "uninstantiated" template.
>> +check ovn-nbctl acl-add sw from-lport 1 "ip4.src == 42.42.42.42 &&
>> ^CONDITION" allow
>> +
>> +check ovs-vsctl add-port br-int p1 -- set interface p1
>> external_ids:iface-id=lsp1
>> +check ovs-vsctl add-port br-int p2 -- set interface p2
>> external_ids:iface-id=lsp2
>> +
>> +wait_for_ports_up
>> +ovn-nbctl --wait=hv sync
>> +
>> +dnl Ensure the ACL is not translated to OpenFlow.
>> +as hv1
>> +AT_CHECK([ovs-ofctl dump-flows br-int | grep '42\.42\.42\.42'], [1], [])
>> +
>> +dnl Create a Chassis_Template_Var mapping for CONDITION.
>> +check ovn-nbctl --wait=hv set Chassis_Template_Var hv1
>> variables:CONDITION='inport == @pg1'
>> +
>> +lsp1=0x$(fetch_column Port_Binding tunnel_key logical_port=lsp1)
>> +lsp2=0x$(fetch_column Port_Binding tunnel_key logical_port=lsp2)
>> +
>> +dnl Ensure the ACL is translated to OpenFlows expanding pg1.
>> +as hv1
>> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int | grep
>> '42\.42\.42\.42' | ofctl_strip_all], [0], [dnl
>> + table=16,
>> priority=1001,ip,reg14=$lsp1,metadata=0x1,nw_src=42.42.42.42
>> actions=resubmit(,17)
>> + table=16,
>> priority=1001,ip,reg14=$lsp2,metadata=0x1,nw_src=42.42.42.42
>> actions=resubmit(,17)
>> +])
>> +
>> +dnl Remove a port from pg1 and expect OpenFlows to be correctly updated.
>> +check ovn-nbctl --wait=hv pg-set-ports pg1 lsp2
>> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int | grep
>> '42\.42\.42\.42' | ofctl_strip_all], [0], [dnl
>> + table=16,
>> priority=1001,ip,reg14=$lsp2,metadata=0x1,nw_src=42.42.42.42
>> actions=resubmit(,17)
>> +])
>> +
>> +dnl Change the Chassis_Template_Var mapping to use the address set.
>> +check ovn-nbctl --wait=hv set Chassis_Template_Var hv1
>> variables:CONDITION='ip4.dst == $as1'
>> +
>> +dnl Ensure the ACL is translated to OpenFlows expanding as1.
>> +as hv1
>> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int | grep
>> '42\.42\.42\.42' | ofctl_strip_all], [0], [dnl
>> + table=16,
>> priority=1001,ip,metadata=0x1,nw_src=42.42.42.42,nw_dst=1.1.1.1
>> actions=resubmit(,17)
>> + table=16,
>> priority=1001,ip,metadata=0x1,nw_src=42.42.42.42,nw_dst=1.1.1.2
>> actions=resubmit(,17)
>> +])
>> +
>> +dnl Remove an IP from AS1 and expect OpenFlows to be correctly updated.
>> +check ovn-nbctl set address_set as1 addresses=\"1.1.1.1\"
>> +AT_CHECK_UNQUOTED([ovs-ofctl dump-flows br-int | grep
>> '42\.42\.42\.42' | ofctl_strip_all], [0], [dnl
>> + table=16,
>> priority=1001,ip,metadata=0x1,nw_src=42.42.42.42,nw_dst=1.1.1.1
>> actions=resubmit(,17)
>> +])
>> +
>> +dnl Remove the mapping and expect OpenFlows to be removed.
>> +check ovn-nbctl --wait=hv clear Chassis_Template_Var hv1 variables
>> +AT_CHECK([ovs-ofctl dump-flows br-int | grep '42\.42\.42\.42'], [1], [])
>> +
>> +OVN_CLEANUP([hv1])
>> +AT_CLEANUP
>> +])
>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>> index a241f150d1..c422bafd70 100644
>> --- a/tests/test-ovn.c
>> +++ b/tests/test-ovn.c
>> @@ -291,12 +291,14 @@ test_parse_expr__(int steps)
>> struct shash symtab;
>> struct shash addr_sets;
>> struct shash port_groups;
>> + struct smap template_vars;
>> struct simap ports;
>> struct ds input;
>> create_symtab(&symtab);
>> create_addr_sets(&addr_sets);
>> create_port_groups(&port_groups);
>> + smap_init(&template_vars);
>> simap_init(&ports);
>> simap_put(&ports, "eth0", 5);
>> @@ -355,6 +357,7 @@ test_parse_expr__(int steps)
>> shash_destroy(&addr_sets);
>> expr_const_sets_destroy(&port_groups);
>> shash_destroy(&port_groups);
>> + smap_destroy(&template_vars);
>> }
>> static void
>> @@ -913,8 +916,8 @@ test_tree_shape_exhaustively(struct expr *expr,
>> struct shash *symtab,
>> expr_format(expr, &s);
>> char *error;
>> - modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
>> - NULL, NULL, NULL, 0, &error);
>> + modified = expr_parse_string(ds_cstr(&s), symtab, NULL,
>> NULL, NULL,
>> + NULL, 0, &error);
>> if (error) {
>> fprintf(stderr, "%s fails to parse (%s)\n",
>> ds_cstr(&s), error);
>> @@ -1286,6 +1289,8 @@ test_parse_actions(struct ovs_cmdl_context *ctx
>> OVS_UNUSED)
>> struct ds input;
>> bool ok = true;
>> + struct smap template_vars = SMAP_INITIALIZER(&template_vars);
>> +
>> create_symtab(&symtab);
>> create_gen_opts(&dhcp_opts, &dhcpv6_opts, &nd_ra_opts,
>> &event_opts);
>> @@ -1322,7 +1327,11 @@ test_parse_actions(struct ovs_cmdl_context
>> *ctx OVS_UNUSED)
>> .n_tables = 24,
>> .cur_ltable = 10,
>> };
>> - error = ovnacts_parse_string(ds_cstr(&input), &pp, &ovnacts,
>> &prereqs);
>> + char *exp_input_expanded = NULL;
>> + const char *exp_input =
>> + lexer_parse_template_string(ds_cstr(&input),
>> &template_vars, NULL,
>> + &exp_input_expanded);
>> + error = ovnacts_parse_string(exp_input, &pp, &ovnacts,
>> &prereqs);
>> if (!error) {
>> /* Convert the parsed representation back to a string
>> and print it,
>> * if it's different from the input. */
>> @@ -1409,6 +1418,7 @@ test_parse_actions(struct ovs_cmdl_context *ctx
>> OVS_UNUSED)
>> expr_destroy(prereqs);
>> ovnacts_free(ovnacts.data, ovnacts.size);
>> ofpbuf_uninit(&ovnacts);
>> + free(exp_input_expanded);
>> }
>> ds_destroy(&input);
>> @@ -1419,6 +1429,7 @@ test_parse_actions(struct ovs_cmdl_context
>> *ctx OVS_UNUSED)
>> dhcp_opts_destroy(&dhcpv6_opts);
>> nd_ra_opts_destroy(&nd_ra_opts);
>> controller_event_opts_destroy(&event_opts);
>> + smap_destroy(&template_vars);
>> ovn_extend_table_destroy(&group_table);
>> ovn_extend_table_destroy(&meter_table);
>> exit(ok ? EXIT_SUCCESS : EXIT_FAILURE);
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index 6fa5137d95..b837884ccc 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -505,6 +505,7 @@ static struct hmap dhcp_opts; /* Contains
>> "struct gen_opts_map"s. */
>> static struct hmap dhcpv6_opts; /* Contains "struct gen_opts_map"s. */
>> static struct hmap nd_ra_opts; /* Contains "struct gen_opts_map"s. */
>> static struct controller_event_options event_opts;
>> +static struct smap template_vars;
>> static struct ovntrace_datapath *
>> ovntrace_datapath_find_by_sb_uuid(const struct uuid *sb_uuid)
>> @@ -955,9 +956,15 @@ parse_lflow_for_datapath(const struct
>> sbrec_logical_flow *sblf,
>> char *error;
>> struct expr *match;
>> - match = expr_parse_string(sblf->match, &symtab, &address_sets,
>> + char *match_expanded_s = NULL;
>> + const char *match_s = lexer_parse_template_string(sblf->match,
>> +
>> &template_vars,
>> + NULL,
>> +
>> &match_expanded_s);
>> + match = expr_parse_string(match_s, &symtab, &address_sets,
>> &port_groups, NULL, NULL,
>> dp->tunnel_key,
>> &error);
>> + free(match_expanded_s);
>> if (error) {
>> VLOG_WARN("%s: parsing expression failed (%s)",
>> sblf->match, error);
>> @@ -980,7 +987,12 @@ parse_lflow_for_datapath(const struct
>> sbrec_logical_flow *sblf,
>> uint64_t stub[1024 / 8];
>> struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(stub);
>> struct expr *prereqs;
>> - error = ovnacts_parse_string(sblf->actions, &pp, &ovnacts,
>> &prereqs);
>> + char *actions_expanded_s = NULL;
>> + const char *actions_s =
>> + lexer_parse_template_string(sblf->actions, &template_vars,
>> + NULL, &actions_expanded_s);
>> + error = ovnacts_parse_string(actions_s, &pp, &ovnacts,
>> &prereqs);
>> + free(actions_expanded_s);
>> if (error) {
>> VLOG_WARN("%s: parsing actions failed (%s)",
>> sblf->actions, error);
>> free(error);
>> @@ -1078,6 +1090,7 @@ read_gen_opts(void)
>> nd_ra_opts_init(&nd_ra_opts);
>> controller_event_opts_init(&event_opts);
>> + smap_init(&template_vars);
>> }
>> static void
>> @@ -3420,9 +3433,15 @@ trace_parse(const char *dp_s, const char *flow_s,
>> *
>> * First make sure that the expression parses. */
>> char *error;
>> - struct expr *e = expr_parse_string(flow_s, &symtab,
>> &address_sets,
>> + char *flow_expanded_s = NULL;
>> + const char *flow_exp_s = lexer_parse_template_string(flow_s,
>> +
>> &template_vars,
>> + NULL,
>> +
>> &flow_expanded_s);
>> + struct expr *e = expr_parse_string(flow_exp_s, &symtab,
>> &address_sets,
>> &port_groups, NULL, NULL, 0,
>> &error);
>> + free(flow_expanded_s);
>> if (!e) {
>> return trace_parse_error(error);
>> }
>> @@ -3447,9 +3466,15 @@ trace_parse(const char *dp_s, const char *flow_s,
>> free(port_name);
>> }
>> - char *error = expr_parse_microflow(flow_s, &symtab, &address_sets,
>> + char *flow_expanded_s = NULL;
>> + const char *flow_exp_s = lexer_parse_template_string(flow_s,
>> + &template_vars,
>> + NULL,
>> +
>> &flow_expanded_s);
>> + char *error = expr_parse_microflow(flow_exp_s, &symtab,
>> &address_sets,
>> &port_groups,
>> ovntrace_lookup_port,
>> *dpp, uflow);
>> + free(flow_expanded_s);
>> if (error) {
>> return trace_parse_error(error);
>> }
>>
>
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
