o/hookstate/ctlcmd: queue service commands if run from default-configure hook #13960

ZeyadYasser · 2024-05-10T12:03:28Z

This PR queues snapctl services' commands to be run after the default-configure similar to what is done for configure hook #4070. This is to avoid a problem where the service doesn't see a new value if it uses snapctl get ..., because it's still not committed by default-configure hook similar to what was described in LP#2047949 .

Fixes: https://bugs.launchpad.net/snapd/+bug/2047949

zyga

Looks good.

Please make sure to link updated documentation on the snapcraft forum.

ZeyadYasser · 2024-05-22T08:13:05Z

Please make sure to link updated documentation on the snapcraft forum.

This fix just makes the implementation consistent with the documented behavior https://snapcraft.io/docs/supported-snap-hooks#heading--default-configure and the snapctl docs don't mention this behavior at all.

Do you have a specific doc page in mind that would need updating?

ernestl · 2024-05-27T14:11:20Z

It's not clear why the user still wants to start services from within the default configure hook. My understanding is the the reason for this was specific to the order of configure-hook VS starting services as part of the do-install flow which should not be required anymore unless I am missing something.

A secondary more subtle point, it does not seem like a good idea to start/interfere with service commands before do-install start service is done. Longer term we might want explicitly limit snapctl powers depending on how far the install sequence progressed to avoid creative permutations that cause problems.

ernestl · 2024-05-27T15:24:50Z

It's not clear why the user still wants to start services from within the default configure hook. My understanding is the the reason for this was specific to the order of configure-hook VS starting services as part of the do-install flow which should not be required anymore unless I am missing something.

A secondary more subtle point, it does not seem like a good idea to start/interfere with service commands before do-install start service is done. Longer term we might want explicitly limit snapctl powers depending on how far the install sequence progressed to avoid creative permutations that cause problems.

The reason why the user have service disabled still: They use the snap for core and for classic, and in classic there is no concept of seeding configuration with gadget. They rely on the user manually modifying the configuration and starting the service, so it is required to have it disabled by default.

pedronis

some question/comments

overlord/hookstate/ctlcmd/services_test.go

pedronis · 2024-05-28T16:22:24Z

overlord/hookstate/ctlcmd/helpers.go

@@ -149,7 +149,7 @@ func runServiceCommand(context *hookstate.Context, inst *servicestate.Instructio
 		return err
 	}

