Skip to content

support disk usage metrics for containerd factory #3502

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

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

Conversation

chengjoey
Copy link

query rootfs dir from contianerd status and get disk usage stat by rootfs dir /run/containerd/io.containerd.runtime.v2.task/${contianerid}/rootfs

set hasFileSystem to true for containerd when included disk usage metrics
added common fs handler for container rootfs

@k8s-ci-robot
Copy link
Collaborator

Hi @chengjoey. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@chengjoey
Copy link
Author

@@ -169,7 +169,7 @@ func TestAddDiskStatsCheck(t *testing.T) {
fileSystems := []fs.Fs{fileSystem}

var fsStats info.FsStats
addDiskStats(fileSystems, nil, &fsStats)
fs.AddDiskStats(fileSystems, nil, &fsStats)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should be moved to fs module.

Copy link
Author

Choose a reason for hiding this comment

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

done~

@@ -203,7 +203,7 @@ func TestAddDiskStats(t *testing.T) {
fileSystems := []fs.Fs{fileSystem}

// Act
addDiskStats(fileSystems, &fsInfo, &fsStats)
fs.AddDiskStats(fileSystems, &fsInfo, &fsStats)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

Copy link
Author

Choose a reason for hiding this comment

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

done~

}
configStr := r.Info["config"]
config := make(map[string]interface{})
if err := json.Unmarshal([]byte(configStr), &config); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you unmarshal config into a struct? It will be easier to grasp what values are interesting.

Copy link
Author

Choose a reason for hiding this comment

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

The original definition of config is here

I extracted the struct in config.go to container/containerd/config

@iwankgb
Copy link
Collaborator

iwankgb commented Mar 31, 2024

/ok-to-test

@chengjoey
Copy link
Author

/retest

@silence8013
Copy link

Will this commit continue? Looking forward to it~

@duanhongyi
Copy link

I am looking forward to this feature merging.

@jianxiaoguo
Copy link

I eagerly anticipate the completion of this feature.

@EamonZhang
Copy link

This feature is particularly important and I'm looking forward to it

@wsszh
Copy link

wsszh commented May 24, 2024

Hi, I think the disk usage of a container should include the usage of log files in /var/log/pods.

@chengjoey
Copy link
Author

Hi, I think the disk usage of a container should include the usage of log files in /var/log/pods.

hi @wsszh , The disk usage calculation of contianerd is similar to that of docker and podman. I think that several container factories currently do not include /var/log/pods in the calculation. This may be because cadvisor is more about container-level calculations rather than pods. It is also difficult to know which directory should be calculated because it is not fixed in /var/log/pods. although I think it should be calculated, the specific directory of the log cannot be obtained from the info interface of cri.

@chengjoey
Copy link
Author

Hi, I think the disk usage of a container should include the usage of log files in /var/log/pods.

if a container should include the usage of log files, I would like to submit another PR to modify the container runtimes such as podman, docker, containerd, etc. This PR only does usage metric for containerd

@chengjoey chengjoey force-pushed the feat/containerd-disk-usage branch 6 times, most recently from 347eed9 to 1b85ff8 Compare August 14, 2024 15:04
query rootfs dir from contianerd status and get disk usage stat by rootfs dir `/run/containerd/io.containerd.runtime.v2.task/${contianerid}/rootfs`

Signed-off-by: joey <[email protected]>
@chengjoey chengjoey force-pushed the feat/containerd-disk-usage branch from 1b85ff8 to 9e7a939 Compare August 14, 2024 15:13
@kxbin
Copy link

kxbin commented Dec 19, 2024

great work,Hope to speed up to merge in master

@melikeiremguler
Copy link

melikeiremguler commented Dec 30, 2024

Hi, will this pull request solve this problem ?

@chengjoey
Copy link
Author

Hi, will this pull request solve this problem ?

@melikeiremguler , Yes, this PR is to solve the issue of 2785.

@gigilee
Copy link

gigilee commented Mar 24, 2025

Is this PR going to be merged?

@Imlosing
Copy link

Looking forward to the early merge.

@Imlosing
Copy link

Looking forward to the early merge.

Or is there any alternative solution now?

@kam1kaze
Copy link

kam1kaze commented May 5, 2025

@iwankgb How could we speed up merge process of this code? We are using it on production and it works perfect.

@Imlosing
Copy link

@iwankgb How could we speed up merge process of this code? We are using it on production and it works perfect.

@kam1kaze I also merged this PR and tried to use it, but I found that the actual value collected was greater than the writable layer capacity. Have you encountered this problem?

@kamikaze
Copy link

please

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.