overlord/snapstate: track time of postponed refreshes #6621
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a refresh is attempted but for skipped because a particular snap is busy then a time-stamp is recorded under snap state (refresh postponed time). This time-stamp can be used to determine when a refresh is mandatory, even if a snap is busy. This can prevent snaps from running old revisions indefinitely simply because an app is open. The patch adds the time-stamp to SnapState and ensures it is reset on refresh. Somewhat arbitrarily the instant that this is done was picked to be the "link-snap" task. The timer is set and reset regardless of the refresh-app-awareness feature flag because otherwise the state would go out-of-sync with reality and this aspect of the change is not complex. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Apparently comparing time using c.Check(t1, Equals, t2) (also DeepEquals) works fine as long as TZ is set. When TZ is unset, like when testing in Travis, the returned value is somehow different (even though both t1 and t2 are supposedly the same time, just marshalled back and forth. Using Time.Equals avoids that at the cost of rather ugly c.Check(t1.Equals(t2), Equals, true) Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
mvo5
approved these changes
Mar 26, 2019
zyga
commented
Mar 26, 2019
pedronis
reviewed
Mar 26, 2019
When we don't refresh a snap while it is running we postpone or inhibit the refresh for a later date. Internally we store the time-stamp of when that first happened. To avoid confusion using the word inhibited implies time-of-occurrence better than postpone which may imply time-of-next-attempt. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
chipaca
approved these changes
Mar 26, 2019
chipaca
suggested changes
Mar 26, 2019
There's on semantic difference but John has pointed out that storing the zero value of refresh inhibited time will leave plenty of serialized time.Time cruft in the state. By switching to a pointer we can just store nil when the time is not necessary. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
chipaca
approved these changes
Mar 27, 2019
mvo5
approved these changes
Mar 28, 2019
|
I will unblock and use this after #6684 lands in a follow-up that starts recording and acting on the time in the soft check. |
|
I have a use case for this now, merging :) EDIT: I see the remark about the PR that is using this being approved as well, oh well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a refresh is attempted but for skipped because a particular snap is
busy then a time-stamp is recorded under snap state (refresh postponed
time). This time-stamp can be used to determine when a refresh is
mandatory, even if a snap is busy. This can prevent snaps from running
old revisions indefinitely simply because an app is open.
The patch adds the time-stamp to SnapState and ensures it is reset on
refresh. Somewhat arbitrarily the instant that this is done was picked
to be the "link-snap" task.
The timer is set and reset regardless of the refresh-app-awareness
feature flag because otherwise the state would go out-of-sync with
reality and this aspect of the change is not complex.
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com