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

tylerslaton · 2024-01-26T15:40:51Z

This PR adjusts two main things.

Removes the cpuScaler field from ResolvedOfferings in favor of a new cpu field.
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

The title of this PR would make a good line in Acorn's Release Note's Changelog
The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
Commits follow contributing guidance
Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

tylerslaton · 2024-01-26T16:14:11Z

This will have overlap with #2454. I will merge that one first but want to open this up for review.

pkg/controller/resolvedofferings/computeclass.go

pkg/controller/appdefinition/testdata/computeclass/all-set/existing.yaml

keyallis

1 quick comment but otherwise LGTM

keyallis · 2024-01-29T21:06:15Z

pkg/controller/resolvedofferings/testdata/computeclass/compute-class-default/existing.yaml

+  min: 100Mi # 1Mi
+  max: 200Mi # 2Mi
+  default: 100Mi # 1Mi


Update the comments here

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?

thedadams · 2024-01-30T11:36:00Z

pkg/controller/quota/quota.go

+			quotaRequest.Spec.Resources.CPU.Add(cpu)
+			quotaRequest.Spec.Resources.Memory.Add(memory)


nit: cal we use the Mul method here instead of this loop?

We can, that change logic is being done in a different PR.

tylerslaton · 2024-01-30T16:38:40Z

@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.

…se resolvedOfferings This change makes it so that we always quota against the resolvedOfferings instead of the scheduling for the app. This is because the scheduling for the app is always accurate to what is actually running, but the resolvedOfferings is what the user requested. This change also adds a CPU field to the resolvedOfferings so that we can quota against the CPU that the user requested from one place instead of having to calculate it from the cpuScaler. Signed-off-by: tylerslaton <[email protected]>

tylerslaton · 2024-02-02T20:10:15Z

pkg/controller/quota/quota.go

-			quotaRequest.Spec.Resources.Memory.Add(requirements.Requests["memory"])
-		}
+		// Multiply the memory/cpu requests by the scale of the container
+		cpu.Mul(replicas(container.Scale))


This was previously in the ComputeClass quota PR but I just brought it here instead.

g-linville · 2024-02-03T23:01:17Z

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 · 2024-02-04T05:19:15Z

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:

Read from the limits section of scheduling (already vetoed)
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 · 2024-02-05T14:26:26Z

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

njhale

One non-blocking question.

njhale · 2024-02-05T16:15:48Z

pkg/controller/resolvedofferings/computeclass.go

-			CPUScaler: appInstance.Status.ResolvedOfferings.Containers[""].CPUScaler,
-			Class:     appInstance.Status.ResolvedOfferings.Containers[""].Class,
+			Memory: appInstance.Spec.Memory[""],
+			CPU:    z.Pointer(cpuQuantity.MilliValue()),


just curious: is there a behavioral difference between nil and a pointer to a zero value?

Talked offline. This reflects a weirder issue in quota. I'm going to make an issue for that, merge this, and come back to fix what this points to.

tylerslaton requested review from g-linville, thedadams and keyallis January 26, 2024 15:40

tylerslaton force-pushed the quota-ro branch from ed9bf05 to 18c2fad Compare January 26, 2024 16:13

tylerslaton force-pushed the quota-ro branch from 18c2fad to ff64eb6 Compare January 26, 2024 17:03

tylerslaton marked this pull request as ready for review January 26, 2024 17:03

g-linville suggested changes Jan 26, 2024

View reviewed changes

pkg/controller/resolvedofferings/computeclass.go Outdated Show resolved Hide resolved

pkg/controller/resolvedofferings/computeclass.go Show resolved Hide resolved

tylerslaton force-pushed the quota-ro branch from ff64eb6 to ab898ef Compare January 26, 2024 17:29

tylerslaton requested a review from g-linville January 26, 2024 17:30

tylerslaton force-pushed the quota-ro branch from ab898ef to 86e26f1 Compare January 26, 2024 17:42

g-linville approved these changes Jan 26, 2024

View reviewed changes

pkg/controller/appdefinition/testdata/computeclass/all-set/existing.yaml Outdated Show resolved Hide resolved

keyallis approved these changes Jan 29, 2024

View reviewed changes

thedadams reviewed Jan 30, 2024

View reviewed changes

tylerslaton requested a review from thedadams January 30, 2024 16:38

thedadams approved these changes Jan 30, 2024

View reviewed changes

tylerslaton force-pushed the quota-ro branch 2 times, most recently from b74db86 to 9926d71 Compare February 1, 2024 19:30

tylerslaton force-pushed the quota-ro branch from 9926d71 to 7d17579 Compare February 2, 2024 20:08

tylerslaton requested review from thedadams, keyallis and g-linville February 2, 2024 20:09

tylerslaton commented Feb 2, 2024

View reviewed changes

g-linville approved these changes Feb 5, 2024

View reviewed changes

njhale approved these changes Feb 5, 2024

View reviewed changes

tylerslaton merged commit 6a26e9b into acorn-io:main Feb 5, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tylerslaton commented Jan 26, 2024

tylerslaton commented Jan 26, 2024

keyallis left a comment

keyallis Jan 29, 2024

thedadams left a comment

thedadams Jan 30, 2024

tylerslaton Jan 30, 2024 •

edited

Loading

tylerslaton commented Jan 30, 2024 •

edited

Loading

tylerslaton Feb 2, 2024

g-linville commented Feb 3, 2024

tylerslaton commented Feb 4, 2024

g-linville commented Feb 5, 2024

njhale left a comment

njhale Feb 5, 2024

tylerslaton Feb 5, 2024

		quotaRequest.Spec.Resources.CPU.Add(cpu)
		quotaRequest.Spec.Resources.Memory.Add(memory)

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

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

Conversation

tylerslaton commented Jan 26, 2024

Checklist

tylerslaton commented Jan 26, 2024

keyallis left a comment

Choose a reason for hiding this comment

keyallis Jan 29, 2024

Choose a reason for hiding this comment

thedadams left a comment

Choose a reason for hiding this comment

thedadams Jan 30, 2024

Choose a reason for hiding this comment

tylerslaton Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

tylerslaton commented Jan 30, 2024 • edited Loading

tylerslaton Feb 2, 2024

Choose a reason for hiding this comment

g-linville commented Feb 3, 2024

tylerslaton commented Feb 4, 2024

g-linville commented Feb 5, 2024

njhale left a comment

Choose a reason for hiding this comment

njhale Feb 5, 2024

Choose a reason for hiding this comment

tylerslaton Feb 5, 2024

Choose a reason for hiding this comment

tylerslaton Jan 30, 2024 •

edited

Loading

tylerslaton commented Jan 30, 2024 •

edited

Loading