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

Downward API proposal for resources (cpu, memory) limits and requests #24051

Conversation

aveshagarwal
Copy link
Member

@aveshagarwal aveshagarwal commented Apr 8, 2016

Proposal to address #9473
This PR proposes three approaches to expose values of resource limits and requests as env vars and volumes.This proposal has details about merits and demerits of each approach, and I am looking for community feedback regarding which one (or may more than one) we would like to go with. Also would like to know if there is any other approach.


This change is Reviewable

@aveshagarwal
Copy link
Member Author

@kubernetes/rh-cluster-infra

@aveshagarwal
Copy link
Member Author

For reference: #9473

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 8, 2016
@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests branch from 3db2747 to 7561842 Compare April 8, 2016 18:10

## Background

Currently downward APIs (via environment variables and volume plugin) only support exposing a Pod's name, namespace, annotations, labels and its IP. This document explains the need and design to extend them to expose resources (e.g. cpu, memory, storage) limits and requests.
Copy link
Member

Choose a reason for hiding this comment

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

We just call it 'the downward API'

Copy link
Member

Choose a reason for hiding this comment

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

Please link to the main downward API doc on kubernetes.io

@pmorie
Copy link
Member

pmorie commented Apr 8, 2016

@aveshagarwal What's your opinion about which option to support? What are the relative merits and drawbacks? A proposal should examine the different options, contrast them on the merits, and pick one. So far this is a good explanation of the options, but I think it could use more comparison/contrast/and opinion.

@bgrant0607
Copy link
Member

cc @justinsb @vishh @dchen1107

| ---- | ------------------- |
| CPU_LIMIT | .spec.containers[?(@.name=="container-name")].resources.limits.cpu |
| MEMORY_LIMIT | .spec.containers[?(@.name=="container-name")].resources.limits.memory |
| STORAGE_LIMIT | .spec.containers[?(@.name=="container-name")].resources.limits.storage |
Copy link
Member

Choose a reason for hiding this comment

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

We don't have storage limits and requests.

@pmorie pmorie added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 15, 2016

#### Environment variables

This table shows how selectors can be used for various requests and limits to be exposed as environment variables.
Copy link
Contributor

Choose a reason for hiding this comment

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

How would this work if we support dynamically updating resource limits in the future?
cc @bgrant0607

Copy link
Member

Choose a reason for hiding this comment

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

IMO, in general, if a user updates some property that's fed into an env var via downward API, then it should cause the container to restart. If the user doesn't want the container to restart, they can use volumes instead or the API.

But it's sort of moot right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bgrant0607 Is it fair to assume that resource limits/requests can change often in the future, if we have vertical autoscaling? If yes, should we even allow exposing these values via env vars?

Copy link
Member

Choose a reason for hiding this comment

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

@vishh We need a way to set Java heap size and GOMAXPROCS on the command line. I'm more worried about making it possible to run common workloads now. I think we're far, far away from enabling vertical autosizing by default. Users who opt in will need to make other changes to their apps in order to adequately take advantage of it. Apps aren't built that way today.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests branch from 7561842 to 82b9ccd Compare April 19, 2016 04:58
@aveshagarwal
Copy link
Member Author

I have updated this proposal with more details and addressed all comments except following 2: pmorie's cgroup output format and bgrant's Examples of how to use these to set the Java heap size and GOMAXPROCS would be helpful. I will incorporate them soon.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests branch 3 times, most recently from 155706b to 1d6617d Compare April 19, 2016 13:43
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/design Categorizes issue or PR as related to design. and removed kind/design Categorizes issue or PR as related to design. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 20, 2016
| cpu_request | requests.cpu |
| memory_request | requests.memory |

Since environment variables are container scoped, the container name must
Copy link
Member

Choose a reason for hiding this comment

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

The container name must be specified because volume are pod scoped rather than container scoped.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah will be fixed in next update soon.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests branch from e150ecd to e8d4a3a Compare May 20, 2016 02:14
@aveshagarwal
Copy link
Member Author

fixed tests.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 20, 2016
@pmorie
Copy link
Member

pmorie commented May 20, 2016

@aveshagarwal The information here is good. My suggestions are purely additive:

There needs to be a passage stating which approach we will implement and why. It would be fine with me to add it at the end, and keep from spending time discussing how to mutate the text of this proposal so we can update the PR.

I think other than that, this LGTM. Thanks a lot for the proposal.

@aveshagarwal aveshagarwal force-pushed the master-downward-api-resources-limits-requests branch from e8d4a3a to 0f3a8bd Compare May 20, 2016 02:57
@aveshagarwal
Copy link
Member Author

@pmorie updated PR.

@pmorie
Copy link
Member

pmorie commented May 20, 2016

@aveshagarwal LGTM

@bgrant0607 PTAL

@bparees
Copy link
Contributor

bparees commented May 20, 2016

As long as the format is consistent with the divisor, I think its okay. I would want @bparees to weigh in.

agree and also agree that we don't want units by default, i think there are lots of cases where the unit would make the value harder to use in a config.

@pmorie
Copy link
Member

pmorie commented May 20, 2016

I do not want units period initially. You know the divisor you used; you know what the unit is.

@k8s-bot
Copy link

k8s-bot commented May 20, 2016

GCE e2e build/test passed for commit 0f3a8bd.

@aveshagarwal
Copy link
Member Author

@bparees yeah there are no units in the output now in the proposal with the selected approach.

| memory_request | requests.memory |

Volumes are pod scoped, the container name must be specified as part of
`containerSpecFieldRef` with them.
Copy link
Member

Choose a reason for hiding this comment

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

resourceFieldRef

@bgrant0607
Copy link
Member

Thank you. One nit, but LGTM. Feel free to reapply lgtm if you fix the nit.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 9e23c55 into kubernetes:master May 20, 2016
@pmorie
Copy link
Member

pmorie commented May 20, 2016

@aveshagarwal, send a follow-up PR to fix that nit and I will tag it.
Thanks again.

On Friday, May 20, 2016, k8s-merge-robot notifications@github.com wrote:

Merged #24051 #24051.


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

@aveshagarwal
Copy link
Member Author

@pmorie #25962

@aveshagarwal aveshagarwal deleted the master-downward-api-resources-limits-requests branch May 24, 2016 18:12
k8s-github-robot pushed a commit that referenced this pull request May 25, 2016
…rces-limits-requests-implementation

Automatic merge from submit-queue

Downward API implementation for resources limits and requests

This is an implementation of Downward API for resources limits and requests, and it works with environment variables and volume plugin.

This is based on proposal #24051. This implementation follows API with magic keys approach as discussed in the proposal.

@kubernetes/rh-cluster-infra

<!-- Reviewable:start -->
---
This change is [<img src="http://reviewable.k8s.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](http://reviewable.k8s.io/reviews/kubernetes/kubernetes/24179)
<!-- Reviewable:end -->
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Oct 30, 2019
UPSTREAM: 84561: FIPS changes for passing unit tests

Origin-commit: 38755aa39d1c483aedd51524ca34963f671bcce9
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Dec 9, 2019
UPSTREAM: 84561: FIPS changes for passing unit tests

Origin-commit: 38755aa39d1c483aedd51524ca34963f671bcce9
p0lyn0mial pushed a commit to p0lyn0mial/kubernetes that referenced this pull request Dec 20, 2019
UPSTREAM: 84561: FIPS changes for passing unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet