feat(Flow Control)/Expand Flow Control capacity limits schema(resource.Quantity)#2492
Conversation
- add struct CapacityLimits - add Limits Config in PriorityBandConfig and FlowControlConfig
…ConfigFromAPI` function: - Add New Logic: Implement logic to write the new configuration field `Limit.MaxBytes` to a `uint64` variable, including the necessary type conversion. - Remove Old Logic: Delete the existing logic responsible for writing the legacy `MaxBytes` configuration.
- use the new conf for the logs print
|
@BizerNotNull: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Welcome @BizerNotNull! |
|
Hi @BizerNotNull. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ahg-g
left a comment
There was a problem hiding this comment.
/ok-to-test
/assign @LukeAVanDrie
| github.com/fxamacker/cbor/v2 v2.9.0 // indirect | ||
| github.com/go-openapi/jsonpointer v0.22.1 // indirect | ||
| github.com/go-openapi/jsonreference v0.21.3 // indirect | ||
| github.com/go-openapi/jsonpointer v0.22.4 // indirect |
There was a problem hiding this comment.
pls revert the changes to the go.mod and go.sum
There was a problem hiding this comment.
I've reverted this. But this is because the go.mod in the root directory and the one in the conformance folder have mismatched versions, which causes updates every time make test is run
| // Default: 0 (unlimited). | ||
| MaxBytes *int64 `json:"maxBytes,omitempty"` | ||
| // If not specified, no global limits are enforced. | ||
| Limits *CapacityLimits `json:"limits,omitempty"` |
There was a problem hiding this comment.
just curious what other types of limits you forsee here. since we are in a config object keeping the API flat also seems reasonable.
There was a problem hiding this comment.
I think just requests and bytes. The only other possibility I can think of is estimated tokens, but that doesn't seem practical at the moment. Definitely open to keeping the structure flat.
There was a problem hiding this comment.
I've flated the Limits by the new commits, I'll update the pr's description soon.
| } | ||
|
|
||
| // CapacityLimits defines capacity boundaries for a priority band. | ||
| type CapacityLimits struct { |
There was a problem hiding this comment.
question: since the key proposed is limits will capacity be reasonable in the future if for example we require rate specific config.
There was a problem hiding this comment.
Currently, many of the functions related to maxBytes logic are using capacity in their names. I think we can open a separate issue to rename them when appropriate.
There was a problem hiding this comment.
On second thought, the maxBytes here doesn't refer to the concurrency accepted by the backend, but rather the capacity of the manageQueue within each flow. From this perspective, 'capacity' is actually correct. When I implemented maxRequests in #2495 , I also calculated the number of items in the manageQueue.
9304893 to
0e54514
Compare
LukeAVanDrie
left a comment
There was a problem hiding this comment.
Thanks @BizerNotNull! This LGTM
Do you mind updating these examples in our guides to use the improved syntax?
|
/assign @ahg-g |
|
@LukeAVanDrie Sure,I'd like |
|
Docs is done in 8442b98 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, BizerNotNull, LukeAVanDrie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
…e.Quantity) (kubernetes-sigs#2492) * feat(conf): add config `Limits` in EndponintPickerConfig - add struct CapacityLimits - add Limits Config in PriorityBandConfig and FlowControlConfig * feat(config): Modify the MaxBytes configuration logic within the `NewConfigFromAPI` function: - Add New Logic: Implement logic to write the new configuration field `Limit.MaxBytes` to a `uint64` variable, including the necessary type conversion. - Remove Old Logic: Delete the existing logic responsible for writing the legacy `MaxBytes` configuration. * chore(conf): supplyment the comment for new conf * feat(conf): delete the old config `MaxBytes` - use the new conf for the logs print * refactor(config): extract `resolveMaxBytes` helper and improve Limits comments * feat(test): change the `MaxBytes` in test to `Limit.MaxBytes` * feat(test): add `ShouldSucceed_WithKubernetesQuantityFormat` to test the k8s case * chore(deepcopy): remake `deepcopy.go` * revert(config): flat the `Limits` * chore(make): after `make` * feat(docs): supplyment flow control docs for the description of `quantity format`
…e.Quantity) (kubernetes-sigs/gateway-api-inference-extension#2492) * feat(conf): add config `Limits` in EndponintPickerConfig - add struct CapacityLimits - add Limits Config in PriorityBandConfig and FlowControlConfig * feat(config): Modify the MaxBytes configuration logic within the `NewConfigFromAPI` function: - Add New Logic: Implement logic to write the new configuration field `Limit.MaxBytes` to a `uint64` variable, including the necessary type conversion. - Remove Old Logic: Delete the existing logic responsible for writing the legacy `MaxBytes` configuration. * chore(conf): supplyment the comment for new conf * feat(conf): delete the old config `MaxBytes` - use the new conf for the logs print * refactor(config): extract `resolveMaxBytes` helper and improve Limits comments * feat(test): change the `MaxBytes` in test to `Limit.MaxBytes` * feat(test): add `ShouldSucceed_WithKubernetesQuantityFormat` to test the k8s case * chore(deepcopy): remake `deepcopy.go` * revert(config): flat the `Limits` * chore(make): after `make` * feat(docs): supplyment flow control docs for the description of `quantity format`
What type of PR is this?
/kind feature
/kind test
/kind deprecation
What this PR does / why we need it:
maxBytesinendpointpickerconfig_type, change to uselimits.maxBytesby adding structCapacityLimits.limits.maxBytesuseresource.Quantityto replace theint, means you can use1Gilike k8s.example similar to:
before:
after:
config.go: add one helperresolveMaxBytesto replace the check ofmaxBytes, and the conversion ofresource.Quantitytouint64also occurs here.make generateregenerate the deepcopyWhich issue(s) this PR fixes:
Fixes Part 1 of #2486
Does this PR introduce a user-facing change?: