mount-control: step 1 by mardy · Pull Request #10653 · canonical/snapd

mardy · 2021-08-20T08:26:38Z

This includes the changes in systemd, overlord and wrappers to create, list and remove mount unit files.

miguelpires

Just a small code-focused review, mostly on naming and stuff. I looked at the draft PR to get some context but I'm likely still missing a lot of it =) Didn't look at the tests yet, will do later

miguelpires · 2021-08-20T16:20:05Z

+	PersistentUnit UnitLifetime = iota
+	TransientUnit


Nitpick: "Unit" is repeated across the value and type, and could be removed from the former.
This would be enough IMO:

Suggested change

PersistentUnit UnitLifetime = iota

TransientUnit

Persistent UnitLifetime = iota

Transient

miguelpires · 2021-08-20T16:39:40Z

+			case "FragmentPath":
+				fragmentPath = splitVal[1]
+			default:
+				return nil, fmt.Errorf("Unexpected property %q", splitVal[0])


Error messages should start with lowercase since they're often composed

Suggested change

return nil, fmt.Errorf("Unexpected property %q", splitVal[0])

return nil, fmt.Errorf("unexpected property %q", splitVal[0])

miguelpires · 2021-08-20T16:40:59Z

 	return filepath.Join(dirs.SnapServicesDir, escapedPath+".mount")
 }

+// MountUnitPathWithLifetime returns the path of a transient {,auto}mount unit


I think the word transient is out of place in this comment right?

Yes, it makes no sense -- probably a leftover from a previous formulation :-)

Fixed now.

miguelpires · 2021-08-20T16:50:45Z

 	LogReader(services []string, n int, follow bool) (io.ReadCloser, error)
 	// AddMountUnitFile adds/enables/starts a mount unit.
 	AddMountUnitFile(name, revision, what, where, fstype string) (string, error)
+	// AddMountUnitFileFull adds/enables/starts a mount unit with options.


Suggested change

// AddMountUnitFileFull adds/enables/starts a mount unit with options.

// AddMountUnitFileWithOptions adds/enables/starts a mount unit with options.

miguelpires · 2021-08-20T17:02:45Z

+		for _, line := range lines {
+			splitVal := strings.SplitN(string(line), "=", 2)
+			if len(splitVal) != 2 {
+				return nil, fmt.Errorf("Cannot parse systemctl output: %q", line)


Error messages should start with lowercase since they're often composed

Suggested change

return nil, fmt.Errorf("Cannot parse systemctl output: %q", line)

return nil, fmt.Errorf("cannot parse systemctl output: %q", line)

miguelpires · 2021-08-20T17:19:28Z

+
+		// Is this an error, or are there valid cases when this might happen?
+		if where == "" {
+			continue


I'd say this is an error since the documentation states it's a mandatory option in the mount file but I'll leave people more knowledgeable than me to comment. If, in case of doubt, we want to not fail, we can also log that it happened unexpectedly

Yeah I agree this is probably an error, also the point by which we got here we know that this mount unit was written by us, and we should never purposefully write a mount unit without a Where= so this is an error

codecov-commenter · 2021-08-23T11:16:00Z

Codecov Report

Merging #10653 (fee9abb) into master (0c7afc8) will decrease coverage by 0.00%.
The diff coverage is 82.11%.

@@            Coverage Diff             @@
##           master   #10653      +/-   ##
==========================================
- Coverage   78.35%   78.34%   -0.01%     
==========================================
  Files         888      888              
  Lines       99852   100012     +160     
==========================================
+ Hits        78237    78355     +118     
- Misses      16712    16746      +34     
- Partials     4903     4911       +8

Flag	Coverage Δ
unittests	`78.34% <82.11%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
overlord/snapstate/handlers.go	`70.71% <0.00%> (-0.10%)`	⬇️
systemd/emulation.go	`40.18% <69.23%> (+3.34%)`	⬆️
systemd/systemd.go	`85.94% <83.60%> (-0.46%)`	⬇️
dirs/dirs.go	`94.62% <100.00%> (+0.02%)`	⬆️
overlord/snapstate/backend/mountunit.go	`88.88% <100.00%> (+8.88%)`	⬆️
overlord/hookstate/ctlcmd/refresh.go	`70.88% <0.00%> (-2.50%)`	⬇️
store/cache.go	`69.23% <0.00%> (-1.93%)`	⬇️
daemon/daemon.go	`76.40% <0.00%> (-1.52%)`	⬇️
daemon/api_connections.go	`93.04% <0.00%> (-0.54%)`	⬇️
interfaces/builtin/block_devices.go	`100.00% <0.00%> (ø)`
... and 2 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 0c7afc8...fee9abb. Read the comment docs.

miguelpires · 2021-08-23T15:14:05Z

+
+func (b Backend) RemoveSnapMountUnits(s snap.PlaceInfo, meter progress.Meter) error {
+	if err := wrappers.RemoveMountUnitFiles(s, "", meter); err != nil {
+		logger.Noticef("Cannot remove mount units for %q: %v", s.InstanceName(), err)


I'm not sure what the intended behaviour is here regarding failures. Can you explain to me why is this logged? wrappers.RemoveMountUnitFiles returns an error immediately on failure to remove a mount so, if one removal fails, RemoveMountUnitFiles won't try to remove the others but the caller of this method also can't react since it never gets an error.

You are right, this does not make much sense. It's a leftover from when I moved this code away from link.go, where this actually made sense. I'll fix it. :-)

Thanks 🙂 I think there's some unit tests failing, probably related to this change

miguelpires

LGTM

anonymouse64

some nitpicks, but otherwise lgtm

anonymouse64 · 2021-08-28T00:23:05Z

+type MountUnitOptions struct {
+	// Whether the unit is transient or persistent across reboots
+	Lifetime                                UnitLifetime
+	SnapName, Revision, What, Where, Fstype string


nitpick, I think it's easier to read if these were all on their own line

Suggested change

SnapName, Revision, What, Where, Fstype string

SnapName string

Revision string

What string

Where string

Fstype string

anonymouse64 · 2021-08-28T00:29:05Z

-	content := fmt.Sprintf(mountUnitTemplate, snapName, revision, what, where, fstype, strings.Join(options, ","))
-	mu := MountUnitPath(where)
+const (
+	snappyCreatorModule = "X-SnapdOrigin"


nitpick: if the setting we put into the mount unit file is Origin, can we use Origin instead of Creator for all the properties/variables/etc. here too?

anonymouse64 · 2021-08-28T00:36:44Z

+
+		// Is this an error, or are there valid cases when this might happen?
+		if where == "" {
+			continue


Yeah I agree this is probably an error, also the point by which we got here we know that this mount unit was written by us, and we should never purposefully write a mount unit without a Where= so this is an error

anonymouse64 · 2021-08-28T00:40:58Z

 	}
 }
+
+type Interacter interface {


if the function below is using the exported Interactor from the wrappers package, why do we also introduce one here?

Oops! Removed.

anonymouse64 · 2021-08-28T00:49:19Z

+
+	sysd := New(SystemMode, nil)
+	units, err := sysd.ListMountUnits("some-snap", "")
+	c.Check(units, IsNil)


I have better luck when I expect a list to be empty to use

Suggested change

c.Check(units, IsNil)

c.Check(units, HasLen, 0)

otherwise I will inevitably end up refactoring the function to return an empty list instead of a nil one and the IsNil condition is false

anonymouse64 · 2021-08-28T00:51:05Z

+
+	sysd := New(SystemMode, nil)
+	units, err := sysd.ListMountUnits("some-snap", "")
+	c.Check(units, IsNil)


in error cases usually I will omit the first argument and just check that the error is what we expect it to be, but if you want to keep this case I would suggest changing it like above to be HasLen based

anonymouse64 · 2021-08-28T00:53:08Z

+	c.Check(err, ErrorMatches, "cannot parse systemctl output:.*")
+}
+
+func (s *SystemdTestSuite) TestListMountUnitsHappy(c *C) {


It would be nice to see a test with both units created by AddMountUnitFileWithOptions and then those units returned/found by ListMountUnits, since we expect them to work together IRL too

I don't think it's possible to do it without also mocking the systemctl command, at which point I don't see an advantage in combining the unit file creation and listing in the same tests (because we would still have a mock between them).

pedronis

I didn't quite finish a full review but some things that caught my eye for now

pedronis · 2021-08-30T17:43:30Z

 	"github.com/snapcore/snapd/progress"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/systemd"
+	"github.com/snapcore/snapd/wrappers"


I didn't notice originally that this used systemd directly, from that POV maybe it doesn't make sense to have the helper in wrappers, the code could just live here directly.

pedronis · 2021-08-30T17:44:22Z

 }
+
+func (b Backend) RemoveSnapMountUnits(s snap.PlaceInfo, meter progress.Meter) error {
+	return wrappersRemoveMountUnitFiles(s, "", meter)


relatedly this is mocked in the tests here and the code in wrappers doesn't have unit tests. Anyway as I said maybe the code in wrappers should just move herere because the other functions here use systemd directly anyway...

Removed. For adding more tests, I'd like to get #10704 merged first. Or if you are fine with the current amount of tests, of course we can land this first and add tests later. :-)

pedronis

looks good, some detail comments and questions

pedronis · 2021-09-06T14:33:23Z

baseDir is not the clearest name? wouldn't mountpointDir be a better name?

is this going later to be used outside of systemd? because it's not in the current PR

Done. This is not currently used, but it will be used by the snapctl mount command.

pedronis · 2021-09-06T14:34:09Z

same comments as for the previous helper

pedronis

thanks

mvo5

Looks fine, some nitpicks/ideas inline

mvo5 · 2021-09-09T18:19:19Z

(nitpick^2) I think it's fine to have this and the following in a single line, go has no hard rules here and we are not really consistent but I personally like (slightly) longer lines over this wrapping. But it's fine, no need to change it, just couldn't not comment on it :)

mvo5 · 2021-09-09T18:21:28Z

(nitpick) maybe this shold include the lifetime valid that it did not understand? i.e. panin(fmt.Sprintf("unknown systemd unit lifetime %q", lifetime)

mvo5 · 2021-09-09T18:23:23Z

Could this instead be an error? I am a bit worried about panics in snapd in general and given that we already return errors here at least snap would not crash and potentially run into an endless crash loop (if this is part of a task snapd would keep trying to run the task) with an error instead of a panic?

My reasoning was that passing nil for the option is a programming error, something that in C would be handled by an assert(): panic when running tests (or in debug mode), and invisible in production. But I see that golang has not such a thing, so I think the best option here is to follow your advice and return an error.

mvo5 · 2021-09-09T18:24:57Z

(nitpick^2) I wonder if text.Template would make things easier to read here - probably followup material and maybe/likely not really helpful but another one of my idle thoughts :)

I'll take this suggestion in a follow-up PR, since the change is not trivial and I don't want to delay this too much :-)

Taking a struct with parameters makes the method more extensible. We also add the information about which module caused the creation of the mount unit, and do not write the revision unless explicitly requested.

mardy mentioned this pull request Aug 20, 2021

interfaces/builtin: add mount-control interface #10473

Closed

mardy force-pushed the mount-control-1 branch from 4ac69d7 to df3e4e4 Compare August 20, 2021 08:30

mardy changed the title ~~Mount control interface, step 1~~ mount-control: step 1 Aug 20, 2021

mardy closed this Aug 20, 2021

mardy reopened this Aug 20, 2021

miguelpires reviewed Aug 20, 2021

View reviewed changes

miguelpires reviewed Aug 23, 2021

View reviewed changes

miguelpires approved these changes Aug 26, 2021

View reviewed changes

mardy force-pushed the mount-control-1 branch from 97680d2 to ff32e1f Compare August 26, 2021 11:58

pedronis self-requested a review August 27, 2021 11:59

anonymouse64 approved these changes Aug 28, 2021

View reviewed changes

pedronis reviewed Aug 30, 2021

View reviewed changes

pedronis self-requested a review August 30, 2021 17:59

anonymouse64 added the Needs Samuele review Needs a review from Samuele before it can land label Aug 30, 2021

mardy force-pushed the mount-control-1 branch 6 times, most recently from 994d202 to 9938875 Compare September 6, 2021 08:15

mardy mentioned this pull request Sep 6, 2021

mount-control: step 2 #10739

Merged

pedronis reviewed Sep 6, 2021

View reviewed changes

pedronis self-requested a review September 8, 2021 12:50

pedronis approved these changes Sep 8, 2021

View reviewed changes

mardy force-pushed the mount-control-1 branch from 2ae2e17 to dd472d6 Compare September 9, 2021 08:12

mvo5 approved these changes Sep 9, 2021

View reviewed changes

mardy added 3 commits September 10, 2021 12:12

systemd: add AddMountUnitFileWithOptions()

5771550

Taking a struct with parameters makes the method more extensible. We also add the information about which module caused the creation of the mount unit, and do not write the revision unless explicitly requested.

systemd: allow creating temporary mount units

7d86a7d

systemd: add a method to list mount units created by a snap

ff307c1

mardy added 3 commits September 10, 2021 12:12

tests: mock systemctl output for listing mount units

31cae3c

o/snapstate/backend: add method to remove mount units created by a snap

b14a375

systemd: add Origin to description

793e210

mardy force-pushed the mount-control-1 branch from fee9abb to 793e210 Compare September 10, 2021 09:12

mvo5 removed the Needs Samuele review Needs a review from Samuele before it can land label Sep 10, 2021

mvo5 merged commit 8cbd240 into canonical:master Sep 10, 2021

	return nil, fmt.Errorf("Unexpected property %q", splitVal[0])
	return nil, fmt.Errorf("unexpected property %q", splitVal[0])

	// AddMountUnitFileFull adds/enables/starts a mount unit with options.
	// AddMountUnitFileWithOptions adds/enables/starts a mount unit with options.

	return nil, fmt.Errorf("Cannot parse systemctl output: %q", line)
	return nil, fmt.Errorf("cannot parse systemctl output: %q", line)

-	SnapName, Revision, What, Where, Fstype string
+	SnapName string
+	Revision string
+	What string
+	Where string
+	Fstype string

Conversation

mardy commented Aug 20, 2021

Uh oh!

miguelpires left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miguelpires left a comment

Choose a reason for hiding this comment

Uh oh!

anonymouse64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedronis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 23, 2021 •

edited

Loading