netplan.c: Don't drop files with just global values on 'set' (LP: #2027584)

[slyon]

The "unlink" part at the bottom of netplan.c:netplan_state_update_yaml_hierarchy seems to ignore any global values (such as "renderer") and operates on files containing netdefs only.

The issue is happens due to a combination of the following PRs: #254 #299
Which got SRUed into Jammy via 0.105-0ubuntu2~22.04.2

Here's a minimal reproducer:

netplan set network.renderer=NetworkManager --origin-hint=00-no-netdefs-just-globals
netplan set network.ethernets.eth99.dhcp4=true --origin-hint=90-some-netdefs
ls -l /etc/netplan/
netplan set network.ethernets.eth99=NULL
cat /etc/netplan/00-no-netdefs-just-globals.yaml

FR-4793

Description

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. LP#2027584

[slyon]

The test failure needs to be investigated. It's related to netplan set:

======================================================================
FAIL: test_remove_virtual_interfaces (__main__.TestNetworkd)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.3nndtN/build.0mT/real-tree/tests/integration/scenarios.py", line 135, in test_remove_virtual_interfaces
    self.assertIn('not exist', res.stderr)
AssertionError: 'not exist' not found in ''

[slyon]

Yeah, well... we're not handling them as of now. But it's supposed to be fixed via LP#2003727

[slyon]

I fixed the autopkgtest, added another unit-test and rebased.
=> Ready for review.

[daniloegea]

The fix looks correct to me.

One little left over after executing your reproducer is an, almost, empty 70-netplan-set.yaml file that is created from nowhere. I was wondering if we could avoid creating it as it's useless and maybe unexpected.

Maybe we could skip the default file creation in the loop below the code you added by checking if it's the fallback file, if there are no netdefs and if the default file wasn't loaded from disk. Something like this:

    g_hash_table_iter_init(&hash_iter, perfile_netdefs);
    while (g_hash_table_iter_next (&hash_iter, &key, &value)) {
        const char *filename = key;
        gboolean is_fallback = (g_strcmp0(filename, default_path) == 0);
        GList* netdefs = value;

        /* new code */
        if (is_fallback && !netdefs && np_state->sources && !g_hash_table_contains(np_state->sources, default_path))
            continue;

        out_fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0600);
        if (out_fd < 0)
            goto file_error;
        if (!netplan_netdef_list_write_yaml(np_state, netdefs, out_fd, filename, is_fallback, error))
            goto cleanup; // LCOV_EXCL_LINE
        close(out_fd);
    }

Do you think it would work?

[slyon]

Do you think it would work?

Oh very good catch! But instead of skipping the creation of that 70-netplan-set.yaml default file in the loop you mentioned, I went with not adding it to the list of files to be written (perfile_netdefs) a few lines above, instead. And added tests for the relevant cases.