Feat/persysctl api route+sdk fmt - #31
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Summary
Companion to the persys-gateway routing refactor (see gateway PR link).
That PR changed the gateway to support cluster-scoped routes
(
/clusters/{cluster_id}/...) while also keeping the original flatroutes (
/workloads/...,/nodes/..., etc.) working for backwardcompatibility. This PR makes which shape
persysctltargetsconfigurable, and separately fixes a real bug in
TaintNodethat thegateway refactor surfaced.
New:
api_versionconfigconfig.APIVersion:"v1"(default/unset) or"v2"./workloads/schedule,/nodes/{id}/drain,/cluster/metrics,/forgery/builds/trigger, ...), resolving tothe gateway's default cluster server-side. This is the original
route shape every persys-gateway has always served — existing
configs need zero changes, since unset behaves identically to
before this PR.
(
/clusters/{cluster_id}/workloads/schedule, ...), using the newconfig.ClusterIDfield. Fails fast with a clear error ifapi_versionis"v2"butcluster_idisn't set, rather thansilently hitting the wrong (or no) cluster.
clusterPath(suffix string) (string, error)helper onClient,used by every workload/node/cluster-metrics/forgery HTTP call site.
Not used for
/clusters(lists all clusters, inherentlycluster-agnostic),
/automation/*, or/ai/*— none of those gainedcluster-scoped variants on the gateway side, in either version.
Note:
internal/config.ConfigneedsAPIVersion stringandClusterID stringfields added — seeconfig_additions.go.txtin the patch for the exact struct fields andsuggested CLI flag wiring (
--api-version,--cluster-id). I don'thave visibility into the actual
internal/configfile in this repo, sothat part is a diff to apply by hand rather than a generated patch.
Bug fix:
TaintNoderequest shapeIndependent of API versioning —
TaintNodeRequest.Taintis a nestedproto message (
{key, value, effect}), unlike every siblingnode-management request (
Drain,Undrain,SetNodeLabel, etc.),which have those fields flat at the top level.
taintNodeHTTPwassending a flat body (
{"key":...,"value":...,"effect":...}), whichnever matched the real request shape. Fixed to send
{"taint": {"key":...,"value":...,"effect":...}}.This was likely masked before by the gateway's old hand-written
TaintNodeHandler, which probably did its own field-by-fieldtranslation regardless of body shape; the gateway's new generic
reflection-based dispatch decodes the body directly against the real
proto, so the mismatch is no longer silently absorbed on that end.
Testing
clusterPathend-to-end for both v1 (unchanged behavior,confirmed against a live gateway) and v2 (confirmed cluster-scoped
paths resolve and the missing-
cluster_idfail-fast fires correctly).TaintNodeverified against the corrected gateway(
PathParams-awaregrpcbridge) — this fix depends on that gatewaychange; deploy together, or
TaintNodewill continue to fail againstan un-patched gateway.
Breaking changes
None for existing configs/usage —
api_versionunset behaves exactlyas before.
TaintNodecallers get a request that actually reaches thescheduler correctly, which in practice means "was broken, now works,"
not a behavior change existing code depended on.
Depends on
persys-gateway: dynamic routing +
grpcbridgePathParamsfix (seecompanion PR). Both should land together — this client's v1 flat paths
rely on the gateway's dual v1/v2 mount, and
TaintNoderelies on thegateway's path-param injection fix.