CRI: "Fix" imageFSPath behavior #9216

dcantah · 2023-10-11T10:14:45Z

This was spurred by seeing:

"Failed to get the info of the filesystem with mountpoint"
err="no such file or directory" mountpoint="/path/to/io.containerd.snapshotter.v1.native"

in the kubelets logs. This is due to the imageFSPath for the CRI plugin assuming all of the snapshotters are located under the default location that gets passed on plugin initialization. Several of the snapshotters offer the ability to set different root paths and override this however, and afaict for remote snapshotters it's entirely up to them so this location can be wrong in some cases.

This change exports the root path for the remaining built-in snapshotters that allow you to change their location, and then uses the introspection API to grab the path.

Now ImageFSInfo on my host returns:

containerd sudo crictl -t 20s imagefsinfo
{
  "status": {
    "timestamp": "1697024624876142000",
    "fsId": {
      "mountpoint": "/Users/dcantah/containerd/blockfile"
    },
    "usedBytes": {
      "value": "0"
    },
    "inodesUsed": {
      "value": "0"
    }
  }
}

k8s-ci-robot · 2023-10-11T10:14:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ktock · 2023-10-11T11:25:24Z

This might be related to #9214 (comment)

dcantah · 2023-10-11T11:28:01Z

Yep! Thanks for pointing me to that thread. We've been seeing this for quite a bit now apparently but finally looked into it, I think it's definitely more pronounced for remote sn's but all of the built in linux snapshotters allow you to point wherever you want also.

Edit: As Derek mentioned in that issue there's more to be done for the CRI call there, especially if we want to take into account the experimental multi snapshotter support, but this is a good start I think.

dmcgowan · 2023-10-11T20:55:16Z

I wonder if this type of static metadata is better suited for the introspection API rather than a dedicated endpoint. If the data is stateful or dynamic, then such a call probably makes more sense. We do need to figure out a better way for out of process plugins to set their plugin metadata though, it might just have to be done through the proxy plugin config for now.

dcantah · 2023-10-11T20:58:51Z

I wonder if this type of static metadata is better suited for the introspection API rather than a dedicated endpoint.

Not a bad idea from looking, I honestly have never used it 😆

dcantah · 2023-10-12T02:00:59Z

Swapping to an introspection approach

fuweid · 2023-10-12T02:58:09Z

I think the metadata in plugin is good enough to get the information.

containerd/snapshots/overlay/plugin/plugin.go

Line 93 in 4205030

ic.Meta.Exports["root"] = root

samuelkarp · 2023-10-12T07:31:34Z

pkg/cri/sbserver/service.go

 // Note that if containerd changes directory layout, we also needs to change this.
