diff mbox series

[ovs-dev,5/6] vswitchd: Allow user to configure controllers as "primary" or "service".

Message ID 20181029225751.5936-5-blp@ovn.org
State Accepted
Headers show
Series [ovs-dev,1/6] connmgr: Modernize coding style. | expand

Commit Message

Ben Pfaff Oct. 29, 2018, 10:57 p.m. UTC
Normally it makes sense for an active connection to be primary and a
passive connection to be a service connection, but I've run into a corner
case where it is better for a passive connection to be a primary
connection.  This specific case is for use with OFtest, which expects to be
a primary controller.  However, it also wants to reconnect frequently,
which is slow for active connections because of the backoff; by
configuring a passive, primary controller, OFtest can reconnect as
frequently and as quickly as it wants, making the overall test much faster.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/connmgr.c          |  29 +++++-------
 ofproto/connmgr.h          |  21 ---------
 ofproto/ofproto.c          |   6 +++
 ofproto/ofproto.h          |  18 ++++++++
 vswitchd/bridge.c          |  11 +++++
 vswitchd/vswitch.ovsschema |   8 +++-
 vswitchd/vswitch.xml       | 111 ++++++++++++++++++++++-----------------------
 7 files changed, 106 insertions(+), 98 deletions(-)

Comments

Justin Pettit Feb. 5, 2019, 8:50 p.m. UTC | #1
> On Oct 29, 2018, at 3:57 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> Normally it makes sense for an active connection to be primary and a
> passive connection to be a service connection, but I've run into a corner
> case where it is better for a passive connection to be a primary
> connection.  This specific case is for use with OFtest, which expects to be
> a primary controller.  However, it also wants to reconnect frequently,
> which is slow for active connections because of the backoff; by
> configuring a passive, primary controller, OFtest can reconnect as
> frequently and as quickly as it wants, making the overall test much faster.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>

Nice.  Have you made the necessary changes for OFtest?

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Feb. 5, 2019, 9:54 p.m. UTC | #2
On Tue, Feb 05, 2019 at 12:50:16PM -0800, Justin Pettit wrote:
> 
> > On Oct 29, 2018, at 3:57 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > Normally it makes sense for an active connection to be primary and a
> > passive connection to be a service connection, but I've run into a corner
> > case where it is better for a passive connection to be a primary
> > connection.  This specific case is for use with OFtest, which expects to be
> > a primary controller.  However, it also wants to reconnect frequently,
> > which is slow for active connections because of the backoff; by
> > configuring a passive, primary controller, OFtest can reconnect as
> > frequently and as quickly as it wants, making the overall test much faster.
> > 
> > Signed-off-by: Ben Pfaff <blp@ovn.org>
> 
> Nice.  Have you made the necessary changes for OFtest?

I've been holding back further patches until these got in.
diff mbox series

