Ignore transient errors when gathering stats for Windows Containers in Dockershim #98510

jsturtevant · 2021-01-28T06:50:37Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
When a Windows container is started by docker there is a case when stats may be accessed before the container is started causing errors.

Which issue(s) this PR fixes:

Fixes #98509

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix errors when accessing Windows container stats for Dockershim

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig windows

jsturtevant · 2021-01-28T06:53:23Z

/priority critical-urgent

ehashman

/triage accepted
LGTM but I'm not a windows person :)

pkg/kubelet/dockershim/docker_stats_windows.go

jsturtevant · 2021-01-28T19:13:29Z

/assign @marosset

jsturtevant · 2021-01-28T22:08:05Z

looks like pull-kubernetes-bazel-build failed with

2021/01/28 21:58:14 Image pull was unsuccessful: unable to save remote image k8s.gcr.io/build-image/debian-base@sha256:afff10fcd513483e492807f8d934bdf0be4a237997f55e0f1f8e34c04a6cb213: unable to write image layers: unable to write image layer: unable to write the contents of layer 0 to /bazel-scratch/.cache/bazel/_bazel_root/7989b31489f31aee54f32688da2f0120/external/debian-base-amd64/image/000.tar.gz: read tcp 10.32.255.3:46322->142.250.128.128:443: read: connection reset by peer

/retest

jsturtevant · 2021-01-28T23:35:58Z

looks like the azure disk is known issue: #98492

jsturtevant · 2021-01-28T23:39:27Z

/test pull-kubernetes-e2e-azure-disk-windows

pkg/kubelet/dockershim/docker_stats_windows.go

marosset · 2021-01-29T00:13:08Z

pkg/kubelet/dockershim/docker_stats_windows.go

+			// This is transient; skip container for now
+			// These hcs errors do not have helpers exposed in public package so need to query for the known codes:
+			// https://github.com/microsoft/hcsshim/blob/master/internal/hcs/errors.go
+			klog.Errorf("Container is not in a state that stats can be accessed '%s': %v. This occurs when the container is created but not started.", containerID, err)


Should we make this an info message since nothing is actually wrong and there is nothing actionable?

I was following the same pattern as above

kubernetes/pkg/kubelet/dockershim/docker_stats_windows.go

Lines 41 to 43 in 4a8b2d3

if !hcsshim.IsNotExist(err) && !hcsshim.IsAlreadyStopped(err) {

klog.Errorf("Error opening container (stats will be missing) '%s': %v", containerID, err)

}

If we do that here I think we should do it there too?

Let's update both messages to log as Info at V(4).
That matches some similar logs messages i'm seeing in dockershim

kubernetes/pkg/kubelet/dockershim/docker_container.go

Line 76 in 64a6ee4

klog.V(4).Infof("Unable to convert docker to runtime API container: %v", err)

@ehashman any concerns or comments with this?

I know we're not yet on structured logging in this file yet but I don't think there's any issue with changing this to an Info, the current guidance is that expected errors are fine to log at the info level: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#using-errors

I've updated to log using info

marosset · 2021-02-02T22:00:48Z

/lgtm
/approve

marosset · 2021-02-02T22:01:12Z

/milestone v1.21

jsturtevant · 2021-02-03T00:27:25Z

/test pull-kubernetes-e2e-azure-disk-windows

jsturtevant · 2021-02-04T18:00:32Z

@ehashman this is ready for final approval

jsturtevant · 2021-02-04T18:02:27Z

@ehashman I see you are not the approver for this sorry!

/assign @dchen1107 @derekwaynecarr

mrunalp · 2021-02-09T17:08:54Z

/approve

k8s-ci-robot · 2021-02-09T17:09:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsturtevant, marosset, mrunalp

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

AbelHu · 2021-05-13T06:09:46Z

@jsturtevant Thanks for fixing this issue. Do you have a plan to backport this fix to earlier versions since lots of users are hitting this issue?

…#98510-upstream-release-1.20 Automated cherry pick of #98510: Ignore transient errors when gather stats

…#98510-upstream-release-1.19 Automated cherry pick of #98510: Ignore transient errors when gather stats

k8s-ci-robot requested review from derekwaynecarr and odinuge January 28, 2021 06:51

k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 28, 2021

jsturtevant changed the title ~~Ignore transient errors when gather stats for Windows Containers in Dockershim~~ Ignore transient errors when gathering stats for Windows Containers in Dockershim Jan 28, 2021

ehashman added this to Triage in SIG Node PR Triage Jan 28, 2021

ehashman reviewed Jan 28, 2021

View reviewed changes

pkg/kubelet/dockershim/docker_stats_windows.go Outdated Show resolved Hide resolved

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 28, 2021

ehashman moved this from Triage to Needs Reviewer in SIG Node PR Triage Jan 28, 2021

ehashman reviewed Jan 28, 2021

View reviewed changes

pkg/kubelet/dockershim/docker_stats_windows.go Show resolved Hide resolved

k8s-ci-robot assigned marosset Jan 28, 2021

jsturtevant force-pushed the windows-metrics-access-denied branch from 02ab72e to 1e696b1 Compare January 28, 2021 21:55

marosset reviewed Jan 29, 2021

View reviewed changes

pkg/kubelet/dockershim/docker_stats_windows.go Outdated Show resolved Hide resolved

marosset reviewed Jan 29, 2021

View reviewed changes

Ignore transient errors when gather stats

3da0d99

jsturtevant force-pushed the windows-metrics-access-denied branch from 1e696b1 to 3da0d99 Compare February 2, 2021 21:57

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 2, 2021

ehashman moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Feb 2, 2021

k8s-ci-robot assigned dchen1107 and derekwaynecarr Feb 4, 2021

mrunalp moved this from Needs Approver to Done in SIG Node PR Triage Feb 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021

k8s-ci-robot merged commit b9b11d3 into kubernetes:master Feb 9, 2021

This was referenced Jun 1, 2021

Automated cherry pick of #98510: Ignore transient errors when gather stats #102499

Merged

Automated cherry pick of #98510: Ignore transient errors when gather stats #102500

Merged

k8s-ci-robot added a commit that referenced this pull request Jun 8, 2021

Merge pull request #102499 from jsturtevant/automated-cherry-pick-of-…

70bfeab

…#98510-upstream-release-1.20 Automated cherry pick of #98510: Ignore transient errors when gather stats

k8s-ci-robot added a commit that referenced this pull request Jun 8, 2021

Merge pull request #102500 from jsturtevant/automated-cherry-pick-of-…

67f68f9

…#98510-upstream-release-1.19 Automated cherry pick of #98510: Ignore transient errors when gather stats

AbelHu mentioned this pull request Jun 24, 2021

Cache v1.19.12, v1.20.8 and v1.21.2 in AKS Windows VHD Azure/AgentBaker#901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore transient errors when gathering stats for Windows Containers in Dockershim #98510

Ignore transient errors when gathering stats for Windows Containers in Dockershim #98510

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

ehashman left a comment

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

marosset Jan 29, 2021

jsturtevant Jan 29, 2021

marosset Jan 29, 2021

ehashman Jan 29, 2021

jsturtevant Feb 2, 2021

marosset commented Feb 2, 2021

marosset commented Feb 2, 2021

jsturtevant commented Feb 3, 2021

jsturtevant commented Feb 4, 2021

jsturtevant commented Feb 4, 2021

mrunalp commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

AbelHu commented May 13, 2021

	if !hcsshim.IsNotExist(err) && !hcsshim.IsAlreadyStopped(err) {
	klog.Errorf("Error opening container (stats will be missing) '%s': %v", containerID, err)
	}

Ignore transient errors when gathering stats for Windows Containers in Dockershim #98510

Ignore transient errors when gathering stats for Windows Containers in Dockershim #98510

Conversation

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

ehashman left a comment

Choose a reason for hiding this comment

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

jsturtevant commented Jan 28, 2021

marosset Jan 29, 2021

Choose a reason for hiding this comment

jsturtevant Jan 29, 2021

Choose a reason for hiding this comment

marosset Jan 29, 2021

Choose a reason for hiding this comment

ehashman Jan 29, 2021

Choose a reason for hiding this comment

jsturtevant Feb 2, 2021

Choose a reason for hiding this comment

marosset commented Feb 2, 2021

marosset commented Feb 2, 2021

jsturtevant commented Feb 3, 2021

jsturtevant commented Feb 4, 2021

jsturtevant commented Feb 4, 2021

mrunalp commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

AbelHu commented May 13, 2021