systemd: add AtLeast() method, add mocking in systemdtest #10748

mardy · 2021-09-07T11:26:10Z

Allow the systemd.Version() method to be mocked, and add a new AtLeast() method, as per the TODO.

codecov-commenter · 2021-09-07T11:47:02Z

Codecov Report

Merging #10748 (973ab87) into master (10588fd) will decrease coverage by 0.01%.
The diff coverage is 77.27%.

❗ Current head 973ab87 differs from pull request most recent head 2a1c3c1. Consider uploading reports for the commit 2a1c3c1 to get more accurate results

@@            Coverage Diff             @@
##           master   #10748      +/-   ##
==========================================
- Coverage   78.36%   78.34%   -0.02%     
==========================================
  Files         888      888              
  Lines      100091   100109      +18     
==========================================
- Hits        78433    78432       -1     
- Misses      16743    16762      +19     
  Partials     4915     4915

Flag	Coverage Δ
unittests	`78.34% <77.27%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
overlord/configstate/configcore/journal.go	`60.29% <40.00%> (+1.56%)`	⬆️
usersession/userd/privileged_desktop_launcher.go	`85.44% <60.00%> (+0.37%)`	⬆️
systemd/systemd.go	`85.97% <87.50%> (+0.02%)`	⬆️
overlord/servicestate/quota_control.go	`86.47% <100.00%> (+1.85%)`	⬆️
osutil/synctree.go	`76.41% <0.00%> (-2.84%)`	⬇️
store/cache.go	`69.23% <0.00%> (-1.93%)`	⬇️
daemon/api_connections.go	`91.97% <0.00%> (-1.61%)`	⬇️
daemon/daemon.go	`76.40% <0.00%> (-1.52%)`	⬇️
overlord/hookstate/hookmgr.go	`74.17% <0.00%> (-0.67%)`	⬇️
overlord/ifacestate/helpers.go	`77.02% <0.00%> (-0.50%)`	⬇️

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 10588fd...2a1c3c1. Read the comment docs.

stolowski

Looks nice, thank you! A few remarks/suggestions to consider

stolowski · 2021-09-08T11:15:13Z

overlord/servicestate/quota_control.go

 }

 func quotaGroupsAvailable(st *state.State) error {
 	// check if the systemd version is too old
-	if systemdVersion < 230 {
-		return fmt.Errorf("systemd version too old: snap quotas requires systemd 230 and newer (currently have %d)", systemdVersion)


I wonder if not showing the detected version could bite us (e.g. if we suddenly encounter a version string we don't parse properly).

Good point. I restored it, though I had to refactor things a bit.

Thanks for the changes, I think they look good. Still a bit unsure about the placement of mocking per my other comment (but feel free to object ;)).

Nope, I simply didn't get to it yet :-)

stolowski · 2021-09-08T11:27:48Z

systemd/systemdtest/systemdtest.go

@@ -59,3 +60,14 @@ Type=simple
 	}
 	return output
 }
+
+func MockSystemdVersion(version int) (restore func()) {


I think we typically put such functions in the respective package (systemd in this case), and then systemd.Version doesn't have to be exported?

Also, I would make it possible to mock the function with complete return value (int, error), not just hardcode one that returns (version, nil).

stolowski · 2021-09-08T11:30:50Z

systemd/systemd.go

+// false.
+func AtLeast(requiredVersion int) bool {
+	version, err := Version()
+	return err == nil && version >= requiredVersion


I think we should log the error somewhere like we did before this change, otherwise we won't know if something went wrong.

I reowrked this a little, please have another look.

bboozzoo · 2021-09-09T05:21:46Z

systemd/systemd.go

+		return err
+	}
+	if version < requiredVersion {
+		return fmt.Errorf(`installed version %d is too old`, version)


Should this be a separate error type? eg.

type systemdTooOldError struct { expected int got int } func (e *systemdTooOldError) Error() string { return fmt.Sprintf("systemd version %d is too old (expected at least %d)", e.got, e.expected) }

Then here:

return &systemdTooOld{got: version, expected: requiredVersion}

then in quotas code you can just return:

return fmt.Errorf("cannot use quotas with incompatible systemd: %v", err)

which would produce eg. cannot use quotas with incompatible systemd: systemd version 123 is too old (expected 345)

Nice suggestion, done!

mvo5

Looks fine, a suggestion about a test inline but with that I think this is a 👍

mvo5 · 2021-09-13T08:52:19Z

systemd/systemd.go

@@ -118,6 +121,17 @@ var systemctlCmd = func(args ...string) ([]byte, error) {
 	return bs, nil
 }

+func MockSystemdVersion(version int, injectedError error) (restore func()) {


The injectedError is not used yet AFAICT but AIUI this will be used in a followup PR(?). Maybe a trivial test that ithe mocking works as expected? in systemd_test.go or something?

No, I don't have a plan to use it, but I might :-) It was actually Pavel who suggested to add the error, and I think it can help to test some corner cases.

I've now added a unit test for it, so it won't appear to be unused anymore :-)

bboozzoo

LGTM

systemd/systemd.go

stolowski · 2021-09-09T15:21:25Z

systemd/systemd.go

+func MockSystemdVersion(version int, injectedError error) (restore func()) {
+	osutil.MustBeTestBinary("cannot mock systemd version outside of tests")
+	old := Version
+	Version = func() (int, error) {
+		return version, injectedError
+	}
+	return func() {
+		Version = old
+	}


I had the usual MockSystemdVersion(f func() (int, err)) (restore .... in mind, but that's fine too and maybe even more handy.

stolowski · 2021-09-09T15:27:58Z

systemd/systemd.go

+func EnsureAtLeast(requiredVersion int) error {
+	version, err := Version()
+	if err != nil {
+		return err
+	}
+	if version < requiredVersion {
+		return &systemdTooOldError{got: version, expected: requiredVersion}
+	}
+	return nil
+}


This should probably have a simple test now in systemd_test.go.

stolowski

LGTM, thank you!

stolowski reviewed Sep 8, 2021

View reviewed changes

bboozzoo reviewed Sep 9, 2021

View reviewed changes

mardy added the Squash-merge Please squash this PR when merging. label Sep 9, 2021

mardy force-pushed the systemd-atleast branch from ec3849c to 8a62deb Compare September 9, 2021 07:57

mvo5 approved these changes Sep 13, 2021

View reviewed changes

bboozzoo approved these changes Sep 13, 2021

View reviewed changes

systemd/systemd.go Show resolved Hide resolved

stolowski reviewed Sep 13, 2021

View reviewed changes

stolowski approved these changes Sep 13, 2021

View reviewed changes

mardy added 5 commits September 13, 2021 13:49

systemd: allow mocking the systemd version

93c7664

o/servicestate: update quota control tests to use MockSystemdVersion

0ef48aa

systemd: have a specific error for old systemd versions

57ea536

usersession: use new systemd.EnsureAtLeast() method

b18b14e

o/configstate/configcore/journal: use systemd.EnsureAtLeast

2a1c3c1

mardy force-pushed the systemd-atleast branch from 973ab87 to 2a1c3c1 Compare September 13, 2021 10:49

tests: update spread test for new error message

912f80e

mardy removed the Squash-merge Please squash this PR when merging. label Sep 13, 2021

mvo5 merged commit 6151af3 into snapcore:master Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemd: add AtLeast() method, add mocking in systemdtest #10748

systemd: add AtLeast() method, add mocking in systemdtest #10748

mardy commented Sep 7, 2021

codecov-commenter commented Sep 7, 2021 •

edited

stolowski left a comment

stolowski Sep 8, 2021

mardy Sep 8, 2021

stolowski Sep 8, 2021

mardy Sep 9, 2021

stolowski Sep 8, 2021

mardy Sep 9, 2021

stolowski Sep 8, 2021

mardy Sep 8, 2021

bboozzoo Sep 9, 2021

mardy Sep 9, 2021

mvo5 left a comment

mvo5 Sep 13, 2021

mardy Sep 13, 2021

bboozzoo left a comment

stolowski Sep 9, 2021

stolowski Sep 9, 2021

mardy Sep 13, 2021

stolowski left a comment

systemd: add AtLeast() method, add mocking in systemdtest #10748

systemd: add AtLeast() method, add mocking in systemdtest #10748

Conversation

mardy commented Sep 7, 2021

codecov-commenter commented Sep 7, 2021 • edited

Codecov Report

stolowski 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

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

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo 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

stolowski left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 7, 2021 •

edited