mount-control: step 3 #10864

mardy · 2021-09-30T12:17:57Z

This adds support for persistent mounts, via a snapctl subcommand.

codecov-commenter · 2021-09-30T12:45:40Z

Codecov Report

Merging #10864 (0559698) into master (7594879) will increase coverage by 0.02%.
The diff coverage is 98.15%.

@@            Coverage Diff             @@
##           master   #10864      +/-   ##
==========================================
+ Coverage   78.37%   78.40%   +0.02%     
==========================================
  Files         923      925       +2     
  Lines      105246   105409     +163     
==========================================
+ Hits        82484    82643     +159     
- Misses      17624    17627       +3     
- Partials     5138     5139       +1

Flag	Coverage Δ
unittests	`78.40% <98.15%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
overlord/hookstate/ctlcmd/mount.go	`97.70% <97.70%> (ø)`
interfaces/builtin/mount_control.go	`98.28% <100.00%> (+0.02%)`	⬆️
overlord/hookstate/ctlcmd/umount.go	`100.00% <100.00%> (ø)`
overlord/ifacestate/helpers.go	`76.96% <0.00%> (-0.49%)`	⬇️
cmd/snap/cmd_debug_state.go	`70.64% <0.00%> (+0.45%)`	⬆️
store/cache.go	`71.15% <0.00%> (+1.92%)`	⬆️

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 7594879...0559698. Read the comment docs.

woodrow-shen · 2021-11-10T10:08:11Z

@mardy @pedronis do you think it's reasonable to add mountpoint command into mount-control as we have a use case that can rely on it to query the path is mounted or not?

mardy · 2021-11-11T08:38:12Z

@mardy @pedronis do you think it's reasonable to add mountpoint command into mount-control as we have a use case that can rely on it to query the path is mounted or not?

Yes, to me it makes sense. But I'll let @pedronis make the decision (this will also require a rule like @{PROC}/@{pid}/mountinfo r)

woodrow-shen · 2021-12-13T03:26:36Z

@mardy @pedronis do you think it's reasonable to add mountpoint command into mount-control as we have a use case that can rely on it to query the path is mounted or not?

Yes, to me it makes sense. But I'll let @pedronis make the decision (this will also require a rule like @{PROC}/@{pid}/mountinfo r)

@pedronis hi, do you have any concern about supporting mountpoint in this interface? Thanks.

pedronis

overall looks good, bunch of initial comments

pedronis · 2021-12-14T15:31:37Z

overlord/hookstate/ctlcmd/mount.go

+var (
+	shortMountHelp = i18n.G("Create a temporary or permanent mount")
+	longMountHelp  = i18n.G(`
+The mount command mounts the given source onto the given destination path.`)


the long help should mention mount-control

pedronis · 2021-12-14T15:32:27Z

overlord/hookstate/ctlcmd/mount.go

+		Where string `positional-arg-name:"<where>" required:"yes" description:"path to the destination mount point"`
+	} `positional-args:"yes" required:"yes"`
+	Persistent  bool   `long:"persistent" description:"make the mount persist across reboots"`
+	Type        string `long:"type" short:"t" description:"partition type"`


s/partition/filesystem/

pedronis · 2021-12-14T15:33:06Z

overlord/hookstate/ctlcmd/mount.go

+	sysd := systemd.New(systemd.SystemMode, nil)
+	name, err := m.createMountUnit(sysd)
+	if err != nil {
+		return fmt.Errorf("Failed to create mount unit: %v", err)


error should be lowercase, s/Failed to/cannot/

pedronis · 2021-12-14T15:33:18Z

overlord/hookstate/ctlcmd/mount.go

+	}
+
+	if err := sysd.Start(name); err != nil {
+		// TODO remove mount unit?


yes, that makes sense I think

pedronis · 2021-12-14T15:33:26Z

overlord/hookstate/ctlcmd/mount.go

+
+	if err := sysd.Start(name); err != nil {
+		// TODO remove mount unit?
+		return fmt.Errorf("Failed to start mount unit: %v", err)


same about the error

pedronis · 2021-12-14T15:35:19Z

overlord/hookstate/ctlcmd/umount.go

+var (
+	shortUmountHelp = i18n.G("Undo a temporary or permanent mount")
+	longUmountHelp  = i18n.G(`
+The umount command unmounts the given mount point.`)


umount should mention that the mount point should have been created with snapctl mount

pedronis · 2021-12-14T15:39:07Z

overlord/hookstate/ctlcmd/mount.go

+	}
+
+	if m.Type != "" {
+		// TODO: what should we do if the type is not given?


we should accept Type to be empty only if it's empty also in the attributes. In that case we also will omit from the unit.

overlord/hookstate/ctlcmd/mount.go

pedronis · 2021-12-14T15:42:46Z

overlord/hookstate/ctlcmd/mount.go

+		return false
+	}
+
+	expandedPattern := snapInfo.ExpandSnapVariables(pattern)


afaict the unit tests don't exercise this code, we need test for $SNAP_DATA and $SNAP_COMMON and the valid/invalid combinations with persistent

Added more tests.

I don't see them, forgot to push something?

This code is now tested (it appears green in the coverage report), and the combinatio with persistent is tested here. You didn't mention $SNAP_COMMON in your other comment, so I didn't treat it specially. Let me know if I should.

coverage and tested are not the same thing. I haven't rechecked but if this line is removed no tests fail, so this is not tested.

You are right, I added one test now. I wanted to mock snapInfo.ExpandSnapVariables() but snap.Info is not an interface, so I fell back on using the production code and just compose the "where" in such a way that it would replicate the logic of info.DataDir().

pedronis · 2021-12-14T15:43:35Z

overlord/hookstate/ctlcmd/mount.go

+	st.Lock()
+	defer st.Unlock()
+
+	conns, err := ifacestate.ConnectionStates(st)


for a follow up but now I think we have 2 or 3 places in ctlcmd doing matches against this, maybe they can share a helper? worth checking at least

pedronis

another pass, some feedback changes seems not pushed

pedronis · 2021-12-16T15:13:31Z

overlord/hookstate/ctlcmd/mount.go

@@ -37,7 +38,9 @@ import (
 var (
 	shortMountHelp = i18n.G("Create a temporary or permanent mount")
 	longMountHelp  = i18n.G(`
