daemon, cmd/snapd: propagate context #14130

bboozzoo · 2024-06-26T10:40:49Z

This is another take at #13961, but this time around the branch is attempting at a refactor of how the context is propagated and how it interacts with cancelation.

Related: SNAPDENG-22871

sergio-costas

Little detail, not very important.

sergio-costas · 2024-06-26T10:45:03Z

daemon/api_snaps.go

@@ -145,6 +145,7 @@ func postSnap(c *Command, r *http.Request, user *auth.UserState) Response {
 		return BadRequest("unknown action %s", inst.Action)
 	}

+	// TODO use an epxlicit context parameter


thanks for catching, I've updated the patch

sergio-costas · 2024-06-26T14:02:04Z

Tested with snapd-desktop-integration. Seems to work as expected. (EDIT: at least after the last patch; before it crashed).

Establish a cancelation chain for incoming API requests, to ensure orderly shutdown. This prevents a situation in which an API request, such as notices wait can block snapd shtudown for a long time. Signed-off-by: Maciej Borzecki <[email protected]>

Request's can be canceled based on the code actually issuing a cancel on the associted context, hence an Internal Server Error seems more appropriate. Signed-off-by: Maciej Borzecki <[email protected]>

Signed-off-by: Maciej Borzecki <[email protected]>

Pass the context from the API request further down. Signed-off-by: Maciej Borzecki <[email protected]>

Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo · 2024-06-27T06:43:08Z

there were some conflicts in cmd/snapd/main.go, so I rebased on top of current master

MiguelPires

LGTM

ZeyadYasser

I really like this, Thank you!

ZeyadYasser · 2024-06-27T13:13:46Z

daemon/api_snaps.go

