On Tue, Apr 2, 2024 at 2:47 PM Mark Michelson <mmich...@redhat.com> wrote:
> On 4/2/24 01:50, Ales Musil wrote: > > > > > > On Mon, Apr 1, 2024 at 10:36 PM Mark Michelson <mmich...@redhat.com > > <mailto:mmich...@redhat.com>> wrote: > > > > Hi Ales, > > > > I have some high-level suggestions/questions. > > > > > > Hi Mark, > > thank you for the discussion. > > > > > > 1) The units for the probe interval should be milliseconds. All other > > configurable probe intervals use milliseconds, as far as I can tell. > I > > realize the rconn API in OVS uses seconds, but all *configuration* I > > could find uses milliseconds. > > > > > > It can definitely be in milliseconds instead, it's just slightly strange > > because OvS simply doesn't have this granularity. > > I 100% agree. I don't know why the configuration options are all > milliseconds, but introducing a new probe interval that uses different > units from all the rest is confusing. > You are right, I'll update it in v2 for the sake of consistency. > > > > > > > 2) Let's say a user starts ovn-controller. They set a > > "--br-int-probe-interval" but they do not set a "--br-int-remote". > > Should we honor the configured probe interval? We don't have the same > > worries about liveness with a unix socket. Plus, ovs-vswitchd sets a > > hard-coded 60 second probe interval on its unix mgmt socket. > > > > > > I would say we should honor it, in the end it's up to the user to set it > > up correctly. I know there were some problems with probe intervals being > > set but that was due to the fact the we had hardcoded 5 seconds in > > multiple places. This option now controls all of them so it can be used > > independently of the remote connection which is a good thing IMO. > > > > > > 3) Should the default probe interval depend on the remote connection > > type? For unix, it makes sense to default to 0. But if someone > > specifies > > a tcp target but does not set a probe interval, should we default to > a > > finite interval instead? > > > > > > I'm kinda hesitant to be smart in those scenarios. The deployments might > > be different with different needs, consider for example connection via > > localhost that is a completely valid scenario or connection that doesn't > > leave the host at all (container network). In those examples it should > > be reliable enough, but in the end the user should know the best if and > > when to use the probe. > > Thanks for answering (2) and (3). I'm totally fine with your reasoning > behind those answers. > > I'll give the patch a more detailed review today. > Ack, thanks, I'll wait with v2 until then. > > > > > > > On 3/28/24 04:53, Ales Musil wrote: > > > The br-int connection is hardcoded to use unix socket, which > requires > > > for the socket to be visible for ovn-controller. This is > > achievable in > > > container by mounting the socket, but in turn the container > requires > > > additional privileges. > > > > > > Add option to ovn-controller that allows to specify remote target > for > > > br-int. This gives the user possibility to connect to br-int in > > different > > > manner than unix socket, defaulting to the unix socket when not > > specified. > > > In addition, there is an option to specify inactivity probe for > this > > > connection, disabled by default. > > > > > > Reported-at: https://issues.redhat.com/browse/FDP-243 > > <https://issues.redhat.com/browse/FDP-243> > > > Signed-off-by: Ales Musil <amu...@redhat.com > > <mailto:amu...@redhat.com>> > > > --- > > > NEWS | 6 +++ > > > controller/ofctrl.c | 10 +---- > > > controller/ofctrl.h | 5 ++- > > > controller/ovn-controller.8.xml | 12 ++++++ > > > controller/ovn-controller.c | 68 > > ++++++++++++++++++++++++++------- > > > controller/pinctrl.c | 56 ++++++--------------------- > > > controller/pinctrl.h | 6 ++- > > > controller/statctrl.c | 66 > > ++++++-------------------------- > > > controller/statctrl.h | 3 +- > > > include/ovn/features.h | 2 +- > > > lib/features.c | 35 +++++------------ > > > lib/ovn-util.c | 26 +++++++++++++ > > > lib/ovn-util.h | 4 ++ > > > lib/test-ovn-features.c | 6 +-- > > > tests/ovn-controller.at <http://ovn-controller.at> | 31 > > +++++++++++++++ > > > utilities/ovn-ctl | 10 +++++ > > > 16 files changed, 192 insertions(+), 154 deletions(-) > > > > > > diff --git a/NEWS b/NEWS > > > index 4d6ebea89..4979bb806 100644 > > > --- a/NEWS > > > +++ b/NEWS > > > @@ -12,6 +12,12 @@ Post v24.03.0 > > > flow table id. > > > "lflow-stage-to-oftable STAGE_NAME" that converts stage > > name into OpenFlow > > > table id. > > > + - Add option to ovn-controller called "--br-int-remote=REMOTE" > > that allows > > > + to specify connection method to integration bridge for > > ovn-controller, > > > + defaulting to the unix socket. > > > + - Add option to ovn-controller called > > "--br-int-probe-interval=INTERVAL" > > > + that sets probe interval for integration bridge connection, > > > + disabled by default. > > > > > > OVN v24.03.0 - 01 Mar 2024 > > > -------------------------- > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > > index 6ca2ea4ce..83532bc96 100644 > > > --- a/controller/ofctrl.c > > > +++ b/controller/ofctrl.c > > > @@ -772,19 +772,13 @@ ofctrl_get_mf_field_id(void) > > > * Returns 'true' if an OpenFlow reconnect happened; 'false' > > otherwise. > > > */ > > > bool > > > -ofctrl_run(const struct ovsrec_bridge *br_int, > > > +ofctrl_run(const char *conn_target, int probe_interval, > > > const struct ovsrec_open_vswitch_table *ovs_table, > > > struct shash *pending_ct_zones) > > > { > > > - char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), > > br_int->name); > > > bool reconnected = false; > > > > > > - if (strcmp(target, rconn_get_target(swconn))) { > > > - VLOG_INFO("%s: connecting to switch", target); > > > - rconn_connect(swconn, target, target); > > > - } > > > - free(target); > > > - > > > + ovn_update_swconn_at(swconn, conn_target, probe_interval, > > "ofctrl"); > > > rconn_run(swconn); > > > > > > if (!rconn_is_connected(swconn) || !pending_ct_zones) { > > > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > > > index 502c73da6..7df0a24ea 100644 > > > --- a/controller/ofctrl.h > > > +++ b/controller/ofctrl.h > > > @@ -50,8 +50,9 @@ struct ovn_desired_flow_table { > > > /* Interface for OVN main loop. */ > > > void ofctrl_init(struct ovn_extend_table *group_table, > > > struct ovn_extend_table *meter_table); > > > -bool ofctrl_run(const struct ovsrec_bridge *br_int, > > > - const struct ovsrec_open_vswitch_table *, > > > + > > > +bool ofctrl_run(const char *conn_target, int probe_interval, > > > + const struct ovsrec_open_vswitch_table > *ovs_table, > > > struct shash *pending_ct_zones); > > > enum mf_field_id ofctrl_get_mf_field_id(void); > > > void ofctrl_put(struct ovn_desired_flow_table *lflow_table, > > > diff --git a/controller/ovn-controller.8.xml > > b/controller/ovn-controller.8.xml > > > index 5ebef048d..a86b41eea 100644 > > > --- a/controller/ovn-controller.8.xml > > > +++ b/controller/ovn-controller.8.xml > > > @@ -30,6 +30,18 @@ > > > </p> > > > > > > <h1>Options</h1> > > > + <dl> > > > + > <dt><code>--br-int-remote=<var>remote-socket</var></code></dt> > > > + <dd> > > > + Connection to the br-int management interface. > > Otherwise, the default > > > + is <code>unix:@RUNDIR > @/<var>BR_INT_NAME</var>.mgmt</code>. > > > + </dd> > > > + > > > <dt><code>--br-int-remote-probe-interval=<var>interval</var></code></dt> > > > + <dd> > > > + Probe interval in seconds, with <code>0</code> meaning > > disabled. > > > + Otherwise, the default is <code>0</code>. > > > + </dd> > > > + </dl> > > > > > > <h2>Daemon Options</h2> > > > <xi:include href="lib/daemon.xml" > > xmlns:xi="http://www.w3.org/2003/XInclude > > <http://www.w3.org/2003/XInclude>"/> > > > diff --git a/controller/ovn-controller.c > > b/controller/ovn-controller.c > > > index c9ff5967a..657574fa8 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -122,7 +122,13 @@ static unixctl_cb_func > > debug_ignore_startup_delay; > > > #define OVS_NB_CFG_TS_NAME "ovn-nb-cfg-ts" > > > #define OVS_STARTUP_TS_NAME "ovn-startup-ts" > > > > > > -static char *parse_options(int argc, char *argv[]); > > > +struct br_int_remote { > > > + char *target; > > > + int probe_interval; > > > + bool use_unix_socket; > > > +}; > > > + > > > +static char *parse_options(int argc, char *argv[], struct > > br_int_remote *); > > > OVS_NO_RETURN static void usage(void); > > > > > > /* SSL options */ > > > @@ -5052,11 +5058,30 @@ check_northd_version(struct ovsdb_idl > > *ovs_idl, struct ovsdb_idl *ovnsb_idl, > > > return true; > > > } > > > > > > +static void > > > +br_int_remote_update(struct br_int_remote *remote, > > > + const struct ovsrec_bridge *br_int) > > > +{ > > > + if (!remote->use_unix_socket || !br_int) { > > > + return; > > > + } > > > + > > > + char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), > > br_int->name); > > > + if (!remote->target || strcmp(remote->target, target)) { > > > + free(remote->target); > > > + remote->target = xstrdup(target); > > > + } > > > + free(target); > > > +} > > > + > > > int > > > main(int argc, char *argv[]) > > > { > > > struct unixctl_server *unixctl; > > > struct ovn_exit_args exit_args = {0}; > > > + struct br_int_remote br_int_remote = (struct br_int_remote) { > > > + .use_unix_socket = true > > > + }; > > > int retval; > > > > > > /* Read from system-id-override file once on startup. */ > > > @@ -5065,7 +5090,7 @@ main(int argc, char *argv[]) > > > ovs_cmdl_proctitle_init(argc, argv); > > > ovn_set_program_name(argv[0]); > > > service_start(&argc, &argv); > > > - char *ovs_remote = parse_options(argc, argv); > > > + char *ovs_remote = parse_options(argc, argv, &br_int_remote); > > > fatal_ignore_sigpipe(); > > > > > > daemonize_start(true, false); > > > @@ -5666,6 +5691,11 @@ main(int argc, char *argv[]) > > > > > ovsrec_server_has_datapath_table(ovs_idl_loop.idl) > > > ? &br_int_dp > > > : NULL); > > > + br_int_remote_update(&br_int_remote, br_int); > > > + 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) { > > > @@ -5720,7 +5750,8 @@ main(int argc, char *argv[]) > > > if (ovs_idl_txn > > > && ovs_feature_support_run(br_int_dp ? > > > > > &br_int_dp->capabilities : NULL, > > > - br_int ? br_int->name > > : NULL)) { > > > + br_int_remote.target, > > > + > > br_int_remote.probe_interval)) { > > > VLOG_INFO("OVS feature set changed, force > > recompute."); > > > engine_set_force_recompute(true); > > > if (ovs_feature_set_discovered()) { > > > @@ -5738,7 +5769,8 @@ main(int argc, char *argv[]) > > > > > > if (br_int) { > > > ct_zones_data = engine_get_data(&en_ct_zones); > > > - if (ofctrl_run(br_int, ovs_table, > > > + if (ofctrl_run(br_int_remote.target, > > > + br_int_remote.probe_interval, > > ovs_table, > > > ct_zones_data ? > > &ct_zones_data->pending > > > : NULL)) { > > > static struct vlog_rate_limit rl > > > @@ -5855,7 +5887,7 @@ main(int argc, char *argv[]) > > > } > > > > stopwatch_start(PINCTRL_RUN_STOPWATCH_NAME, > > > time_msec()); > > > - pinctrl_update(ovnsb_idl_loop.idl, > > br_int->name); > > > + pinctrl_update(ovnsb_idl_loop.idl); > > > pinctrl_run(ovnsb_idl_txn, > > > > sbrec_datapath_binding_by_key, > > > > sbrec_port_binding_by_datapath, > > > @@ -5909,7 +5941,6 @@ main(int argc, char *argv[]) > > > } > > > > > > if (mac_cache_data) { > > > - statctrl_update(br_int->name); > > > statctrl_run(ovnsb_idl_txn, > > mac_cache_data); > > > } > > > > > > @@ -6030,13 +6061,6 @@ main(int argc, char *argv[]) > > > binding_wait(); > > > } > > > > > > - if (!northd_version_match && br_int) { > > > - /* Set the integration bridge name to pinctrl so > > that the pinctrl > > > - * thread can handle any packet-ins when we are not > > processing > > > - * any DB updates due to version mismatch. */ > > > - pinctrl_set_br_int_name(br_int->name); > > > - } > > > - > > > unixctl_server_run(unixctl); > > > > > > unixctl_server_wait(unixctl); > > > @@ -6169,6 +6193,7 @@ loop_done: > > > ovsdb_idl_loop_destroy(&ovnsb_idl_loop); > > > > > > ovs_feature_support_destroy(); > > > + free(br_int_remote.target); > > > free(ovs_remote); > > > free(file_system_id); > > > free(cli_system_id); > > > @@ -6181,7 +6206,7 @@ loop_done: > > > } > > > > > > static char * > > > -parse_options(int argc, char *argv[]) > > > +parse_options(int argc, char *argv[], struct br_int_remote > > *br_int_remote) > > > { > > > enum { > > > OPT_PEER_CA_CERT = UCHAR_MAX + 1, > > > @@ -6190,6 +6215,8 @@ parse_options(int argc, char *argv[]) > > > OVN_DAEMON_OPTION_ENUMS, > > > SSL_OPTION_ENUMS, > > > OPT_ENABLE_DUMMY_VIF_PLUG, > > > + BR_INT_REMOTE, > > > + BR_INT_PROBE_INTERVAL, > > > }; > > > > > > static struct option long_options[] = { > > > @@ -6203,6 +6230,9 @@ parse_options(int argc, char *argv[]) > > > {"chassis", required_argument, NULL, 'n'}, > > > {"enable-dummy-vif-plug", no_argument, NULL, > > > OPT_ENABLE_DUMMY_VIF_PLUG}, > > > + {"br-int-remote", required_argument, NULL, > BR_INT_REMOTE}, > > > + {"br-int-probe-interval", required_argument, NULL, > > > + BR_INT_PROBE_INTERVAL}, > > > {NULL, 0, NULL, 0} > > > }; > > > char *short_options = > > ovs_cmdl_long_options_to_short_options(long_options); > > > @@ -6264,6 +6294,16 @@ parse_options(int argc, char *argv[]) > > > cli_system_id = xstrdup(optarg); > > > break; > > > > > > + case BR_INT_REMOTE: > > > + free(br_int_remote->target); > > > + br_int_remote->target = xstrdup(optarg); > > > + br_int_remote->use_unix_socket = false; > > > + break; > > > + > > > + case BR_INT_PROBE_INTERVAL: > > > + str_to_int(optarg, 10, > &br_int_remote->probe_interval); > > > + break; > > > + > > > case '?': > > > exit(EXIT_FAILURE); > > > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > > index 2d3595cd2..27c47d02b 100644 > > > --- a/controller/pinctrl.c > > > +++ b/controller/pinctrl.c > > > @@ -173,7 +173,8 @@ static bool garp_rarp_continuous; > > > static void *pinctrl_handler(void *arg); > > > > > > struct pinctrl { > > > - char *br_int_name; > > > + /* OpenFlow connection to the switch. */ > > > + struct rconn *swconn; > > > pthread_t pinctrl_thread; > > > /* Latch to destroy the 'pinctrl_thread' */ > > > struct latch pinctrl_thread_exit; > > > @@ -563,7 +564,7 @@ pinctrl_init(void) > > > init_svc_monitors(); > > > bfd_monitor_init(); > > > init_fdb_entries(); > > > - pinctrl.br_int_name = NULL; > > > + pinctrl.swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > > OFP15_VERSION); > > > pinctrl.mac_binding_can_timestamp = false; > > > pinctrl_handler_seq = seq_create(); > > > pinctrl_main_seq = seq_create(); > > > @@ -3537,30 +3538,13 @@ notify_pinctrl_main(void) > > > seq_change(pinctrl_main_seq); > > > } > > > > > > -static void > > > -pinctrl_rconn_setup(struct rconn *swconn, const char > *br_int_name) > > > - OVS_REQUIRES(pinctrl_mutex) > > > -{ > > > - if (br_int_name) { > > > - char *target = xasprintf("unix:%s/%s.mgmt", > > ovs_rundir(), br_int_name); > > > - > > > - if (strcmp(target, rconn_get_target(swconn))) { > > > - VLOG_INFO("%s: connecting to switch", target); > > > - rconn_connect(swconn, target, target); > > > - } > > > - free(target); > > > - } else { > > > - rconn_disconnect(swconn); > > > - } > > > -} > > > - > > > /* pinctrl_handler pthread function. */ > > > static void * > > > pinctrl_handler(void *arg_) > > > { > > > struct pinctrl *pctrl = arg_; > > > /* OpenFlow connection to the switch. */ > > > - struct rconn *swconn; > > > + struct rconn *swconn = pctrl->swconn; > > > /* Last seen sequence number for 'swconn'. When this > > differs from > > > * rconn_get_connection_seqno(rconn), 'swconn' has > > reconnected. */ > > > unsigned int conn_seq_no = 0; > > > @@ -3576,13 +3560,10 @@ pinctrl_handler(void *arg_) > > > static long long int svc_monitors_next_run_time = LLONG_MAX; > > > static long long int send_prefixd_time = LLONG_MAX; > > > > > > - swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > OFP15_VERSION); > > > - > > > while (!latch_is_set(&pctrl->pinctrl_thread_exit)) { > > > long long int bfd_time = LLONG_MAX; > > > > > > ovs_mutex_lock(&pinctrl_mutex); > > > - pinctrl_rconn_setup(swconn, pctrl->br_int_name); > > > ip_mcast_snoop_run(); > > > ovs_mutex_unlock(&pinctrl_mutex); > > > > > > @@ -3640,37 +3621,24 @@ pinctrl_handler(void *arg_) > > > poll_block(); > > > } > > > > > > - rconn_destroy(swconn); > > > return NULL; > > > } > > > > > > -static void > > > -pinctrl_set_br_int_name_(const char *br_int_name) > > > - OVS_REQUIRES(pinctrl_mutex) > > > +void > > > +pinctrl_update_swconn(const char *target, int probe_interval) > > > { > > > - if (br_int_name && (!pinctrl.br_int_name || > > strcmp(pinctrl.br_int_name, > > > - > > br_int_name))) { > > > - free(pinctrl.br_int_name); > > > - pinctrl.br_int_name = xstrdup(br_int_name); > > > - /* Notify pinctrl_handler that integration bridge is > > > - * set/changed. */ > > > + if (ovn_update_swconn_at(pinctrl.swconn, target, > > > + probe_interval, "pinctrl")) { > > > + /* Notify pinctrl_handler that integration bridge > > > + * target is set/changed. */ > > > notify_pinctrl_handler(); > > > } > > > } > > > > > > void > > > -pinctrl_set_br_int_name(const char *br_int_name) > > > -{ > > > - ovs_mutex_lock(&pinctrl_mutex); > > > - pinctrl_set_br_int_name_(br_int_name); > > > - ovs_mutex_unlock(&pinctrl_mutex); > > > -} > > > - > > > -void > > > -pinctrl_update(const struct ovsdb_idl *idl, const char > *br_int_name) > > > +pinctrl_update(const struct ovsdb_idl *idl) > > > { > > > ovs_mutex_lock(&pinctrl_mutex); > > > - pinctrl_set_br_int_name_(br_int_name); > > > > > > bool can_mb_timestamp = > > > > sbrec_server_has_mac_binding_table_col_timestamp(idl); > > > @@ -4292,7 +4260,7 @@ pinctrl_destroy(void) > > > latch_set(&pinctrl.pinctrl_thread_exit); > > > pthread_join(pinctrl.pinctrl_thread, NULL); > > > latch_destroy(&pinctrl.pinctrl_thread_exit); > > > - free(pinctrl.br_int_name); > > > + rconn_destroy(pinctrl.swconn); > > > destroy_send_garps_rarps(); > > > destroy_ipv6_ras(); > > > destroy_ipv6_prefixd(); > > > diff --git a/controller/pinctrl.h b/controller/pinctrl.h > > > index 23343f097..3462b670c 100644 > > > --- a/controller/pinctrl.h > > > +++ b/controller/pinctrl.h > > > @@ -62,8 +62,10 @@ void pinctrl_run(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > > const struct ovsrec_open_vswitch_table > > *ovs_table); > > > void pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > > void pinctrl_destroy(void); > > > -void pinctrl_set_br_int_name(const char *br_int_name); > > > -void pinctrl_update(const struct ovsdb_idl *idl, const char > > *br_int_name); > > > + > > > +void pinctrl_update_swconn(const char *target, int > probe_interval); > > > + > > > +void pinctrl_update(const struct ovsdb_idl *idl); > > > > > > struct activated_port { > > > uint32_t dp_key; > > > diff --git a/controller/statctrl.c b/controller/statctrl.c > > > index 8cce97df8..6e6d7d799 100644 > > > --- a/controller/statctrl.c > > > +++ b/controller/statctrl.c > > > @@ -80,7 +80,8 @@ struct stats_node { > > > }; > > > > > > struct statctrl_ctx { > > > - char *br_int; > > > + /* OpenFlow connection to the switch. */ > > > + struct rconn *swconn; > > > > > > pthread_t thread; > > > struct latch exit_latch; > > > @@ -95,8 +96,6 @@ static struct statctrl_ctx statctrl_ctx; > > > static struct ovs_mutex mutex; > > > > > > static void *statctrl_thread_handler(void *arg); > > > -static void statctrl_rconn_setup(struct rconn *swconn, char > > *conn_target) > > > - OVS_REQUIRES(mutex); > > > static void statctrl_handle_rconn_msg(struct rconn *swconn, > > > struct statctrl_ctx *ctx, > > > struct ofpbuf *msg); > > > @@ -109,8 +108,6 @@ static void statctrl_send_request(struct > > rconn *swconn, > > > struct statctrl_ctx *ctx) > > > OVS_REQUIRES(mutex); > > > static void statctrl_notify_main_thread(struct statctrl_ctx > *ctx); > > > -static void statctrl_set_conn_target(const char *br_int_name) > > > - OVS_REQUIRES(mutex); > > > static void statctrl_wait_next_request(struct statctrl_ctx *ctx) > > > OVS_REQUIRES(mutex); > > > static bool statctrl_update_next_request_timestamp(struct > > stats_node *node, > > > @@ -121,7 +118,7 @@ static bool > > statctrl_update_next_request_timestamp(struct stats_node *node, > > > void > > > statctrl_init(void) > > > { > > > - statctrl_ctx.br_int = NULL; > > > + statctrl_ctx.swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > > OFP15_VERSION); > > > latch_init(&statctrl_ctx.exit_latch); > > > ovs_mutex_init(&mutex); > > > statctrl_ctx.thread_seq = seq_create(); > > > @@ -185,11 +182,14 @@ statctrl_run(struct ovsdb_idl_txn > > *ovnsb_idl_txn, > > > } > > > > > > void > > > -statctrl_update(const char *br_int_name) > > > +statctrl_update_swconn(const char *target, int probe_interval) > > > { > > > - ovs_mutex_lock(&mutex); > > > - statctrl_set_conn_target(br_int_name); > > > - ovs_mutex_unlock(&mutex); > > > + if (ovn_update_swconn_at(statctrl_ctx.swconn, target, > > > + probe_interval, "statctrl")) { > > > + /* Notify statctrl thread that integration bridge > > > + * target is set/changed. */ > > > + seq_change(statctrl_ctx.thread_seq); > > > + } > > > } > > > > > > void > > > @@ -217,7 +217,7 @@ statctrl_destroy(void) > > > latch_set(&statctrl_ctx.exit_latch); > > > pthread_join(statctrl_ctx.thread, NULL); > > > latch_destroy(&statctrl_ctx.exit_latch); > > > - free(statctrl_ctx.br_int); > > > + rconn_destroy(statctrl_ctx.swconn); > > > seq_destroy(statctrl_ctx.thread_seq); > > > seq_destroy(statctrl_ctx.main_seq); > > > > > > @@ -233,14 +233,9 @@ statctrl_thread_handler(void *arg) > > > struct statctrl_ctx *ctx = arg; > > > > > > /* OpenFlow connection to the switch. */ > > > - struct rconn *swconn = rconn_create(0, 0, DSCP_DEFAULT, > > > - 1 << OFP15_VERSION); > > > + struct rconn *swconn = ctx->swconn; > > > > > > while (!latch_is_set(&ctx->exit_latch)) { > > > - ovs_mutex_lock(&mutex); > > > - statctrl_rconn_setup(swconn, ctx->br_int); > > > - ovs_mutex_unlock(&mutex); > > > - > > > rconn_run(swconn); > > > uint64_t new_seq = seq_read(ctx->thread_seq); > > > > > > @@ -273,29 +268,9 @@ statctrl_thread_handler(void *arg) > > > poll_block(); > > > } > > > > > > - rconn_destroy(swconn); > > > return NULL; > > > } > > > > > > -static void > > > -statctrl_rconn_setup(struct rconn *swconn, char *br_int) > > > - OVS_REQUIRES(mutex) > > > -{ > > > - if (!br_int) { > > > - rconn_disconnect(swconn); > > > - return; > > > - } > > > - > > > - char *conn_target = xasprintf("unix:%s/%s.mgmt", > > ovs_rundir(), br_int); > > > - > > > - if (strcmp(conn_target, rconn_get_target(swconn))) { > > > - VLOG_INFO("%s: connecting to switch", conn_target); > > > - rconn_connect(swconn, conn_target, conn_target); > > > - } > > > - > > > - free(conn_target); > > > -} > > > - > > > static void > > > statctrl_handle_rconn_msg(struct rconn *swconn, struct > > statctrl_ctx *ctx, > > > struct ofpbuf *msg) > > > @@ -400,23 +375,6 @@ statctrl_notify_main_thread(struct > > statctrl_ctx *ctx) > > > } > > > } > > > > > > -static void > > > -statctrl_set_conn_target(const char *br_int_name) > > > - OVS_REQUIRES(mutex) > > > -{ > > > - if (!br_int_name) { > > > - return; > > > - } > > > - > > > - > > > - if (!statctrl_ctx.br_int || strcmp(statctrl_ctx.br_int, > > br_int_name)) { > > > - free(statctrl_ctx.br_int); > > > - statctrl_ctx.br_int = xstrdup(br_int_name); > > > - /* Notify statctrl thread that integration bridge is > > set/changed. */ > > > - seq_change(statctrl_ctx.thread_seq); > > > - } > > > -} > > > - > > > static void > > > statctrl_wait_next_request(struct statctrl_ctx *ctx) > > > OVS_REQUIRES(mutex) > > > diff --git a/controller/statctrl.h b/controller/statctrl.h > > > index c5cede353..026ff387f 100644 > > > --- a/controller/statctrl.h > > > +++ b/controller/statctrl.h > > > @@ -21,7 +21,8 @@ > > > void statctrl_init(void); > > > void statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > > > struct mac_cache_data *mac_cache_data); > > > -void statctrl_update(const char *br_int_name); > > > + > > > +void statctrl_update_swconn(const char *target, int > probe_interval); > > > void statctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn); > > > void statctrl_destroy(void); > > > > > > diff --git a/include/ovn/features.h b/include/ovn/features.h > > > index 08f1d8288..9e7c88b2a 100644 > > > --- a/include/ovn/features.h > > > +++ b/include/ovn/features.h > > > @@ -49,7 +49,7 @@ enum ovs_feature_value { > > > void ovs_feature_support_destroy(void); > > > bool ovs_feature_is_supported(enum ovs_feature_value feature); > > > bool ovs_feature_support_run(const struct smap > *ovs_capabilities, > > > - const char *br_name); > > > + const char *conn_target, int > > probe_interval); > > > bool ovs_feature_set_discovered(void); > > > uint32_t ovs_feature_max_meters_get(void); > > > uint32_t ovs_feature_max_select_groups_get(void); > > > diff --git a/lib/features.c b/lib/features.c > > > index b04437235..607e4bd31 100644 > > > --- a/lib/features.c > > > +++ b/lib/features.c > > > @@ -29,8 +29,10 @@ > > > #include "openvswitch/ofp-meter.h" > > > #include "openvswitch/ofp-group.h" > > > #include "openvswitch/ofp-util.h" > > > +#include "openvswitch/rconn.h" > > > #include "ovn/features.h" > > > #include "controller/ofctrl.h" > > > +#include "ovn-util.h" > > > > > > VLOG_DEFINE_THIS_MODULE(features); > > > > > > @@ -120,34 +122,12 @@ ovs_feature_is_supported(enum > > ovs_feature_value feature) > > > return supported_ovs_features & feature; > > > } > > > > > > -static void > > > -ovs_feature_rconn_setup(const char *br_name) > > > -{ > > > - if (!swconn) { > > > - swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > > OFP15_VERSION); > > > - } > > > - > > > - if (!rconn_is_connected(swconn)) { > > > - char *target = xasprintf("unix:%s/%s.mgmt", > > ovs_rundir(), br_name); > > > - if (strcmp(target, rconn_get_target(swconn))) { > > > - VLOG_INFO("%s: connecting to switch", target); > > > - rconn_connect(swconn, target, target); > > > - } > > > - free(target); > > > - } > > > -} > > > > > > static bool > > > -ovs_feature_get_openflow_cap(const char *br_name) > > > +ovs_feature_get_openflow_cap(void) > > > { > > > struct ofpbuf *msg; > > > > > > - if (!br_name) { > > > - return false; > > > - } > > > - > > > - ovs_feature_rconn_setup(br_name); > > > - > > > rconn_run(swconn); > > > if (!rconn_is_connected(swconn)) { > > > rconn_run_wait(swconn); > > > @@ -231,7 +211,7 @@ ovs_feature_support_destroy(void) > > > /* Returns 'true' if the set of tracked OVS features has been > > updated. */ > > > bool > > > ovs_feature_support_run(const struct smap *ovs_capabilities, > > > - const char *br_name) > > > + const char *conn_target, int > probe_interval) > > > { > > > static struct smap empty_caps = > SMAP_INITIALIZER(&empty_caps); > > > bool updated = false; > > > @@ -240,7 +220,12 @@ ovs_feature_support_run(const struct smap > > *ovs_capabilities, > > > ovs_capabilities = &empty_caps; > > > } > > > > > > - if (ovs_feature_get_openflow_cap(br_name)) { > > > + if (!swconn) { > > > + swconn = rconn_create(0, 0, DSCP_DEFAULT, 1 << > > OFP15_VERSION); > > > + } > > > + ovn_update_swconn_at(swconn, conn_target, probe_interval, > > "features"); > > > + > > > + if (ovs_feature_get_openflow_cap()) { > > > updated = true; > > > } > > > > > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > > > index ee5cbcdc3..4f50606c0 100644 > > > --- a/lib/ovn-util.c > > > +++ b/lib/ovn-util.c > > > @@ -22,6 +22,7 @@ > > > #include "daemon.h" > > > #include "include/ovn/actions.h" > > > #include "openvswitch/ofp-parse.h" > > > +#include "openvswitch/rconn.h" > > > #include "openvswitch/vlog.h" > > > #include "lib/vswitch-idl.h" > > > #include "ovn-dirs.h" > > > @@ -1284,3 +1285,28 @@ ovn_exit_args_finish(struct ovn_exit_args > > *exit_args) > > > } > > > free(exit_args->conns); > > > } > > > + > > > +bool > > > +ovn_update_swconn_at(struct rconn *swconn, const char *target, > > > + int probe_interval, const char *where) > > > +{ > > > + if (!target) { > > > + rconn_disconnect(swconn); > > > + return true; > > > + } > > > + > > > + bool notify = false; > > > + > > > + if (strcmp(target, rconn_get_target(swconn))) { > > > + VLOG_INFO("%s: connecting to switch: \"%s\"", where, > > target); > > > + rconn_connect(swconn, target, target); > > > + notify = true; > > > + } > > > + > > > + if (probe_interval != rconn_get_probe_interval(swconn)) { > > > + rconn_set_probe_interval(swconn, probe_interval); > > > + notify = true; > > > + } > > > + > > > + return notify; > > > +} > > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > > > index 042e6bf82..f75b821b6 100644 > > > --- a/lib/ovn-util.h > > > +++ b/lib/ovn-util.h > > > @@ -48,6 +48,7 @@ struct smap; > > > struct svec; > > > struct uuid; > > > struct unixctl_conn; > > > +struct rconn; > > > > > > struct ipv4_netaddr { > > > ovs_be32 addr; /* 192.168.10.123 */ > > > @@ -477,4 +478,7 @@ void ovn_exit_command_callback(struct > > unixctl_conn *conn, int argc, > > > const char *argv[], void > > *exit_args_); > > > void ovn_exit_args_finish(struct ovn_exit_args *exit_args); > > > > > > +bool ovn_update_swconn_at(struct rconn *swconn, const char > *target, > > > + int probe_interval, const char *where); > > > + > > > #endif /* OVN_UTIL_H */ > > > diff --git a/lib/test-ovn-features.c b/lib/test-ovn-features.c > > > index aabc547e6..cddeae779 100644 > > > --- a/lib/test-ovn-features.c > > > +++ b/lib/test-ovn-features.c > > > @@ -26,15 +26,15 @@ test_ovn_features(struct ovs_cmdl_context > > *ctx OVS_UNUSED) > > > struct smap features = SMAP_INITIALIZER(&features); > > > > > > smap_add(&features, "ct_zero_snat", "false"); > > > - ovs_assert(!ovs_feature_support_run(&features, NULL)); > > > + ovs_assert(!ovs_feature_support_run(&features, NULL, 0)); > > > > > ovs_assert(!ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)); > > > > > > smap_replace(&features, "ct_zero_snat", "true"); > > > - ovs_assert(ovs_feature_support_run(&features, NULL)); > > > + ovs_assert(ovs_feature_support_run(&features, NULL, 0)); > > > > ovs_assert(ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)); > > > > > > smap_add(&features, "unknown_feature", "true"); > > > - ovs_assert(!ovs_feature_support_run(&features, NULL)); > > > + ovs_assert(!ovs_feature_support_run(&features, NULL, 0)); > > > > > > smap_destroy(&features); > > > } > > > diff --git a/tests/ovn-controller.at <http://ovn-controller.at> > > b/tests/ovn-controller.at <http://ovn-controller.at> > > > index fdcc5aab2..e15cd02b0 100644 > > > --- a/tests/ovn-controller.at <http://ovn-controller.at> > > > +++ b/tests/ovn-controller.at <http://ovn-controller.at> > > > @@ -2874,3 +2874,34 @@ AT_CHECK([test x"$port_uuid"=$(ovs-vsctl > > get port $fakech_tunnel _uuid)]) > > > OVN_CLEANUP([hv1]) > > > AT_CLEANUP > > > ]) > > > + > > > +AT_SETUP([ovn-controller - br-int 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 > > > +ovs-vsctl -- add-port br-int vif1 -- \ > > > + set interface vif1 external-ids:iface-id=vif1 > > > + > > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > > + > > > +# Set the br-int remote and wait for the connection > > > +ovs-vsctl set-controller br-int ptcp:1234 > > > +start_daemon ovn-controller --br-int-remote="tcp:127.0.0.1:1234 > > <http://127.0.0.1:1234>" --br-int-probe-interval=5 > > > + > > > +OVS_WAIT_UNTIL([test $(grep -c 'connecting to switch: > > "tcp:127.0.0.1:1234 <http://127.0.0.1:1234>"' > > hv1/ovn-controller.log) = 4]) > > > +OVS_WAIT_UNTIL([test $(grep -c 'tcp:127.0.0.1:1234 > > <http://127.0.0.1:1234>: connected' hv1/ovn-controller.log) = 4]) > > > + > > > +ovn-nbctl ls-add sw0 > > > + > > > +check ovn-nbctl lsp-add sw0 vif1 > > > +check ovn-nbctl lsp-set-addresses vif1 "00:00:00:00:00:01 > > 192.168.0.10 1000::10" > > > + > > > +wait_for_ports_up > > > +ovn-nbctl --wait=hv sync > > > + > > > +OVN_CLEANUP([hv1]) > > > +AT_CLEANUP > > > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl > > > index 700efe35a..ce83889d8 100755 > > > --- a/utilities/ovn-ctl > > > +++ b/utilities/ovn-ctl > > > @@ -617,6 +617,12 @@ start_controller () { > > > if test X"$OVN_CONTROLLER_SSL_CIPHERS" != X; then > > > set "$@" --ssl-ciphers=$OVN_CONTROLLER_SSL_CIPHERS > > > fi > > > + if test X"$OVN_CONTROLLER_BR_INT_REMOTE" != X; then > > > + set "$@" --br-int-remote=$OVN_CONTROLLER_BR_INT_REMOTE > > > + fi > > > + if test "$OVN_CONTROLLER_BR_INT_PROBE_INTERVAL" != 0; then > > > + set "$@" > > --br-int-probe-interval=$OVN_CONTROLLER_BR_INT_PROBE_INTERVAL > > > + fi > > > > > > [ "$OVN_USER" != "" ] && set "$@" --user "$OVN_USER" > > > > > > @@ -831,6 +837,8 @@ set_defaults () { > > > OVN_USER= > > > > > > OVN_CONTROLLER_LOG="-vconsole:emer -vsyslog:err -vfile:info" > > > + OVN_CONTROLLER_BR_INT_REMOTE="" > > > + OVN_CONTROLLER_BR_INT_PROBE_INTERVAL=0 > > > OVN_NORTHD_LOG="-vconsole:emer -vsyslog:err -vfile:info" > > > OVN_NORTHD_LOGFILE="" > > > OVN_NORTHD_N_THREADS=1 > > > @@ -1041,6 +1049,8 @@ Options: > > > --ovn-controller-ssl-bootstrap-ca-cert=CERT Bootstrapped OVN > > Southbound SSL CA certificate file > > > --ovn-controller-ssl-protocols=PROTOCOLS OVN Southbound SSL > > protocols > > > --ovn-controller-ssl-ciphers=CIPHERS OVN Southbound SSL > > cipher list > > > + --ovn-controller-br-int-remote=REMOTE Active or passive > > connection to br-int > > > + --ovn-controller-br-int-probe-interval=INTERVAL Probe interval > > in seconds for the br-int connection > > > --ovn-nb-db-ssl-key=KEY OVN Northbound DB SSL private key file > > > --ovn-nb-db-ssl-cert=CERT OVN Northbound DB SSL certificate > file > > > --ovn-nb-db-ssl-ca-cert=CERT OVN Northbound DB SSL CA > > certificate file > > > > > > Thanks, > > Ales > > -- > > > > Ales Musil > > > > Senior Software Engineer - OVN Core > > > > Red Hat EMEA <https://www.redhat.com> > > > > amu...@redhat.com <mailto:amu...@redhat.com> > > > > <https://red.ht/sig> > > > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