-	if !context.IsEphemeral() && context.HookName() == "configure" {
+	if !context.IsEphemeral() && (context.HookName() == "configure" || context.HookName() == "default-configure") {


where do the tasks queued after default-configure exactly end up? it's not very transparent from tests

They are queued at the end after all install tasks
https://github.com/snapcore/snapd/blob/0bf3e39e00948e76ed45cbcc96facee8da181395/overlord/hookstate/ctlcmd/helpers.go#L114-L124

I added clarifying comments in the test

does this mean after the configure hook? does this solve the original issue people were having in ways they expect?

Yes, after both the configure hook and the health-check hook.

For illustration this is the expected order of tasks for installing a snap which is used in the unit tests:

"prerequisites", "download-snap", "validate-snap", "mount-snap", "copy-snap-data", "setup-profiles", "link-snap", "auto-connect", "set-auto-aliases", "setup-aliases", "run-hook[install]", "run-hook[default-configure]", "start-snap-services", "run-hook[configure]", "run-hook[check-health]", "exec-command", <---- task for snapctl command "service-control", <---- task for service operation

I see, we probably want to ask the reporter on the bugs if this is too late for them or ok?

Good point, I will check.

Thanks @ZeyadYasser for the fix and your explanations.

The use case is from a snap that has a one-shot service doing several firewall and network configuration to set up a Thread bridge (linked in the bug report). As mentioned in #13960 (comment), the services are disabled by default, so that the user (on classic but also a pre-built Core image) gets a chance to configure for e.g. the networking interface before starting the services. However, the same snap needs to start automatically on Core when the config is seeded from a gadget. (A workaround was suggested by @zyga to keep services enabled by default but not run them when the needed config isn't set; this could work well for a limited config set.)

In our case, with the workflow proposed in this PR, it could work as follows:

default-configure hook calls snapctl start if the autostart config option is set to true.

configure hook does some validation (not done in default-configure to avoid redundant checks).

services start.

But I suppose this isn't valid in every use case, as some applications expect the configure hook to execute while the services are running.

In summary, while the proposed order isn't an issue for our use case, I believe starting the services right after the hook succeeds makes this more inline with the snap best practices.

Side note: I wonder why run-hook[check-health] is ordered between configure and exec-command/service-control tasks.

Another solution I can think of is to add the ability to just enable/disable services via snapctl without starting or stopping them. Then, the default-configure hook could simply call for e.g. the snapctl enable --service <service> (snapctl enable does something else) command (without needing to queue) and the following task (start-snap-services) should just take care of the startup. I've already discussed this with @ernestl, and I'm not sure if this is going to work. I have experimented by calling snapctl stop --disable <service> from install and default-configure hooks for an enabled service, and it works as expected: the service gets disabled and never starts.

the earliest we can start those services is after snap-start-services, @ZeyadYasser could you look what would be the complexity of queuing commands from default-configure there?

We could have a variant of queueCommand or some special handling code that does the following:

if the service commands are coming from default-configure hook

simply inject the commands' tasksets after start-snap-services using snapstate.InjectTasks

snapstate.InjectTasks takes care of the complexity of adjusting the tasks' dependencies

The PR is now updated to inject service commands after start-snap-services for the default-configure hook.

pedronis

this is fine with me assuming it solves the original problems as is

ZeyadYasser · 2024-06-11T09:16:20Z

I rebased the PR to include test changes from #14032.

bboozzoo

LGTM

pedronis

I think there are issues related to where to inject if lanes are shared like if transaction=all-snaps is used

overlord/hookstate/ctlcmd/helpers.go

this will need re-reviews

pedronis

thank you, test question

pedronis · 2024-07-01T12:27:05Z

overlord/hookstate/ctlcmd/services_test.go

+
+func (s *servicectlSuite) TestQueuedCommandsOrderingConfigureHook(c *C) {
+	const hook = "configure"
+	const singleTransaction = false


don't we need the variant when this is true? or is the check above not working for that case? if we want not to test we should at least leave a comment somewhere

Yes, It doesn't work for configure hook. It is tricky to get snap name for all task kinds in the case of configure hook so I didn't add the single transaction fix for configure hook since it is a non-issue due how commands are queued at the very end .

I added the test (but commented out) with a comment explaining why it is left untested.

pedronis

thanks

bboozzoo

Thanks

…re hook Queue "snapctl restart ..." and "snapctl start ..." commands to be run after default-configure similar to configure hook. This is to avoid a problem where the service doesn't see a new value if it uses "snapctl get ...", because it's still not commited by default-configure hook. Fixes: https://bugs.launchpad.net/snapd/+bug/2047949 Signed-off-by: Zeyad Gouda <[email protected]>

And add comments explaining tasks relative order. Signed-off-by: Zeyad Gouda <[email protected]>

Signed-off-by: Zeyad Gouda <[email protected]>

…t-snap-services Signed-off-by: Zeyad Gouda <[email protected]>

Multiple snaps could be installed in a single transaction where all snap tasksets are in a single lane. The old simplistic approach of looking up the first "start-snap-services" fails when we have multiple tasks for multiple snaps in the same lane. A test is added to trigger this corner case, and the fix just checks the snap name associated with the "start-snap-services" tasks. Signed-off-by: Zeyad Gouda <[email protected]>

…le transaction Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser · 2024-07-03T09:55:08Z

Rebased to fix failing arch tests

ernestl

Thanks

ZeyadYasser requested review from bboozzoo and ernestl May 10, 2024 12:03

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label May 10, 2024

ZeyadYasser force-pushed the default-configure-hook-queue-services branch from 153e4fd to 757129b Compare May 10, 2024 12:22

zyga previously approved these changes May 22, 2024

View reviewed changes

ZeyadYasser marked this pull request as draft May 27, 2024 14:06

ZeyadYasser assigned ZeyadYasser and unassigned ZeyadYasser May 27, 2024

ernestl marked this pull request as ready for review May 27, 2024 15:07

pedronis added the Needs Samuele review Needs a review from Samuele before it can land label May 27, 2024

ernestl requested a review from pedronis May 27, 2024 18:29

pedronis reviewed May 28, 2024

View reviewed changes

ZeyadYasser requested a review from pedronis May 29, 2024 11:58

pedronis approved these changes May 29, 2024

View reviewed changes

pedronis requested review from pedronis and removed request for pedronis May 30, 2024 09:33

ZeyadYasser force-pushed the default-configure-hook-queue-services branch from 91078ff to 42355fc Compare June 11, 2024 09:11

ZeyadYasser requested review from zyga, pedronis and farshidtz June 11, 2024 09:16

bboozzoo previously approved these changes Jun 11, 2024

View reviewed changes

pedronis requested changes Jun 12, 2024

View reviewed changes

overlord/hookstate/ctlcmd/helpers.go Show resolved Hide resolved

ZeyadYasser requested review from pedronis and bboozzoo June 26, 2024 22:40

pedronis reviewed Jul 1, 2024

View reviewed changes

ZeyadYasser requested a review from pedronis July 1, 2024 13:38

pedronis approved these changes Jul 1, 2024

View reviewed changes

bboozzoo approved these changes Jul 2, 2024

View reviewed changes

ZeyadYasser added 6 commits July 3, 2024 12:54

o/hookstate/ctlcmd: refactor TestQueuedCommands into two tests

a02cc40

And add comments explaining tasks relative order. Signed-off-by: Zeyad Gouda <[email protected]>

o/hookstate/ctlcmd: fix typo in TestQueuedCommandsDefaultConfigureHook

c8b968f

Signed-off-by: Zeyad Gouda <[email protected]>

o/hookstate/ctlcmd: inject default-configure hook commands after star…

2c63e0e

…t-snap-services Signed-off-by: Zeyad Gouda <[email protected]>

o/hookstate/ctlcmd: explain why configure hook is not tested for sing…

3bcbee9

…le transaction Signed-off-by: Zeyad Gouda <[email protected]>

ZeyadYasser force-pushed the default-configure-hook-queue-services branch from 56a6bae to 3bcbee9 Compare July 3, 2024 09:54

ZeyadYasser added this to the 2.64 milestone Jul 3, 2024

ernestl approved these changes Jul 4, 2024

View reviewed changes

ernestl merged commit 4669c7c into canonical:master Jul 4, 2024
45 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/hookstate/ctlcmd: queue service commands if run from default-configure hook #13960

o/hookstate/ctlcmd: queue service commands if run from default-configure hook #13960

ZeyadYasser commented May 10, 2024

zyga left a comment

ZeyadYasser commented May 22, 2024

ernestl commented May 27, 2024

ernestl commented May 27, 2024

pedronis left a comment

pedronis May 28, 2024

ZeyadYasser May 29, 2024 •

edited

Loading

ZeyadYasser May 29, 2024

pedronis May 29, 2024

ZeyadYasser May 29, 2024 •

edited

Loading

ZeyadYasser May 29, 2024

farshidtz May 29, 2024 •

edited

Loading

pedronis Jun 5, 2024

ZeyadYasser Jun 5, 2024

ZeyadYasser Jun 11, 2024

pedronis left a comment

ZeyadYasser commented Jun 11, 2024

bboozzoo left a comment

pedronis left a comment

pedronis left a comment

pedronis Jul 1, 2024

ZeyadYasser Jul 1, 2024

pedronis left a comment

bboozzoo left a comment

ZeyadYasser commented Jul 3, 2024

ernestl left a comment

o/hookstate/ctlcmd: queue service commands if run from default-configure hook #13960

o/hookstate/ctlcmd: queue service commands if run from default-configure hook #13960

Conversation

ZeyadYasser commented May 10, 2024

zyga left a comment

Choose a reason for hiding this comment

ZeyadYasser commented May 22, 2024

ernestl commented May 27, 2024

ernestl commented May 27, 2024

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeyadYasser May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farshidtz May 29, 2024 • 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

ZeyadYasser commented Jun 11, 2024

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis left a 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

bboozzoo left a comment

Choose a reason for hiding this comment

ZeyadYasser commented Jul 3, 2024

ernestl left a comment

Choose a reason for hiding this comment

ZeyadYasser May 29, 2024 •

edited

Loading

ZeyadYasser May 29, 2024 •

edited

Loading

farshidtz May 29, 2024 •

edited

Loading