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

support disk usage metrics for containerd factory #3502

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

support disk usage metrics for containerd factory #3502

wants to merge 1 commit into 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

container_fs_usage_bytes metrics miss pod/container label when useing containerd

This was referenced Mar 19, 2024
Open
Open
iwankgb
iwankgb requested changes Mar 31, 2024
View reviewed changes
container/docker/handler_test.go Outdated
@@ -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~

container/docker/handler_test.go Outdated
@@ -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~

container/containerd/client.go
}
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
@chengjoey
support disk usage metrics for containerd factory
9e7a939 
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 <zchengjoey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwankgb iwankgb iwankgb requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@chengjoey @k8s-ci-robot @iwankgb @silence8013 @duanhongyi @jianxiaoguo @EamonZhang @wsszh