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.

Sign up for GitHub

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

Jump to bottom

Fix runtime platform loading in cri image plugin init #11165

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix runtime platform loading in cri image plugin init #11165

wants to merge 1 commit into from

Conversation

djdongjin
Copy link
Member

@djdongjin djdongjin commented Dec 16, 2024 •
edited
Loading

The cri image plugin init has a bug where, after getting FSPath for snapshotter_i, it always stores/overwrites it under options.ImageFSPaths[defaultSnapshotter] instead.

Also make a few other refactor:

  1. Dedup the snapshotRoot logic.
  2. Remove some unnecessary/nop logic in RuntimePlatforms for-loop.

Adding this to config.toml:

    [plugins.'io.containerd.cri.v1.images'.runtime_platforms.native-runtime]
      snapshotter = "native"

    [plugins.'io.containerd.cri.v1.images'.runtime_platforms.overlayfs-runtime]
      snapshotter = "overlayfs"

After this change, in containerd log:

# both overlayfs (default) and native snapshotters get correct FS path.
$ sudo journalctl -u containerd | grep "Get image filesystem path"
Dec 16 01:15:34 ip-172-31-29-62 containerd[1595006]: time="2024-12-16T01:15:34.566632057Z" level=info msg="Get image filesystem path \"/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs\" for snapshotter \"overlayfs\""
Dec 16 01:15:34 ip-172-31-29-62 containerd[1595006]: time="2024-12-16T01:15:34.566649570Z" level=info msg="Get image filesystem path \"/var/lib/containerd/io.containerd.snapshotter.v1.native\" for snapshotter \"native\""

# crictl also shows t he correct mountpoint from overlayfs.
$ sudo crictl imagefsinfo
{
  "status": {
    "imageFilesystems": [
      {
        "timestamp": "1734311757669464945",
        "fsId": {
          "mountpoint": "/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs"
        },
        "usedBytes": {
          "value": "0"
        },
        "inodesUsed": {
          "value": "0"
        }
      }
    ],
    "containerFilesystems": []
  }
}

In main branch:

# since we put native's FS path under overlayfs, so the 2nd log prints empty path for native.
$ sudo journalctl -u containerd | grep "Get image filesystem path"
Dec 16 01:11:54 ip-172-31-29-62 containerd[1593310]: time="2024-12-16T01:11:54.035841937Z" level=info msg="Get image filesystem path \"/var/lib/containerd/io.containerd.snapshotter.v1.overlayfs\" for snapshotter \"overlayfs\""
Dec 16 01:11:54 ip-172-31-29-62 containerd[1593310]: time="2024-12-16T01:11:54.035854073Z" level=info msg="Get image filesystem path \"\" for snapshotter \"native\""

# also the mountpoint incorrectly shows `native`, because of the accidental overwrite.
$ sudo crictl imagefsinfo
{
  "status": {
    "imageFilesystems": [
      {
        "timestamp": "1734311595741050452",
        "fsId": {
          "mountpoint": "/var/lib/containerd/io.containerd.snapshotter.v1.native"
        },
        "usedBytes": {
          "value": "0"
        },
        "inodesUsed": {
          "value": "0"
        }
      }
    ],
    "containerFilesystems": []
  }
}

@dosubot dosubot bot added area/cri Container Runtime Interface (CRI) kind/bug labels Dec 16, 2024
@djdongjin djdongjin force-pushed the fix-cri-image-snapshotter-loading branch from 5a4e576 to 62689c4 Compare December 16, 2024 01:21
@djdongjin djdongjin changed the title Fix runtime platform loading in cri image config Fix runtime platform loading in cri image plugin init Dec 16, 2024
@djdongjin
Fix runtime platform loading in cri image plugin init
ef0e709 
The cri image service init has a bug where, after getting FSPath
for snapshotter_i, it stores it under defaultSnapshotter instead
of snapshotter_i.

Also make a few other refactor:

1. Dedup the snapshotRoot loading for defaultSnapshotter
2. Remove some unnecessary logic in RuntimePlatforms for-loop

Signed-off-by: Jin Dong <[email protected]>
@djdongjin djdongjin force-pushed the fix-cri-image-snapshotter-loading branch from 62689c4 to ef0e709 Compare December 16, 2024 01:28
djdongjin
djdongjin commented Dec 16, 2024
View reviewed changes
plugins/cri/images/plugin.go
Comment on lines -115 to -119
if s, ok := options.Snapshotters[defaultSnapshotter]; ok {
options.Snapshotters[defaultSnapshotter] = s
} else {
return nil, fmt.Errorf("failed to find snapshotter %q", defaultSnapshotter)
}
Copy link
Member Author

@djdongjin djdongjin Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if this is only needed for defaultSnapshotter (if so this is noop and we can remove since we've done this at line 94-98.

Or is this also a bug? (i.e., what we want is:

					if s, ok := allSnapshotters[snapshotter]; ok {
						options.Snapshotters[snapshotter] = s
					} else {
						return nil, fmt.Errorf("failed to find snapshotter %q", snapshotter)
					}

(The options.Snapshotters is only used by snapshotSyncer to sync snapshots to cri's snapshotStore. So with current logic, we're only doing this for the default snapshotter).

containerd/internal/cri/server/images/service.go

Lines 116 to 117 in 2207955

svc.snapshotStore,
options.Snapshotters,

@djdongjin
Copy link
Member Author

/test pull-containerd-k8s-e2e-ec2

@djdongjin
Copy link
Member Author

cc @dmcgowan

@djdongjin
Copy link
Member Author

kindly ping @dmcgowan could you please PTAL this fix when you get some time? thanks! 😄

@dmcgowan dmcgowan added the cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
area/cri Container Runtime Interface (CRI) cherry-pick/2.0.x Change to be cherry picked to release/2.0 branch kind/bug size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@djdongjin @dmcgowan @k8s-ci-robot