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

fix(stargz-snapshotter): set default root path #452

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

daper
Copy link
Contributor

@daper daper commented Aug 15, 2024

Description

This PR adds the default root path parameter to stargz-snapshotter extension as an argument: --root. It sets the path to /var/lib/containerd/io.containerd.snapshotter.v1.stargz as it is a more common place to find it. The snapshotter default path is defined in stargz-snapshotter/cmd/containerd-stargz-grpc/main.go#L66 and points to /var/lib/containerd-stargz-grpc.

Motivation

When trying the stargz-snapshotter extension I found some errors and seemed that the snapshotter wasn't working properly. I didn't copy the errors but they were something in the lines of:

  • missing directory /var/lib/containerd/io.containerd.snapshotter.v1.stargz.
  • stargz-snapshotter plugin is not exporting its root directory.

Investigation

Searching around I found some issues:

The last of the issues listed above pointed me to the right track. There is a linked PR (containerd/containerd#10127) where it is added to containerd the functionality to inform the remote snapshotter's root dir. Although it is advised to point it (if possible) to /var/lib/containerd/ for better compatibility.

containerd/nydus-snapshotter#288 (comment)

But we'd better still set snapshotter's default root path to /var/lib/containerd/io.containerd.snapshotter.v1.nydus for the better compatibility.

Testing

I've built the extension with the change, then built an installer image and upgraded a testing cluster with it. After it all the logs seemed normal. Also these logs from the CRI stream, leads me to think that cleanup of stale snapshots is working as expected:

{"level":"debug","msg":"schedule snapshotter cleanup","snapshotter":"stargz","time":"2024-08-15T15:17:49.143802230Z"}
{"key":"k8s.io/138/96b15cc0376d1abec916fb9fa9af3e6e83c7d0862ab82abd026fd309cee54723","level":"debug","msg":"removed snapshot","snapshotter":"stargz","time":"2024-08-15T15:17:49.147120976Z"}

@daper daper force-pushed the main branch 2 times, most recently from edaba55 to ce4d48a Compare August 15, 2024 15:57
@rsmitty
Copy link
Member

rsmitty commented Aug 15, 2024

Oh shoot, this is awesome. I poked at this same bit quite a while back without success. Glad to see you cracked it!

We'll sign this and get it committed.

This PR adds the default root path parameter to stargz-snapshotter
extension as an argument: `--root.` It sets the path to
`/var/lib/containerd/io.containerd.snapshotter.v1.stargz` as it is a more
common place to find it. The snapshotter default path is defined in
stargz-snapshotter/cmd/containerd-stargz-grpc/main.go#L66 and points to
/var/lib/containerd-stargz-grpc.

Signed-off-by: David Peralta <[email protected]>
Signed-off-by: Andrey Smirnov <[email protected]>
@smira
Copy link
Member

smira commented Aug 16, 2024

/m

@talos-bot talos-bot merged commit bb94c9d into siderolabs:main Aug 16, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants