Add beginnings of quota support for emptyDir on XFS. by dgoodwin · Pull Request #7351 · openshift/origin

dgoodwin · 2016-02-16T18:07:24Z

Wraps the k8s empty_dir volume plugin in one which layers on code to apply
quotas if the volume dir is on XFS, mounted with gquota, and an FSGroup is
available for the pod.

This is likely a stepping stone to a more permanent solution in the future.

More details on the research into this proposed approach are available here: #7206

This attempt makes an effort to make no kubernetes modifications we need to carry. Instead the goal was to accomplish this purely in origin and look to push to something upstream in time.

Alternatively we could just modify the empty_dir plugin in kubernetes and carry the patches for time being.

dgoodwin · 2016-02-16T18:08:20Z

Likely of interest to @ironcladlou and @smarterclayton

Still some debugging code in there as I'm working on a couple things still, the node-config.yaml setting doesn't seem to be getting read for one.

ironcladlou · 2016-02-16T18:12:39Z

Still some debugging code in there as I'm working on a couple things still, the node-config.yaml setting doesn't seem to be getting read for one.

You added the field to the internal types, but you also need it to the public versioned types (e.g. v1), and then regenerate conversions, etc.

ironcladlou · 2016-02-16T18:18:07Z

pkg/volume/empty_dir/quota.go

+		return nil
+	}
+	if !xqa.isXfs(dir) {
+		glog.V(3).Infof("%s is not on an XFS filesystem, skipping quota application.")


Seems like maybe this needs to be smarter, otherwise emptyDir on a misconfigured node would allow all pods to use unrestricted storage. I would expect failure to apply quota to propagate back up as a failure to establish the volume for the pod (except if the pod is somehow special and allowed to bypass quota application).

Hmm yes I suppose so, originally I was thinking it's a plugin that's in play regardless, but if there's an explicit config option to turn this on then failure to apply the quota does seem pretty bad. I'll rig this up, I think I'll skip wrapping the plugin entirely unless you have something in your config file for it, and then treat it like a proper error internally.

Quota right now is going to be admin policy applied to the node. If the policy is enabled, failure to apply the policy should definitely be an error. When it moves to the pod, if the node can't satisfy the policy, that should also be an error.

dgoodwin · 2016-02-22T19:21:21Z

Ok I think this is ready for review, dropping the WIP tag. I will squash commits once closer to merge but leaving them separate for now in case reverts are needed. Since original PR was opened:

fixed the reading of projectEmptyDirQuota from node-config.xml.
using an int * for the NodeConfig API type to support the value not being set, and yet allow the use of 0 effectively shutting down emptyDir usage.
failure to apply the quota is now treated as an error
use df --output=source for more reliable lookup of the device for a dir
added a suite of tests for quota.go.
uppercased some abbreviations
added tests for loading node-config.xml files with various projectEmptyDirQuota settings, or none at all.
added validation code to prevent negative projectEmptyDirQuota settings, and associated tests.

Is it feasible to think about end to end integration tests here? It would require the runtime environment to be on XFS with group quota enabled and tests running as root. If so any advice on a starting point (how to make sure this is the case, or who to contact) would be appreciated.

ironcladlou · 2016-02-22T19:38:07Z

pkg/cmd/server/api/validation/node_test.go

+	if emptyDirQuotaError == nil {
+		t.Fatalf("expected projectEmptyDirQuota error but got none")
+	}
+	if !strings.Contains("must be a positive integer", emptyDirQuotaError.Detail) {


Could also check for errors.IsInvalid

dgoodwin · 2016-02-23T14:59:44Z

Thanks for the quick review @ironcladlou, I think I've got everything updated now.

ironcladlou · 2016-02-23T18:33:35Z

This looks really close to me; let's find a second reviewer.

dgoodwin · 2016-02-23T19:03:09Z

Awesome. I will squash it down to one commit now.

deads2k · 2016-02-24T15:38:04Z

pkg/cmd/server/api/types.go

+	// ProjectEmptyDirQuota is the quota applied to emptyDir volumes specified in MB.
+	// This is currently implemented only for XFS, and is a limit per namespace per node.
+	// (this may change in the future)
+	ProjectEmptyDirQuota *int


I haven't followed this, but why would I want a different way to express resource.Quantity?

@deads2k apologies I'm not quite familiar with that, could you (or anyone else) fill me in on what that's a ref to?

@deads2k apologies I'm not quite familiar with that, could you (or anyone else) fill me in on what that's a ref to?

When specifying limits for things that take MB or other standard units in the API, we've used this type: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/resource/quantity.go#L91. I would expect this configuration file to be consistent in its types unless there is a strong reason to deviate.

Great idea I will see if I can make it work.

dgoodwin · 2016-02-25T15:39:59Z

Ok updated to use resource.Quantity throughout and added some more testing. Had to register the conversion func in the server/api/v1/conversions.go.

ironcladlou · 2016-02-25T15:43:18Z

pkg/cmd/server/start/node_args.go


+	// TODO: Remove this:
+	tempDefaultQuota := resource.MustParse("1024Gi")
+	internal.(*configapi.NodeConfig).ProjectEmptyDirQuota = &tempDefaultQuota


Did you intend to commit this?

Nope my bad. Fixed now.

ironcladlou · 2016-02-25T16:32:11Z

Refactored API LGTM, @deads2k, want to give it another look?

deads2k · 2016-02-25T17:53:55Z

I haven't looked at the implementation just the api. Did @liggitt approve adding the top level element to the config? That looks off to my eye.

smarterclayton · 2016-03-14T16:13:58Z

Can you run the extended as part of this PR yet (via test extended)?

dgoodwin · 2016-03-14T16:18:03Z

If I'm following your question, yes it should be included in a default extended test suite run as part of this PR: https://github.com/dgoodwin/origin/blob/quota-emptydir/test/extended/extended_test.go#L12

It will fail if you don't have proper mount point setup, which our images should have at this point.

smarterclayton · 2016-03-14T17:36:42Z

Looks like it failed

On Mon, Mar 14, 2016 at 1:35 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/testonlyextended FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2086/)
(Extended Tests: core(local storage quota))

—
Reply to this email directly or view it on GitHub
#7351 (comment).

dgoodwin · 2016-03-15T12:37:11Z

Trying to figure out what to do here, we setup the base images to mount an XFS filesystem to /tmp/openshift/xfs-vol-dir, but it would appear something is later coming along and mounting /tmp as a tmpfs, which looks like it's effectively hiding the /tmp/openshift/xfs-vol-dir mount point and preventing the test from being able to use it and pass.

smarterclayton · 2016-03-15T14:54:42Z

@dgoodwin, that's something we have to do on the test machines. @deads2k
we may have a problem here where we have to change the ordering of the /tmp
mount.

On Tue, Mar 15, 2016 at 9:15 AM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2131/)
(Extended Tests: core(local storage quota))

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#7351 (comment)

dgoodwin · 2016-03-15T14:56:27Z

@smarterclayton I think I found a good route past it, I've modified the images to mount to /mnt/openshift-xfs-vol-dir instead, and I have an update to the test here locally that I'll push as soon as the images are ready. This avoids any problems with tmp and it can continue to be mounted as it is today.

I'll push the change as soon as they images are done building, that is underway as we speak.

dgoodwin · 2016-03-16T13:00:19Z

This latest failure is looking more like a legit test failure and getting further now that the images have all been rebuilt, it's possible it's because I only gave it 30 seconds for the pod to create and quota be applied. Adding more debug info and increasing timeout for another try.

dgoodwin · 2016-03-16T16:10:44Z

Extended test is now passing, there was a bug in the test logic if run against a fresh XFS partition, most of my work here was reusing a partition due to the complications with "deleting" an XFS quota.

Should be ready for merge.

smarterclayton · 2016-03-16T16:34:34Z

Please squash and remove all debugging logic.

Introduces a volume plugin which simply wraps the k8s emptyDir volume plugin, and layers in functionality to apply quotas to an XFS filesystem. This approach allows us to avoid carrying a patch to k8s to accomplish the goal. Composition is used over inheritance to ensure we don't suddenly no longer implement the correct interface after a k8s rebase. The behavior is triggered via a new optional setting in node-config.yaml: volumeConfig: localQuota: perFSGroup: 512Mi The implementation will use FSGroup as the gid for the quota, if no FSGroup is specified (indicating the request matched an SCC with RunAsAny) the quota application is quietly skipped. Errors will be thrown if the config option is specified, but the volume dir does not reside on an XFS filesytem, or it is mounted without gquota option. XFS cannot remove a quota without unmounting the filesystem and re-creating all other quotas, so unused quotas are left dangling. Should the same FSGroup ever appear on the node the quota will be re-initialized to whatever it should be at that point in time.

dgoodwin · 2016-03-16T17:14:34Z

Apologies forgot all about the debug commits. Updated now.

openshift-bot · 2016-03-16T17:15:49Z

Evaluated for origin testonlyextended up to 1ae5b25

openshift-bot · 2016-03-16T17:46:44Z

continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2207/) (Extended Tests: core(local storage quota))

dgoodwin · 2016-03-16T19:11:48Z

All green. Hoping to make Friday's build if possible!

smarterclayton · 2016-03-16T19:29:45Z

[merge]

On Wed, Mar 16, 2016 at 2:45 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2206/)
(Extended Tests: core(local storage quota))

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7351 (comment)

openshift-bot · 2016-03-16T19:30:40Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2226/) (Image: devenv-rhel7_3750)

openshift-bot · 2016-03-16T19:30:41Z

Evaluated for origin merge up to 1ae5b25

openshift-bot · 2016-03-16T19:30:42Z

[Test]ing while waiting on the merge queue

dgoodwin · 2016-03-17T01:46:25Z

Flake, re[test] please.

openshift-bot · 2016-03-17T01:55:29Z

Evaluated for origin test up to 1ae5b25

openshift-bot · 2016-03-17T03:24:26Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2226/)

Merged by openshift-bot

ironcladlou · 2016-03-17T18:00:24Z

😂

ironcladlou reviewed Feb 16, 2016
View reviewed changes

dgoodwin force-pushed the quota-emptydir branch from a7b4a77 to c8c6bbb Compare February 17, 2016 17:20

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2016

dgoodwin force-pushed the quota-emptydir branch from c8c6bbb to 445738a Compare February 18, 2016 12:40

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2016

dgoodwin changed the title ~~WIP: Add beginnings of quota support for emptyDir on XFS.~~ Add beginnings of quota support for emptyDir on XFS. Feb 22, 2016

ironcladlou reviewed Feb 22, 2016
View reviewed changes

dgoodwin force-pushed the quota-emptydir branch from 66a920b to e188af3 Compare February 23, 2016 19:58

deads2k reviewed Feb 24, 2016
View reviewed changes

dgoodwin force-pushed the quota-emptydir branch 2 times, most recently from ce88df7 to ab7de1f Compare February 25, 2016 15:38

ironcladlou reviewed Feb 25, 2016
View reviewed changes

dgoodwin force-pushed the quota-emptydir branch from ab7de1f to e0947d9 Compare February 25, 2016 16:29

dgoodwin force-pushed the quota-emptydir branch from e7c7071 to 8d69180 Compare March 15, 2016 18:56

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2016

dgoodwin force-pushed the quota-emptydir branch from 8d69180 to 2ab2b55 Compare March 16, 2016 12:10

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2016

dgoodwin force-pushed the quota-emptydir branch from 58f6b27 to 1ae5b25 Compare March 16, 2016 17:14

openshift-bot pushed a commit that referenced this pull request Mar 17, 2016

Merge pull request #7351 from dgoodwin/quota-emptydir

e4129ea

Merged by openshift-bot

openshift-bot merged commit e4129ea into openshift:master Mar 17, 2016

dgoodwin mentioned this pull request Mar 17, 2016

Document perFSGroup quota node config. openshift/openshift-docs#1758

Merged

Comments

Conversation

dgoodwin commented Feb 16, 2016

Uh oh!

dgoodwin commented Feb 16, 2016

Uh oh!

ironcladlou commented Feb 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Feb 17, 2016 via email

Choose a reason for hiding this comment

Uh oh!

dgoodwin commented Feb 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgoodwin commented Feb 23, 2016

Uh oh!

ironcladlou commented Feb 23, 2016

Uh oh!

dgoodwin commented Feb 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgoodwin commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou commented Feb 25, 2016

Uh oh!

deads2k commented Feb 25, 2016

Uh oh!

smarterclayton commented Mar 14, 2016

Uh oh!

dgoodwin commented Mar 14, 2016

Uh oh!

smarterclayton commented Mar 14, 2016

Uh oh!

dgoodwin commented Mar 15, 2016

Uh oh!

smarterclayton commented Mar 15, 2016

Uh oh!

dgoodwin commented Mar 15, 2016

Uh oh!

dgoodwin commented Mar 16, 2016

Uh oh!

dgoodwin commented Mar 16, 2016

Uh oh!

smarterclayton commented Mar 16, 2016

Uh oh!

dgoodwin commented Mar 16, 2016

Uh oh!

openshift-bot commented Mar 16, 2016

Uh oh!

openshift-bot commented Mar 16, 2016

Uh oh!

dgoodwin commented Mar 16, 2016

Uh oh!

smarterclayton commented Mar 16, 2016

Uh oh!

openshift-bot commented Mar 16, 2016

Uh oh!

openshift-bot commented Mar 16, 2016

Uh oh!

openshift-bot commented Mar 16, 2016

Uh oh!

dgoodwin commented Mar 17, 2016

Uh oh!

openshift-bot commented Mar 17, 2016