-
Notifications
You must be signed in to change notification settings - Fork 616
api + implementation: AgentgatewayParameters #13018
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.
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
api + implementation: AgentgatewayParameters #13018
Conversation
|
The test/deployer diffs would be greatly simplified if we first landed a PR that only changed the *-out.yaml files' map key ordering to alphabetical instead of 'as helm renders it'. |
josh-pritchard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review yet but have some feedback. Overall I really like the direction and I think we are simplifying some things by not having a package in internal & pkg. I am looking forward to you bringing the existing generator code in-line with this when this merges
install/helm/kgateway-crds/templates/agentgateway.dev_agentgatewaybackends.yaml
Outdated
Show resolved
Hide resolved
Before, they were helm's output. Soon we'll introduce a strategic-merge-patch step after that, so let's make the kgateway-dev#13018 code review easier by switching to golden files that are marshalled YAML of the objects we'll deploy (the identity function (for now) on helm's output but with alphabetical map keys since helm isn't producing the golden files). Signed-off-by: David L. Chandler <[email protected]>
I've broken out a prequel PR #13037; this review will shrink once that lands and this is updated. |
|
I duplicated the 'Image' API now that GVKs have split. |
howardjohn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks really good
|
Not done with review comments. I'll leave PDBs and HPAs to a follow up since those are new features and I'd like to discuss them further. |
Signed-off-by: David L. Chandler <[email protected]>
|
#13037 has landed and this has been updated. No *-out.yaml golden files were changed (except brand new ones). |
howardjohn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with followups
Add PDB and HPA
Remove GatewayParams support
Split HelmConfig for AGW and KGW
Merge GC and Gateway level params
@howardjohn wrote the API in kgateway-dev#13007 Signed-off-by: David L. Chandler <[email protected]>
e10c07b to
110fdca
Compare
|
@howardjohn, your commits on #13007 were squashed with mine into a single commit because it's the safest, fastest way I could think of to solve the problem of forgetting to sign off on a commit in the middle of the stack, which I did by using 'git revert' instead of 'git revert --signoff'. |
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
- DRY up TLS & other tests - add validator support to make it less likely an engineer and their code reviewers miss something. why comment when you can assert? - VerifyAllYAMLFilesReferenced Signed-off-by: David L. Chandler <[email protected]>
This is especially helpful because it is now obvious when a change to your config will propagate to your actual serving data plane pods. Signed-off-by: David L. Chandler <[email protected]>
|
I went ahead and proposed an API addition rawConfig so that the ~200 config options there. It is now obvious when a change to your config will propagate to your actual serving data-plane pods. |
…e/reorder Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
…nvoy GWC Signed-off-by: David L. Chandler <[email protected]>
|
/retest |
| namespace: default | ||
| spec: | ||
| shutdown: | ||
| minSeconds: 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be a followup: would prefer to do min: 15s vs minSeconds. This would be more consistent with other agentgateway resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I had to fix json vs. Json, I changed this too.
| config.yaml: | | ||
| config: | ||
| logging: | ||
| format: Json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe agentgateway requires this to be json not Json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it locally and you are correct. Changed it.
I missed `#[serde(rename_all = "camelCase")]` in the agentgateway code. Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
Signed-off-by: David L. Chandler <[email protected]>
|
/merge |
Signed-off-by: David L. Chandler <[email protected]>
Head branch was pushed to by a user without write access
|
/merge |
Description
This is an implementation of a continuation of the API in #13007. It is intended to be followed up with:
kgateway.dev/agentgatewaydisallows GatewayParameters #13054You can see more precisely, and in context, how an end user would use this in test/deployer/testdata/agentgateway-params-primary.yaml or test/deployer/testdata/agentgateway-strategic-merge-patch-out.yaml
See https://github.com/howardjohn/kgateway/compare/api/agwp...chandler-solo:kgateway:chandler/api/agwp?expand=1 to understand what has changed vs. #13007
Relevant text from the description of #13007, highlighting the desire to make it possible to avoid AgentgatewayParameters' RawConfig, lightly edited because this API differs a little, follows:
Change Type
/kind feature
Changelog
Additional Notes