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

cmd/snap-update-ns: handle anomalies better #8961

Merged
merged 5 commits into from
Jul 2, 2020

Conversation

zyga
Copy link
Contributor

@zyga zyga commented Jul 1, 2020

When snap-update-ns decides to mount or unmount something, it makes the
decision based on the differences between mount profiles - fstab like
files that describe what is probably mounted and what should be mounted.
Actual mount table is much more complex, containing mounts performed by
snap-confine as well as the effect of complex recursive bind mounts and
unmounts.

As an analogy, we navigate a submarine not by looking out the window but
by looking at a map and our internal instruments. We may run into
problems if there's disagreement between the outside world and our
simplified representation.

This patch provides two improvements to situations when our internal
representation disagrees with reality: one for umount failing with
EINVAL and anther for rmdir/unlink which fails with ENOENT. In both
cases the idea is the same, we try to remove something which is not
there.

In the first case, when unmount fails with EINVAL, we can check if the
path we intended to unmount is really a mount point. If it is not then
the call has failed because our desired result is already present.

In the second case, when removing a file or directory fails with ENOENT
then the object is clearly already removed.

In both cases we can continue instead of failing.

Ref: https://bugs.launchpad.net/snapd/+bug/1884849
Ref: https://forum.snapcraft.io/t/18393/10
Signed-off-by: Zygmunt Krynicki me@zygoon.pl

When snap-update-ns decides to mount or unmount something, it makes the
decision based on the differences between mount profiles - fstab like
files that describe what is probably mounted and what should be mounted.
Actual mount table is much more complex, containing mounts performed by
snap-confine as well as the effect of complex recursive bind mounts and
unmounts.

As an analogy, we navigate a submarine not by looking out the window but
by looking at a map and our internal instruments. We may run into
problems if there's disagreement between the outside world and our
simplified representation.

This patch provides two improvements to situations when our internal
representation disagrees with reality: one for umount failing with
EINVAL and anther for rmdir/unlink which fails with ENOENT. In both
cases the idea is the same, we try to remove something which is not
there.

In the first case, when unmount fails with EINVAL, we can check if the
path we intended to unmount is really a mount point. If it is not then
the call has failed because our desired result is already present.

In the second case, when removing a file or directory fails with ENOENT
then the object is clearly already removed.

In both cases we can continue instead of failing.

Ref: https://bugs.launchpad.net/snapd/+bug/1884849
Ref: https://forum.snapcraft.io/t/18393/10
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga zyga added the Bug label Jul 1, 2020
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Thanks Zyga. Assuming that the PR text correctly describes the problem, the fix implementation looks sane and should address the issue.

@cmatsuoka
Copy link
Contributor

By the way, thanks for the detailed PR message, it makes it much easier to understand the issue and match the actual implementation with what was intended by the author even if the reviewer is not completely familiar with the subsystem being worked on.

Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

Little tweaks and a question about the spread test

cmd/snap-update-ns/change.go Outdated Show resolved Hide resolved
tests/regression/lp-1884849/task.yaml Outdated Show resolved Hide resolved
cmd/snap-update-ns/change.go Show resolved Hide resolved
zyga added 3 commits July 2, 2020 14:05
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
The regression test does not need to depend on the real gimp snap,
instead we can get the exact same property using test-snapd-desktop.

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

Thanks

Signed-off-by: Zygmunt Krynicki <me@zygoon.pl>
@zyga zyga merged commit 2c7d281 into canonical:master Jul 2, 2020
@zyga zyga deleted the fix/lp-1884849 branch July 2, 2020 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants