wrappers,tests: restart always enabled units #12670

alfonsosanchezbeato · 2023-03-23T15:16:22Z

Due to a previous change we were not restarting any non-running unit, but actually that should happen only if the unit is also disabled. Otherwise, stopping and the restarting will not work. Fixes LP#2012630. For reference, see [1], which looks like it might have been misinterpreted.

[1] https://forum.snapcraft.io/t/command-line-interface-to-manipulate-services/262/46

pedronis

thanks, as we discussed we should check if there is any intersection with the behavior around handling services during refreshes, I suspect not, but worth double checking

alfonsosanchezbeato · 2023-03-31T08:10:50Z

thanks, as we discussed we should check if there is any intersection with the behavior around handling services during refreshes, I suspect not, but worth double checking

Refreshes do "stop-snap-services" and later a "start-snap-services". The former takes a list of the disabled services, not considering the running state. The latter restart the snap services but those in the list previously considered, so it looks like that behavior is already coherent with this PR.

RestartServices is called when doing a "snap restart" and when quota configuration is changed. For the latter, maybe the new behavior changes things (I do not know how automatic are these restarts) - cc @Meulengracht as maybe he can comment.

Meulengracht · 2023-03-31T08:41:07Z

thanks, as we discussed we should check if there is any intersection with the behavior around handling services during refreshes, I suspect not, but worth double checking

Refreshes do "stop-snap-services" and later a "start-snap-services". The former takes a list of the disabled services, not considering the running state. The latter restart the snap services but those in the list previously considered, so it looks like that behavior is already coherent with this PR.

RestartServices is called when doing a "snap restart" and when quota configuration is changed. For the latter, maybe the new behavior changes things (I do not know how automatic are these restarts) - cc @Meulengracht as maybe he can comment.

I checked up on how we restart services in quota groups, and we do this when moving snaps in and out of service groups, were we restart services for that snap. It will of course change the behaviour, but I don't see this as an issue for the quota case.

pedronis · 2023-04-04T08:33:58Z

I checked up on how we restart services in quota groups, and we do this when moving snaps in and out of service groups, were we restart services for that snap. It will of course change the behaviour, but I don't see this as an issue for the quota case.

are we saying that now changing a quota group might restart services that were enabled but not running? it sounds like we might need different behavior for that vs snap(ctl) restart requests?

alfonsosanchezbeato · 2023-04-05T07:12:18Z

I checked up on how we restart services in quota groups, and we do this when moving snaps in and out of service groups, were we restart services for that snap. It will of course change the behaviour, but I don't see this as an issue for the quota case.

are we saying that now changing a quota group might restart services that were enabled but not running? it sounds like we might need different behavior for that vs snap(ctl) restart requests?

Yes, to me it sounds like that too. Restarting when moving snaps across quota groups sounds like something transparent to the user that should respect the current state of the processes, which is different from requiring a restart from API.

alfonsosanchezbeato · 2023-04-11T09:18:13Z

I've added now a flag to discriminate between restarting active services or not.

pedronis

question about the signature change vs consistency, otherwise this looks reasonable

pedronis · 2023-04-11T11:11:26Z

wrappers/services.go

