overlord,systemd: do not restart mount units on snapd start #13685

alfonsosanchezbeato · 2024-03-11T18:42:51Z

Do not restart the mount units when starting snapd even if the file
itself has been modified. This is to prevent systemd stopping services
when the mount unit gets restarted, as there are required dependencies
in the mount path for services defined in snaps.

pedronis

thanks, so this is beyond just snapd itself, as we turn off restarting of all mounts in snapmgr?

pedronis · 2024-03-12T08:50:04Z

systemd/systemd_test.go

@@ -1160,7 +1160,7 @@ func (s *SystemdTestSuite) TestAddMountUnit(c *C) {
 	mockSnapPath := filepath.Join(c.MkDir(), "/var/lib/snappy/snaps/foo_1.0.snap")
 	makeMockFile(c, mockSnapPath)

-	mountUnitName, err := NewUnderRoot(rootDir, SystemMode, nil).EnsureMountUnitFile("Mount unit for foo, revision 42", mockSnapPath, "/snap/snapname/123", "squashfs")
+	mountUnitName, err := NewUnderRoot(rootDir, SystemMode, nil).EnsureMountUnitFile("Mount unit for foo, revision 42", mockSnapPath, "/snap/snapname/123", "squashfs", &systemd.EnsureMountUnitFlags{})


do we want to add a test with IgnoreModified: true here as well?

Added now (see testEnsureMountUnitChanged)

valentindavid

Looks good. I have left some comments more for discussion. I think the PR is fine to merge if the tests pass.

valentindavid · 2024-03-12T09:10:07Z

systemd/systemd.go

@@ -323,6 +323,9 @@ type MountUnitOptions struct {
 	Fstype      string
 	Options     []string
 	Origin      string
+	// IgnoreModified is set if we do not want to restart the
+	// mount unit if modified
+	IgnoreModified bool


I am not sure this is obvious from the name. Maybe PreventRestartModified?

valentindavid · 2024-03-12T09:11:55Z

systemd/systemd.go

@@ -1508,19 +1518,20 @@ func hostFsTypeAndMountOptions(fstype string) (hostFsType string, options []stri
 	return hostFsType, options
 }

-func (s *systemd) EnsureMountUnitFile(description, what, where, fstype string) (string, error) {
+func (s *systemd) EnsureMountUnitFile(description, what, where, fstype string, flags *EnsureMountUnitFlags) (string, error) {


I wonder if we should just not use this function when we need to set the flag, but use EnsureMountUnitFileWithOptions.

I thought about that, but hostFsTypeAndMountOptions would need to be public as well or build fstype/options slightly differently. Maybe something to consider in the future, I've added a TODO about this now.

valentindavid · 2024-03-12T09:15:01Z

systemd/systemd.go

-			return "", err
+
+		// If just modified, some times it is not convenient to restart
+		if modified != mountUpdated || !unitOptions.IgnoreModified {


In the else case, we could make a reload. I think that will safely change the options if they changed with a -o remount.

Isn't the previous call to daemon-reload already reloading the unit configuration? Afaiu, reload-or-restart would only run anything under ExecReload= in the unit if present, or restart otherwise.

daemon-reload does not do the remount. Maybe it is not well documented in systemd, but the mount unit has a reload. See https://github.com/systemd/systemd/blob/7b44a24c1bc764b5c0555fed08911665e1e7d446/src/core/mount.c#L2437

That calls in the end mount_enter_remounting https://github.com/systemd/systemd/blob/7b44a24c1bc764b5c0555fed08911665e1e7d446/src/core/mount.c#L1228

Which will use -o remount.

That said, -o remount does not support all changes. For example, you cannot change the device.

valentindavid · 2024-03-12T09:18:57Z

overlord/snapstate/snapmgr.go

+			// Ensure mount files, but do not restart mount units
+			// of snap files if the units are modified as services
+			// in the snap have a Requires= on them. Otherwise the
+			// services would be restarted.
+			if _, err = sysd.EnsureMountUnitFile(info.MountDescription(),
+				squashfsPath, whereDir, "squashfs",
+				&systemd.EnsureMountUnitFlags{IgnoreModified: true}); err != nil {
 				return err
 			}


I see you decided not to restart any mount units not just snapd. I think this is fine. Could you add in the comment, that if we decided to revert that we should never restart the mount for snapd otherwise snapd will kill itself on first start after an update?

Added an additional comment about snapd

alfonsosanchezbeato

Thanks for the reviews, I've answered the comments and squashed the commits.

alfonsosanchezbeato · 2024-03-12T10:43:41Z

overlord/snapstate/snapmgr.go

+			// Ensure mount files, but do not restart mount units
+			// of snap files if the units are modified as services
+			// in the snap have a Requires= on them. Otherwise the
+			// services would be restarted.
+			if _, err = sysd.EnsureMountUnitFile(info.MountDescription(),
+				squashfsPath, whereDir, "squashfs",
+				&systemd.EnsureMountUnitFlags{IgnoreModified: true}); err != nil {
 				return err
 			}


Added an additional comment about snapd

alfonsosanchezbeato · 2024-03-12T10:43:58Z

systemd/systemd.go

@@ -323,6 +323,9 @@ type MountUnitOptions struct {
 	Fstype      string
 	Options     []string
 	Origin      string
+	// IgnoreModified is set if we do not want to restart the
+	// mount unit if modified
+	IgnoreModified bool


alfonsosanchezbeato · 2024-03-12T10:55:38Z

systemd/systemd.go

-			return "", err
+
+		// If just modified, some times it is not convenient to restart
+		if modified != mountUpdated || !unitOptions.IgnoreModified {


Isn't the previous call to daemon-reload already reloading the unit configuration? Afaiu, reload-or-restart would only run anything under ExecReload= in the unit if present, or restart otherwise.

alfonsosanchezbeato · 2024-03-12T10:58:18Z

systemd/systemd.go

@@ -1508,19 +1518,20 @@ func hostFsTypeAndMountOptions(fstype string) (hostFsType string, options []stri
 	return hostFsType, options
 }

-func (s *systemd) EnsureMountUnitFile(description, what, where, fstype string) (string, error) {
+func (s *systemd) EnsureMountUnitFile(description, what, where, fstype string, flags *EnsureMountUnitFlags) (string, error) {


I thought about that, but hostFsTypeAndMountOptions would need to be public as well or build fstype/options slightly differently. Maybe something to consider in the future, I've added a TODO about this now.

codecov-commenter · 2024-03-12T11:16:39Z

Codecov Report

Attention: Patch coverage is 87.87879% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 78.89%. Comparing base (81d3d51) to head (2fad2d1).
Report is 7 commits behind head on master.

Files	Patch %	Lines
systemd/systemd.go	76.92%	2 Missing and 1 partial ⚠️
overlord/snapstate/snapmgr.go	90.00%	0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13685   +/-   ##
=======================================
  Coverage   78.89%   78.89%           
=======================================
  Files        1040     1040           
  Lines      133862   133876   +14     
=======================================
+ Hits       105610   105624   +14     
- Misses      21658    21659    +1     
+ Partials     6594     6593    -1

Flag	Coverage Δ
unittests	`78.89% <87.87%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zyga

LGTM with one comment about passing flags by value and one comment about wording of the documentation.

zyga · 2024-03-12T11:33:18Z

systemd/emulation.go

@@ -124,20 +124,21 @@ func (s *emulation) LogReader(services []string, n int, follow, namespaces bool)
 	return nil, fmt.Errorf("LogReader")
 }

-func (s *emulation) EnsureMountUnitFile(description, what, where, fstype string) (string, error) {
+func (s *emulation) EnsureMountUnitFile(description, what, where, fstype string, flags *EnsureMountUnitFlags) (string, error) {


Passing EnsureMountUnitFlags by value is probably better here. It's less worrying about escape analysis to optimize this away, less chance of a nil pointer.

Changed now

zyga · 2024-03-12T11:34:09Z

systemd/systemd.go

+// remove this type instead?
+type EnsureMountUnitFlags struct {
+	// PreventRestartIfModified is set if we do not want to restart the
+	// mount unit if modified


Suggested change

// mount unit if modified

// mount unit even thought it has been modified.```

Do not restart the mount units when starting snapd even if the file itself has been modified. This is to prevent systemd stopping services when the mount unit gets restarted, as there are required dependencies in the mount path for services defined in snaps.

related to remounting units for installed snaps if we have new options set in snapd.

alfonsosanchezbeato added the Run nested The PR also runs tests inluded in nested suite label Mar 11, 2024

alfonsosanchezbeato added this to the 2.62 milestone Mar 11, 2024

alfonsosanchezbeato requested review from valentindavid and zyga March 11, 2024 18:42

alfonsosanchezbeato force-pushed the not-restart-snapd-snap-mounts branch from 6dd279d to 5bfb319 Compare March 12, 2024 08:10

alfonsosanchezbeato changed the title ~~overlord,systemd: do not restart snapd snap mount units on start~~ overlord,systemd: do not restart mount units on snapd start Mar 12, 2024

pedronis added the ⚠ Critical High-priority stuff (e.g. to fix master) label Mar 12, 2024

pedronis approved these changes Mar 12, 2024

View reviewed changes

valentindavid approved these changes Mar 12, 2024

View reviewed changes

alfonsosanchezbeato force-pushed the not-restart-snapd-snap-mounts branch from 5bfb319 to 54e4291 Compare March 12, 2024 11:02

alfonsosanchezbeato commented Mar 12, 2024

View reviewed changes

zyga approved these changes Mar 12, 2024

View reviewed changes

alfonsosanchezbeato force-pushed the not-restart-snapd-snap-mounts branch from 54e4291 to 2fad2d1 Compare March 12, 2024 13:04

tests/main/snapd-update-services: adapt to change in snapd behavior

a2be009

related to remounting units for installed snaps if we have new options set in snapd.

alfonsosanchezbeato force-pushed the not-restart-snapd-snap-mounts branch from 7bb4cea to a2be009 Compare March 13, 2024 15:28

alfonsosanchezbeato merged commit 253256f into snapcore:master Mar 13, 2024
38 of 44 checks passed

olivercalder mentioned this pull request Mar 13, 2024

client,daemon,features: expose feature flags supported/enabled info #13681

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlord,systemd: do not restart mount units on snapd start #13685

overlord,systemd: do not restart mount units on snapd start #13685

alfonsosanchezbeato commented Mar 11, 2024 •

edited

pedronis left a comment

pedronis Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

valentindavid left a comment

valentindavid Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

valentindavid Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

valentindavid Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

valentindavid Mar 12, 2024

valentindavid Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

codecov-commenter commented Mar 12, 2024 •

edited

zyga left a comment

zyga Mar 12, 2024

alfonsosanchezbeato Mar 12, 2024

zyga Mar 12, 2024

	// mount unit if modified
	// mount unit even thought it has been modified.```

overlord,systemd: do not restart mount units on snapd start #13685

overlord,systemd: do not restart mount units on snapd start #13685

Conversation

alfonsosanchezbeato commented Mar 11, 2024 • edited

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

valentindavid 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

alfonsosanchezbeato 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

codecov-commenter commented Mar 12, 2024 • edited

Codecov Report

zyga 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

alfonsosanchezbeato commented Mar 11, 2024 •

edited

codecov-commenter commented Mar 12, 2024 •

edited