This change is 

fyi @vishh @sjenning

/lgtm

/approve

@k8s-bot kops aws e2e test this

/approve

@k8s-bot kops aws e2e test this

@derekwaynecarr I added a do not merge label to prevent this PR from going in to the next beta release.

@dchen1107 wants this feature and node allocatable to be separated from CRI to make it easy to debug issues in beta releases.

@k8s-bot kops aws e2e test this

A release has been made with CRI. This PR is ready to go.

Yes, this pr is ready to go. Thanks!

so i just pushed a rebase, but i have no clue how the kops environment is configured.

looking at the kops logs for kubelet on earlier runs:

I see messages like the following:

W0217 23:16:39.530238    1431 container_manager_linux.go:728] CPUAccounting not enabled for pid: 995
W0217 23:16:39.530526    1431 container_manager_linux.go:731] MemoryAccounting not enabled for pid: 995
I0217 23:16:39.530804    1431 container_manager_linux.go:388] [ContainerManager]: Discovered runtime cgroups name: /system.slice/docker.service
W0217 23:16:39.531152    1431 container_manager_linux.go:728] CPUAccounting not enabled for pid: 1431
W0217 23:16:39.531431    1431 container_manager_linux.go:731] MemoryAccounting not enabled for pid: 1431

This tells me that the image they run does not have cpu and memory accounting on by default. Someone on the kops team needs to fix the image to enable accounting globally via /etc/systemd/system.conf (DefaultCPUAccounting=true DefaultMemoryAccounting=true).

Further down the list of problems:

Failed cleaning pods: failed to get list of pods that still exist on cgroup mounts: failed to read the cgroup directory /sys/fs/cgroup/systemd/docker/BestEffort : open /sys/fs/cgroup/systemd/docker/BestEffort: no such file or directory

This confuses me, its possible cgroup-root is being configured incorrectly.

Are there any @kubernetes/kops-maintainers that can help fix their environments or debug this?

We are trying to roll out a new cgroup topology, and node allocatable support, and that requires some requisite changes to the configuration of the OS that may or may not be present.

Or can the @kubernetes/kops-maintainers set --cgroups-per-qos=false by default in their configuration while we stabilize the rollout of pod level cgroups / node allocatable?

/cc @vishh @dchen1107 @smarterclayton @chrislovecnm

Ok, it looks like kops has cgroup-root set to docker. That won't work, I can use this as a case study to error better, but we need kops to change I think...

@justinsb -- who is creating the /docker cgroup with this configuration?

If not running the systemd cgroup driver, I think it would be safe to ignore that entire mountpoint entirely. If latest test runs validate that theory, I will add that patch to this PR.

@justinsb -- who is creating the /docker cgroup with this configuration?

Docker was. Honestly I don't understand this, and nor did anyone else when I asked at the time. But I probably should have asked you :-)

So: What cgroups configuration should we be running in kubernetes (and therefore kops)?

@justinsb -- you should run with --cgroup-root=/ (which will be the default when this PR is enabled) and you should use the --cgroup-driver= to the value returned when you call docker info for its cgroup driver. it looks like on your install, it's using the cgroupfs driver, and so you taking the kube default for --cgroup-driver=cgroupfs makes sense.

i think for the bug exposed in this pr, it should be safe to ignore the systemd mountpoint entirely if you are not running the systemd cgroup driver. i will update, and see if the tests are happier.

OK that is super helpful @derekwaynecarr - I'll make the change in kops. Also, if we should be using a particular cgroup driver, we can probably force it at least on the images we build - let me know!

@justinsb -- ok, my last commit tested explicitly overriding --cgroup-root to / for kops, and as you can see kops e2e is happy on this pr now. if you can merge kubernetes/kops#1944 then I can remove the band-aid on this PR.

@k8s-bot gce etcd3 e2e test this

the kops pr to change cgroup-root to / has merged here:
kubernetes/kops#1955

I dropped my temporary patch and have kicked off the PR to hopefully see us passing.

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: derekwaynecarr, eparis, vishh

Needs approval from an approver in each of these OWNERS Files:

• cmd/kubelet/app/OWNERS [derekwaynecarr,vishh]
• hack/OWNERS [eparis]
• pkg/apis/componentconfig/OWNERS [vishh]
• pkg/kubelet/OWNERS [derekwaynecarr,vishh]
• test/e2e_node/OWNERS [vishh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

yay, kops passed!

Hooray - thanks for helping us clean up cgroups in kops @derekwaynecarr :-)

@k8s-bot gci gke e2e test this
@k8s-bot cvm gke e2e test this
@k8s-bot unit test this

@k8s-bot cvm gce e2e test this
@k8s-bot gci gke e2e test this
@k8s-bot cvm gke e2e test this

@k8s-bot cvm gce e2e test this

all green, retagging to merge today.

Automatic merge from submit-queue (batch tested with PRs 41349, 41532, 41256, 41587, 41657)