On 10/21/22 08:00, Han Zhou wrote:
> On Fri, Oct 7, 2022 at 1:26 AM Dumitru Ceara <[email protected]> wrote:
>>
>> On 10/7/22 04:03, Han Zhou wrote:
>>> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <[email protected]> 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 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]>
>>>
>>> Hi Dumitru,
>>>
>>
>> Hi Han,
>>
>>> In addition to the two-stage parsing concerns we are discussing in the
>>
>> I'm doing some more targetted performance testing. I'll update the
>> other thread when I have more data to share.
>>
>>> other thread, I have some minor comments below. The major one is
> whether we
>>> should allow matching hostname or not.
>>>
>>>> ---
>>>> controller/lflow.c | 59 +++++++-
>>>> controller/lflow.h | 1
>>>> controller/lport.c | 3
>>>> controller/ofctrl.c | 9 +
>>>> controller/ofctrl.h | 3
>>>> controller/ovn-controller.c | 317
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>> include/ovn/expr.h | 4 -
>>>> include/ovn/lex.h | 14 +-
>>>> lib/actions.c | 9 +
>>>> lib/expr.c | 18 ++
>>>> lib/lex.c | 55 +++++++
>>>> lib/objdep.c | 1
>>>> lib/objdep.h | 1
>>>> lib/ovn-util.c | 7 +
>>>> lib/ovn-util.h | 3
>>>> tests/ovn.at | 2
>>>> tests/test-ovn.c | 16 ++
>>>> utilities/ovn-trace.c | 26 +++-
>>>> 18 files changed, 512 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/controller/lflow.c b/controller/lflow.c
>>>> index f9f2e6286..ad58fcd3c 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 *,
>>>> @@ -356,10 +358,15 @@ consider_lflow_for_added_as_ips__(
>>>> .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);
>>>> + char *actions_s =
>>> lexer_parse_template_string(xstrdup(lflow->actions),
>>>
>>> It may be better to xstrdup inside the function when necessary, and use
>>> const char* as parameter.
>>>
>>
>> Definitely, I'll change it in v2.
>>
>>>> +
>>> l_ctx_in->template_vars,
>>>> + &template_vars_ref);
>>>> + error = ovnacts_parse_string(actions_s, &pp, &ovnacts, &prereqs);
>>>
>>> The two functions can be wrapped to a single function, because they
> will be
>>> called together again in consider_logical_flow__().
>>>
>>
>> I can do that. I didn't do it initially because there's already quite
>> some overlap between consider_lflow_for_added_as_ips__() and
>> consider_logical_flow__(). But that's something for a different patch I
>> guess.
>>
>>>> + free(actions_s);
>>>> if (error) {
>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>>> VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
>>>> @@ -367,6 +374,7 @@ consider_lflow_for_added_as_ips__(
>>>> free(error);
>>>> ovnacts_free(ovnacts.data, ovnacts.size);
>>>> ofpbuf_uninit(&ovnacts);
>>>> + sset_destroy(&template_vars_ref);
>>>> return true;
>>>> }
>>>>
>>>> @@ -430,6 +438,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) {
>>>> @@ -501,6 +511,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);
>>>> + }
>>>> + sset_destroy(&template_vars_ref);
>>>> +
>>>> return handled;
>>>> }
>>>>
>>>> @@ -911,6 +929,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)
>>>> {
>>>> @@ -918,11 +938,16 @@ 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_s = lexer_parse_template_string(xstrdup(lflow->match),
>>>> + template_vars,
>>>> + template_vars_ref);
>>>> + 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_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,
>>>> @@ -1027,7 +1052,12 @@ consider_logical_flow__(const struct
>>> sbrec_logical_flow *lflow,
>>>> struct expr *prereqs = NULL;
>>>> char *error;
>>>>
>>>> - error = ovnacts_parse_string(lflow->actions, &pp, &ovnacts,
>>> &prereqs);
>>>> + struct sset template_vars_ref =
> SSET_INITIALIZER(&template_vars_ref);
>>>> + char *actions_s =
>>> lexer_parse_template_string(xstrdup(lflow->actions),
>>>> +
>>> l_ctx_in->template_vars,
>>>> + &template_vars_ref);
>>>> + error = ovnacts_parse_string(actions_s, &pp, &ovnacts, &prereqs);
>>>> + free(actions_s);
>>>> if (error) {
>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>>> VLOG_WARN_RL(&rl, "error parsing actions \"%s\": %s",
>>>> @@ -1035,6 +1065,7 @@ consider_logical_flow__(const struct
>>> sbrec_logical_flow *lflow,
>>>> free(error);
>>>> ovnacts_free(ovnacts.data, ovnacts.size);
>>>> ofpbuf_uninit(&ovnacts);
>>>> + sset_destroy(&template_vars_ref);
>>>> return;
>>>> }
>>>>
>>>> @@ -1085,6 +1116,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) {
>>>> @@ -1099,11 +1132,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);
>>>> }
>>>>
>>>> @@ -1188,6 +1223,13 @@ done:
>>>> expr_destroy(cached_expr);
>>>> expr_matches_destroy(matches);
>>>> free(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);
>>>> + }
>>>> + sset_destroy(&template_vars_ref);
>>>> }
>>>>
>>>> static void
>>>> @@ -1951,8 +1993,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,
>>>> struct ovn_desired_flow_table *flow_table,
>>>> struct simap *ids)
>>>> {
>>>> @@ -2041,8 +2082,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);
>>>> }
>>>> }
>>>>
>>>> diff --git a/controller/lflow.h b/controller/lflow.h
>>>> index a9a0f7506..8175bdbea 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/lport.c b/controller/lport.c
>>>> index add7e91aa..f12ce4e63 100644
>>>> --- a/controller/lport.c
>>>> +++ b/controller/lport.c
>>>> @@ -134,8 +134,7 @@ lport_can_bind_on_this_chassis(const struct
>>> sbrec_chassis *chassis_rec,
>>>> enum can_bind can_bind = CAN_BIND_AS_MAIN;
>>>> for (chassis = strtok_r(tokstr, ",", &save_ptr); chassis != NULL;
>>>> chassis = strtok_r(NULL, ",", &save_ptr)) {
>>>> - if (!strcmp(chassis, chassis_rec->name)
>>>> - || !strcmp(chassis, chassis_rec->hostname)) {
>>>> + if (chassis_name_equals(chassis, chassis_rec)) {
>>>> free(tokstr);
>>>> return can_bind;
>>>> }
>>>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>>>> index c77991258..bf161dfde 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,13 @@ 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_exp_s = lexer_parse_template_string(xstrdup(flow_s),
>>>> + template_vars,
>>>> + NULL);
>>>> + char *error = expr_parse_microflow(flow_exp_s, &symtab, addr_sets,
>>>> port_groups,
> ofctrl_lookup_port,
>>>> br_int, &uflow);
>>>> + free(flow_exp_s);
>>>> if (error) {
>>>> return error;
>>>> }
>>>> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
>>>> index 71d3f5838..396824512 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 32a24a076..40afebf4e 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,11 @@ 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_template_var_add_clause_chassis_name(&tv, OVSDB_F_EQ,
>>>> + chassis->name);
>>>> + sbrec_template_var_add_clause_chassis_name(&tv, OVSDB_F_EQ,
>>>> + chassis->hostname);
>>>> } else {
>>>> /* During initialization, we monitor all records in
>>> Chassis_Private so
>>>> * that we don't try to recreate existing ones. */
>>>> @@ -290,6 +300,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_template_var_set_condition(ovnsb_idl, &tv),
>>>> };
>>>>
>>>> unsigned int expected_cond_seqno = 0;
>>>> @@ -307,6 +318,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;
>>>> }
>>>>
>>>> @@ -990,7 +1002,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(template_var, "template_var")
>>>>
>>>> enum sb_engine_node {
>>>> #define SB_NODE(NAME, NAME_STR) SB_##NAME,
>>>> @@ -1652,6 +1665,214 @@ runtime_data_sb_datapath_binding_handler(struct
>>> engine_node *node OVS_UNUSED,
>>>> return true;
>>>> }
>>>>
>>>> +struct local_templates {
>>>> + struct uuidset uuids;
>>>> + struct smap vars;
>>>> +};
>>>> +
>>>> +static void
>>>> +local_templates_init(struct local_templates *table)
>>>> +{
>>>> + uuidset_init(&table->uuids);
>>>> + smap_init(&table->vars);
>>>> +}
>>>> +
>>>> +static void
>>>> +local_templates_clear(struct local_templates *table)
>>>> +{
>>>> + uuidset_clear(&table->uuids);
>>>> + smap_clear(&table->vars);
>>>> +}
>>>> +
>>>> +static void
>>>> +local_templates_destroy(struct local_templates *table)
>>>> +{
>>>> + uuidset_destroy(&table->uuids);
>>>> + smap_destroy(&table->vars);
>>>> +}
>>>> +
>>>> +static void
>>>> +local_templates_insert(struct local_templates *table,
>>>> + const char *name, const struct uuid *uuid,
>>>> + const char *value)
>>>> +{
>>>> + uuidset_insert(&table->uuids, uuid);
>>>> + smap_replace(&table->vars, name, value);
>>>> +}
>>>> +
>>>> +static void
>>>> +local_templates_remove(struct local_templates *table,
>>>> + const char *name, const struct uuid *uuid)
>>>> +{
>>>> + uuidset_find_and_delete(&table->uuids, uuid);
>>>> + smap_remove(&table->vars, name);
>>>> +}
>>>> +
>>>> +static bool
>>>> +local_templates_contains_uuid(const struct local_templates *table,
>>>> + const struct uuid *uuid)
>>>> +{
>>>> + return !!uuidset_find(CONST_CAST(struct uuidset *, &table->uuids),
>>> uuid);
>>>> +}
>>>> +
>>>> +static void
>>>> +local_templates_to_string(const struct local_templates *table,
>>>> + struct ds *out_data)
>>>> +{
>>>> + const struct smap_node *node;
>>>> +
>>>> + SMAP_FOR_EACH (node, &table->vars) {
>>>> + ds_put_format(out_data, "name: '%s' value: '%s'\n",
>>>> + node->key, node->value);
>>>> + }
>>>> +}
>>>> +
>>>> +struct ed_type_template_vars {
>>>> + struct local_templates var_table;
>>>> +
>>>> + bool change_tracked;
>>>> + struct sset new;
>>>> + struct sset deleted;
>>>> + struct sset updated;
>>>> +};
>>>> +
>>>> +static void
>>>> +template_vars_init(const struct sbrec_template_var_table *tv_table,
>>>> + const struct sbrec_chassis *chassis,
>>>> + struct local_templates *var_table)
>>>> +{
>>>> + const struct sbrec_template_var *tv;
>>>> + SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
>>>> + if (chassis_name_equals(tv->chassis_name, chassis)) {
>>>
>>> I am not sure if it is a good idea to allow using hostname to match the
>>> template var name. It provides flexibility to CMS, but we will need more
>>> complexity to protect against corner cases.
>>> For example, if there are two records:
>>> r1: name="abc", value="v1"
>>> r2: hostname="abc", value="v2"
>>>
>>> Now with the current logic, whichever is handled later will take
> precedence
>>> (in the local_templates.vars) and the value will be used (assume r2
> "v2" is
>>> used). This may be fine, because the user should be responsible for the
>>> inconsistent configurations.
>>>
>>> Later, when the user detects the problem and wants to correct the
>>> configuration. He/she deletes the r2 and expects the var "abc" to be
>>> expanded as "v1". But the logic in template_vars_update() would call
>>> local_templates_remove() which simply deletes the var ("abc" -> "v2")
>>> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" -> "v1"
>>> will still be left in the uuidset, which is useless. This is an
> unexpected
>>> behavior.
>>>
>>> Similar behavior would happen if there are duplicate hostnames, e.g.:
>>> r1: hostname="abc", value="v1"
>>> r2: hostname="abc", value="v2"
>>>
>>
>> Very good point, nice catch!
>>
>>> So, if we really want to support the flexibility, we will need to track
> the
>>> duplicated keys, and handle the delete/replace properly, which I feel
> is a
>>> little bit overkill for this. I think CMS should be able to always use
>>> chassis_name. This would simplify the code because chassis_name is part
> of
>>> the table key and no need to check redundancy.
>>>
>>> In addition, in either case, I don't see a benefit of maintaining the
>>> uuidset. Did I miss anything?
>>>
>>
>> The uuidset is a left over from my try to make template variables use a
>> generic predicate. I was using it exactly to determine duplicates but
>> I'll remove it in v2.
>>
>> I think we should stick to chassis->name as you suggested. That will
>> make the code less complex and avoid ambiguities like you described above.
>>
>>>> + local_templates_insert(var_table, tv->name,
>>>> + &tv->header_.uuid, tv->value);
>>>
>>> nit: indent alignment problem.
>>>
>>
>> Ack.
>>
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +static void
>>>> +template_vars_update(const struct sbrec_template_var_table *tv_table,
>>>> + const struct sbrec_chassis *chassis,
>>>> + struct local_templates *var_table,
>>>> + struct sset *new, struct sset *deleted,
>>>> + struct sset *updated)
>>>> +{
>>>> + const struct sbrec_template_var *tv;
>>>> + SBREC_TEMPLATE_VAR_TABLE_FOR_EACH_TRACKED (tv, tv_table) {
>>>> + if (!sbrec_template_var_is_deleted(tv)) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + if (!local_templates_contains_uuid(var_table,
>>> &tv->header_.uuid)) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + local_templates_remove(var_table, tv->name,
> &tv->header_.uuid);
>>>> + sset_add(deleted, tv->name);
>>>> + }
>>>> +
>>>> + SBREC_TEMPLATE_VAR_TABLE_FOR_EACH_TRACKED (tv, tv_table) {
>>>> + if (sbrec_template_var_is_deleted(tv)) {
>>>> + continue;
>>>> + }
>>>> +
>>>> + struct sset *tv_set = sbrec_template_var_is_new(tv)
>>>> + ? new : updated;
>>>> + if (chassis_name_equals(tv->chassis_name, chassis)) {
>>>> + local_templates_insert(var_table, tv->name,
>>>> + &tv->header_.uuid, tv->value);
>>>
>>> nit: indent alignment problem.
>>>
>>
>> Ack.
>>
>>> Thanks,
>>> Han
>>>
>>
>> Thanks for the review!
>>
>> Regards,
>> Dumitru
>>
>>>> + sset_add(tv_set, tv->name);
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +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);
>>>> + local_templates_init(&tv_data->var_table);
>>>> + 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 sbrec_template_var_table *tv_table =
>>>> + EN_OVSDB_GET(engine_get_input("SB_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));
>>>> +
>>>> + local_templates_clear(&tv_data->var_table);
>>>> +
>>>> + template_vars_init(tv_table, chassis, &tv_data->var_table);
>>>> + engine_set_node_state(node, EN_UPDATED);
>>>> +}
>>>> +
>>>> +static bool
>>>> +template_vars_sb_template_var_handler(struct engine_node *node, void
>>> *data)
>>>> +{
>>>> + struct ed_type_template_vars *tv_data = data;
>>>> +
>>>> + const struct sbrec_template_var_table *tv_table =
>>>> + EN_OVSDB_GET(engine_get_input("SB_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->var_table,
>>>> + &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;
>>>> + local_templates_destroy(&tv_data->var_table);
>>>> +
>>>> + 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;
>>>> @@ -2706,6 +2927,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 =
>>>> @@ -2738,6 +2962,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->var_table.vars;
>>>>
>>>> l_ctx_out->flow_table = &fo->flow_table;
>>>> l_ctx_out->group_table = &fo->group_table;
>>>> @@ -3064,6 +3289,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)
>>>> @@ -3711,6 +3994,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");
>>>> @@ -3737,6 +4021,11 @@ 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,
>>>> + engine_noop_handler);
>>>> + engine_add_input(&en_template_vars, &en_sb_chassis,
>>> engine_noop_handler);
>
> Hi Dumitru, sorry that I forgot to add one more comment here. For the above
> two noop handler usages, I think they should use NULL - to trigger
> recompute. I don't see any obvious reason of using noop handler here.
> noop handler is used only if we know there is dependency for an input but
> for some reason we can safely ignore the changes of that input. If that's
> the case, let's add some documentation to clarify. If it is because we know
> that these changes will trigger recompute at other nodes, it shouldn't be a
> reason to use noop handler, and we should simply use NULL.
>
I was thinking that the chassis name can't really change so we could
ignore SB chassis changes all toghether. We only need access to the
nodes' Open_vSwitch table and chassis_by_name index so that we lookup
the determine the local chassis name in the en_template_vars run and
change handlers.
But, just in case, it's probably better to set the handler to NULL and
trigger full recomputes when any chassis changes. I'll do that.
> Thanks,
> Han
>
Thanks,
Dumitru
>>>> + engine_add_input(&en_template_vars, &en_sb_template_var,
>>>> + template_vars_sb_template_var_handler);
>>>>
>>>> engine_add_input(&en_addr_sets, &en_sb_address_set,
>>>> addr_sets_sb_address_set_handler);
>>>> @@ -3804,6 +4093,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,
>>>> @@ -3921,6 +4212,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,
>>>> @@ -3982,6 +4275,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->var_table);
>>>> +
>>>> unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>>>> debug_ignore_startup_delay, NULL);
>>>>
>>>> @@ -4358,9 +4655,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->var_table.vars);
>>>> if (error) {
>>>> unixctl_command_reply_error(pending_pkt.conn,
>>> error);
>>>> free(error);
>>>> @@ -4829,6 +5129,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 3b141b034..80d95ff67 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 ecb7ace24..38c4aa9f3 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,7 @@ void lexer_syntax_error(struct lexer *, const char
>>> *message, ...)
>>>>
>>>> char *lexer_steal_error(struct lexer *);
>>>>
>>>> +char *lexer_parse_template_string(char *s, const struct smap
>>> *template_vars,
>>>> + struct sset *template_vars_ref);
>>>> +
>>>> #endif /* ovn/lex.h */
>>>> diff --git a/lib/actions.c b/lib/actions.c
>>>> index adbb42db4..d7d486883 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 d1f9d28ca..43f44771c 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 c84d52aa8..b01a7d75c 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,38 @@ 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'.
>>>> + */
>>>> +char *
>>>> +lexer_parse_template_string(char *s, const struct smap *template_vars,
>>>> + struct sset *template_vars_ref)
>>>> +{
>>>> + /* No '^' means no templates. */
>>>> + if (!strchr(s, '^')) {
>>>> + 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);
>>>> + free(s);
>>>> + return ds_steal_cstr(&expanded);
>>>> +}
>>>> \ No newline at end of file
>>>> diff --git a/lib/objdep.c b/lib/objdep.c
>>>> index 0224a5d06..0e62b88a9 100644
>>>> --- a/lib/objdep.c
>>>> +++ b/lib/objdep.c
>>>> @@ -246,6 +246,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 059a11a62..b96c98163 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/lib/ovn-util.c b/lib/ovn-util.c
>>>> index d80db179a..596e93129 100644
>>>> --- a/lib/ovn-util.c
>>>> +++ b/lib/ovn-util.c
>>>> @@ -615,6 +615,13 @@ ovn_logical_flow_hash_datapath(const struct uuid
>>> *logical_datapath,
>>>> return hash_add(hash, uuid_hash(logical_datapath));
>>>> }
>>>>
>>>> +bool
>>>> +chassis_name_equals(const char *chassis_name,
>>>> + const struct sbrec_chassis *target)
>>>> +{
>>>> + return !strcmp(chassis_name, target->name) ||
>>>> + !strcmp(chassis_name, target->hostname);
>>>> +}
>>>>
>>>> struct tnlid_node {
>>>> struct hmap_node hmap_node;
>>>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>>>> index 145f974ed..c53d5a19a 100644
>>>> --- a/lib/ovn-util.h
>>>> +++ b/lib/ovn-util.h
>>>> @@ -29,6 +29,7 @@
>>>> #define ROUTE_ORIGIN_STATIC "static"
>>>>
>>>> struct nbrec_logical_router_port;
>>>> +struct sbrec_chassis;
>>>> struct sbrec_logical_flow;
>>>> struct svec;
>>>> struct uuid;
>>>> @@ -119,6 +120,8 @@ uint32_t ovn_logical_flow_hash(uint8_t table_id,
> enum
>>> ovn_pipeline pipeline,
>>>> const char *match, const char
> *actions);
>>>> uint32_t ovn_logical_flow_hash_datapath(const struct uuid
>>> *logical_datapath,
>>>> uint32_t hash);
>>>> +bool chassis_name_equals(const char *chassis_name,
>>>> + const struct sbrec_chassis *target);
>>>> void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>>> const char *argv[] OVS_UNUSED, void *idl_);
>>>>
>>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>>> index 0dc23dbd2..ee9c7f55f 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
>>>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>>>> index a241f150d..7842cd915 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,10 @@ 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 =
>>> lexer_parse_template_string(xstrdup(ds_cstr(&input)),
>>>> + &template_vars,
>>>> + NULL);
>>>> + 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 +1417,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);
>>>> }
>>>> ds_destroy(&input);
>>>>
>>>> @@ -1419,6 +1428,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 d9e7129d9..112dec2de 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,13 @@ 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_s =
> lexer_parse_template_string(xstrdup(sblf->match),
>>>> + &template_vars,
>>>> + NULL);
>>>> + match = expr_parse_string(match_s, &symtab, &address_sets,
>>>> &port_groups, NULL, NULL,
>>> dp->tunnel_key,
>>>> &error);
>>>> + free(match_s);
>>>> if (error) {
>>>> VLOG_WARN("%s: parsing expression failed (%s)",
>>>> sblf->match, error);
>>>> @@ -980,7 +985,11 @@ 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_s =
>>> lexer_parse_template_string(xstrdup(sblf->actions),
>>>> + &template_vars,
>>>> + NULL);
>>>> + error = ovnacts_parse_string(actions_s, &pp, &ovnacts,
> &prereqs);
>>>> + free(actions_s);
>>>> if (error) {
>>>> VLOG_WARN("%s: parsing actions failed (%s)",
> sblf->actions,
>>> error);
>>>> free(error);
>>>> @@ -1078,6 +1087,7 @@ read_gen_opts(void)
>>>> nd_ra_opts_init(&nd_ra_opts);
>>>>
>>>> controller_event_opts_init(&event_opts);
>>>> + smap_init(&template_vars);
>>>> }
>>>>
>>>> static void
>>>> @@ -3428,9 +3438,13 @@ 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_exp_s =
> lexer_parse_template_string(xstrdup(flow_s),
>>>> + &template_vars,
>>>> + NULL);
>>>> + struct expr *e = expr_parse_string(flow_exp_s, &symtab,
>>> &address_sets,
>>>> &port_groups, NULL, NULL,
> 0,
>>>> &error);
>>>> + free(flow_exp_s);
>>>> if (!e) {
>>>> return trace_parse_error(error);
>>>> }
>>>> @@ -3455,9 +3469,13 @@ 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_exp_s = lexer_parse_template_string(xstrdup(flow_s),
>>>> + &template_vars,
>>>> + NULL);
>>>> + char *error = expr_parse_microflow(flow_exp_s, &symtab,
>>> &address_sets,
>>>> &port_groups,
>>> ovntrace_lookup_port,
>>>> *dpp, uflow);
>>>> + free(flow_exp_s);
>>>> if (error) {
>>>> return trace_parse_error(error);
>>>> }
>>>>
>>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
