Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: discard preserved namespace after removing snap #1847
Conversation
zyga
added some commits
Sep 5, 2016
niemeyer
added
the
Blocked
label
Sep 5, 2016
|
Let's please only discuss this once RTM is golden. |
zyga
referenced this pull request
in snapcore/snap-confine
Sep 12, 2016
Merged
Add snap-discard-ns #135
mvo5
reviewed
Sep 14, 2016
| @@ -91,6 +93,11 @@ func StripRootDir(dir string) string { | ||
| return "/" + result | ||
| } | ||
| +// InternalCmdPath returns the full path to a given internal command (e.g. to snap-confine) | ||
| +func InternalCmdPath(cmd string) string { |
mvo5
Sep 14, 2016
Collaborator
I don't think we need this, we don't have this for any other for the dirs.
mvo5
reviewed
Sep 14, 2016
| + // command is not meant to always succeed because the namespace may not | ||
| + // have been created yet in practice (e.g. the snap was removed before it | ||
| + // was started since last reboot). | ||
| + cmd := exec.Command(dirs.InternalCmdPath("snap-discard-ns"), snapName) |
mvo5
Sep 14, 2016
Collaborator
filepath.Join(dirs.LibexecDir, "snap-discard-ns") seems to be ok here?
zyga
Sep 15, 2016
Contributor
Sure it is just more "magic" and less obvious what this means. I can change it if if you like though.
mvo5
reviewed
Sep 14, 2016
| + si := s.mockSnap(c, sampleSnapYaml) | ||
| + | ||
| + // Mock enough bits so that we can observe calls to snap-discard-ns | ||
| + defer testutil.MockInternalCmdPath("")() |
mvo5
Sep 14, 2016
Collaborator
I think this is slightly confusing, restorer := testutils.MockInternalCmdPath("")\ndefer restorer()\n would make it easier for me to read.
mvo5
Sep 14, 2016
Collaborator
I'm not sure we really need this MockInternalCmdPath (just yet), we could simply do "dirs.LibexecDir = cmd.BinDir` after the mockCommand was created.
mvo5
reviewed
Sep 14, 2016
| + | ||
| +// MockInternalCmdPath mocks the path used for lookups of internal commands, | ||
| +// such as snap-confine, that are not on PATH. | ||
| +func MockInternalCmdPath(path string) (restore func()) { |
mvo5
Sep 14, 2016
Collaborator
See above, not sure we need this, given that we generally do something like "dirs.SetRootDir(c.MkDir())" in the tests, so this is usually already pointing to a place that we can mock around plus it gets restored for free usually.
|
Code looks fine, I have some suggestion for simplification. I would love to hear what Gustavo has to say here before merging it, I'm not sure what he had in mind when he wrote the comment. Also technically we require a database migration and need to add this task to existing changes. However given that this is currently a non-op (and that the corresponding binary does not even exists yet) that seems to be not needed. |
zyga
added some commits
Sep 22, 2016
|
This failed only on |
zyga
added some commits
Sep 22, 2016
| + echo "Discarding preserved snap namespaces" | ||
| + # opportunistic as those might not be actually mounted | ||
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount "$mnt" || true |
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount "$mnt" || true | ||
| + done | ||
| + umount /run/snapd/ns/ || true |
zyga
Sep 22, 2016
•
Edited 1 time
-
zyga
Sep 22, 2016
Contributor
-
Sep 22, 2016
This is a bind mount "over itself" perhaps we could actually leave it alone entirely. I'll use --lazy "just in case"
| + echo "Discarding preserved snap namespaces" | ||
| + # opportunistic as those might not be actually mounted | ||
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount "$mnt" || true |
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount "$mnt" || true | ||
| + done | ||
| + umount /run/snapd/ns/ || true |
zyga
Sep 22, 2016
•
Edited 1 time
-
zyga
Sep 22, 2016
Contributor
-
Sep 22, 2016
This is a bind mount "over itself" perhaps we could actually leave it alone entirely. I'll use --lazy "just in case"
| @@ -303,3 +306,24 @@ func (m *InterfaceManager) doDisconnect(task *state.Task, _ *tomb.Tomb) error { | ||
| setConns(st, conns) | ||
| return nil | ||
| } | ||
| + | ||
| +func (m *InterfaceManager) doDiscardNamespace(task *state.Task, _ *tomb.Tomb) error { |
niemeyer
Sep 22, 2016
Contributor
Why is this in the interface manager? Isn't that a foundational property of every snap?
zyga
Sep 22, 2016
Contributor
To some extent yes but you can envision a sister task that lives right next to this one that goes into a namespace to reconfigure it based on interface changes. I think we can move it to the snapcore package but depending on how things unfold it might "go back" here.
niemeyer
Sep 22, 2016
Contributor
Interfaces can tweak it, but this sounds like a property of every snap. We have unlink-snap, unlink-current-snap, clear-snap, and discard-snap. Why do we need another task?
zyga
Sep 23, 2016
Contributor
You are right, it was just my knee-jerk reaction to put it in ifacestate and make it a task.
| + | ||
| + // Run "snap-discard-ns $SNAP_NAME" and ignore the error code. This | ||
| + // command is not meant to always succeed because the namespace may not | ||
| + // have been created yet in practice (e.g. the snap was removed before it |
niemeyer
Sep 22, 2016
•
Edited 1 time
-
niemeyer
Sep 22, 2016
Contributor
-
Sep 22, 2016
That sounds bad. If we want to ignore a particular condition, we should teach snap-discard-ns to ignore that one particular condition, probably behind a flag. Or even better: why do we run this command if the namespace is not there? Simply ignoring the fact it fails in whichever way will hide real bugs. The tool might not even be there, and this will still pretend it's all good.
Let's please get both the error code and the combined output, and actually fail the task if it doesn't work. We want to know.
zyga
Sep 22, 2016
Contributor
I think you are right. I've patched snap-discard-ns since writing this code to be resilient to this error. I'll make sure we actually check for errors in this task.
| @@ -62,6 +62,7 @@ func Manager(s *state.State, extra []interfaces.Interface) (*InterfaceManager, e | ||
| runner.AddHandler("setup-profiles", m.doSetupProfiles, m.doRemoveProfiles) | ||
| runner.AddHandler("remove-profiles", m.doRemoveProfiles, m.doSetupProfiles) | ||
| runner.AddHandler("discard-conns", m.doDiscardConns, m.undoDiscardConns) | ||
| + runner.AddHandler("discard-namespace", m.doDiscardNamespace, nil) |
niemeyer
Sep 22, 2016
•
Edited 1 time
-
niemeyer
Sep 22, 2016
Contributor
-
Sep 22, 2016
Reading these changes it's unclear to me whether this command is about mount namespaces specifically, or other namespaces too. What's the case here? The task and the tool need to be named after their actual intended functionality, and it must be realistic going forward.
niemeyer
Sep 22, 2016
Contributor
In fact, do we even need a new task for that? Is there any benefit of doing this in isolation rather than in an existing removal task?
niemeyer
Sep 22, 2016
Contributor
Should probably be done inside discard-snap. That's where we remove the snap files, and there's no undo there either ATM (might change with trashed data + cleanup), so if we want to undo a discard we need to fix both cases.
niemeyer
Sep 22, 2016
Contributor
Implementation note: should discard the namespace after the snap file and the respective mount units are removed, as this is the point of no return.
chipaca
reviewed
Sep 22, 2016
This LGTM, beyond agreeing with gustavo's concerns about it not needing to be a task. And a minor nit.
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount --lazy "$mnt" || true | ||
| + done | ||
| + umount --lazy /run/snapd/ns/ || true |
zyga
Sep 26, 2016
Contributor
Done, thanks, I missed this earlier. I changed all occurrences of umount --lazy to umount -l
mvo5
added
Critical
and removed
Blocked
labels
Sep 23, 2016
zyga
added some commits
Sep 26, 2016
|
Tests green! Please review me |
|
Looks nice, thank you! Two small comments. |
| + for mnt in /run/snapd/ns/*.mnt; do | ||
| + umount --lazy "$mnt" || true | ||
| + done | ||
| + umount --lazy /run/snapd/ns/ || true |
zyga
Sep 26, 2016
Contributor
Done, thanks, I missed this earlier. I changed all occurrences of umount --lazy to umount -l
| + cmd := exec.Command(snapDiscardNs, snapName) | ||
| + output, err := cmd.CombinedOutput() | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot discard preserved namespaces of snap %q: %s", snapName, output) |
zyga
added some commits
Sep 26, 2016
| + if osutil.FileExists(snapDiscardNs) { | ||
| + // Preserved namespaces need to be discarded only if they are being | ||
| + // preserved by snap-confine. If the snap-discard-ns command doesn't | ||
| + // exist then the shared mount namespace feature is not available and |
niemeyer
Sep 26, 2016
Contributor
This is not quite a safe assumption. Can't we test for the existence of the mount namespace file instead?
zyga commentedSep 5, 2016
This branch adds support for new internal command, snap-discard-namespace, that is provided by the snap-confine project. The purpose of the command is to discard preserved namespaces so that removed snap is not holding kernel resources associated with any preserved namespaces.