Detect local volume capacity #135

ddysher · 2017-05-24T13:18:32Z

Detect local volume capacity, a couple of implementation notes:

to correctly detect host devices, daemonset is changed to run in privileged mode
after investigating cadvisor, i think we can just detect capacity using statfs, instead of collecting all information from device name, device number, mountpoint, etc; we don't need that much data
statfs returns 'total, free, available'. We use 'total - free + available' as pv capacity
if user just create regular directory, then the available space for the mount device is returned, in which case the behavior is similar to hostpath

Future work

zfs and devicemapper might require special handling
hostpath mount option should be rshared

@msau42 @jsafrane @humblec @wongma7

msau42

Can you also add/extend a unit test to check that the created PV object has the correct capacity specified?

msau42 · 2017-05-30T20:43:51Z

local-volume/provisioner/pkg/discovery/discovery.go

-			// TODO: detect capacity
-			d.createPV(file, relativePath, class)
+
+			avail, err := d.VolUtil.GetFsAvailable(filePath)


Can you call this GetFsAvailable<Unit> to be more explicit? Where unit is bytes, mb, gb, etc.

msau42 · 2017-05-30T20:46:28Z

local-volume/provisioner/pkg/discovery/discovery.go

@@ -85,11 +90,12 @@ func generatePVName(file, node, class string) string {
 	return fmt.Sprintf("%v-%v-%v", class, node, file)
 }