-The mount command mounts the given source onto the given destination path.`)
+The mount command mounts the given source onto the given destination path,
+provided that the snap has a plug for the mount-control interface which allow


s/allow/allows/

pedronis · 2021-12-16T15:16:22Z

overlord/hookstate/ctlcmd/mount.go

+		return false
+	}
+
+	expandedPattern := snapInfo.ExpandSnapVariables(pattern)


I don't see them, forgot to push something?

pedronis · 2021-12-16T15:20:58Z

overlord/hookstate/ctlcmd/mount.go

+			return false
+		}
+	} else {
+		// The filesystem type was not given; we let it through only if the


is this scenario tested?

Yes, one branch was tested by the existing tests, and this new else is tested with this small addition

I don't see a test for the happy case where both the plug and the command don't have type

The newly added TestHappyWithVariableExpansion() incidentally tests that. Let me know if you'd rather have a separate test.

pedronis

thank you, one question about expanding the spread test

pedronis · 2021-12-17T10:06:44Z

tests/main/interfaces-mount-control/task.yaml

+
+        echo "Verify that a mount not matching the allowed FS type will fail"
+        mkdir -p /media/somedir
+        if "${SNAP_NAME}".cmd mount -t debugfs "/dev/sda" "/media/somedir"; then


would it be possible to add a happy/positive test using -t ?

Ah! I tried it with tmpfs, just to realize that the interface does not really support it. I created #11190 to address the issue (and this also adds a positive test for the -t option)

miguelpires

LGTM

miguelpires · 2022-01-10T17:19:28Z

overlord/hookstate/ctlcmd/umount.go

+		return err
+	}
+
+	if m.Positional.Where == "" {


Isn't this already guaranteed by the required:"yes" in line 43?

It doesn't seem so; at least, the unit test at https://github.com/mardy/snapd/blob/mount-control-3/overlord/hookstate/ctlcmd/umount_test.go#L108-L111 is able to trigger this condition.

It doesn't seem so; at least, the unit test at https://github.com/mardy/snapd/blob/mount-control-3/overlord/hookstate/ctlcmd/umount_test.go#L108-L111 is able to trigger this condition.

The test isn't triggering the go-flags validation because of the empty string arg (I left a comment in the test). Fixing that means we can also remove this.
(I quoted your comment because even though I replied in the thread, it doesn't show in the new review's comment which makes this look like I'm talking to myself)

miguelpires · 2022-01-10T17:30:19Z

overlord/hookstate/ctlcmd/umount.go

+
+	found := false
+	for _, where := range mountPoints {
+		if where == m.Positional.Where {


nitpick: you could invert the condition and continue to remove a level of nesting but the code isn't contrived as it is so might not be worth it

miguelpires · 2022-01-10T18:15:29Z

overlord/hookstate/ctlcmd/mount.go

+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

miguelpires · 2022-01-10T18:15:42Z

overlord/hookstate/ctlcmd/mount_test.go

+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

miguelpires · 2022-01-10T18:15:54Z

overlord/hookstate/ctlcmd/umount.go

+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

miguelpires · 2022-01-10T18:16:02Z

overlord/hookstate/ctlcmd/umount_test.go

+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2021 Canonical Ltd


Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

stolowski

LGTM

stolowski · 2022-01-11T08:55:35Z

overlord/hookstate/ctlcmd/umount.go

+)
+
+var (
+	shortUmountHelp = i18n.G("Undo a temporary or permanent mount")


Nitpick^2, maybe "Remove a temporary.." or "Unmount a temporary.." ?

miguelpires · 2022-01-13T12:37:34Z

overlord/hookstate/ctlcmd/umount_test.go

+	_, _, err := ctlcmd.Run(s.mockContext, []string{"umount", ""}, 0)
+	c.Check(err, ErrorMatches, `mount point cannot be empty`)


This test isn't triggering the go-flags validation because of the empty string arg. Removing it fixes it (which means we also don't need to do the manual validation in umount.go)

Suggested change

_, _, err := ctlcmd.Run(s.mockContext, []string{"umount", ""}, 0)

c.Check(err, ErrorMatches, `mount point cannot be empty`)

_, _, err := ctlcmd.Run(s.mockContext, []string{"umount"}, 0)

c.Check(err, ErrorMatches, "the required argument `<where>` was not provided")

miguelpires · 2022-01-13T12:38:57Z

overlord/hookstate/ctlcmd/umount.go

+		return err
+	}
+
+	if m.Positional.Where == "" {


It doesn't seem so; at least, the unit test at https://github.com/mardy/snapd/blob/mount-control-3/overlord/hookstate/ctlcmd/umount_test.go#L108-L111 is able to trigger this condition.

The test isn't triggering the go-flags validation because of the empty string arg (I left a comment in the test). Fixing that means we can also remove this.
(I quoted your comment because even though I replied in the thread, it doesn't show in the new review's comment which makes this look like I'm talking to myself)

mardy added the ⛔ Blocked label Sep 30, 2021

mardy force-pushed the mount-control-3 branch 3 times, most recently from f4d6bb6 to e19b4d8 Compare October 1, 2021 11:58

pedronis self-requested a review October 7, 2021 13:26

mardy force-pushed the mount-control-3 branch from e19b4d8 to 3807565 Compare December 9, 2021 09:47

mardy removed the ⛔ Blocked label Dec 9, 2021

mardy force-pushed the mount-control-3 branch from 3807565 to 7ea0c54 Compare December 9, 2021 10:18

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Dec 9, 2021

pedronis reviewed Dec 14, 2021

View reviewed changes

pedronis self-requested a review December 16, 2021 14:04

pedronis reviewed Dec 16, 2021

View reviewed changes

pedronis self-requested a review December 17, 2021 09:02

pedronis approved these changes Dec 17, 2021

View reviewed changes

miguelpires approved these changes Jan 10, 2022

View reviewed changes

miguelpires reviewed Jan 10, 2022

View reviewed changes

stolowski approved these changes Jan 11, 2022

View reviewed changes

stolowski mentioned this pull request Jan 11, 2022

o/ifacestate: add convenience Active() method to ConnectionState struct #11233

Merged

mardy and others added 3 commits January 11, 2022 16:06

i/b/mount_control: disallow persistent mounts on $SNAP_DATA

e8e2445

overlord/hookstate/ctlcmd: add mount and umount commands

7279be2

tests: extend mount-control test to verify persistent mounts

ff43166

mardy force-pushed the mount-control-3 branch from a4611cd to ff43166 Compare January 11, 2022 13:09

miguelpires reviewed Jan 13, 2022

View reviewed changes

mvo5 merged commit 235cbb4 into canonical:master Jan 18, 2022

	* Copyright (C) 2021 Canonical Ltd
	* Copyright (C) 2022 Canonical Ltd

		_, _, err := ctlcmd.Run(s.mockContext, []string{"umount", ""}, 0)
		c.Check(err, ErrorMatches, `mount point cannot be empty`)

mount-control: step 3 #10864

mount-control: step 3 #10864

Conversation

mardy commented Sep 30, 2021 • edited Loading

codecov-commenter commented Sep 30, 2021 • edited Loading

Codecov Report

woodrow-shen commented Nov 10, 2021

mardy commented Nov 11, 2021

woodrow-shen commented Dec 13, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Dec 16, 2021 • edited Loading

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

Choose a reason for hiding this comment

miguelpires left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpires Jan 13, 2022 • edited Loading

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

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

miguelpires Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

mardy commented Sep 30, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading

pedronis Dec 16, 2021 •

edited

Loading

miguelpires Jan 13, 2022 •

edited

Loading

miguelpires Jan 13, 2022 •

edited

Loading