Volume Accounting Interface methods #18232

pwittrock · 2015-12-04T21:40:37Z

WIP / RFC for methods to add Volume accounting support.

pwittrock · 2015-12-04T21:40:44Z

@vishh

k8s-github-robot · 2015-12-04T21:47:02Z

Labelling this PR as size/L

k8s-bot · 2015-12-04T22:22:22Z

GCE e2e build/test failed for commit ff684565a889a1d613549c2e7fa52f2241e7dd2e.

vishh · 2015-12-04T22:56:00Z

cc @thockin

vishh · 2015-12-04T23:39:39Z

pkg/volume/volume.go

+const unknownSize = -1
+
+type Accounting struct {
+	BytesUsed int


What about capacity?

Should the units be uint64?

vishh · 2015-12-04T23:58:19Z

Just a few comments. Overall structure LGTM.
ping @saad-ali

k8s-bot · 2015-12-05T00:27:30Z

GCE e2e test build/test passed for commit 05d85465bb1a97c364aae9b9f90405fec1b149b3.

k8s-bot · 2015-12-05T01:10:49Z

GCE e2e test build/test passed for commit 06f32b14a1f7ae88e2311b23e1116aaf574415d2.

thockin · 2015-12-07T23:29:53Z

pkg/volume/empty_dir/empty_dir.go

@@ -103,6 +105,7 @@ func (plugin *emptyDirPlugin) newCleanerInternal(volName string, podUID types.UI
 		mountDetector: mountDetector,
 		plugin:        plugin,
 	}
+	ed.AccountingDu.Init(ed.GetPath())


embedding when you have a really generic name like Init() exposed is bound to cause problems. Maybe InitAccounting()?

k8s-bot · 2015-12-08T20:58:39Z

GCE e2e build/test failed for commit f19f299ba6a4b6b1250823e100d4aab8d12bcf53.

k8s-bot · 2015-12-08T21:09:52Z

GCE e2e test build/test passed for commit 3629ffac03d42a10ec1c55886ce8a770154da029.

pwittrock · 2015-12-08T22:44:29Z

I agree will all of the comments, and believe I have addressed them.
Other changes:

Changed the names of several structs / interfaces / functions to be what I hope are more descriptive. Open to suggestions on what these should be.
Added testing for the Metrics implementations
Changed secret to use MetricsNil until we know that using MetricsDu is the correct thing to do.

thockin · 2015-12-08T23:08:52Z

pkg/volume/empty_dir/empty_dir.go

@@ -133,6 +136,7 @@ type emptyDir struct {
 	mountDetector mountDetector
 	plugin        *emptyDirPlugin
 	rootContext   string
+	volume.MetricsDu


minor idea: rather than embedding MetricsDu and then calling Init(), why not embed MetricsProvider and initialize it in the struct definition as MetricsProvider: volume.NewMetricsDu(path) ?

Done. Performed some refactoring to get the path value without an initialized emptyDir. Also made metricsDu unexported so that it cannot be directly created. Let me know if you prefer unexported metricsDu, or as it was before.

vishh · 2015-12-09T23:06:53Z

pkg/volume/metrics_nil.go

+// See MetricsProvider.GetMetrics
+// GetMetrics returns an empty Metrics and an error.
+func (*MetricsNil) GetMetrics() (*Metrics, error) {
+	return &Metrics{}, errors.New("metrics are not supported for MetricsNil Volumes")


I suspect we might have to expose this error message as a special variable to differentiate between metrics errors vs unsupported scenarios. This code is Ok for this PR though.

Sounds good. The correct behavior may become more clear once these are actually used.

vishh · 2015-12-09T23:11:28Z

@pwittrock: Just a few nits. Otherwise LGTM. This is no longer a WIP :)

googlebot · 2015-12-10T00:06:05Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

- Add volume.MetricsProvider function to Volume interface. - Add volume.MetricsDu for providing metrics via executing "du". - Add volulme.MetricsNil for unsupported Volumes.

googlebot · 2015-12-10T00:06:37Z

CLAs look good, thanks!

pwittrock · 2015-12-10T00:09:03Z

Comments addressed. PTAL. I squashed all of the commits into 1.

pwittrock · 2015-12-10T00:57:34Z

@k8s-bot test this

k8s-bot · 2015-12-10T01:33:59Z

GCE e2e build/test failed for commit c67ce88.

k8s-bot · 2015-12-10T02:13:12Z

GCE e2e test build/test passed for commit c67ce88.

k8s-github-robot · 2015-12-10T19:47:01Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-10T20:21:32Z

GCE e2e test build/test passed for commit c67ce88.

k8s-github-robot · 2015-12-11T08:14:01Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-12-11T08:47:47Z

GCE e2e test build/test passed for commit c67ce88.

k8s-github-robot · 2015-12-11T08:48:09Z

Automatic merge from submit-queue

Auto commit by PR queue bot

rootfs · 2015-12-11T16:39:26Z

pkg/volume/empty_dir/empty_dir_test.go

+	if err != nil {
+		t.Errorf("Unexpected error when calling GetMetrics %v", err)
+	}
+	if metrics.Used.Value() != 4096 {


this is very fs type dependent. In tmpfs, directory size is 0, and thus we'd expect != 4096 is correct. A fs can also have block size more than 4096, see stat(2) . The formula should probably be metrics.Used.Value() == st.st_blksize * st.st_blocks

Auto commit by PR queue bot

pwittrock assigned saad-ali Dec 4, 2015

googlebot added the cla: yes label Dec 4, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 4, 2015

vishh reviewed Dec 4, 2015
View reviewed changes

thockin reviewed Dec 7, 2015
View reviewed changes

thockin reviewed Dec 8, 2015
View reviewed changes

vishh reviewed Dec 9, 2015
View reviewed changes

pwittrock changed the title ~~WIP: Volume Accounting Interface methods~~ Volume Accounting Interface methods Dec 9, 2015

pwittrock force-pushed the fsacct branch from 1fde1d2 to 223bee3 Compare December 10, 2015 00:06

googlebot added cla: no and removed cla: yes labels Dec 10, 2015

Volume Metrics Interface and base implementation.

c67ce88

- Add volume.MetricsProvider function to Volume interface. - Add volume.MetricsDu for providing metrics via executing "du". - Add volulme.MetricsNil for unsupported Volumes.

pwittrock force-pushed the fsacct branch from 223bee3 to c67ce88 Compare December 10, 2015 00:06

googlebot added cla: yes and removed cla: no labels Dec 10, 2015

pwittrock assigned vishh and unassigned saad-ali Dec 10, 2015

vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2015

k8s-github-robot pushed a commit that referenced this pull request Dec 11, 2015

Merge pull request #18232 from pwittrock/fsacct

23baca8

Auto commit by PR queue bot

k8s-github-robot merged commit 23baca8 into kubernetes:master Dec 11, 2015

rootfs reviewed Dec 11, 2015
View reviewed changes

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

Merge pull request kubernetes#18232 from pwittrock/fsacct

c68f066

Auto commit by PR queue bot

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Volume Accounting Interface methods #18232

Volume Accounting Interface methods #18232

pwittrock commented Dec 4, 2015

pwittrock commented Dec 4, 2015

k8s-github-robot commented Dec 4, 2015

k8s-bot commented Dec 4, 2015

vishh commented Dec 4, 2015

vishh Dec 4, 2015

vishh Dec 4, 2015

pwittrock Dec 8, 2015

vishh commented Dec 4, 2015

k8s-bot commented Dec 5, 2015

k8s-bot commented Dec 5, 2015

thockin Dec 7, 2015

pwittrock Dec 8, 2015

k8s-bot commented Dec 8, 2015

k8s-bot commented Dec 8, 2015

pwittrock commented Dec 8, 2015

thockin Dec 8, 2015

vishh Dec 8, 2015

pwittrock Dec 9, 2015

vishh Dec 9, 2015

pwittrock Dec 9, 2015

vishh commented Dec 9, 2015

googlebot commented Dec 10, 2015

googlebot commented Dec 10, 2015

pwittrock commented Dec 10, 2015

pwittrock commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

rootfs Dec 11, 2015

Volume Accounting Interface methods #18232

Volume Accounting Interface methods #18232

Conversation

pwittrock commented Dec 4, 2015

pwittrock commented Dec 4, 2015

k8s-github-robot commented Dec 4, 2015

k8s-bot commented Dec 4, 2015

vishh commented Dec 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishh commented Dec 4, 2015

k8s-bot commented Dec 5, 2015

k8s-bot commented Dec 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Dec 8, 2015

k8s-bot commented Dec 8, 2015

pwittrock commented Dec 8, 2015

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

vishh commented Dec 9, 2015

googlebot commented Dec 10, 2015

googlebot commented Dec 10, 2015

pwittrock commented Dec 10, 2015

pwittrock commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 10, 2015

k8s-bot commented Dec 10, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

Choose a reason for hiding this comment