-func (d *Discoverer) createPV(file, relativePath, class string) {
+func (d *Discoverer) createPV(file, relativePath, class string, avail uint64) {


Also add the unit to the variable name

msau42 · 2017-05-30T20:47:15Z

local-volume/provisioner/pkg/discovery/discovery.go

-				// TODO: detect capacity
-				v1.ResourceName(v1.ResourceStorage): resource.MustParse("10Gi"),
+				v1.ResourceName(v1.ResourceStorage): *resource.NewQuantity(int64(avail), resource.BinarySI),
+				// v1.ResourceName(v1.ResourceStorage): resource.MustParse("10Gi"),


This line can be removed

msau42 · 2017-05-30T21:16:33Z

local-volume/provisioner/pkg/util/volume_util.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"syscall"


I think this pkg is deprecated, and we're supposed to use https://godoc.org/golang.org/x/sys/unix instead.

msau42 · 2017-05-30T21:31:21Z

local-volume/provisioner/pkg/util/volume_util.go

+	if err := syscall.Statfs(fullPath, &s); err != nil {
+		return 0, err
+	}
+	return uint64(s.Frsize) * s.Bavail, nil


What's the difference between frsize and bsize?

Do we want to use total blocks instead of available? I'm thinking of a situation where they might want pre-populated data in the volume.

Also in the shared directory case, we could get inconsistent capacities depending on the order that pods fill them up (although the capacity is already going to be inaccurate in that case).

I found this and this which explain the concept pretty well (the concept applies to most fs).

What is not quite clear is how this was handled in all sorts of filesystem; but it looks like the values are usually the same (the option to create different fragment size is actually removes the option from mke2fs, though man page still has the option).

Using Frsize here is mainly based on cadvisor https://github.com/google/cadvisor/blob/aee77359083ccef523d0299316e831f1aed712bd/fs/fs.go#L530

Also, df uses frsize as welll (fall back to bsize if it is not available): https://github.com/coreutils/gnulib/blob/2a0c08e48834ae056964922ee6d21ccd223d1d80/lib/fsusage.c#L131

@msau42 I'm ok with using total blocks for reasons listed in ur comment (pre-populated data and consistent capacity). My concern is that the total block is generally not the capacity user can actually use, e.g. for ext4, there's 5% system reserved space.

Hm is there a field that can distinguish between system reserved, and user used?

There is no such a field that i know of. Looking further into df, I think what we can do is to use 'used + available'.

related logic from df:

bv->total = fsu->fsu_blocks; bv->available_to_root = fsu->fsu_bfree; bv->used = bv->total - fsu->fsu_bfree; uintmax_t nonroot_total = v->used + v->available;

so simply put, we can return s.Blocks - s.Bfree + s.Bavail. This will return total capacity for non root user, with system reserved removed altogether.

WDYT

ddysher · 2017-06-02T07:28:58Z

@msau42 comment addressed

ddysher · 2017-06-02T09:24:10Z

Also, to correctly detect capacity for mountpoint after provisioner has already started, we need mount propagation for hostpath
https://github.com/kubernetes/community/blob/master/contributors/design-proposals/propagation.md

msau42

just one minor comment, lgtm!

msau42 · 2017-06-06T00:24:10Z

local-volume/provisioner/pkg/util/volume_util.go

-	var s syscall.Statfs_t
-	if err := syscall.Statfs(fullPath, &s); err != nil {
+// GetFsAvailableByte returns available capacity in byte about a mounted filesystem.
+// fullPath is the pathname of any file within the mounted filesystem.


Can you also add in this comment that this does not include fs reserved space?

msau42 · 2017-06-06T00:39:52Z

Remind me to add the limitation about mount propagation in the docs.

msau42 · 2017-06-06T00:40:23Z

@jsafrane do you know the status for the mount propagation?

ddysher · 2017-06-07T01:21:19Z

@msau42

talked to @jsafrane in another issue, this missed 1.7

kubernetes/community#659

ddysher · 2017-06-10T02:29:27Z

@msau42 rebase to HEAD, PTAL

msau42 · 2017-06-12T18:15:42Z

/lgtm

Detect local volume capacity

Add Block volume support for CSI provisioner

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2017

msau42 reviewed May 30, 2017

View reviewed changes

ddysher force-pushed the detect-capacity branch 2 times, most recently from f4e9811 to c7fedb2 Compare June 2, 2017 07:25

ddysher changed the title ~~WIP: detect local volume capacity~~ Detect local volume capacity Jun 2, 2017

msau42 reviewed Jun 6, 2017

View reviewed changes

ddysher mentioned this pull request Jun 7, 2017

[local-volume] Detect capacity of mount points #132

Closed

ddysher force-pushed the detect-capacity branch 3 times, most recently from 1b922ca to dfadbd1 Compare June 10, 2017 02:25

Detect local volume capacity

a4b609a

ddysher force-pushed the detect-capacity branch from dfadbd1 to a4b609a Compare June 10, 2017 02:27

ddysher mentioned this pull request Jun 10, 2017

Add test to verify storageclass #154

Merged

wongma7 merged commit a3a14e5 into kubernetes-retired:master Jun 12, 2017

ianchakeres mentioned this pull request Jul 10, 2017

[local-volume] Frsize not supported on mac osx (darwin) - unable to run local-volume make test #224

Closed

cofyc pushed a commit to cofyc/external-storage that referenced this pull request Dec 17, 2018

Merge pull request kubernetes-retired#135 from ddysher/detect-capacity

1eff95f

Detect local volume capacity

yangruiray pushed a commit to yangruiray/external-storage that referenced this pull request Jul 19, 2019

Merge pull request kubernetes-retired#135 from mkimuram/issue/110v3

3827f80

Add Block volume support for CSI provisioner

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect local volume capacity #135

Detect local volume capacity #135

ddysher commented May 24, 2017 •

edited

Loading

msau42 left a comment

msau42 May 30, 2017

msau42 May 30, 2017

msau42 May 30, 2017

msau42 May 30, 2017

msau42 May 30, 2017

ddysher May 31, 2017 •

edited

Loading

ddysher Jun 2, 2017

msau42 Jun 2, 2017

ddysher Jun 2, 2017 •

edited

Loading

ddysher commented Jun 2, 2017

ddysher commented Jun 2, 2017 •

edited

Loading

msau42 left a comment

msau42 Jun 6, 2017

msau42 commented Jun 6, 2017

msau42 commented Jun 6, 2017

ddysher commented Jun 7, 2017 •

edited

Loading

ddysher commented Jun 10, 2017

msau42 commented Jun 12, 2017

Detect local volume capacity #135

Detect local volume capacity #135

Conversation

ddysher commented May 24, 2017 • edited Loading

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddysher Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

ddysher commented Jun 2, 2017

ddysher commented Jun 2, 2017 • edited Loading

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msau42 commented Jun 6, 2017

msau42 commented Jun 6, 2017

ddysher commented Jun 7, 2017 • edited Loading

ddysher commented Jun 10, 2017

msau42 commented Jun 12, 2017

ddysher commented May 24, 2017 •

edited

Loading

ddysher May 31, 2017 •

edited

Loading

ddysher Jun 2, 2017 •

edited

Loading

ddysher commented Jun 2, 2017 •

edited

Loading

ddysher commented Jun 7, 2017 •

edited

Loading