From bf79783a24c5425bb3ed946aa30386e62315aa70 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 14:11:53 -0500 Subject: [PATCH 1/3] fix: surface the analyzer's computed L4Route weights, not the declared spec weight The gateway topology showed spec.rules[].backendRefs[].weight -- the author's declared intent, often a placeholder -- while the weights the analyzer actually computed live in the k8s.f5.com/service-settings annotation on the L4Route, keyed {service: {pod_ip: weight}}. That annotation was parsed NOWHERE in the backend, so the UI displayed the wrong numbers (#8): the reporter saw the topology show one thing while `kubectl get l4route ... service-settings` showed 1/99. _parse_service_settings reads the annotation (tolerating absent/malformed JSON -> {}). _build_backend attaches, per backend service: - analyzerWeights: the {pod_ip: weight} the analyzer computed, or None - effectiveWeight: their sum -- the single number the UI should display -- or None when the analyzer has not weighted this service. The declared `weight` is preserved for backward compatibility and as the fallback the UI uses when effectiveWeight is None. This is the backend half. The frontend must prefer effectiveWeight (then analyzerWeights, then weight) to actually change what the user sees; that consumer change and cluster validation are called out in the PR. Filed as the root-cause fix: the correct data was simply never surfaced. Fixes #8 Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/services/bnk/topology.py | 78 ++++++++++++++++++++++--- backend/tests/unit/test_bnk_topology.py | 56 ++++++++++++++++++ 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/backend/services/bnk/topology.py b/backend/services/bnk/topology.py index 1a4db70..e4765ab 100644 --- a/backend/services/bnk/topology.py +++ b/backend/services/bnk/topology.py @@ -9,6 +9,8 @@ and returns a structured topology dict. """ +import json +import logging import re from typing import Any @@ -39,6 +41,8 @@ # --------------------------------------------------------------------------- +logger = logging.getLogger(__name__) + def analyze_topology(data: dict[str, Any]) -> dict[str, Any]: """Build the gateway topology response from raw BNK data.""" resources = data["resources"] @@ -187,6 +191,14 @@ def _match_routes_to_listener( route_spec = route.get("spec", {}) route_ns = route_meta.get("namespace", "") + # The analyzer records the weights it actually computed per backend + # service in the k8s.f5.com/service-settings annotation, NOT in + # spec.rules[].backendRefs[].weight (which is the author's declared + # intent, often a placeholder). The topology surfaced only the declared + # weight, so the UI showed the wrong numbers (#8). Parse the annotation + # once per route and attach the analyzer weight to each backend. + analyzer_weights = _parse_service_settings(route_meta) + for parent in route_spec.get("parentRefs", []): parent_ns = parent.get("namespace", route_ns) if parent.get("name") != gw_name or parent_ns != gw_ns: @@ -196,14 +208,7 @@ def _match_routes_to_listener( continue backends = [ - { - "name": br.get("name", ""), - "namespace": br.get("namespace"), - "port": br.get("port"), - "weight": br.get("weight"), - "kind": br.get("kind", "Service"), - "group": br.get("group", ""), - } + _build_backend(br, analyzer_weights) for rule in route_spec.get("rules", []) for br in rule.get("backendRefs", []) ] @@ -223,6 +228,63 @@ def _match_routes_to_listener( return routes_data +SERVICE_SETTINGS_ANNOTATION = "k8s.f5.com/service-settings" + + +def _parse_service_settings(route_meta: dict) -> dict[str, dict[str, int]]: + """Analyzer-computed weights from the k8s.f5.com/service-settings annotation. + + Shape (from a live L4Route): ``{service_name: {pod_ip: weight}}`` -- e.g. + ``{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}}``. Returns {} when the + annotation is absent or unparseable; the caller then falls back to the + declared weight, so a route the analyzer has not yet processed still shows + something. + """ + raw = (route_meta.get("annotations") or {}).get(SERVICE_SETTINGS_ANNOTATION) + if not raw: + return {} + try: + parsed = json.loads(raw) + except (ValueError, TypeError): + logger.warning("Unparseable %s on route %s", SERVICE_SETTINGS_ANNOTATION, + route_meta.get("name")) + return {} + if not isinstance(parsed, dict): + return {} + # Keep only the well-formed {service: {ip: int}} entries. + out: dict[str, dict[str, int]] = {} + for service, ip_weights in parsed.items(): + if isinstance(ip_weights, dict): + clean = {ip: w for ip, w in ip_weights.items() if isinstance(w, int)} + if clean: + out[str(service)] = clean + return out + + +def _build_backend(br: dict, analyzer_weights: dict[str, dict[str, int]]) -> dict: + """One backendRef, with the analyzer's computed weight surfaced. + + ``weight`` remains the DECLARED value for backward compatibility. The + analyzer's per-pod weights for this backend's service are added as + ``analyzerWeights`` ({pod_ip: weight}), and ``effectiveWeight`` collapses + them to the single number the UI should show -- the sum across the + service's pods, or None when the analyzer has not weighted this service + (in which case the UI should fall back to the declared ``weight``). + """ + name = br.get("name", "") + per_pod = analyzer_weights.get(name) + return { + "name": name, + "namespace": br.get("namespace"), + "port": br.get("port"), + "weight": br.get("weight"), + "analyzerWeights": per_pod, + "effectiveWeight": (sum(per_pod.values()) if per_pod else None), + "kind": br.get("kind", "Service"), + "group": br.get("group", ""), + } + + def _match_analyzers( analyzers: list[dict], route_name: str, diff --git a/backend/tests/unit/test_bnk_topology.py b/backend/tests/unit/test_bnk_topology.py index 6daaec6..fbc3c36 100644 --- a/backend/tests/unit/test_bnk_topology.py +++ b/backend/tests/unit/test_bnk_topology.py @@ -8,6 +8,7 @@ import pytest from services.bnk.topology import ( + _build_backend, _build_cne_instance, _build_data_plane, _build_egress, @@ -16,6 +17,7 @@ _match_net_policies, _match_routes_to_listener, _match_sec_policies, + _parse_service_settings, analyze_topology, resolve_list_refs, ) @@ -225,6 +227,60 @@ def test_route_not_matching_different_gateway(self): assert result == [] +# --------------------------------------------------------------------------- +# analyzer weights (#8) — k8s.f5.com/service-settings annotation +# --------------------------------------------------------------------------- + + +def _l4route_with_settings(name, gw_name, gw_ns, backends, settings_json=None): + r = _resource(name, gw_ns, spec={ + "parentRefs": [{"name": gw_name, "namespace": gw_ns}], + "rules": [{"backendRefs": backends}], + }) + if settings_json is not None: + r["metadata"]["annotations"] = {"k8s.f5.com/service-settings": settings_json} + return r + + +class TestAnalyzerWeights: + def test_parse_service_settings_returns_per_service_pod_weights(self): + meta = {"name": "r", "annotations": {"k8s.f5.com/service-settings": + '{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99},' + ' "p-vlm-vllm-agg-vlmfrontend": {"10.244.139.205": 1}}'}} + parsed = _parse_service_settings(meta) + assert parsed["vlm-vllm-agg-vlmfrontend"] == {"10.244.123.12": 99} + assert parsed["p-vlm-vllm-agg-vlmfrontend"] == {"10.244.139.205": 1} + + def test_parse_missing_or_bad_annotation_is_empty(self): + assert _parse_service_settings({"name": "r"}) == {} + assert _parse_service_settings({"annotations": {"k8s.f5.com/service-settings": "not json"}}) == {} + assert _parse_service_settings({"annotations": {"k8s.f5.com/service-settings": "[1,2]"}}) == {} + + def test_build_backend_surfaces_analyzer_weight_over_declared(self): + weights = {"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}} + # Declared weight is 1; the analyzer computed 99. effectiveWeight is 99. + b = _build_backend({"name": "vlm-vllm-agg-vlmfrontend", "port": 8000, "weight": 1}, weights) + assert b["weight"] == 1 # declared, preserved + assert b["analyzerWeights"] == {"10.244.123.12": 99} + assert b["effectiveWeight"] == 99 + + def test_build_backend_without_analyzer_data_falls_back(self): + b = _build_backend({"name": "unweighted-svc", "port": 80, "weight": 50}, {}) + assert b["analyzerWeights"] is None + assert b["effectiveWeight"] is None # UI falls back to `weight` + + def test_route_backends_carry_analyzer_weights_end_to_end(self): + route = _l4route_with_settings( + "l4route-vlm-internal-gw1", "gw", "f5-bnk", + backends=[{"name": "vlm-vllm-agg-vlmfrontend", "port": 8000, "weight": 1}], + settings_json='{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}}', + ) + result = _match_routes_to_listener([(route, "L4Route")], [], "gw", "f5-bnk", "l4") + assert len(result) == 1 + backend = result[0]["backends"][0] + assert backend["effectiveWeight"] == 99, "the displayed weight must come from the analyzer, not the spec" + + # --------------------------------------------------------------------------- # _match_analyzers # --------------------------------------------------------------------------- From 5b362099a5f961feae7181af201100d0c77659e1 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 14:56:14 -0500 Subject: [PATCH 2/3] fix: display the analyzer's computed weight in both BNK backend views (#8) The backend change (prior commit) put analyzerWeights/effectiveWeight on each topology backend dict, but nothing consumed them yet, so the UI still showed the declared spec weight -- the reporter's exact symptom. This completes the chain so the user actually sees the analyzer's number. Backend: - build_route_ref_map (helpers.py) now carries effectiveWeight and analyzerWeights through from the topology backend into each route ref, so the backends-collection view -- a second surface built from the same dicts -- gets them too. Present-and-None when there's no annotation, so the UI can tell "analyzer said nothing" from "analyzer said 0" and fall back cleanly. Frontend, both weight-rendering surfaces prefer effectiveWeight ?? weight: - F5BNKTopologyViewer route backends: shows the analyzer value (badged 'info' to distinguish it), falling back to the declared weight when absent. - BackendsCollection route refs: same preference, keeping the existing hide-the-default-of-1 behaviour. - TopologyRouteBackend / BnkBackendRouteRef / the viewer's local TopologyBackend all gain effectiveWeight?/analyzerWeights?. - TrafficFlowOverview reads only backend name/namespace, so it needed no change. Tests (all non-vacuous -- verified failing against the unpatched code): - helpers: map propagates effectiveWeight/analyzerWeights; absent -> None. - viewer: analyzer effectiveWeight (99) shown over declared (1); declared value stands when no analyzer weight. Reverting just the viewer render makes the first fail (renders 'weight 1'), confirming the assertion bites. Still wants one apply against a cluster with weighted L4Routes (the reporter's dynamo-system) to confirm the numbers now match the service-settings annotation end to end -- the tests prove the data flows, not that the analyzer emits what we assume. Fixes #8 Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/services/bnk/helpers.py | 8 ++ backend/tests/unit/test_bnk_backends.py | 30 ++++++ .../src/components/k8s/BackendsCollection.tsx | 19 ++-- .../components/k8s/F5BNKTopologyViewer.tsx | 19 +++- .../__tests__/F5BNKTopologyViewer.test.tsx | 98 +++++++++++++++++++ frontend-v2/src/types/f5bnk.ts | 13 ++- 6 files changed, 176 insertions(+), 11 deletions(-) diff --git a/backend/services/bnk/helpers.py b/backend/services/bnk/helpers.py index 77b4f37..e5eb00d 100644 --- a/backend/services/bnk/helpers.py +++ b/backend/services/bnk/helpers.py @@ -187,6 +187,14 @@ def build_route_ref_map(topology: list[dict]) -> dict[tuple[str, str], list[dict "listenerName": listener.get("name", ""), "port": backend.get("port"), "weight": backend.get("weight"), + # Analyzer-computed weights, carried through from the + # topology backend (topology._build_backend). The backends + # view must prefer effectiveWeight over the declared weight + # for the same reason the topology tree does -- otherwise + # this second surface shows the spec value the analyzer + # overrode (#8). None when the annotation didn't weight it. + "effectiveWeight": backend.get("effectiveWeight"), + "analyzerWeights": backend.get("analyzerWeights"), }) return ref_map diff --git a/backend/tests/unit/test_bnk_backends.py b/backend/tests/unit/test_bnk_backends.py index b205668..ed1b197 100644 --- a/backend/tests/unit/test_bnk_backends.py +++ b/backend/tests/unit/test_bnk_backends.py @@ -93,6 +93,36 @@ def test_multiple_routes_same_backend(self): ref_map = _build_route_ref_map(topology) assert len(ref_map[("ns", "svc")]) == 2 + def test_propagates_analyzer_weights_from_backend(self): + """#8: the backends view must carry effectiveWeight/analyzerWeights so it + can prefer the analyzer's number over the declared spec weight, exactly + as the topology tree does. topology._build_backend puts them on the + backend dict; this map must not drop them.""" + topology = [{ + "name": "gw", + "listeners": [{"name": "http", "routes": [{ + "name": "r1", "namespace": "ns", "kind": "L4Route", + "backends": [{ + "name": "svc", "namespace": "ns", "port": 80, + "weight": 1, # declared spec weight + "effectiveWeight": 99, # what the analyzer computed + "analyzerWeights": {"10.1.2.3": 99}, + }], + }]}], + }] + ref = _build_route_ref_map(topology)[("ns", "svc")][0] + assert ref["weight"] == 1 # declared preserved + assert ref["effectiveWeight"] == 99 # analyzer value carried + assert ref["analyzerWeights"] == {"10.1.2.3": 99} + + def test_absent_analyzer_weights_carry_through_as_none(self): + """No annotation -> the keys are present and None, so the UI falls back + to the declared weight rather than seeing undefined.""" + topology = _topology_with_route("svc-1") + ref = _build_route_ref_map(topology)[("f5-bnk", "svc-1")][0] + assert ref["effectiveWeight"] is None + assert ref["analyzerWeights"] is None + # --------------------------------------------------------------------------- # analyze_backends diff --git a/frontend-v2/src/components/k8s/BackendsCollection.tsx b/frontend-v2/src/components/k8s/BackendsCollection.tsx index 3f553df..b79be12 100644 --- a/frontend-v2/src/components/k8s/BackendsCollection.tsx +++ b/frontend-v2/src/components/k8s/BackendsCollection.tsx @@ -185,12 +185,19 @@ function BackendRow({ port {ref.port} )} - {ref.weight != null && ref.weight !== 1 && ( - <> - | - weight {ref.weight} - - )} + {(() => { + // Prefer the analyzer's computed weight over the declared spec + // weight; fall back to `weight` when the analyzer didn't weight + // this backend (#8). Hide the default of 1 as before. + const shownWeight = ref.effectiveWeight ?? ref.weight; + if (shownWeight == null || shownWeight === 1) return null; + return ( + <> + | + weight {shownWeight} + + ); + })()} ))} diff --git a/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx b/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx index 3f9c23e..2007d87 100644 --- a/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx +++ b/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx @@ -52,7 +52,12 @@ interface TopologyBackend { name: string; namespace?: string | null; port: number | null; - weight: number | null; + weight: number | null; // declared spec weight + // Analyzer-computed weights (k8s.f5.com/service-settings). effectiveWeight is + // the number to show; null when the analyzer didn't weight this backend, so + // fall back to the declared `weight` (#8). + effectiveWeight?: number | null; + analyzerWeights?: Record | null; kind?: string; group?: string; } @@ -603,8 +608,16 @@ export function F5BNKTopologyViewer({ clusterId, namespace, onSelectResource }: if (isCustomKind) { badges.push({ text: be.kind!, variant: 'outline' }); } - if (be.weight) { - badges.push({ text: `weight ${be.weight}` }); + // Prefer the analyzer's computed weight over the declared + // spec weight; fall back to `weight` when the analyzer didn't + // weight this backend (#8). + const shownWeight = be.effectiveWeight ?? be.weight; + if (shownWeight) { + const fromAnalyzer = be.effectiveWeight != null; + badges.push({ + text: `weight ${shownWeight}`, + ...(fromAnalyzer ? { variant: 'info' as const } : {}), + }); } return ( { expect(screen.getByText('f5-bnk')).toBeInTheDocument(); }); + it('shows the analyzer effectiveWeight over the declared spec weight (#8)', async () => { + server.use( + http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { + return HttpResponse.json({ + ...emptyBnkData, + topology: [{ + name: 'bnk-gateway', + namespace: 'bnk-demo', + gatewayClassName: 'f5-bnk', + addresses: ['10.1.1.100'], + listeners: [{ + name: 'l4', + protocol: 'TCP', + port: 8080, + networkPolicies: [], + routes: [{ + name: 'dynamo-route', + namespace: 'dynamo-system', + kind: 'L4Route', + hostnames: [], + analyzers: [], + backends: [{ + name: 'dynamo-svc', + namespace: 'dynamo-system', + port: 8080, + weight: 1, // declared spec weight the old code showed + effectiveWeight: 99, // what the analyzer actually computed + analyzerWeights: { '10.1.2.3': 99 }, + }], + }], + }], + securityPolicies: [], + }], + topologyCounts: { ...emptyBnkData.topologyCounts, gateways: 1, listeners: 1 }, + }); + }) + ); + + const user = userEvent.setup(); + render(); + + // Listener is open by default; expand the route to reveal its backends. + await user.click(await screen.findByText('dynamo-route')); + + expect(await screen.findByText('dynamo-svc')).toBeInTheDocument(); + // The analyzer's 99, not the declared 1. + expect(screen.getByText('weight 99')).toBeInTheDocument(); + expect(screen.queryByText('weight 1')).not.toBeInTheDocument(); + }); + + it('falls back to the declared weight when no analyzer weight is present (#8)', async () => { + server.use( + http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { + return HttpResponse.json({ + ...emptyBnkData, + topology: [{ + name: 'bnk-gateway', + namespace: 'bnk-demo', + gatewayClassName: 'f5-bnk', + addresses: ['10.1.1.100'], + listeners: [{ + name: 'l4', + protocol: 'TCP', + port: 8080, + networkPolicies: [], + routes: [{ + name: 'plain-route', + namespace: 'bnk-demo', + kind: 'HTTPRoute', + hostnames: [], + analyzers: [], + backends: [{ + name: 'plain-svc', + namespace: 'bnk-demo', + port: 80, + weight: 42, // no annotation -> declared value stands + effectiveWeight: null, + analyzerWeights: null, + }], + }], + }], + securityPolicies: [], + }], + topologyCounts: { ...emptyBnkData.topologyCounts, gateways: 1, listeners: 1 }, + }); + }) + ); + + const user = userEvent.setup(); + render(); + + await user.click(await screen.findByText('plain-route')); + + expect(await screen.findByText('plain-svc')).toBeInTheDocument(); + expect(screen.getByText('weight 42')).toBeInTheDocument(); + }); + it('shows error state on failure', async () => { server.use( http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { diff --git a/frontend-v2/src/types/f5bnk.ts b/frontend-v2/src/types/f5bnk.ts index 5cae9c9..b36dd9a 100644 --- a/frontend-v2/src/types/f5bnk.ts +++ b/frontend-v2/src/types/f5bnk.ts @@ -371,7 +371,12 @@ export interface TopologyRouteBackend { name: string; namespace?: string | null; port: number | null; - weight: number | null; + weight: number | null; // declared spec weight (backendRefs[].weight) + // Analyzer-computed weights from the k8s.f5.com/service-settings annotation. + // effectiveWeight is the number the UI should show; it's null when the + // analyzer didn't weight this service, in which case fall back to `weight` (#8). + effectiveWeight?: number | null; + analyzerWeights?: Record | null; // {pod_ip: weight} kind?: string; // "Service" (default), "ServiceImport", etc. group?: string; // "" = core API group } @@ -570,7 +575,11 @@ export interface BnkBackendRouteRef { gatewayName: string; listenerName: string; port?: number | null; - weight?: number | null; + weight?: number | null; // declared spec weight + // Carried through from the topology backend so the backends view prefers the + // analyzer's number over the declared weight, same as the topology tree (#8). + effectiveWeight?: number | null; + analyzerWeights?: Record | null; } export interface BnkBackendEntry { From 2a135581eedd9e3870a2da5fc405edfe93d4fed6 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 15:15:44 -0500 Subject: [PATCH 3/3] review: back out the guessed effectiveWeight; parse service-settings faithfully mwiget is right, on all three points, and the catch matters: this repo already reads k8s.f5.com/service-settings in F5AIAnalyzerViewer, grounded in F5 docs, and it reads it differently. My "parsed nowhere in the backend" was true but carried the whole design on a qualifier. The three disagreements, each a silent fallback that looks like the bug still being unfixed: 1. Top-level key is a POOL (docs: {"pool-3": {ip: w}}), not a Service. My per-backendRef lookup keyed by service name, so in production every lookup would miss, effectiveWeight would always be None, and the UI would quietly show the declared weight again -- a fix that closes the issue while doing nothing. The tests passed only because they used service-name keys, i.e. the same wrong assumption under test. 2. The per-pod weights in the docs example sum to 100 within one pool -- a within-pool distribution, not a between-service share. sum() would then render "weight 100 / weight 100": a more convincing wrong answer. 3. isinstance(int) dropped floats/numeric-strings the existing Number(w) reader accepts -- another silent fallback. None of that is settleable without a real multi-backend/multi-pod cluster sample. So I'm not shipping the guess: - Removed effectiveWeight, the per-backend attribution, _build_backend, the helpers.py propagation, the frontend render swaps, and the type fields. - _parse_service_settings now parses the annotation FAITHFULLY: pool-keyed structure preserved, weights coerced tolerantly (int/float/numeric-string via _coerce_weight, matching F5AIAnalyzerViewer's Number(w); bool excluded). - The parsed annotation rides on the route as `serviceSettings` (the same shape the existing consumer reads), available for a validated consumer without another backend change. Backends keep the DECLARED weight -- unchanged on screen. This no longer changes the displayed weight, so it does not close #8; the on-screen fix needs the cluster sample (dump the raw annotation from a route with >=2 weighted backends across multiple pods) to settle key->service mapping and the weight semantic. Reframed on the PR. Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- backend/services/bnk/helpers.py | 8 -- backend/services/bnk/topology.py | 106 ++++++++++-------- backend/tests/unit/test_bnk_backends.py | 30 ----- backend/tests/unit/test_bnk_topology.py | 71 ++++++++---- .../src/components/k8s/BackendsCollection.tsx | 19 +--- .../components/k8s/F5BNKTopologyViewer.tsx | 19 +--- .../__tests__/F5BNKTopologyViewer.test.tsx | 98 ---------------- frontend-v2/src/types/f5bnk.ts | 13 +-- 8 files changed, 122 insertions(+), 242 deletions(-) diff --git a/backend/services/bnk/helpers.py b/backend/services/bnk/helpers.py index e5eb00d..77b4f37 100644 --- a/backend/services/bnk/helpers.py +++ b/backend/services/bnk/helpers.py @@ -187,14 +187,6 @@ def build_route_ref_map(topology: list[dict]) -> dict[tuple[str, str], list[dict "listenerName": listener.get("name", ""), "port": backend.get("port"), "weight": backend.get("weight"), - # Analyzer-computed weights, carried through from the - # topology backend (topology._build_backend). The backends - # view must prefer effectiveWeight over the declared weight - # for the same reason the topology tree does -- otherwise - # this second surface shows the spec value the analyzer - # overrode (#8). None when the annotation didn't weight it. - "effectiveWeight": backend.get("effectiveWeight"), - "analyzerWeights": backend.get("analyzerWeights"), }) return ref_map diff --git a/backend/services/bnk/topology.py b/backend/services/bnk/topology.py index e4765ab..bb77390 100644 --- a/backend/services/bnk/topology.py +++ b/backend/services/bnk/topology.py @@ -191,13 +191,19 @@ def _match_routes_to_listener( route_spec = route.get("spec", {}) route_ns = route_meta.get("namespace", "") - # The analyzer records the weights it actually computed per backend - # service in the k8s.f5.com/service-settings annotation, NOT in - # spec.rules[].backendRefs[].weight (which is the author's declared - # intent, often a placeholder). The topology surfaced only the declared - # weight, so the UI showed the wrong numbers (#8). Parse the annotation - # once per route and attach the analyzer weight to each backend. - analyzer_weights = _parse_service_settings(route_meta) + # The analyzer records the weights it actually computed in the + # k8s.f5.com/service-settings annotation, NOT in + # spec.rules[].backendRefs[].weight (the author's declared intent). We + # surface the annotation faithfully at the route level as + # ``serviceSettings`` -- the same pool-keyed shape F5AIAnalyzerViewer + # already reads -- so a validated consumer can reinterpret it without + # another backend change (#8). We deliberately do NOT collapse it into a + # per-backendRef ``effectiveWeight`` here: the annotation is keyed by + # POOL, a backendRef is a Service, and the per-pod weights may be a + # within-pool distribution rather than a between-service share -- so the + # pool->service attribution and the collapse both need a real + # multi-backend/multi-pod cluster sample to settle (see PR discussion). + service_settings = _parse_service_settings(route_meta) for parent in route_spec.get("parentRefs", []): parent_ns = parent.get("namespace", route_ns) @@ -208,7 +214,14 @@ def _match_routes_to_listener( continue backends = [ - _build_backend(br, analyzer_weights) + { + "name": br.get("name", ""), + "namespace": br.get("namespace"), + "port": br.get("port"), + "weight": br.get("weight"), + "kind": br.get("kind", "Service"), + "group": br.get("group", ""), + } for rule in route_spec.get("rules", []) for br in rule.get("backendRefs", []) ] @@ -220,6 +233,7 @@ def _match_routes_to_listener( "kind": route_kind, "hostnames": route_spec.get("hostnames", []), "backends": backends, + "serviceSettings": service_settings or None, "analyzers": _match_analyzers( analyzers, route_name, route_kind, route_ns, gw_ns, ), @@ -231,14 +245,39 @@ def _match_routes_to_listener( SERVICE_SETTINGS_ANNOTATION = "k8s.f5.com/service-settings" -def _parse_service_settings(route_meta: dict) -> dict[str, dict[str, int]]: - """Analyzer-computed weights from the k8s.f5.com/service-settings annotation. +def _coerce_weight(value: Any) -> float | None: + """Tolerant numeric coercion, matching F5AIAnalyzerViewer's ``Number(w)``. - Shape (from a live L4Route): ``{service_name: {pod_ip: weight}}`` -- e.g. - ``{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}}``. Returns {} when the - annotation is absent or unparseable; the caller then falls back to the - declared weight, so a route the analyzer has not yet processed still shows - something. + The annotation's weights arrive as ints in the samples we have, but the + existing frontend consumer accepts floats and numeric strings too, and the + two readers of this annotation must not disagree on what counts as a weight + (a stricter reader silently drops values and falls back to the declared + weight -- invisibly). bool is excluded: it is an int subclass but never a + weight. + """ + if isinstance(value, bool): + return None + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value) + except ValueError: + return None + return None + + +def _parse_service_settings(route_meta: dict) -> dict[str, dict[str, float]]: + """Faithful parse of the k8s.f5.com/service-settings annotation. + + Per F5 docs the shape is ``{pool_name: {pod_ip: weight}}`` -- e.g. + ``{"pool-3": {"10.244.114.53": 33, "10.244.114.54": 34, "10.244.99.91": 33}}``. + The top-level key is a POOL, not necessarily a Service name; the per-pod + weights may be a within-pool distribution. We return the structure as-is + (keys preserved, weights coerced tolerantly) rather than interpreting it, + because the interpretation is exactly what's unsettled (#8) and the existing + consumer, F5AIAnalyzerViewer, already reads the same annotation this way. + Returns {} when the annotation is absent or unparseable. """ raw = (route_meta.get("annotations") or {}).get(SERVICE_SETTINGS_ANNOTATION) if not raw: @@ -251,40 +290,19 @@ def _parse_service_settings(route_meta: dict) -> dict[str, dict[str, int]]: return {} if not isinstance(parsed, dict): return {} - # Keep only the well-formed {service: {ip: int}} entries. - out: dict[str, dict[str, int]] = {} - for service, ip_weights in parsed.items(): + out: dict[str, dict[str, float]] = {} + for pool, ip_weights in parsed.items(): if isinstance(ip_weights, dict): - clean = {ip: w for ip, w in ip_weights.items() if isinstance(w, int)} + clean = {} + for ip, w in ip_weights.items(): + cw = _coerce_weight(w) + if cw is not None: + clean[str(ip)] = cw if clean: - out[str(service)] = clean + out[str(pool)] = clean return out -def _build_backend(br: dict, analyzer_weights: dict[str, dict[str, int]]) -> dict: - """One backendRef, with the analyzer's computed weight surfaced. - - ``weight`` remains the DECLARED value for backward compatibility. The - analyzer's per-pod weights for this backend's service are added as - ``analyzerWeights`` ({pod_ip: weight}), and ``effectiveWeight`` collapses - them to the single number the UI should show -- the sum across the - service's pods, or None when the analyzer has not weighted this service - (in which case the UI should fall back to the declared ``weight``). - """ - name = br.get("name", "") - per_pod = analyzer_weights.get(name) - return { - "name": name, - "namespace": br.get("namespace"), - "port": br.get("port"), - "weight": br.get("weight"), - "analyzerWeights": per_pod, - "effectiveWeight": (sum(per_pod.values()) if per_pod else None), - "kind": br.get("kind", "Service"), - "group": br.get("group", ""), - } - - def _match_analyzers( analyzers: list[dict], route_name: str, diff --git a/backend/tests/unit/test_bnk_backends.py b/backend/tests/unit/test_bnk_backends.py index ed1b197..b205668 100644 --- a/backend/tests/unit/test_bnk_backends.py +++ b/backend/tests/unit/test_bnk_backends.py @@ -93,36 +93,6 @@ def test_multiple_routes_same_backend(self): ref_map = _build_route_ref_map(topology) assert len(ref_map[("ns", "svc")]) == 2 - def test_propagates_analyzer_weights_from_backend(self): - """#8: the backends view must carry effectiveWeight/analyzerWeights so it - can prefer the analyzer's number over the declared spec weight, exactly - as the topology tree does. topology._build_backend puts them on the - backend dict; this map must not drop them.""" - topology = [{ - "name": "gw", - "listeners": [{"name": "http", "routes": [{ - "name": "r1", "namespace": "ns", "kind": "L4Route", - "backends": [{ - "name": "svc", "namespace": "ns", "port": 80, - "weight": 1, # declared spec weight - "effectiveWeight": 99, # what the analyzer computed - "analyzerWeights": {"10.1.2.3": 99}, - }], - }]}], - }] - ref = _build_route_ref_map(topology)[("ns", "svc")][0] - assert ref["weight"] == 1 # declared preserved - assert ref["effectiveWeight"] == 99 # analyzer value carried - assert ref["analyzerWeights"] == {"10.1.2.3": 99} - - def test_absent_analyzer_weights_carry_through_as_none(self): - """No annotation -> the keys are present and None, so the UI falls back - to the declared weight rather than seeing undefined.""" - topology = _topology_with_route("svc-1") - ref = _build_route_ref_map(topology)[("f5-bnk", "svc-1")][0] - assert ref["effectiveWeight"] is None - assert ref["analyzerWeights"] is None - # --------------------------------------------------------------------------- # analyze_backends diff --git a/backend/tests/unit/test_bnk_topology.py b/backend/tests/unit/test_bnk_topology.py index fbc3c36..ebb6556 100644 --- a/backend/tests/unit/test_bnk_topology.py +++ b/backend/tests/unit/test_bnk_topology.py @@ -8,11 +8,11 @@ import pytest from services.bnk.topology import ( - _build_backend, _build_cne_instance, _build_data_plane, _build_egress, _build_vlan, + _coerce_weight, _match_analyzers, _match_net_policies, _match_routes_to_listener, @@ -242,43 +242,70 @@ def _l4route_with_settings(name, gw_name, gw_ns, backends, settings_json=None): return r -class TestAnalyzerWeights: - def test_parse_service_settings_returns_per_service_pod_weights(self): +class TestServiceSettingsParsing: + """Faithful, tolerant parse of k8s.f5.com/service-settings. + + We surface the annotation as-is (pool-keyed, weights coerced), matching how + F5AIAnalyzerViewer already reads it. We deliberately do NOT collapse it into + a per-backend effectiveWeight here: the top-level key is a POOL (not + necessarily a Service), and whether the per-pod weights are a within-pool + distribution or a between-service share is unsettled without a real + multi-backend/multi-pod cluster sample (#8). + """ + + def test_parse_preserves_pool_keyed_structure(self): + # The F5-docs shape: top-level key is a pool, value is {pod_ip: weight}. meta = {"name": "r", "annotations": {"k8s.f5.com/service-settings": - '{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99},' - ' "p-vlm-vllm-agg-vlmfrontend": {"10.244.139.205": 1}}'}} + '{"pool-3": {"10.244.114.53": 33, "10.244.114.54": 34, "10.244.99.91": 33}}'}} parsed = _parse_service_settings(meta) - assert parsed["vlm-vllm-agg-vlmfrontend"] == {"10.244.123.12": 99} - assert parsed["p-vlm-vllm-agg-vlmfrontend"] == {"10.244.139.205": 1} + assert parsed == {"pool-3": {"10.244.114.53": 33.0, "10.244.114.54": 34.0, "10.244.99.91": 33.0}} def test_parse_missing_or_bad_annotation_is_empty(self): assert _parse_service_settings({"name": "r"}) == {} assert _parse_service_settings({"annotations": {"k8s.f5.com/service-settings": "not json"}}) == {} assert _parse_service_settings({"annotations": {"k8s.f5.com/service-settings": "[1,2]"}}) == {} - def test_build_backend_surfaces_analyzer_weight_over_declared(self): - weights = {"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}} - # Declared weight is 1; the analyzer computed 99. effectiveWeight is 99. - b = _build_backend({"name": "vlm-vllm-agg-vlmfrontend", "port": 8000, "weight": 1}, weights) - assert b["weight"] == 1 # declared, preserved - assert b["analyzerWeights"] == {"10.244.123.12": 99} - assert b["effectiveWeight"] == 99 - - def test_build_backend_without_analyzer_data_falls_back(self): - b = _build_backend({"name": "unweighted-svc", "port": 80, "weight": 50}, {}) - assert b["analyzerWeights"] is None - assert b["effectiveWeight"] is None # UI falls back to `weight` + def test_numeric_coercion_matches_the_existing_consumer(self): + # F5AIAnalyzerViewer accepts Number(w): int, float, and numeric strings. + # A stricter reader would silently drop these and fall back to the + # declared weight -- the exact invisible failure mode to avoid. + assert _coerce_weight(99) == 99.0 + assert _coerce_weight(33.5) == 33.5 + assert _coerce_weight("42") == 42.0 + assert _coerce_weight("not-a-number") is None + assert _coerce_weight(True) is None # bool is not a weight + assert _coerce_weight(None) is None + + def test_parse_coerces_float_and_string_weights(self): + meta = {"name": "r", "annotations": {"k8s.f5.com/service-settings": + '{"pool-1": {"10.0.0.1": "50", "10.0.0.2": 50.0}}'}} + parsed = _parse_service_settings(meta) + assert parsed == {"pool-1": {"10.0.0.1": 50.0, "10.0.0.2": 50.0}} - def test_route_backends_carry_analyzer_weights_end_to_end(self): + def test_route_carries_raw_service_settings_end_to_end(self): + # The parsed annotation rides on the route as serviceSettings; backends + # keep the DECLARED weight (the on-screen collapse is deferred to cluster + # validation, so we do not overwrite it with a guess). route = _l4route_with_settings( "l4route-vlm-internal-gw1", "gw", "f5-bnk", backends=[{"name": "vlm-vllm-agg-vlmfrontend", "port": 8000, "weight": 1}], - settings_json='{"vlm-vllm-agg-vlmfrontend": {"10.244.123.12": 99}}', + settings_json='{"pool-3": {"10.244.123.12": 99}}', ) result = _match_routes_to_listener([(route, "L4Route")], [], "gw", "f5-bnk", "l4") assert len(result) == 1 + assert result[0]["serviceSettings"] == {"pool-3": {"10.244.123.12": 99.0}} + # Declared weight preserved; no guessed effectiveWeight on the backend. backend = result[0]["backends"][0] - assert backend["effectiveWeight"] == 99, "the displayed weight must come from the analyzer, not the spec" + assert backend["weight"] == 1 + assert "effectiveWeight" not in backend + + def test_route_without_annotation_has_none_service_settings(self): + route = _l4route_with_settings( + "plain", "gw", "f5-bnk", + backends=[{"name": "svc", "port": 80, "weight": 50}], + ) + result = _match_routes_to_listener([(route, "HTTPRoute")], [], "gw", "f5-bnk", "l4") + assert result[0]["serviceSettings"] is None # --------------------------------------------------------------------------- diff --git a/frontend-v2/src/components/k8s/BackendsCollection.tsx b/frontend-v2/src/components/k8s/BackendsCollection.tsx index b79be12..3f553df 100644 --- a/frontend-v2/src/components/k8s/BackendsCollection.tsx +++ b/frontend-v2/src/components/k8s/BackendsCollection.tsx @@ -185,19 +185,12 @@ function BackendRow({ port {ref.port} )} - {(() => { - // Prefer the analyzer's computed weight over the declared spec - // weight; fall back to `weight` when the analyzer didn't weight - // this backend (#8). Hide the default of 1 as before. - const shownWeight = ref.effectiveWeight ?? ref.weight; - if (shownWeight == null || shownWeight === 1) return null; - return ( - <> - | - weight {shownWeight} - - ); - })()} + {ref.weight != null && ref.weight !== 1 && ( + <> + | + weight {ref.weight} + + )} ))} diff --git a/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx b/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx index 2007d87..3f9c23e 100644 --- a/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx +++ b/frontend-v2/src/components/k8s/F5BNKTopologyViewer.tsx @@ -52,12 +52,7 @@ interface TopologyBackend { name: string; namespace?: string | null; port: number | null; - weight: number | null; // declared spec weight - // Analyzer-computed weights (k8s.f5.com/service-settings). effectiveWeight is - // the number to show; null when the analyzer didn't weight this backend, so - // fall back to the declared `weight` (#8). - effectiveWeight?: number | null; - analyzerWeights?: Record | null; + weight: number | null; kind?: string; group?: string; } @@ -608,16 +603,8 @@ export function F5BNKTopologyViewer({ clusterId, namespace, onSelectResource }: if (isCustomKind) { badges.push({ text: be.kind!, variant: 'outline' }); } - // Prefer the analyzer's computed weight over the declared - // spec weight; fall back to `weight` when the analyzer didn't - // weight this backend (#8). - const shownWeight = be.effectiveWeight ?? be.weight; - if (shownWeight) { - const fromAnalyzer = be.effectiveWeight != null; - badges.push({ - text: `weight ${shownWeight}`, - ...(fromAnalyzer ? { variant: 'info' as const } : {}), - }); + if (be.weight) { + badges.push({ text: `weight ${be.weight}` }); } return ( { expect(screen.getByText('f5-bnk')).toBeInTheDocument(); }); - it('shows the analyzer effectiveWeight over the declared spec weight (#8)', async () => { - server.use( - http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { - return HttpResponse.json({ - ...emptyBnkData, - topology: [{ - name: 'bnk-gateway', - namespace: 'bnk-demo', - gatewayClassName: 'f5-bnk', - addresses: ['10.1.1.100'], - listeners: [{ - name: 'l4', - protocol: 'TCP', - port: 8080, - networkPolicies: [], - routes: [{ - name: 'dynamo-route', - namespace: 'dynamo-system', - kind: 'L4Route', - hostnames: [], - analyzers: [], - backends: [{ - name: 'dynamo-svc', - namespace: 'dynamo-system', - port: 8080, - weight: 1, // declared spec weight the old code showed - effectiveWeight: 99, // what the analyzer actually computed - analyzerWeights: { '10.1.2.3': 99 }, - }], - }], - }], - securityPolicies: [], - }], - topologyCounts: { ...emptyBnkData.topologyCounts, gateways: 1, listeners: 1 }, - }); - }) - ); - - const user = userEvent.setup(); - render(); - - // Listener is open by default; expand the route to reveal its backends. - await user.click(await screen.findByText('dynamo-route')); - - expect(await screen.findByText('dynamo-svc')).toBeInTheDocument(); - // The analyzer's 99, not the declared 1. - expect(screen.getByText('weight 99')).toBeInTheDocument(); - expect(screen.queryByText('weight 1')).not.toBeInTheDocument(); - }); - - it('falls back to the declared weight when no analyzer weight is present (#8)', async () => { - server.use( - http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { - return HttpResponse.json({ - ...emptyBnkData, - topology: [{ - name: 'bnk-gateway', - namespace: 'bnk-demo', - gatewayClassName: 'f5-bnk', - addresses: ['10.1.1.100'], - listeners: [{ - name: 'l4', - protocol: 'TCP', - port: 8080, - networkPolicies: [], - routes: [{ - name: 'plain-route', - namespace: 'bnk-demo', - kind: 'HTTPRoute', - hostnames: [], - analyzers: [], - backends: [{ - name: 'plain-svc', - namespace: 'bnk-demo', - port: 80, - weight: 42, // no annotation -> declared value stands - effectiveWeight: null, - analyzerWeights: null, - }], - }], - }], - securityPolicies: [], - }], - topologyCounts: { ...emptyBnkData.topologyCounts, gateways: 1, listeners: 1 }, - }); - }) - ); - - const user = userEvent.setup(); - render(); - - await user.click(await screen.findByText('plain-route')); - - expect(await screen.findByText('plain-svc')).toBeInTheDocument(); - expect(screen.getByText('weight 42')).toBeInTheDocument(); - }); - it('shows error state on failure', async () => { server.use( http.get('*/api/k8s/clusters/:id/f5bnk/data', () => { diff --git a/frontend-v2/src/types/f5bnk.ts b/frontend-v2/src/types/f5bnk.ts index b36dd9a..5cae9c9 100644 --- a/frontend-v2/src/types/f5bnk.ts +++ b/frontend-v2/src/types/f5bnk.ts @@ -371,12 +371,7 @@ export interface TopologyRouteBackend { name: string; namespace?: string | null; port: number | null; - weight: number | null; // declared spec weight (backendRefs[].weight) - // Analyzer-computed weights from the k8s.f5.com/service-settings annotation. - // effectiveWeight is the number the UI should show; it's null when the - // analyzer didn't weight this service, in which case fall back to `weight` (#8). - effectiveWeight?: number | null; - analyzerWeights?: Record | null; // {pod_ip: weight} + weight: number | null; kind?: string; // "Service" (default), "ServiceImport", etc. group?: string; // "" = core API group } @@ -575,11 +570,7 @@ export interface BnkBackendRouteRef { gatewayName: string; listenerName: string; port?: number | null; - weight?: number | null; // declared spec weight - // Carried through from the topology backend so the backends view prefers the - // analyzer's number over the declared weight, same as the topology tree (#8). - effectiveWeight?: number | null; - analyzerWeights?: Record | null; + weight?: number | null; } export interface BnkBackendEntry {