mount-control: step 3 by mardy · Pull Request #10864 · canonical/snapd

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

the long help should mention mount-control

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

s/partition/filesystem/

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

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

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

yes, that makes sense I think

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

same about the error

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

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

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

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.

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

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

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

s/allow/allows/

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

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

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

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

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

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

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

Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

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

Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

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

Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

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

Suggested change

* Copyright (C) 2021 Canonical Ltd

* Copyright (C) 2022 Canonical Ltd

stolowski

LGTM

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

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

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

+	_, _, 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

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`)

Conversation

mardy commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

woodrow-shen commented Nov 10, 2021

Uh oh!

mardy commented Nov 11, 2021

Uh oh!

woodrow-shen commented Dec 13, 2021

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

pedronis Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

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