[ovs-dev] controller: Remote connection option to OpenFlow switch.

Message ID 1502119087-8419-1-git-send-email-JaiSingh.Rana@caviumnetworks.com
State Changes Requested
Headers show

Commit Message

Jai Singh Rana Aug. 7, 2017, 3:18 p.m.
Currently ovn-controller uses default unix domain socket in Open
vSwitch's "run" directory to connect to OpenFlow switch. If
ovn-controller/ovsdb-server and vswitchd are running on different
systems, remote connection method needs to be provided.

Added configuration option to override default connection by using OVSDB
external-ids "ovn-ofswitch-remote". Using this external-id, desired tcp
or unix connection to OpenFlow switch can be specified.

Tested this by using tcp/unix method configured through external-id
"ovn-ofswitch-remote" and confirmed connection as flows getting updated
in Open vSwitch.

Signed-off-by: Jai Singh Rana <JaiSingh.Rana@caviumnetworks.com>
---
 ovn/controller/ofctrl.c             | 13 +++++++------
 ovn/controller/ofctrl.h             |  4 ++--
 ovn/controller/ovn-controller.8.xml |  9 +++++++++
 ovn/controller/ovn-controller.c     | 37 ++++++++++++++++++++++++++++++++++---
 ovn/controller/pinctrl.c            |  5 +++--
 ovn/controller/pinctrl.h            |  3 ++-
 6 files changed, 57 insertions(+), 14 deletions(-)

Comments

Ben Pfaff Oct. 30, 2017, 11:41 p.m. | #1
On Mon, Aug 07, 2017 at 08:48:07PM +0530, Jai Singh Rana wrote:
> Currently ovn-controller uses default unix domain socket in Open
> vSwitch's "run" directory to connect to OpenFlow switch. If
> ovn-controller/ovsdb-server and vswitchd are running on different
> systems, remote connection method needs to be provided.
> 
> Added configuration option to override default connection by using OVSDB
> external-ids "ovn-ofswitch-remote". Using this external-id, desired tcp
> or unix connection to OpenFlow switch can be specified.
> 
> Tested this by using tcp/unix method configured through external-id
> "ovn-ofswitch-remote" and confirmed connection as flows getting updated
> in Open vSwitch.
> 
> Signed-off-by: Jai Singh Rana <JaiSingh.Rana@caviumnetworks.com>

Thanks for the revised patch.

With this change, ofctrl_run() and pinctrl_run() both use xstrdup() to
make a copy of the ovn_ofswitch_remote string, and later free it.  I
don't see any reason to make the copy.

Please try to maintain the general principle of parameter ordering in
Open vSwitch, where read-only parameters go before read/write and
write-only parameters.  In this case, ofctrl_run() and pinctrl_run()
only read from ovn_ofswitch_remote, so it should go before the
parameters that it writes.

It looks like get_ovn_ofswitch_remote() will log the default remote on
every single trip through ovn-controller's main loop.  I don't think
it's necessary to log it at all.

This new feature seems to be pretty specialized.  The documentation
should explain that it's not normally necessary to set the new setting,
that it is only for use in situations where the most common
configuration will not work.

Thanks,

Ben.

Patch

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index fc88a41..c2ea1a5 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -451,14 +451,15 @@  recv_S_UPDATE_FLOWS(const struct ofp_header *oh, enum ofptype type,
     }
 }
 
-/* Runs the OpenFlow state machine against 'br_int', which is local to the
- * hypervisor on which we are running.  Attempts to negotiate a Geneve option
- * field for class OVN_GENEVE_CLASS, type OVN_GENEVE_TYPE.  If successful,
- * returns the MFF_* field ID for the option, otherwise returns 0. */
+/* Runs the OpenFlow state machine against 'ovn_ofswitch_remote', which can
+ * be local or remote to hypervisor on which we are running. Attempts to
+ * negotiate a Geneve option field for class OVN_GENEVE_CLASS,
+ * type OVN_GENEVE_TYPE. If successful, returns the MFF_* field ID for the
+ * option, otherwise returns 0. */
 enum mf_field_id
-ofctrl_run(const struct ovsrec_bridge *br_int, struct shash *pending_ct_zones)
+ofctrl_run(struct shash *pending_ct_zones, char *ovn_ofswitch_remote)
 {
-    char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+    char *target = xstrdup(ovn_ofswitch_remote);
     if (strcmp(target, rconn_get_target(swconn))) {
         VLOG_INFO("%s: connecting to switch", target);
         rconn_connect(swconn, target, target);
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index d83f6ae..a23ed4c 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -32,8 +32,8 @@  struct shash;
 
 /* Interface for OVN main loop. */
 void ofctrl_init(struct group_table *group_table);
-enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int,
-                            struct shash *pending_ct_zones);
+enum mf_field_id ofctrl_run(struct shash *pending_ct_zones,
+                            char *ovn_ofswitch_remote);
 bool ofctrl_can_put(void);
 void ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
                 int64_t nb_cfg);
diff --git a/ovn/controller/ovn-controller.8.xml b/ovn/controller/ovn-controller.8.xml
index 5641abc..4cd68c4 100644
--- a/ovn/controller/ovn-controller.8.xml
+++ b/ovn/controller/ovn-controller.8.xml
@@ -151,6 +151,15 @@ 
         network interface card, enabling encapsulation checksum may incur
         performance loss. In such cases, encapsulation checksums can be disabled.
       </dd>
+
+      <dt><code>external_ids:ovn-ofswitch-remote</code></dt>
+      <dd>
+        The OpenFlow connection method that this system can connect to reach
+        Open vSwitch using the <code>unix</code> or <code>tcp</code> forms
+        documented above for the <var>ovs-database</var>. If not configured,
+        default local unix domain socket file in the local Open vSwitch's
+        "run" directory is used.
+      </dd>
     </dl>
 
     <p>
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index e2c9652..358b98b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -328,6 +328,30 @@  get_ovnsb_remote(struct ovsdb_idl *ovs_idl)
     }
 }
 
+/* Retrieves the OVN OpenFlow switch remote location from the
+ * "external-ids:ovn-ofswitch-remote" key in 'ovs_idl' and returns
+ *  a copy of it.
+ */
+static char *
+get_ovn_ofswitch_remote(struct ovsdb_idl *ovs_idl,
+                        const struct ovsrec_bridge *br_int)
+{
+    const struct ovsrec_open_vswitch *cfg
+        = ovsrec_open_vswitch_first(ovs_idl);
+    if (cfg) {
+        const char *remote =
+            smap_get(&cfg->external_ids, "ovn-ofswitch-remote");
+        if (remote) {
+            return xstrdup(remote);
+        }
+    }
+
+    char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+    VLOG_INFO("OVN OVSDB OpenFlow switch remote not specified."
+              "Using default %s", target);
+    return target;
+}
+
 static void
 update_ct_zones(struct sset *lports, const struct hmap *local_datapaths,
                 struct simap *ct_zones, unsigned long *ct_zone_bitmap,
@@ -690,15 +714,21 @@  main(int argc, char *argv[])
         }
         if (br_int && chassis) {
             struct shash addr_sets = SHASH_INITIALIZER(&addr_sets);
+
+            /* Get ovn connection method to OpenFlow switch. */
+            char *ovn_ofswitch_remote = get_ovn_ofswitch_remote(ctx.ovs_idl,
+                                                                br_int);
+
             addr_sets_init(&ctx, &addr_sets);
 
             patch_run(&ctx, br_int, chassis);
 
-            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
-                                                         &pending_ct_zones);
+            enum mf_field_id mff_ovn_geneve = ofctrl_run(&pending_ct_zones,
+                                                         ovn_ofswitch_remote);
 
             pinctrl_run(&ctx, br_int, chassis, &chassis_index,
-                        &local_datapaths, &active_tunnels);
+                        &local_datapaths, &active_tunnels,
+                        ovn_ofswitch_remote);
             update_ct_zones(&local_lports, &local_datapaths, &ct_zones,
                             ct_zone_bitmap, &pending_ct_zones);
             if (ctx.ovs_idl_txn) {
@@ -750,6 +780,7 @@  main(int argc, char *argv[])
 
             expr_addr_sets_destroy(&addr_sets);
             shash_destroy(&addr_sets);
+            free(ovn_ofswitch_remote);
         }
 
         /* If we haven't handled the pending packet insertion
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 469a355..eda8d76 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -1025,9 +1025,10 @@  pinctrl_run(struct controller_ctx *ctx,
             const struct sbrec_chassis *chassis,
             const struct chassis_index *chassis_index,
             struct hmap *local_datapaths,
-            struct sset *active_tunnels)
+            struct sset *active_tunnels,
+            char *ovn_ofswitch_remote)
 {
-    char *target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), br_int->name);
+    char *target = xstrdup(ovn_ofswitch_remote);
     if (strcmp(target, rconn_get_target(swconn))) {
         VLOG_INFO("%s: connecting to switch", target);
         rconn_connect(swconn, target, target);
diff --git a/ovn/controller/pinctrl.h b/ovn/controller/pinctrl.h
index fc9cca8..6abf0f8 100644
--- a/ovn/controller/pinctrl.h
+++ b/ovn/controller/pinctrl.h
@@ -33,7 +33,8 @@  void pinctrl_init(void);
 void pinctrl_run(struct controller_ctx *,
                  const struct ovsrec_bridge *, const struct sbrec_chassis *,
                  const struct chassis_index *, struct hmap *local_datapaths,
-                 struct sset *active_tunnels);
+                 struct sset *active_tunnels,
+                 char *ovn_ofswitch_remote);
 void pinctrl_wait(struct controller_ctx *);
 void pinctrl_destroy(void);