daemon, o/i/apparmorprompting: implement ShutDown for InterfacesRequestsManager - #17147
daemon, o/i/apparmorprompting: implement ShutDown for InterfacesRequestsManager#17147natibek wants to merge 8 commits into
Conversation
github-actions
Bot
added
Needs Documentation -auto-
|
Fri Jul 24 02:14:56 UTC 2026 Failures:Preparing:
Executing:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list of the below tests you wish to run (unskip plus test list must be valid yaml)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17147 +/- ##
==========================================
- Coverage 79.11% 78.87% -0.25%
==========================================
Files 1386 1401 +15
Lines 193780 196251 +2471
Branches 2466 2462 -4
==========================================
+ Hits 153315 154789 +1474
- Misses 31273 32176 +903
- Partials 9192 9286 +94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4bc5889 to
284fd8a
Compare
github-actions
Bot
removed
the
Run only one system
284fd8a to
3801a39
Compare
| // ShutDown stops the listener, prompt DB, and rule DB from receiving new | ||
| // requests. | ||
| func (m *InterfacesRequestsManager) ShutDown() { | ||
| if m.shutdown { |
There was a problem hiding this comment.
seems we might need a lock around this flag? cc @andrewphelpsj
There was a problem hiding this comment.
Yeah I don't think we need a dedicated bool to mark whether this has already been done, this would be equivalent:
select {
case <-m.snapdShuttingDown:
return
default:
// we're about to shut down for the first time
}But either way Samuele's right and we need some sort of synchronization. I think a sync.Once may be the most semantically clear approach, something like:
m.shutdownOnce.Do(func() {
close(m.snapdShuttingDown)
})then there's no need for a check at all, we can just unconditionally do this and the sync.Once guarantees the function is only invoked once. Atomics under the hood I believe.
Rnfudge02
left a comment
Rnfudge02
left a comment
There was a problem hiding this comment.
Looks good to me, but I agree with Samuele that we probably want a lock inside of the ShutDown function.
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
Thanks, nice work! Two main things:
-
I think we need to close
snapdShuttingDownvia async.Once, rather than a dedicatedbool(which would need to be atomic) --sync.Onceis basically a nice wrapper around an atomic compare-and-exchange which ensures you only call a closure once. -
I think we need a new case in the prompting manager run loop, so it knows to stop waiting for requests once
ShutDownhas been called. I'm not absolutely certain this is correct, since this will causedisconnect()to be called, and thus be equivalent to simply callingm.Stop(). AndAsk()already waits onm.tomb.Dying().
I think the big question is whether we want keep ShutDown and Stop distinct from the POV of the prompting manager. If not, then we can have m.ShutDown()simply callm.Stop(), which accomplishes what we want since everything waits on m.tomb.Dying(). Otherwise, some of the old comments in the test are wrong, any my comment that we should add a select case for m.snapdShuttingDown` to the run loop is also wrong.
My gut says we want these to be distinct, and ShutDown to purely affect the Ask method, not prevent requests from the listener or other API requests to e.g. reply to a prompt. So in that case we don't want to listen on m.snapdShuttingDown in the run loop.
| // ShutDown stops the listener, prompt DB, and rule DB from receiving new | ||
| // requests. | ||
| func (m *InterfacesRequestsManager) ShutDown() { | ||
| if m.shutdown { |
There was a problem hiding this comment.
Yeah I don't think we need a dedicated bool to mark whether this has already been done, this would be equivalent:
select {
case <-m.snapdShuttingDown:
return
default:
// we're about to shut down for the first time
}But either way Samuele's right and we need some sort of synchronization. I think a sync.Once may be the most semantically clear approach, something like:
m.shutdownOnce.Do(func() {
close(m.snapdShuttingDown)
})then there's no need for a check at all, we can just unconditionally do this and the sync.Once guarantees the function is only invoked once. Atomics under the hood I believe.
| // InterfacesRequestsManager needs to stop receiving requests and | ||
| // finish handling existing requests. | ||
| snapdShuttingDown chan struct{} | ||
| shutdown bool |
There was a problem hiding this comment.
| shutdown bool | |
| shutdownOnce sync.Once |
| // favor of a manager-level shutdown triggered by the daemon stopping, most | ||
| // of this comment can be removed. | ||
| mgr.ShutDown() | ||
| c.Check(mgr.Stop(), IsNil) |
There was a problem hiding this comment.
I think we don't need to call mgr.Stop() here anymore, we can do it at the end of the test now instead, like the other tests. That's what the comment was talking about iirc.
We can change this now after adding another case to the run loop which breaks out if m.snapdShuttingDown closes.
| // Now Ask should be waiting for a reply. Stop the manager instead. | ||
| stopResultChan := make(chan error) | ||
| go func() { | ||
| select { | ||
| case stopResultChan <- mgr.Stop(): |
There was a problem hiding this comment.
Should we call ShutDown now instead of Stop? Test needs some tweaks perhaps.
Edit: indeed, this test and the next one should be combined. There's no need to do this in a goroutine at all.
| } | ||
|
|
||
| // Proceed with closing the manager | ||
| close(proceedWithClose) |
There was a problem hiding this comment.
Then perhaps this whole MockListenerWithDelayedClose is unnecessary and can just be MockListener?
There was a problem hiding this comment.
This test should be removed/merged with the one above.
There was a problem hiding this comment.
Or perhaps we want to keep them both, one for if Stop is called without having previously called ShutDown, for some reason.
…hannel and remove test for previous shutdown implementation
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
Thanks, I think the implementation of the snapdShuttingDown channel now looks good.
The inclusion of the check for <-m.snapdShuttingDown in the run loop select statement now means that Stop() and ShutDown() have basically the same effect: stopping the run loop and closing all the backends. But they do so through parallel means (<-m.snapdShuttingDown vs m.tomb.Dying()).
I think we have two options:
- Make
ShutDown()just stopAsk()method calls, but not stop the whole manager and backends --- in this case, we'd want to remove the new case from the run loop'sselect - Make
ShutDown()andStop()have the same effect --- in this case, I think we don't really need two parallel concepts of shutdown/stop signalling: everywhere that<-m.snapdShuttingDownis checked, we also already check<-m.tomb.Dying(), and the effect is the same (albeit with slightly different messages in some cases), so I think we can remove the newm.snapdShuttingDownchannel entirely and just usem.tomb.Dying(), or better yet, just haveShutDown()callStop()or vice versa (probably move existing logic fromStop()intoShutDown()and then haveStop()callShutdown()).
I don't have a strong preference, I'm curious what @pedronis thinks. There's probably not precedent for stopping a manager via the ShutDown() method instead of Stop() (since the former didn't exist until the previous PR), but perhaps that would be the simplest and most consistent approach, and we could merge the two and just use the tomb? Otherwise I think reducing the scope of ShutDown() to just affect the Ask() method would be good too.
| } | ||
| case <-m.snapdShuttingDown: | ||
| logger.Debugf("InterfacesRequestsManager is shutting down") | ||
| break run_loop |
There was a problem hiding this comment.
This causes the loop to break, which causes disconnect() to be called. So the effect of this is that calling ShutDown() causes all the backends to be closed, just like Stop() does.
So basically, ShutDown() has the same effect as Stop() if this case <-m.snapdShuttingDown is added to this select statement in the run loop.
My gut says this is probably not what we want? At least, it will cause Stop() and ShutDown() to have basically the same effect through parallel means. m.snapdShuttingDown and m.tomb.Dying() are checked together everywhere.
There was a problem hiding this comment.
Yeah, that doesn't sound consistent with how ShutDown is used for the HookManager either. Would just be more confusing to conflate the ShutDown and Stop methods.
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
Thanks! Generally +1 from me. Question about the removed test but otherwise looks good.
The fact that we select on both m.snapdShuttingDown and m.tomb.Dying() in the Ask() method select statements suggests that we should perhaps be testing both cases?
And one last thing: I don't know if it's the case, but we might want to ensure that Ask always returns after the manager run loop exits, so that e.g. there's no reply which has been received successfully but not handed off to the Ask() method call which originated it (I think we already handle this case correctly), or no other change in rules which happens to apply to an outstanding Ask() call but the manager is stopped (tomb killed) causing the Ask() call to abort prior to receiving the ensuing response, which is still in flight because the manager handler is still operating and has not got back to the run loop to observe the m.tomb.Dying() yet.
The way we could implement that is removing the m.tomb.Dying() checks from the Ask() method and instead firing off something like this at the start of the run method:
m.tomb.Go(func() error {
<-m.tomb.Dying()
m.shutdownOnce.Do(func() { close(m.snapdShuttingDown) })
})What do you think about this?
| c.Check(err, Equals, prompting_errors.ErrPromptingClosed) | ||
| } | ||
|
|
||
| func (s *apparmorpromptingSuite) TestAskShutdownBeforeReply(c *C) { |
There was a problem hiding this comment.
It's probably fine to remove this test, but does it decrease coverage to do so? I think we end up testing the snapdShuttingDown cases but now missing the m.tomb.Dying() cases in the select statements, yes?
| } | ||
|
|
||
| // XXX: this test only exists since there are currently two ways to tell Ask to | ||
| // stop waiting: the manager closing, and the snapdShuttingDown channel closing. |
There was a problem hiding this comment.
That is, there are still two ways to tell Ask to stop waiting. And I think there will always be two ways if we want Stop() and ShutDown() to coexist and be different.
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
I discussed this a bit with Samuele today. Here are some key takeaways:
- Conceptually,
ShutDown()means "get into a state such that we don't expect requests from the snaps anymore"- This is very clear for the hook manager: snap hooks can do many things, such as install a component, which is a call to the snapd API. If the snapd-snap.socket were closed, then that hook's call to the API would fail, so the hook would fail, so we need to ensure that hooks are done making any calls to the API before we shut down the daemon socket.
- For the prompting manager, it's a bit different: the daemon socket can't close until all outstanding connections close, and if one of those connections has called
Ask(), that call would block untilStop()is called. ButStop()isn't called until after the daemon has closed the socket. So there would be a deadlock without a way to stop theAsk()calls prior to closing the daemon socket. - Other API calls and calls into the prompting backends are more short-lived, and don't come from something running within snapd which needs to be done before closing the socket. So
Ask()is different and makes sense to be stopped early without closing the rest of the prompting backends.
- There may be a long delay between
ShutDown()being called andStop(), and we want to ensure that the prompting manager doesn't receive theShutDown()call until as late in that process as possible- The hook manager may wait up to 10 minutes for hooks to finish
- Blocking
Ask()requests for 10 minutes is not good, as it will block services like wireplumber from connecting streams for snaps due to retrying repeatedly after the503response - Manager methods (e.g.
ShutDown()) are IIRC called sequentially on each of the managers, so we want to make sureShutDown()is called on the hook manager before the prompting manager -- this is probably already the case, but want to confirm
And regarding this:
And one last thing: I don't know if it's the case, but we might want to ensure that
Askalways returns after the manager run loop exits, so that e.g. there's no reply which has been received successfully but not handed off to theAsk()method call which originated it (I think we already handle this case correctly), or no other change in rules which happens to apply to an outstandingAsk()call but the manager is stopped (tomb killed) causing theAsk()call to abort prior to receiving the ensuing response, which is still in flight because the manager handler is still operating and has not got back to the run loop to observe them.tomb.Dying()yet.
I was wrong, I think this is already handled correctly. If a reply call is received by the API, it needs to successfully call the Reply() method on the request, which calls sendResponse(), which contains a select on m.snapdShuttingDown to return an error. So the Reply() call will error out and that is passed all the way back up to the API, which results in a 503 response to the reply API request. So everything is fine here.
Your latest change around making m.snapdShuttingDown only affect Ask() is the correct decision.
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
Thanks! Looks good, just one last comment about code organization in the interface manager.
| m.interfacesRequestsManagerMu.Lock() | ||
| defer m.interfacesRequestsManagerMu.Unlock() | ||
| if m.interfacesRequestsManager == nil { | ||
| return | ||
| } | ||
| interfacesRequestsManagerShutDown(m.interfacesRequestsManager) |
There was a problem hiding this comment.
It may be worth moving this into a dedicated shutDownInterfacesRequestsManager() method, like we have for stopInterfacesRequestsManager(), since the InterfacesRequestsManager is one of several managers managed by the InterfaceManager.
…nterfaceManager to shutDownInterfacesRequestsManger
olivercalder
left a comment
olivercalder
left a comment
There was a problem hiding this comment.
LGTM, thanks for all your work on this!
github-actions
Bot
added
the
Auto rerun spread
pedronis
left a comment
pedronis
left a comment
There was a problem hiding this comment.
looking good, question about the API we are defining though
| c.Assert(s.se.Ensure(), IsNil) | ||
| } | ||
| s.se.ShutDown() | ||
| s.se.Stop() |
There was a problem hiding this comment.
@olivercalder this poses an interesting question, should Stop imply ShutDown if ShutDown was not called yet, or we don't strictly needs this?
There was a problem hiding this comment.
We don't strictly need it, as everything which select on m.snapdShuttingDown still selects on m.tomb.Dying(), which is killed by Stop().
But conceptually this could be a nice thing to implement at the overlord/StateEngine level, I think. Or we could leave it up to each manager to ensure it is implemented correctly, as needed. I'm not sure, but we should decide on one of the choices and document that choice.
There was a problem hiding this comment.
@olivercalder @natibek I think the way to look at this is that Stop should work correctly even if ShutDown was not called and stop any (remaining) manager activity, so ShutDown is just a way to phase things (that's not too dissimilar for how shutdown and close work for sockets)
4 participants
Implement
ShutDownfor theInterfacesRequestsManagerto replace the existing shutdown process that usesc.d.tomb.Dying()to signalsnapdShuttingDown.Tracked with: SNAPDENG-36591