Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix assumptions about tmpfs in metrics du tests #18580

Merged
merged 1 commit into from
Dec 14, 2015
Merged

Fix assumptions about tmpfs in metrics du tests #18580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions pkg/volume/empty_dir/empty_dir_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,18 @@ func TestMetrics(t *testing.T) {
// Need to create the subdirectory
os.MkdirAll(builder.GetPath(), 0755)

expectedEmptyDirUsage, err := volume.FindEmptyDirectoryUsageOnTmpfs()
if err != nil {
t.Errorf("Unexpected error finding expected empty directory usage on tmpfs: %v", err)
}

// TODO(pwittroc): Move this into a reusable testing utility
metrics, err := builder.GetMetrics()
if err != nil {
t.Errorf("Unexpected error when calling GetMetrics %v", err)
}
if metrics.Used.Value() != 4096 {
t.Errorf("Expected Used %d to be 4096", metrics.Used.Value())
if e, a := expectedEmptyDirUsage.Value(), metrics.Used.Value(); e != a {
t.Errorf("Unexpected value for empty directory; expected %v, got %v", e, a)
}
if metrics.Capacity.Value() <= 0 {
t.Errorf("Expected Capacity to be greater than 0")
Expand Down
10 changes: 7 additions & 3 deletions pkg/volume/host_path/host_path_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,17 @@ func TestMetrics(t *testing.T) {
t.Errorf("Failed to make a new Builder: %v", err)
}

// TODO(pwittroc): Move this into a reusable testing utility
expectedEmptyDirUsage, err := volume.FindEmptyDirectoryUsageOnTmpfs()
if err != nil {
t.Errorf("Unexpected error finding expected empty directory usage on tmpfs: %v", err)
}

metrics, err := builder.GetMetrics()
if err != nil {
t.Errorf("Unexpected error when calling GetMetrics %v", err)
}
if metrics.Used.Value() != 4096 {
t.Errorf("Expected Used %d to be 4096", metrics.Used)
if e, a := expectedEmptyDirUsage.Value(), metrics.Used.Value(); e != a {
t.Errorf("Unexpected value for empty directory; expected %v, got %v", e, a)
}
if metrics.Capacity.Value() <= 0 {
t.Errorf("Expected Capacity to be greater than 0")
Expand Down
23 changes: 15 additions & 8 deletions pkg/volume/metrics_du_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
"testing"
)

const expectedBlockSize = 4096
Copy link
Contributor

Choose a reason for hiding this comment

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

while VERY usually correct, not all blocks are 4k. should the test get the block size from stat info and just accept either 0 or 1 blocks in size? maybe that doesn't help either...

Copy link
Contributor

Choose a reason for hiding this comment

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

See if my comment can help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @rootfs, I'll work this in.


// TestMetricsDuGetCapacity tests that MetricsDu can read disk usage
// for path
func TestMetricsDuGetCapacity(t *testing.T) {
Expand All @@ -33,21 +35,26 @@ func TestMetricsDuGetCapacity(t *testing.T) {
defer os.RemoveAll(tmpDir)
metrics := NewMetricsDu(tmpDir)

expectedEmptyDirUsage, err := FindEmptyDirectoryUsageOnTmpfs()
if err != nil {
t.Errorf("Unexpected error finding expected empty directory usage on tmpfs: %v", err)
}

actual, err := metrics.GetMetrics()
if err != nil {
t.Errorf("Unexpected error when calling GetMetrics %v", err)
}
if actual.Used.Value() != 4096 {
t.Errorf("Expected Used %d for empty directory to be 4096.", actual.Used.Value())
if e, a := expectedEmptyDirUsage.Value(), actual.Used.Value(); e != a {
t.Errorf("Unexpected value for empty directory; expected %v, got %v", e, a)
}

// TODO(pwittroc): Figure out a way to test these values for correctness, maybe by formatting and mounting a file
// as a filesystem
if actual.Capacity.Value() <= 0 {
t.Errorf("Expected Capacity %d to be greater than 0.", actual.Capacity.Value())
if a := actual.Capacity.Value(); a <= 0 {
t.Errorf("Expected Capacity %d to be greater than 0.", a)
}
if actual.Available.Value() <= 0 {
t.Errorf("Expected Available %d to be greater than 0.", actual.Available.Value())
if a := actual.Available.Value(); a <= 0 {
t.Errorf("Expected Available %d to be greater than 0.", a)
}

// Write a file and expect Used to increase
Expand All @@ -56,8 +63,8 @@ func TestMetricsDuGetCapacity(t *testing.T) {
if err != nil {
t.Errorf("Unexpected error when calling GetMetrics %v", err)
}
if actual.Used.Value() != 8192 {
t.Errorf("Unexpected Used for directory with file. Expected 8192, was %d.", actual.Used.Value())
if e, a := (expectedEmptyDirUsage.Value() + expectedBlockSize), actual.Used.Value(); e != a {
t.Errorf("Unexpected Used for directory with file. Expected %v, got %d.", e, a)
}
}

Expand Down
23 changes: 23 additions & 0 deletions pkg/volume/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ package volume

import (
"fmt"
"io/ioutil"
"os"
"os/exec"
"path"
"strings"

"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/resource"
client "k8s.io/kubernetes/pkg/client/unversioned"
"k8s.io/kubernetes/pkg/cloudprovider"
"k8s.io/kubernetes/pkg/types"
Expand Down Expand Up @@ -267,3 +271,22 @@ func (fc *FakeProvisioner) NewPersistentVolumeTemplate() (*api.PersistentVolume,
func (fc *FakeProvisioner) Provision(pv *api.PersistentVolume) error {
return nil
}

// FindEmptyDirectoryUsageOnTmpfs finds the expected usage of an empty directory existing on
// a tmpfs filesystem on this system.
func FindEmptyDirectoryUsageOnTmpfs() (*resource.Quantity, error) {
tmpDir, err := ioutil.TempDir(os.TempDir(), "metrics_du_test")
Copy link
Contributor

Choose a reason for hiding this comment

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

what makes you think this is on tmpfs?

Copy link
Member Author

Choose a reason for hiding this comment

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

os.TempDir() gives you /tmp on linux...

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose you're right though, it's really expected usage of an empty temporary directory however that might be defined on your system.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this method is not working around a tmpfs issue, but rather a GCE ext4 issue. Should we rename the method appropriately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll rename the method to be what it represents. The issue isn't with any filesystem, really, it's with the assumptions baked into the test.

if err != nil {
return nil, err
}
out, err := exec.Command("nice", "-n", "19", "du", "-s", "-B", "1", tmpDir).CombinedOutput()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse existing du code? And why is the usage not 0 to begin with?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vissh under the debian flavor in GCE, directories in /tmp report 4096 under du. :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think /tmp is a tmpfs on GCE. But it does report 4096 as usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vishh you're right:

pmorie@du-test:~$ mount
sysfs on /sys type sysfs (rw,nosuid,nodev,noexec,relatime)
proc on /proc type proc (rw,nosuid,nodev,noexec,relatime)
udev on /dev type devtmpfs (rw,relatime,size=10240k,nr_inodes=472940,mode=755)
devpts on /dev/pts type devpts (rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000)
tmpfs on /run type tmpfs (rw,nosuid,relatime,size=760208k,mode=755)
/dev/sda1 on / type ext4 (rw,relatime,data=ordered)
securityfs on /sys/kernel/security type securityfs (rw,nosuid,nodev,noexec,relatime)
tmpfs on /dev/shm type tmpfs (rw,nosuid,nodev)
tmpfs on /run/lock type tmpfs (rw,nosuid,nodev,noexec,relatime,size=5120k)
tmpfs on /sys/fs/cgroup type tmpfs (ro,nosuid,nodev,noexec,mode=755)
cgroup on /sys/fs/cgroup/systemd type cgroup (rw,nosuid,nodev,noexec,relatime,xattr,release_agent=/lib/systemd/s
ystemd-cgroups-agent,name=systemd)
pstore on /sys/fs/pstore type pstore (rw,nosuid,nodev,noexec,relatime)
cgroup on /sys/fs/cgroup/cpuset type cgroup (rw,nosuid,nodev,noexec,relatime,cpuset)
cgroup on /sys/fs/cgroup/cpu,cpuacct type cgroup (rw,nosuid,nodev,noexec,relatime,cpu,cpuacct)
cgroup on /sys/fs/cgroup/devices type cgroup (rw,nosuid,nodev,noexec,relatime,devices)
cgroup on /sys/fs/cgroup/freezer type cgroup (rw,nosuid,nodev,noexec,relatime,freezer)
cgroup on /sys/fs/cgroup/net_cls,net_prio type cgroup (rw,nosuid,nodev,noexec,relatime,net_cls,net_prio)
cgroup on /sys/fs/cgroup/blkio type cgroup (rw,nosuid,nodev,noexec,relatime,blkio)
cgroup on /sys/fs/cgroup/perf_event type cgroup (rw,nosuid,nodev,noexec,relatime,perf_event)
systemd-1 on /proc/sys/fs/binfmt_misc type autofs (rw,relatime,fd=23,pgrp=1,timeout=300,minproto=5,maxproto=5,di
rect)
debugfs on /sys/kernel/debug type debugfs (rw,relatime)
mqueue on /dev/mqueue type mqueue (rw,relatime)
hugetlbfs on /dev/hugepages type hugetlbfs (rw,relatime)

if err != nil {
return nil, fmt.Errorf("failed command 'du' on %s with error %v", tmpDir, err)
}
used, err := resource.ParseQuantity(strings.Fields(string(out))[0])
if err != nil {
return nil, fmt.Errorf("failed to parse 'du' output %s due to error %v", out, err)
}
used.Format = resource.BinarySI
return used, nil
}