On Tue, Oct 4, 2022 at 10:27 PM Han Zhou <[email protected]> wrote:
>
>
>
> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara <[email protected]> wrote:
> >
> > Propagate the contents of the NB table to the Southbound.
> >
> > Signed-off-by: Dumitru Ceara <[email protected]>
> > ---
> > Note:
> > - ovn-trace doesn't support template variables (yet).
> > ---
> > northd/automake.mk | 4 ++
> > northd/en-northd.c | 4 ++
> > northd/inc-proc-northd.c | 8 ++++-
> > northd/northd.c | 41 ++++++++++++++++++++++++
> > northd/northd.h | 2 +
> > northd/template-var.c | 77
++++++++++++++++++++++++++++++++++++++++++++++
> > northd/template-var.h | 59 +++++++++++++++++++++++++++++++++++
> > ovn-nb.ovsschema | 16 ++++++++--
> > ovn-nb.xml | 34 ++++++++++++++++++++
> > ovn-sb.ovsschema | 11 +++++--
> > ovn-sb.xml | 20 ++++++++++++
> > tests/ovn-northd.at | 32 +++++++++++++++++++
> > 12 files changed, 300 insertions(+), 8 deletions(-)
> > create mode 100644 northd/template-var.c
> > create mode 100644 northd/template-var.h
> >
> > diff --git a/northd/automake.mk b/northd/automake.mk
> > index 81582867d..31134bc32 100644
> > --- a/northd/automake.mk
> > +++ b/northd/automake.mk
> > @@ -13,7 +13,9 @@ northd_ovn_northd_SOURCES = \
> > northd/inc-proc-northd.c \
> > northd/inc-proc-northd.h \
> > northd/ipam.c \
> > - northd/ipam.h
> > + northd/ipam.h \
> > + northd/template-var.c \
> > + northd/template-var.h
> > northd_ovn_northd_LDADD = \
> > lib/libovn.la \
> > $(OVSDB_LIBDIR)/libovsdb.la \
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 7fe83db64..3a4828ec3 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -80,6 +80,8 @@ void en_northd_run(struct engine_node *node, void
*data)
> > EN_OVSDB_GET(engine_get_input("NB_acl", node));
> > input_data.nbrec_static_mac_binding_table =
> > EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
> > + input_data.nbrec_template_var_table =
> > + EN_OVSDB_GET(engine_get_input("NB_template_var", node));
> >
> > input_data.sbrec_sb_global_table =
> > EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> > @@ -113,6 +115,8 @@ void en_northd_run(struct engine_node *node, void
*data)
> > EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
> > input_data.sbrec_static_mac_binding_table =
> > EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
> > + input_data.sbrec_template_var_table =
> > + EN_OVSDB_GET(engine_get_input("SB_template_var", node));
> >
> > northd_run(&input_data, data,
> > eng_ctx->ovnnb_idl_txn,
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 54e0ad3b0..e301f6714 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -64,7 +64,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> > NB_NODE(ha_chassis_group, "ha_chassis_group") \
> > NB_NODE(ha_chassis, "ha_chassis") \
> > NB_NODE(bfd, "bfd") \
> > - NB_NODE(static_mac_binding, "static_mac_binding")
> > + NB_NODE(static_mac_binding, "static_mac_binding") \
> > + NB_NODE(template_var, "template_var")
> >
> > enum nb_engine_node {
> > #define NB_NODE(NAME, NAME_STR) NB_##NAME,
> > @@ -114,7 +115,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> > SB_NODE(load_balancer, "load_balancer") \
> > SB_NODE(bfd, "bfd") \
> > SB_NODE(fdb, "fdb") \
> > - 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,
> > @@ -186,6 +188,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_add_input(&en_northd, &en_nb_ha_chassis_group, NULL);
> > engine_add_input(&en_northd, &en_nb_ha_chassis, NULL);
> > engine_add_input(&en_northd, &en_nb_static_mac_binding, NULL);
> > + engine_add_input(&en_northd, &en_nb_template_var, NULL);
> >
> > engine_add_input(&en_northd, &en_sb_sb_global, NULL);
> > engine_add_input(&en_northd, &en_sb_chassis, NULL);
> > @@ -215,6 +218,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_add_input(&en_northd, &en_sb_load_balancer, NULL);
> > engine_add_input(&en_northd, &en_sb_fdb, NULL);
> > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> > + engine_add_input(&en_northd, &en_sb_template_var, NULL);
> > engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> > engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> > engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 8500d069d..63879ccd0 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -51,6 +51,7 @@
> > #include "lib/stopwatch-names.h"
> > #include "stream.h"
> > #include "timeval.h"
> > +#include "template-var.h"
> > #include "util.h"
> > #include "uuid.h"
> > #include "ovs-thread.h"
> > @@ -15113,6 +15114,44 @@ sync_dns_entries(struct northd_input
*input_data,
> > }
> > hmap_destroy(&dns_map);
> > }
> > +
> > +static void
> > +sync_template_vars(struct northd_input *input_data,
> > + struct ovsdb_idl_txn *ovnsb_txn)
> > +{
> > + struct template_var_table nb_tvs =
TEMPLATE_VAR_TABLE_INITIALIZER(&nb_tvs);
Sorry, I forgot to mention one more minor problem:
My gcc complains:
../northd/northd.c:15122:40: error: missing braces around initializer
> > +
> > + const struct nbrec_template_var *nb_tv;
> > + const struct sbrec_template_var *sb_tv;
> > + struct template_var *tv;
> > +
> > + NBREC_TEMPLATE_VAR_TABLE_FOR_EACH (
> > + nb_tv, input_data->nbrec_template_var_table) {
> > + template_var_insert(&nb_tvs, nb_tv);
> > + }
> > +
> > + SBREC_TEMPLATE_VAR_TABLE_FOR_EACH_SAFE (
> > + sb_tv, input_data->sbrec_template_var_table) {
> > + tv = template_var_find(&nb_tvs, sb_tv->name,
sb_tv->chassis_name);
> > + if (!tv) {
> > + sbrec_template_var_delete(sb_tv);
> > + continue;
> > + }
> > + if (strcmp(tv->nb->value, sb_tv->value)) {
> > + sbrec_template_var_set_value(sb_tv, tv->nb->value);
> > + }
> > + template_var_remove(&nb_tvs, tv);
> > + template_var_destroy(tv);
> > + }
> > +
> > + TEMPLATE_VAR_TABLE_FOR_EACH (tv, &nb_tvs) {
> > + sb_tv = sbrec_template_var_insert(ovnsb_txn);
> > + sbrec_template_var_set_name(sb_tv, tv->nb->name);
> > + sbrec_template_var_set_chassis_name(sb_tv,
tv->nb->chassis_name);
> > + sbrec_template_var_set_value(sb_tv, tv->nb->value);
> > + }
> > + template_var_table_destroy(&nb_tvs);
> > +}
> >
> > static void
> > destroy_datapaths_and_ports(struct hmap *datapaths, struct hmap *ports,
> > @@ -15629,6 +15668,8 @@ ovnnb_db_run(struct northd_input *input_data,
> > sync_port_groups(input_data, ovnsb_txn, &data->port_groups);
> > sync_meters(input_data, ovnsb_txn, &data->meter_groups);
> > sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
> > + sync_template_vars(input_data, ovnsb_txn);
> > +
> > cleanup_stale_fdb_entries(input_data, &data->datapaths);
> > stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> >
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 60601803f..254688afa 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -36,6 +36,7 @@ struct northd_input {
> > const struct nbrec_acl_table *nbrec_acl_table;
> > const struct nbrec_static_mac_binding_table
> > *nbrec_static_mac_binding_table;
> > + const struct nbrec_template_var_table *nbrec_template_var_table;
> >
> > /* Southbound table references */
> > const struct sbrec_sb_global_table *sbrec_sb_global_table;
> > @@ -55,6 +56,7 @@ struct northd_input {
> > const struct sbrec_chassis_private_table
*sbrec_chassis_private_table;
> > const struct sbrec_static_mac_binding_table
> > *sbrec_static_mac_binding_table;
> > + const struct sbrec_template_var_table *sbrec_template_var_table;
> >
> > /* Indexes */
> > struct ovsdb_idl_index *sbrec_chassis_by_name;
> > diff --git a/northd/template-var.c b/northd/template-var.c
> > new file mode 100644
> > index 000000000..8a2d118c3
> > --- /dev/null
> > +++ b/northd/template-var.c
> > @@ -0,0 +1,77 @@
> > +/* Copyright (c) 2022, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "template-var.h"
> > +
> > +struct template_var_table *
> > +template_var_table_create(void)
> > +{
> > + struct template_var_table *table = xmalloc(sizeof *table);
> > +
> > + hmap_init(&table->vars);
> > + return table;
> > +}
> > +
> > +void
> > +template_var_table_destroy(struct template_var_table *table)
> > +{
> > + struct template_var *tv;
> > +
> > + HMAP_FOR_EACH_POP (tv, hmap_node, &table->vars) {
> > + template_var_destroy(tv);
> > + }
> > + hmap_destroy(&table->vars);
> > +}
> > +
> > +void
> > +template_var_insert(struct template_var_table *table,
> > + const struct nbrec_template_var *nbrec_tv)
> > +{
> > + struct template_var *tv = xmalloc(sizeof *tv);
> > + tv->nb = nbrec_tv;
> > + hmap_insert(&table->vars, &tv->hmap_node,
> > + template_var_hash(nbrec_tv->name,
nbrec_tv->chassis_name));
> > +}
> > +
> > +struct template_var *
> > +template_var_find(struct template_var_table *table,
> > + const char *name, const char *chassis_name)
> > +{
> > + struct template_var *tv;
> > +
> > + HMAP_FOR_EACH_WITH_HASH (tv, hmap_node,
> > + template_var_hash(name, chassis_name),
> > + &table->vars) {
> > + if (!strcmp(name, tv->nb->name) &&
> > + !strcmp(chassis_name, tv->nb->chassis_name)) {
> > + return tv;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +void
> > +template_var_remove(struct template_var_table *table, struct
template_var *tv)
> > +{
> > + hmap_remove(&table->vars, &tv->hmap_node);
> > +}
> > +
> > +void
> > +template_var_destroy(struct template_var *tv)
> > +{
> > + free(tv);
> > +}
> > diff --git a/northd/template-var.h b/northd/template-var.h
> > new file mode 100644
> > index 000000000..42bdbf243
> > --- /dev/null
> > +++ b/northd/template-var.h
> > @@ -0,0 +1,59 @@
> > +/* Copyright (c) 2022, Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef OVN_NORTHD_TEMPLATE_VAR_H
> > +#define OVN_NORTHD_TEMPLATE_VAR_H 1
> > +
> > +#include "openvswitch/hmap.h"
> > +#include "lib/ovn-nb-idl.h"
> > +
> > +struct template_var {
> > + struct hmap_node hmap_node;
> > +
> > + const struct nbrec_template_var *nb;
> > +};
> > +
> > +struct template_var_table {
> > + struct hmap vars;
> > +};
> > +
> > +#define TEMPLATE_VAR_TABLE_INITIALIZER(TBL) \
> > + HMAP_INITIALIZER(&(TBL)->vars)
> > +
> > +#define TEMPLATE_VAR_TABLE_FOR_EACH(NODE, TBL) \
> > + HMAP_FOR_EACH (NODE, hmap_node, &(TBL)->vars)
> > +
> > +struct template_var_table *template_var_table_create(void);
> > +void template_var_table_destroy(struct template_var_table *table);
>
> nit: "table" can be omitted, and same for nbrec_tv and tv, etc. below.
>
> > +
> > +static inline uint32_t
> > +template_var_hash(const char *tv_name, const char *tv_chassis)
> > +{
> > + return hash_string(tv_name, hash_string(tv_chassis, 0));
> > +}
> > +
> > +void template_var_insert(struct template_var_table *table,
> > + const struct nbrec_template_var *nbrec_tv);
> > +
> > +struct template_var *
> > +template_var_find(struct template_var_table *table,
> > + const char *name, const char *chassis_name);
> > +
> > +void template_var_remove(struct template_var_table *table,
> > + struct template_var *tv);
> > +
> > +void template_var_destroy(struct template_var *tv);
> > +
> > +#endif /* OVN_NORTHD_TEMPLATE_VAR_H 1 */
> > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> > index 174364c8b..f8bded220 100644
> > --- a/ovn-nb.ovsschema
> > +++ b/ovn-nb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Northbound",
> > - "version": "6.3.0",
> > - "cksum": "4042813038 31869",
> > + "version": "6.4.0",
> > + "cksum": "169316805 32311",
> > "tables": {
> > "NB_Global": {
> > "columns": {
> > @@ -620,6 +620,16 @@
> > "mac": {"type": "string"},
> > "override_dynamic_mac": {"type": "boolean"}},
> > "indexes": [["logical_port", "ip"]],
> > - "isRoot": true}
> > + "isRoot": true},
> > + "Template_Var": {
>
> As discussed in the RFC review, it would be more clear to name this table
as Chassis_Template_Var, to reflect the fact that the template
instantantiation is per chassis. (also avoid confusion if in the future we
introduce some other types of template).
>
> > + "columns": {
> > + "name": {"type": "string"},
> > + "value": {"type": {"key": "string"}},
> > + "chassis_name": {"type": "string"},
> > + "external_ids": {
> > + "type": {"key": "string", "value": "string",
> > + "min": 0, "max": "unlimited"}}},
> > + "indexes": [["name", "chassis_name"]],
> > + "isRoot": true}
> > }
> > }
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index f41e9d7c0..bd28ac372 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -4436,4 +4436,38 @@
> > </column>
> > </group>
> > </table>
> > +
> > + <table name="Template_Var">
> > + <p>
> > + Each record represents a template variable instantiation. A
template
> > + variable has a <code>name</code> and potentially different
values on
> > + different hypervisors in the OVN cluster. The mapping between
> > + variable names and their actual values is done through the
> > + <code>value</code> and <code>chassis_name</code> fields. For
example,
> > + two rows, <code>R1 = (.name=N, .value=V1,
.chassis_name=C1)</code> and
> > + <code>R2 = (.name=N, .value=V2, .chassis_name=C2)</code> will
make
> > + <code>ovn-controller</code> running on chassis <code>C1</code>
and
> > + <code>C2</code> interpret the token <code>N</code> either as
> > + <code>V1</code> (on <code>C1</code>) or as <code>V2</code> (on
> > + <code>C2</code>). Records of type <code>Template_Var</code> are
> > + indexed by the tuple <code>(name, chassis_name)</code>.
>
> We may omit this statement about index, which is already reflected in the
schema definition.
>
> > Users can
> > + refer to template variables within ACL matches and Load_Balancer
VIP
> > + and backend definitions.
>
> If ACL works, QoS and Logical_Router_Policy should also work. It would be
better to mention in a more generic way and include Load_Balancer, ACL, QoS
and Logical_Router_Policy as examples.
> In addition, since it is mentioned as official support, I'd also expect
to see in this series at least one test case other than load-balancer using
this feature.
>
> > + </p>
> > + <column name="name">
> > + The name of the template variable.
> > + </column>
> > + <column name="value">
> > + The value it should exmpand to on the chassis specified by
> > + <code>chassis_name</code>.
> > + </column>
> > + <column name="chassis_name">
> > + The chassis on which this template variable should be expanded.
> > + </column>
> > + <group title="Common Columns">
> > + <column name="external_ids">
> > + See <em>External IDs</em> at the beginning of this document.
> > + </column>
> > + </group>
> > + </table>
> > </database>
> > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > index 576ebbdeb..f6cc60d0f 100644
> > --- a/ovn-sb.ovsschema
> > +++ b/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Southbound",
> > - "version": "20.25.0",
> > - "cksum": "53184112 28845",
> > + "version": "20.26.0",
> > + "cksum": "4176994634 29127",
> > "tables": {
> > "SB_Global": {
> > "columns": {
> > @@ -565,6 +565,13 @@
> > "key": {"type": "uuid",
> > "refTable":
"Datapath_Binding"}}}},
> > "indexes": [["logical_port", "ip"]],
> > + "isRoot": true},
> > + "Template_Var": {
> > + "columns": {
> > + "name": {"type": "string"},
> > + "value": {"type": {"key": "string"}},
> > + "chassis_name": {"type": "string"}},
> > + "indexes": [["name", "chassis_name"]],
> > "isRoot": true}
> > }
> > }
> > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > index 315d60853..a76dba063 100644
> > --- a/ovn-sb.xml
> > +++ b/ovn-sb.xml
> > @@ -4801,4 +4801,24 @@ tcp.flags = RST;
> > The logical datapath to which the logical router port belongs.
> > </column>
> > </table>
> > +
> > + <table name="Template_Var">
> > + <p>
> > + Each record represents a template variable instantiation and is
populated
> > + by <code>ovn-northd</code> from the contents of the
> > + <code>OVN_Northbound.Template_Var</code> table. Records of type
> > + <code>Template_Var</code> are indexed by the tuple
> > + <code>(name, chassis_name)</code>.
> > + </p>
> > + <column name="name">
> > + The name of the template variable.
> > + </column>
> > + <column name="value">
> > + The value it should exmpand to on the chassis specified by
>
> s/exmpand/expand
>
> With the above comments addressed:
> Acked-by: Han Zhou <[email protected]>
>
> > + <code>chassis_name</code>.
> > + </column>
> > + <column name="chassis_name">
> > + The chassis on which this template variable should be expanded.
> > + </column>
> > + </table>
> > </database>
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index a210fc575..b6843956c 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -7898,3 +7898,35 @@ AT_CHECK([grep "lr_in_arp_resolve" R1flows |
grep priority=90 | sort], [0], [dnl
> >
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([NB to SB Template_Var propagation])
> > +AT_KEYWORDS([templates])
> > +ovn_start
> > +
> > +tv1=$(ovn-nbctl create template_var name="tv" value="v1"
chassis_name="hv1")
> > +tv2=$(ovn-nbctl create template_var name="tv" value="v2"
chassis_name="hv2")
> > +
> > +AS_BOX([Ensure values are propagated to SB])
> > +check ovn-nbctl --wait=sb sync
> > +check_column "v1" sb:Template_Var value name="tv" chassis_name="hv1"
> > +check_column "v2" sb:Template_Var value name="tv" chassis_name="hv2"
> > +
> > +AS_BOX([Ensure SB is reconciled])
> > +check ovn-sbctl --all destroy Template_Var
> > +check ovn-nbctl --wait=sb sync
> > +check_column "v1" sb:Template_Var value name="tv" chassis_name="hv1"
> > +check_column "v2" sb:Template_Var value name="tv" chassis_name="hv2"
> > +
> > +AS_BOX([Ensure SB is reconciled - deletion])
> > +check ovn-nbctl destroy template_var $tv1
> > +check ovn-nbctl --wait=sb sync
> > +check_column "v2" sb:Template_Var value name="tv" chassis_name="hv2"
> > +
> > +AS_BOX([Ensure SB is reconciled - cleanup])
> > +check ovn-nbctl destroy template_var $tv2
> > +check ovn-nbctl --wait=sb sync
> > +check_row_count sb:Template_Var 0
> > +
> > +AT_CLEANUP
> > +])
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
