Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Support consuming Open vSwitch from snap #425

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

RFC: Support consuming Open vSwitch from snap #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c93eea
tests/ctests: Build ctests separately
fnordahl Dec 2, 2023
880db4e
ovs: Support consuming OVS from snap
fnordahl Dec 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,12 @@ test_env = [
]

if get_option('unit_testing')
subdir('tests/ctests')
test('ctests',
find_program('tests/ctests/run.sh'),
workdir: join_paths(meson.current_source_dir(), 'tests/ctests'),
args: [join_paths(meson.current_build_dir(), '_ctestsbuild'),
'-Dparent_current_source_dir=' + meson.current_source_dir(),
'-Dparent_current_build_dir=' + meson.current_build_dir()])
endif

#FIXME: exclude doc/env/
Expand Down
23 changes: 17 additions & 6 deletions netplan_cli/cli/ovs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: We also have some NetworkManager.snap handling in cli/utils.py:is_nm_snap_enabled(), which handles a similar situation a bit differently (checking for the snap related systemd service). Both have in common that the snap path gets a higher priority.

My initial idea was to bring the OVS handling in line with the NM handling, using a similar logic. But OTOH, we also have the changes in src/openvswitch.c (that we don't have for NM.snap), mimicking the path-fallback logic implemented here. So it might be better to keep the OVS-VSCL handling in-line between .c and .py and rather adopt/refactor the NM.snap handling at some point in the future.

'/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 = {
Expand All @@ -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, OSError):
raise OvsDbServerNotRunning('{} is unable to connect to database, '
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: This might have some implications for #421

'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)
Expand Down Expand Up @@ -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()
Expand Down
111 changes: 90 additions & 21 deletions src/openvswitch.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand All @@ -29,6 +30,48 @@
#include "util.h"
#include "util-internal.h"

#ifndef PREFIX
#define PREFIX "/usr/"
#endif

#ifndef BINDIR
#define BINDIR PREFIX"bin"
#endif

#ifndef SNAPBINDIR
#define SNAPBINDIR "/snap/bin"
#endif

#define OPENVSWITCH_OVS_VSCTL "ovs-vsctl"

static char netplan_openvswitch_ovs_vsctl_path[
MAX(sizeof BINDIR + 1 + sizeof OPENVSWITCH_OVS_VSCTL,
sizeof SNAPBINDIR + 1 + sizeof OPENVSWITCH_OVS_VSCTL)];

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be sure this is always null-initialized? Some architectures/compilers might not do that for us. We could probably do that above, or use a higher level concept, like glib's GString, that takes care of that for us.

Another thought/nitpick: What happens to a longer-running Netplan process (e.g. a daemon linking to libnetplan), where the OVS snap gets installed midway through the Netplan process' lifetime? I think it would continue calling into the system-OVS, which might or might not be OK. Granted, that is more of a future topic, as the OVS generator is mostly executed in isolation currently.

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)
{
Expand All @@ -51,7 +94,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) {
Expand Down Expand Up @@ -127,7 +171,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);
}

Expand All @@ -142,8 +187,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);
}
}
Expand Down Expand Up @@ -175,8 +221,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)) {
Expand All @@ -202,8 +249,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
Expand All @@ -216,7 +263,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",
Expand All @@ -233,7 +281,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)) {
Expand All @@ -243,8 +292,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);
}
}
Expand All @@ -259,7 +309,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);
}
Expand Down Expand Up @@ -301,7 +352,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:
Expand Down Expand Up @@ -340,7 +392,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;
Expand All @@ -351,15 +405,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)
Expand All @@ -374,7 +436,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);
}
}
Expand All @@ -400,7 +466,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;

