feat(deploy): add Kustomize deployment option - #245
Closed
mkoushni wants to merge 7 commits into
Closed
Conversation
Signed-off-by: mkoushni <mkoushni@redhat.com>
v9 moves the action's runtime from Node 20 to Node 24, per the upstream release notes. Signed-off-by: mkoushni <mkoushni@redhat.com>
golangci-lint flagged plugin_test.go for improper gofmt formatting and an unused waitForTokenPrices helper with no call sites. Signed-off-by: mkoushni <mkoushni@redhat.com>
mkoushni
force-pushed
the
feat/kustomize-deployment
branch
from
July 16, 2026 10:21
f54b42e to
af5b45a
Compare
senanz
suggested changes
Jul 18, 2026
senanz
left a comment
There was a problem hiding this comment.
Review Summary
Good work — the Kustomize structure is well-conceived, the base/overlay separation is correct, documentation is thorough, and the commit hygiene (DCO sign-off, logical separation) is solid. This fills a real gap for GitOps and benchmark integration workflows.
However, there are a few issues to address before merge:
Must-Fix
| # | File | Issue |
|---|---|---|
| 1 | Makefile |
Kustomize download hardcodes linux_amd64 — breaks macOS/ARM |
Should-Fix (production readiness)
| # | File | Issue |
|---|---|---|
| 2 | overlays/istio/envoyfilter.yaml, destinationrule.yaml |
Namespace hardcoded in FQDNs — silently breaks when namespace is changed via the documented method |
| 3 | base/deployment.yaml |
No resource requests/limits |
| 4 | base/deployment.yaml |
No liveness/readiness probes despite exposing health port |
| 5 | base/service.yaml |
Missing port name (needed for Istio protocol detection) |
Non-blocking suggestions
- GKE
timeout: 1sseems aggressive for full-body ext-proc processing insecureSkipVerify: truein DestinationRule should be documented as an explicit trade-off- Consider using
app.kubernetes.io/standard labels instead of bareapp:labels - Mixed concerns across commits (CI bump + lint fixes + feature) — not blocking but worth noting for future PRs
What's done well
- Additive to Helm — no breaking changes to existing path
- README with customization recipes (multi-namespace RBAC, image override, gateway name)
- goconst fixes are appropriately scoped
- Overlay-driven namespace design is the right pattern
Happy to re-review once the critical items are addressed. Nice contribution overall.
…pport The kustomize target hardcoded a linux_amd64 release tarball download, which broke on macOS and ARM dev environments. Use the existing go-install-tool helper (already used for helm, yq, controller-gen) so installation works on any OS/arch supported by the Go toolchain. Signed-off-by: mkoushni <mkoushni@redhat.com>
…ements The EnvoyFilter cluster_name and DestinationRule host embedded the namespace as a hardcoded FQDN segment, requiring manual updates that could silently drift out of sync with the namespace: field in kustomization.yaml. Use Kustomize replacements to derive the namespace from the Service resource and inject it into both fields at build time, so kustomization.yaml's namespace: is the single source of truth. Signed-off-by: mkoushni <mkoushni@redhat.com>
- Add resource requests/limits to the base Deployment so the Pod can't be OOMKilled or starve neighbors in a shared cluster. - Add readiness/liveness probes using the gRPC health protocol served on port 9005 (grpc.health.v1, see cmd/runner/health.go) so rolling updates and self-healing work correctly. - Name the Deployment's container ports and the Service port so Istio (and other mesh sidecars) detect the protocol instead of treating traffic as opaque TCP. - Document why insecureSkipVerify is set on the Istio DestinationRule (IPP serves a self-signed cert with no shared CA) so it isn't mistaken for an oversight or copied onto external-facing hosts. - Raise the GKE GCPRoutingExtension timeout from 1s to 10s, since IPP streams full request/response bodies and runs model-selection/cost logic that can exceed 1s under load. Signed-off-by: mkoushni <mkoushni@redhat.com>
mkoushni
force-pushed
the
feat/kustomize-deployment
branch
from
July 19, 2026 11:37
c4b2a76 to
221f3fb
Compare
Resolves conflicts with upstream's CostGuard epoch-management and requestcostmetadata window/reset changes (PR llm-d#266), which landed alongside this branch's kustomize deployment work.
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
[Feature] Enable deployment using Kustomize
Adds a Kustomize deployment path for the Inference Payload Processor (IPP),
aligning with the llm-d community deployment standard and enabling IPP to be
consumed as a step in llm-d-benchmark
and GitOps workflows that prefer Kustomize over Helm.
The existing Helm chart (
config/charts/payload-processor/) is untouched;Kustomize is an additive alternative.
Fixes #125
Changes
New:
config/kustomize/Each overlay exposes a single
namespace:field as the sole namespace controlpoint — the base layer intentionally omits
metadata.namespaceso that theoverlay drives it.
Modified:
MakefileNew
##@ Deploymentsection:make kustomizekustomizebinary tobin/if absent (viago install, cross-platform)make kustomize-build [KUSTOMIZE_OVERLAY=default|istio|gke]make kustomize-deploy [KUSTOMIZE_OVERLAY=default|istio|gke]make kustomize-undeploy [KUSTOMIZE_OVERLAY=default|istio|gke]Modified:
README.mdAdded a Deployment section with side-by-side Helm and Kustomize quick-start
commands.
Fixed: pre-existing
goconstlint issuesExtracted repeated string literals into named constants to clear
golangci-lint(4 issues, pre-existing):
pkg/config/loader/defaults.goconfigAPIVersion,configKindtest/integration/util.goheaderContentLength,bodyFieldPromptReview Feedback Addressed
Thanks @senanz for the review — two issues are fixed in follow-up commits:
Platform-specific
kustomizeinstall (comment) —the
kustomizeMakefile target hardcoded alinux_amd64release tarballdownload, which broke on macOS/ARM. Switched to
go install sigs.k8s.io/kustomize/kustomize/v5@$(KUSTOMIZE_VERSION)via therepo's existing
go-install-toolhelper (same pattern already used forhelm,yq,controller-gen), which is cross-platform by construction.Namespace drift risk in the Istio overlay (comment) —
the EnvoyFilter
cluster_nameand DestinationRulehostembedded thenamespace as a hardcoded FQDN segment that had to be manually kept in sync
with
namespace:inkustomization.yaml. Implemented the suggested"ideal fix": a Kustomize
replacementsblock that derives the namespacefrom the
Serviceresource and injects it into both fields at build time,so
namespace:is now the single source of truth and can't drift.Verified by rendering the overlay with a custom namespace and confirming
both fields update automatically.
Usage
See
config/kustomize/README.mdfor customizationoptions (namespace, image tag, custom IPP config, multi-namespace RBAC).
Testing
Kustomize build validation
All three overlays render without errors and produce the expected resource kinds:
defaultistiogkeValidated with:
Go test suite
Re-ran the full suite after the review fixes above (
make test-coverage, i.e.go test -race -coverprofile=coverage.out -covermode=atomic ./...). Alltested packages still pass:
pkg/commonpkg/common/envoypkg/common/envoy/testpkg/common/errorpkg/common/observability/loggingpkg/config/loaderpkg/datalayerpkg/datastore/inmemorypkg/framework/interface/datalayerpkg/framework/interface/datalayer/pricingpkg/framework/interface/requesthandlingpkg/framework/plugins/datalayer/modelconfigcollectorpkg/framework/plugins/datalayer/requestcostmetadatapkg/framework/plugins/datalayer/requestmetadatapkg/framework/plugins/modelselector/filter/modelgrouppkg/framework/plugins/modelselector/filter/modelnamepkg/framework/plugins/modelselector/picker/maxscorepkg/framework/plugins/modelselector/picker/randompkg/framework/plugins/modelselector/picker/weightedrandompkg/framework/plugins/modelselector/scorer/costawarepkg/framework/plugins/modelselector/scorer/sessionaffinitypkg/framework/plugins/requesthandling/basemodelextractorpkg/framework/plugins/requesthandling/bodyfieldtoheaderpkg/framework/plugins/requesthandling/modelselectorpkg/framework/plugins/requesthandling/profilepicker/singlepkg/framework/plugins/responsehandling/modelnametoheaderpkg/handlerspkg/metricspkg/modelselectorpkg/servertest/integrationTotal coverage: 60.4% (statements)
Kustomize namespace-drift regression check
Rendered the
istiooverlay with a temporarily patchednamespace: my-namespaceand confirmed both the EnvoyFilter
cluster_nameand DestinationRulehostupdated automatically to
payload-processor.my-namespace.svc.cluster.local.Linting
Release note (write
NONEif no user-facing change):