Patch

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 30d543d220e5..1674db1594a8 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -142,7 +142,7 @@  struct ofconn {
 
 static unsigned int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT;
 
-static void ofconn_create(struct ofservice *, struct rconn *, enum ofconn_type,
+static void ofconn_create(struct ofservice *, struct rconn *,
                           const struct ofproto_controller *settings)
     OVS_EXCLUDED(ofproto_mutex);
 static void ofconn_destroy(struct ofconn *) OVS_REQUIRES(ofproto_mutex);
@@ -1150,7 +1150,7 @@  bundle_remove_expired(struct ofconn *ofconn, long long int now)
 
 static void
 ofconn_create(struct ofservice *ofservice, struct rconn *rconn,
-              enum ofconn_type type, const struct ofproto_controller *settings)
+              const struct ofproto_controller *settings)
     OVS_EXCLUDED(ofproto_mutex)
 {
     ovs_mutex_lock(&ofproto_mutex);
@@ -1164,7 +1164,7 @@  ofconn_create(struct ofservice *ofservice, struct rconn *rconn,
     ovs_list_push_back(&ofservice->conns, &ofconn->ofservice_node);
 
     ofconn->rconn = rconn;
-    ofconn->type = type;
+    ofconn->type = settings->type;
     ofconn->band = settings->band;
 
     ofconn->role = OFPCR12_ROLE_EQUAL;
@@ -1708,8 +1708,9 @@  connmgr_get_max_probe_interval(const struct connmgr *mgr)
     return max_probe_interval;
 }
 
-/* Returns the number of seconds for which all of 'mgr's primary controllers
- * have been disconnected.  Returns 0 if 'mgr' has no primary controllers. */
+/* Returns the number of seconds for which all of 'mgr's active, primary
+ * controllers have been disconnected.  Returns 0 if 'mgr' has no active,
+ * primary controllers. */
 int
 connmgr_failure_duration(const struct connmgr *mgr)
 {
@@ -1717,7 +1718,7 @@  connmgr_failure_duration(const struct connmgr *mgr)
 
     struct ofservice *ofservice;
     HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) {
-        if (ofservice->rconn) {
+        if (ofservice->s.type == OFCONN_PRIMARY && ofservice->rconn) {
             int failure_duration = rconn_failure_duration(ofservice->rconn);
             min_failure_duration = MIN(min_failure_duration, failure_duration);
         }
@@ -1734,7 +1735,8 @@  connmgr_is_any_controller_connected(const struct connmgr *mgr)
 {
     struct ofservice *ofservice;
     HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) {
-        if (ofservice->rconn && rconn_is_connected(ofservice->rconn)) {
+        if (ofservice->s.type == OFCONN_PRIMARY
+            && !ovs_list_is_empty(&ofservice->conns)) {
             return true;
         }
     }
@@ -1905,7 +1907,7 @@  ofservice_create(struct connmgr *mgr, const char *target,
     ofservice->connmgr = mgr;
     ofservice->target = xstrdup(target);
     ovs_list_init(&ofservice->conns);
-    ofservice->type = rconn ? OFCONN_PRIMARY : OFCONN_SERVICE;
+    ofservice->type = c->type;
     ofservice->rconn = rconn;
     ofservice->pvconn = pvconn;
     ofservice->s = *c;
@@ -1962,7 +1964,7 @@  ofservice_run(struct ofservice *ofservice)
             rconn_connect_unreliably(rconn, vconn, name);
             free(name);
 
-            ofconn_create(ofservice, rconn, OFCONN_SERVICE, &ofservice->s);
+            ofconn_create(ofservice, rconn, &ofservice->s);
         } else if (retval != EAGAIN) {
             VLOG_WARN_RL(&rl, "accept failed (%s)", ovs_strerror(retval));
         }
@@ -1972,8 +1974,7 @@  ofservice_run(struct ofservice *ofservice)
         bool connected = rconn_is_connected(ofservice->rconn);
         bool has_ofconn = !ovs_list_is_empty(&ofservice->conns);
         if (connected && !has_ofconn) {
-            ofconn_create(ofservice, ofservice->rconn, OFCONN_PRIMARY,
-                          &ofservice->s);
+            ofconn_create(ofservice, ofservice->rconn, &ofservice->s);
         }
     }
 }
@@ -2287,12 +2288,6 @@  ofmonitor_wait(struct connmgr *mgr)
     ovs_mutex_unlock(&ofproto_mutex);
 }
 
-const char *
-ofconn_type_to_string(enum ofconn_type type)
-{
-    return type == OFCONN_PRIMARY ? "primary" : "service";
-}
-
 void
 ofproto_async_msg_free(struct ofproto_async_msg *am)
 {
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 11c8f9a85121..46f75b3dcf2f 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -37,27 +37,6 @@  struct rule;
 struct simap;
 struct sset;
 
-/* ofproto supports two kinds of OpenFlow connections:
- *
- *   - "Primary" connections to ordinary OpenFlow controllers.  ofproto
- *     maintains persistent connections to these controllers and by default
- *     sends them asynchronous messages such as packet-ins.
- *
- *   - "Service" connections, e.g. from ovs-ofctl.  When these connections
- *     drop, it is the other side's responsibility to reconnect them if
- *     necessary.  ofproto does not send them asynchronous messages by default.
- *
- * Currently, active (tcp, ssl, unix) connections are always "primary"
- * connections and passive (ptcp, pssl, punix) connections are always "service"
- * connections.  There is no inherent reason for this, but it reflects the
- * common case.
- */
-enum ofconn_type {
-    OFCONN_PRIMARY,             /* An ordinary OpenFlow controller. */
-    OFCONN_SERVICE              /* A service connection, e.g. "ovs-ofctl". */
-};
-const char *ofconn_type_to_string(enum ofconn_type);
-
 /* An asynchronous message that might need to be queued between threads. */
 struct ofproto_async_msg {
     struct ovs_list list_node;  /* For queuing. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 222c749940ec..29a40cd539f5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1892,6 +1892,12 @@  ofproto_free_ofproto_controller_info(struct shash *info)
     connmgr_free_controller_info(info);
 }
 
+const char *
+ofconn_type_to_string(enum ofconn_type type)
+{
+    return type == OFCONN_PRIMARY ? "primary" : "service";
+}
+
 /* Makes a deep copy of 'old' into 'port'. */
 void
 ofproto_port_clone(struct ofproto_port *port, const struct ofproto_port *old)
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 595729dd61f1..50208d481862 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -207,7 +207,25 @@  enum ofproto_band {
     OFPROTO_OUT_OF_BAND         /* Out-of-band connection to controller. */
 };
 
+/* ofproto supports two kinds of OpenFlow connections:
+ *
+ *   - "Primary" connections to ordinary OpenFlow controllers.  ofproto
+ *     maintains persistent connections to these controllers and by default
+ *     sends them asynchronous messages such as packet-ins.
+ *
+ *   - "Service" connections, e.g. from ovs-ofctl.  When these connections
+ *     drop, it is the other side's responsibility to reconnect them if
+ *     necessary.  ofproto does not send them asynchronous messages by default.
+ */
+enum ofconn_type {
+    OFCONN_PRIMARY,             /* An ordinary OpenFlow controller. */
+    OFCONN_SERVICE              /* A service connection, e.g. "ovs-ofctl". */
+};
+const char *ofconn_type_to_string(enum ofconn_type);
+
+/* Configuration for an OpenFlow controller. */
 struct ofproto_controller {
+    enum ofconn_type type;      /* Primary or service controller. */
     int max_backoff;            /* Maximum reconnection backoff, in seconds. */
     int probe_interval;         /* Max idle time before probing, in seconds. */
     enum ofproto_band band;     /* In-band or out-of-band? */
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index de5793dd03e4..a427b0122b49 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -46,6 +46,7 @@ 
 #include "openvswitch/meta-flow.h"
 #include "openvswitch/ofp-print.h"
 #include "openvswitch/ofpbuf.h"
+#include "openvswitch/vconn.h"
 #include "openvswitch/vlog.h"
 #include "ovs-lldp.h"
 #include "ovs-numa.h"
@@ -3536,6 +3537,14 @@  equal_pathnames(const char *a, const char *b, size_t b_stoplen)
     }
 }
 
+static enum ofconn_type
+get_controller_ofconn_type(const char *target, const char *type)
+{
+    return (type
+            ? (!strcmp(type, "primary") ? OFCONN_PRIMARY : OFCONN_SERVICE)
+            : (!vconn_verify_name(target) ? OFCONN_PRIMARY : OFCONN_SERVICE));
+}
+
 static void
 bridge_configure_remotes(struct bridge *br,
                          const struct sockaddr_in *managers, size_t n_managers)
@@ -3571,6 +3580,7 @@  bridge_configure_remotes(struct bridge *br,
     /* Add managment controller. */
     struct ofproto_controller *oc = xmalloc(sizeof *oc);
     *oc = (struct ofproto_controller) {
+        .type = OFCONN_SERVICE,
         .probe_interval = 60,
         .band = OFPROTO_OUT_OF_BAND,
         .enable_async_msgs = true,
@@ -3639,6 +3649,7 @@  bridge_configure_remotes(struct bridge *br,
 
         oc = xmalloc(sizeof *oc);
         *oc = (struct ofproto_controller) {
+            .type = get_controller_ofconn_type(c->target, c->type),
             .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8,
             .probe_interval = (c->inactivity_probe
                                ? *c->inactivity_probe / 1000 : 5),
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 1e4e342dcb96..9be4ca54ecd3 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@ 
 {"name": "Open_vSwitch",
- "version": "7.16.1",
- "cksum": "1452282319 23860",
+ "version": "7.17.0",
+ "cksum": "2073324140 24021",
  "tables": {
    "Open_vSwitch": {
      "columns": {
@@ -548,6 +548,10 @@ 
      "indexes": [["id", "bridge"]]},
    "Controller": {
      "columns": {
+       "type": {
+         "type": {"key": {"type": "string",
+                  "enum": ["set", ["primary", "service"]]},
+                  "min": 0, "max": 1}},
        "target": {
          "type": "string"},
        "max_backoff": {
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 6d1fc1c1c7b0..08bec290ae18 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -967,9 +967,12 @@ 
           avoid loops on such a bridge, configure <code>secure</code> mode or
           enable STP (see <ref column="stp_enable"/>).
         </p>
-        <p>When more than one controller is configured,
-        <ref column="fail_mode"/> is considered only when none of the
-        configured controllers can be contacted.</p>
+        <p>
+          The <ref column="fail_mode"/> setting applies only to primary
+          controllers.  When more than one primary controller is configured,
+          <ref column="fail_mode"/> is considered only when none of the
+          configured controllers can be contacted.
+        </p>
         <p>
           Changing <ref column="fail_mode"/> when no primary controllers are
           configured clears the OpenFlow flow tables, group table, and meter
@@ -4453,71 +4456,64 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
   <table name="Controller" title="OpenFlow controller configuration.">
     <p>An OpenFlow controller.</p>
 
-    <p>
-      Open vSwitch supports two kinds of OpenFlow controllers:
-    </p>
-
-    <dl>
-      <dt>Primary controllers</dt>
-      <dd>
+    <group title="Core Features">
+      <column name="type">
         <p>
-          This is the kind of controller envisioned by the OpenFlow 1.0
-          specification.  Usually, a primary controller implements a network
-          policy by taking charge of the switch's flow table.
+          Open vSwitch supports two kinds of OpenFlow controllers.  A bridge
+          may have any number of each kind:
         </p>
 
-        <p>
-          Open vSwitch initiates and maintains persistent connections to
-          primary controllers, retrying the connection each time it fails or
-          drops.  The <ref table="Bridge" column="fail_mode"/> column in the
-          <ref table="Bridge"/> table applies to primary controllers.
-        </p>
+        <dl>
+          <dt>Primary controllers</dt>
+          <dd>
+            <p>
+              This is the kind of controller envisioned by the OpenFlow
+              specifications.  Usually, a primary controller implements a
+              network policy by taking charge of the switch's flow table.
+            </p>
 
-        <p>
-          Open vSwitch permits a bridge to have any number of primary
-          controllers.  When multiple controllers are configured, Open
-          vSwitch connects to all of them simultaneously.  Because
-          OpenFlow 1.0 does not specify how multiple controllers
-          coordinate in interacting with a single switch, more than
-          one primary controller should be specified only if the
-          controllers are themselves designed to coordinate with each
-          other.  (The Nicira-defined <code>NXT_ROLE</code> OpenFlow
-          vendor extension may be useful for this.)
-        </p>
-      </dd>
-      <dt>Service controllers</dt>
-      <dd>
-        <p>
-          These kinds of OpenFlow controller connections are intended for
-          occasional support and maintenance use, e.g. with
-          <code>ovs-ofctl</code>.  Usually a service controller connects only
-          briefly to inspect or modify some of a switch's state.
-        </p>
+            <p>
+              The <ref table="Bridge" column="fail_mode"/> column in the <ref
+              table="Bridge"/> table applies to primary controllers.
+            </p>
 
-        <p>
-          Open vSwitch listens for incoming connections from service
-          controllers.  The service controllers initiate and, if necessary,
-          maintain the connections from their end.  The <ref table="Bridge"
-          column="fail_mode"/> column in the <ref table="Bridge"/> table does
-          not apply to service controllers.
-        </p>
+            <p>
+              When multiple primary controllers are configured, Open vSwitch
+              connects to all of them simultaneously.  OpenFlow provides few
+              facilities to allow multiple controllers to coordinate in
+              interacting with a single switch, so more than one primary
+              controller should be specified only if the controllers are
+              themselves designed to coordinate with each other.
+            </p>
+          </dd>
+          <dt>Service controllers</dt>
+          <dd>
+            <p>
+              These kinds of OpenFlow controller connections are intended for
+              occasional support and maintenance use, e.g. with
+              <code>ovs-ofctl</code>.  Usually a service controller connects
+              only briefly to inspect or modify some of a switch's state.
+            </p>
+
+            <p>
+              The <ref table="Bridge" column="fail_mode"/> column in the <ref
+              table="Bridge"/> table does not apply to service controllers.
+            </p>
+          </dd>
+        </dl>
 
         <p>
-          Open vSwitch supports configuring any number of service controllers.
+          By default, Open vSwitch treats controllers with active connection
+          methods as primary controllers and those with passive connection
+          methods as service controllers.  Set this column to the desired type
+          to override this default.
         </p>
-      </dd>
-    </dl>
-
-    <p>
-      The <ref column="target"/> determines the type of controller.
-    </p>
+      </column>
 
-    <group title="Core Features">
       <column name="target">
         <p>Connection method for controller.</p>
         <p>
-          The following connection methods are currently supported for primary
-          controllers:
+          The following active connection methods are currently supported:
         </p>
         <dl>
           <dt><code>ssl:<var>host</var></code>[<code>:<var>port</var></code>]</dt>
@@ -4546,8 +4542,7 @@  ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
           </dd>
         </dl>
         <p>
-          The following connection methods are currently supported for service
-          controllers:
+          The following passive connection methods are currently supported:
         </p>
         <dl>
           <dt><code>pssl:</code>[<var>port</var>][<code>:<var>host</var></code>]</dt>