Update heuristic for container creation time #2800

odinuge · 2021-02-07T16:08:28Z

The file "cgroup.clone_children" is not a part of cgroup v2, so use
"cgroup.events" instead. This heuristic is quite bad, so also check the
cgroup directories to see.

The old metric based on this is also constantly increasing due to this, making it more or less useless:

The file "cgroup.clone_children" is not a part of cgroup v2, so use "cgroup.events" instead. This heuristic is quite bad, so also check the cgroup directories to see.

k8s-ci-robot · 2021-02-07T16:08:36Z

Hi @odinuge. 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.

iwankgb · 2021-02-07T16:44:00Z

/ok-to-test

iwankgb

@odinuge, can you provide a test for your change, please?

odinuge · 2021-02-10T10:42:14Z

@odinuge, can you provide a test for your change, please?

Yeah! Thought I could see if you guys were interested before writing tests. This function doesn't have a test already, but can definitely add one.

iwankgb · 2021-02-10T18:08:19Z

The chart the you added to this PR clearly show a bug: monotonically increasing start time of a running container :D

bobbypage · 2021-02-12T01:46:35Z

Thanks @odinuge for the fix. Do you think this will fix kubernetes/kubernetes#98494 as well?

I'm curious how well the ModTime of the cgroup path works as a heuristic there as you proposed.

odinuge · 2021-02-12T10:21:37Z

Do you think this will fix kubernetes/kubernetes#98494 as well?

Ahh, I see. The current, and with my new patch, that would probably not be the case. In cadvisor, the CreationTime

cadvisor/info/v2/container.go

Lines 68 to 69 in 013e451

    
           // Time at which the container was created. 
        
           CreationTime time.Time `json:"creation_time,omitempty"`

clearly states that is it the time of the container creation (and that is not ==start time of a container). I think that the CreationTime should keep its value as long as the cgroup dir is not removed, and with that logic, this patch works.

However, if you want the start time of the CRI, I think we need another (and better) heuristic, preferably from CRI and not by reading cgroup files. I really don't think it is a good idea to track system daemon start times by reading mod times on cgroup files, compared to just providing it from the system or CRI. That is however another discussion.

I'm curious how well the ModTime of the cgroup path works as a heuristic there as you proposed.

If we talk about the "container creation time", I think this patch is better than the old one, but I still think that this heuristic can never be 100% by nature, due to the fact that cgroups aren't designed to to this. One can look at the start time of the oldest proc in a cgroup, or something like that.

Also, started looking at tests, but not really sure how useful they will be. Guess I have to mock an fs, or use temp dirs.

bobbypage · 2021-02-17T07:27:27Z

Thanks @odinuge. Agree, the goal here is to track CreationTime, not last start time.

However, if you want the start time of the CRI, I think we need another (and better) heuristic, preferably from CRI and not by reading cgroup files. I really don't think it is a good idea to track system daemon start times by reading mod times on cgroup files, compared to just providing it from the system or CRI. That is however another discussion.

Definitely, agree the tracking of that should be moved into CRI. I think the confusion stemed from the fact that the kubelet summary API calls this field startTime while it is in fact CreationTime.

LGTM on your changes here. Only one question, did you have a chance to test out the modTime of the cgroup and to see it if results in more or less stable CreationTime? I hope the results are a bit better than the graph you posted in #2800 (comment) since we should expect that CreationTime should remain static for a running container :)

harche · 2021-02-17T08:58:29Z

Thanks @odinuge, I see failures reported in /stats/summary test to with respect to container start time. I have documented it here, #2801 (comment)

harche · 2021-02-17T09:13:04Z

I just verified running Summary API test with this change, and it seem like this PR addresses the start time related failures in /stats/summary reported in #2801 (comment)

Thanks @odinuge for addressing this.

harche · 2021-02-17T09:13:46Z

LGTM

odinuge · 2021-02-17T12:20:48Z

Thanks @bobbypage!

LGTM on your changes here. Only one question, did you have a chance to test out the modTime of the cgroup and to see it if results in more or less stable CreationTime? I hope the results are a bit better than the graph you posted in #2800 (comment) since we should expect that CreationTime should remain static for a running container :)

