Currently ovn-controller controls the OpenFlow flows of the internal bridge br-int through the default service controller of the <bridge>.mgmt management port. That connection is not visible to the bridge's fail-open/fail- secure state machine: ofproto_is_alive() only tracks primary controllers, so OVS cannot distinguish a live ovn-controller from a dead one.
Register a primary OpenFlow controller on br-int pointing at a passive listener, punix:<rundir>/br-int.ovn-primary, and make the ofctrl/pinctrl/statctrl software connections dial that endpoint instead of the .mgmt port. Making the connection a primary controller lets the bridge's fail-open/fail-secure state machine and ofproto_is_alive() track the ovn-controller connection. The primary target is derived from the same endpoint br_int_remote_update() dials, so external_ids:ovn-bridge-remote is honored. A primary controller is a listener hosted by ovn-controller, which only applies to local unix: and punix: endpoints; for remote tcp: and ssl: endpoints the operator configures the primary listener separately (e.g. with "ovs-vsctl set-controller"). Passive inet targets use a "[<port>][:<host>]" layout, so the active scheme cannot be swapped onto them. The br-int swconns are attempted only after the first successful OVS DB commit, so the punix listener has time to be created before ovn-controller dials it. Note that upgrading to this release adds the first primary controller to an existing br-int, which causes OVS to flush the OpenFlow tables; ovn-controller reinstalls them on the next run. With fail_mode=secure, br-int traffic on that chassis is dropped for the reinstall window. Reported-at: https://redhat.atlassian.net/browse/FDP-3771 Assisted-by: Qwen3.8-27B-FP8, OpenCode Signed-off-by: Jacob Tanenbaum <[email protected]> --- NEWS | 16 ++++ controller/ovn-controller.8.xml | 21 +++++- controller/ovn-controller.c | 130 +++++++++++++++++++++++++++++--- ovn-architecture.7.xml | 19 +++++ tests/ovn-controller.at | 80 +++++++++++++++++++- 5 files changed, 254 insertions(+), 12 deletions(-) diff --git a/NEWS b/NEWS index aadf6416d..7110bd895 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,21 @@ Post v26.09.0 -------------- + - ovn-controller now controls the OpenFlow flows of the br-int + integration bridge through a primary OpenFlow controller that it + registers on the bridge (target "punix:<run_dir>/br-int.ovn-primary"), + instead of the service controller of the br-int.mgmt management + port. This lets the bridge's fail-open/fail-secure state machine + track the ovn-controller connection. When + external_ids:ovn-bridge-remote is a local unix: or punix: target, the + primary controller listens on the same socket ovn-controller dials; + for a remote tcp: or ssl: target the operator must configure the + primary controller separately, e.g. with "ovs-vsctl + set-controller br-int ptcp:<port>". + - Note that upgrading to this release adds the first primary + controller to an existing br-int, which causes OVS to flush the + OpenFlow flow, group, and meter tables. ovn-controller reinstalls + them on its next run; with fail_mode=secure, br-int traffic on the + chassis is dropped for that window. - Removed implementations of the commit_ecmp_nh, chk_ecmp_nh, and chk_ecmp_nh_mac actions from the code. - Mark tunnel ports as transient (other_config:transient=true) when the diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml index 8121f7045..1c6090f1d 100644 --- a/controller/ovn-controller.8.xml +++ b/controller/ovn-controller.8.xml @@ -396,7 +396,26 @@ <dd> <p> Connection to the OVN management bridge in OvS. It defaults to - <code>unix:<var>br-int</var>.mgmt</code> when not specified. + <code>unix:<var>br-int</var>.ovn-primary</code> when not specified. + This is the primary OpenFlow controller connection for the + integration bridge; the usual + <code>unix:<var>br-int</var>.mgmt</code> management socket remains + available to tools such as <code>ovs-ofctl</code>(8) and + <code>ovs-appctl</code>(8). + </p> + <p> + When this option is unset, or is a + <code>unix:</code>/<code>punix:</code> target, the connection is + registered on the bridge as a <code>primary</code> OpenFlow + controller whose target is the passive (listening) + <code>punix:</code> form of this endpoint, so that OVS + fail-open/fail-secure tracks the <code>ovn-controller</code> + connection. For <code>tcp:</code> or <code>ssl:</code> targets the + listener is not hosted by <code>ovn-controller</code>; the primary + OpenFlow controller (listener) must be configured separately, e.g. + with <code>ovs-vsctl set-controller</code>(8). In that case OVS + fail-secure tracks that configured controller rather than one created + by <code>ovn-controller</code>. </p> </dd> <dt><code>external_ids:ovn-bridge-remote-probe-interval</code></dt> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index c601f89dc..784a6be5c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -145,6 +145,7 @@ static unixctl_cb_func debug_delay_nb_cfg_report; #define OVS_NB_CFG_TS_NAME "ovn-nb-cfg-ts" #define OVS_NB_CFG_SB_TS_NAME "ovn-nb-cfg-sb-ts" #define OVS_STARTUP_TS_NAME "ovn-startup-ts" +#define BR_INT_PRIMARY_CTRL_ID "ovn-br-int-primary" struct br_int_remote { char *target; @@ -655,6 +656,104 @@ get_br_datapath(const struct ovsrec_open_vswitch *cfg, return NULL; } +/* Returns the primary Controller target ovn-controller should register for + * the integration bridge, or NULL if it should not register one. 'remote' is + * the value of external_ids:ovn-bridge-remote, or NULL when unset (in which + * case the default local "unix:<rundir>/<bridge>.ovn-primary" endpoint is + * used). + * + * The primary Controller must be a listener that ovn-controller itself hosts + * on the same endpoint its software connection dials, so that OVS's + * fail-open/fail-secure state machine (ofproto_is_alive()) can observe the + * ovn-controller connection. That is only possible for local endpoints: + * - unix:/punix: (and the default) are local sockets ovn-controller can + * listen on, so the passive "punix:<path>" form is registered. + * - tcp:/ssl: (and ptcp:/pssl:) are remote endpoints; the listener is not + * hosted by ovn-controller (it dials the peer). The operator configures + * the primary controller (listener) separately, e.g. with "ovs-vsctl + * set-controller", so nothing is registered here. Registering a local + * stand-in would be wrong (and, for inet targets, the passive form uses a + * "[<port>][:<host>]" layout that does not match "host:port"). + */ +static char * +br_int_primary_target(const char *remote, const struct ovsrec_bridge *br_int) +{ + if (!remote) { + return xasprintf("punix:%s/%s.ovn-primary", ovs_rundir(), + br_int->name); + } + if (!strncmp(remote, "unix:", 5) || !strncmp(remote, "punix:", 6)) { + return xasprintf("punix:%s", strchr(remote, ':') + 1); + } + return NULL; +} + +/* Ensure the integration bridge has a primary OpenFlow controller that + * points at the passive listener ovn-controller connects through (see + * br_int_remote_update()). Making that connection a *primary* controller + * (as opposed to the service controller that OVS always adds internally for + * the <bridge>.mgmt port) causes the bridge's OpenFlow fail-open/fail-secure + * state machine and ofproto_is_alive() to track the ovn-controller + * connection. A primary controller is registered only for local + * (unix/punix) endpoints, for which ovn-controller hosts the listener; for + * remote (tcp/ssl) endpoints the operator configures the primary controller + * separately. This runs regardless of whether the bridge was just created + * or pre-existed (e.g. created by the distribution's startup scripts). */ +static void +process_br_int_primary_controller(struct ovsdb_idl_txn *ovs_idl_txn, + const struct ovsrec_bridge *br_int, + const struct ovsrec_open_vswitch *cfg) +{ + char *primary_target = + br_int_primary_target( + smap_get(&cfg->external_ids, "ovn-bridge-remote"), br_int); + + /* Drop any primary controller registered by a previous run that no + * longer matches the configured endpoint (e.g. because + * external_ids:ovn-bridge-remote was changed or removed, or the run + * directory moved). Only rows created by ovn-controller -- tagged with + * BR_INT_PRIMARY_CTRL_ID in their external_ids -- are managed; primary + * controllers configured by the operator (e.g. "ovs-vsctl + * set-controller") are left untouched. */ + for (size_t i = 0; i < br_int->n_controller; i++) { + const struct ovsrec_controller *c = br_int->controller[i]; + /* c->type may be unset (NULL) for controllers configured by the + * operator, so identify rows managed by ovn-controller by the + * external_ids tag, which is safe to read, rather than by type. */ + if (c + && smap_get_bool(&c->external_ids, BR_INT_PRIMARY_CTRL_ID, false) + && (!primary_target + || strcmp(c->target, primary_target))) { + ovsrec_bridge_update_controller_delvalue(br_int, c); + ovsrec_controller_delete(c); + } + } + if (!primary_target) { + return; + } + + /* A primary controller for the configured endpoint already exists; keep + * it. This also covers a matching row created before this release + * tagged it, so it is not duplicated. */ + for (size_t i = 0; i < br_int->n_controller; i++) { + const struct ovsrec_controller *c = br_int->controller[i]; + if (c && !strcmp(c->target, primary_target) + && c->type && !strcmp(c->type, "primary")) { + free(primary_target); + return; + } + } + + struct ovsrec_controller *primary = ovsrec_controller_insert(ovs_idl_txn); + ovsrec_controller_set_target(primary, primary_target); + ovsrec_controller_set_type(primary, "primary"); + const struct smap ext_ids = + SMAP_CONST1(&ext_ids, BR_INT_PRIMARY_CTRL_ID, "true"); + ovsrec_controller_set_external_ids(primary, &ext_ids); + free(primary_target); + ovsrec_bridge_update_controller_addvalue(br_int, primary); +} + static void process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_bridge_table *bridge_table, @@ -699,6 +798,7 @@ process_br_int(struct ovsdb_idl_txn *ovs_idl_txn, ovsrec_bridge_set_fail_mode(br_int, "secure"); VLOG_WARN("Integration bridge fail-mode changed to 'secure'."); } + process_br_int_primary_controller(ovs_idl_txn, br_int, cfg); if (br_int_dp) { *br_int_dp = get_br_datapath(cfg, datapath_type); if (!(*br_int_dp)) { @@ -992,6 +1092,11 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_flow_tables); ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_other_config); ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_external_ids); + ovsdb_idl_add_column(ovs_idl, &ovsrec_bridge_col_controller); + ovsdb_idl_add_table(ovs_idl, &ovsrec_table_controller); + ovsdb_idl_add_column(ovs_idl, &ovsrec_controller_col_target); + ovsdb_idl_add_column(ovs_idl, &ovsrec_controller_col_type); + ovsdb_idl_add_column(ovs_idl, &ovsrec_controller_col_external_ids); ovsdb_idl_add_table(ovs_idl, &ovsrec_table_flow_table); ovsdb_idl_add_column(ovs_idl, &ovsrec_flow_table_col_prefixes); ovsdb_idl_add_table(ovs_idl, &ovsrec_table_ssl); @@ -7820,7 +7925,7 @@ br_int_remote_update(struct br_int_remote *remote, smap_get(&cfg->external_ids, "ovn-bridge-remote"); char *target = ext_target ? xstrdup(ext_target) - : xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name); + : xasprintf("unix:%s/%s.ovn-primary", ovs_rundir(), br_int->name); if (!remote->target || strcmp(remote->target, target)) { free(remote->target); @@ -8182,6 +8287,7 @@ main(int argc, char *argv[]) VLOG_INFO("OVN internal version is : [%s]", ovn_version); /* Main loop. */ + bool first_commit = true; int ovnsb_txn_status = 1; struct tracked_acl_ids *tracked_acl_ids = NULL; while (!exit_args.exiting) { @@ -8273,15 +8379,6 @@ main(int argc, char *argv[]) const struct ovsrec_datapath *br_int_dp = NULL; const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_table_first(ovs_table); - process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int, - ovsrec_server_has_datapath_table(ovs_idl_loop.idl) - ? &br_int_dp - : NULL); - br_int_remote_update(&br_int_remote, br_int, ovs_table); - statctrl_update_swconn(br_int_remote.target, - br_int_remote.probe_interval); - pinctrl_update_swconn(br_int_remote.target, - br_int_remote.probe_interval); /* Enable ACL matching for double tagged traffic. */ if (ovs_idl_txn && cfg) { @@ -8311,6 +8408,18 @@ main(int argc, char *argv[]) } } + process_br_int(ovs_idl_txn, bridge_table, ovs_table, &br_int, + ovsrec_server_has_datapath_table(ovs_idl_loop.idl) + ? &br_int_dp + : NULL); + if (!first_commit && br_int && br_int_remote.target) { + statctrl_update_swconn(br_int_remote.target, + br_int_remote.probe_interval); + pinctrl_update_swconn(br_int_remote.target, + br_int_remote.probe_interval); + } + br_int_remote_update(&br_int_remote, br_int, ovs_table); + static bool chassis_idx_stored = false; if (ovs_idl_txn && !chassis_idx_stored) { store_chassis_index_if_needed(ovs_table); @@ -8777,6 +8886,7 @@ main(int argc, char *argv[]) vif_plug_clear_changed( &vif_plug_changed_iface_ids); } else if (ovs_txn_status == 1) { + first_commit = false; /* The transaction committed successfully * (or it did not change anything in the database). */ ct_zones_data = engine_get_data(&en_ct_zones); diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml index 1e0c6145a..a57eb15e9 100644 --- a/ovn-architecture.7.xml +++ b/ovn-architecture.7.xml @@ -371,6 +371,25 @@ Settings</code> in <code>ovs-vsctl</code>(8) for more information. </dd> + <dt>a <code>primary</code> <code>Controller</code></dt> + <dd> + <code>ovn-controller</code> connects to the integration bridge through a + <code>Controller</code> row of type <code>primary</code> rather than + through the service controller that OVS always adds internally for the + <code>br-int.mgmt</code> port. The row's target is the passive listener + <code>punix:<var>run_dir</var>/br-int.ovn-primary</code> and the + <code>ovn-controller</code> OpenFlow connections dial the corresponding + <code>unix:</code> form of the same path. Because the connection is a + <em>primary</em> controller, the bridge's OpenFlow fail-open/fail-secure + state machine and <code>ofproto_is_alive()</code> track the + <code>ovn-controller</code> connection. When + <code>external_ids:ovn-bridge-remote</code> names a local + <code>unix:</code> or <code>punix:</code> target, the listener is + created on that path instead; for a remote <code>tcp:</code> or + <code>ssl:</code> target the operator configures the primary controller + separately, e.g. with <code>ovs-vsctl set-controller</code>(8). + </dd> + <dt><code>other-config:disable-in-band=true</code></dt> <dd> Suppresses in-band control flows for the integration bridge. It would be diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index a7b79fc67..0b0080a0a 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -3319,6 +3319,83 @@ OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1235: connected' hv1/ovn-controller.log]) OVN_CLEANUP([hv1]) AT_CLEANUP +AT_SETUP([ovn-controller - br-int primary controller]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.20 + +# ovn-controller registers itself as a primary OpenFlow controller of the +# integration bridge, so OVS fail-open/fail-secure tracks the +# ovn-controller connection. This holds whether the bridge was just created +# or pre-existed (ovn_attach pre-creates br-int here). +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=type --bare list Controller) = "xprimary"]) +AT_CHECK([ovs-vsctl --columns=type --bare list Controller], [0], [primary +]) +# The bridge keeps its fail-mode=secure setting. +AT_CHECK([ovs-vsctl --columns=fail_mode --bare list bridge br-int], [0], [secure +]) + +# ovn-controller connects through the primary (not the default .mgmt) socket. +OVS_WAIT_UNTIL([grep -q 'connecting to switch: "unix:.*br-int\.ovn-primary"' hv1/ovn-controller.log]) +OVS_WAIT_UNTIL([grep -q 'br-int\.ovn-primary: connected' hv1/ovn-controller.log]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + +AT_SETUP([ovn-controller - br-int primary controller with ovn-bridge-remote]) +AT_KEYWORDS([ovn]) +ovn_start + +net_add n1 +sim_add hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.20 + +# With external_ids:ovn-bridge-remote unset, ovn-controller registers the +# default primary controller on the default local listener. +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller type=primary) = "xpunix:$ovs_base/hv1/br-int.ovn-primary"]) + +# A local unix: override: the primary controller moves to the passive form of +# the overridden path, and the previous (stale) primary is removed. +check ovs-vsctl set open . external_ids:ovn-bridge-remote=unix:$ovs_base/hv1/br-int.ovn-override +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller type=primary) = "xpunix:$ovs_base/hv1/br-int.ovn-override"]) + +# Remove the local override: ovn-controller restores the default primary. +check ovs-vsctl remove open . external_ids ovn-bridge-remote +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller type=primary) = "xpunix:$ovs_base/hv1/br-int.ovn-primary"]) + +# A remote tcp: override: the operator configures the primary listener +# (separately, since ovn-controller cannot host a remote listener) and then +# points the override at it. ovn-controller dials the endpoint, removes the +# stale local primary, and registers no primary controller of its own; the +# operator-configured controller (which has no type) is left alone. +check ovs-vsctl set-controller br-int ptcp:1276 +# "set-controller" wiped the controllers; with the override unset, +# ovn-controller re-registers the default primary. +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller type=primary) = "xpunix:$ovs_base/hv1/br-int.ovn-primary"]) +check ovs-vsctl set open . external_ids:ovn-bridge-remote=tcp:127.0.0.1:1276 +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller) = "xptcp:1276"]) +OVS_WAIT_UNTIL([grep -q 'tcp:127.0.0.1:1276: connected' hv1/ovn-controller.log]) + +# Removing the override: ovn-controller registers the default primary again, +# keeping the operator-configured controller, and the swconn reconnects +# through the default socket. +check ovs-vsctl remove open . external_ids ovn-bridge-remote +OVS_WAIT_UNTIL([test x$(ovs-vsctl --columns=target --bare find Controller type=primary) = "xpunix:$ovs_base/hv1/br-int.ovn-primary"]) +OVS_WAIT_UNTIL([test $(grep -c 'ofctrl: connecting to switch: "unix:'$ovs_base'/hv1/br-int.ovn-primary"' hv1/ovn-controller.log) -ge 2]) +# Both controllers remain: the operator-configured one and the default +# primary. "find --bare" separates rows with a blank line, so count the +# non-empty lines. +OVS_WAIT_UNTIL([test 2 = $(ovs-vsctl --columns=target --bare find Controller | grep -c .)]) +OVS_WAIT_UNTIL([ovs-vsctl --columns=target --bare find Controller | grep -qx ptcp:1276]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + AT_SETUP([ovn-controller - br-int flow table prefixes]) AT_KEYWORDS([ovn-controller prefixes]) ovn_start @@ -3664,8 +3741,9 @@ AT_CHECK([ovsdb-client --bare dump unix:db.sock Open_vSwitch Open_vSwitch], [], Open_vSwitch table ]) check ovs-vsctl --no-wait init -OVS_WAIT_FOR_OUTPUT([ovs-vsctl show | tail -n +2], [], [dnl +OVS_WAIT_FOR_OUTPUT_UNQUOTED([ovs-vsctl show | tail -n +2], [], [dnl Bridge br-int + Controller "punix:$(pwd)/br-int.ovn-primary" fail_mode: secure datapath_type: system Port br-int -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
