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.

Sign up for GitHub

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

Jump to bottom

feat: add StateStarterUp to move expensive init ops to dedicated methods #330

Merged
merged 2 commits into from
Nov 22, 2023
Merged

feat: add StateStarterUp to move expensive init ops to dedicated methods #330

merged 2 commits into from
Nov 22, 2023

Conversation

dmitry-lyfar
Copy link
Contributor

This PR introduces the StateStarterUp interface alongside its implementations for Overlord and StateEngine.

These changes were ported and adopted for pebble from the original snapd PR: canonical/snapd#7132

See also: #327

@dmitry-lyfar
Add StateStarterUp to move expensive init ops to dedicated methods
4abf699 
Introduce the StateStarterUp interface alongside its implementations for
Overlord and StateEngine.

These changes were cherry-picked and adopted for pebble from the
original snapd PR: canonical/snapd#7132
@dmitry-lyfar
Copy link
Contributor Author

@benhoyt @anpep based on our previous discussion in #327, I started the state package updates merge from the StateStarterUp interface as it seems is required by others already.

The other changes as per the plan discussed here will follow in separate PRs.

@dmitry-lyfar
Check if the daemon.Start has returned any errors
3d044d1 
Previously, the daemon start logic did not return anything; With the
StateStarterUp introduced one needs to verify there were no issues at
Start to continue the daemon initialisation
benhoyt
benhoyt approved these changes Nov 19, 2023
View reviewed changes
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've compared to the snapd PR in question, and this looks good to me. Thanks! @anpep can you also please review just for a second eye, and then we'll merge?

@benhoyt benhoyt changed the title Add StateStarterUp to move expensive init ops to dedicated methods feat: add StateStarterUp to move expensive init ops to dedicated methods Nov 19, 2023
@anpep anpep self-requested a review November 20, 2023 07:41
anpep
anpep approved these changes Nov 20, 2023
View reviewed changes
Copy link
Collaborator

@anpep anpep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you! There were a lot of overlooks in my PR, but this one surely gets things right (:

internals/overlord/stateengine.go Show resolved Hide resolved
@benhoyt benhoyt merged commit 4468986 into canonical:master Nov 22, 2023
11 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anpep anpep anpep approved these changes

@benhoyt benhoyt benhoyt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dmitry-lyfar @benhoyt @anpep