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

fix(overlord): allow ensure when state is available #482

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

flotter
Copy link
Contributor

@flotter flotter commented Aug 12, 2024

Managers can explicitly call state.EnsureBefore() if they wish to apply changes sooner than the scheduled 5min ensure overlord timer.

Although managers get a state instance at creation time, during overlord.New(), it is currently forbidden to call ensure early on before the Daemon started the overlord.Loop(). The current restriction stems from the fact that state.EnsureBefore() adjusts the ensure timer expiry time, and the timer is only created when overlord.Loop() is called.

The problem:

Since the overlord ensure loop is only started later on during daemon.Start(), a manager has no direct way to know when it's safe to call Ensure. Managers currently have to use the first call to their Ensure() method as an indication the overlord loop was entered, which is unnecessary manager boilerplate code. See overlord/checkstate/manager.go.

Managers (and managers with dependencies on other managers), may want to implement state changes during callbacks. These events can come from outside the overlord framework (e.g. from the Linux kernel or early on during manager StartUp), meaning that handlers using state may be triggered asynchronously from the Daemon startup sequence, and arrive before the overlord Loop is started.

Solution:

Allow state.EnsureBefore() to be called before the timer exists. Since an implicit ensure is performed on overlord loop entry, simply returning while the timer does not exist is the same as saying an ensure is already scheduled.

@flotter
Copy link
Contributor Author

flotter commented Aug 12, 2024

Following feedback from @pedronis and @niemeyer, this is my second attempt at making state.EnsureBefore() usage in interdependent managers simpler.

Samuele will review later this week, following Ubuntu Core release.

internals/overlord/overlord.go Outdated Show resolved Hide resolved
internals/overlord/overlord.go Outdated Show resolved Hide resolved
internals/overlord/overlord.go Outdated Show resolved Hide resolved
internals/overlord/overlord.go Outdated Show resolved Hide resolved
Copy link

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

my input

internals/overlord/overlord.go Outdated Show resolved Hide resolved
Copy link

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you, especially for the added comments

@flotter
Copy link
Contributor Author

flotter commented Aug 16, 2024

@benhoyt Ready for you to review. I think the proposal from @pedronis makes this much less risky. I've added some comments to explain where some of the code comes from historically, and I believe this is still Go 1.23 timer compatible.

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.

Thanks!

@flotter
Copy link
Contributor Author

flotter commented Aug 19, 2024

@dmitry-lyfar Just a quick check that you are fine with this, and knows about this tweak ?

@dmitry-lyfar
Copy link
Contributor

@flotter thanks, having comments there is great. I see no issues with this.

@flotter flotter merged commit b0d5e05 into canonical:master Aug 19, 2024
17 checks passed
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.

4 participants