| Message ID | 20221115151822.283543-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Han Zhou |
| Headers | show |
| Series | northd IP for address sets | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Tue, Nov 15, 2022 at 7:19 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > A sub-engine node 'en_address_set_sync' is added with-in the > 'en_sb_sync' node to sync the Address_Set table in the > SB database. To start with, it falls back to full recompute > all the time. Upcoming patch will add the incremental processing > support to sync the SB Address_Set table. > > 'en_sb_sync' engine node can be enhanced further to sync other > SB tables like - Port_Group, DHCP_Options, DNS etc. Thanks Numan. First of all, this approach looks good to me. At the same time let me elaborate my concerns and how I convinced myself. My first impression was that the new engine node "en_address_set_sync" doesn't fit the I-P engine because it generates data but doesn't own any data. I-P engine was designed with a focus on data and their dependencies. Each engine node maintains some data and its methods compute/generate the data, according to its inputs. Ideally, the I-P engine's task is to generate output data only, and some other modules may use the output data in their own ways, such as sending flows to OVS, or writing states to SB DB. This design, from my view, would keep the I-P engine clean and easy (relatively) to maintain. However, in the actual implementation of the methods, we not only generate the data, but sometimes also write the data to SB DB (or even NB DB). This wasn't a big problem in ovn-controller, because there is not much data to write to SB DB (even though there were several tricky problems related to this and had to be very carefully handled). Now for northd, it is a much bigger problem because the majority of the output of northd needs to be written to SB DB. If we take a clean approach that I-P engine purely generates data but never touches external states, we should have some "output" engine nodes that maintain the data in memory, and then a module outside of the I-P engine syncs the delta (either by a full comparing or by tracked changes generated by I-P) to external storage. If I understand correctly this is also the way DDlog works. However, this approach would waste a lot of memory and processing on the extra copy of the data, which could be avoided by directly storing the output in the SB IDL. Adding the "data-less" engine nodes as is done by this patch solves the problem smartly. Another way to think about this is that the node does maintain the generated SB address_set. It just maintains the data in the SB IDL instead of an extra copy of memory. It would be good to document such considerations in the I-P engine documentation, to emphasize that the I-P engine should still be data-focused and mention such "data-less" engine nodes as special cases. Or I can add such documentation as a separate patch. Let me know. In addition, please see some other comments below. > > Acked-by: Mark Michelson <mmichels@redhat.com> > Acked-by: Ales Musil <amusil@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > lib/ovn-util.c | 30 +++++ > lib/ovn-util.h | 3 + > northd/automake.mk | 4 + > northd/en-northd-output.c | 57 ++++++++++ > northd/en-northd-output.h | 17 +++ > northd/en-sb-sync.c | 230 ++++++++++++++++++++++++++++++++++++++ > northd/en-sb-sync.h | 14 +++ > northd/inc-proc-northd.c | 30 ++++- > northd/northd.c | 173 ++-------------------------- > northd/northd.h | 1 + > 10 files changed, 394 insertions(+), 165 deletions(-) > create mode 100644 northd/en-northd-output.c > create mode 100644 northd/en-northd-output.h > create mode 100644 northd/en-sb-sync.c > create mode 100644 northd/en-sb-sync.h > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index 597625a29..868472ace 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -938,3 +938,33 @@ daemon_started_recently(void) > /* Ensure that at least an amount of time has passed. */ > return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS; > } > + > +/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) > + * for the router's load balancer VIP address set, combining the logical > + * router's datapath tunnel key and address family. > + * > + * Also prefixes the name with 'prefix'. > + */ > +static char * > +lr_lb_address_set_name_(uint32_t lr_tunnel_key, const char *prefix, > + int addr_family) > +{ > + return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, lr_tunnel_key, > + addr_family == AF_INET ? "4" : "6"); > +} > + > +/* Builds the router's load balancer VIP address set name. */ > +char * > +lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family) > +{ > + return lr_lb_address_set_name_(lr_tunnel_key, "", addr_family); > +} > + > +/* Builds a string that refers to the the router's load balancer VIP address > + * set name, that is: $<address_set_name>. > + */ > +char * > +lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) > +{ > + return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); > +} > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index a1f1cf0ad..809ff1d36 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -315,4 +315,7 @@ void daemon_started_recently_ignore(void); > bool daemon_started_recently(void); > int64_t daemon_startup_ts(void); > > +char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); > +char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); > + > #endif /* OVN_UTIL_H */ > diff --git a/northd/automake.mk b/northd/automake.mk > index 81582867d..138b7b32e 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -10,6 +10,10 @@ northd_ovn_northd_SOURCES = \ > northd/en-northd.h \ > northd/en-lflow.c \ > northd/en-lflow.h \ > + northd/en-northd-output.c \ > + northd/en-northd-output.h \ > + northd/en-sb-sync.c \ > + northd/en-sb-sync.h \ > northd/inc-proc-northd.c \ > northd/inc-proc-northd.h \ > northd/ipam.c \ > diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c > new file mode 100644 > index 000000000..0033d371e > --- /dev/null > +++ b/northd/en-northd-output.c > @@ -0,0 +1,57 @@ > +/* > + * 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 <getopt.h> > +#include <stdlib.h> > +#include <stdio.h> > + > +#include "openvswitch/util.h" > + > +#include "en-northd-output.h" > +#include "lib/inc-proc-eng.h" > + > +void * > +en_northd_output_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +void > +en_northd_output_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void > +en_northd_output_cleanup(void *data OVS_UNUSED) > +{ > + > +} > + > +bool > +northd_output_sb_sync_handler(struct engine_node *node, void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > + > +bool > +northd_output_lflow_handler(struct engine_node *node, void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > + return true; > +} > diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h > new file mode 100644 > index 000000000..1258ead94 > --- /dev/null > +++ b/northd/en-northd-output.h > @@ -0,0 +1,17 @@ > +#ifndef EN_NORTHD_OUTPUT_H > +#define EN_NORTHD_OUTPUT_H 1 > + > +#include "lib/inc-proc-eng.h" > + > +void *en_northd_output_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED); > +void en_northd_output_run(struct engine_node *node OVS_UNUSED, > + void *data OVS_UNUSED); > + > +void en_northd_output_cleanup(void *data); > +bool northd_output_sb_sync_handler(struct engine_node *node, > + void *data OVS_UNUSED); > +bool northd_output_lflow_handler(struct engine_node *node, > + void *data OVS_UNUSED); > + > +#endif > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c > new file mode 100644 > index 000000000..c3ba315df > --- /dev/null > +++ b/northd/en-sb-sync.c > @@ -0,0 +1,230 @@ > +/* > + * 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 <getopt.h> > +#include <stdlib.h> > +#include <stdio.h> > + > +#include "lib/svec.h" > +#include "openvswitch/util.h" > + > +#include "en-sb-sync.h" > +#include "lib/inc-proc-eng.h" > +#include "lib/lb.h" > +#include "lib/ovn-nb-idl.h" > +#include "lib/ovn-sb-idl.h" > +#include "lib/ovn-util.h" > +#include "northd.h" > + > +#include "openvswitch/vlog.h" > + > +VLOG_DEFINE_THIS_MODULE(en_sb_sync); > + > +static void sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > + const char **addrs, size_t n_addrs, > + struct shash *sb_address_sets); > +static void sync_address_sets(const struct nbrec_address_set_table *, > + const struct nbrec_port_group_table *, > + const struct sbrec_address_set_table *, > + struct ovsdb_idl_txn *ovnsb_txn, > + struct hmap *datapaths); > + > +void * > +en_sb_sync_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +void > +en_sb_sync_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void > +en_sb_sync_cleanup(void *data OVS_UNUSED) > +{ > + > +} > + > +void * > +en_address_set_sync_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +void > +en_address_set_sync_run(struct engine_node *node, void *data OVS_UNUSED) > +{ > + const struct nbrec_address_set_table *nb_address_set_table = > + EN_OVSDB_GET(engine_get_input("NB_address_set", node)); > + const struct nbrec_port_group_table *nb_port_group_table = > + EN_OVSDB_GET(engine_get_input("NB_port_group", node)); > + const struct sbrec_address_set_table *sb_address_set_table = > + EN_OVSDB_GET(engine_get_input("SB_address_set", node)); > + > + const struct engine_context *eng_ctx = engine_get_context(); > + struct northd_data *northd_data = engine_get_input_data("northd", node); > + > + sync_address_sets(nb_address_set_table, nb_port_group_table, > + sb_address_set_table, eng_ctx->ovnsb_idl_txn, > + &northd_data->datapaths); > + > + engine_set_node_state(node, EN_UPDATED); > +} > + > +void > +en_address_set_sync_cleanup(void *data OVS_UNUSED) > +{ > + > +} > + > +/* static functions. */ > +static void > +sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > + const char **addrs, size_t n_addrs, > + struct shash *sb_address_sets) > +{ > + const struct sbrec_address_set *sb_address_set; > + sb_address_set = shash_find_and_delete(sb_address_sets, > + name); > + if (!sb_address_set) { > + sb_address_set = sbrec_address_set_insert(ovnsb_txn); > + sbrec_address_set_set_name(sb_address_set, name); > + } > + > + sbrec_address_set_set_addresses(sb_address_set, > + addrs, n_addrs); > +} > + > +/* OVN_Southbound Address_Set table contains same records as in north > + * bound, plus the records generated from Port_Group table in north bound. > + * > + * There are 2 records generated from each port group, one for IPv4, and > + * one for IPv6, named in the format: <port group name>_ip4 and > + * <port group name>_ip6 respectively. MAC addresses are ignored. > + * > + * We always update OVN_Southbound to match the Address_Set and Port_Group > + * in OVN_Northbound, so that the address sets used in Logical_Flows in > + * OVN_Southbound is checked against the proper set.*/ > +static void > +sync_address_sets( > + const struct nbrec_address_set_table *nb_address_set_table, > + const struct nbrec_port_group_table *nb_port_group_table, > + const struct sbrec_address_set_table *sb_address_set_table, > + struct ovsdb_idl_txn *ovnsb_txn, struct hmap *datapaths) > +{ > + struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); > + > + const struct sbrec_address_set *sb_address_set; > + SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, > + sb_address_set_table) { > + shash_add(&sb_address_sets, sb_address_set->name, sb_address_set); > + } > + > + /* Service monitor MAC. */ > + const char *svc_monitor_macp = northd_get_svc_monitor_mac(); > + sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, > + &sb_address_sets); > + > + /* sync port group generated address sets first */ > + const struct nbrec_port_group *nb_port_group; > + NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, > + nb_port_group_table) { > + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > + for (size_t i = 0; i < nb_port_group->n_ports; i++) { > + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) { > + const char *addrs = nb_port_group->ports[i]->addresses[j]; > + if (!is_dynamic_lsp_address(addrs)) { > + split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); > + } > + } > + if (nb_port_group->ports[i]->dynamic_addresses) { > + split_addresses(nb_port_group->ports[i]->dynamic_addresses, > + &ipv4_addrs, &ipv6_addrs); > + } > + } > + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); > + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); > + sync_address_set(ovnsb_txn, ipv4_addrs_name, > + /* "char **" is not compatible with "const char **" */ > + (const char **) ipv4_addrs.names, > + ipv4_addrs.n, &sb_address_sets); > + sync_address_set(ovnsb_txn, ipv6_addrs_name, > + /* "char **" is not compatible with "const char **" */ > + (const char **) ipv6_addrs.names, > + ipv6_addrs.n, &sb_address_sets); > + free(ipv4_addrs_name); > + free(ipv6_addrs_name); > + svec_destroy(&ipv4_addrs); > + svec_destroy(&ipv6_addrs); > + } > + > + /* Sync router load balancer VIP generated address sets. */ > + struct ovn_datapath *od; > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbr) { > + continue; > + } > + > + if (sset_count(&od->lb_ips->ips_v4_reachable)) { > + char *ipv4_addrs_name = lr_lb_address_set_name(od->tunnel_key, > + AF_INET); > + const char **ipv4_addrs = > + sset_array(&od->lb_ips->ips_v4_reachable); > + > + sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, > + sset_count(&od->lb_ips->ips_v4_reachable), > + &sb_address_sets); > + free(ipv4_addrs_name); > + free(ipv4_addrs); > + } > + > + if (sset_count(&od->lb_ips->ips_v6_reachable)) { > + char *ipv6_addrs_name = lr_lb_address_set_name(od->tunnel_key, > + AF_INET6); > + const char **ipv6_addrs = > + sset_array(&od->lb_ips->ips_v6_reachable); > + > + sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, > + sset_count(&od->lb_ips->ips_v6_reachable), > + &sb_address_sets); > + free(ipv6_addrs_name); > + free(ipv6_addrs); > + } > + } > + > + /* sync user defined address sets, which may overwrite port group > + * generated address sets if same name is used */ > + const struct nbrec_address_set *nb_address_set; > + NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, > + nb_address_set_table) { > + sync_address_set(ovnsb_txn, nb_address_set->name, > + /* "char **" is not compatible with "const char **" */ > + (const char **) nb_address_set->addresses, > + nb_address_set->n_addresses, &sb_address_sets); > + } > + > + struct shash_node *node; > + SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { > + sbrec_address_set_delete(node->data); > + shash_delete(&sb_address_sets, node); > + } > + shash_destroy(&sb_address_sets); > +} > diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h > new file mode 100644 > index 000000000..f99d6a9fc > --- /dev/null > +++ b/northd/en-sb-sync.h > @@ -0,0 +1,14 @@ > +#ifndef EN_SB_SYNC_H > +#define EN_SB_SYNC_H 1 > + > +#include "lib/inc-proc-eng.h" > + > +void *en_sb_sync_init(struct engine_node *, struct engine_arg *); > +void en_sb_sync_run(struct engine_node *, void *data); > +void en_sb_sync_cleanup(void *data); > + > +void *en_address_set_sync_init(struct engine_node *, struct engine_arg *); > +void en_address_set_sync_run(struct engine_node *, void *data); > +void en_address_set_sync_cleanup(void *data); > + > +#endif > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index 54e0ad3b0..b48f53f17 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -32,6 +32,8 @@ > #include "inc-proc-northd.h" > #include "en-northd.h" > #include "en-lflow.h" > +#include "en-northd-output.h" > +#include "en-sb-sync.h" > #include "util.h" > > VLOG_DEFINE_THIS_MODULE(inc_proc_northd); > @@ -153,6 +155,9 @@ static ENGINE_NODE(northd, "northd"); > static ENGINE_NODE(lflow, "lflow"); > static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); > static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); > +static ENGINE_NODE(northd_output, "northd_output"); > +static ENGINE_NODE(sb_sync, "sb_sync"); Suggest to rename it as "sync_to_sb". Otherwise, it may be confused as some SB table, because all the SB table engine nodes' names start with "sb_". > +static ENGINE_NODE(address_set_sync, "address_set_sync"); > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > struct ovsdb_idl_loop *sb) > @@ -229,6 +234,29 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > * once I-P engine allows multiple root nodes. */ > engine_add_input(&en_lflow, &en_mac_binding_aging, NULL); > > + /* en_address_set_sync engine node syncs the SB database tables from > + * the NB database tables. > + * Right now this engine only syncs the SB Address_Set table. > + */ The above comment is a little confusing. 1. s/en_address_set_sync/en_sb_sync 2. the en_sb_sync itself doesn't sync anything. So better to say: Right now this engine node only links to en_address_set_sync to sync address sets. > + engine_add_input(&en_address_set_sync, &en_nb_address_set, NULL); > + engine_add_input(&en_address_set_sync, &en_nb_port_group, NULL); > + engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL); > + engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group, NULL); > + engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL); > + engine_add_input(&en_address_set_sync, &en_sb_address_set, > + engine_noop_handler); I think the noop handler here is a problem. If SB Address_Set is changed by something other than northd (for whatever reason), northd is expected to resync and correct it. Ignoring the change with noop handler would fail that expectation, right? One the other hand, if we assume SB data is well protected and should never be changed by external components (or human), we should set these data as "write-only" for northd, and then we just use NULL here instead of noop handler, because northd would not receive any update notifications. Thanks, Han > + > + /* We need the en_northd generated data as input to en_address_set_sync > + * node to access the data generated by it (eg. struct ovn_datapath). > + */ > + engine_add_input(&en_address_set_sync, &en_northd, NULL); > + > + engine_add_input(&en_sb_sync, &en_address_set_sync, NULL); > + engine_add_input(&en_northd_output, &en_sb_sync, > + northd_output_sb_sync_handler); > + engine_add_input(&en_northd_output, &en_lflow, > + northd_output_lflow_handler); > + > struct engine_arg engine_arg = { > .nb_idl = nb->idl, > .sb_idl = sb->idl, > @@ -249,7 +277,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath > = mac_binding_by_datapath_index_create(sb->idl); > > - engine_init(&en_lflow, &engine_arg); > + engine_init(&en_northd_output, &engine_arg); > > engine_ovsdb_node_add_index(&en_sb_chassis, > "sbrec_chassis_by_name", > diff --git a/northd/northd.c b/northd/northd.c > index e1f3bace8..7b5bed568 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -787,37 +787,6 @@ lr_has_lb_vip(struct ovn_datapath *od) > return false; > } > > -/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) > - * for the router's load balancer VIP address set, combining the logical > - * router's datapath tunnel key and address family. > - * > - * Also prefixes the name with 'prefix'. > - */ > -static char * > -lr_lb_address_set_name_(const struct ovn_datapath *od, const char *prefix, > - int addr_family) > -{ > - ovs_assert(od->nbr); > - return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, od->tunnel_key, > - addr_family == AF_INET ? "4" : "6"); > -} > - > -/* Builds the router's load balancer VIP address set name. */ > -static char * > -lr_lb_address_set_name(const struct ovn_datapath *od, int addr_family) > -{ > - return lr_lb_address_set_name_(od, "", addr_family); > -} > - > -/* Builds a string that refers to the the router's load balancer VIP address > - * set name, that is: $<address_set_name>. > - */ > -static char * > -lr_lb_address_set_ref(const struct ovn_datapath *od, int addr_family) > -{ > - return lr_lb_address_set_name_(od, "$", addr_family); > -} > - > static void > init_lb_for_datapath(struct ovn_datapath *od) > { > @@ -12866,7 +12835,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > } > > /* Create a single ARP rule for all IPs that are used as VIPs. */ > - char *lb_ips_v4_as = lr_lb_address_set_ref(op->od, AF_INET); > + char *lb_ips_v4_as = lr_lb_address_set_ref(op->od->tunnel_key, > + AF_INET); > build_lrouter_arp_flow(op->od, op, lb_ips_v4_as, > REG_INPORT_ETH_ADDR, > match, false, 90, NULL, lflows); > @@ -12882,7 +12852,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > } > > /* Create a single ND rule for all IPs that are used as VIPs. */ > - char *lb_ips_v6_as = lr_lb_address_set_ref(op->od, AF_INET6); > + char *lb_ips_v6_as = lr_lb_address_set_ref(op->od->tunnel_key, > + AF_INET6); > build_lrouter_nd_flow(op->od, op, "nd_na", lb_ips_v6_as, NULL, > REG_INPORT_ETH_ADDR, match, false, 90, > NULL, lflows, meter_groups); > @@ -14642,136 +14613,6 @@ void build_lflows(struct lflow_input *input_data, > hmap_destroy(&mcast_groups); > } > > -static void > -sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > - const char **addrs, size_t n_addrs, > - struct shash *sb_address_sets) > -{ > - const struct sbrec_address_set *sb_address_set; > - sb_address_set = shash_find_and_delete(sb_address_sets, > - name); > - if (!sb_address_set) { > - sb_address_set = sbrec_address_set_insert(ovnsb_txn); > - sbrec_address_set_set_name(sb_address_set, name); > - } > - > - sbrec_address_set_set_addresses(sb_address_set, > - addrs, n_addrs); > -} > - > -/* OVN_Southbound Address_Set table contains same records as in north > - * bound, plus the records generated from Port_Group table in north bound. > - * > - * There are 2 records generated from each port group, one for IPv4, and > - * one for IPv6, named in the format: <port group name>_ip4 and > - * <port group name>_ip6 respectively. MAC addresses are ignored. > - * > - * We always update OVN_Southbound to match the Address_Set and Port_Group > - * in OVN_Northbound, so that the address sets used in Logical_Flows in > - * OVN_Southbound is checked against the proper set.*/ > -static void > -sync_address_sets(struct northd_input *input_data, > - struct ovsdb_idl_txn *ovnsb_txn, > - struct hmap *datapaths) > -{ > - struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); > - > - const struct sbrec_address_set *sb_address_set; > - SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, > - input_data->sbrec_address_set_table) { > - shash_add(&sb_address_sets, sb_address_set->name, sb_address_set); > - } > - > - /* Service monitor MAC. */ > - const char *svc_monitor_macp = svc_monitor_mac; > - sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, > - &sb_address_sets); > - > - /* sync port group generated address sets first */ > - const struct nbrec_port_group *nb_port_group; > - NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, > - input_data->nbrec_port_group_table) { > - struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > - struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > - for (size_t i = 0; i < nb_port_group->n_ports; i++) { > - for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) { > - const char *addrs = nb_port_group->ports[i]->addresses[j]; > - if (!is_dynamic_lsp_address(addrs)) { > - split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); > - } > - } > - if (nb_port_group->ports[i]->dynamic_addresses) { > - split_addresses(nb_port_group->ports[i]->dynamic_addresses, > - &ipv4_addrs, &ipv6_addrs); > - } > - } > - char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); > - char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); > - sync_address_set(ovnsb_txn, ipv4_addrs_name, > - /* "char **" is not compatible with "const char **" */ > - (const char **)ipv4_addrs.names, > - ipv4_addrs.n, &sb_address_sets); > - sync_address_set(ovnsb_txn, ipv6_addrs_name, > - /* "char **" is not compatible with "const char **" */ > - (const char **)ipv6_addrs.names, > - ipv6_addrs.n, &sb_address_sets); > - free(ipv4_addrs_name); > - free(ipv6_addrs_name); > - svec_destroy(&ipv4_addrs); > - svec_destroy(&ipv6_addrs); > - } > - > - /* Sync router load balancer VIP generated address sets. */ > - struct ovn_datapath *od; > - HMAP_FOR_EACH (od, key_node, datapaths) { > - if (!od->nbr) { > - continue; > - } > - > - if (sset_count(&od->lb_ips->ips_v4_reachable)) { > - char *ipv4_addrs_name = lr_lb_address_set_name(od, AF_INET); > - const char **ipv4_addrs = > - sset_array(&od->lb_ips->ips_v4_reachable); > - > - sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, > - sset_count(&od->lb_ips->ips_v4_reachable), > - &sb_address_sets); > - free(ipv4_addrs_name); > - free(ipv4_addrs); > - } > - > - if (sset_count(&od->lb_ips->ips_v6_reachable)) { > - char *ipv6_addrs_name = lr_lb_address_set_name(od, AF_INET6); > - const char **ipv6_addrs = > - sset_array(&od->lb_ips->ips_v6_reachable); > - > - sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, > - sset_count(&od->lb_ips->ips_v6_reachable), > - &sb_address_sets); > - free(ipv6_addrs_name); > - free(ipv6_addrs); > - } > - } > - > - /* sync user defined address sets, which may overwrite port group > - * generated address sets if same name is used */ > - const struct nbrec_address_set *nb_address_set; > - NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, > - input_data->nbrec_address_set_table) { > - sync_address_set(ovnsb_txn, nb_address_set->name, > - /* "char **" is not compatible with "const char **" */ > - (const char **)nb_address_set->addresses, > - nb_address_set->n_addresses, &sb_address_sets); > - } > - > - struct shash_node *node; > - SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { > - sbrec_address_set_delete(node->data); > - shash_delete(&sb_address_sets, node); > - } > - shash_destroy(&sb_address_sets); > -} > - > /* Each port group in Port_Group table in OVN_Northbound has a corresponding > * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the entries > * contains lport uuids, while in OVN_Southbound we store the lport names. > @@ -15642,7 +15483,6 @@ ovnnb_db_run(struct northd_input *input_data, > ovn_update_ipv6_prefix(&data->ports); > > sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > - sync_address_sets(input_data, ovnsb_txn, &data->datapaths); > 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); > @@ -15919,3 +15759,8 @@ void northd_run(struct northd_input *input_data, > stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); > } > > +const char * > +northd_get_svc_monitor_mac(void) > +{ > + return svc_monitor_mac; > +} > diff --git a/northd/northd.h b/northd/northd.h > index da90e2815..48edcb310 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -281,4 +281,5 @@ void bfd_cleanup_connections(struct lflow_input *input_data, > struct hmap *bfd_map); > void run_update_worker_pool(int n_threads); > > +const char *northd_get_svc_monitor_mac(void); > #endif /* NORTHD_H */ > -- > 2.38.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Fri, Nov 18, 2022 at 1:37 AM Han Zhou <zhouhan@gmail.com> wrote: > > On Tue, Nov 15, 2022 at 7:19 AM <numans@ovn.org> wrote: > > > > From: Numan Siddique <numans@ovn.org> > > > > A sub-engine node 'en_address_set_sync' is added with-in the > > 'en_sb_sync' node to sync the Address_Set table in the > > SB database. To start with, it falls back to full recompute > > all the time. Upcoming patch will add the incremental processing > > support to sync the SB Address_Set table. > > > > 'en_sb_sync' engine node can be enhanced further to sync other > > SB tables like - Port_Group, DHCP_Options, DNS etc. > > Thanks Numan. First of all, this approach looks good to me. At the same > time let me elaborate my concerns and how I convinced myself. Thanks for the review comments. > My first impression was that the new engine node "en_address_set_sync" > doesn't fit the I-P engine because it generates data but doesn't own any > data. > I-P engine was designed with a focus on data and their dependencies. Each > engine node maintains some data and its methods compute/generate the data, > according to its inputs. Ideally, the I-P engine's task is to generate > output data only, and some other modules may use the output data in their > own ways, such as sending flows to OVS, or writing states to SB DB. This > design, from my view, would keep the I-P engine clean and easy (relatively) > to maintain. > However, in the actual implementation of the methods, we not only generate > the data, but sometimes also write the data to SB DB (or even NB DB). This > wasn't a big problem in ovn-controller, because there is not much data to > write to SB DB (even though there were several tricky problems related to > this and had to be very carefully handled). Now for northd, it is a much > bigger problem because the majority of the output of northd needs to be > written to SB DB. > If we take a clean approach that I-P engine purely generates data but never > touches external states, we should have some "output" engine nodes that > maintain the data in memory, and then a module outside of the I-P engine > syncs the delta (either by a full comparing or by tracked changes generated > by I-P) to external storage. If I understand correctly this is also the way > DDlog works. However, this approach would waste a lot of memory and > processing on the extra copy of the data, which could be avoided by > directly storing the output in the SB IDL. Adding the "data-less" engine > nodes as is done by this patch solves the problem smartly. Another way to > think about this is that the node does maintain the generated SB > address_set. It just maintains the data in the SB IDL instead of an extra > copy of memory. Unfortunately I don't see any other way if we want to adopt the I-P engine to ovn-northd. > It would be good to document such considerations in the I-P engine > documentation, to emphasize that the I-P engine should still be > data-focused and mention such "data-less" engine nodes as special cases. Or > I can add such documentation as a separate patch. Let me know. I'd appreciate it if you could send the follow up patch for the documentation. I think you can better represent your thoughts in the documentation. > > In addition, please see some other comments below. > > > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Acked-by: Ales Musil <amusil@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > lib/ovn-util.c | 30 +++++ > > lib/ovn-util.h | 3 + > > northd/automake.mk | 4 + > > northd/en-northd-output.c | 57 ++++++++++ > > northd/en-northd-output.h | 17 +++ > > northd/en-sb-sync.c | 230 ++++++++++++++++++++++++++++++++++++++ > > northd/en-sb-sync.h | 14 +++ > > northd/inc-proc-northd.c | 30 ++++- > > northd/northd.c | 173 ++-------------------------- > > northd/northd.h | 1 + > > 10 files changed, 394 insertions(+), 165 deletions(-) > > create mode 100644 northd/en-northd-output.c > > create mode 100644 northd/en-northd-output.h > > create mode 100644 northd/en-sb-sync.c > > create mode 100644 northd/en-sb-sync.h > > > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > > index 597625a29..868472ace 100644 > > --- a/lib/ovn-util.c > > +++ b/lib/ovn-util.c > > @@ -938,3 +938,33 @@ daemon_started_recently(void) > > /* Ensure that at least an amount of time has passed. */ > > return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS; > > } > > + > > +/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) > > + * for the router's load balancer VIP address set, combining the logical > > + * router's datapath tunnel key and address family. > > + * > > + * Also prefixes the name with 'prefix'. > > + */ > > +static char * > > +lr_lb_address_set_name_(uint32_t lr_tunnel_key, const char *prefix, > > + int addr_family) > > +{ > > + return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, lr_tunnel_key, > > + addr_family == AF_INET ? "4" : "6"); > > +} > > + > > +/* Builds the router's load balancer VIP address set name. */ > > +char * > > +lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family) > > +{ > > + return lr_lb_address_set_name_(lr_tunnel_key, "", addr_family); > > +} > > + > > +/* Builds a string that refers to the the router's load balancer VIP > address > > + * set name, that is: $<address_set_name>. > > + */ > > +char * > > +lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) > > +{ > > + return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); > > +} > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > > index a1f1cf0ad..809ff1d36 100644 > > --- a/lib/ovn-util.h > > +++ b/lib/ovn-util.h > > @@ -315,4 +315,7 @@ void daemon_started_recently_ignore(void); > > bool daemon_started_recently(void); > > int64_t daemon_startup_ts(void); > > > > +char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); > > +char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); > > + > > #endif /* OVN_UTIL_H */ > > diff --git a/northd/automake.mk b/northd/automake.mk > > index 81582867d..138b7b32e 100644 > > --- a/northd/automake.mk > > +++ b/northd/automake.mk > > @@ -10,6 +10,10 @@ northd_ovn_northd_SOURCES = \ > > northd/en-northd.h \ > > northd/en-lflow.c \ > > northd/en-lflow.h \ > > + northd/en-northd-output.c \ > > + northd/en-northd-output.h \ > > + northd/en-sb-sync.c \ > > + northd/en-sb-sync.h \ > > northd/inc-proc-northd.c \ > > northd/inc-proc-northd.h \ > > northd/ipam.c \ > > diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c > > new file mode 100644 > > index 000000000..0033d371e > > --- /dev/null > > +++ b/northd/en-northd-output.c > > @@ -0,0 +1,57 @@ > > +/* > > + * 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 <getopt.h> > > +#include <stdlib.h> > > +#include <stdio.h> > > + > > +#include "openvswitch/util.h" > > + > > +#include "en-northd-output.h" > > +#include "lib/inc-proc-eng.h" > > + > > +void * > > +en_northd_output_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + return NULL; > > +} > > + > > +void > > +en_northd_output_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +void > > +en_northd_output_cleanup(void *data OVS_UNUSED) > > +{ > > + > > +} > > + > > +bool > > +northd_output_sb_sync_handler(struct engine_node *node, void *data > OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > + > > +bool > > +northd_output_lflow_handler(struct engine_node *node, void *data > OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > + return true; > > +} > > diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h > > new file mode 100644 > > index 000000000..1258ead94 > > --- /dev/null > > +++ b/northd/en-northd-output.h > > @@ -0,0 +1,17 @@ > > +#ifndef EN_NORTHD_OUTPUT_H > > +#define EN_NORTHD_OUTPUT_H 1 > > + > > +#include "lib/inc-proc-eng.h" > > + > > +void *en_northd_output_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED); > > +void en_northd_output_run(struct engine_node *node OVS_UNUSED, > > + void *data OVS_UNUSED); > > + > > +void en_northd_output_cleanup(void *data); > > +bool northd_output_sb_sync_handler(struct engine_node *node, > > + void *data OVS_UNUSED); > > +bool northd_output_lflow_handler(struct engine_node *node, > > + void *data OVS_UNUSED); > > + > > +#endif > > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c > > new file mode 100644 > > index 000000000..c3ba315df > > --- /dev/null > > +++ b/northd/en-sb-sync.c > > @@ -0,0 +1,230 @@ > > +/* > > + * 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 <getopt.h> > > +#include <stdlib.h> > > +#include <stdio.h> > > + > > +#include "lib/svec.h" > > +#include "openvswitch/util.h" > > + > > +#include "en-sb-sync.h" > > +#include "lib/inc-proc-eng.h" > > +#include "lib/lb.h" > > +#include "lib/ovn-nb-idl.h" > > +#include "lib/ovn-sb-idl.h" > > +#include "lib/ovn-util.h" > > +#include "northd.h" > > + > > +#include "openvswitch/vlog.h" > > + > > +VLOG_DEFINE_THIS_MODULE(en_sb_sync); > > + > > +static void sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char > *name, > > + const char **addrs, size_t n_addrs, > > + struct shash *sb_address_sets); > > +static void sync_address_sets(const struct nbrec_address_set_table *, > > + const struct nbrec_port_group_table *, > > + const struct sbrec_address_set_table *, > > + struct ovsdb_idl_txn *ovnsb_txn, > > + struct hmap *datapaths); > > + > > +void * > > +en_sb_sync_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + return NULL; > > +} > > + > > +void > > +en_sb_sync_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +void > > +en_sb_sync_cleanup(void *data OVS_UNUSED) > > +{ > > + > > +} > > + > > +void * > > +en_address_set_sync_init(struct engine_node *node OVS_UNUSED, > > + struct engine_arg *arg OVS_UNUSED) > > +{ > > + return NULL; > > +} > > + > > +void > > +en_address_set_sync_run(struct engine_node *node, void *data OVS_UNUSED) > > +{ > > + const struct nbrec_address_set_table *nb_address_set_table = > > + EN_OVSDB_GET(engine_get_input("NB_address_set", node)); > > + const struct nbrec_port_group_table *nb_port_group_table = > > + EN_OVSDB_GET(engine_get_input("NB_port_group", node)); > > + const struct sbrec_address_set_table *sb_address_set_table = > > + EN_OVSDB_GET(engine_get_input("SB_address_set", node)); > > + > > + const struct engine_context *eng_ctx = engine_get_context(); > > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > > + > > + sync_address_sets(nb_address_set_table, nb_port_group_table, > > + sb_address_set_table, eng_ctx->ovnsb_idl_txn, > > + &northd_data->datapaths); > > + > > + engine_set_node_state(node, EN_UPDATED); > > +} > > + > > +void > > +en_address_set_sync_cleanup(void *data OVS_UNUSED) > > +{ > > + > > +} > > + > > +/* static functions. */ > > +static void > > +sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > > + const char **addrs, size_t n_addrs, > > + struct shash *sb_address_sets) > > +{ > > + const struct sbrec_address_set *sb_address_set; > > + sb_address_set = shash_find_and_delete(sb_address_sets, > > + name); > > + if (!sb_address_set) { > > + sb_address_set = sbrec_address_set_insert(ovnsb_txn); > > + sbrec_address_set_set_name(sb_address_set, name); > > + } > > + > > + sbrec_address_set_set_addresses(sb_address_set, > > + addrs, n_addrs); > > +} > > + > > +/* OVN_Southbound Address_Set table contains same records as in north > > + * bound, plus the records generated from Port_Group table in north > bound. > > + * > > + * There are 2 records generated from each port group, one for IPv4, and > > + * one for IPv6, named in the format: <port group name>_ip4 and > > + * <port group name>_ip6 respectively. MAC addresses are ignored. > > + * > > + * We always update OVN_Southbound to match the Address_Set and > Port_Group > > + * in OVN_Northbound, so that the address sets used in Logical_Flows in > > + * OVN_Southbound is checked against the proper set.*/ > > +static void > > +sync_address_sets( > > + const struct nbrec_address_set_table *nb_address_set_table, > > + const struct nbrec_port_group_table *nb_port_group_table, > > + const struct sbrec_address_set_table *sb_address_set_table, > > + struct ovsdb_idl_txn *ovnsb_txn, struct hmap *datapaths) > > +{ > > + struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); > > + > > + const struct sbrec_address_set *sb_address_set; > > + SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, > > + sb_address_set_table) { > > + shash_add(&sb_address_sets, sb_address_set->name, > sb_address_set); > > + } > > + > > + /* Service monitor MAC. */ > > + const char *svc_monitor_macp = northd_get_svc_monitor_mac(); > > + sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, > > + &sb_address_sets); > > + > > + /* sync port group generated address sets first */ > > + const struct nbrec_port_group *nb_port_group; > > + NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, > > + nb_port_group_table) { > > + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > + for (size_t i = 0; i < nb_port_group->n_ports; i++) { > > + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; > j++) { > > + const char *addrs = > nb_port_group->ports[i]->addresses[j]; > > + if (!is_dynamic_lsp_address(addrs)) { > > + split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); > > + } > > + } > > + if (nb_port_group->ports[i]->dynamic_addresses) { > > + > split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > + &ipv4_addrs, &ipv6_addrs); > > + } > > + } > > + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); > > + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); > > + sync_address_set(ovnsb_txn, ipv4_addrs_name, > > + /* "char **" is not compatible with "const char > **" */ > > + (const char **) ipv4_addrs.names, > > + ipv4_addrs.n, &sb_address_sets); > > + sync_address_set(ovnsb_txn, ipv6_addrs_name, > > + /* "char **" is not compatible with "const char > **" */ > > + (const char **) ipv6_addrs.names, > > + ipv6_addrs.n, &sb_address_sets); > > + free(ipv4_addrs_name); > > + free(ipv6_addrs_name); > > + svec_destroy(&ipv4_addrs); > > + svec_destroy(&ipv6_addrs); > > + } > > + > > + /* Sync router load balancer VIP generated address sets. */ > > + struct ovn_datapath *od; > > + HMAP_FOR_EACH (od, key_node, datapaths) { > > + if (!od->nbr) { > > + continue; > > + } > > + > > + if (sset_count(&od->lb_ips->ips_v4_reachable)) { > > + char *ipv4_addrs_name = > lr_lb_address_set_name(od->tunnel_key, > > + AF_INET); > > + const char **ipv4_addrs = > > + sset_array(&od->lb_ips->ips_v4_reachable); > > + > > + sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, > > + sset_count(&od->lb_ips->ips_v4_reachable), > > + &sb_address_sets); > > + free(ipv4_addrs_name); > > + free(ipv4_addrs); > > + } > > + > > + if (sset_count(&od->lb_ips->ips_v6_reachable)) { > > + char *ipv6_addrs_name = > lr_lb_address_set_name(od->tunnel_key, > > + AF_INET6); > > + const char **ipv6_addrs = > > + sset_array(&od->lb_ips->ips_v6_reachable); > > + > > + sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, > > + sset_count(&od->lb_ips->ips_v6_reachable), > > + &sb_address_sets); > > + free(ipv6_addrs_name); > > + free(ipv6_addrs); > > + } > > + } > > + > > + /* sync user defined address sets, which may overwrite port group > > + * generated address sets if same name is used */ > > + const struct nbrec_address_set *nb_address_set; > > + NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, > > + nb_address_set_table) { > > + sync_address_set(ovnsb_txn, nb_address_set->name, > > + /* "char **" is not compatible with "const char **" */ > > + (const char **) nb_address_set->addresses, > > + nb_address_set->n_addresses, &sb_address_sets); > > + } > > + > > + struct shash_node *node; > > + SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { > > + sbrec_address_set_delete(node->data); > > + shash_delete(&sb_address_sets, node); > > + } > > + shash_destroy(&sb_address_sets); > > +} > > diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h > > new file mode 100644 > > index 000000000..f99d6a9fc > > --- /dev/null > > +++ b/northd/en-sb-sync.h > > @@ -0,0 +1,14 @@ > > +#ifndef EN_SB_SYNC_H > > +#define EN_SB_SYNC_H 1 > > + > > +#include "lib/inc-proc-eng.h" > > + > > +void *en_sb_sync_init(struct engine_node *, struct engine_arg *); > > +void en_sb_sync_run(struct engine_node *, void *data); > > +void en_sb_sync_cleanup(void *data); > > + > > +void *en_address_set_sync_init(struct engine_node *, struct engine_arg > *); > > +void en_address_set_sync_run(struct engine_node *, void *data); > > +void en_address_set_sync_cleanup(void *data); > > + > > +#endif > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index 54e0ad3b0..b48f53f17 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -32,6 +32,8 @@ > > #include "inc-proc-northd.h" > > #include "en-northd.h" > > #include "en-lflow.h" > > +#include "en-northd-output.h" > > +#include "en-sb-sync.h" > > #include "util.h" > > > > VLOG_DEFINE_THIS_MODULE(inc_proc_northd); > > @@ -153,6 +155,9 @@ static ENGINE_NODE(northd, "northd"); > > static ENGINE_NODE(lflow, "lflow"); > > static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); > > static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); > > +static ENGINE_NODE(northd_output, "northd_output"); > > +static ENGINE_NODE(sb_sync, "sb_sync"); > > Suggest to rename it as "sync_to_sb". Otherwise, it may be confused as some > SB table, because all the SB table engine nodes' names start with "sb_". Ack. > > > +static ENGINE_NODE(address_set_sync, "address_set_sync"); > > > > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > struct ovsdb_idl_loop *sb) > > @@ -229,6 +234,29 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > * once I-P engine allows multiple root nodes. */ > > engine_add_input(&en_lflow, &en_mac_binding_aging, NULL); > > > > + /* en_address_set_sync engine node syncs the SB database tables from > > + * the NB database tables. > > + * Right now this engine only syncs the SB Address_Set table. > > + */ > > The above comment is a little confusing. > 1. s/en_address_set_sync/en_sb_sync > 2. the en_sb_sync itself doesn't sync anything. So better to say: Right > now this engine node only links to en_address_set_sync to sync address sets. Ack. I'll fix it. I intended to add this comment for the engine node "sb_sync" but somehow added it here. > > > + engine_add_input(&en_address_set_sync, &en_nb_address_set, NULL); > > + engine_add_input(&en_address_set_sync, &en_nb_port_group, NULL); > > + engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL); > > + engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group, > NULL); > > + engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL); > > + engine_add_input(&en_address_set_sync, &en_sb_address_set, > > + engine_noop_handler); > > I think the noop handler here is a problem. If SB Address_Set is changed by > something other than northd (for whatever reason), northd is expected to > resync and correct it. Ignoring the change with noop handler would fail > that expectation, right? > > One the other hand, if we assume SB data is well protected and should never > be changed by external components (or human), we should set these data as > "write-only" for northd, and then we just use NULL here instead of noop > handler, because northd would not receive any update notifications. Let me think about it. Either I'd make this column "write-only" or add a handler for sb_address_set changes. > > Thanks, > Han > > > + > > + /* We need the en_northd generated data as input to > en_address_set_sync > > + * node to access the data generated by it (eg. struct ovn_datapath). > > + */ > > + engine_add_input(&en_address_set_sync, &en_northd, NULL); > > + > > + engine_add_input(&en_sb_sync, &en_address_set_sync, NULL); > > + engine_add_input(&en_northd_output, &en_sb_sync, > > + northd_output_sb_sync_handler); > > + engine_add_input(&en_northd_output, &en_lflow, > > + northd_output_lflow_handler); > > + > > struct engine_arg engine_arg = { > > .nb_idl = nb->idl, > > .sb_idl = sb->idl, > > @@ -249,7 +277,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath > > = mac_binding_by_datapath_index_create(sb->idl); > > > > - engine_init(&en_lflow, &engine_arg); > > + engine_init(&en_northd_output, &engine_arg); > > > > engine_ovsdb_node_add_index(&en_sb_chassis, > > "sbrec_chassis_by_name", > > diff --git a/northd/northd.c b/northd/northd.c > > index e1f3bace8..7b5bed568 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -787,37 +787,6 @@ lr_has_lb_vip(struct ovn_datapath *od) > > return false; > > } > > > > -/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) > > - * for the router's load balancer VIP address set, combining the logical > > - * router's datapath tunnel key and address family. > > - * > > - * Also prefixes the name with 'prefix'. > > - */ > > -static char * > > -lr_lb_address_set_name_(const struct ovn_datapath *od, const char > *prefix, > > - int addr_family) > > -{ > > - ovs_assert(od->nbr); > > - return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, od->tunnel_key, > > - addr_family == AF_INET ? "4" : "6"); > > -} > > - > > -/* Builds the router's load balancer VIP address set name. */ > > -static char * > > -lr_lb_address_set_name(const struct ovn_datapath *od, int addr_family) > > -{ > > - return lr_lb_address_set_name_(od, "", addr_family); > > -} > > - > > -/* Builds a string that refers to the the router's load balancer VIP > address > > - * set name, that is: $<address_set_name>. > > - */ > > -static char * > > -lr_lb_address_set_ref(const struct ovn_datapath *od, int addr_family) > > -{ > > - return lr_lb_address_set_name_(od, "$", addr_family); > > -} > > - > > static void > > init_lb_for_datapath(struct ovn_datapath *od) > > { > > @@ -12866,7 +12835,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > > } > > > > /* Create a single ARP rule for all IPs that are used as > VIPs. */ > > - char *lb_ips_v4_as = lr_lb_address_set_ref(op->od, AF_INET); > > + char *lb_ips_v4_as = > lr_lb_address_set_ref(op->od->tunnel_key, > > + AF_INET); > > build_lrouter_arp_flow(op->od, op, lb_ips_v4_as, > > REG_INPORT_ETH_ADDR, > > match, false, 90, NULL, lflows); > > @@ -12882,7 +12852,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > > } > > > > /* Create a single ND rule for all IPs that are used as > VIPs. */ > > - char *lb_ips_v6_as = lr_lb_address_set_ref(op->od, AF_INET6); > > + char *lb_ips_v6_as = > lr_lb_address_set_ref(op->od->tunnel_key, > > + AF_INET6); > > build_lrouter_nd_flow(op->od, op, "nd_na", lb_ips_v6_as, > NULL, > > REG_INPORT_ETH_ADDR, match, false, 90, > > NULL, lflows, meter_groups); > > @@ -14642,136 +14613,6 @@ void build_lflows(struct lflow_input > *input_data, > > hmap_destroy(&mcast_groups); > > } > > > > -static void > > -sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > > - const char **addrs, size_t n_addrs, > > - struct shash *sb_address_sets) > > -{ > > - const struct sbrec_address_set *sb_address_set; > > - sb_address_set = shash_find_and_delete(sb_address_sets, > > - name); > > - if (!sb_address_set) { > > - sb_address_set = sbrec_address_set_insert(ovnsb_txn); > > - sbrec_address_set_set_name(sb_address_set, name); > > - } > > - > > - sbrec_address_set_set_addresses(sb_address_set, > > - addrs, n_addrs); > > -} > > - > > -/* OVN_Southbound Address_Set table contains same records as in north > > - * bound, plus the records generated from Port_Group table in north > bound. > > - * > > - * There are 2 records generated from each port group, one for IPv4, and > > - * one for IPv6, named in the format: <port group name>_ip4 and > > - * <port group name>_ip6 respectively. MAC addresses are ignored. > > - * > > - * We always update OVN_Southbound to match the Address_Set and > Port_Group > > - * in OVN_Northbound, so that the address sets used in Logical_Flows in > > - * OVN_Southbound is checked against the proper set.*/ > > -static void > > -sync_address_sets(struct northd_input *input_data, > > - struct ovsdb_idl_txn *ovnsb_txn, > > - struct hmap *datapaths) > > -{ > > - struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); > > - > > - const struct sbrec_address_set *sb_address_set; > > - SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, > > - input_data->sbrec_address_set_table) { > > - shash_add(&sb_address_sets, sb_address_set->name, > sb_address_set); > > - } > > - > > - /* Service monitor MAC. */ > > - const char *svc_monitor_macp = svc_monitor_mac; > > - sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, > > - &sb_address_sets); > > - > > - /* sync port group generated address sets first */ > > - const struct nbrec_port_group *nb_port_group; > > - NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, > > - input_data->nbrec_port_group_table) > { > > - struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > - struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > - for (size_t i = 0; i < nb_port_group->n_ports; i++) { > > - for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; > j++) { > > - const char *addrs = > nb_port_group->ports[i]->addresses[j]; > > - if (!is_dynamic_lsp_address(addrs)) { > > - split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); > > - } > > - } > > - if (nb_port_group->ports[i]->dynamic_addresses) { > > - > split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > - &ipv4_addrs, &ipv6_addrs); > > - } > > - } > > - char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); > > - char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); > > - sync_address_set(ovnsb_txn, ipv4_addrs_name, > > - /* "char **" is not compatible with "const char > **" */ > > - (const char **)ipv4_addrs.names, > > - ipv4_addrs.n, &sb_address_sets); > > - sync_address_set(ovnsb_txn, ipv6_addrs_name, > > - /* "char **" is not compatible with "const char > **" */ > > - (const char **)ipv6_addrs.names, > > - ipv6_addrs.n, &sb_address_sets); > > - free(ipv4_addrs_name); > > - free(ipv6_addrs_name); > > - svec_destroy(&ipv4_addrs); > > - svec_destroy(&ipv6_addrs); > > - } > > - > > - /* Sync router load balancer VIP generated address sets. */ > > - struct ovn_datapath *od; > > - HMAP_FOR_EACH (od, key_node, datapaths) { > > - if (!od->nbr) { > > - continue; > > - } > > - > > - if (sset_count(&od->lb_ips->ips_v4_reachable)) { > > - char *ipv4_addrs_name = lr_lb_address_set_name(od, AF_INET); > > - const char **ipv4_addrs = > > - sset_array(&od->lb_ips->ips_v4_reachable); > > - > > - sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, > > - sset_count(&od->lb_ips->ips_v4_reachable), > > - &sb_address_sets); > > - free(ipv4_addrs_name); > > - free(ipv4_addrs); > > - } > > - > > - if (sset_count(&od->lb_ips->ips_v6_reachable)) { > > - char *ipv6_addrs_name = lr_lb_address_set_name(od, AF_INET6); > > - const char **ipv6_addrs = > > - sset_array(&od->lb_ips->ips_v6_reachable); > > - > > - sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, > > - sset_count(&od->lb_ips->ips_v6_reachable), > > - &sb_address_sets); > > - free(ipv6_addrs_name); > > - free(ipv6_addrs); > > - } > > - } > > - > > - /* sync user defined address sets, which may overwrite port group > > - * generated address sets if same name is used */ > > - const struct nbrec_address_set *nb_address_set; > > - NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, > > - input_data->nbrec_address_set_table) { > > - sync_address_set(ovnsb_txn, nb_address_set->name, > > - /* "char **" is not compatible with "const char **" */ > > - (const char **)nb_address_set->addresses, > > - nb_address_set->n_addresses, &sb_address_sets); > > - } > > - > > - struct shash_node *node; > > - SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { > > - sbrec_address_set_delete(node->data); > > - shash_delete(&sb_address_sets, node); > > - } > > - shash_destroy(&sb_address_sets); > > -} > > - > > /* Each port group in Port_Group table in OVN_Northbound has a > corresponding > > * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the > entries > > * contains lport uuids, while in OVN_Southbound we store the lport > names. > > @@ -15642,7 +15483,6 @@ ovnnb_db_run(struct northd_input *input_data, > > ovn_update_ipv6_prefix(&data->ports); > > > > sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); > > - sync_address_sets(input_data, ovnsb_txn, &data->datapaths); > > 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); > > @@ -15919,3 +15759,8 @@ void northd_run(struct northd_input *input_data, > > stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); > > } > > > > +const char * > > +northd_get_svc_monitor_mac(void) > > +{ > > + return svc_monitor_mac; > > +} > > diff --git a/northd/northd.h b/northd/northd.h > > index da90e2815..48edcb310 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -281,4 +281,5 @@ void bfd_cleanup_connections(struct lflow_input > *input_data, > > struct hmap *bfd_map); > > void run_update_worker_pool(int n_threads); > > > > +const char *northd_get_svc_monitor_mac(void); > > #endif /* NORTHD_H */ > > -- > > 2.38.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/ovn-util.c b/lib/ovn-util.c index 597625a29..868472ace 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -938,3 +938,33 @@ daemon_started_recently(void) /* Ensure that at least an amount of time has passed. */ return time_wall_msec() - startup_ts <= DAEMON_STARTUP_DELAY_MS; } + +/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) + * for the router's load balancer VIP address set, combining the logical + * router's datapath tunnel key and address family. + * + * Also prefixes the name with 'prefix'. + */ +static char * +lr_lb_address_set_name_(uint32_t lr_tunnel_key, const char *prefix, + int addr_family) +{ + return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, lr_tunnel_key, + addr_family == AF_INET ? "4" : "6"); +} + +/* Builds the router's load balancer VIP address set name. */ +char * +lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family) +{ + return lr_lb_address_set_name_(lr_tunnel_key, "", addr_family); +} + +/* Builds a string that refers to the the router's load balancer VIP address + * set name, that is: $<address_set_name>. + */ +char * +lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family) +{ + return lr_lb_address_set_name_(lr_tunnel_key, "$", addr_family); +} diff --git a/lib/ovn-util.h b/lib/ovn-util.h index a1f1cf0ad..809ff1d36 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -315,4 +315,7 @@ void daemon_started_recently_ignore(void); bool daemon_started_recently(void); int64_t daemon_startup_ts(void); +char *lr_lb_address_set_name(uint32_t lr_tunnel_key, int addr_family); +char *lr_lb_address_set_ref(uint32_t lr_tunnel_key, int addr_family); + #endif /* OVN_UTIL_H */ diff --git a/northd/automake.mk b/northd/automake.mk index 81582867d..138b7b32e 100644 --- a/northd/automake.mk +++ b/northd/automake.mk @@ -10,6 +10,10 @@ northd_ovn_northd_SOURCES = \ northd/en-northd.h \ northd/en-lflow.c \ northd/en-lflow.h \ + northd/en-northd-output.c \ + northd/en-northd-output.h \ + northd/en-sb-sync.c \ + northd/en-sb-sync.h \ northd/inc-proc-northd.c \ northd/inc-proc-northd.h \ northd/ipam.c \ diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c new file mode 100644 index 000000000..0033d371e --- /dev/null +++ b/northd/en-northd-output.c @@ -0,0 +1,57 @@ +/* + * 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 <getopt.h> +#include <stdlib.h> +#include <stdio.h> + +#include "openvswitch/util.h" + +#include "en-northd-output.h" +#include "lib/inc-proc-eng.h" + +void * +en_northd_output_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +void +en_northd_output_run(struct engine_node *node, void *data OVS_UNUSED) +{ + engine_set_node_state(node, EN_UPDATED); +} + +void +en_northd_output_cleanup(void *data OVS_UNUSED) +{ + +} + +bool +northd_output_sb_sync_handler(struct engine_node *node, void *data OVS_UNUSED) +{ + engine_set_node_state(node, EN_UPDATED); + return true; +} + +bool +northd_output_lflow_handler(struct engine_node *node, void *data OVS_UNUSED) +{ + engine_set_node_state(node, EN_UPDATED); + return true; +} diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h new file mode 100644 index 000000000..1258ead94 --- /dev/null +++ b/northd/en-northd-output.h @@ -0,0 +1,17 @@ +#ifndef EN_NORTHD_OUTPUT_H +#define EN_NORTHD_OUTPUT_H 1 + +#include "lib/inc-proc-eng.h" + +void *en_northd_output_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED); +void en_northd_output_run(struct engine_node *node OVS_UNUSED, + void *data OVS_UNUSED); + +void en_northd_output_cleanup(void *data); +bool northd_output_sb_sync_handler(struct engine_node *node, + void *data OVS_UNUSED); +bool northd_output_lflow_handler(struct engine_node *node, + void *data OVS_UNUSED); + +#endif diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c new file mode 100644 index 000000000..c3ba315df --- /dev/null +++ b/northd/en-sb-sync.c @@ -0,0 +1,230 @@ +/* + * 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 <getopt.h> +#include <stdlib.h> +#include <stdio.h> + +#include "lib/svec.h" +#include "openvswitch/util.h" + +#include "en-sb-sync.h" +#include "lib/inc-proc-eng.h" +#include "lib/lb.h" +#include "lib/ovn-nb-idl.h" +#include "lib/ovn-sb-idl.h" +#include "lib/ovn-util.h" +#include "northd.h" + +#include "openvswitch/vlog.h" + +VLOG_DEFINE_THIS_MODULE(en_sb_sync); + +static void sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, + const char **addrs, size_t n_addrs, + struct shash *sb_address_sets); +static void sync_address_sets(const struct nbrec_address_set_table *, + const struct nbrec_port_group_table *, + const struct sbrec_address_set_table *, + struct ovsdb_idl_txn *ovnsb_txn, + struct hmap *datapaths); + +void * +en_sb_sync_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +void +en_sb_sync_run(struct engine_node *node, void *data OVS_UNUSED) +{ + engine_set_node_state(node, EN_UPDATED); +} + +void +en_sb_sync_cleanup(void *data OVS_UNUSED) +{ + +} + +void * +en_address_set_sync_init(struct engine_node *node OVS_UNUSED, + struct engine_arg *arg OVS_UNUSED) +{ + return NULL; +} + +void +en_address_set_sync_run(struct engine_node *node, void *data OVS_UNUSED) +{ + const struct nbrec_address_set_table *nb_address_set_table = + EN_OVSDB_GET(engine_get_input("NB_address_set", node)); + const struct nbrec_port_group_table *nb_port_group_table = + EN_OVSDB_GET(engine_get_input("NB_port_group", node)); + const struct sbrec_address_set_table *sb_address_set_table = + EN_OVSDB_GET(engine_get_input("SB_address_set", node)); + + const struct engine_context *eng_ctx = engine_get_context(); + struct northd_data *northd_data = engine_get_input_data("northd", node); + + sync_address_sets(nb_address_set_table, nb_port_group_table, + sb_address_set_table, eng_ctx->ovnsb_idl_txn, + &northd_data->datapaths); + + engine_set_node_state(node, EN_UPDATED); +} + +void +en_address_set_sync_cleanup(void *data OVS_UNUSED) +{ + +} + +/* static functions. */ +static void +sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, + const char **addrs, size_t n_addrs, + struct shash *sb_address_sets) +{ + const struct sbrec_address_set *sb_address_set; + sb_address_set = shash_find_and_delete(sb_address_sets, + name); + if (!sb_address_set) { + sb_address_set = sbrec_address_set_insert(ovnsb_txn); + sbrec_address_set_set_name(sb_address_set, name); + } + + sbrec_address_set_set_addresses(sb_address_set, + addrs, n_addrs); +} + +/* OVN_Southbound Address_Set table contains same records as in north + * bound, plus the records generated from Port_Group table in north bound. + * + * There are 2 records generated from each port group, one for IPv4, and + * one for IPv6, named in the format: <port group name>_ip4 and + * <port group name>_ip6 respectively. MAC addresses are ignored. + * + * We always update OVN_Southbound to match the Address_Set and Port_Group + * in OVN_Northbound, so that the address sets used in Logical_Flows in + * OVN_Southbound is checked against the proper set.*/ +static void +sync_address_sets( + const struct nbrec_address_set_table *nb_address_set_table, + const struct nbrec_port_group_table *nb_port_group_table, + const struct sbrec_address_set_table *sb_address_set_table, + struct ovsdb_idl_txn *ovnsb_txn, struct hmap *datapaths) +{ + struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); + + const struct sbrec_address_set *sb_address_set; + SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, + sb_address_set_table) { + shash_add(&sb_address_sets, sb_address_set->name, sb_address_set); + } + + /* Service monitor MAC. */ + const char *svc_monitor_macp = northd_get_svc_monitor_mac(); + sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, + &sb_address_sets); + + /* sync port group generated address sets first */ + const struct nbrec_port_group *nb_port_group; + NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, + nb_port_group_table) { + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; + for (size_t i = 0; i < nb_port_group->n_ports; i++) { + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) { + const char *addrs = nb_port_group->ports[i]->addresses[j]; + if (!is_dynamic_lsp_address(addrs)) { + split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); + } + } + if (nb_port_group->ports[i]->dynamic_addresses) { + split_addresses(nb_port_group->ports[i]->dynamic_addresses, + &ipv4_addrs, &ipv6_addrs); + } + } + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); + sync_address_set(ovnsb_txn, ipv4_addrs_name, + /* "char **" is not compatible with "const char **" */ + (const char **) ipv4_addrs.names, + ipv4_addrs.n, &sb_address_sets); + sync_address_set(ovnsb_txn, ipv6_addrs_name, + /* "char **" is not compatible with "const char **" */ + (const char **) ipv6_addrs.names, + ipv6_addrs.n, &sb_address_sets); + free(ipv4_addrs_name); + free(ipv6_addrs_name); + svec_destroy(&ipv4_addrs); + svec_destroy(&ipv6_addrs); + } + + /* Sync router load balancer VIP generated address sets. */ + struct ovn_datapath *od; + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbr) { + continue; + } + + if (sset_count(&od->lb_ips->ips_v4_reachable)) { + char *ipv4_addrs_name = lr_lb_address_set_name(od->tunnel_key, + AF_INET); + const char **ipv4_addrs = + sset_array(&od->lb_ips->ips_v4_reachable); + + sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, + sset_count(&od->lb_ips->ips_v4_reachable), + &sb_address_sets); + free(ipv4_addrs_name); + free(ipv4_addrs); + } + + if (sset_count(&od->lb_ips->ips_v6_reachable)) { + char *ipv6_addrs_name = lr_lb_address_set_name(od->tunnel_key, + AF_INET6); + const char **ipv6_addrs = + sset_array(&od->lb_ips->ips_v6_reachable); + + sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, + sset_count(&od->lb_ips->ips_v6_reachable), + &sb_address_sets); + free(ipv6_addrs_name); + free(ipv6_addrs); + } + } + + /* sync user defined address sets, which may overwrite port group + * generated address sets if same name is used */ + const struct nbrec_address_set *nb_address_set; + NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, + nb_address_set_table) { + sync_address_set(ovnsb_txn, nb_address_set->name, + /* "char **" is not compatible with "const char **" */ + (const char **) nb_address_set->addresses, + nb_address_set->n_addresses, &sb_address_sets); + } + + struct shash_node *node; + SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { + sbrec_address_set_delete(node->data); + shash_delete(&sb_address_sets, node); + } + shash_destroy(&sb_address_sets); +} diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h new file mode 100644 index 000000000..f99d6a9fc --- /dev/null +++ b/northd/en-sb-sync.h @@ -0,0 +1,14 @@ +#ifndef EN_SB_SYNC_H +#define EN_SB_SYNC_H 1 + +#include "lib/inc-proc-eng.h" + +void *en_sb_sync_init(struct engine_node *, struct engine_arg *); +void en_sb_sync_run(struct engine_node *, void *data); +void en_sb_sync_cleanup(void *data); + +void *en_address_set_sync_init(struct engine_node *, struct engine_arg *); +void en_address_set_sync_run(struct engine_node *, void *data); +void en_address_set_sync_cleanup(void *data); + +#endif diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 54e0ad3b0..b48f53f17 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -32,6 +32,8 @@ #include "inc-proc-northd.h" #include "en-northd.h" #include "en-lflow.h" +#include "en-northd-output.h" +#include "en-sb-sync.h" #include "util.h" VLOG_DEFINE_THIS_MODULE(inc_proc_northd); @@ -153,6 +155,9 @@ static ENGINE_NODE(northd, "northd"); static ENGINE_NODE(lflow, "lflow"); static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); static ENGINE_NODE(mac_binding_aging_waker, "mac_binding_aging_waker"); +static ENGINE_NODE(northd_output, "northd_output"); +static ENGINE_NODE(sb_sync, "sb_sync"); +static ENGINE_NODE(address_set_sync, "address_set_sync"); void inc_proc_northd_init(struct ovsdb_idl_loop *nb, struct ovsdb_idl_loop *sb) @@ -229,6 +234,29 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, * once I-P engine allows multiple root nodes. */ engine_add_input(&en_lflow, &en_mac_binding_aging, NULL); + /* en_address_set_sync engine node syncs the SB database tables from + * the NB database tables. + * Right now this engine only syncs the SB Address_Set table. + */ + engine_add_input(&en_address_set_sync, &en_nb_address_set, NULL); + engine_add_input(&en_address_set_sync, &en_nb_port_group, NULL); + engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL); + engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group, NULL); + engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL); + engine_add_input(&en_address_set_sync, &en_sb_address_set, + engine_noop_handler); + + /* We need the en_northd generated data as input to en_address_set_sync + * node to access the data generated by it (eg. struct ovn_datapath). + */ + engine_add_input(&en_address_set_sync, &en_northd, NULL); + + engine_add_input(&en_sb_sync, &en_address_set_sync, NULL); + engine_add_input(&en_northd_output, &en_sb_sync, + northd_output_sb_sync_handler); + engine_add_input(&en_northd_output, &en_lflow, + northd_output_lflow_handler); + struct engine_arg engine_arg = { .nb_idl = nb->idl, .sb_idl = sb->idl, @@ -249,7 +277,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = mac_binding_by_datapath_index_create(sb->idl); - engine_init(&en_lflow, &engine_arg); + engine_init(&en_northd_output, &engine_arg); engine_ovsdb_node_add_index(&en_sb_chassis, "sbrec_chassis_by_name", diff --git a/northd/northd.c b/northd/northd.c index e1f3bace8..7b5bed568 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -787,37 +787,6 @@ lr_has_lb_vip(struct ovn_datapath *od) return false; } -/* Builds a unique address set compatible name ([a-zA-Z_.][a-zA-Z_.0-9]*) - * for the router's load balancer VIP address set, combining the logical - * router's datapath tunnel key and address family. - * - * Also prefixes the name with 'prefix'. - */ -static char * -lr_lb_address_set_name_(const struct ovn_datapath *od, const char *prefix, - int addr_family) -{ - ovs_assert(od->nbr); - return xasprintf("%s_rtr_lb_%"PRIu32"_ip%s", prefix, od->tunnel_key, - addr_family == AF_INET ? "4" : "6"); -} - -/* Builds the router's load balancer VIP address set name. */ -static char * -lr_lb_address_set_name(const struct ovn_datapath *od, int addr_family) -{ - return lr_lb_address_set_name_(od, "", addr_family); -} - -/* Builds a string that refers to the the router's load balancer VIP address - * set name, that is: $<address_set_name>. - */ -static char * -lr_lb_address_set_ref(const struct ovn_datapath *od, int addr_family) -{ - return lr_lb_address_set_name_(od, "$", addr_family); -} - static void init_lb_for_datapath(struct ovn_datapath *od) { @@ -12866,7 +12835,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, } /* Create a single ARP rule for all IPs that are used as VIPs. */ - char *lb_ips_v4_as = lr_lb_address_set_ref(op->od, AF_INET); + char *lb_ips_v4_as = lr_lb_address_set_ref(op->od->tunnel_key, + AF_INET); build_lrouter_arp_flow(op->od, op, lb_ips_v4_as, REG_INPORT_ETH_ADDR, match, false, 90, NULL, lflows); @@ -12882,7 +12852,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, } /* Create a single ND rule for all IPs that are used as VIPs. */ - char *lb_ips_v6_as = lr_lb_address_set_ref(op->od, AF_INET6); + char *lb_ips_v6_as = lr_lb_address_set_ref(op->od->tunnel_key, + AF_INET6); build_lrouter_nd_flow(op->od, op, "nd_na", lb_ips_v6_as, NULL, REG_INPORT_ETH_ADDR, match, false, 90, NULL, lflows, meter_groups); @@ -14642,136 +14613,6 @@ void build_lflows(struct lflow_input *input_data, hmap_destroy(&mcast_groups); } -static void -sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, - const char **addrs, size_t n_addrs, - struct shash *sb_address_sets) -{ - const struct sbrec_address_set *sb_address_set; - sb_address_set = shash_find_and_delete(sb_address_sets, - name); - if (!sb_address_set) { - sb_address_set = sbrec_address_set_insert(ovnsb_txn); - sbrec_address_set_set_name(sb_address_set, name); - } - - sbrec_address_set_set_addresses(sb_address_set, - addrs, n_addrs); -} - -/* OVN_Southbound Address_Set table contains same records as in north - * bound, plus the records generated from Port_Group table in north bound. - * - * There are 2 records generated from each port group, one for IPv4, and - * one for IPv6, named in the format: <port group name>_ip4 and - * <port group name>_ip6 respectively. MAC addresses are ignored. - * - * We always update OVN_Southbound to match the Address_Set and Port_Group - * in OVN_Northbound, so that the address sets used in Logical_Flows in - * OVN_Southbound is checked against the proper set.*/ -static void -sync_address_sets(struct northd_input *input_data, - struct ovsdb_idl_txn *ovnsb_txn, - struct hmap *datapaths) -{ - struct shash sb_address_sets = SHASH_INITIALIZER(&sb_address_sets); - - const struct sbrec_address_set *sb_address_set; - SBREC_ADDRESS_SET_TABLE_FOR_EACH (sb_address_set, - input_data->sbrec_address_set_table) { - shash_add(&sb_address_sets, sb_address_set->name, sb_address_set); - } - - /* Service monitor MAC. */ - const char *svc_monitor_macp = svc_monitor_mac; - sync_address_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, - &sb_address_sets); - - /* sync port group generated address sets first */ - const struct nbrec_port_group *nb_port_group; - NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_port_group, - input_data->nbrec_port_group_table) { - struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; - struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; - for (size_t i = 0; i < nb_port_group->n_ports; i++) { - for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) { - const char *addrs = nb_port_group->ports[i]->addresses[j]; - if (!is_dynamic_lsp_address(addrs)) { - split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); - } - } - if (nb_port_group->ports[i]->dynamic_addresses) { - split_addresses(nb_port_group->ports[i]->dynamic_addresses, - &ipv4_addrs, &ipv6_addrs); - } - } - char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); - char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); - sync_address_set(ovnsb_txn, ipv4_addrs_name, - /* "char **" is not compatible with "const char **" */ - (const char **)ipv4_addrs.names, - ipv4_addrs.n, &sb_address_sets); - sync_address_set(ovnsb_txn, ipv6_addrs_name, - /* "char **" is not compatible with "const char **" */ - (const char **)ipv6_addrs.names, - ipv6_addrs.n, &sb_address_sets); - free(ipv4_addrs_name); - free(ipv6_addrs_name); - svec_destroy(&ipv4_addrs); - svec_destroy(&ipv6_addrs); - } - - /* Sync router load balancer VIP generated address sets. */ - struct ovn_datapath *od; - HMAP_FOR_EACH (od, key_node, datapaths) { - if (!od->nbr) { - continue; - } - - if (sset_count(&od->lb_ips->ips_v4_reachable)) { - char *ipv4_addrs_name = lr_lb_address_set_name(od, AF_INET); - const char **ipv4_addrs = - sset_array(&od->lb_ips->ips_v4_reachable); - - sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, - sset_count(&od->lb_ips->ips_v4_reachable), - &sb_address_sets); - free(ipv4_addrs_name); - free(ipv4_addrs); - } - - if (sset_count(&od->lb_ips->ips_v6_reachable)) { - char *ipv6_addrs_name = lr_lb_address_set_name(od, AF_INET6); - const char **ipv6_addrs = - sset_array(&od->lb_ips->ips_v6_reachable); - - sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, - sset_count(&od->lb_ips->ips_v6_reachable), - &sb_address_sets); - free(ipv6_addrs_name); - free(ipv6_addrs); - } - } - - /* sync user defined address sets, which may overwrite port group - * generated address sets if same name is used */ - const struct nbrec_address_set *nb_address_set; - NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, - input_data->nbrec_address_set_table) { - sync_address_set(ovnsb_txn, nb_address_set->name, - /* "char **" is not compatible with "const char **" */ - (const char **)nb_address_set->addresses, - nb_address_set->n_addresses, &sb_address_sets); - } - - struct shash_node *node; - SHASH_FOR_EACH_SAFE (node, &sb_address_sets) { - sbrec_address_set_delete(node->data); - shash_delete(&sb_address_sets, node); - } - shash_destroy(&sb_address_sets); -} - /* Each port group in Port_Group table in OVN_Northbound has a corresponding * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the entries * contains lport uuids, while in OVN_Southbound we store the lport names. @@ -15642,7 +15483,6 @@ ovnnb_db_run(struct northd_input *input_data, ovn_update_ipv6_prefix(&data->ports); sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs); - sync_address_sets(input_data, ovnsb_txn, &data->datapaths); 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); @@ -15919,3 +15759,8 @@ void northd_run(struct northd_input *input_data, stopwatch_stop(OVNSB_DB_RUN_STOPWATCH_NAME, time_msec()); } +const char * +northd_get_svc_monitor_mac(void) +{ + return svc_monitor_mac; +} diff --git a/northd/northd.h b/northd/northd.h index da90e2815..48edcb310 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -281,4 +281,5 @@ void bfd_cleanup_connections(struct lflow_input *input_data, struct hmap *bfd_map); void run_update_worker_pool(int n_threads); +const char *northd_get_svc_monitor_mac(void); #endif /* NORTHD_H */