ovs: Support consuming OVS from snap

At present netplan hard codes the location of the `ovs-vsctl` binary. This does not work well when trying to integrate with snaps. Determine path of `ovs-vsctl` at runtime by checking for both '/snap/bin/ovs-vsctl' and '/usr/bin/ovs-vsctl' with a preference of the former. This allows making use of the Netplan Open vSwitch integration with for example the MicroOVN snap (and its consumers such as MicroCloud). TODO: Figure out how to hanlde the ovsdb-server.service unit check TODO: Add test cases that checks the snap workflow Signed-off-by: Frode Nordahl <[email protected]>
canonical · Dec 2, 2023 · ee3da9b · ee3da9b
1 parent 3c93eea
commit ee3da9b
Show file tree

Hide file tree

Showing 5 changed files with 108 additions and 30 deletions.
diff --git a/netplan_cli/cli/ovs.py b/netplan_cli/cli/ovs.py
@@ -20,10 +20,9 @@
 import subprocess
 import re
 
-from .utils import systemctl_is_active
-
-OPENVSWITCH_OVS_VSCTL = '/usr/bin/ovs-vsctl'
-OPENVSWITCH_OVSDB_SERVER_UNIT = 'ovsdb-server.service'
+OPENVSWITCH_OVS_VSCTL = (
+    '/snap/bin/ovs-vsctl' if os.path.exists('/snap/bin/ovs-vsctl') else
+    '/usr/bin/ovs-vsctl')
 # Defaults for non-optional settings, as defined here:
 # http://www.openvswitch.org/ovs-vswitchd.conf.db.5.pdf
 DEFAULTS = {
@@ -43,6 +42,19 @@ class OvsDbServerNotRunning(Exception):
     pass
 
 
+def _assert_ovsdb_server_connection():
+    """Invoke OPENVSWITCH_OVS_VSCTL, raise OvsDbServerNotRunning on error."""
+    try:
+        # The `get-manager` command is a light weight database operation which
+        # we use as indication of the client being able to connect.
+        subprocess.check_call(
+            [OPENVSWITCH_OVS_VSCTL, '--timeout', '5', 'get-manager'])
+    except subprocess.CalledProcessError:
+        raise OvsDbServerNotRunning('{} is unable to connect to database, '
+                                    'ensure ovsdb-server is running'
+                                    .format(OPENVSWITCH_OVS_VSCTL))
+
+
 def _del_col(type, iface, column, value):
     """Cleanup values from a column (i.e. "column=value")"""
     default = DEFAULTS.get(column)
@@ -125,8 +137,7 @@ def apply_ovs_cleanup(config_manager, ovs_old, ovs_current):  # pragma: nocover
     Also filter for individual settings tagged netplan/<column>[/<key]=value
     in external-ids and clear them if they have been set by netplan.
     """
-    if not systemctl_is_active(OPENVSWITCH_OVSDB_SERVER_UNIT):
-        raise OvsDbServerNotRunning('{} is not running'.format(OPENVSWITCH_OVSDB_SERVER_UNIT))
+    _assert_ovsdb_server_connection()
 
     config_manager.parse()
     ovs_ifaces = set()

diff --git a/src/openvswitch.c b/src/openvswitch.c
@@ -16,6 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <stdio.h>
 #include <unistd.h>
 #include <errno.h>
 
@@ -29,6 +30,42 @@
 #include "util.h"
 #include "util-internal.h"
 
+/* 20 bytes for "/snap/bin/" or "/usr/bin/ovs-vsctl" + \0 */
+static char netplan_openvswitch_ovs_vsctl_path[20];
+#define OPENVSWITCH_OVS_VSCTL "ovs-vsctl"
+
+#ifndef BINDIR
+#define BINDIR "/usr/bin"
+#endif
+
+#ifndef SNAPBINDIR
+#define SNAPBINDIR "/snap/bin"
+#endif
+
+void
+netplan_openvswitch_vsctl__(void)
+{
+    if (g_file_test(SNAPBINDIR"/"OPENVSWITCH_OVS_VSCTL, G_FILE_TEST_EXISTS)) {
+        snprintf(netplan_openvswitch_ovs_vsctl_path,
+                 sizeof netplan_openvswitch_ovs_vsctl_path,
+                 SNAPBINDIR"/"OPENVSWITCH_OVS_VSCTL);
+    } else {
+        snprintf(netplan_openvswitch_ovs_vsctl_path,
+                 sizeof netplan_openvswitch_ovs_vsctl_path,
+                 BINDIR"/"OPENVSWITCH_OVS_VSCTL);
+    }
+
+}
+
+const char *
+netplan_openvswitch_ovs_vsctl(void)
+{
+    if (!*netplan_openvswitch_ovs_vsctl_path) {
+        netplan_openvswitch_vsctl__();
+    }
+    return (const char *) &netplan_openvswitch_ovs_vsctl_path;
+}
+
 static gboolean
 write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir, gboolean physical, gboolean cleanup, const char* dependency, GError** error)
 {
@@ -51,7 +88,8 @@ write_ovs_systemd_unit(const char* id, const GString* cmds, const char* rootdir,
         g_string_append_printf(s, "After=netplan-ovs-cleanup.service\n");
     } else {
         /* The netplan-ovs-cleanup unit shall not run on systems where Open vSwitch is not installed. */
-        g_string_append(s, "ConditionFileIsExecutable=" OPENVSWITCH_OVS_VSCTL "\n");
+        g_string_append_printf(s, "ConditionFileIsExecutable=%s\n",
+                               netplan_openvswitch_ovs_vsctl());
     }
     g_string_append(s, "Before=network.target\nWants=network.target\n");
     if (dependency) {
@@ -127,7 +165,8 @@ write_ovs_tag_setting(const gchar* id, const char* type, const char* col, const
     if (key)
         g_string_append_printf(s, "/%s", key);
     g_string_append_printf(s, "=%s", clean_value);
-    append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s %s", type, id, s->str);
+    append_systemd_cmd(cmds, "%s set %s %s %s",
+                       netplan_openvswitch_ovs_vsctl(), type, id, s->str);
     g_string_free(s, TRUE);
 }
 
@@ -142,8 +181,9 @@ write_ovs_additional_data(GHashTable *data, const char* type, const gchar* id, G
     while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &value)) {
         /* XXX: we need to check what happens when an invalid key=value pair
             gets supplied here. We might want to handle this somehow. */
-        append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s %s:%s=%s",
-                           type, id, setting, key, value);
+        append_systemd_cmd(cmds, "%s set %s %s %s:%s=%s",
+                           netplan_openvswitch_ovs_vsctl(), type, id, setting,
+                           key, value);
         write_ovs_tag_setting(id, type, setting, key, value, cmds);
     }
 }
@@ -175,8 +215,9 @@ write_ovs_bond_interfaces(const NetplanState* np_state, const NetplanNetDefiniti
         return NULL;
     }
 
-    s = g_string_new(OPENVSWITCH_OVS_VSCTL " --may-exist add-bond");
-    g_string_append_printf(s, " %s %s", def->bridge, def->id);
+    s = g_string_new(NULL);
+    g_string_printf(s, "%s --may-exist add-bond %s %s",
+                    netplan_openvswitch_ovs_vsctl(), def->bridge, def->id);
 
     g_hash_table_iter_init(&iter, np_state->netdefs);
     while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &tmp_nd)) {
@@ -202,8 +243,8 @@ static void
 write_ovs_tag_netplan(const gchar* id, const char* type, GString* cmds)
 {
     /* Mark this bridge/port/interface as created by netplan */
-    append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set %s %s external-ids:netplan=true",
-                       type, id);
+    append_systemd_cmd(cmds, "%s set %s %s external-ids:netplan=true",
+                       netplan_openvswitch_ovs_vsctl(), type, id);
 }
 
 static gboolean
@@ -216,7 +257,8 @@ write_ovs_bond_mode(const NetplanNetDefinition* def, GString* cmds, GError** err
         !strcmp(def->bond_params.mode, "balance-tcp") ||
         !strcmp(def->bond_params.mode, "balance-slb")) {
         value = def->bond_params.mode;
-        append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Port %s bond_mode=%s", def->id, value);
+        append_systemd_cmd(cmds, "%s set Port %s bond_mode=%s",
+                           netplan_openvswitch_ovs_vsctl(), def->id, value);
         write_ovs_tag_setting(def->id, "Port", "bond_mode", NULL, value, cmds);
     } else {
         g_set_error(error, NETPLAN_BACKEND_ERROR, NETPLAN_ERROR_VALIDATION, "%s: bond mode '%s' not supported by Open vSwitch\n",
@@ -233,7 +275,8 @@ write_ovs_bridge_interfaces(const NetplanState* np_state, const NetplanNetDefini
     GHashTableIter iter;
     gchar* key;
 
-    append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-br %s", def->id);
+    append_systemd_cmd(cmds, "%s --may-exist add-br %s",
+                       netplan_openvswitch_ovs_vsctl(), def->id);
 
     g_hash_table_iter_init(&iter, np_state->netdefs);
     while (g_hash_table_iter_next(&iter, (gpointer) &key, (gpointer) &tmp_nd)) {
@@ -243,8 +286,9 @@ write_ovs_bridge_interfaces(const NetplanState* np_state, const NetplanNetDefini
             GString * patch_ports = g_string_new("");
             if (tmp_nd->type == NETPLAN_DEF_TYPE_PORT)
                 setup_patch_port(patch_ports, tmp_nd);
-            append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-port %s %s%s",
-                               def->id, tmp_nd->id, patch_ports->str);
+            append_systemd_cmd(cmds, "%s --may-exist add-port %s %s%s",
+                               netplan_openvswitch_ovs_vsctl(), def->id,
+                               tmp_nd->id, patch_ports->str);
             g_string_free(patch_ports, TRUE);
         }
     }
@@ -259,7 +303,8 @@ write_ovs_protocols(const NetplanOVSSettings* ovs_settings, const gchar* bridge,
     for (unsigned i = 1; i < ovs_settings->protocols->len; ++i)
         g_string_append_printf(s, ",%s", g_array_index(ovs_settings->protocols, char*, i));
 
-    append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s protocols=%s", bridge, s->str);
+    append_systemd_cmd(cmds, "%s set Bridge %s protocols=%s",
+                       netplan_openvswitch_ovs_vsctl(), bridge, s->str);
     write_ovs_tag_setting(bridge, "Bridge", "protocols", NULL, s->str, cmds);
     g_string_free(s, TRUE);
 }
@@ -301,7 +346,8 @@ write_ovs_bridge_controller_targets(const NetplanOVSSettings* settings, const Ne
     }
     g_string_erase(s, s->len-1, 1);
 
-    append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-controller %s %s", bridge, s->str);
+    append_systemd_cmd(cmds, "%s set-controller %s %s",
+                       netplan_openvswitch_ovs_vsctl(), bridge, s->str);
     write_ovs_tag_setting(bridge, "Bridge", "global", "set-controller", s->str, cmds);
 
 cleanup:
@@ -340,7 +386,9 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio
                 write_ovs_tag_netplan(def->id, type, cmds);
                 /* Set LACP mode, default to "off" */
                 value = def->ovs_settings.lacp? def->ovs_settings.lacp : "off";
-                append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Port %s lacp=%s", def->id, value);
+                append_systemd_cmd(cmds, "%s set Port %s lacp=%s",
+                                   netplan_openvswitch_ovs_vsctl(),
+                                   def->id, value);
                 write_ovs_tag_setting(def->id, type, "lacp", NULL, value, cmds);
                 if (def->bond_params.mode && !write_ovs_bond_mode(def, cmds, error))
                     return FALSE;
@@ -351,15 +399,23 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio
                 write_ovs_tag_netplan(def->id, type, cmds);
                 /* Set fail-mode, default to "standalone" */
                 value = def->ovs_settings.fail_mode? def->ovs_settings.fail_mode : "standalone";
-                append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-fail-mode %s %s", def->id, value);
+                append_systemd_cmd(cmds, "%s set-fail-mode %s %s",
+                                   netplan_openvswitch_ovs_vsctl(),
+                                   def->id, value);
                 write_ovs_tag_setting(def->id, type, "global", "set-fail-mode", value, cmds);
                 /* Enable/disable mcast-snooping */ 
                 value = def->ovs_settings.mcast_snooping? "true" : "false";
-                append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s mcast_snooping_enable=%s", def->id, value);
+                append_systemd_cmd(cmds,
+                                   "%s set Bridge %s mcast_snooping_enable=%s",
+                                   netplan_openvswitch_ovs_vsctl(),
+                                   def->id, value);
                 write_ovs_tag_setting(def->id, type, "mcast_snooping_enable", NULL, value, cmds);
                 /* Enable/disable rstp */
                 value = def->ovs_settings.rstp? "true" : "false";
-                append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Bridge %s rstp_enable=%s", def->id, value);
+                append_systemd_cmd(cmds,
+                                   "%s set Bridge %s rstp_enable=%s",
+                                   netplan_openvswitch_ovs_vsctl(),
+                                   def->id, value);
                 write_ovs_tag_setting(def->id, type, "rstp_enable", NULL, value, cmds);
                 /* Set protocols */
                 if (def->ovs_settings.protocols && def->ovs_settings.protocols->len > 0)
@@ -374,7 +430,11 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio
                     /* Set controller connection mode, only applicable if at least one controller target address was set */
                     if (def->ovs_settings.controller.connection_mode) {
                         value = def->ovs_settings.controller.connection_mode;
-                        append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set Controller %s connection-mode=%s", def->id, value);
+                        append_systemd_cmd(cmds,
+                                           "%s set Controller %s "
+                                           "connection-mode=%s",
+                                           netplan_openvswitch_ovs_vsctl(),
+                                           def->id, value);
                         write_ovs_tag_setting(def->id, "Controller", "connection-mode", NULL, value, cmds);
                     }
                 }
@@ -400,7 +460,9 @@ netplan_netdef_write_ovs(const NetplanState* np_state, const NetplanNetDefinitio
                 g_assert(def->vlan_link);
                 dependency = def->vlan_link->id;
                 /* Create a fake VLAN bridge */
-                append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " --may-exist add-br %s %s %i", def->id, def->vlan_link->id, def->vlan_id)
+                append_systemd_cmd(cmds, "%s --may-exist add-br %s %s %i",
+                                   netplan_openvswitch_ovs_vsctl(),
+                                   def->id, def->vlan_link->id, def->vlan_id)
                 write_ovs_tag_netplan(def->id, type, cmds);
                 break;
 
@@ -477,7 +539,8 @@ netplan_state_finish_ovs_write(const NetplanState* np_state, const char* rootdir
                         settings->ssl.client_key,
                         settings->ssl.client_certificate,
                         settings->ssl.ca_certificate);
-        append_systemd_cmd(cmds, OPENVSWITCH_OVS_VSCTL " set-ssl %s", value->str);
+        append_systemd_cmd(cmds, "%s set-ssl %s",
+                           netplan_openvswitch_ovs_vsctl(), value->str);
         write_ovs_tag_setting(".", "open_vswitch", "global", "set-ssl", value->str, cmds);
         g_string_free(value, TRUE);
     }

diff --git a/src/openvswitch.h b/src/openvswitch.h
@@ -29,3 +29,6 @@ netplan_netdef_write_ovs(
 
 NETPLAN_INTERNAL gboolean
 netplan_ovs_cleanup(const char* rootdir);
+
+NETPLAN_INTERNAL const char *
+netplan_openvswitch_ovs_vsctl(void);
diff --git a/src/util-internal.h b/src/util-internal.h
@@ -61,8 +61,6 @@ wifi_get_freq5(int channel);
 NETPLAN_ABI gchar*
 systemd_escape(char* string);
 
-#define OPENVSWITCH_OVS_VSCTL "/usr/bin/ovs-vsctl"
-
 void
 mark_data_as_dirty(NetplanParser* npp, const void* data_ptr);
 

diff --git a/src/validation.c b/src/validation.c
@@ -31,6 +31,7 @@
 #include "error.h"
 #include "util-internal.h"
 #include "validation.h"
+#include "openvswitch.h"
 
 /* Check coherence for address types */
 
@@ -401,7 +402,9 @@ validate_netdef_grammar(const NetplanParser* npp, NetplanNetDefinition* nd, GErr
 
     if (nd->backend == NETPLAN_BACKEND_OVS) {
         // LCOV_EXCL_START
-        if (!g_file_test(OPENVSWITCH_OVS_VSCTL, G_FILE_TEST_EXISTS)) {
+        if (!g_file_test(netplan_openvswitch_ovs_vsctl(),
+                         G_FILE_TEST_EXISTS))
+        {
             /* Tested via integration test */
             return yaml_error(npp, NULL, error, "%s: The 'ovs-vsctl' tool is required to setup OpenVSwitch interfaces.", nd->id);
         }