On Thu, Apr 4, 2024 at 9:56 PM Mark Michelson <mmich...@redhat.com> wrote:

> Hi Ales,
>
> The patch looks good to me except for one problem.
>
> On 4/3/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 vswitchd external-ids that allows to specify remote
> > target for management bridge. This gives the user possibility to
> > connect to management bridge 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 <amu...@redhat.com>
> > ---
> > v2: Rebase on top of current main.
> >      Make the probe interval accept milliseconds to be aligned with
> other probe intervals.
> >      Use external-ids instead of options for the ovn-controller.
> > ---
> >   NEWS                            |  6 +++
> >   controller/ofctrl.c             | 10 +----
> >   controller/ofctrl.h             |  5 ++-
> >   controller/ovn-controller.8.xml | 15 ++++++++
> >   controller/ovn-controller.c     | 59 +++++++++++++++++++++++------
> >   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         | 45 ++++++++++++++++++++++
> >   15 files changed, 193 insertions(+), 151 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 141f1831c..4663780a8 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -13,6 +13,12 @@ Post v24.03.0
> >       "lflow-stage-to-oftable STAGE_NAME" that converts stage name into
> OpenFlow
> >       table id.
> >     - Rename the ovs-sandbox script to ovn-sandbox.
> > +  - Add "ovn-bridge-remote" config option to vswitchd external-ids,
> > +    that allows to specify connection method to management bridge for
> > +    ovn-controller, defaulting to the unix socket.
> > +  - Add "ovn-bridge-remote-probe-interval" config option to vswitchd
> > +    external-ids, 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 6a2564604..9d181a782 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -771,19 +771,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..7ae98e4b0 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -388,6 +388,21 @@
> >           cap for the exponential backoff used by
> <code>ovn-controller</code>
> >           to send GARPs packets.
> >         </dd>
> > +      <dt><code>external_ids:ovn-bridge-remote</code></dt>
> > +      <dd>
> > +        <p>
> > +          Connection to the OVN management bridge in OvS. It defaults to
> > +          <code>unix:<var>br-int</var>.mgmt</code> when not specified.
> > +        </p>
> > +      </dd>
> > +      <dt><code>external_ids:ovn-bridge-remote</code></dt>
>
> The name of this option is "ovn-bridge-remote-probe-interval"
>

Oops copy-pasta I'll fix it in v3.


>
> > +      <dd>
> > +        <p>
> > +          The inactivity probe interval of the connection to the OVN
> management
> > +          bridge, in milliseconds.
> > +          If the value is zero, it disables the inactivity probe.
> > +        </p>
> > +      </dd>
> >       </dl>
> >
> >       <p>
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index c9ff5967a..134911a29 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -122,6 +122,11 @@ 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"
> >
> > +struct br_int_remote {
> > +    char *target;
> > +    int probe_interval;
> > +};
> > +
> >   static char *parse_options(int argc, char *argv[]);
> >   OVS_NO_RETURN static void usage(void);
> >
> > @@ -5052,11 +5057,43 @@ 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,
> > +                     const struct ovsrec_open_vswitch_table *ovs_table)
> > +{
> > +    if (!br_int) {
> > +        return;
> > +    }
> > +
> > +    const struct ovsrec_open_vswitch *cfg =
> > +            ovsrec_open_vswitch_table_first(ovs_table);
> > +
> > +    const char *ext_target =
> > +            smap_get(&cfg->external_ids, "ovn-bridge-remote");
> > +    char *target = ext_target
> > +            ? xstrdup(ext_target)
> > +            : xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
> > +
> > +    if (!remote->target || strcmp(remote->target, target)) {
> > +        free(remote->target);
> > +        remote->target = target;
> > +    } else {
> > +        free(target);
> > +    }
> > +
> > +    unsigned long long probe_interval =
> > +            smap_get_ullong(&cfg->external_ids,
> > +                            "ovn-bridge-remote-probe-interval", 0);
> > +    remote->probe_interval = MIN(probe_interval / 1000, INT_MAX);
> > +}
> > +
> >   int
> >   main(int argc, char *argv[])
> >   {
> >       struct unixctl_server *unixctl;
> >       struct ovn_exit_args exit_args = {0};
> > +    struct br_int_remote br_int_remote = {0};
> >       int retval;
> >
> >       /* Read from system-id-override file once on startup. */
> > @@ -5666,6 +5703,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, ovs_table);
> > +        statctrl_update_swconn(br_int_remote.target,
> > +                               br_int_remote.probe_interval);
> > +        pinctrl_update_swconn(br_int_remote.target,
> > +                              br_int_remote.probe_interval);
> >
> >           /* Enable ACL matching for double tagged traffic. */
> >           if (ovs_idl_txn) {
> > @@ -5720,7 +5762,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 +5781,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 +5899,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 +5953,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 +6073,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 +6205,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);
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index 9a4deb80a..f0a7dce7a 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();
> > @@ -3523,30 +3524,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;
> > @@ -3562,13 +3546,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);
> >
> > @@ -3626,37 +3607,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);
> > @@ -4278,7 +4246,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 3202f0bef..df84f5bdf 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -2879,3 +2879,48 @@ 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
> > +
> > +# Set the br-int remote and wait for the connection
> > +check ovs-vsctl set-controller br-int ptcp:1234
> > +check ovs-vsctl -- \
> > +    set open . external-ids:ovn-bridge-remote="tcp:127.0.0.1:1234" -- \
> > +    set open . external-ids:ovn-bridge-remote-probe-interval=5000
> > +
> > +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])
> > +
> > +check 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
> > +check ovn-nbctl --wait=hv sync
> > +
> > +check ovs-vsctl -- \
> > +    remove open . external-ids ovn-bridge-remote -- \
> > +    remove open . external-ids ovn-bridge-remote-probe-interval
> > +
> > +check ovs-vsctl del-controller br-int
> > +
> > +# Set different br-int remote and wait for the connection
> > +check ovs-vsctl set-controller br-int ptcp:1235
> > +check ovs-vsctl -- \
> > +    set open . external-ids:ovn-bridge-remote="tcp:127.0.0.1:1235"
> > +
> > +OVS_WAIT_UNTIL([test $(grep -c 'connecting to switch: "tcp:
> 127.0.0.1:1235"' hv1/ovn-controller.log) = 4])
> > +OVS_WAIT_UNTIL([test $(grep -c 'tcp:127.0.0.1:1235: connected'
> hv1/ovn-controller.log) = 4])
> > +
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
>
>
Thanks,
Ales
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to