o/snapstate: make a managed refresh schedule not require any additional checks #14107
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
github-actions
bot
added
the
Needs Documentation -auto-
MiguelPires
previously approved these changes
Jun 20, 2024
pedronis
reviewed
Jun 20, 2024
| @@ -109,6 +109,8 @@ func validateRefreshSchedule(tr RunTransaction) error { | |||
| st.Lock() | |||
| defer st.Unlock() | |||
|
|
|||
| // ensure there is a snap entitled to managing the refreshes | |||
| // in an autonomic manner | |||
There was a problem hiding this comment.
the last part of the comment inside reportOrIgnoreInvalidManageRefreshes needs also changes I think
| s.state.Lock() | ||
| defer s.state.Unlock() | ||
|
|
||
| // enable gate-auto-refresh-hook feature |
pedronis
reviewed
Jun 20, 2024
There was a problem hiding this comment.
@bboozzoo @Meulengracht the test looks useful but doesn't seem that it would test fix itself?
bboozzoo
force-pushed
the
bboozzoo/refresh-schedule-managed-stays-on
branch
from
7c24ca1
to
fc54f59
Compare
|
@pedronis have a look at the test changes I pushed. I tried to make it more realistic and match more closely the scenario we're addressing (unfortunately it's at the cost of the test becoming less easier to follow). |
MiguelPires
approved these changes
Jun 21, 2024
overlord/managers_test.go
Outdated
Comment on lines
14428
to
14440
| // This test exists to verify that any distruptions of a refresh | ||
| // will maintain any pre-existing connection that was held before | ||
| // the snap is marked inactive. The goal of this test is to run all | ||
| // tasks that involve something making the snap unavailable/not active | ||
| // and then simulating a reboot inside the interface manager. We then | ||
| // re-verify the connection and see that functionality used return | ||
| // the expected values. The interface used for testing is snapd-control | ||
| // with the managed refresh schedule attribute. | ||
| // | ||
| // This was selected based on a customer case. Prior to version 2.58 this | ||
| // would cause the connection to be dropped, if a well-timed restart of snapd | ||
| // would occur during a refresh. In 2.58 a fix for this was merged, and this | ||
| // test shall be passing. |
There was a problem hiding this comment.
Suggested change
| // This test exists to verify that any distruptions of a refresh | |
| // will maintain any pre-existing connection that was held before | |
| // the snap is marked inactive. The goal of this test is to run all | |
| // tasks that involve something making the snap unavailable/not active | |
| // and then simulating a reboot inside the interface manager. We then | |
| // re-verify the connection and see that functionality used return | |
| // the expected values. The interface used for testing is snapd-control | |
| // with the managed refresh schedule attribute. | |
| // | |
| // This was selected based on a customer case. Prior to version 2.58 this | |
| // would cause the connection to be dropped, if a well-timed restart of snapd | |
| // would occur during a refresh. In 2.58 a fix for this was merged, and this | |
| // test shall be passing. | |
| // This test exists to verify that any disruptions of a refresh | |
| // will maintain any pre-existing connection that was held before | |
| // the snap is marked inactive. The goal of this test is to run all | |
| // tasks that involve something making the snap unavailable/not active | |
| // and then simulating a reboot inside the interface manager. We then | |
| // re-verify the connection and see that functionality used return | |
| // the expected values. The interface used for testing is snapd-control | |
| // with the managed refresh schedule attribute. | |
| // | |
| // This was selected based on a customer case. Prior to version 2.58 this | |
| // would cause the connection to be dropped, if a well-timed restart of snapd | |
| // would occur during a refresh. In 2.58 a fix for this was merged, and this | |
| // test should be passing. |
pedronis
reviewed
Jun 21, 2024
overlord/managers_test.go
Outdated
| // reflected by the repo | ||
| c.Assert(err, ErrorMatches, `snap "snap-with-snapd-control" has no .* "snapd-control"`) | ||
| c.Assert(ref, HasLen, 0) | ||
| // but returns false without the fix, yet we still made no |
There was a problem hiding this comment.
is the yet and no right here? above we have c.Assert(len(reqs) > 0, Equals, true) ?
There was a problem hiding this comment.
sorry, yes, that code was slightly later (after the checks that auto refresh wasn't done), but then I moved it around without updating the comment
Meulengracht
approved these changes
Jun 24, 2024
pedronis
approved these changes
Jun 24, 2024
…al checks Drop the additional check to CanManageRefreshes() when the refresh schedule is already set to 'managed'. This was originally a way to ensure that there is at least one snap entitled to directly manage the refreshes or fall back to the default auto-refresh schedule. However, the conditions in which the fallback would be applied are incorrect and could lead to a situation when snapd would trigger an auto-refresh even while a snap which is entitled to using a managed refresh schedule is being refreshed (due to the snapd-control being temporarily disconnected). On top of this, since the device was once switched to managed, it clearly means that it was entitled to do so and it was intentional, hence we should not accidentally break the expectations. Signed-off-by: Maciej Borzecki <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
…t to managed Signed-off-by: Maciej Borzecki <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
Signed-off-by: Maciej Borzecki <[email protected]>
bboozzoo
force-pushed
the
bboozzoo/refresh-schedule-managed-stays-on
branch
from
04067c6
to
eb43042
Compare
Signed-off-by: Maciej Borzecki <[email protected]>
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.
Drop the additional check to CanManageRefreshes() when the refresh schedule is
already set to 'managed'. This was originally a way to ensure that there is at
least one snap entitled to directly manage the refreshes or fall back to the
default auto-refresh schedule. However, the conditions in which the fallback
would be applied are incorrect and could lead to a situation when snapd would
trigger an auto-refresh even while a snap which is entitled to using a managed
refresh schedule is being refreshed (due to the snapd-control being temporarily
disconnected). On top of this, since the device was once switched to managed, it
clearly means that it was entitled to do so and it was intentional, hence we
should not accidentally break the expectations.
An attempt to fix this was introduced in 01ef5a9 which landed in 2.58, but when refreshing snapd from a version prior to 2.58, the pending security bits would not be set in the state and thus the new code would not be triggered as expected.