-func imageFSPath(rootDir, snapshotter string) string {
-	return filepath.Join(rootDir, plugin.SnapshotPlugin.String()+"."+snapshotter)
+func imageFSPath(rootDir, snapshotter string, client *containerd.Client) string {


Would you be able to write a unit test for this?

Possibly.. I'm not sure how I'd effectively use this in a unit test scenario though as it requires the daemon. Let me see if we have any CRI tests that write out a different config to use, I think that'd make the most sense

fuweid · 2023-10-13T23:33:36Z

Need a rebase here~

Rethinking about this and it should be backported into supported release.

dcantah · 2023-10-13T23:44:26Z

I still need to write a test for this, trying to find a spare hour 😅

dmcgowan · 2023-10-17T00:48:37Z

Approach looks good, you have a few changes to rebase on top of.

We should consider defining the export keys in plugins/types.go.

dcantah · 2023-10-17T01:13:09Z

@dmcgowan Will do

Some of the snapshotters that allow you to change their root location were already doing this, this just makes all of them follow the same pattern. Signed-off-by: Danny Canter <danny@dcantah.dev>

Currently it didn't take into account that certain snapshots can explicitly have their root directories placed at a different location. This changes it to use the RootPath method of the snapshotter if it implements it. Signed-off-by: Danny Canter <danny@dcantah.dev>

dcantah · 2023-10-17T19:57:38Z

Still trying to find time to write a decent test here, sorry for the delay :(.

dmcgowan · 2023-10-18T01:03:33Z

An indecent test seems fine as well.

I wonder if use of the introspection service there makes it trickier. The snapshots are already marked as dependencies, you could just populate the snapshotter->root map quickly with a GetByType call during init and pass that along.

fuweid · 2023-10-18T01:32:37Z

@dcantah sorry I just merged one related pull request. It needs you to rebase it. Thanks

k8s-ci-robot · 2023-11-29T05:39:19Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Kern-- · 2024-04-09T22:31:08Z

I think we’re seeing the same issue: awslabs/soci-snapshotter#1093

I don’t see anything here for out-of-process snapshotters that go through the proxy. Am I just missing something?

dcantah · 2024-04-10T13:23:14Z

@Kern-- Trying to page this back in, but as far as I remember Derek added support to be able to specify exports for proxy plugins, so I'd assume if your proxy plugin defined the "root" export and set it then this change would probably work.

I'll try and rebase this shortly, but I remember not being able to find a decent spot to add a test given it needs a containerd config change to effectively test. If anyone has ideas, or @Kern-- if you'd wanna work on it I can give you access to this branch after rebasing so we can try and land sooner

Kern-- · 2024-04-10T18:00:14Z

Looking at this a bit deeper, it looks like this was fixed as part of the CRI restructuring in #9152.

Maybe this PR should be rebased on top of release/1.7?

dcantah · 2024-04-10T18:03:17Z

Oh that's sweet, if so I'll look to add to 1.7 instead. I likely won't have time this week however. @dmcgowan given you did the cri restructuring, does this change look needed in main anymore?

Kern-- · 2024-04-11T18:51:41Z

I think the first commit to make sure all the snapshotters add their root path to the introspection service is still useful in main.

@dcantah I'm happy to open new PRs with the cherry-picks/rebases if you'd like. I just don't want to step on your toes :)

dcantah · 2024-04-11T18:54:47Z

If I don't get to it by Friday please feel free 😭. I'd rather this land now that it's affecting a service, I'd found this just digging through logs last year but it wasn't really impacting anything

dcantah · 2024-04-14T13:28:37Z

Closing this, and moving the first commit of this to #10073. @Kern-- Feel free to plop this in 1.7 if it's needed!

austinvazquez · 2025-05-26T17:30:18Z

Cleaning up the labels here and marking this as "cherry-picked". As documented above, this fix was applied for containerd 1.6/1.7 branches; however, the issue was inadvertently fixed in mainline at the time (containerd 2.0+) through CRI restructuring so was not merged here.

dcantah force-pushed the sn-rootpath branch from c38e97e to 921df52 Compare October 11, 2023 11:45

dcantah mentioned this pull request Oct 11, 2023

Generalize plugin library #9214

Merged

dcantah marked this pull request as ready for review October 11, 2023 11:48

dcantah force-pushed the sn-rootpath branch from 921df52 to 799bc8d Compare October 12, 2023 06:54

dcantah changed the title ~~Snapshots: Add RootPath method/rpc~~ CRI: "Fix" imageFSPath behavior Oct 12, 2023

dcantah force-pushed the sn-rootpath branch 2 times, most recently from 2882e98 to b9f7f44 Compare October 12, 2023 07:02

samuelkarp reviewed Oct 12, 2023

View reviewed changes

fuweid added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Oct 13, 2023

Snapshotters: Export the root path

181fa5c

Some of the snapshotters that allow you to change their root location were already doing this, this just makes all of them follow the same pattern. Signed-off-by: Danny Canter <danny@dcantah.dev>

dmcgowan mentioned this pull request Oct 17, 2023

Add exports to proxy plugin config #9253

Merged

dcantah force-pushed the sn-rootpath branch from b9f7f44 to 6aeec45 Compare October 17, 2023 08:56

k8s-ci-robot added the needs-rebase label Nov 29, 2023

dims added the area/cri Container Runtime Interface (CRI) label Feb 7, 2024

Kern-- mentioned this pull request Apr 10, 2024

[Bug] Container Ephemeral storage limit is not enforced when using SOCI Snapshotter. awslabs/soci-snapshotter#1093

Closed

dcantah closed this Apr 15, 2024

Kern-- mentioned this pull request Apr 19, 2024

[1.6/1.7] kubernetes ephemeral-storage limits not enforced with remote snapshotters #10095

Closed

ruiwen-zhao mentioned this pull request Apr 19, 2024

[release/1.7] Fix CRI snapshotter root path when not under containerd root #10096

Merged

Kern-- mentioned this pull request Apr 24, 2024

[release/1.6] Fix snapshotter root path when not under containerd root #10127

Merged

austinvazquez added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels May 26, 2025

CRI: "Fix" imageFSPath behavior #9216

CRI: "Fix" imageFSPath behavior #9216

Uh oh!

Conversation

dcantah commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 11, 2023

Uh oh!

ktock commented Oct 11, 2023

Uh oh!

dcantah commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan commented Oct 11, 2023

Uh oh!

dcantah commented Oct 11, 2023

Uh oh!

dcantah commented Oct 12, 2023

Uh oh!

fuweid commented Oct 12, 2023

Uh oh!

samuelkarp Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

dcantah Oct 12, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcantah commented Oct 13, 2023

Uh oh!

dmcgowan commented Oct 17, 2023

Uh oh!

dcantah commented Oct 17, 2023

Uh oh!

dcantah commented Oct 17, 2023

Uh oh!

dmcgowan commented Oct 18, 2023

Uh oh!

fuweid commented Oct 18, 2023

Uh oh!

k8s-ci-robot commented Nov 29, 2023

Uh oh!

Kern-- commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcantah commented Apr 10, 2024

Uh oh!

Kern-- commented Apr 10, 2024

Uh oh!

dcantah commented Apr 10, 2024

Uh oh!

Kern-- commented Apr 11, 2024

Uh oh!

dcantah commented Apr 11, 2024

Uh oh!

dcantah commented Apr 14, 2024

Uh oh!

austinvazquez commented May 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dcantah commented Oct 11, 2023 •

edited

Loading

dcantah commented Oct 11, 2023 •

edited

Loading

fuweid commented Oct 13, 2023 •

edited

Loading

Kern-- commented Apr 9, 2024 •

edited

Loading