[ovs-dev] controller: Allow br-int connection via other methods.

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
Signed-off-by: Ales Musil <amusil@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         | 31 +++++++++++++++
 utilities/ovn-ctl               | 10 +++++
 16 files changed, 192 insertions(+), 154 deletions(-)

Hi Ales,

I have some high-level suggestions/questions.

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.

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.

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?

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
> Signed-off-by: Ales Musil <amusil@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         | 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"/>
> 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 b/tests/ovn-controller.at
> index fdcc5aab2..e15cd02b0 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/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" --br-int-probe-interval=5
> +
> +OVS_WAIT_UNTIL([test $(grep -c 'connecting to switch: "tcp:127.0.0.1:1234"' hv1/ovn-controller.log) = 4])
> +OVS_WAIT_UNTIL([test $(grep -c 'tcp: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

On Mon, Apr 1, 2024 at 10:36 PM Mark Michelson <mmichels@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.


>
> 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.


> 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
> > Signed-off-by: Ales Musil <amusil@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         | 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"/>
> > 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 b/tests/ovn-controller.at
> > index fdcc5aab2..e15cd02b0 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/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"
> --br-int-probe-interval=5
> > +
> > +OVS_WAIT_UNTIL([test $(grep -c 'connecting to switch: "tcp:
> 127.0.0.1:1234"' hv1/ovn-controller.log) = 4])
> > +OVS_WAIT_UNTIL([test $(grep -c 'tcp: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

On 4/2/24 01:50, Ales Musil wrote:
> 
> 
> On Mon, Apr 1, 2024 at 10:36 PM Mark Michelson <mmichels@redhat.com 
> <mailto:mmichels@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.

> 
> 
>     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.

> 
> 
>     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 <amusil@redhat.com
>     <mailto:amusil@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>
> 
> amusil@redhat.com <mailto:amusil@redhat.com>
> 
> <https://red.ht/sig>
>

On Tue, Apr 2, 2024 at 2:47 PM Mark Michelson <mmichels@redhat.com> wrote:

> On 4/2/24 01:50, Ales Musil wrote:
> >
> >
> > On Mon, Apr 1, 2024 at 10:36 PM Mark Michelson <mmichels@redhat.com
> > <mailto:mmichels@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 <amusil@redhat.com
> >     <mailto:amusil@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>
> >
> > amusil@redhat.com <mailto:amusil@redhat.com>
> >
> > <https://red.ht/sig>
> >
>
>
Thanks,
Ales

On 3/28/24 09: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
> Signed-off-by: Ales Musil <amusil@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         | 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.

I didn't review the code, but I don't think the names should be changed.
There is nothing user-facing called "br-int" and any bridge name can be
chosen for a deployment.  It should be called "ovn-bridge-mgmt-remote" or
something like that.

Best regards, Ilya Maximets.

On 4/2/24 15:02, Ilya Maximets wrote:
> On 3/28/24 09: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
>> Signed-off-by: Ales Musil <amusil@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         | 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.
> 
> I didn't review the code, but I don't think the names should be changed.

s/don't// :)

> There is nothing user-facing called "br-int" and any bridge name can be
> chosen for a deployment.  It should be called "ovn-bridge-mgmt-remote" or
> something like that.
> 
> Best regards, Ilya Maximets.
>

On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 3/28/24 09: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
> > Signed-off-by: Ales Musil <amusil@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         | 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.
>
> I didn't review the code, but I don't think the names should be changed.
> There is nothing user-facing called "br-int" and any bridge name can be
> chosen for a deployment.  It should be called "ovn-bridge-mgmt-remote" or
> something like that.
>

I'm fine either way, I just went with br-int because the code around that
is actually called br-int even if we have different management bridge name.


>
> Best regards, Ilya Maximets.
>
>
Thanks,
Ales

On 4/2/24 15:36, Ales Musil wrote:
> 
> 
> On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 3/28/24 09: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 <amusil@redhat.com <mailto:amusil@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.
> 
>     I didn't review the code, but I don't think the names should be changed.
>     There is nothing user-facing called "br-int" and any bridge name can be
>     chosen for a deployment.  It should be called "ovn-bridge-mgmt-remote" or
>     something like that.
> 
> 
> I'm fine either way, I just went with br-int because the code around that is actually
> called br-int even if we have different management bridge name.

The point is - users do not know how the code looks like
and they may have nothing called 'br-int' in their setup.

Also, maybe these should be a database configuration instead
of command line arguments?  Alongside the 'ovn-bridge'.

Best regards, Ilya Maximets.

On Tue, Apr 2, 2024 at 4:03 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 4/2/24 15:36, Ales Musil wrote:
> >
> >
> > On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org
> <mailto:i.maximets@ovn.org>> wrote:
> >
> >     On 3/28/24 09: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 <amusil@redhat.com <mailto:
> amusil@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.
> >
> >     I didn't review the code, but I don't think the names should be
> changed.
> >     There is nothing user-facing called "br-int" and any bridge name can
> be
> >     chosen for a deployment.  It should be called
> "ovn-bridge-mgmt-remote" or
> >     something like that.
> >
> >
> > I'm fine either way, I just went with br-int because the code around
> that is actually
> > called br-int even if we have different management bridge name.
>
> The point is - users do not know how the code looks like
> and they may have nothing called 'br-int' in their setup.
>

Sure that is fine I can change the user facing part.


>
> Also, maybe these should be a database configuration instead
> of command line arguments?  Alongside the 'ovn-bridge'.
>

So having in DB means we have to react to any change, the question is, do
we expect uset to change this configuration often? Also "ovs-database" is
also just an argument to the ovn-controller.


>
> Best regards, Ilya Maximets.
>
>
Thanks,
Ales

On 4/2/24 16:17, Ales Musil wrote:
> 
> 
> On Tue, Apr 2, 2024 at 4:03 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote:
> 
>     On 4/2/24 15:36, Ales Musil wrote:
>     >
>     >
>     > On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org> <mailto:i.maximets@ovn.org <mailto:i.maximets@ovn.org>>> wrote:
>     >
>     >     On 3/28/24 09: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> <https://issues.redhat.com/browse/FDP-243 <https://issues.redhat.com/browse/FDP-243>>
>     >     > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:amusil@redhat.com> <mailto:amusil@redhat.com <mailto:amusil@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> <http://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.
>     >
>     >     I didn't review the code, but I don't think the names should be changed.
>     >     There is nothing user-facing called "br-int" and any bridge name can be
>     >     chosen for a deployment.  It should be called "ovn-bridge-mgmt-remote" or
>     >     something like that.
>     >
>     >
>     > I'm fine either way, I just went with br-int because the code around that is actually
>     > called br-int even if we have different management bridge name.
> 
>     The point is - users do not know how the code looks like
>     and they may have nothing called 'br-int' in their setup.
> 
> 
> Sure that is fine I can change the user facing part.
>  
> 
> 
>     Also, maybe these should be a database configuration instead
>     of command line arguments?  Alongside the 'ovn-bridge'.
> 
> 
> So having in DB means we have to react to any change, the question is, do we expect
> uset to change this configuration often?

Do we react to 'ovn-bridge' changes?  I guess, ovn-controller can react to
connection changes the same way.  This configuration should not change
frequently, IMO.

> Also "ovs-database" is also just an argument to the ovn-controller.

Database connection and OF connection are different things, and we have
to know how to connect to the database.  The model until now was to have
a single command line argument - database connection method - and store
all the other configuration in the database.

>  
> 
> 
>     Best regards, Ilya Maximets.
> 
> 
> Thanks,
> Ales
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA <https://www.redhat.com>
> 
> amusil@redhat.com <mailto:amusil@redhat.com> 
> 
> <https://red.ht/sig>
>

On Tue, Apr 2, 2024 at 4:25 PM Ilya Maximets <i.maximets@ovn.org> wrote:

> On 4/2/24 16:17, Ales Musil wrote:
> >
> >
> > On Tue, Apr 2, 2024 at 4:03 PM Ilya Maximets <i.maximets@ovn.org
> <mailto:i.maximets@ovn.org>> wrote:
> >
> >     On 4/2/24 15:36, Ales Musil wrote:
> >     >
> >     >
> >     > On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org
> <mailto:i.maximets@ovn.org> <mailto:i.maximets@ovn.org <mailto:
> i.maximets@ovn.org>>> wrote:
> >     >
> >     >     On 3/28/24 09: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> <
> https://issues.redhat.com/browse/FDP-243 <
> https://issues.redhat.com/browse/FDP-243>>
> >     >     > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:
> amusil@redhat.com> <mailto:amusil@redhat.com <mailto:amusil@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> <
> http://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.
> >     >
> >     >     I didn't review the code, but I don't think the names should
> be changed.
> >     >     There is nothing user-facing called "br-int" and any bridge
> name can be
> >     >     chosen for a deployment.  It should be called
> "ovn-bridge-mgmt-remote" or
> >     >     something like that.
> >     >
> >     >
> >     > I'm fine either way, I just went with br-int because the code
> around that is actually
> >     > called br-int even if we have different management bridge name.
> >
> >     The point is - users do not know how the code looks like
> >     and they may have nothing called 'br-int' in their setup.
> >
> >
> > Sure that is fine I can change the user facing part.
> >
> >
> >
> >     Also, maybe these should be a database configuration instead
> >     of command line arguments?  Alongside the 'ovn-bridge'.
> >
> >
> > So having in DB means we have to react to any change, the question is,
> do we expect
> > uset to change this configuration often?
>
> Do we react to 'ovn-bridge' changes?  I guess, ovn-controller can react to
> connection changes the same way.  This configuration should not change
> frequently, IMO.
>

I'll let others chime in, again I don't have a strong preference for this
one, it can be either of those.


>
> > Also "ovs-database" is also just an argument to the ovn-controller.
>
> Database connection and OF connection are different things, and we have
> to know how to connect to the database.  The model until now was to have
> a single command line argument - database connection method - and store
> all the other configuration in the database.
>
> >
> >
> >
> >     Best regards, Ilya Maximets.
> >
> >
> > Thanks,
> > Ales
> > --
> >
> > Ales Musil
> >
> > Senior Software Engineer - OVN Core
> >
> > Red Hat EMEA <https://www.redhat.com>
> >
> > amusil@redhat.com <mailto:amusil@redhat.com>
> >
> > <https://red.ht/sig>
> >
>
>
Thanks,
Ales

On 4/2/24 16:32, Ales Musil wrote:
> On Tue, Apr 2, 2024 at 4:25 PM Ilya Maximets <i.maximets@ovn.org> wrote:
> 
>> On 4/2/24 16:17, Ales Musil wrote:
>>>
>>>
>>> On Tue, Apr 2, 2024 at 4:03 PM Ilya Maximets <i.maximets@ovn.org
>> <mailto:i.maximets@ovn.org>> wrote:
>>>
>>>     On 4/2/24 15:36, Ales Musil wrote:
>>>     >
>>>     >
>>>     > On Tue, Apr 2, 2024 at 3:02 PM Ilya Maximets <i.maximets@ovn.org
>> <mailto:i.maximets@ovn.org> <mailto:i.maximets@ovn.org <mailto:
>> i.maximets@ovn.org>>> wrote:
>>>     >
>>>     >     On 3/28/24 09: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> <
>> https://issues.redhat.com/browse/FDP-243 <
>> https://issues.redhat.com/browse/FDP-243>>
>>>     >     > Signed-off-by: Ales Musil <amusil@redhat.com <mailto:
>> amusil@redhat.com> <mailto:amusil@redhat.com <mailto:amusil@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> <
>> http://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.
>>>     >
>>>     >     I didn't review the code, but I don't think the names should
>> be changed.
>>>     >     There is nothing user-facing called "br-int" and any bridge
>> name can be
>>>     >     chosen for a deployment.  It should be called
>> "ovn-bridge-mgmt-remote" or
>>>     >     something like that.
>>>     >
>>>     >
>>>     > I'm fine either way, I just went with br-int because the code
>> around that is actually
>>>     > called br-int even if we have different management bridge name.
>>>
>>>     The point is - users do not know how the code looks like
>>>     and they may have nothing called 'br-int' in their setup.
>>>
>>>
>>> Sure that is fine I can change the user facing part.
>>>
>>>
>>>
>>>     Also, maybe these should be a database configuration instead
>>>     of command line arguments?  Alongside the 'ovn-bridge'.
>>>
>>>
>>> So having in DB means we have to react to any change, the question is,
>> do we expect
>>> uset to change this configuration often?
>>
>> Do we react to 'ovn-bridge' changes?  I guess, ovn-controller can react to
>> connection changes the same way.  This configuration should not change
>> frequently, IMO.
>>
> 
> I'll let others chime in, again I don't have a strong preference for this
> one, it can be either of those.
> 

If I may, I vote for storing the OF connection method as external ID in
the OVS database.  I agree with Ilya, the "br-int" bridge name
(ovn-bridge) is already there, it makes the most sense to keep
everything together in one place.

> 
>>
>>> Also "ovs-database" is also just an argument to the ovn-controller.
>>
>> Database connection and OF connection are different things, and we have
>> to know how to connect to the database.  The model until now was to have
>> a single command line argument - database connection method - and store
>> all the other configuration in the database.
>>
>>>
>>>
>>>
>>>     Best regards, Ilya Maximets.
>>>
>>>
>>> Thanks,
>>> Ales
>>> --

Regards,
Dumitru