dbus: Build the copy path correctly

[daniloegea]

Dbus Config()/Get() were not working due to a missing / in the destination path.

We still need a nice integration test to catch this issue.

Description

sudo dbus-send --system --print-reply --type=method_call --dest=io.netplan.Netplan /io/netplan/Netplan io.netplan.Netplan.Config

From dbus-monitor:

string "Failed to copy file /etc/netplan/01-network-manager-all.yaml -> /run/netplan/config-1YXM01etc/netplan/01-network-manager-all.yaml: Error opening file “/run/netplan/config-1YXM01etc/netplan/01-network-manager-all.yaml”: No such file or directory

The path is malformed: /run/netplan/config-1YXM01etc/netplan/01-network-manager-all.yaml

Checklist

• Runs make check successfully.
• Retains 100% code coverage (make check-coverage).
• New/changed keys in YAML format are documented.
• (Optional) Adds example YAML for new feature.
• (Optional) Closes an open bug in Launchpad.

[slyon]

Ugh. This should not happen. The fix LGTM!
I assume this is related to LP#1997467 ?

Can we build a test case around it, to avoid such failure in the future?

[daniloegea]

I've created a new integration test for dbus at tests/integration/dbus.py with 3 tests. As we need to add it to debian/tests/control to get it running, I also added the test for the bug fixed by this PR in the file integration/regressions.py.

[daniloegea]

I think #330 would also be a nice autopkgtest.

[ezeriver94]

I came across the same issue when creating Config through D-Bus, maybe it's worth adding error control to the _copy_yaml_state method

r = _copy_yaml_state(NETPLAN_ROOT, path, ret_error);  
if (r < 0){  
        return sd_bus_error_setf(ret_error, SD_BUS_ERROR_FAILED,  
                                "Failed to copy yaml structure to'%s': %s\n", path, strerror(errno));  
}

Without this, the execution appears to end up OK (even it shows the path of the new config created) but when you go to the folder it is empty, and that could cause removing the entire netplan of the machine if you apply it

[slyon]

This was initially just a small bug fix, but I still like the idea of adding some more error handling (and unit tests for coverage), while on it. Danilo, could you have a look into that if it's feasible?

Wrt the integration tests, we're checking (and modifying!) the eth0 interface as part of the new dbus tests. We consider eth0 en* to be our management interfaces for the test runner, so we really shouldn't be playing with it inside the tests. It can change in name, depending on the distribution+version this test is being run on, and if we bring down the management interface, our test runners might go down.

Please see tests/integration/base.py:create_devices() and make use of self.dev_e_client (eth42) and/or self.dev_e2_client (eth43) during the tests.

[daniloegea]

Thanks for your inputs @ezeriver94 :)

As _copy_yaml_state() was already setting an error code/message, I just changed the callers to propagate it.

Now a call to netplan-bus will fail like this:

$ sudo busctl -j call --system io.netplan.Netplan /io/netplan/Netplan io.netplan.Netplan Config
Call failed: Failed to copy file /etc/netplan/01-network-manager-all.yaml -> /run/netplan/config-08LD11etc/netplan/01-network-manager-all.yaml: Error opening file “/run/netplan/config-08LD11etc/netplan/01-network-manager-all.yaml”: No such file or directory

I also reworked the integration tests to not use eth0 and output the error in case of failures.

[slyon]

Thank you @daniloegea and @ezeriver94 for your input on this one. I really like the addition of the new dbus integration tests! We'll also rebase #330 on top of this, to make sure it passes, too.

[slyon]

The new spread tests passed (after some tiny fixes to the tests). So I think this is ready to be merged!

[mvo5]

I think #330 would also be a nice autopkgtest.

Fwiw, it's also easy to run the spread tests as part of autopkgtest (we do this in snapd, see https://github.com/snapcore/snapd/blob/master/packaging/ubuntu-16.04/tests/integrationtests#L57).

[slyon]

Nice! I think that's something we'd want to do. I think it should also avoid the packaging issue with meson, as we'd use pre-built binary packages to run the autopkgtest.