Fix gRPC-tunnel targets never reaching gNMIc, and Prometheus output Service port drift#106
Fix gRPC-tunnel targets never reaching gNMIc, and Prometheus output Service port drift#106zentavr wants to merge 4 commits into
Conversation
…get-matches endpoint instead of relying on the broken bulk /config/apply field
…e port drifting from gNMIc's real listen port
|
The prometheus port drift is nice catch, thanks for that. But I think the operator should not configure the tunnel target matches statically using the config file. gNMIc was missing |
|
@karimra so what's the plan? just wait for gNMIc release (v0.47.0) and that would be self-fixed? |
| @@ -486,6 +485,14 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |||
| r.plans[cluster.Namespace+"/"+cluster.Name] = applyPlan | |||
| r.m.Unlock() | |||
|
|
|||
| // reconcile statefulset (needs applyPlan.TunnelTargetMatches for tunnel-server.targets) | |||
| statefulSet, err := r.reconcileStatefulSet(ctx, &cluster, applyPlan.TunnelTargetMatches) | |||
| if err != nil { | |||
| return ctrl.Result{}, err | |||
| } | |||
|
|
|||
| logger.Info("reconciled cluster statefulset", "replicas", ptr.Deref(statefulSet.Spec.Replicas, 0), "image", statefulSet.Spec.Template.Spec.Containers[0].Image) | |||
|
|
|||
There was a problem hiding this comment.
There is no need to swap these steps order once gnmic properly accepts the tunnel-target-matches (v0.47.0).
Also no need to include tunnel-server.targets in the static config.
| podPlan, ok := distResult.PerPodPlans[podIndex] | ||
| if !ok { | ||
| continue | ||
| // DistributeTargets/boundedLoadRendezvousHash returns a | ||
| // completely empty PerPodPlans map when there are zero static | ||
| // Targets to distribute (tunnel-only deployments have none) -- | ||
| // `continue`-ing here used to skip sendApplyRequest and | ||
| // applyTunnelTargetMatches for every single pod, every single | ||
| // reconcile, silently. Subscriptions/outputs/processors/ | ||
| // tunnel-target-matches apply to every pod regardless of | ||
| // static target distribution, so build the same fallback shape | ||
| // DistributeTargets would have, just with no targets assigned. | ||
| podPlan = &gnmic.ApplyPlan{ | ||
| Targets: make(map[string]*gapi.TargetConfig), | ||
| Subscriptions: plan.Subscriptions, | ||
| Outputs: plan.Outputs, | ||
| Inputs: plan.Inputs, | ||
| Processors: plan.Processors, | ||
| TunnelTargetMatches: plan.TunnelTargetMatches, | ||
| } |
There was a problem hiding this comment.
This is the main (operator-side) bug that prevented tunnel targets from being applied.
Let's keep it, but it would be good to wrap it in separate function.
| // tunnel-target-matches are pushed separately via | ||
| // /api/v1/config/tunnel-target-matches, not through the bulk | ||
| // /api/v1/config/apply request above: gnmic's ConfigApplyRequest. | ||
| // TunnelTargetMatches field has no `mapstructure` tag (only `json`), | ||
| // and the apply handler decodes the request body through | ||
| // mapstructure (not encoding/json) -- mapstructure's default | ||
| // field matching is case-insensitive but hyphen-blind, so the | ||
| // "tunnel-target-matches" JSON key never matches the | ||
| // TunnelTargetMatches field name and is silently dropped on every | ||
| // request regardless of payload content (confirmed by reading | ||
| // gnmic's pkg/collector/api/server/apply.go and | ||
| // decodeRequestMap()). The per-resource endpoint decodes the | ||
| // body directly into config.TunnelTargetMatch, whose own fields | ||
| // (type/id/config) DO have mapstructure tags and aren't | ||
| // hyphenated, so it works correctly. | ||
| if err := r.applyTunnelTargetMatches(ctx, podBaseURL, plan.TunnelTargetMatches, httpClient); err != nil { | ||
| return 0, fmt.Errorf("failed to apply tunnel-target-matches to pod %d: %w", podIndex, err) | ||
| } | ||
|
|
||
| logger.Info("config applied to pod", "pod", podIndex, "targets", len(podPlan.Targets), "tunnelTargetMatches", len(plan.TunnelTargetMatches)) |
There was a problem hiding this comment.
this won't be needed anymore since tunnel-target-matches will be applied by r.sendApplyRequest (gnmic v0.47.0)
|
I put comments on the parts that need to change. The goal is to only use the /config/apply endpoint from the operator so that everything gets applied at the same time, the config stays dynamic (including tunnel-targets) and is applied atomically in each pod. |
2 participants
Fixes #102
Summary
A TunnelTargetPolicy-based tunnel deployment (no static Targets) never worked: the
switch/router's tunnel registration was always rejected, and even after fixing that,
the Prometheus output's Service pointed at the wrong port. Four issues, found in order
while tracing the whole path from TunnelTargetPolicy CRD to gNMIc's running config:
resolved
TunnelTargetPolicy->TunnelTargetMatchbut nothing in the reconcilepath pushed the result anywhere gNMIc could see it.
.spec.match(match-all) rendered as literal empty strings, not a wildcard.The final config.yaml is rendered via
gopkg.in/yaml.v2, which does not honorjsonstruct tags at all -
TunnelTargetMatchonly hadjsontags, so yaml.v2 serializedevery field regardless of zero-value, turning "match everything" into
type: ""/id: "", which gNMIc does not treat as a wildcard./api/v1/config/applysilently dropstunnel-target-matches. gNMIc's ownmapstructure-based decoder for that endpoint is hyphen-blind on this field. Switchedto POSTing each
TunnelTargetMatchindividually to/api/v1/config/tunnel-target-matches, which decodes correctly.applyConfigToPodsskipped every pod when there were zero static Targets.DistributeTargets/boundedLoadRendezvousHashreturns an emptyPerPodPlansmapwhen
len(Targets) == 0(true for any tunnel-only deployment), and the per-pod loopdid
continueon a missing entry -- silently skippingsendApplyRequestand thetunnel-target-matches push for every pod, every reconcile. This meant fixes point 1- point 3
never actually reached a running pod. Build the same fallback shape
DistributeTargetswould have produced (emptyTargets, everything else from theplan) so subscriptions/outputs/processors/tunnel-target-matches still reach every pod.
parseListenPortAndPathdefaulted a Prometheus Output with no explicitlistenfieldto a fabricated port 9804, which
reconcilePrometheusServicesthen treated as anexplicit user override for the generated Service (and its
prometheus.io/portannotation). The port gNMIc actually binds to is computed independently and
unconditionally by
assignPrometheusOutputPorts-- a hash-based assignment across a1009-slot pool starting at 9804, so that multiple Prometheus outputs sharing one
gNMIc process never collide. Whenever the hash didn't happen to land on 9804 itself,
the Service and the container disagreed and scraping silently failed. Return port 0
when
listenisn't set so the caller falls back to the port the plan actuallyassigned.
Testing
gRPC tunnel; confirmed via operator + gNMIc pod logs that registration went from
"target ignored, not matching any rule"(every ~5 min, indefinitely) to a successfulmatch and running subscriptions/outputs.
helpers_test.go) covering the Prometheus listen-portparsing edge cases (config present with no
listenfield, and{}).go build ./...andgo vet ./...clean.