This repository has been archived by the owner on Mar 16, 2024. It is now read-only.
enhancement: add ComputeClasses and VolumeClasses fields to BaseResources #2472
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylerslaton
force-pushed
the
compute-class-quota
branch
from
8c5b667 to
817a740
Compare
WalkthroughThe recent update introduces a significant overhaul in how compute and volume storage resources are managed, shifting from individual resource quantities to structured Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
pkg/controller/quota/testdata/basic/input.yamlis excluded by:!**/*.yamlpkg/openapi/generated/openapi_generated.gois excluded by:!**/generated/**
Files selected for processing (14)
- pkg/apis/internal.admin.acorn.io/v1/baseresources.go (7 hunks)
- pkg/apis/internal.admin.acorn.io/v1/baseresources_test.go (5 hunks)
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources_test.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/quotarequests.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/quotarequests_test.go (23 hunks)
- pkg/apis/internal.admin.acorn.io/v1/resources.go (3 hunks)
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources_test.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/zz_generated.deepcopy.go (4 hunks)
- pkg/controller/quota/quota.go (4 hunks)
- pkg/controller/quota/testdata/basic/expected.golden (1 hunks)
- pkg/controller/quota/testdata/implicit-pv-bind/expected.golden (1 hunks)
- pkg/controller/quota/testdata/status-default-volume-size/expected.golden (1 hunks)
Additional comments: 56
pkg/controller/quota/testdata/implicit-pv-bind/expected.golden (2)
- 12-15: LGTM!
- 20-20: LGTM!
pkg/controller/quota/testdata/status-default-volume-size/expected.golden (2)
- 12-15: LGTM!
- 20-22: LGTM!
pkg/controller/quota/testdata/basic/expected.golden (2)
- 12-15: LGTM!
- 20-22: LGTM!
pkg/apis/internal.admin.acorn.io/v1/resources.go (3)
- 67-67: Ensure returning a new
resource.Quantityinstance inSubQuantitydoes not introduce unexpected behavior elsewhere in the codebase.- 86-88: Renaming
ResourcesToStringtoCountResourcesToStringand removing the parameter improves clarity and reduces complexity.- 103-111: Adding
QuantityResourceToStringenhances modularity by providing a dedicated function for string representation of a resource and its quantity.pkg/apis/internal.admin.acorn.io/v1/quotarequests.go (1)
- 113-113: Using
CountResourcesToStringin theToStringmethod aligns with the improvements made inresources.goand enhances code readability.pkg/apis/internal.admin.acorn.io/v1/volumeclassresources.go (1)
- 1-119: The implementation of
VolumeClassResourcesand its methods correctly supports the addition, removal, fitting, and string representation of volume class resources. Ensure that the logic inAdd,Remove,Fits, andToStringmethods correctly handles all edge cases, especially with unlimited resources.pkg/apis/internal.admin.acorn.io/v1/baseresources.go (5)
- 18-21: The addition of
ComputeClassesandVolumeClassesfields toBaseResourcesstruct aligns with the PR's objectives of improving resource categorization and tracking.- 32-39: Ensure the initialization of
ComputeClassesandVolumeClassesdoes not introduce unnecessary overhead when these fields are already initialized.- 50-52: The conditional removal of
VolumeClassesbased on theallflag is logical. Verify that this behavior is consistent with the intended functionality across all use cases.- 84-89: The refactoring of the
Fitsmethod to include checks forComputeClassesandVolumeClassesis crucial for ensuring resources do not exceed their allocations. Confirm that the error aggregation logic correctly handles multiple errors.- 109-122: The updated
ToStringmethod now includesComputeClassesandVolumeClasses, enhancing the string representation ofBaseResources. Ensure the string formatting logic is efficient and error-free.pkg/apis/internal.admin.acorn.io/v1/computeclassresources.go (1)
- 1-150: The implementation of
ComputeClassResourcesand its methods correctly supports the addition, removal, fitting, and string representation of compute class resources. Ensure that the logic inAdd,Remove,Fits, andToStringmethods correctly handles all edge cases, especially with unlimited resources.pkg/apis/internal.admin.acorn.io/v1/volumeclassresources_test.go (1)
- 1-289: The tests for
VolumeClassResourcesmethods (Add,Remove,Equals,Fits,ToString) appear comprehensive and correctly validate the functionality. Ensure that all edge cases, especially handling of unlimited resources, are covered.pkg/controller/quota/quota.go (4)
- 127-127: Converting the result of
replicas(container.Scale)to anintbefore adding it toquotaRequest.Spec.Resources.Containersis correct. Ensure this conversion aligns with the expected data types throughout the codebase.- 142-156: The updates to
addComputeto handleComputeClasseswith the correct multiplication of CPU and memory requests by the container scale are logical. Verify that the handling ofcomputeClassand resource multiplication aligns with the intended functionality and does not introduce precision issues.- 201-205: The
addStoragefunction's update to include volume class information when adding volume storage toquotaRequest.Spec.Resourcesis correct. Ensure that the handling ofvolumeClassaligns with the intended functionality and that the class-based resource tracking is accurately reflected.- 273-275: Changing the
replicasfunction signature fromfunc replicas(s *int32) inttofunc replicas(s *int32) int64is logical given the context. Confirm that this change does not introduce type mismatches or conversion issues elsewhere in the codebase.pkg/apis/internal.admin.acorn.io/v1/baseresources_test.go (10)
- 20-34: Ensure the test case "add to existing BaseResources resources" correctly updates the
ComputeClassesandVolumeClassesto reflect the new fields' addition.- 38-54: The handling of unlimited values in "add where current and incoming have a resource specified with unlimited" is correctly implemented, following the expected logic that adding any value to unlimited remains unlimited.
- 56-79: The test case "add where current and incoming have ComputeClasses and VolumeClasses" correctly demonstrates the addition of resources within
ComputeClassesandVolumeClasses, including the doubling of values for matching classes.- 83-99: The test case "add where current is empty and incoming has ComputeClasses and VolumeClasses" correctly initializes the
BaseResourceswith the incoming values when the current is empty.- 123-157: The handling of unlimited values in removal operations, such as "remove where current has a resource specified with unlimited" and "remove where incoming has a resource specified with unlimited", is correctly implemented, preserving the unlimited status when applicable.
- 160-178: The test case "remove where current and incoming have ComputeClasses and VolumeClasses" with
allset to true correctly results in an emptyBaseResources, demonstrating the removal logic's correctness.- 181-189: The test case "does not remove volume storage when all is false" correctly retains the
VolumeClasseswhenallis not specified, demonstrating the conditional removal logic.- 218-234: The test cases for
BaseResourcesEquals, including handling unlimited values and comparingComputeClassesandVolumeClasses, are correctly implemented, ensuring accurate equality checks.- 299-320: The test cases for
BaseResourcesFits, including handling unlimited values and checking fits forComputeClassesandVolumeClasses, are correctly implemented, ensuring accurate fit checks.- 386-426: The
BaseResourcesToStringmethod's test cases, including handling unlimited values and multipleComputeClassesandVolumeClasses, are correctly implemented, ensuring accurate string representations.pkg/apis/internal.admin.acorn.io/v1/computeclassresources_test.go (5)
- 20-88: The test cases for
ComputeClassResourcesAdd, including handling unlimited values and adding resources withAllComputeClasses, are correctly implemented, demonstrating the addition logic for compute class resources.- 109-197: The test cases for
ComputeClassResourcesRemove, including handling unlimited values and removing resources withAllComputeClasses, are correctly implemented, demonstrating the removal logic for compute class resources.- 218-252: The test cases for
ComputeClassResourcesEquals, including handling unlimited values, are correctly implemented, ensuring accurate equality checks for compute class resources.- 264-390: The test cases for
ComputeClassResourcesFits, including handling unlimited values and fitting resources withAllComputeClasses, are correctly implemented, ensuring accurate fit checks for compute class resources.- 404-445: The
ComputeClassResourcesToStringmethod's test cases, including handling unlimited values and multiple compute classes, are correctly implemented, ensuring accurate string representations for compute class resources.pkg/apis/internal.admin.acorn.io/v1/quotarequests_test.go (15)
- 24-34: The test case "add to empty QuotaRequestResources" correctly initializes the
QuotaRequestResourceswith the incoming values, includingComputeClassesandVolumeClasses.- 43-63: The test case "add to existing QuotaRequestResources" correctly demonstrates the addition of resources within
QuotaRequestResources, including the aggregation ofComputeClassesandVolumeClasses.- 72-90: The handling of unlimited values in "add where current has a resource specified with unlimited" is correctly implemented, following the expected logic that adding any value to unlimited remains unlimited.
- 99-117: The test case "add where incoming has a resource specified with unlimited" correctly updates the
QuotaRequestResourcesto reflect unlimited values for all resources.- 126-144: The test case "add where current and incoming have a resource specified with unlimited" correctly maintains the unlimited status for all resources.
- 172-174: The test case "remove from empty QuotaRequestResources" correctly demonstrates that removing resources from an empty
QuotaRequestResourcesresults in no change.- 185-195: The test case "should never get negative values" correctly ensures that removing more resources than available results in an empty
QuotaRequestResources.- 207-215: The test case "removes persistent resources with all" correctly demonstrates the removal of all resources when
allis true.- 228-236: The test case "does not remove persistent resources without all" correctly retains resources when
allis not specified, demonstrating conditional removal logic.- 251-269: The handling of unlimited values in removal operations, such as "remove where current has a resource specified with unlimited", is correctly implemented, preserving the unlimited status when applicable.
- 278-296: The test case "remove where incoming has a resource specified with unlimited" correctly retains the original resources when the incoming request specifies unlimited values.
- 305-321: The test case "remove where current and incoming have a resource specified with unlimited" correctly maintains the unlimited status for all resources.
- 388-405: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [352-442]
The test cases for
QuotaRequestResourcesEquals, including handling unlimited values and comparingComputeClassesandVolumeClassResources, are correctly implemented, ensuring accurate equality checks.
- 489-505: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [473-542]
The test cases for
QuotaRequestResourcesFits, including handling unlimited values and checking fits forComputeClassesandVolumeClasses, are correctly implemented, ensuring accurate fit checks.
- 615-639: The
QuotaRequestResourcesToStringmethod's test cases, including handling unlimited values and multipleComputeClassesandVolumeClasses, are correctly implemented, ensuring accurate string representations.pkg/apis/internal.admin.acorn.io/v1/zz_generated.deepcopy.go (4)
- 271-289: The
DeepCopyIntoandDeepCopymethods forComputeClassResourcesare correctly implemented to ensure a deep copy of map elements. No issues found.- 292-306: The
DeepCopyIntoandDeepCopymethods forComputeResourcescorrectly perform deep copies ofMemoryandCPU, which are presumably complex types requiring deep copying. No issues found.- 707-724: The
DeepCopyIntoandDeepCopymethods forVolumeClassResourcesare correctly implemented to ensure a deep copy of map elements. No issues found.- 743-755: The
DeepCopyIntoandDeepCopymethods forVolumeResourcescorrectly perform a deep copy ofVolumeStorage, which is presumably a complex type requiring deep copying. No issues found.
Comment on lines
+17
to
+30
| if in.ComputeClasses != nil { | ||
| in, out := &in.ComputeClasses, &out.ComputeClasses | ||
| *out = make(ComputeClassResources, len(*in)) | ||
| for key, val := range *in { | ||
| (*out)[key] = *val.DeepCopy() | ||
| } | ||
| } | ||
| if in.VolumeClasses != nil { | ||
| in, out := &in.VolumeClasses, &out.VolumeClasses | ||
| *out = make(VolumeClassResources, len(*in)) | ||
| for key, val := range *in { | ||
| (*out)[key] = *val.DeepCopy() | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure that nil checks are performed before attempting to make deep copies of ComputeClasses and VolumeClasses to prevent potential nil pointer dereferences.
if in.ComputeClasses != nil {
+ if out.ComputeClasses == nil {
+ out.ComputeClasses = make(ComputeClassResources)
+ }
in, out := &in.ComputeClasses, &out.ComputeClasses
*out = make(ComputeClassResources, len(*in))
for key, val := range *in {
(*out)[key] = *val.DeepCopy()
}
}
if in.VolumeClasses != nil {
+ if out.VolumeClasses == nil {
+ out.VolumeClasses = make(VolumeClassResources)
+ }
in, out := &in.VolumeClasses, &out.VolumeClasses
*out = make(VolumeClassResources, len(*in))
for key, val := range *in {
(*out)[key] = *val.DeepCopy()
}
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| if in.ComputeClasses != nil { | |
| in, out := &in.ComputeClasses, &out.ComputeClasses | |
| *out = make(ComputeClassResources, len(*in)) | |
| for key, val := range *in { | |
| (*out)[key] = *val.DeepCopy() | |
| } | |
| } | |
| if in.VolumeClasses != nil { | |
| in, out := &in.VolumeClasses, &out.VolumeClasses | |
| *out = make(VolumeClassResources, len(*in)) | |
| for key, val := range *in { | |
| (*out)[key] = *val.DeepCopy() | |
| } | |
| } | |
| if in.ComputeClasses != nil { | |
| if out.ComputeClasses == nil { | |
| out.ComputeClasses = make(ComputeClassResources) | |
| } | |
| in, out := &in.ComputeClasses, &out.ComputeClasses | |
| *out = make(ComputeClassResources, len(*in)) | |
| for key, val := range *in { | |
| (*out)[key] = *val.DeepCopy() | |
| } | |
| } | |
| if in.VolumeClasses != nil { | |
| if out.VolumeClasses == nil { | |
| out.VolumeClasses = make(VolumeClassResources) | |
| } | |
| in, out := &in.VolumeClasses, &out.VolumeClasses | |
| *out = make(VolumeClassResources, len(*in)) | |
| for key, val := range *in { | |
| (*out)[key] = *val.DeepCopy() | |
| } | |
| } |
tylerslaton
force-pushed
the
compute-class-quota
branch
from
817a740 to
18bf563
Compare
thedadams
approved these changes
Feb 6, 2024
…rces This commit enhances the BaseResources struct by adding ComputeClasses and VolumeClasses fields. These new fields allow for a more detailed specification of compute and storage resources, categorized by class instead of just by resource type. This enhancement is crucial for accurately tracking the usage of memory, CPU, and storage for each specific compute or volume class. Consequently, the QuotaRequest logic has been updated to account for these new fields. These new fields are maps and they introduce a unique approach to handle unlimited resources. They include special keys, `AllComputeClasses` and `AllVolumeClasses`. If a value is assigned to these keys in ComputeClassResources or VolumeClassResources, all compute or volume classes will be evaluated against the value. Signed-off-by: tylerslaton <[email protected]>
tylerslaton
force-pushed
the
compute-class-quota
branch
from
18bf563 to
ba9ce98
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
pkg/controller/quota/testdata/basic/input.yamlis excluded by:!**/*.yamlpkg/openapi/generated/openapi_generated.gois excluded by:!**/generated/**
Files selected for processing (14)
- pkg/apis/internal.admin.acorn.io/v1/baseresources.go (7 hunks)
- pkg/apis/internal.admin.acorn.io/v1/baseresources_test.go (5 hunks)
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources_test.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/quotarequests.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/quotarequests_test.go (23 hunks)
- pkg/apis/internal.admin.acorn.io/v1/resources.go (3 hunks)
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources_test.go (1 hunks)
- pkg/apis/internal.admin.acorn.io/v1/zz_generated.deepcopy.go (4 hunks)
- pkg/controller/quota/quota.go (2 hunks)
- pkg/controller/quota/testdata/basic/expected.golden (1 hunks)
- pkg/controller/quota/testdata/implicit-pv-bind/expected.golden (1 hunks)
- pkg/controller/quota/testdata/status-default-volume-size/expected.golden (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- pkg/apis/internal.admin.acorn.io/v1/baseresources.go
- pkg/apis/internal.admin.acorn.io/v1/baseresources_test.go
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources.go
- pkg/apis/internal.admin.acorn.io/v1/computeclassresources_test.go
- pkg/apis/internal.admin.acorn.io/v1/quotarequests.go
- pkg/apis/internal.admin.acorn.io/v1/quotarequests_test.go
- pkg/apis/internal.admin.acorn.io/v1/resources.go
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources.go
- pkg/apis/internal.admin.acorn.io/v1/volumeclassresources_test.go
- pkg/apis/internal.admin.acorn.io/v1/zz_generated.deepcopy.go
- pkg/controller/quota/quota.go
- pkg/controller/quota/testdata/basic/expected.golden
- pkg/controller/quota/testdata/implicit-pv-bind/expected.golden
- pkg/controller/quota/testdata/status-default-volume-size/expected.golden
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit enhances the BaseResources struct by adding ComputeClasses and VolumeClasses fields. These new fields allow for a more detailed specification of compute and storage resources, categorized by class instead of just by resource type.
This enhancement is crucial for accurately tracking the usage of memory, CPU, and storage for each specific compute or volume class. Consequently, the QuotaRequest logic has been updated to account for these new fields.
These new fields are maps and they introduce a unique approach to handle unlimited resources. They include special keys,
AllComputeClassesandAllVolumeClasses. If a value is assigned to these keys in ComputeClassResources or VolumeClassResources, all compute or volume classes will be evaluated against the value.Checklist
This is a title (#1216). Here's an exampleSummary by CodeRabbit
ComputeClassesandVolumeClasses.ComputeClassesandVolumeClasses.