@@ -882,7 +881,7 @@ func meetSnapConstraintsForEnforce(inst *snapInstruction, st *state.State, vErr
 	return snapstateResolveValSetsEnforcementError(context.TODO(), st, vErr, pinnedSeqs, inst.userID)


Do we want to pass context here?

thanks for spotting

pedronis

thanks, I think the changes need a bit more direct testing

pedronis · 2024-06-27T17:59:08Z

daemon/daemon.go

@@ -320,7 +321,7 @@ func (d *Daemon) initStandbyHandling() {
 }

 // Start the Daemon


the doc comment should probably say something about ctx

pedronis · 2024-06-27T17:59:44Z

daemon/daemon.go

+	defer func() {
+		// cancel the context on any errors
+		if err != nil {
+			cancel()


we don't really have tests that show that is relevant, do we ?

not really, but coverage shows this place is hit in TestHandleUnexpectedRestart test

pedronis · 2024-06-27T18:00:02Z

daemon/daemon.go

@@ -482,6 +499,10 @@ func (d *Daemon) Stop(sigCh chan<- os.Signal) error {
 		return fmt.Errorf("internal error: no Overlord")
 	}

+	if d.cancel != nil {
+		d.cancel()


this probably needs an explicit test in daemon_test.go

I"ve added couple more tests that verify context cancelation and propagation, and one which hijacks d.cancel

@ZeyadYasser

Thanks to @ZeyadYasser Signed-off-by: Maciej Borzecki <[email protected]>

Signed-off-by: Maciej Borzecki <[email protected]>

pedronis

thank you, one test question

pedronis · 2024-06-28T08:33:54Z

daemon/daemon_test.go

+	// since the tests aren't in a package, we directly hijack d.cancel
+	c.Assert(d.cancel, check.NotNil)
+	cancel := d.cancel
+	defer cancel()


I'm not entirely sure what this tests is about, shouldn't this check a bit more?

I think I'll drop this one for now. The idea was to check that Stop() wouldn't call cancel if it's nil. Another way to trigger this would be to call Stop(), without a prior call to Start(), but we clearly do not support that and it segfaults on calling d.standbyOpinions.Stop().

The test isn't very useful. Another option to trigger this would be to call Stop() without a prior call to Start(), but this segfaults on d.standbyOpinions.Stop(), so it'c clear this needs a followup fix or callign Stop() this way isn't supported. Signed-off-by: Maciej Borzecki <[email protected]>

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

bboozzoo changed the title ~~Bboozzoo/context cleanup cancel~~ daemon, cmd/snapd: propagate context Jun 26, 2024

bboozzoo closed this Jun 26, 2024

bboozzoo reopened this Jun 26, 2024

bboozzoo removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 26, 2024

bboozzoo requested review from ZeyadYasser, sergio-costas, pedronis and MiguelPires June 26, 2024 10:42

sergio-costas suggested changes Jun 26, 2024

View reviewed changes

bboozzoo force-pushed the bboozzoo/context-cleanup-cancel branch from c87a81d to d57db28 Compare June 26, 2024 10:48

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

bboozzoo removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 26, 2024

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

bboozzoo removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 26, 2024

bboozzoo requested a review from sergio-costas June 26, 2024 14:16

bboozzoo added 6 commits June 27, 2024 08:40

daemon: return 500 when the request context gets canceled

76a3667

Request's can be canceled based on the code actually issuing a cancel on the associted context, hence an Internal Server Error seems more appropriate. Signed-off-by: Maciej Borzecki <[email protected]>

o/snapstate: leave TODOs about using caller provided context

c2f23cc

Signed-off-by: Maciej Borzecki <[email protected]>

daemon: pass down request context where possible

be0cd26

Pass the context from the API request further down. Signed-off-by: Maciej Borzecki <[email protected]>

daemon: set context in snap instruction for many-snap operation

4f43162

Signed-off-by: Maciej Borzecki <[email protected]>

daemon: pass context as an explicit parameter to request handlers

f01e830

Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo force-pushed the bboozzoo/context-cleanup-cancel branch from f80e7ce to f01e830 Compare June 27, 2024 06:42

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

MiguelPires approved these changes Jun 27, 2024

View reviewed changes

ZeyadYasser approved these changes Jun 27, 2024

View reviewed changes

pedronis reviewed Jun 27, 2024

View reviewed changes

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

bboozzoo added 3 commits June 28, 2024 08:56

daemon: pass context

ad9bab2

Thanks to @ZeyadYasser Signed-off-by: Maciej Borzecki <[email protected]>

daemon: comment on Start() taking a context.

4f36e4d

Signed-off-by: Maciej Borzecki <[email protected]>

daemon: add unit tests targeting context passed to Start()

d27c0b8

Signed-off-by: Maciej Borzecki <[email protected]>

bboozzoo requested a review from pedronis June 28, 2024 06:57

pedronis approved these changes Jun 28, 2024

View reviewed changes

bboozzoo merged commit 61d7eba into canonical:master Jun 28, 2024
48 of 51 checks passed

bboozzoo deleted the bboozzoo/context-cleanup-cancel branch June 28, 2024 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon, cmd/snapd: propagate context #14130

daemon, cmd/snapd: propagate context #14130

bboozzoo commented Jun 26, 2024

sergio-costas left a comment

sergio-costas Jun 26, 2024

bboozzoo Jun 26, 2024

sergio-costas commented Jun 26, 2024 •

edited

Loading

bboozzoo commented Jun 27, 2024

MiguelPires left a comment

ZeyadYasser left a comment

ZeyadYasser Jun 27, 2024

bboozzoo Jun 28, 2024

pedronis left a comment

pedronis Jun 27, 2024

pedronis Jun 27, 2024

bboozzoo Jun 28, 2024

pedronis Jun 27, 2024

bboozzoo Jun 28, 2024

pedronis left a comment

pedronis Jun 28, 2024

bboozzoo Jun 28, 2024

		@@ -882,7 +881,7 @@ func meetSnapConstraintsForEnforce(inst snapInstruction, st state.State, vErr
		return snapstateResolveValSetsEnforcementError(context.TODO(), st, vErr, pinnedSeqs, inst.userID)

		@@ -320,7 +321,7 @@ func (d *Daemon) initStandbyHandling() {
		}

		// Start the Daemon

daemon, cmd/snapd: propagate context #14130

daemon, cmd/snapd: propagate context #14130

Conversation

bboozzoo commented Jun 26, 2024

sergio-costas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergio-costas commented Jun 26, 2024 • edited Loading

bboozzoo commented Jun 27, 2024

MiguelPires left a comment

Choose a reason for hiding this comment

ZeyadYasser 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

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

sergio-costas commented Jun 26, 2024 •

edited

Loading