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

o/snapstate: extend support for holding refreshes #12035

Merged
merged 10 commits into from Aug 24, 2022

Conversation

MiguelPires
Copy link
Contributor

Adds or extends refresh holding support for all-snaps and specific snaps in snapstate:

  • e226d30 adds support for holding refreshes indefinitely for all the system's snaps
  • 4d28afb adds support for holding refreshes of sets of snaps indefinitely
  • 8de767e makes general refreshes consistent with auto-refreshes by respecting specific holds
  • 0b28d1f changes spread tests that rely on auto-refreshes and depended on holds being limited.

Extends the 'refresh.hold' "all-snaps" hold mechanism to allow
indefinite holds.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Adds support for the system administrator to hold the refresh of a set
of snaps for any amount of time.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
General refreshes will respect specific holds, in order to be more
consistent with auto-refreshes. This doesn't apply to specific refreshes
(snap refresh foo bar) or to "all-snaps" holds (snap refresh --hold).
Allow users to hold specific snaps but still do system refreshes.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Tests relying on auto-refreshing triggered them by setting the
'last-refresh' to distant time which would then exceed the maximum
postponement limit and trigger an auto-refresh despite any holds. Since
refresh can now be held indefinitely and the system starts up with a 2h
hold, we need to remove this hold in order for the auto-refresh to trigger.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, but I wonder if it could be made even easier to read for those who are not deep into the code. :-)

overlord/snapstate/autorefresh.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Show resolved Hide resolved
overlord/snapstate/snapstate.go Outdated Show resolved Hide resolved
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
@MiguelPires MiguelPires requested a review from mardy August 16, 2022 11:54
@MiguelPires
Copy link
Contributor Author

@mardy thanks for the feedback. I tried to answer your points, let me know if there's something expand on. I also experimented with writing an "implementation notes" document for this, which I can share if you want. It's short and tries not to overlap with the specification, but it's meant to document implementation decisions and roadblocks that might be relevant for someone trying to come up to speed on it

Copy link
Contributor

@mardy mardy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Fix merge conflict in overlord/snapstate/autorefresh.go
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Copy link
Collaborator

@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, did a pass, some comments

overlord/configstate/configcore/refresh.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating_test.go Show resolved Hide resolved
overlord/snapstate/autorefresh_gating_test.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh_gating_test.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh_gating_test.go Outdated Show resolved Hide resolved
overlord/snapstate/autorefresh_gating.go Outdated Show resolved Hide resolved
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
@pedronis pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Aug 19, 2022
Copy link
Collaborator

@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, small remark. I will +1 this on Monday after I have done a clarification pass on the spec

overlord/snapstate/autorefresh_gating_test.go Show resolved Hide resolved
@pedronis pedronis self-requested a review August 19, 2022 12:36
Signed-off-by: Miguel Pires <miguel.pires@canonical.com>
Copy link
Collaborator

@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

@MiguelPires MiguelPires merged commit 9bb978e into snapcore:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Samuele review Needs a review from Samuele before it can land
Projects
None yet
3 participants