Skip to content

Commit

Permalink
Merge pull request #61482 from filbranden/summary3
Browse files Browse the repository at this point in the history
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix summary_test to work when XFS is used under overlay2 and add a check for Delegate=yes missing from docker.service

**What this PR does / why we need it**:
This fixes the summary_test checks to work in cases where:
1. Docker is using overlay2 for its images with XFS as backing filesystem.
1. The systemd unit for Docker does not include Delegate=yes.

The former will break RootFs minimum usage check from summary_test, since it expects _some_ usage even though the upper layer only contains directories that are used as mount points. It turns out the XFS filesystem returns "0" blocks in the stat() result for a directory, so this breaks the test. Fix it by creating a file with some small contents in the test, so that `du` will actually return some usage.

**NOTE**: I introduced this step in the loop part of the function. It works, but maybe it's not the best... Let me know if you think we should do some small cleanup here too, I'd be happy to do that.

Regarding the latter, when `Delegate=yes` is not included in `docker.service`, then systemd might choose not to create Memory and CPU cgroups (actually, any of the resource cgroups) for the unit when it starts it. It's a bit more complicated than that, because it *does* create them if any sibling units need it, so the behavior is a bit hard to control... In any case, here we're checking on it and accepting that we might get a "nil" from cAdvisor in cases where `Delegate=yes` is missing.

Both of these issues can be found on CentOS/RHEL, that's the motivation for the fixes.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
N/A

**Special notes for your reviewer**:
/assign dashpole

**Release note**:

```release-note
NONE
```
  • Loading branch information
Kubernetes Submit Queue authored Mar 30, 2018
2 parents 5ae7bba + b8c39b7 commit 189a166
Showing 1 changed file with 27 additions and 2 deletions.
29 changes: 27 additions & 2 deletions test/e2e_node/summary_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package e2e_node
import (
"fmt"
"io/ioutil"
"os/exec"
"strings"
"time"

Expand Down Expand Up @@ -118,9 +119,33 @@ var _ = framework.KubeDescribe("Summary API", func() {
"PageFaults": bounded(0, 1000000),
"MajorPageFaults": bounded(0, 10),
})
runtimeContExpectations := sysContExpectations().(*gstruct.FieldsMatcher)
if systemdutil.IsRunningSystemd() && framework.TestContext.ContainerRuntime == "docker" {
// Some Linux distributions still ship a docker.service that is missing
// a `Delegate=yes` setting (or equivalent CPUAccounting= and MemoryAccounting=)
// that allows us to monitor the container runtime resource usage through
// the "cpu" and "memory" cgroups.
//
// Make an exception here for those distros, only for Docker, so that they
// can pass the full node e2e tests even in that case.
//
// For newer container runtimes (using CRI) and even distros that still
// ship Docker, we should encourage them to always set `Delegate=yes` in
// order to make monitoring of the runtime possible.
stdout, err := exec.Command("systemctl", "show", "-p", "Delegate", "docker.service").CombinedOutput()
if err == nil && strings.TrimSpace(string(stdout)) == "Delegate=no" {
// Only make these optional if we can successfully confirm that
// Delegate is set to "no" (in other words, unset.) If we fail
// to check that, default to requiring it, which might cause
// false positives, but that should be the safer approach.
By("Making runtime container expectations optional, since systemd was not configured to Delegate=yes the cgroups")
runtimeContExpectations.Fields["Memory"] = Or(BeNil(), runtimeContExpectations.Fields["Memory"])
runtimeContExpectations.Fields["CPU"] = Or(BeNil(), runtimeContExpectations.Fields["CPU"])
}
}
systemContainers := gstruct.Elements{
"kubelet": sysContExpectations(),
"runtime": sysContExpectations(),
"runtime": runtimeContExpectations,
"pods": podsContExpectations,
}
// The Kubelet only manages the 'misc' system container if the host is not running systemd.
Expand Down Expand Up @@ -324,7 +349,7 @@ func getSummaryTestPods(f *framework.Framework, numRestarts int32, names ...stri
{
Name: "busybox-container",
Image: busyboxImage,
Command: getRestartingContainerCommand("/test-empty-dir-mnt", 0, numRestarts, "ping -c 1 google.com; echo 'hello world' >> /test-empty-dir-mnt/file;"),
Command: getRestartingContainerCommand("/test-empty-dir-mnt", 0, numRestarts, "echo 'some bytes' >/outside_the_volume.txt; ping -c 1 google.com; echo 'hello world' >> /test-empty-dir-mnt/file;"),
Resources: v1.ResourceRequirements{
Limits: v1.ResourceList{
// Must set memory limit to get MemoryStats.AvailableBytes
Expand Down

0 comments on commit 189a166

Please sign in to comment.