overlord/snapstate: discard mount namespace when undoing 1st link snap #6509
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 snap is installed for the first time but the installation fails, e.g. because the install hook fails or because a service fails to start, the mount namespace would remain behind, possibly messing up subsequent attempts to install the snap. When the undo handler for "link-snap" detects that it runs for the 1st revision is already removes snap cookies, this is the perfect place to add the logic to discard the mount namespace. Thanks to Samuele for pointing this out! Fixes: https://bugs.launchpad.net/snapd/+bug/1815722 Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
stolowski
reviewed
Feb 14, 2019
| err = m.backend.DiscardSnapNamespace(snapsup.InstanceName()) | ||
| if err != nil { | ||
| t.Errorf("cannot discard snap namespace %q, will retry in 3 mins: %s", snapsup.InstanceName(), err) | ||
| return &state.Retry{After: 3 * time.Minute} |
There was a problem hiding this comment.
3 minutes is a lot, is there a reason to wait that long?
Also two questions:
- shouldn't we bail out if it keeps failing? Unlike Retry in other places where we know a problem (e.g. a conflict) will eventually go away, in this case it might be a persistent issue (until rebooted)?
- what happens if there is a reboot before we execute undo and the namespace is naturally gone? Will we hit an error here?
There was a problem hiding this comment.
We also use 3 minutes in the natural case in discard-snap. I don't recall why we wait this long but I didn't want to invent a new time now.
- This doesn't fail in any way I know of
- If there's a reboot this gets cleaned up and snap-discard-ns just happily goes ahead
Codecov Report
@@ Coverage Diff @@
## master #6509 +/- ##
==========================================
- Coverage 79.16% 79.15% -0.02%
==========================================
Files 575 575
Lines 44518 44522 +4
==========================================
- Hits 35244 35241 -3
- Misses 6424 6428 +4
- Partials 2850 2853 +3
Continue to review full report at Codecov.
|
mvo5
approved these changes
Feb 14, 2019
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 snap is installed for the first time but the installation fails,
e.g. because the install hook fails or because a service fails to start,
the mount namespace would remain behind, possibly messing up subsequent
attempts to install the snap.
When the undo handler for "link-snap" detects that it runs for the 1st
revision is already removes snap cookies, this is the perfect place to
add the logic to discard the mount namespace.
Thanks to Samuele for pointing this out!
Fixes: https://bugs.launchpad.net/snapd/+bug/1815722
Signed-off-by: Zygmunt Krynicki me@zygoon.pl