Will test this later today and tomorrow, and report back as soon as I have more details.. 😄

Definitely, agree the tracking of that should be moved into CRI. I think the confusion stemed from the fact that the kubelet summary API calls this field startTime while it is in fact CreationTime.

Yeah! If k8s wants a better "started" time, they could look into it separately. 👍

@harche: Thanks @odinuge for addressing this.

Thanks! I am working with this becuase I want a more mature cgroup v2 support in Kubernetes so that I/we can start using it in production. 😄

odinuge · 2021-02-22T08:42:34Z

I have only had time to test with cgroup v2 (have no clusters with prometheus and cgroup v2), but it looks like it works well with there!

Pods on a given node:

The lowest line is kubepods.slice, and the timestamp corresponds to boot.

Here is a zoomed out one where one clearly can see the ever increasing metric, before it drops down with this patch:

The only issue I see so far is the root cgroup (only applies to cgroup v1, looks like this is correct for v2). You see it as the bottom line here, with a timestamp corresponding to the install date of the OS (I assume, since it is a date in early 2020). Maybe we can use boot time for the root cgroup?

edit: fixed by using boot time as lowest possible value

harche · 2021-02-22T08:47:46Z

The only issue I see so far is the root cgroup (only applies to cgroup v1, looks like this is correct for v2). You see it as the bottom line here, with a timestamp corresponding to the install date of the OS (I assume, since it is a date in early 2020). Maybe we can use boot time for the root cgroup?

Makes sense to use boot time.

odinuge · 2021-02-22T12:28:31Z

Makes sense to use boot time.

Nice! Added a check to instead always use max(creation_time, boot) in the last commit. Does that look ok? The fix is working, and the root cgroup gets the boot time as "CreatedAt".

harche · 2021-02-23T09:23:38Z

container/common/helpers.go