Expand Down Expand Up @@ -477,7 +545,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);
}
Expand Down
3 changes: 3 additions & 0 deletions src/openvswitch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,6 @@ netplan_netdef_write_ovs(

NETPLAN_INTERNAL gboolean
netplan_ovs_cleanup(const char* rootdir);

NETPLAN_INTERNAL const char *
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to drop the NETPLAN_INTERNAL attribute here. It will make the netplan_openvswitch_ovs_vsctl symbol visible as part of the library, which we don't need here, it should be part of all the relevant object files. (Yes, the NETPLAN_INTERNAL naming is confusing here, sorry for that.)

Should it be needed, the new symbol should be prefixed with "_" (_netplan_openvswitch_ovs_vsctl) to clearly mark it as private API.

netplan_openvswitch_ovs_vsctl(void);
6 changes: 4 additions & 2 deletions src/util-internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -179,3 +177,7 @@ _netplan_netdef_pertype_iter_next(struct netdef_pertype_iter* it);

NETPLAN_INTERNAL void
_netplan_netdef_pertype_iter_free(struct netdef_pertype_iter* it);

#ifndef MAX
#define MAX(X, Y) ((X) > (Y) ? (X) : (Y))
#endif
5 changes: 4 additions & 1 deletion src/validation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "error.h"
#include "util-internal.h"
#include "validation.h"
#include "openvswitch.h"

/* Check coherence for address types */

Expand Down Expand Up @@ -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);
}
Expand Down
9 changes: 9 additions & 0 deletions tests/ctests/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/bin/sh

BUILDDIR="$1"; shift
meson setup ${BUILDDIR} $*
meson compile -C ${BUILDDIR} --verbose

if [ "$(basename $0)" = "run.sh" ]; then
meson test -C ${BUILDDIR}
fi
23 changes: 20 additions & 3 deletions tests/ctests/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
project('ctests', 'c')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get @daniloegea's opinion on this new project structure. Overall, it looks fine to me and I'm happy to change it that way, as long as we can run our tests as usual and don't need to adopt the packaging.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ctests were intentionally not linked against libnetplan. The idea was to build only the necessary files with the test files. The problem is that each file has dependencies spread across almost all the others so to get it working properly we need to do a good refactoring in the C code...

I think it should be fine to separate and link them against libnetplan to make things easier for the time being as long as we still can run them as part of meson test and the coverage checks still work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: would adding link_with: libnetplan to tests/ctests/meson.build work?


add_project_arguments(
'-DSBINDIR="' + join_paths(get_option('prefix'), get_option('sbindir')) + '"',
'-D_GNU_SOURCE',
language: 'c')

tests = {
'test_netplan_parser': false,
'test_netplan_state': false,
Expand All @@ -10,15 +17,25 @@ tests = {
'test_netplan_openvswitch': false,
}

glib = dependency('glib-2.0')
gio = dependency('gio-2.0')
yaml = dependency('yaml-0.1')
uuid = dependency('uuid')
cmocka = dependency('cmocka', required: true)
netplan = meson.get_compiler('c').find_library(
'netplan',
dirs: [join_paths(get_option('parent_current_build_dir'), 'src')])

foreach name, should_fail: tests
exe = executable(name,
'@[email protected]'.format(name),
include_directories: [inc, inc_internal],
dependencies: [cmocka, glib, gio, yaml, uuid],
include_directories: [
join_paths(get_option('parent_current_source_dir'), 'include'),
join_paths(get_option('parent_current_source_dir'), 'src'),
],
dependencies: [cmocka, glib, gio, yaml, uuid, netplan],
c_args: [
'-DFIXTURESDIR="' + meson.project_source_root() + '/tests/ctests/fixtures"',
'-DFIXTURESDIR="' + meson.project_source_root() + '/fixtures"',
'-Wno-deprecated-declarations',
'-D_GNU_SOURCE',
],
Expand Down
2 changes: 2 additions & 0 deletions tests/ctests/meson_options.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
option('parent_current_source_dir', type: 'string', value: '')
option('parent_current_build_dir', type: 'string', value: '')
1 change: 1 addition & 0 deletions tests/ctests/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
build.sh
Loading
Loading