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

[RFC] inspect: show mounts info from CRI/ctr containers #2939

Merged
merged 2 commits into from May 14, 2024

Conversation

zjumoon01
Copy link

@zjumoon01 zjumoon01 commented Apr 15, 2024

Fix #2934

Containers created by CRI or ctr with bind mounts
don't have label nerdct/mounts.

However, 'inspect' with dockercompat mode only parses mounts from label "nerdct/mounts".

After this patch, 'inspect' with dockercompat mode can show CRI and ctr container bind mounts.

ResolvConfPath, HostnamePath and Hostname are taken to the same consideration.

Further comments:

  1. only show bind mount from CRI/ ctr containers. Because native container mounts contain all types of mount, such as tmpfs, proc, sysfs. That are not expected as dockercompat
  2. nerdctl/mounts in label has higher priority. So do nerdctl/hostname and nerdctl/state-dir. CRI/ctr mounts and label "nerdct/mounts" are not merged. It shouldn't happen that container created by nerdctl with bind mount doesn't inject mount info to label nerdctl/mounts

How to test:

ctr:

ctr run -d --mount type=bind,src=/home/ttt/test,dst=/disk002,options=rbind:rw docker.io/library/ubuntu:22.04 test-c1 sleep 200

Before this patch

#nerdctl -n default inspect test-c1 | grep -i mounts
"Mounts": null,

After this patch

#nerdctl -n default inspect test-c1 | grep -i mounts -A 10
"Mounts": [
{
"Type": "bind",
"Source": "/home/ttt/test",
"Destination": "/disk002",
"Mode": "",
"RW": true,
"Propagation": ""
}
],
"Config": {

#nerdctl -n default inspect test-c1 -f {{.Mounts}}
[{bind /home/ttt/test /disk002 true }]

We can also test ResolvConf and hostname by crictl pod

@zjumoon01
Copy link
Author

test crictl container

pod.json
{ "metadata": { "name": "testpod", "namespace": "default" }, "hostname": "myhostname", "log_directory": "/var/log/podlog" }

container.json
{ "metadata": { "name": "test-container" }, "image":{ "image": "docker.io/library/ubuntu:22.04" }, "command": [ "/bin/sh" ], "args": [ "-c", "sleep infinity" ], "log_path":"ubuntu.0.log", "linux": { }, "mounts": [ { "container_path": "/tmp", "host_path": "/tmp" } ] }

podid=`crictl runp pod.json`
container=`crictl create $podid container.json pod.json`
crictl start $container
nerdctl inspect -f {{.Config.Hostname}} $container
myhostname
nerdctl inspect -f {{.HostnamePath}} $container
/home/containerd/io.containerd.grpc.v1.cri/sandboxes/cdeae27e72b9bd8de07b8f7f938d2606767a88342c6531c7913c8422bbb17065/hostname
nerdctl inspect -f {{.ResolvConfPath}} $container
/home/containerd/io.containerd.grpc.v1.cri/sandboxes/cdeae27e72b9bd8de07b8f7f938d2606767a88342c6531c7913c8422bbb17065/resolv.conf

@AkihiroSuda
Copy link
Member

Can we have a test?

@zjumoon01
Copy link
Author

Can we have a test?

OK.
I have added a test and split into two commits.

Signed-off-by: baijia <baijia.wr@antgroup.com>
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone May 1, 2024
@AkihiroSuda AkihiroSuda requested a review from a team May 1, 2024 00:11
Comment on lines 238 to 242
if mount.Destination == "/etc/resolv.conf" {
c.ResolvConfPath = mount.Source
}
if mount.Destination == "/etc/hostname" {
c.HostnamePath = mount.Source
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can use else if at line 241 to save an unnecessary if.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Fix containerd#2934

Containers created by CRI or ctr with bind mounts
don't have label "nerdct/mounts".

However, 'inspect' with dockercompat mode only parses mounts
from label "nerdct/mounts".

After this patch, 'inspect' with dockercompat mode can
show CRI and ctr container bind mounts.

ResolvConfPath, HostnamePath and Hostname are taken to the same
consideration.

Signed-off-by: baijia <baijia.wr@antgroup.com>
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

LGTM other than some minor comments, thanks


// TODO: add more internal labels testcases
labelMount := lbs[labels.Mounts]
expectedLabelMount := "[{\"Type\":\"bind\",\"Source\":\"/tmp\",\"Destination\":\"/app\",\"Mode\":\"rprivate,rbind\",\"RW\":true,\"Propagation\":\"rprivate\"}]"
Copy link
Member

Choose a reason for hiding this comment

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

nit: it'll be easier to visualize if you use raw string, eg.

expectedLabelMount := `[{"Type":"bind","Source":"/tmp",...}]`

inspect := base.InspectContainer(testContainer)
lbs := inspect.Config.Labels

// TODO: add more internal labels testcases
Copy link
Member

Choose a reason for hiding this comment

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

do you still have more tests to add?

Copy link
Author

Choose a reason for hiding this comment

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

do you still have more tests to add?

No.
There were never tests for internal labels, such as “containerd.io/restart.status” and "nerdctl/hostname", that are beyond the scope of this commit.

Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 4229bdb into containerd:main May 14, 2024
22 checks passed
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.

nerdctl container inspect can't show Mounts of container which is created by CRI or ctr
3 participants