Change SizeLimit to a pointer #50163

jingxu97 · 2017-08-04T18:54:24Z

This PR fixes #50121

The `emptyDir.sizeLimit` field is now correctly omitted from API requests and responses when unset.

liggitt · 2017-08-04T20:10:06Z

pkg/api/validation/validation.go

@@ -383,8 +383,7 @@ func validateVolumeSource(source *api.VolumeSource, fldPath *field.Path) field.E
 	if source.EmptyDir != nil {
 		numVolumes++
 		if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
-			unsetSizeLimit := resource.Quantity{}
-			if unsetSizeLimit.Cmp(source.EmptyDir.SizeLimit) != 0 {
+			if source.EmptyDir.SizeLimit != nil {


1.7 clients will send "0" and we could have 1.7 data stored as 0... need to tolerate that

I added a check. Will this work for the case of data stored as 0? Thanks!

Can you add a unit test for that?

jingxu97 · 2017-08-07T21:49:46Z

@liggitt I updated the PR, PTAL. Thanks!

jingxu97 · 2017-08-09T20:05:06Z

@liggitt I addressed your comment. Could you please help review it again? Thanks!

kyessenov · 2017-08-16T20:55:25Z

Is there a date when this gets merged?

liggitt · 2017-08-07T22:51:38Z

pkg/api/validation/validation.go

@@ -383,8 +383,7 @@ func validateVolumeSource(source *api.VolumeSource, fldPath *field.Path) field.E
 	if source.EmptyDir != nil {
 		numVolumes++
 		if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
-			unsetSizeLimit := resource.Quantity{}
-			if unsetSizeLimit.Cmp(source.EmptyDir.SizeLimit) != 0 {
+			if source.EmptyDir.SizeLimit != nil && source.EmptyDir.SizeLimit.Cmp(resource.Quantity{}) > 0 {


Previously, this checked != 0, not > 0

Is < 0 even valid? I'm not seeing that forbidden by validation… but I'd expect < 0 to be forbidden even when the feature gate is enabled

I am thinking I only need to check sizeLimit != nil. No matter >0, <0 or =0 should not be allowed

1.7 clients (and existing data) could have 0, which you now have to tolerate.

liggitt · 2017-08-07T22:55:07Z

plugin/pkg/scheduler/algorithm/predicates/predicates.go

@@ -507,7 +507,7 @@ func GetResourceRequest(pod *v1.Pod) *schedulercache.Resource {
 	// Account for storage requested by emptydir volumes
 	// If the storage medium is memory, should exclude the size
 	for _, vol := range pod.Spec.Volumes {


For example, a negative value here would mess up this calculation

liggitt · 2017-08-18T16:09:20Z

one question on the zero compare (and associated validation), needs rebase, then LGTM

jingxu97 · 2017-08-29T23:38:43Z

/test pull-kubernetes-e2e-gce-bazel

liggitt · 2017-08-30T00:53:02Z

/retest

liggitt · 2017-08-30T00:53:45Z

looks like a new usage cropped up:

W0829 00:13:59.563] 2017/08/29 00:13:59 error running compiler: exit status 1 W0829 00:13:59.563] Use --strategy=GoCompile=standalone to disable sandboxing for the failing actions. I0829 00:13:59.664] test/e2e_node/local_storage_isolation_eviction_test.go:221: cannot use *resource.NewQuantity(int64(100000), resource.BinarySI) (type resource.Quantity) as type *resource.Quantity in field value

jingxu97 · 2017-08-30T05:36:34Z

@liggitt I already changed the code in test/e2e_node/local_storage_isolation_eviction_test.go:221: to use pointer, could not figure out why it has the compile error..

This PR fixes issue kubernetes#50121

generated files

liggitt · 2017-08-30T20:34:44Z

/lgtm

k8s-github-robot · 2017-08-30T20:35:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jingxu97, liggitt, timothysc, vishh

Associated issue: 50121

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/api/OWNERS~~ [liggitt]
~~pkg/kubelet/eviction/OWNERS~~ [vishh]
~~plugin/pkg/scheduler/OWNERS~~ [timothysc]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/e2e_node/OWNERS~~ [liggitt,vishh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

jingxu97 · 2017-08-31T05:47:31Z

/retest

fejta-bot · 2017-08-31T10:48:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-08-31T16:45:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-31T22:51:05Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2017-09-01T01:01:53Z

@jingxu97: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	a5cada9383abeafc0c3ad764598b3116b1234768	link	`/test pull-kubernetes-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

liggitt · 2017-09-01T01:03:04Z

/test pull-kubernetes-bazel
/test pull-kubernetes-e2e-kops-aws

k8s-github-robot · 2017-09-01T01:43:37Z

Automatic merge from submit-queue (batch tested with PRs 51707, 51662, 51723, 50163, 51633)

…3-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #50163 Cherry pick of #50163 on release-1.7. fixes #50121 #50163: Change SizeLimit to a pointer ```release-note The alpha `emptyDir.sizeLimit` field is now correctly omitted from API requests and responses when unset. ```

k8s-cherrypick-bot · 2017-09-06T15:20:12Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

janetkuo · 2017-11-30T22:39:41Z

staging/src/k8s.io/api/core/v1/types.go

@@ -739,7 +739,7 @@ type EmptyDirVolumeSource struct {
 	// The default is nil which means that the limit is undefined.
 	// More info: http://kubernetes.io/docs/user-guide/volumes#emptydir
 	// +optional
-	SizeLimit resource.Quantity `json:"sizeLimit,omitempty" protobuf:"bytes,2,opt,name=sizeLimit"`
+	SizeLimit *resource.Quantity `json:"sizeLimit,omitempty" protobuf:"bytes,2,opt,name=sizeLimit"`


Doesn't this break backward compatibility?

Actually, the introduction of the non-pointer field broke compatibility, and this restored it

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

k8s-github-robot assigned sttts and vishh Aug 4, 2017

jingxu97 force-pushed the Aug/sizeLimit branch from 403136d to b0fc2f8 Compare August 4, 2017 20:05

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017

jingxu97 requested a review from liggitt August 4, 2017 20:08

liggitt reviewed Aug 4, 2017

View reviewed changes

jingxu97 force-pushed the Aug/sizeLimit branch 3 times, most recently from f303a58 to fd8304c Compare August 4, 2017 21:07

jingxu97 removed the release-note-label-needed label Aug 7, 2017

k8s-github-robot added the release-note-label-needed label Aug 7, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2017

jingxu97 force-pushed the Aug/sizeLimit branch 2 times, most recently from 38138a0 to 7d33b15 Compare August 16, 2017 22:37

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017

jingxu97 force-pushed the Aug/sizeLimit branch from 7d33b15 to e378a7e Compare August 16, 2017 22:59

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017

liggitt reviewed Aug 18, 2017

View reviewed changes

liggitt added this to the v1.7 milestone Aug 18, 2017

liggitt added the cherrypick-candidate label Aug 18, 2017

jingxu97 force-pushed the Aug/sizeLimit branch from e378a7e to 28c8564 Compare August 18, 2017 20:03

jingxu97 force-pushed the Aug/sizeLimit branch from 61ad488 to 416b692 Compare August 29, 2017 22:18

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2017

jingxu97 added 2 commits August 30, 2017 11:50

Change SizeLimit to a pointer

4d6da1f

This PR fixes issue kubernetes#50121

Generated files

e1460ef

generated files

jingxu97 force-pushed the Aug/sizeLimit branch from 416b692 to e1460ef Compare August 30, 2017 19:45

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-github-robot merged commit 8679a8f into kubernetes:master Sep 1, 2017

liggitt mentioned this pull request Sep 1, 2017

Automated cherry pick of #50163 #51763

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 6, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 6, 2017

janetkuo reviewed Nov 30, 2017

View reviewed changes

This was referenced Dec 11, 2017

volumes.emptyDir.sizeLimit field is silently dropped #57058

Closed

Deployment enters creation hot-loop when rs field is mutated by API server #57167

Open

ayj mentioned this pull request Jan 3, 2018

update kubernetes golang dependencies to release-1.9 istio/istio#2396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change SizeLimit to a pointer #50163

Change SizeLimit to a pointer #50163

jingxu97 commented Aug 4, 2017 •

edited by liggitt

Loading

liggitt Aug 4, 2017

jingxu97 Aug 7, 2017

sttts Aug 10, 2017

jingxu97 commented Aug 7, 2017

jingxu97 commented Aug 9, 2017

kyessenov commented Aug 16, 2017

liggitt Aug 7, 2017

jingxu97 Aug 18, 2017

liggitt Aug 18, 2017

liggitt Aug 7, 2017

liggitt commented Aug 18, 2017

jingxu97 commented Aug 29, 2017

liggitt commented Aug 30, 2017

liggitt commented Aug 30, 2017 •

edited

Loading

jingxu97 commented Aug 30, 2017

liggitt commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

jingxu97 commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

k8s-ci-robot commented Sep 1, 2017 •

edited

Loading

liggitt commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-cherrypick-bot commented Sep 6, 2017

janetkuo Nov 30, 2017

liggitt Nov 30, 2017

Change SizeLimit to a pointer #50163

Change SizeLimit to a pointer #50163

Conversation

jingxu97 commented Aug 4, 2017 • edited by liggitt Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Aug 7, 2017

jingxu97 commented Aug 9, 2017

kyessenov commented Aug 16, 2017

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

liggitt commented Aug 18, 2017

jingxu97 commented Aug 29, 2017

liggitt commented Aug 30, 2017

liggitt commented Aug 30, 2017 • edited Loading

jingxu97 commented Aug 30, 2017

liggitt commented Aug 30, 2017

k8s-github-robot commented Aug 30, 2017

jingxu97 commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

fejta-bot commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

k8s-ci-robot commented Sep 1, 2017 • edited Loading

liggitt commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-cherrypick-bot commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Aug 4, 2017 •

edited by liggitt

Loading

liggitt commented Aug 30, 2017 •

edited

Loading

k8s-ci-robot commented Sep 1, 2017 •

edited

Loading