RFC: Support consuming Open vSwitch from snap #425
Conversation
fnordahl
force-pushed
the
snapped-ovs
branch
2 times, most recently
from
40059ba
to
6f0fbfb
Compare
At present the ctests are not properly linked with the built code, this causes issues when there are interdependencies between the modules. The meson build system does not appear to have support for internal build order or dependncies, so the only way I have found to make this work is to break out the build of the ctests as a separate project. This is required to avoid undefined symbols in the test code for the next patch in this series. Signed-off-by: Frode Nordahl <[email protected]>
fnordahl
force-pushed
the
snapped-ovs
branch
3 times, most recently
from
ee3da9b
to
1987edb
Compare
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]>
There was a problem hiding this comment.
Thank you very much for bringing this up early, Frode!
Besides the obvious CI failures that need fixing, the approach LGTM!
# COVERAGE
11/12 coverage-c FAIL 0.00s exit status 1
12/12 coverage-py FAIL 0.61s exit status 2
# AUTOPKGTEST-CI
FAIL: test_ovsdb_server_is_not_running (__main__.TestOVS)
I left some inline-comments for more specific issues that need some extra thoughs or discussions.
| const char * | ||
| netplan_openvswitch_ovs_vsctl(void) | ||
| { | ||
| if (!*netplan_openvswitch_ovs_vsctl_path) { |
There was a problem hiding this comment.
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.
| @@ -29,3 +29,6 @@ netplan_netdef_write_ovs( | |||
|
|
|||
| NETPLAN_INTERNAL gboolean | |||
| netplan_ovs_cleanup(const char* rootdir); | |||
|
|
|||
| NETPLAN_INTERNAL const char * | |||
There was a problem hiding this comment.
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.
| @@ -1,3 +1,10 @@ | |||
| project('ctests', 'c') | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
thought: would adding link_with: libnetplan to tests/ctests/meson.build work?
|
|
||
| OPENVSWITCH_OVS_VSCTL = '/usr/bin/ovs-vsctl' | ||
| OPENVSWITCH_OVSDB_SERVER_UNIT = 'ovsdb-server.service' | ||
| OPENVSWITCH_OVS_VSCTL = ( |
There was a problem hiding this comment.
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.
| subprocess.check_call( | ||
| [OPENVSWITCH_OVS_VSCTL, '--timeout', '5', 'get-manager']) | ||
| except (subprocess.CalledProcessError, OSError): | ||
| raise OvsDbServerNotRunning('{} is unable to connect to database, ' |
There was a problem hiding this comment.
note: This might have some implications for #421
slyon
added
community
Description
There are multiple projects in development that provide Open vSwitch in a snap (MicroOVN/MicroCloud, Sunbeam and possibly others). These projects have use for the Netplan Open vSwitch integration.
I had use for this functionality in my homelab, so as an itch to scratch I'm proposing this early to get feedback on the approach.
Please refer to https://bugs.launchpad.net/microovn/+bug/2045490 for a concrete use case example.
TODO:
Please review commit by commit.
Checklist
make checksuccessfully.make check-coverage).