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.

Sign up for GitHub

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

Jump to bottom

ResourceRequest Limits does not work as described #7355

Closed
gmarek opened this issue Apr 27, 2015 · 12 comments
Closed

ResourceRequest Limits does not work as described #7355

gmarek opened this issue Apr 27, 2015 · 12 comments
Assignees
@davidopp
Labels
area/isolation priority/backlog Higher priority than priority/awaiting-more-evidence. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.

Comments

@gmarek
Copy link
Contributor

gmarek commented Apr 27, 2015

The API v3 says that:

// ResourceRequirements describes the compute resource requirements.
type ResourceRequirements struct {
    // Limits describes the maximum amount of compute resources required.
    Limits ResourceList `json:"limits,omitempty" description:"Maximum amount of compute resources allowed"`
    // Requests describes the minimum amount of compute resources required.
    Requests ResourceList `json:"requests,omitempty" description:"Minimum amount of resources requested"`
}

But in our scheduler implementation we use:

func getResourceRequest(pod *api.Pod) resourceRequest {
    result := resourceRequest{}
    for ix := range pod.Spec.Containers {
        limits := pod.Spec.Containers[ix].Resources.Limits
        result.memory += limits.Memory().Value()
        result.milliCPU += limits.Cpu().MilliValue()
    }
    return result
}

This means that in practice we use Limits as an amount used by the Scheduler to see if Pod fits the Node (i.e. we use it as Request should be), and we don't use Requests in any meaningful way.

For obvious reasons this is confusing, and we should fix that by either removing Requests from API and changing the comment, or fix current usages. I'm in favor of removing, even though I see that in future we may wan't to introduce this split again.

@ggarson @bgrant0607 @davidopp @thockin
@gmarek gmarek added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. team/master labels Apr 27, 2015
@gmarek gmarek added this to the v1.0 milestone Apr 27, 2015
@derekwaynecarr
Copy link
Member

If limits are specified, they need to always take precedence over Requests. I think Requests right now is only used for persistent volume storage sizes.

@thockin
Copy link
Member

thockin commented Apr 27, 2015

I agree that the comment is worded badly, but the description makes more
sense to me. In the PersistentVolume discussion, I argued this same point

  • Limit was the wrong field to start with. I can not find the notes on
    the discussion, but maybe Brian's bear-trap memory will bring it back to
    light.

On Mon, Apr 27, 2015 at 7:46 AM, Derek Carr notifications@github.com
wrote:

If limits are specified, they need to always take precedence over Requests.
I think Requests right now is only used for persistent volume storage
sizes.

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

@markturansky
Copy link
Contributor

Here's one bit of that conversation: #4055 (comment)

@markturansky
Copy link
Contributor

A more recent one: #5105 (comment)

It can be difficult to piece together an entire conversation on github.

@thockin
Copy link
Member

thockin commented Apr 27, 2015

So my comments there were about Request. I think Request is correct.

On Mon, Apr 27, 2015 at 9:35 AM, Mark Turansky notifications@github.com
wrote:

A more recent one: #5105 (comment)
#5105 (comment)

It can be difficult to piece together an entire conversation on github.

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

@lavalamp lavalamp removed this from the v1.0 milestone Apr 27, 2015
@lavalamp
Copy link
Member

I don't think this is a 1.0 blocker.

@derekwaynecarr
Copy link
Member

I think the problem is we have no way of enforcing Requests in the Kubelet right now via the Docker resource controls since it only allows for hard limits.

In the near term, we could look to do the following:

  1. Validate that container.resources.requests is <= container.resources.limits for each specified key
  2. Scheduler should merge container.resources.requests with container.resources.limits (where limits takes precedence) to build the getResourceRequest to perform node placement
  3. Kubelet ignores container.resources.requests until we have pod level cgroup support

@bgrant0607
Copy link
Member

The comments/descriptions reflect what we want to have implemented. Requests has not been implemented, except for storage.

The scheduler should use Requests.

Kubelet doesn't need to enforce Requests.

Requests < Limits implies a lower QoS (#147).

See the explanation in https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/resources.md

cc @vishh

@bgrant0607 bgrant0607 added priority/backlog Higher priority than priority/awaiting-more-evidence. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. area/isolation and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Apr 28, 2015
@vishh vishh mentioned this issue Apr 28, 2015
Merged
@davidopp davidopp self-assigned this Apr 28, 2015
@bgrant0607
Copy link
Member

We should document how we translate cpu limit to shares. The swagger isn't really the right place. Probably we need a more user-focused version of resources.md.

@gmarek
Copy link
Contributor Author

gmarek commented Jun 30, 2015

Because we provide an abstraction of a higher level than Docker and our users don't really need to understand Docker way of handling resources (i.e. what kind of guarantees they can/cannot expect), I believe that we should explain this as well. The same holds for other container engines.

@bgrant0607 bgrant0607 mentioned this issue Jul 6, 2015
Closed
@bgrant0607 bgrant0607 mentioned this issue Jul 23, 2015
Merged
@gmarek
Copy link
Contributor Author

gmarek commented Aug 21, 2015

@bgrant0607 is this fixed by current QoS implementation?

@bgrant0607
Copy link
Member

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davidopp davidopp

Labels
area/isolation priority/backlog Higher priority than priority/awaiting-more-evidence. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@lavalamp @gmarek @mikedanese @thockin @derekwaynecarr @bgrant0607 @markturansky @davidopp