Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Size memory backed volumes #1968

Merged
merged 1 commit into from
Sep 11, 2020

Conversation

derekwaynecarr
Copy link
Member

Improve portability of pods that consume memory backed volumes by decoupling the size of the volume from the node that runs the pod.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr

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:

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

@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 3, 2020
@derekwaynecarr
Copy link
Member Author

/assign @dashpole @sjenning @dchen1107
/milestone v1.20

@derekwaynecarr
Copy link
Member Author

The verify job failing is related to a sig-scheduling kep and not this kep.


## Proposal

Define a new feature gate: `SizeMemoryBackedVolumes`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feature flag is on top of another feature flag LocalStorageCapacityIsolation? Would it worth mentioning this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LocalStorageCapacityIsolation did not impact the size of memory backed volume, so I kept it separate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely. The only thing I was trying to say is that emptyDir.sizeLimit wouldn't work without the LocalStorageCapacityIsolation enabled.

@dims
Copy link
Member

dims commented Sep 3, 2020

@derekwaynecarr does this KEP change the meaning of the definition of sizeLimit here?
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L645-L651

	// Total amount of local storage required for this EmptyDir volume.
	// The size limit is also applicable for memory medium.
	// The maximum usage on memory medium EmptyDir would be the minimum value between
	// the SizeLimit specified here and the sum of memory limits of all containers in a pod.
	// 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"`

I feel the definition above means there is at least that much memory as mentioned in SizeLimit, right?

## Design Details

The design for this implementation makes the existing `emptyDir.sizeLimit`
not just used during eviction heuristics, but for sizing of the volume.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be precise, general memory or disk eviction don't take emptyDir.sizeLimit into account. But as part of local ephemeral storage capacity isolation, we evict pods where the usage of an empty-dir exceeds its sizeLimit

@derekwaynecarr
Copy link
Member Author

@dims this KEP actually implements the behavior described in types.go where in the past, it just took 50% of node memory. Usage was restricted based on memory accounting in practice. In practical terms, a pod that requested 10Gi of memory that was scheduled to a Linux host with 16Gi of memory would have its empty dir set to 8Gi. This made it that pods could not actually use what was requested!

@dims
Copy link
Member

dims commented Sep 4, 2020

got it! thanks @derekwaynecarr

@justaugustus justaugustus removed their request for review September 4, 2020 05:55
@dashpole
Copy link
Contributor

/lgtm
/hold
for any additional comments. @derekwaynecarr feel free to remove when you are satisfied. I'll probably have to reapply lgtm after you fix verify anyways

cc @johnbelamaric, who is listed as the PRR reviewer.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 11, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2020
@derekwaynecarr
Copy link
Member Author

/hold cancel

rebased to pick up fix to verify, ptal @dashpole

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2020
@dashpole
Copy link
Contributor

/lgtm

Thanks a bunch Derek

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 11, 2020
@k8s-ci-robot k8s-ci-robot merged commit c0059b3 into kubernetes:master Sep 11, 2020
@SergeyKanzhelev
Copy link
Member

cross link to k/k code for completeness: kubernetes/kubernetes#94444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants