canonical / pebble

Take control of your internal daemons!
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
143 stars 54 forks source link

feat(stateengine): merge `StateStarterUp` interface from snapd #327

Closed anpep closed 9 months ago

anpep commented 9 months ago

This PR complements (or even replaces) #214. The aim is for state managers to perform expensive startup operations in a StartUp() method. This way, we could either:

The use cases are:

  1. Allowing dry run scenarios for e.g. validating the plan without actually running the daemon, or in general attempt to run Pebble without actually running it so that we can ensure e.g. that Pebble is running in the proper environment prior to actually starting.
  2. Move all manager initialization code that requires reading or writing manager state to StartUp(). Right now, Pebble assumes that the state backend is ready, but in some cases this does not hold true (e.g. we're running in a system that requires the overlord to be initialized before mounting storage). By introducing this barrier for state readiness, we can ensure (through a manager included in the overlord extension, that should be started up before the rest of the managers) that the system is ready for this.
benhoyt commented 9 months ago

Thanks! @dmitry-lyfar was working on this too (or maybe you spoke with him about it?). See also this comment where Gustavo said we do want to pull this over from snapd. @dmitry-lyfar any comments?

dmitry-lyfar commented 9 months ago

@benhoyt @anpep that to me looks very much like the snapd's approach except a couple of relatively minor differences: pebble's own multiError; no recording of the operation start time in overlord.StartUp that is later on used for the changes pruning.

Having said that, @anpep I suppose you have more logic to be pulled in as the daemon module has not been changed here (which changes quite a bit of daemon unit tests)?

Another observation is that this change does introduce the StateStartUp interface to the StateEngine, but the state engine's own Ensure remains unaware of the new startedUp flag. Which, if we compare it to the snapd's StateEngine.Ensure(), is used to decide whether to make the ensure pass over the list of managers or not. It makes the changeset logic a bit fragmented, as the Ensure's usage of the startedUp flag is also a part of the StateEngine implementation of the interface. That includes its unit tests, e.g. TestEnsureError and others should call the StartUp before Ensure as per the interface's use case logic. As of now, if I wanted to rely on the StateEngine's StartUp implementation as a client, I would not be able to do so because its Ensure is not aware of the flag.

I planned to push my patches in a series of PRs after the sprint shortly. If this one goes in it should not complicate the state package merge significantly. I would still be able to push the major state management changes. However, if it is not urgent and can wait a day or two, I would start pushing the series of my PRs that would also include the interface implementation mirrored from snapd which we can work together on and merge along the state package changes.

benhoyt commented 9 months ago

However, if it is not urgent and can wait a day or two, I would start pushing the series of my PRs that would also include the interface implementation mirrored from snapd which we can work together on and merge along the state package changes.

Thanks @dmitry-lyfar. Yes, let's do that then.

anpep commented 9 months ago

Having said that, @anpep I suppose you have more logic to be pulled in as the daemon module has not been changed here (which changes quite a bit of daemon unit tests)?

Hi @dmitry-lyfar! Thanks for your changes and sorry that we didn't communicate about these before. The reason I created this draft PR is partly because we intend Pebble and snapd code to converge in some areas like state management, but the real reason was that we were considering the StateStarterUp interface for solving a particular design we had in mind, but that we have ultimately ditched.

That said, I think your changes are more comprehensive and in line with the goal of merging snapd state changes into Pebble, so if you don't mind I will be closing this PR so that we can focus on that effort in your series of patches (:

Thanks for the great work!