[ovs-dev] [PATCH ovn] Allow to run multiple controllers on the same machine
Dumitru Ceara
dceara at redhat.com
Fri Aug 7 12:25:36 UTC 2020
On 7/31/20 8:03 PM, Ihar Hrachyshka wrote:
> User stories:
> 1) NFV: an admin wants to run two separate instances of OVN controller
> using the same database but configuring ports on different bridges.
> Some of these bridges may use DPDK while others may not.
>
> 2) Parallel OVN instances: an admin wants to run two separate
> instances of OVN controller using different databases. The
> instances are completely independent and serve different consumers.
> For example, the same machine runs both OpenStack and OpenShift
> stacks, each running its own separate OVN stack.
>
> To serve these use cases, several features should be added to
> ovn-controller:
>
> - use different database configuration for multiple controllers;
> - customize chassis name used by controller.
>
> =====
>
> For each of the following database configuration options, their
> extended chassis specific counterparts are introduced:
>
> external_ids:ovn-remote -> external_ids:ovn-remote-<chassis-name>=
> external_ids:ovn-encap-type -> external_ids:ovn-encap-type-<chassis-name>=
> external_ids:ovn-encap-ip -> external_ids:ovn-encap-ip-<chassis-name>=
> external_ids:ovn-bridge -> external_ids:ovn-bridge-<chassis-name>=
>
> Priority wise, <chassis-name> specific options take precedence.
>
> =====
>
> For system-id,
>
> You can now pass intended chassis name via CLI argument:
>
> $ ovn-controller ... -n <chassis_name>
>
> Alternatively, you can configure a chassis name by putting it into the
> ${ovs_sysconfdir}/system-id file before running the controller.
>
> The latter option may be more useful in container environment where
> the same image may be reused for multiple controller instances, where
> ovs_sysconfigdir/system-id is a volume mounted into this generic
> image.
>
> Priority wise, this is the order in which different means to configure
> the chassis name are used:
>
> - external_ids:system-id= ovsdb option;
> - ${ovs_sysconfdir}/system-id file;
> - ovn-controller ... -n <chassis_name> CLI argument.
>
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
Hi Ihar,
Overall the changes look OK to me. I do have a few small comments here
and there.
I get the following errors when compiling with your patch:
controller/ovn-controller.c:87:12: error: symbol 'controller_chassis'
was not declared. Should it be static?
controller/ovn-controller.c:374:6: error: symbol 'file_system_id' was
not declared. Should it be static?
Another thing is that we probably need to add an entry to NEWS regarding
this new feature.
> ---
> controller/binding.h | 1 +
> controller/chassis.c | 21 +++++++++-
> controller/ovn-controller.c | 84 +++++++++++++++++++++++++++++++++----
> controller/ovn-controller.h | 3 ++
> ovn-sb.xml | 3 +-
> tests/ovn-controller.at | 9 ++--
> tests/ovn-macros.at | 41 ++++++++++++++----
> tests/ovn.at | 50 +++++++++++++++++++++-
> 8 files changed, 186 insertions(+), 26 deletions(-)
>
> diff --git a/controller/binding.h b/controller/binding.h
> index c9740560f..12557b3b9 100644
> --- a/controller/binding.h
> +++ b/controller/binding.h
> @@ -32,6 +32,7 @@ struct ovsrec_port_table;
> struct ovsrec_qos_table;
> struct ovsrec_bridge_table;
> struct ovsrec_open_vswitch_table;
> +struct ovsrec_open_vswitch;
> struct sbrec_chassis;
> struct sbrec_port_binding_table;
> struct sset;
> diff --git a/controller/chassis.c b/controller/chassis.c
> index eec270ea3..e2ebd6305 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -264,8 +264,25 @@ chassis_parse_ovs_config(const struct ovsrec_open_vswitch_table *ovs_table,
> return false;
> }
>
> - const char *encap_type = smap_get(&cfg->external_ids, "ovn-encap-type");
> - const char *encap_ips = smap_get(&cfg->external_ids, "ovn-encap-ip");
> + const char *encap_type = NULL;
> + const char *chassis_id = get_ovs_chassis_id(cfg);
> + if (chassis_id != NULL) {
> + char *type_key = xasprintf("ovn-encap-type-%s", chassis_id);
> + encap_type = smap_get(&cfg->external_ids, type_key);
We leak 'type_key' here.
> + }
> + if (!encap_type) {
> + encap_type = smap_get(&cfg->external_ids, "ovn-encap-type");
> + }
> +
> + const char *encap_ips = NULL;
> + if (chassis_id != NULL) {
> + char *ip_key = xasprintf("ovn-encap-ip-%s", chassis_id);
> + encap_ips = smap_get(&cfg->external_ids, ip_key);
We leak 'ip_key' here.
> + }
> + if (!encap_ips) {
> + encap_ips = smap_get(&cfg->external_ids, "ovn-encap-ip");
> + }
> +
> if (!encap_type || !encap_ips) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_INFO_RL(&rl, "Need to specify an encap type and ip");
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 5ca32ac43..8c976b2ee 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -18,10 +18,14 @@
> #include "ovn-controller.h"
>
> #include <errno.h>
> +#include <fcntl.h>
> #include <getopt.h>
> #include <signal.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
>
> #include "bfd.h"
> #include "binding.h"
> @@ -80,6 +84,8 @@ static unixctl_cb_func cluster_state_reset_cmd;
>
> #define CONTROLLER_LOOP_STOPWATCH_NAME "ovn-controller-flow-generation"
>
> +const char *controller_chassis = NULL;
> +
> static char *parse_options(int argc, char *argv[]);
> OVS_NO_RETURN static void usage(void);
>
> @@ -242,7 +248,18 @@ out:
> static const char *
> br_int_name(const struct ovsrec_open_vswitch *cfg)
> {
> - return smap_get_def(&cfg->external_ids, "ovn-bridge", DEFAULT_BRIDGE_NAME);
> + const char *bridge_name = NULL;
> + const char *chassis_id = get_ovs_chassis_id(cfg);
> + if (chassis_id != NULL) {
> + char *key = xasprintf("ovn-bridge-%s", chassis_id);
> + bridge_name = smap_get(&cfg->external_ids, key);
> + free(key);
> + }
> + if (!bridge_name) {
> + bridge_name = smap_get_def(
> + &cfg->external_ids, "ovn-bridge", DEFAULT_BRIDGE_NAME);
> + }
> + return bridge_name;
> }
>
> static const struct ovsrec_bridge *
> @@ -354,17 +371,48 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn,
> return br_int;
> }
>
> -static const char *
> -get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table)
> +char file_system_id[64];
> +
I guess this could be defined static inside get_file_system_id() as we
don't use it explicitly anywhere else.
> +static const char *get_file_system_id(void)
> +{
> + const char *ret = NULL;
> + char *filename = xasprintf("%s/system-id", ovs_sysconfdir());
> + errno = 0;
> + int fd = open(filename, O_RDONLY);
> + if (fd != -1) {
> + int nread = read(fd, file_system_id, sizeof file_system_id);
> + if (nread) {
> + file_system_id[nread] = '\0';
> + if (file_system_id[nread - 1] == '\n') {
> + file_system_id[nread - 1] = '\0';
> + }
> + ret = file_system_id;
> + }
> + close(fd);
> + }
> +
> + free(filename);
> + return ret;
> +}
> +
> +const char *
> +get_ovs_chassis_id(const struct ovsrec_open_vswitch *cfg)
> {
> - const struct ovsrec_open_vswitch *cfg
> - = ovsrec_open_vswitch_table_first(ovs_table);
> const char *chassis_id = cfg ? smap_get(&cfg->external_ids, "system-id")
> : NULL;
>
> if (!chassis_id) {
> + const char *id_from_file = get_file_system_id();
> + if (id_from_file) {
> + return id_from_file;
> + }
> + if (controller_chassis != NULL) {
Nit: this could be "if (controller_chasiss)"
> + return controller_chassis;
> + }
> +
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> - VLOG_WARN_RL(&rl, "'system-id' in Open_vSwitch database is missing.");
> + VLOG_WARN_RL(&rl, "Failed to detect system-id, "
> + "configuration not found.");
> }
>
> return chassis_id;
> @@ -477,7 +525,16 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
> }
>
> /* Set remote based on user configuration. */
> - const char *remote = smap_get(&cfg->external_ids, "ovn-remote");
> + const char *remote = NULL;
> + const char *chassis_id = get_ovs_chassis_id(cfg);
> + if (chassis_id != NULL) {
> + char *key = xasprintf("ovn-remote-%s", chassis_id);
> + remote = smap_get(&cfg->external_ids, key);
> + free(key);
> + }
> + if (!remote) {
> + remote = smap_get(&cfg->external_ids, "ovn-remote");
> + }
> ovsdb_idl_set_remote(ovnsb_idl, remote, true);
>
> /* Set probe interval, based on user configuration and the remote. */
> @@ -1138,7 +1195,9 @@ init_binding_ctx(struct engine_node *node,
> struct ovsrec_bridge_table *bridge_table =
> (struct ovsrec_bridge_table *)EN_OVSDB_GET(
> engine_get_input("OVS_bridge", node));
> - const char *chassis_id = get_ovs_chassis_id(ovs_table);
> + const struct ovsrec_open_vswitch *cfg =
> + ovsrec_open_vswitch_table_first(ovs_table);
> + const char *chassis_id = get_ovs_chassis_id(cfg);
> const struct ovsrec_bridge *br_int = get_br_int(bridge_table, ovs_table);
>
> ovs_assert(br_int && chassis_id);
> @@ -2382,7 +2441,9 @@ main(int argc, char *argv[])
> sbrec_chassis_table_get(ovnsb_idl_loop.idl);
> const struct ovsrec_bridge *br_int =
> process_br_int(ovs_idl_txn, bridge_table, ovs_table);
> - const char *chassis_id = get_ovs_chassis_id(ovs_table);
> + const struct ovsrec_open_vswitch *cfg =
> + ovsrec_open_vswitch_table_first(ovs_table);
> + const char *chassis_id = get_ovs_chassis_id(cfg);
> const struct sbrec_chassis *chassis = NULL;
> if (chassis_id) {
> chassis = chassis_run(ovnsb_idl_txn, sbrec_chassis_by_name,
> @@ -2668,6 +2729,7 @@ parse_options(int argc, char *argv[])
> STREAM_SSL_LONG_OPTIONS,
> {"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
> {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
> + {"chassis", required_argument, NULL, 'n'},
> {NULL, 0, NULL, 0}
> };
> char *short_options = ovs_cmdl_long_options_to_short_options(long_options);
> @@ -2700,6 +2762,10 @@ parse_options(int argc, char *argv[])
> stream_ssl_set_ca_cert_file(optarg, true);
> break;
>
> + case 'n':
> + controller_chassis = xstrdup(optarg);
> + break;
> +
> case '?':
> exit(EXIT_FAILURE);
>
> diff --git a/controller/ovn-controller.h b/controller/ovn-controller.h
> index 5d9466880..f37c71deb 100644
> --- a/controller/ovn-controller.h
> +++ b/controller/ovn-controller.h
> @@ -17,6 +17,7 @@
> #ifndef OVN_CONTROLLER_H
> #define OVN_CONTROLLER_H 1
>
> +#include "binding.h"
We don't really need to include "binding.h". We could just move the
forward declaration from "binding.h" here.
> #include "simap.h"
> #include "lib/ovn-sb-idl.h"
>
> @@ -87,4 +88,6 @@ enum chassis_tunnel_type {
>
> uint32_t get_tunnel_type(const char *name);
>
> +const char *get_ovs_chassis_id(const struct ovsrec_open_vswitch *cfg);
> +
> #endif /* controller/ovn-controller.h */
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index a626dbba8..aa75b4aa3 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -243,7 +243,8 @@
> ovn-controller populates this column using <ref key="system-id"
> table="Open_vSwitch" column="external_ids" db="Open_vSwitch"/>
> in the Open_vSwitch database's <ref table="Open_vSwitch"
> - db="Open_vSwitch"/> table. ovn-controller-vtep populates this
> + db="Open_vSwitch"/> table or other configuration means.
I don't think we specify anywhere in the man pages what the "other
means" are.
> + ovn-controller-vtep populates this
> column with <ref table="Physical_Switch" column="name"
> db="hardware_vtep"/> in the hardware_vtep database's
> <ref table="Physical_Switch" db="hardware_vtep"/> table.
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 77936c776..f656afc37 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -50,8 +50,7 @@ patch
> # is mirrored into the Chassis record in the OVN_Southbound db.
> check_bridge_mappings () {
> local_mappings=$1
> - sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
> - OVS_WAIT_UNTIL([test x"${local_mappings}" = x$(ovn-sbctl get Chassis ${sysid} other_config:ovn-bridge-mappings | sed -e 's/\"//g')])
> + OVS_WAIT_UNTIL([test x"${local_mappings}" = x$(ovn-sbctl get Chassis ${sandbox} other_config:ovn-bridge-mappings | sed -e 's/\"//g')])
> }
>
> # Initially there should be no patch ports.
> @@ -133,13 +132,13 @@ ovs-vsctl \
> -- add-br br-eth2
> ovn_attach n1 br-phys 192.168.0.1
>
> -sysid=$(ovs-vsctl get Open_vSwitch . external_ids:system-id)
> +sysid=${sandbox}
>
> # Make sure that the datapath_type set in the Bridge table
> # is mirrored into the Chassis record in the OVN_Southbound db.
> check_datapath_type () {
> datapath_type=$1
> - chassis_datapath_type=$(ovn-sbctl get Chassis ${sysid} other_config:datapath-type | sed -e 's/"//g') #"
> + chassis_datapath_type=$(ovn-sbctl get Chassis ${sandbox} other_config:datapath-type | sed -e 's/"//g') #"
> test "${datapath_type}" = "${chassis_datapath_type}"
> }
>
> @@ -187,7 +186,7 @@ OVS_WAIT_UNTIL([
> test "${expected_iface_types}" = "${chassis_iface_types}"
> ])
>
> -# Change the value of external_ids:system-id and make sure it's mirrored
> +# Set the value of external_ids:system-id and make sure it's mirrored
> # in the Chassis record in the OVN_Southbound database.
> sysid=${sysid}-foo
> ovs-vsctl set Open_vSwitch . external-ids:system-id="${sysid}"
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index c639c0ceb..47cc5209e 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -215,7 +215,7 @@ net_attach () {
>
> # ovn_az_attach AZ NETWORK BRIDGE IP [MASKLEN]
> ovn_az_attach() {
> - local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24}
> + local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24} intbr=${6-br-int} chassis=$7
> net_attach $net $bridge || return 1
>
> mac=`ovs-vsctl get Interface $bridge mac_in_use | sed s/\"//g`
> @@ -229,15 +229,40 @@ ovn_az_attach() {
> else
> ovn_remote=unix:$ovs_base/$az/ovn-sb/ovn-sb.sock
> fi
> +
> + remote_key=ovn-remote
> + encap_type_key=ovn-encap-type
> + encap_ip_key=ovn-encap-ip
> + if [[ -n "${chassis}" ]]; then
> + remote_key=ovn-remote-${chassis}
> + encap_type_key=ovn-encap-type-${chassis}
> + encap_ip_key=ovn-encap-ip-${chassis}
> + else
> + chassis=$sandbox
> + fi
> +
> ovs-vsctl \
> - -- set Open_vSwitch . external-ids:system-id=$sandbox \
> - -- set Open_vSwitch . external-ids:ovn-remote=$ovn_remote \
> - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve,vxlan \
> - -- set Open_vSwitch . external-ids:ovn-encap-ip=$ip \
> - -- --may-exist add-br br-int \
> - -- set bridge br-int fail-mode=secure other-config:disable-in-band=true \
> + -- set Open_vSwitch . external-ids:$remote_key=$ovn_remote \
> + -- set Open_vSwitch . external-ids:$encap_type_key=geneve,vxlan \
> + -- set Open_vSwitch . external-ids:$encap_ip_key=$ip \
> + -- --may-exist add-br ${intbr} \
> + -- set bridge ${intbr} fail-mode=secure other-config:disable-in-band=true \
> || return 1
> - start_daemon ovn-controller || return 1
> +
> + if [[ "${intbr}" = br-int ]]; then
> + pidfile="${OVS_RUNDIR}/ovn-controller.pid"
> + logfile="${OVS_LOGDIR}/ovn-controller.log"
> + else
> + pidfile="${OVS_RUNDIR}/ovn-controller-${intbr}.pid"
> + logfile="${OVS_LOGDIR}/ovn-controller-${chassis}.log"
> + fi
> +
> + ovn-controller \
> + -n ${chassis} \
> + -vconsole:off --detach --no-chdir \
> + --pidfile=${pidfile} \
> + --log-file=${logfile} || return 1
> + on_exit "test -e \"$pidfile\" && kill \`cat \"$pidfile\"\`"
> }
>
> # ovn_attach NETWORK BRIDGE IP [MASKLEN]
> diff --git a/tests/ovn.at b/tests/ovn.at
> index edddc76e1..ed1fb5791 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1662,7 +1662,55 @@ AT_CLEANUP
>
> AT_BANNER([OVN end-to-end tests])
>
> -# 3 hypervisors, one logical switch, 3 logical ports per hypervisor
> +AT_SETUP([ovn -- 2 virtual hosts, same node])
Shouldn't this be "3 virtual hosts"?
> +AT_KEYWORDS([ovn])
> +ovn_start
> +ovn-nbctl ls-add lsw0
> +net_add n1
> +sim_add hv
> +
> +as hv
> +for i in 1 2 3; do
> + ovs-vsctl add-br br-phys-$i
> + ovn_attach n1 br-phys-$i 192.168.0.$i 24 br-int-$i host-$i
> +
> + for j in 1 2 3; do
> + ovs-vsctl add-port br-int-$i vif$i$j -- set Interface vif$i$j external-ids:iface-id=lp$i$j options:tx_pcap=hv$i/vif$i$j-tx.pcap options:rxq_pcap=hv$i/vif$i$j-rx.pcap ofport-request=$i$j
> + ovn-nbctl lsp-add lsw0 lp$i$j
> + ovn-nbctl lsp-set-addresses lp$i$j "f0:00:00:00:00:$i$j 192.168.0.$i$j" unknown
> + done
> +
> +done
> +
> +for i in 1 2 3; do
> + > expout
> + for vif in 1 2 3; do
> + echo vif$i$vif >> expout
> + done
> + AT_CHECK([ovs-vsctl list-ports br-int-$i], [0], [expout])
> +done
Should we also check that the port bindings are claimed?
Thanks,
Dumitru
> +
> +AT_CLEANUP
> +
> +AT_SETUP([ovn -- system-id in file])
> +AT_KEYWORDS([ovn])
> +
> +ovn_start
> +net_add n1
> +sim_add hv
> +
> +as hv
> +
> +echo otherid > ${OVS_SYSCONFDIR}/system-id
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +# system-id file overrides chassis name selected via cli
> +echo otherid > expout
> +AT_CHECK([ovn-sbctl --bare --columns name list chassis], [0], [expout])
> +
> +AT_CLEANUP
> +
> AT_SETUP([ovn -- 3 HVs, 1 LS, 3 lports/HV])
> AT_KEYWORDS([ovnarp])
> ovn_start
>
More information about the dev
mailing list