Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State engine Ensure() errors are only logged, not handled #445

Open
thp-canonical opened this issue Jul 9, 2024 · 0 comments
Open

State engine Ensure() errors are only logged, not handled #445

thp-canonical opened this issue Jul 9, 2024 · 0 comments
Labels
25.04 Planned for the 25.04 cycle Simple Nice for a quick look on a minute or two

Comments

@thp-canonical
Copy link
Contributor

Right now in Overlord.Loop(), the error return of the state engine's Ensure() function are ignored:

// in case of errors engine logs them,
// continue to the next Ensure() try for now
o.stateEng.Ensure()

Meanwhile, snapd handles errors - at least in the pressed case - in Ensure():

https://github.com/snapcore/snapd/blob/c59a5f6e87fe59596b14614bc422e0c3a132ca25/overlord/overlord.go#L463-L473

This has been implemented in snapd for preseeding: canonical/snapd#8190

As other components of Pebble might have the same issue (retrying Ensure() forever not resulting in a change), and implementers of managers might believe that an error returned from Ensure() will be handled by the caller, it might make sense to at least allow an optional error handler to handle Ensure() errors in the overlord loop.

Found with @flotter during code review.

@benhoyt benhoyt added Simple Nice for a quick look on a minute or two 25.04 Planned for the 25.04 cycle labels Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
25.04 Planned for the 25.04 cycle Simple Nice for a quick look on a minute or two
Projects
None yet
Development

No branches or pull requests

2 participants