configcore: implement netplan write support via dbus #10752

mvo5 · 2021-09-07T15:59:26Z

Implement writing netplan configuration using the dbus interface.
This works by creating a configuration snapshot that is then
Try()ed and if the store is reachable after the try (or was
never reachable in the first place) then it it Apply()ed.

The netplan configuration will need to check if the store is reachable before and after the network configuration got updated. This commit adds the needed helper and tests to ensure that the store is reachable.

Implement writing netplan configuration using the dbus interface. This works by creating a configuration snapshot that is then `Try()ed` and if the store is reachable after the try (or was never reachable in the first place) then it it `Apply()ed`.

…. always clear config first)

codecov-commenter · 2021-09-09T07:59:14Z

Codecov Report

Merging #10752 (68d1c34) into master (eec64b4) will decrease coverage by 0.05%.
The diff coverage is 9.39%.

@@            Coverage Diff             @@
##           master   #10752      +/-   ##
==========================================
- Coverage   78.31%   78.26%   -0.06%     
==========================================
  Files         920      923       +3     
  Lines      104871   105473     +602     
==========================================
+ Hits        82131    82548     +417     
- Misses      17611    17783     +172     
- Partials     5129     5142      +13

Flag	Coverage Δ
unittests	`78.26% <9.39%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
overlord/configstate/configcore/netplan.go	`9.83% <8.78%> (-11.32%)`	⬇️
overlord/configstate/configcore/runwithstate.go	`92.47% <100.00%> (ø)`
store/cache.go	`69.23% <0.00%> (-1.93%)`	⬇️
sandbox/cgroup/tracking.go	`92.72% <0.00%> (-1.27%)`	⬇️
usersession/client/client.go	`81.68% <0.00%> (-0.80%)`	⬇️
dirs/dirs.go	`94.02% <0.00%> (-0.70%)`	⬇️
overlord/overlord.go	`80.69% <0.00%> (-0.20%)`	⬇️
overlord/ifacestate/handlers.go	`64.87% <0.00%> (-0.15%)`	⬇️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eec64b4...68d1c34. Read the comment docs.

Having a different than the default config file seems to also cause grief with netplan so avoid it by just using the default ubuntu core netplan configuration.

mvo5 · 2021-11-04T21:31:58Z

I think this is ready for a review - it still contains a workaround for unexpected netplan dbus behavior but most of the core of the change is ready.

mvo5 · 2021-11-05T09:02:23Z

This fails right now in GCE because there is only /etc/netplan/50-cloud-init.yaml in our test uc20 and netplan will crash if there is no origin hint file on disk https://bugs.launchpad.net/netplan/+bug/1949884

…03-0ubuntu10

mardy

LGTM!

overlord/configstate/configcore/export_test.go

overlord/configstate/configcore/netplan.go

pedronis

clarification

pedronis · 2021-12-17T12:53:23Z

overlord/configstate/configcore/netplan.go

+	// Use a origin hint that is sorted before the console-conf
+	// "00-snapd-config.yaml" so that console-conf can override
+	// our settings when it runs.
+	originHint := "0-snap-set"


sorry, maybe my remark was not clear. We can't just do this either. I fear we need to use different hints at different times:

0-snapd-defaults if seeded is false

90-snapd-config if seeded is true

Sorry for that misunderstanding on my part, updated and tests added (including an early config test).

… during seeding or later (thansk to Samuele)

mvo5 · 2022-01-04T16:07:51Z

Fwiw, I ran the nested test manually:

$ spread -v -debug google-nested:ubuntu-20.04-64:tests/nested/manual/core20-early-config
...
2022-01-04 16:56:51 Preparing google-nested:ubuntu-20.04-64:tests/nested/manual/ (jan041549-956268)...
2022-01-04 16:57:22 Preparing google-nested:ubuntu-20.04-64:tests/nested/manual/core20-early-config (jan041549-956268)...
2022-01-04 17:05:23 Executing google-nested:ubuntu-20.04-64:tests/nested/manual/core20-early-config (jan041549-956268) (1/1)...
2022-01-04 17:06:30 Restoring google-nested:ubuntu-20.04-64:tests/nested/manual/core20-early-config (jan041549-956268)...
2022-01-04 17:06:31 Restoring google-nested:ubuntu-20.04-64:tests/nested/manual/ (jan041549-956268)...
2022-01-04 17:06:37 Restoring google-nested:ubuntu-20.04-64 (jan041549-956268)...
2022-01-04 17:06:38 Discarding google-nested:ubuntu-20.04-64 (jan041549-956268)...
2022-01-04 17:06:39 Successful tasks: 1
2022-01-04 17:06:39 Aborted tasks: 0

pedronis

thanks for the change. Question about testing. I think @slyon should look at this again.

pedronis · 2022-01-10T17:46:47Z

tests/nested/manual/core20-early-config/task.yaml

@@ -64,6 +68,9 @@ execute: |
        tests.nested exec "cat /var/snap/pc/common/debug.txt" | MATCH "hostname: foo"
        tests.nested exec "cat /etc/hostname" | MATCH "foo"
        tests.nested exec "hostname" | MATCH "foo"
+
+        # netplan config
+        tests.nested exec "cat /etc/netplan/0-snapd-defaults.yaml" | MATCH br54


can we somehow check that setting dhcp4 to false or similar works as expected at this point?

I added two more tests for this, one that uses a static IP during boot and one that checks that incremental network settings work.

the extra testing is good, but is different from what I was thinking. My question was to test that setting later on top of the defaults works

@pedronis oh, sorry again that I misunderstood - I added extra checks for this now as well. Good catch!

mvo's test cases look good to me. I can confirm that an interface definition in 0-snapd-defaults.yaml of the same name as an interface definition in 90-snapd-config.yaml would be overridden (the 90-snapd-config.yaml settings be in place).
And that is being tested in the # and updating netplan works section.

@pedronis Is there anything more specific that you wanted me to have a look at?

…o Samuele)

Thanks to Samuele for the suggestion.

pedronis

still a testing question

pedronis · 2022-01-11T10:10:39Z

tests/nested/manual/core20-early-config/task.yaml

@@ -64,6 +68,9 @@ execute: |
        tests.nested exec "cat /var/snap/pc/common/debug.txt" | MATCH "hostname: foo"
        tests.nested exec "cat /etc/hostname" | MATCH "foo"
        tests.nested exec "hostname" | MATCH "foo"
+
+        # netplan config
+        tests.nested exec "cat /etc/netplan/0-snapd-defaults.yaml" | MATCH br54


the extra testing is good, but is different from what I was thinking. My question was to test that setting later on top of the defaults works

…tplan config

pedronis

question about canceling vs errors

pedronis · 2022-01-14T13:15:42Z

tests/nested/manual/core20-early-config/task.yaml

+        tests.nested exec "netplan get bridges.br54.dhcp4" | MATCH false
+        tests.nested exec "sudo snap get system system.network.netplan.network.bridges.br54.dhcp4" | MATCH false
+        # ensure the test can be repeated
+        tests.nested exec "sudo rm -f /etc/netplan/90-snapd-config.yaml"


do we need something also in the restore stanza?

pedronis · 2022-01-14T15:11:40Z

overlord/configstate/configcore/netplan.go

@@ -113,5 +282,14 @@ func getNetplanFromSystem(key string) (result interface{}, err error) {
 		return nil, err
 	}

+	// and discard the config snapshot
+	var wasCancelled bool
+	if err := netplanCfgSnapshot.Call("io.netplan.Netplan.Config.Cancel", 0).Store(&wasCancelled); err != nil {


does it make sense to do this as a defer instead?

pedronis · 2022-01-14T15:12:19Z

overlord/configstate/configcore/netplan.go

+
+	if storeReachableBefore && !storeReachableAfter {
+		var wasCancelled bool
+		if err := netplanCfgSnapshot.Call("io.netplan.Netplan.Config.Cancel", 0).Store(&wasCancelled); err != nil {


should we have this as a defer that checks if we are returning an error?

…en if something failed

slyon

I like the extensive testing of this PR!

I recently had a call with the Field Engineering team and we were wondering if the new snap get/set functionality for networking/netplan settings would be tested in combination with the NetworkManager snap. I do not see any NetworkManager related tests in this PR, would it be possible to add some E2E test for that scenario as well? Something like snap set system system.network.netplan.network ... && nmcli con show ... (and the other way around, to check the bidirectional propagation of (supported) settings).

…uration

mvo5 · 2022-01-17T16:34:09Z

I like the extensive testing of this PR!

I recently had a call with the Field Engineering team and we were wondering if the new snap get/set functionality for networking/netplan settings would be tested in combination with the NetworkManager snap. I do not see any NetworkManager related tests in this PR, would it be possible to add some E2E test for that scenario as well? Something like snap set system system.network.netplan.network ... && nmcli con show ... (and the other way around, to check the bidirectional propagation of (supported) settings).

Thanks for the suggestion and review and I'm happy to do this, I will poke at it a bit but maybe this should be a followup given that this is already quite a big PR.

pedronis

thank you

this has changed quite a bit since

mvo5 · 2022-01-21T09:59:35Z

Uhhh - sorry only now saw that the review from Mardy got dismissed (for the right reasons) - sorry, too trigger happy, happy to fix anything that mardy will find.

mardy

Still LGTM! Added one comment, but it's irrelevant :-)

mardy · 2022-01-21T13:42:12Z

overlord/configstate/configcore/netplan.go

+var snapstateStore = func(st *state.State, deviceCtx snapstate.DeviceContext) connectivityCheckStore {
+	return snapstate.Store(st, deviceCtx)
+}


Any reason why this is not just var snapstateStore = snapstate.Store?

Probably not, silly me! I can look into this next.

Aha, actually I remember now - I think the reaosn is that I wanted to return the connectivityCheckStore interface only so that I need to mock less :)

mvo5 added 7 commits September 6, 2021 13:01

configcore: implement storeReachable() helper in netplan config

a33bb49

The netplan configuration will need to check if the store is reachable before and after the network configuration got updated. This commit adds the needed helper and tests to ensure that the store is reachable.

configcore: add support to unset netplan configuration

56aee65

coreconfig: make netplan tests more realistic and rework setting (i.e…

e43a49f

…. always clear config first)

tests: fix netplan-cfg set tests

0592ab0

configcore: add "nomanagers" tag to netplan

e0c1b63

Merge remote-tracking branch 'upstream/master' into netplan-read-write

c05fdb7

mvo5 added 3 commits October 12, 2021 17:17

Merge remote-tracking branch 'upstream/master' into netplan-read-write

df22c50

Merge remote-tracking branch 'upstream/master' into netplan-read-write

cedb717

configcore: add workaround for LP:1946957 and add more tests

67cc6bd

mvo5 force-pushed the netplan-read-write branch 2 times, most recently from 8e75cbe to 8bccfb2 Compare November 4, 2021 16:51

tests: use "00-snapd-config" as origin hint

42336f4

Having a different than the default config file seems to also cause grief with netplan so avoid it by just using the default ubuntu core netplan configuration.

mvo5 force-pushed the netplan-read-write branch from 8bccfb2 to 42336f4 Compare November 4, 2021 19:07

mvo5 added 3 commits November 4, 2021 20:34

tests: fix netplan-cfg test

c71a1b9

configcore: add workaround for LP:1949893

3a13ce4

tests: don't check for br54 removal, still not fixed in netplan

899dae7

mvo5 marked this pull request as ready for review November 4, 2021 21:31

mvo5 added 3 commits November 23, 2021 16:10

Merge remote-tracking branch 'upstream/master' into netplan-read-write

12bdf19

configcore: use different workaround

2bd029e

configcore: add workaround for netplan LP:1949884

31d2961

mvo5 force-pushed the netplan-read-write branch from 4211d2f to 31d2961 Compare November 23, 2021 20:41

configcore,tests: remove workaround now that netplan is fine with 0.1…

668fe71

…03-0ubuntu10

mardy approved these changes Dec 1, 2021

View reviewed changes

overlord/configstate/configcore/export_test.go Show resolved Hide resolved

overlord/configstate/configcore/netplan.go Outdated Show resolved Hide resolved

mvo5 added 3 commits December 8, 2021 17:42

configcore: rename unreachable->unreachableHost, fix whitespace

d4b6aa9

configcore: fix tests

704782d

Merge remote-tracking branch 'upstream/master' into netplan-read-write

4eb2eb9

configcore: use a originHint that sorts before 00-snapd-config.yaml

7f76b53

pedronis requested changes Dec 17, 2021

View reviewed changes

mvo5 added 3 commits January 4, 2022 12:18

configcore: remove no longer needed workaroudn for netplan bug

ee5b465

coreconfig: write different netplan config depending on if it is done…

431cb19

… during seeding or later (thansk to Samuele)

tests: add early config tests for netplan

cb88ef6

mvo5 added the Run nested The PR also runs tests inluded in nested suite label Jan 4, 2022

tests: update netplan-cfg to match current file names

dc728ca

pedronis self-requested a review January 10, 2022 10:13

pedronis reviewed Jan 10, 2022

View reviewed changes

mvo5 added 2 commits January 10, 2022 20:29

tests: check that incremental config settings are preserved (thanks t…

11e9501

…o Samuele)

tests: add nested test that overrides core defaults for netplan

3f31491

Thanks to Samuele for the suggestion.

pedronis reviewed Jan 11, 2022

View reviewed changes

mvo5 added 2 commits January 12, 2022 15:27

tests: ensure configuration can be overriden that was set by early ne…

68d1c34

…tplan config

tests: fix nested test (needs to be able to run twice)

e4800d8

pedronis self-requested a review January 13, 2022 10:11

pedronis reviewed Jan 14, 2022

View reviewed changes

tests: ensure that /etc/netplan/90-snapd-config.yaml gets removed, ev…

bdd16cb

…en if something failed

slyon reviewed Jan 17, 2022

View reviewed changes

mvo5 added 3 commits January 17, 2022 16:51

configcore: always cancel config via defer in getNetplanFromSystem

de5b414

configcore: refactor cancelNetplanCfgSnapshot

bf38dc3

configcore: "cancel" netplan snapshot on error in handleNetplanConfig…

29cccbd

…uration

pedronis self-requested a review January 20, 2022 12:55

pedronis approved these changes Jan 20, 2022

View reviewed changes

mvo5 merged commit 4e01535 into snapcore:master Jan 21, 2022

mardy reviewed Jan 21, 2022

View reviewed changes

slyon mentioned this pull request Feb 2, 2022

YAML consolidation: netplan set (FR-702) canonical/netplan#254

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configcore: implement netplan write support via dbus #10752

configcore: implement netplan write support via dbus #10752

mvo5 commented Sep 7, 2021 •

edited

codecov-commenter commented Sep 9, 2021 •

edited

mvo5 commented Nov 4, 2021

mvo5 commented Nov 5, 2021

mardy left a comment

pedronis left a comment

pedronis Dec 17, 2021

mvo5 Jan 4, 2022

mvo5 commented Jan 4, 2022

pedronis left a comment

pedronis Jan 10, 2022

mvo5 Jan 10, 2022

pedronis Jan 11, 2022

mvo5 Jan 12, 2022

slyon Jan 17, 2022

pedronis left a comment

pedronis Jan 11, 2022

pedronis left a comment

pedronis Jan 14, 2022

pedronis Jan 14, 2022

pedronis Jan 14, 2022

slyon left a comment

mvo5 commented Jan 17, 2022

pedronis left a comment

mvo5 commented Jan 21, 2022

mardy left a comment

mardy Jan 21, 2022

mvo5 Jan 21, 2022

mvo5 Jan 21, 2022

configcore: implement netplan write support via dbus #10752

configcore: implement netplan write support via dbus #10752

Conversation

mvo5 commented Sep 7, 2021 • edited

codecov-commenter commented Sep 9, 2021 • edited

Codecov Report

mvo5 commented Nov 4, 2021

mvo5 commented Nov 5, 2021

mardy left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Jan 4, 2022

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyon left a comment

Choose a reason for hiding this comment

mvo5 commented Jan 17, 2022

pedronis left a comment

Choose a reason for hiding this comment

mvo5 commented Jan 21, 2022

mardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Sep 7, 2021 •

edited

codecov-commenter commented Sep 9, 2021 •

edited