@@ -1804,7 +1808,7 @@ type RestartServicesFlags struct {
 // TODO: change explicitServices format to be less unusual, more consistent
 // (introduce AppRef?)
 func RestartServices(svcs []*snap.AppInfo, explicitServices []string,
-	flags *RestartServicesFlags, inter Interacter, tm timings.Measurer) error {
+	flags RestartServicesFlags, inter Interacter, tm timings.Measurer) error {


this change is a bit strange because now StopService and StartServices are inconsistent, I would leave things as they were

I've reverted this part. In general I would prefer to force having to pass the parameter as it is more explicit and avoids checks for nil, but as things are the same for StopServices I guess that should be its own PR.

pedronis

thanks

alfonsosanchezbeato · 2023-04-13T10:29:28Z

History rewritten with no code changes

alfonsosanchezbeato · 2023-04-14T09:30:34Z

I've pushed now changes to fix the spread failures.

pedronis

questions

tests/main/services-restart/task.yaml

pedronis · 2023-04-14T11:35:37Z

overlord/servicestate/service_control.go

+	// RestartOnlyActive is only for "restart" and "reload-or-restart"
+	// actions, and when set it restarts only the active services, that is,
+	// enabled non-running services are ignored.
+	RestartOnlyActive bool `json:"restart-only-active,omitempty"`


the default of this is wrong for tasks that were made by old snapd though?

Right, but I'm not sure on the concern here, tasks from older changes would have been already run by the old snapd, and anyway we are changing behavior, it does probably not matter if it affects a task created before snapd refreshes?

independent changes might be running while we update snapd, and it matters again if the tasks were about quota groups, I fear we need to make the default be the old behavior and deal with that

I've reverted the flag now

pedronis

more comments

tests/main/services-restart/task.yaml

pedronis · 2023-04-14T15:45:38Z

overlord/servicestate/quota_handlers.go

+			Action:                  "restart",
+			SnapName:                info.InstanceName(),
+			Services:                getServiceNames(servicesAffected[info]),
+			RestartEnabledNonActive: false,


here we know we want false, but I suspect that there maybe other places where we want to set this to true? where do we also create ServiceAction tasks?

Right, I forgot to change that, I've pushed changes to fix it.

pedronis

thx

Due to a previous decision [1] we were not restarting any non-running unit, this commit changes that back so non-running units are not restarted only if the unit is also disabled. This way, stopping and restarting works more similarly to how it is usually expected. Fixes LP#2012630. [1] https://forum.snapcraft.io/t/command-line-interface-to-manipulate-services/262/46

Now all enabled services are restarted.

alfonsosanchezbeato · 2023-04-17T13:51:56Z

History rewritten and re-pushed now

alfonsosanchezbeato requested a review from pedronis March 23, 2023 15:16

alfonsosanchezbeato force-pushed the restart-enabled-units branch from a1fda5d to 1a7cce5 Compare March 23, 2023 17:10

ernestl approved these changes Mar 29, 2023

View reviewed changes

mvo5 added this to the 2.59 milestone Mar 30, 2023

pedronis reviewed Mar 30, 2023

View reviewed changes

alfonsosanchezbeato requested review from pedronis and Meulengracht April 11, 2023 09:18

pedronis reviewed Apr 11, 2023

View reviewed changes

alfonsosanchezbeato requested a review from pedronis April 12, 2023 16:07

pedronis approved these changes Apr 13, 2023

View reviewed changes

alfonsosanchezbeato force-pushed the restart-enabled-units branch from 678971c to 08da779 Compare April 13, 2023 10:28

alfonsosanchezbeato requested review from pedronis and ernestl April 14, 2023 09:30

pedronis reviewed Apr 14, 2023

View reviewed changes

alfonsosanchezbeato requested a review from pedronis April 14, 2023 14:35

pedronis reviewed Apr 14, 2023

View reviewed changes

pedronis self-requested a review April 17, 2023 12:45

pedronis approved these changes Apr 17, 2023

View reviewed changes

alfonsosanchezbeato added 2 commits April 17, 2023 14:38

tests: adapt to changes in restart behavior

c6ba5f9

Now all enabled services are restarted.

alfonsosanchezbeato force-pushed the restart-enabled-units branch from f80cdb6 to c6ba5f9 Compare April 17, 2023 13:43

mvo5 merged commit 130ddaa into snapcore:master Apr 18, 2023
47 of 50 checks passed

alfonsosanchezbeato mentioned this pull request Apr 27, 2023

overlord,wrappers: restart always enabled units #12785

Merged

mvo5 added the cherry-picked label May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wrappers,tests: restart always enabled units #12670

wrappers,tests: restart always enabled units #12670

alfonsosanchezbeato commented Mar 23, 2023

pedronis left a comment

alfonsosanchezbeato commented Mar 31, 2023

Meulengracht commented Mar 31, 2023

pedronis commented Apr 4, 2023

alfonsosanchezbeato commented Apr 5, 2023

alfonsosanchezbeato commented Apr 11, 2023

pedronis left a comment

pedronis Apr 11, 2023

alfonsosanchezbeato Apr 12, 2023

pedronis left a comment

alfonsosanchezbeato commented Apr 13, 2023

alfonsosanchezbeato commented Apr 14, 2023

pedronis left a comment

pedronis Apr 14, 2023

alfonsosanchezbeato Apr 14, 2023

pedronis Apr 14, 2023

alfonsosanchezbeato Apr 14, 2023

pedronis left a comment

pedronis Apr 14, 2023

alfonsosanchezbeato Apr 17, 2023

pedronis left a comment

alfonsosanchezbeato commented Apr 17, 2023

wrappers,tests: restart always enabled units #12670

wrappers,tests: restart always enabled units #12670

Conversation

alfonsosanchezbeato commented Mar 23, 2023

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Mar 31, 2023

Meulengracht commented Mar 31, 2023

pedronis commented Apr 4, 2023

alfonsosanchezbeato commented Apr 5, 2023

alfonsosanchezbeato commented Apr 11, 2023

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

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Apr 13, 2023

alfonsosanchezbeato commented Apr 14, 2023

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

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

pedronis left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato commented Apr 17, 2023