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

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

Closed

Conversation

anpep
Copy link
Collaborator

@anpep anpep commented Nov 15, 2023

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
Copy link
Contributor

benhoyt commented Nov 15, 2023

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
Copy link
Contributor

@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
Copy link
Contributor

benhoyt commented Nov 16, 2023

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
Copy link
Collaborator Author

anpep commented Nov 17, 2023

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!

@anpep anpep closed this Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants