[cri] Handle pod transition states gracefully while listing pod stats by jsturtevant · Pull Request #8671 · containerd/containerd

jsturtevant · 2023-06-10T00:14:21Z

When the pods are transitioning there are several cases where containers might not be in valid state. There were several cases where the stats where failing hard but we should just continue on as they are transient and will be picked up again when kubelet queries for the stats again.

I've found these after running the e2e tests in kubernetes/kubernetes#116968 several times.

k8s-ci-robot · 2023-06-10T00:14:31Z

Hi @jsturtevant. Thanks for your PR.

I'm waiting for a containerd 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.

Details

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.

jsturtevant · 2023-06-10T00:20:41Z

pkg/cri/sbserver/sandbox_stats_windows.go


 		processes, err := task.Pids(ctx)
 		if err != nil {
+			if errdefs.IsNotFound(err) {


this requires microsoft/hcsshim#1807 but should be ok to merge this and follow up via hcsshim version bump. Alternatively, I can handle the errors directly here.

jsturtevant · 2023-06-13T19:11:15Z

looks like the ci integration was a flake

@dcantah

marosset

LGTM

jsturtevant · 2023-06-15T21:31:41Z

/hold
It looks like I goof up a rebase and only have some changes in the SB server

jsturtevant · 2023-06-16T00:06:45Z

/hold cancel
We are good to go! I ran the e2e suite as well. Below is the stats specific tests but I also ran the entire suite

/bin/e2e.test --provider=skeleton --ginkgo.noColor --ginkgo.focus="\[Feature:Windows\] Kubelet-Stats" --ginkgo.skip="\[Serial\]" --node-os-distro=windows --disable-log-dump --ginkgo.progress=true --ginkgo.slowSpecThreshold=120.0 --ginkgo.flakeAttempts=0 --ginkgo.trace=true --ginkgo.timeout=24h --num-nodes=2 --ginkgo.v=true
Ran 2 of 7207 Specs in 28.926 seconds                                                                                                                                                      SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 7205 Skipped

mikebrow · 2023-06-16T13:03:05Z

/ok-to-test

mikebrow

would prefer doing this in a common way across platform impls over in podsandboxstats and listpodsandboxstats...

not sure it makes sense to return nil vs err for podsandboxstats.. but agree it makes sense to not fail all of listpodsandboxstats for one pod being not ready or unknown at the point of gathering the stats, that feels like an error

jsturtevant · 2023-06-16T16:03:35Z

not sure it makes sense to return nil vs err for podsandboxstats.. but agree it makes sense to not fail all of listpodsandboxstats for one pod being not ready or unknown at the point of gathering the stats, that feels like an error

@bobbypage @haircommander do you have any thoughts? I assume some of the changes to soften the errors would apply to Linux side as well

jsturtevant · 2023-06-21T20:46:04Z

not sure it makes sense to return nil vs err for podsandboxstats.

I am leaning towards returning an err for PodSandboxStats. Then the caller can determine if they want to fail; In the case of the ListPodSandboxStats we don't want to fail most of the time.

It might make sense to wrap some of the transient errs to make the decision easier to understand but at this time I'm not sure I know which errors we would really want to fail on for ListPodSandboxStats as there are a lot of failure returns currently in Linux and Windows in the stats code flow.

haircommander · 2023-06-21T21:03:28Z

I believe most of the time an error is treated as transient. if it's worth logging in the kubelet, then I'd report it. If it's part of normal state changes in containerd, then I would say hide the errors. e.g: is there action a user can take, or a maintainer should be aware of? if so, error. if not, hide

jsturtevant · 2023-06-21T21:07:45Z

@haircommander @mikebrow I've made the following change e489ff5

jsturtevant · 2023-06-26T23:03:21Z

@haircommander @mikebrow PTAL

marosset

LGTM

haircommander · 2023-07-10T18:42:24Z

LGTM

mikebrow

See comments regarding using switch for err processing and changing the default for err to continue setting up to return the best can do results and the first/last/list of transient/errors etc.. vs return nil with err on unknown type of err..

pkg/cri/server/sandbox_stats_list.go

mikebrow

LGTM

mikebrow · 2023-07-11T14:15:12Z

will be interesting on the kubelet side if they will choose to parse and eval the transient stats errors or log and ignore as we are moving to here... or if there will be a subsequent request to go even further and clear the transient errors. FYI @haircommander @dims @saschagrunert @SergeyKanzhelev

marosset · 2023-07-11T18:16:22Z

See comments regarding using switch for err processing and changing the default for err to continue setting up to return the best can do results and the first/last/list of transient/errors etc.. vs return nil with err on unknown type of err..

@mikebrow - I updated the PR to use a switch statement for style

mikebrow

LGTM

mikebrow · 2023-07-12T13:17:18Z

just a couple logistical things.. pls remove the .idea files and squash.. thx!

fuweid · 2023-07-12T15:55:33Z

.idea/.gitignore

@@ -0,0 +1,8 @@
+# Default ignored files


please remove the folder~

When the pods are transitioning there are several cases where containers might not be in valid state. There were several cases where the stats where failing hard but we should just continue on as they are transient and will be picked up again when kubelet queries for the stats again. Signed-off-by: James Sturtevant <jstur@microsoft.com> Signed-off-by: Mark Rossetti <marosset@microsoft.com>

marosset · 2023-07-12T16:58:56Z

just a couple logistical things.. pls remove the .idea files and squash.. thx!

done and done!

mikebrow

/LGTM

mikebrow · 2023-07-12T20:45:29Z

any objections to flagging these as cherry pick for 1.7 and 1.6?

jsturtevant · 2023-07-13T17:25:04Z

thanks @marosset and @mikebrow

k8s-ci-robot added the needs-ok-to-test label Jun 10, 2023

jsturtevant mentioned this pull request Jun 10, 2023

When fetching the pid counts for the container the state can be invalid sometimes microsoft/hcsshim#1807

Merged

jsturtevant commented Jun 10, 2023

View reviewed changes

jsturtevant force-pushed the fix-windows-edge-cases branch from 13c357b to 74cfd2f Compare June 10, 2023 00:23

jsturtevant mentioned this pull request Jun 10, 2023

kubelet: Implement support for Windows podAndContainerStatsFromCRI kubernetes/kubernetes#116968

Merged

jsturtevant force-pushed the fix-windows-edge-cases branch 2 times, most recently from b0f40c9 to d0e8742 Compare June 13, 2023 03:03

marosset reviewed Jun 15, 2023

View reviewed changes

dcantah approved these changes Jun 15, 2023

View reviewed changes

jsturtevant force-pushed the fix-windows-edge-cases branch from d0e8742 to 05b5bcb Compare June 15, 2023 21:39

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 16, 2023

mikebrow requested changes Jun 16, 2023

View reviewed changes

jsturtevant force-pushed the fix-windows-edge-cases branch from 05b5bcb to 67ea3f4 Compare June 21, 2023 21:06

jsturtevant force-pushed the fix-windows-edge-cases branch 3 times, most recently from d3de050 to 5cba93b Compare June 26, 2023 22:03

jsturtevant changed the title ~~[cri] Handle Windows pod transition states gracefully~~ [cri] Handle pod transition states gracefully while listing pod stats Jun 26, 2023

marosset approved these changes Jul 5, 2023

View reviewed changes

mikebrow reviewed Jul 10, 2023

View reviewed changes

pkg/cri/server/sandbox_stats_list.go Outdated Show resolved Hide resolved

pkg/cri/server/sandbox_stats_list.go Outdated Show resolved Hide resolved

mikebrow approved these changes Jul 11, 2023

View reviewed changes

marosset force-pushed the fix-windows-edge-cases branch from 3d7fd8b to f1f144f Compare July 11, 2023 18:15

mikebrow approved these changes Jul 11, 2023

View reviewed changes

fuweid reviewed Jul 12, 2023

View reviewed changes

.idea/.gitignore Outdated

@@ -0,0 +1,8 @@

# Default ignored files

Copy link
Copy Markdown

Member

fuweid Jul 12, 2023

Choose a reason for hiding this comment

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

please remove the folder~

marosset force-pushed the fix-windows-edge-cases branch from f1f144f to f914edf Compare July 12, 2023 16:58

mikebrow approved these changes Jul 12, 2023

View reviewed changes

mikebrow merged commit 3ed1bc1 into containerd:main Jul 12, 2023

mikebrow added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Jul 13, 2023

fuweid mentioned this pull request Mar 1, 2024

[cri] podSandboxStats should return errdefs.ErrUnavailable without wrapping #9897

Closed

dims mentioned this pull request Mar 1, 2024

[release/1.7] Handle pod transition states gracefully while listing pod stats #9905

Merged

samuelkarp added area/cri Container Runtime Interface (CRI) cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Aug 1, 2024

Conversation

jsturtevant commented Jun 10, 2023

Uh oh!

k8s-ci-robot commented Jun 10, 2023

Uh oh!

jsturtevant Jun 10, 2023

Choose a reason for hiding this comment

Uh oh!

jsturtevant commented Jun 13, 2023

Uh oh!

marosset left a comment

Choose a reason for hiding this comment

Uh oh!

jsturtevant commented Jun 15, 2023

Uh oh!

jsturtevant commented Jun 16, 2023

Uh oh!

mikebrow commented Jun 16, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

jsturtevant commented Jun 16, 2023

Uh oh!

jsturtevant commented Jun 21, 2023

Uh oh!

haircommander commented Jun 21, 2023

Uh oh!

jsturtevant commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsturtevant commented Jun 26, 2023

Uh oh!

marosset left a comment

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jul 10, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marosset commented Jul 11, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jul 12, 2023

Uh oh!

fuweid Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

marosset commented Jul 12, 2023

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow commented Jul 12, 2023

Uh oh!

jsturtevant commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jsturtevant commented Jun 21, 2023 •

edited

Loading

mikebrow commented Jul 11, 2023 •

edited

Loading