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

Don't allow the client to keep using WIP snapshot on Prepare() failure #188

Merged
merged 1 commit into from
Dec 4, 2020

Conversation

ktock
Copy link
Member

@ktock ktock commented Dec 4, 2020

#187

Currently, when Prepare() fails to prepare a remote snapshot, it falls back to
the normal overlayfs mode even after the remote snapshot is mounted on this
snapshot directory by FileSystem.Mount().

Here, we shouldn't keep using this already-mounted snapshot (calls WIP
snapshot
here) and we are recently seeing errors related to this. When the
client tries to Prepare the same ChainID's remote snapshot from different
namespaces, the second Prepare() fails on commit() with AlreadyExists
error. Currenlty, this returns the WIP snapshot to the client. When the client
tries to unpack the layer on this WIP snapshot (following the normal snapshot
preparation procedure), it fails because the mounted (read-only) snapshot
doesn't allow to write there.

Note that this bug doesn't occur as long as the client uses single namespace
because the AlreadyExists error is detected in the metadata snapshotter and no
invocation occurs to stargz snpashotter's Prepare() API.

# ctr-remote --namespace=a i rpull ghcr.io/stargz-containers/python:3.7-esgz
# ctr-remote --namespace=b i rpull ghcr.io/stargz-containers/python:3.7-esgz
...
ctr: failed to extract layer sha256:ad3446a8cecccf69691891ba018291d42f530e40a58f811444935a14b49943ed: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount924450568: lchown /var/lib/containerd/tmpmounts/containerd-mount924450568/etc: operation not supported: unknown

This commit fixes this issue by prohibiting the client to use this WIP snapshot
by immediately returning an error on the failure occurring after
FileSystem.Mount() call.

Note that the failure on FileSystem.Mount() call itself still falls back to
the overlayfs snapshotter behaviour, which is needed for supporting non-remote
snapshots. This commit also adds a comment for making sure this fallback is
safe.

Signed-off-by: Kohei Tokunaga ktokunaga.mail@gmail.com

Currently, when `Prepare()` fails to prepare a remote snapshot, it falls back to
the normal overlayfs mode even after the remote snapshot is mounted on this
snapshot directory by `FileSystem.Mount()`.

Here, we shouldn't keep using this already-mounted snapshot (calls *WIP
snapshot* here) and we are recently seeing errors related to this. When the
client tries to `Prepare` the same `ChainID`'s remote snapshot from different
namespaces, the second `Prepare()` fails on `commit()` with `AlreadyExists`
error. Currenlty, this returns the WIP snapshot to the client. When the client
tries to unpack the layer on this WIP snapshot (following the normal snapshot
preparation procedure), it fails because the mounted (read-only) snapshot
doesn't allow to write there.

Note that this bug doesn't occur as long as the client uses single namespace
because the `AlreadyExists` error is detected in the metadata snapshotter and no
invocation occurs to stargz snpashotter's `Prepare()` API.

```
ctr-remote --namespace=a i rpull ghcr.io/stargz-containers/python:3.7-esgz
ctr-remote --namespace=b i rpull ghcr.io/stargz-containers/python:3.7-esgz
...
ctr: failed to extract layer sha256:ad3446a8cecccf69691891ba018291d42f530e40a58f811444935a14b49943ed: mount callback failed on /var/lib/containerd/tmpmounts/containerd-mount924450568: lchown /var/lib/containerd/tmpmounts/containerd-mount924450568/etc: operation not supported: unknown
```

This commit fixes this issue by prohibiting the client to use this WIP snapshot
by immediately returning an error on the failure occurring after
`FileSystem.Mount()` call.

Note that the failure on `FileSystem.Mount()` call itself still falls back to
the overlayfs snapshotter behaviour, which is needed for supporting non-remote
snapshots. This commit also adds a comment for making sure this fallback is
safe.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@AkihiroSuda AkihiroSuda merged commit 3f3c69a into containerd:master Dec 4, 2020
@ktock ktock deleted the namespaces branch October 7, 2022 13:45
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.

None yet

2 participants