search

acorn-io / runtime

A simple application deployment framework built on Kubernetes
https://docs.acorn.io/
Apache License 2.0
1.13k stars 100 forks source link

fix: add CPU field to resolvedOfferings and change QuotaRequests to use resolvedOfferings #2462

Closed tylerslaton closed 9 months ago

tylerslaton commented 10 months ago

This PR adjusts two main things.

  1. Removes the cpuScaler field from ResolvedOfferings in favor of a new cpu field.
  2. Updates QuotaRequests to report against the ResolvedOfferings fields for container compute.

These two things are necessary for ComputeClasses that define a requestScaler on memory as we want to quota against the requested amount, not the incoming scaled down amount.

Checklist

tylerslaton commented 10 months ago

This will have overlap with https://github.com/acorn-io/runtime/pull/2454. I will merge that one first but want to open this up for review.

tylerslaton commented 10 months ago

@thedadams The resolved offerings get recalculated only when the app's generation changes. I believe that means that, after this change goes in, there will be apps that don't have CPUScaler (because it is being removed) nor CPU (because it's not being set). Are we OK with this? Should we handle this case?

The resolvedOfferings CPUScaler is currently not utilized anywhere in our codebase, including any dependencies of runtime. Therefore, removing it will have no impact on existing applications. It's worth noting that while there will be instances where resolvedOfferings.CPU is not set for an application, this is acceptable because the scheduling section will ensure the retention of the previously defined request. Once that application updates it will go through the process of getting resolvedOfferings.CPU instead.

g-linville commented 10 months ago

Forgive me if my memory is failing, but even though I previously approved this PR, I find this choice confusing. I think it makes the most sense to just continue using the scheduling field for quota stuff instead of resolvedOfferings. I also don't know why we want CPU (or CPUScaler for that matter) on resolvedOfferings, because I think that is irrelevant to the user...

tylerslaton commented 10 months ago

Forgive me if my memory is failing, but even though I previously approved this PR, I find this choice confusing. I think it makes the most sense to just continue using the scheduling field for quota stuff instead of resolvedOfferings.

This all revolves around a recent field addition that we made to defining memory in ComputeClasses, requestScaler for memory. Let's look at an example of this in action to help illustrate the need here. Say that we define a ComputeClass where it has a memory requestScaler or 0.5. If a user requests 1Gi of memory then what they will actually have scheduled here is 512Mi of memory. Since quota reads from the scheduling section this means we will quota for 512Mi of memory. However, we really want to quota for 1Gi of memory here. Our options were:

  1. Read from the limits section of scheduling (already vetoed)
  2. Read from the ResolvedOfferings section of status

I also don't know why we want CPU (or CPUScaler for that matter) on resolvedOfferings, because I think that is irrelevant to the user...

I added this mainly since it is nicer to have ResolvedOfferings field tell me what the CPU should be as opposed to using the cpuScaler to determine what the CPU should be. In either situation I need to have an additional field tell me what (or how to calculate what) the CPU should be.

g-linville commented 9 months ago

Thanks for the explanation. That makes sense now. Idk why I got so confused on Saturday lol.