UPSTREAM: <carry>: XFS quota for emptyDir volumes #19533

sjenning · 2018-04-26T21:41:53Z

Moves the XFS quota patch out of origin and into the vendored kube since we use the kubelet directly in 3.10.

The configuration is now /var/lib/origin/openshift.local.volumes/volume-config.yaml and would look like this:

apiVersion: kubelet.config.openshift.io/v1
kind: VolumeConfig
localQuota:
  perFSGroupInGiB: 1

If the file exists, any error in the patching is fatal. If the file does not exist, the patching doesn't occur and PatchVolumePluginsForLocalQuota() return immediately without patching the volume plugins.

xref https://bugzilla.redhat.com/show_bug.cgi?id=1572285

sjenning · 2018-04-27T18:44:30Z

TIL [WIP] is enough to hold
/hold cancel

sjenning · 2018-05-01T21:09:35Z

@smarterclayton @derekwaynecarr @deads2k can I get review on this sooner rather than later? I don't want this to blow up the release or delay 3.10 rollouts on Online.

deads2k · 2018-05-02T12:59:05Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/server_patch.go

@@ -0,0 +1,91 @@
+package app


nit, filename patch_volumequota.go please.

deads2k · 2018-05-02T13:04:10Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/server_patch.go

+// volume plugin will be replaced with a wrapper version which adds quota
+// functionality.
+func PatchVolumePluginsForLocalQuota(rootdir string, plugins *[]volume.VolumePlugin) error {
+	volumeConfigFilePath := rootdir + "volume-config.yaml"


I'm not familiar with the kubelet standards. Is it normal to be this opinionated about filenames? Also, why not path.Join? This isn't on a hot path.

This was to avoid adding a flag. If you don't hardcode the path to this file, you need to get it via some config, which we were looking to avoid.

I can use path.Join

deads2k · 2018-05-02T13:04:32Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/server_patch.go

+)
+
+// VolumeConfig contains options for configuring volumes on the node.
+type VolumeConfig struct {


You'll want a typemeta here for versioning this format.

typemeta. what is this?

sjenning · 2018-05-03T15:58:39Z

@deads2k mind taking another look. Especially wrt to the typemeta thing. Not sure if I'm doing it right.

deads2k · 2018-05-03T16:28:37Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/patch_volumequota.go

+	}
+
+	var volumeConfig VolumeConfig
+	err = yaml.Unmarshal(volumeConfigFile, &volumeConfig)


Make a group and version for this file and require them to be set as you expect. This will allow you to version it later if need be.

VolumeConfig in kubelet.config.openshift.io/v1 comes mind.

sjenning · 2018-05-03T19:03:22Z

@deads2k fixed up again

sjenning · 2018-05-03T21:11:04Z

/retest

sjenning · 2018-05-04T01:59:47Z

/retest

sjenning · 2018-05-04T18:03:59Z

I've tested this now and it works. Note that I changed perFSGroup to perFSGroupInGiB and change type from Quantity to int64. The yaml unmarshaller didn't know what to do with the Quantity type.

sjenning · 2018-05-05T02:18:49Z

/retest

sjenning · 2018-05-07T18:07:50Z

This is ready for review/merge now

deads2k · 2018-05-09T19:23:27Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/patch_volumequota.go

+		},
+	}
+
+	quota := resource.NewQuantity(volumeConfig.LocalQuota.PerFSGroupInGiB*1024*1024*1024, resource.BinarySI)


you may want to check that it's greater than zero.

deads2k · 2018-05-09T19:23:50Z

On disk format looks ok. Parsing code looks reasonable. You might want to add a check for sane values.

/lgtm

derekwaynecarr

it would be good if you can add a test case in a follow-on pr.

derekwaynecarr · 2018-05-14T18:27:23Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/plugins.go

 // ProbeVolumePlugins collects all volume plugins into an easy to use list.
-func probeVolumePlugins() []volume.VolumePlugin {
+func ProbeVolumePlugins() []volume.VolumePlugin {


noting that upstream is already this way...

derekwaynecarr · 2018-05-14T18:29:59Z

vendor/k8s.io/kubernetes/cmd/kubelet/app/patch_volumequota.go

+// filesystem suitable for quota enforcement. If checks pass the k8s emptyDir
+// volume plugin will be replaced with a wrapper version which adds quota
+// functionality.
+func PatchVolumePluginsForLocalQuota(rootdir string, plugins *[]volume.VolumePlugin) error {


can we get a test case for this?

or if not here, something that verifies that the function works moving forward as a follow-on?

derekwaynecarr · 2018-05-14T18:32:02Z

/approve
/lgtm

openshift-ci-robot · 2018-05-14T18:32:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, sjenning

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cmd/OWNERS~~ [derekwaynecarr]
~~test/extended/OWNERS~~ [derekwaynecarr]
~~vendor/k8s.io/kubernetes/cmd/kubelet/OWNERS~~ [derekwaynecarr]
~~vendor/k8s.io/kubernetes/pkg/volume/OWNERS~~ [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sjenning · 2018-05-14T21:43:33Z

/retest

sjenning · 2018-05-14T21:44:43Z

/retest

sjenning · 2018-05-15T03:51:29Z

/retest

sjenning assigned smarterclayton, derekwaynecarr and deads2k Apr 26, 2018

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 26, 2018

openshift-ci-robot requested review from adelton and gnufied April 26, 2018 21:42

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 26, 2018

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2018

deads2k reviewed May 2, 2018

View reviewed changes

sjenning force-pushed the carry-xfs-quota branch from 1022ba9 to 6f49165 Compare May 3, 2018 15:57

deads2k reviewed May 3, 2018

View reviewed changes

sjenning force-pushed the carry-xfs-quota branch from 6f49165 to 5501ec2 Compare May 3, 2018 19:03

sjenning added 3 commits May 4, 2018 13:00

UPSTREAM: <carry>: XFS quota for emptyDir volumes

7331c64

remove XFS quota patch from origin

0d76139

retarget emptydirquota import

7637781

sjenning force-pushed the carry-xfs-quota branch from 5501ec2 to 7637781 Compare May 4, 2018 18:00

sjenning changed the title ~~[WIP] UPSTREAM: <carry>: XFS quota for emptyDir volumes~~ UPSTREAM: <carry>: XFS quota for emptyDir volumes May 4, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2018

deads2k reviewed May 9, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2018

derekwaynecarr approved these changes May 14, 2018

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2018

openshift-merge-robot merged commit ad9b0c1 into openshift:master May 15, 2018

mburke5678 mentioned this pull request May 15, 2018

XFS quota for emptyDir volumes openshift/openshift-docs#9261

Merged

sjenning mentioned this pull request May 18, 2018

UPSTREAM: <carry>: change config file location and restore perFSGroup to Quantity #19773

Merged

sjenning mentioned this pull request Jun 29, 2018

UPSTREAM: <carry>: gracefully handle empty volume-config file #20154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UPSTREAM: <carry>: XFS quota for emptyDir volumes #19533

UPSTREAM: <carry>: XFS quota for emptyDir volumes #19533

sjenning commented Apr 26, 2018 •

edited

Loading

sjenning commented Apr 27, 2018

sjenning commented May 1, 2018

deads2k May 2, 2018

deads2k May 2, 2018

sjenning May 2, 2018

deads2k May 2, 2018

sjenning May 2, 2018

sjenning commented May 3, 2018

deads2k May 3, 2018

deads2k May 3, 2018

sjenning commented May 3, 2018

sjenning commented May 3, 2018

sjenning commented May 4, 2018

sjenning commented May 4, 2018

sjenning commented May 5, 2018

sjenning commented May 7, 2018

deads2k May 9, 2018

deads2k commented May 9, 2018

derekwaynecarr left a comment

derekwaynecarr May 14, 2018

derekwaynecarr May 14, 2018

derekwaynecarr commented May 14, 2018

openshift-ci-robot commented May 14, 2018

sjenning commented May 14, 2018

sjenning commented May 14, 2018

sjenning commented May 15, 2018

UPSTREAM: <carry>: XFS quota for emptyDir volumes #19533

UPSTREAM: <carry>: XFS quota for emptyDir volumes #19533

Conversation

sjenning commented Apr 26, 2018 • edited Loading

sjenning commented Apr 27, 2018

sjenning commented May 1, 2018

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

sjenning commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjenning commented May 3, 2018

sjenning commented May 3, 2018

sjenning commented May 4, 2018

sjenning commented May 4, 2018

sjenning commented May 5, 2018

sjenning commented May 7, 2018

Choose a reason for hiding this comment

deads2k commented May 9, 2018

derekwaynecarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented May 14, 2018

openshift-ci-robot commented May 14, 2018

sjenning commented May 14, 2018

sjenning commented May 14, 2018

sjenning commented May 15, 2018

sjenning commented Apr 26, 2018 •

edited

Loading