+		}
+	}
+
+	for _, cgroupPath := range cgroupPaths {


Why are we iterating over cgroupPaths again when we already doing it at line 79? Can we not merge the two loops and just iterate once? The only difference that I see between the first loop and the second is you are updating cgroupPath variable based on if it's v1 or v2.

I just thought it would be cleaner in order to make it simpler to understand the flow, but I can merge them. (If you are thinking about performance, I have no idea what the go compiler will do to cases like this). Agree that the duplicated logic is kinda stupid, so guess I can fix that anyway. Thanks! 👍

Pushed a new change now. Does that look better @harche?

Yes, thanks @odinuge

harche

LGTM

On some systems, root cgroup might report the time when creating the folder /sys/fs/cgroup/subsys, so limit to boot time.

bobbypage · 2021-03-03T08:10:03Z

LGTM

… creation time

移植upstream对kubelet及cadvisor的修改，修复使用cgroupv2时指标收集统计的问题 1. port cadvisor pr google/cadvisor#2839 reading cpu stats on cgroup v2 2. port cadvisor pr google/cadvisor#2837 read "max" value for cgroup v2 3. port cadvisor pr google/cadvisor#2801 gathering of stats for root cgroup on v2 4. port cadvisor pr google/cadvisor#2800: Update heuristic for container creation time 5. Fix cgroup handling for systemd with cgroup v2 6. test: adjust summary test for cgroup v2

Update heuristic for container creation time

6ecac4f

The file "cgroup.clone_children" is not a part of cgroup v2, so use "cgroup.events" instead. This heuristic is quite bad, so also check the cgroup directories to see.

google-cla bot added the cla: yes label Feb 7, 2021

k8s-ci-robot added the needs-ok-to-test label Feb 7, 2021

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 7, 2021

iwankgb requested changes Feb 10, 2021

View reviewed changes

iwankgb mentioned this pull request Feb 12, 2021

Inconsistent & stale metrics reported for a docker container which was restarted #2791

Open

bobbypage self-assigned this Feb 17, 2021

harche mentioned this pull request Feb 19, 2021

Failing kubelet /stats/summary API and ResourceMetricsAPI tests on cgroup v2 kubernetes/kubernetes#99230

Closed

odinuge force-pushed the container_creation_time branch from be44a6b to 7cc9b4c Compare February 22, 2021 12:30

harche reviewed Feb 23, 2021

View reviewed changes

harche approved these changes Feb 24, 2021

View reviewed changes

odinuge force-pushed the container_creation_time branch from deea5fc to feb4595 Compare February 24, 2021 13:30

odinuge added 2 commits February 24, 2021 14:46

Limit container start time to system boot

0d06830

On some systems, root cgroup might report the time when creating the folder /sys/fs/cgroup/subsys, so limit to boot time.

Refactor container start time calculation

b372640

odinuge force-pushed the container_creation_time branch from feb4595 to b372640 Compare February 24, 2021 13:47

iwankgb approved these changes Mar 2, 2021

View reviewed changes

bobbypage approved these changes Mar 3, 2021

View reviewed changes

bobbypage merged commit 1fcfaa6 into google:master Mar 3, 2021

odinuge deleted the container_creation_time branch March 8, 2021 10:23

bobbypage mentioned this pull request May 11, 2021

Container startTime reported by kubelet's /stats/summay is actually container createTime kubernetes/kubernetes#101902

Closed

liggitt mentioned this pull request Jun 24, 2021

Restarting containerd (and kubelet) does not impact "runtime" startTime in summary API kubernetes/kubernetes#98494

Closed

dashpole mentioned this pull request Jul 14, 2022

When metrics are counted, discard the wrong container StartTime metrics kubernetes/kubernetes#110880

Merged

chenchun pushed a commit to chenchun/kubernetes that referenced this pull request Mar 20, 2024

port cadvisor pr google/cadvisor#2800: Update heuristic for container…

f8a8afb

… creation time

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update heuristic for container creation time #2800

Update heuristic for container creation time #2800

odinuge commented Feb 7, 2021

k8s-ci-robot commented Feb 7, 2021

iwankgb commented Feb 7, 2021

iwankgb left a comment

odinuge commented Feb 10, 2021

iwankgb commented Feb 10, 2021

bobbypage commented Feb 12, 2021 •

edited

Loading

odinuge commented Feb 12, 2021

bobbypage commented Feb 17, 2021 •

edited

Loading

harche commented Feb 17, 2021

harche commented Feb 17, 2021

harche commented Feb 17, 2021

odinuge commented Feb 17, 2021

odinuge commented Feb 22, 2021 •

edited

Loading

harche commented Feb 22, 2021

odinuge commented Feb 22, 2021

harche Feb 23, 2021

odinuge Feb 23, 2021 •

edited

Loading

odinuge Feb 23, 2021

harche Feb 23, 2021

harche left a comment

bobbypage commented Mar 3, 2021

Update heuristic for container creation time #2800

Update heuristic for container creation time #2800

Conversation

odinuge commented Feb 7, 2021

k8s-ci-robot commented Feb 7, 2021

iwankgb commented Feb 7, 2021

iwankgb left a comment

Choose a reason for hiding this comment

odinuge commented Feb 10, 2021

iwankgb commented Feb 10, 2021

bobbypage commented Feb 12, 2021 • edited Loading

odinuge commented Feb 12, 2021

bobbypage commented Feb 17, 2021 • edited Loading

harche commented Feb 17, 2021

harche commented Feb 17, 2021

harche commented Feb 17, 2021

odinuge commented Feb 17, 2021

odinuge commented Feb 22, 2021 • edited Loading

harche commented Feb 22, 2021

odinuge commented Feb 22, 2021

harche Feb 23, 2021

Choose a reason for hiding this comment

odinuge Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

odinuge Feb 23, 2021

Choose a reason for hiding this comment

harche Feb 23, 2021

Choose a reason for hiding this comment

harche left a comment

Choose a reason for hiding this comment

bobbypage commented Mar 3, 2021

bobbypage commented Feb 12, 2021 •

edited

Loading

bobbypage commented Feb 17, 2021 •

edited

Loading

odinuge commented Feb 22, 2021 •

edited

Loading

odinuge Feb 23, 2021 •

edited

Loading