Fix metrics performance issue #15

[OleksiiKhanin]

Details see #15

[chancez]

I might be hitting this with rancher-desktop, causing metrics-server to sometimes timeout. It takes 12 seconds to get stats from kubelet:

time curl https://192.168.192.50:10250/stats/summary?only_cpu_and_memory=true -k -v -H "Authorization: Bearer ${TOKEN}"

[chancez]

I can confirm this fixes the issue I'm seeing.

[wpwoodjr]

I might be hitting this with rancher-desktop, causing metrics-server to sometimes timeout. It takes 12 seconds to get stats from kubelet:

time curl https://192.168.192.50:10250/stats/summary?only_cpu_and_memory=true -k -v -H "Authorization: Bearer ${TOKEN}"

I'm seeing the same behavior. With a few application pods running, it can take 20 seconds.

[pioh]

There are two errors in this code
https://go.dev/play/p/EDXoixwXL2M

Fix:
https://github.com/OleksiiKhanin/cri-dockerd/pull/1/files

[pioh]

Also docker container stat PrivateWorkingSet returning 0 on linux
https://github.com/moby/moby/blob/master/api/types/stats.go#L81

Comment says that this is windows memory stat and may be this fix required:
pioh@6e887d6

[chancez]

Good catch: https://github.com/golang/go/wiki/CommonMistakes#using-goroutines-on-loop-iterator-variables

[travisghansen]

Doesn’t look to me like the bugs mentioned above were addressed in the merged code?

[jandubois]

Doesn’t look to me like the bugs mentioned above were addressed in the merged code?

No, they were not. Maybe someone needs to open them as additional PRs.

There are also changes required to the Github Action code to make it still build and test correctly (needs bump to Golang 1.18, and changes to the test runner command).

[evol262]

The github workflows were updated to 1.18, and ginkgo updated to match.

No, nobody needs to open these as additional PRs. I'm tracing down this moby issue in GH runners, then building debs/rpms/static binaries before cutting a release once it's confirmed that it's actually moby

[jandubois]

I'm tracing down this moby issue in GH runners, then building debs/rpms/static binaries before cutting a release once it's confirmed that it's actually moby

Thank you very much for looking into this!

[evol262]

Sorry it took so long. It's a constant fight with Github, since it doesn't send me any notifications at all unless someone @s me, despite being subscribed to/a maintainer of this repo.

Considering that 0.2.0 was verified working with minikube in January or so, and dockershim/cri-dockerd was more or less complete (it's unlikely that log streaming, for example, will ever actually make it in despite being part of the CRI spec), this repo got put on the back burner until 1.24 was near for testing.

Then I saw a bunch of old PRs/issues. I'm just gonna set a calendar reminder to check the issues/PRs so I don't let it languish.

[jandubois]

I'm just gonna set a calendar reminder to check the issues/PRs so I don't let it languish.

Thanks, very much appreciated. Otherwise I can also ping Brent again, now that I know that he works with you. 😄

[evol262]

That, too. Github is just as easy though ;)

[travisghansen]

I'm unclear why it was merged as is if there are known issues? The CommonMistakes link explains pretty clearly that as-is the results will be erratic no?

[evol262]

Which the follow-up PR linked here resolves. Frankly, I didn't see the comments about it until after merging the PR (the assumption would normally be that the PR would have been updated), which is the reason for an immediate turnaround patch before releasing.

[travisghansen]

Ah! I missed it being included in the other PR, sorry for the confusion!