fix(#202): count Multus pods in the DaemonSet's namespace so OpenShift (openshift-multus) reports correctly

[jgruberf5]

Summary

ClusterScanner reported 0 running Multus pods on ROKS/OpenShift even when Multus was healthy. This fixes the namespace-scoping so the count reflects reality on any cluster layout.

Root cause

The pod fetch (backend/services/scanner/fetch.py) enumerated pods only from a hardcoded namespace set (cert-manager, kube-system, kamaji-system, + BNK/F5 discovery), and analyze_multus (prereqs.py) derived running_pods from the kube-system list alone. On OpenShift/ROKS, Multus runs in openshift-multus, which was never queried — so running_pods read 0. The DaemonSet is still discovered cluster-wide via list_daemon_set_for_all_namespaces, so status showed DETECTED with 0 pods: exactly the reported "0 Multus pods while N running".

Fix (DaemonSet-namespace-driven — preferred option)

Fetch the Multus pods from whatever namespace the discovered DaemonSet actually lives in, rather than a hardcoded kube-system:

• Vanilla k8s (kube-system) reuses the already-fetched pod list — no extra API call.
• OpenShift (openshift-multus) and any future layout fetch that namespace.
• No new hardcode; works for any Multus placement.

Changes:

• fetch.py: _multus_daemonset_namespace + _fetch_multus_pods; new namespace-scoped multus_pods in the fetch dict.
• __init__.py: feed analyze_multus the scoped multus_pods.
• prereqs.py: rename the analyze_multus param to multus_pods; count running pods from it.

How the tests exercise the REAL fetch (not a handed list)

tests/unit/test_scanner_multus_namespace.py runs the actual fetch_scan_data with the k8s API mocked, not a pre-built pod list:

• list_daemon_set_for_all_namespaces reports the Multus DaemonSet in openshift-multus.
• list_namespaced_pod(namespace=...) returns Multus pods only for openshift-multus (empty for kube-system) — the exact shape of a real OpenShift cluster.

The test asserts the fetch actually queried openshift-multus (via call_args_list) and that running_pods == 3. A vanilla-k8s test (Multus in kube-system) still counts (no regression), plus a non-Running-phase guard.

Reproduce + verify evidence

• Reproduce (pre-fix behavior via mutation): reverting _fetch_multus_pods(...) back to multus_pods = kube_system_pods reds the OpenShift tests — running_pods == 0 and openshift-multus never appears in the queried namespaces (assert 'openshift-multus' in {'cert-manager', 'kube-system'} fails). The vanilla-k8s test stays green.
• Verify (post-fix): all 3 new tests pass; full affected suite (test_scanner_prereqs, test_scanner_recommendations, test_proxy_inventory, test_running_release_discovery, test_bnk_pod_discovery) = 172 passed. ruff check clean on all changed files.

Not verifiable without a live cluster

The exact Multus DaemonSet/pod naming and the openshift-multus namespace on a real ROKS/OpenShift cluster are assumed from the issue report; the logic keys off "multus" in the DaemonSet name and its reported namespace, so it adapts to whatever a real cluster presents.

Closes #202

[jgruberf5]

Self-review (cold, adversarial) — no blocker, no major; correct for the real OpenShift scenario

An independent cold auditor reviewed this PR, executing against a mocked k8s API (326 tests green across the scanner/fetch/prereqs/recommendations/inventory suites).

Held under attack (verified):

• Right namespace on the real cluster — with the issue's multi-DaemonSet fixture (multus, multus-additional-cni-plugins, network-metrics-daemon, all in openshift-multus), _multus_daemonset_namespace returns openshift-multus; analyze_multus reads multus_ds[0] from the same list in the same order, so the reported and fetched namespaces are always consistent.
• No KeyError from strict data["multus_pods"] — the only builder is fetch_scan_data's single dict literal, which unconditionally sets the key; no partial/error/alternate path omits it.
• Vanilla-k8s: no extra API call — reuses the already-fetched kube-system list (multus_pods is kube_system_pods → True), count byte-identical to old behavior (zero regression).
• Graceful degradation — _fetch_pods_in_ns catches all exceptions → []; RBAC-denied/absent namespace degrades to the old 0-count, no crash; no-Multus → falls back to kube-system list, key always populated.
• Test exercises the real fetch — mocks CoreV1Api/AppsV1Api, asserts list_namespaced_pod(namespace="openshift-multus") was called; mutation (force multus_pods = kube_system_pods) reds the OpenShift tests, vanilla stays green.

Two MINORs (cosmetic, non-blocking):

1. On OpenShift, running_pods also matches multus-additional-cni-plugins-* pods, so it counts the broader Multus-networking pod set (~2× the multus DaemonSet's ready count) — used only as a display string ("N Multus pods running").
2. _multus_daemonset_namespace picks the first "multus"-named DaemonSet by list order — order-dependent only in the hypothetical where two sit in different namespaces (does not occur on real OpenShift/ROKS).

Addressing both below for robustness.

[bonnyr-f5]

Review discipline — round 1 @ 1e575d0

Ran the pipeline locally against 1e575d0 (7 files, +289/−24, base ab2916c9). Gates green: 126 passed across test_scanner_multus_namespace, test_scanner_prereqs, test_scanner_recommendations, test_proxy_inventory, test_running_release_discovery; ruff check clean on services/scanner/ and the new test.

Cold full-diff audit: not run — not risk-triggered. No authz/multi-tenant scoping, no migrations, no secrets, no money, 7 files (< ~15). Recording that explicitly rather than claiming a stage that didn't happen.

What holds up

• The approach is right. DaemonSet-namespace-driven beats adding openshift-multus to the hardcoded set — no new hardcode, adapts to any layout. Vanilla k8s reuses the already-fetched kube-system list, so the common path pays no extra API call.
• The test exercises the real fetch. fetch_scan_data with the k8s client mocked, asserting via call_args_list that openshift-multus was actually queried — not a hand-built pod list handed to analyze_multus. That's the right shape and it's rare; it's what makes the reproduce claim credible.
• Status refactor is semantically equivalent. has_nad_crd and (multus_pods or multus_ds) → has_nad_crd and (running_multus_pods or primary_ds): primary_ds is truthy exactly when multus_ds_all is non-empty. Verified, no behavior drift.
• The new fetch-dict key was swept to completion. I re-ran that sweep independently: 10 construction sites across 5 files, every one carries multus_pods. No KeyError left anywhere, and data["multus_pods"] (subscript, not .get) matches the file's dominant convention for unconditionally-populated keys.

MINOR 1 — running_pods is 2×nodes on the exact platform this PR fixes

On real OpenShift, openshift-multus runs two name-matching DaemonSets — multus and multus-additional-cni-plugins — one pod per node each. The tolerant name match counts both. Demonstrated on a 3-node fixture:

nodes................. 3
running_pods (UI)..... 6
DaemonSet Ready/Des... 3/3

ClusterScanResults.tsx:530-548 renders those as adjacent rows in one card:

  Running Pods    6
  Ready / Desired 3 / 3

So 0 while N running becomes 2N while N ready. Strictly better than 0, but the PR body's "the count reflects reality on any cluster layout" doesn't hold on the layout it targets.

The in-code justification for leaving it is factually wrong on its stated blocker:

precise per-DaemonSet attribution needs pod ownerReferences we do not fetch

_fetch_daemonsets already reads ds.metadata.labels and walks ds.spec.template.spec.containers; _fetch_pods_in_ns already captures pod labels. So ds.spec.selector.match_labels is a one-line addition to an object already in hand — zero extra API calls, and matching pods by the DaemonSet's own selector is the canonical k8s attribution. The comment's second blocker (name prefixes break on kube-multus-ds-*) is an argument for the label selector, not against precision.

No test covers this. The OpenShift fixture uses 2 primary + 1 sibling pod = 3, which coincidentally equals the DaemonSet's ready=3 — so the discrepancy is invisible to the suite. A fixture with equal per-DaemonSet pod counts (the real topology) is what would surface it.

MINOR 2 — the primary-DaemonSet pick is duplicated across the module boundary

fetch._multus_daemonset_namespace and prereqs.analyze_multus each independently re-implement "the DaemonSet named exactly multus, else the first", with different None handling — ds.get("name") or "" vs ds.get("name", "").lower() (the latter raises AttributeError on an explicit name: None).

The fix is correct only while those two agree, and nothing asserts that they do — the two new tests pin each side separately. If they ever diverge, the fetch queries namespace A while the analyzer reports the DaemonSet in namespace B, and running_pods silently returns to 0: #202 re-armed, with no test failing.

Class fix: one shared primary_multus_daemonset(daemonsets) -> dict | None imported by both, plus a test asserting agreement on a multi-DaemonSet, multi-namespace fixture.

MINOR 3 — the fallback docstring's reasoning is wrong

_fetch_multus_pods:

If no Multus DaemonSet exists, fall back to the kube-system pods (Multus absent → the running-pod count is correctly 0).

The fallback does not yield 0 — it yields the count of Running kube-system pods whose name contains multus. That is the pre-fix behavior and it is the right fallback: a Multus deployed without a name-matching DaemonSet still gets counted. But the parenthetical licenses a future editor to "simplify" it to return [] and regress exactly that case. Fix the comment, keep the code.

Nits

• The extra fetch is serial, outside the pool. _fetch_multus_pods runs after the with ThreadPoolExecutor block joins (fetch.py:947), so OpenShift scans pay a full round-trip serially — outside the parallel burst this module otherwise guards carefully ("the ConfigMap future result is NOT used here to avoid intra-burst deps"). A real tradeoff, not a defect: the namespace isn't known until daemonsets_f resolves. Either acknowledge the cost in the comment, or gate a speculative openshift-multus fetch inside the burst on the already-computed has_routes (fetch.py:740) and pick afterwards.
• bonnyr-f5 #203 review (MINOR n) in production comments — four across two files, referencing rounds with no record on this PR (no reviews, no inline comments). Reviewer-attributed comments age badly post-merge. Keep the reasoning, drop the attribution and numbering.
• Fixture style — "kube_system_pods": [], "multus_pods": [], puts two entries on one line inside dicts that are otherwise one-per-line (test_running_release_discovery.py:268, test_proxy_inventory.py:521, test_scanner_recommendations.py:493).
• Pre-existing, out of scope: analyze_sriov's third parameter kube_system_pods is never used in the body (passed at __init__.py:107). It's the sibling analyzer of the one being fixed, so the next person sweeping this class has to re-derive that SR-IOV is not affected. Worth deleting in a follow-up.

Class sweep — cert-manager has the same shape (follow-up, not this PR)

analyze_cert_manager counts pods from the hardcoded cert-manager namespace (fetch.py:793), with has_crds and running_pods → DETECTED, else PARTIAL. A cert-manager Helm-installed into a non-default namespace reads PARTIAL with 0 pods — #202's class exactly. Latent rather than live, since cert-manager is the conventional default (the Red Hat operator uses it too). The mechanism is already in hand: helm_releases carries the release namespace. Worth an issue so the class closes, not just the Multus instance.

Review Assessment

• Verdict: REVISE
• Audit SHA: 1e575d09db32e443020e42811b98d7a9ee16d72a
• Cold Audit Performed: No — not risk-triggered (no authz/migration/secret/money surface; 7 files)
• Invariants Verified: INV-1, INV-2, INV-3, INV-5, INV-8 (N/A — no DB queries, FK writes, request-schema fields, delete sites or locks in the diff); INV-4 (swept — no alembic revisions; the new multus_pods fetch-dict key checked across all 10 construction sites; concurrent writer fix(#194): enqueue cluster inventory sync on registration and make a resync reliably triggerable #200 also edits scanner/__init__.py but ~120 lines away from fix(#202): count Multus pods in the DaemonSet's namespace so OpenShift (openshift-multus) reports correctly #203's line 105 off the same base blob e65082d — merges clean, no semantic interaction); INV-6 (the frontend running_pods ?? 0 > 0 status is a display metric that fails closed on undefined — fine); INV-7 (N/A — no migrations); INV-9 (N/A — no shell scripts)
• Git & Harness Cleanliness: Clean — PR tree clean; the untracked .gitignore.maf.new and bin/hooks/ are local harness drift in my working tree, not PR content

Findings & Action Items

• Major (Blockers): none
• Minor (Non-blocking):
  • prereqs.py analyze_multus: running_pods counts both multus and multus-additional-cni-plugins pods → 2×nodes next to Ready/Desired N/N on OpenShift. Attribute via ds.spec.selector.match_labels (already-fetched object, zero extra API calls); add a fixture with equal per-DaemonSet pod counts.
  • fetch.py:285 + prereqs.py analyze_multus: extract one shared primary_multus_daemonset() used by both, plus a test asserting they agree — divergence silently restores running_pods == 0.
  • fetch.py _fetch_multus_pods docstring: the "count is correctly 0" parenthetical is false and invites a regression; the fallback is right, the reasoning isn't.
• Nits:
  • fetch.py:947: the OpenShift pod fetch is serial, outside the thread pool.
  • fetch.py, prereqs.py: drop the bonnyr-f5 #203 review (MINOR n) attributions; keep the reasoning.
  • test fixtures: two dict entries on one line, inconsistent with surrounding style.
  • prereqs.py analyze_sriov: unused kube_system_pods parameter (pre-existing).

Verdict is REVISE on three actionable minors, not on anything broken — the fix is directionally correct and strictly better than staging. MINOR 1 and MINOR 2 are the two worth landing before merge: the first because the headline number is still wrong on the target platform, the second because it is the mechanism by which this exact bug comes back unnoticed.

[bonnyr-f5]

Review — review-discipline pipeline, round 1

Reviewed at head 1e575d0. Ran the full pipeline: invariant sweep, an independent cold full-diff audit with no session context, and the project gates. All test output below was produced against an isolated worktree checked out at the PR head — not against staging.

The multus fix itself is correct, honestly tested, and safe. What holds this up is class completeness: #202 is one instance of a three-instance pattern, and the untouched sibling has a worse consequence than the bug being fixed.

What I verified green

• Gates: ruff clean, mypy clean on both changed modules. 8050 passed across tests/unit tests/component; 340 passed in the scanner/prereq subset; 83 passed across the touched files.
• Tests are honest. I extracted the new test file into a merge-base worktree and ran it against pre-fix source: all 5 fail, and the OpenShift case fails on the real assertion — assert 'openshift-multus' in {'cert-manager', 'kube-system'} — not merely on the new dict key.
• Status-logic equivalence holds, provably. primary_ds is non-None exactly when multus_ds was non-empty, and when no DaemonSet exists fetch.py:320 returns kube_system_pods verbatim, so the pod sets are identical. No input changes status; only running_pods and the reported daemonset move.
• Dict contract is complete. All four test files constructing the scan-data dict were updated (9 sites). tests/unit/test_proxy_translate_cis_service.py:1413 calls the real fetch_scan_data and was not updated, but is safe: it patches _fetch_daemonsets → [], so _fetch_multus_pods short-circuits. mcp-server/, bnk-operator/, tests/contract, tests/integration, tests/e2e: zero references.
• SR-IOV is not affected — analyze_sriov derives from the cluster-wide DaemonSet list and node allocatables, so openshift-sriov-network-operator is already handled.
• INV-4 (cross-writer): PR fix(#194): enqueue cluster inventory sync on registration and make a resync reliably triggerable #200 also edits scanner/__init__.py, but at :227 vs this PR's :105 — no textual or semantic collision, merge order irrelevant.
• No frontend change needed: ClusterScanResults.tsx:531-533 renders running_pods ?? 0 and flips the badge on > 0, so the fix reaches the panel as intended.

Major

M-1 · The bug class is 1-of-3 fixed, and the unfixed sibling downgrades status rather than just miscounting.

There are exactly three hardcoded-namespace pod fetches: fetch.py:833 (cert-manager), :834 (kube-system), :895 (kamaji-system). This PR fixed the consumer of one.

Multus had a rescue that cert-manager does not. analyze_multus still reached DETECTED via the cluster-wide DaemonSet list, so #202 was a wrong number. analyze_cert_manager gates on if has_crds and running_pods: — an empty pod list downgrades the status. Verified by running against a cert-manager healthy in a non-cert-manager namespace:

status = partial   pods = {'controller': 0, 'webhook': 0, 'cainjector': 0, 'total_running': 0}   version = None

That PARTIAL propagates into recommendations.py:72-78 and emits a wrong actionable recommendation to the operator — "cert-manager partially installed — CRDs found but some components may be missing. Running pods: 0." — plus a warning at adaptive_module_selector.py:470. Version detection silently returns None.

This is reachable on the very platform that motivated #202: the IBM ROKS cert-manager add-on installs into ibm-cert-manager, older Red Hat operands sit in openshift-cert-manager, and any Helm install can --namespace. kamaji-system (fetch.py:895) is the same shape and also gates DETECTED on running pods — lower likelihood, same class.

This repo already has the right pattern. services/bnk_pod_discovery.py:272 _sweep_all_namespaces uses list_pod_for_all_namespaces with a fallback sweep. One such call, partitioned by namespace/name, replaces all three hardcoded fetches, closes the class permanently, and removes the need for the DaemonSet-namespace indirection added here.

Either resolution is fine by me: collapse the three fetches into the existing sweep, or file follow-ups for cert-manager (higher severity than #202) and kamaji and merge this as-is. Given #202's own "low priority, filed so it isn't lost" framing, the second is defensible — but the cert-manager instance should not stay unfiled.

Minor

m-1 · The "deterministic selection" commit does not fire on the clusters Forge itself builds. Both pickers prefer a DaemonSet named exactly multus, else fall back to multus_ds_all[0] — list-order dependent. But this repo's own installer creates kube-multus-ds (modules/bare_metal/install_multus.py:62, and that is the upstream name; the PR's own vanilla fixture uses kube-multus-ds-* pod names). On any Forge-provisioned or vanilla cluster the exact match never matches, so selection is order-dependent again — precisely what commit 2 set out to remove. The comment's "deterministic regardless of list order" is false for that topology. Suggest ranking candidates (multus → kube-multus-ds → sorted tiebreak) rather than exact-match-then-first.

m-2 · One selection rule, two implementations, divergent null-handling. fetch.py:292 uses (ds.get("name") or ""); prereqs.py:188/:194 use ds.get("name", "").lower(). Probed:

Input	_multus_daemonset_namespace	analyze_multus
{"name": None, ...}	tolerates	AttributeError: 'NoneType' object has no attribute 'lower'
{"name": "multus"}, no namespace	returns None	KeyError: 'namespace'

Neither is reachable today (_fetch_daemonsets always populates both), so this is a drift hazard, not a live crash — but the diff introduced the divergence while holding the safe idiom in the other file. One of the two hardenings is wrong: either None is reachable and analyze_multus crashes the scan, or it isn't and the or "" is dead. Extract one shared pick_primary_multus_daemonset(daemonsets); no test asserts the two pickers agree.

m-3 · The comment describing running_pods contradicts what the code now computes. prereqs.py:198-205 claims the metric is "the broad Multus-networking pod set… on a cluster with a separate multus-additional-cni-plugins DaemonSet this includes those pods too." That was true pre-fix; it is now false whenever the sibling lives elsewhere, because only the primary DaemonSet's namespace is fetched. Verified end-to-end with multus@openshift-multus (3 pods) + sibling@sib-ns (6 pods) → running_pods = 3, sibling excluded. The metric is now a topology-dependent hybrid: neither the broad set nor the DaemonSet's ready count.

The flip side matters more in practice: real OpenShift co-locates both DaemonSets in openshift-multus, so on a 3-node cluster this now reports running_pods: 6 beside a DaemonSet reading 3/3. The issue's symptom "0 Multus pods while N running" becomes "6 while 3 running". Still wrong, in the other direction.

Also, the stated reason for accepting that — "precise per-DaemonSet attribution needs pod ownerReferences we do not fetch" — understates what is available: _fetch_pods_in_ns already captures pod labels (fetch.py:266). The DaemonSet's spec.selector.matchLabels is the canonical mechanism and is a one-line addition to an already-fetched object (_fetch_daemonsets captures metadata.labels but not spec.selector) — no extra API call. The design choice may still be right; the justification for it isn't accurate.

m-4 · An RBAC denial reproduces the exact #202 symptom with zero diagnostic. _fetch_pods_in_ns swallows every exception and returns [] with no logging — unlike sibling _fetch_daemonsets, which logs at fetch.py:252. On a ROKS cluster whose scan credential can list DaemonSets cluster-wide but not pods in openshift-multus, the panel shows the identical wrong running_pods: 0 this PR fixes, and nothing says why. Verified: no exception raised, running_pods = 0, status = detected. One logger.warning(f"Failed to fetch pods in {namespace}: {e}") makes the fix's own failure mode observable.

m-5 · The new fetch is serial, on the critical path. fetch.py:948 sits outside the with pool: block (closes at :936), so it runs after the entire parallel burst, adding up to _request_timeout=10 serially to every OpenShift scan. It correctly adds no call where none is needed (vanilla queries ['cert-manager','kube-system']; OpenShift adds openshift-multus). Resolving daemonsets_f early and submitting the dependent fetch inside the pool keeps it off the critical path — or it disappears entirely under M-1's sweep.

m-6 · Coverage gaps. Untested: empty DaemonSet list; DaemonSet with no namespace key; {"name": None}; and sibling-before-primary end-to-end (_run_fetch hardcodes a single DS named multus, so fetch→analyze agreement under a cross-namespace sibling list is never exercised — only the two unit-level pickers). test_non_running_openshift_multus_pods_not_counted fails pre-fix only via KeyError: 'multus_pods', so it proves the dict contract, not the phase filter it names.

Mock fidelity is acceptable — _v1_pod/_v1_daemonset set every field the parsers read, so no auto-attribute leaks into an assertion. But the docstring's "exercise the REAL fetch path" is generous: with only CoreV1Api/AppsV1Api patched, every other key degrades to empty through swallowed exceptions and version_info comes back as raw MagicMock objects. The multus assertions are sound; the framing overstates.

Nits

• scanner/__init__.py:107 still threads data["kube_system_pods"] into analyze_sriov, whose third parameter now appears only in its signature — I grepped the whole function body, one occurrence. Multus was its last real consumer. Drop the dead parameter, or the plumbing.
• analyze_multus is in __all__ and its 3rd parameter was renamed kube_system_pods → multus_pods. All in-repo calls are positional so nothing breaks; keyword callers outside the repo would.

Review Assessment

• Verdict: REVISE
• Audit SHA: 1e575d09db32e443020e42811b98d7a9ee16d72a
• Cold Audit Performed: Yes — independent agent, no session context, full diff, verified against the repo
• Invariants Verified: INV-4 (cross-writer namespace/file collision — clean vs open fix(#191): validate credential-template provider so an unknown value can't silently inject nothing #199/fix(#194): enqueue cluster inventory sync on registration and make a resync reliably triggerable #200/fix(#195): stream opentofu task logs incrementally and expose log fields in the task list #201/Surface L4Route service-settings faithfully (groundwork for #8; on-screen fix needs cluster data) #172); INV-6 analog (frontend ?? 0 fails closed to missing, acceptable for a display metric); INV-1/2/3/5/7/8/9 N/A (no DB queries, FK writes, schema enums, delete paths, migrations, locks, or shell scripts in this diff)
• Git & Harness Cleanliness: Clean for PR content; the local checkout carries unrelated untracked harness drift (.gitignore.maf.new, bin/hooks/)

Findings & Action Items

• Major (Blockers):
  • fetch.py:833 / :895 (+ prereqs.py cert-manager & kamaji gates): hardcoded-namespace pod fetch is a 3-instance class; only the multus consumer is fixed. cert-manager additionally downgrades status and emits a wrong recommendation. Class fix: one list_pod_for_all_namespaces sweep per the existing bnk_pod_discovery.py:272 pattern — or file the follow-ups explicitly.
• Minor (Non-blocking):
  • fetch.py:295 / prereqs.py:190: exact-multus match never fires on kube-multus-ds; selection falls back to list order.
  • fetch.py:292 vs prereqs.py:188: duplicated picker, divergent null-handling — extract one shared helper.
  • prereqs.py:198-205: comment contradicts the implementation; count reads ~2× the DaemonSet on co-located OpenShift; the ownerReferences rationale ignores already-fetched pod labels.
  • fetch.py:281: add a logger.warning so an RBAC denial is distinguishable from "Multus absent".
  • fetch.py:948: serial fetch outside the pool; submit it inside.
  • tests/unit/test_scanner_multus_namespace.py: add empty-DS-list, missing-namespace, and end-to-end sibling-first cases.
• Nits:
  • scanner/__init__.py:107: analyze_sriov's kube_system_pods parameter is dead.
  • analyze_multus public parameter renamed; positional-only in-repo, safe.

[jgruberf5]

Round-1 review addressed @ 2677c04d

Thanks @bonnyr-f5 — all four items resolved. The multus fix itself was confirmed correct/honestly-tested; these are the completeness/consistency fixes.

M-1 (Major, class completeness) — took the "file follow-ups, keep this PR scoped" option (avoids re-churning analyze_multus's signature). The hardcoded-namespace pod fetch is 1-of-3:

• cert-manager in a non-default namespace downgrades status to PARTIAL (hardcoded pod-fetch namespace, #202 class) #210 — cert-manager (severity:medium, higher than Multus pod inventory is blind to OpenShift's openshift-multus namespace (running_pods reads 0 on ROKS/OpenShift) #202: it downgrades status to PARTIAL + emits a wrong recommendation; reachable on ROKS ibm-cert-manager / openshift-cert-manager).
• kamaji pod count uses hardcoded kamaji-system namespace (#202 class, low likelihood) #211 — kamaji-system (severity:low).

Both note the eventual collapse into services/bnk_pod_discovery.py's _sweep_all_namespaces / list_pod_for_all_namespaces pattern that would close the class permanently.

m-1 (ranked selection) — the exact-multus-then-first pick never fired on Forge/vanilla clusters, whose installer names the DaemonSet kube-multus-ds (modules/bare_metal/install_multus.py:62). Now ranked deterministically: exact multus > kube-multus-ds > sorted-name tiebreak.

m-2 (one rule, one implementation) — extracted pick_primary_multus_daemonset(daemonsets) in prereqs.py, imported by both fetch._multus_daemonset_namespace and analyze_multus. Single-sourced on the safe idiom ((ds.get("name") or "")); analyze_multus now reads DaemonSet fields via .get() (no KeyError on a missing namespace). New test pins that the two callers agree on a multi-DS, multi-namespace fixture.

m-3 (comment vs code) — rewrote the running_pods comment: it now describes the actual computation (Running multus-named pods in the primary DaemonSet's own namespace only), noting that a sibling in a different namespace is excluded while a co-located one on real OpenShift is counted.

Verification: 84 pass in test_scanner_multus_namespace.py (+11 new); 228 pass across the touched suites. Mutation-tested — exact-match-then-first reds 4 tests, a divergent fetch picker reds 2. ruff + mypy clean on fetch.py / prereqs.py.

The other nits (serial fetch outside the pool, RBAC-denial logging, dead analyze_sriov param, one-line fixture style, attribution comment cleanup) are noted; the reviewer-attribution comments were dropped as part of the m-3/m-2 rewrites. The remaining nits are left for the class-fix follow-up (#210/#211) or a separate pass to keep this PR scoped to the review's two land-before-merge items.

https://claude.ai/code/session_01UCsZXDxBsWV2s4kT47DwDW

[jgruberf5]

Round-1 response @ 2677c04d

M-1 (bug class 1-of-3) — took the "file, don't collapse" option you offered: filed #210 (cert-manager, severity:medium — flagged as higher-severity than #202 since an empty pod list downgrades status to PARTIAL + emits a wrong recommendation, reachable on ROKS ibm-cert-manager/openshift-cert-manager) and #211 (kamaji-system, severity:low). Both reference services/bnk_pod_discovery.py:272 _sweep_all_namespaces as the eventual class fix. Kept this PR scoped to the multus instance rather than re-churning analyze_multus's signature (which also avoids amplifying the open cross-writer surface with #200).

m-1 + m-2 (list-order selection; two divergent pickers): extracted one shared pick_primary_multus_daemonset(daemonsets) in prereqs.py, imported by BOTH fetch._multus_daemonset_namespace and analyze_multus. It ranks exact "multus" → "kube-multus-ds" (the Forge/upstream installer name, modules/bare_metal/install_multus.py:62) → sorted-name tiebreak — so selection is deterministic on Forge/vanilla clusters, not list-order dependent. Safe (ds.get("name") or "") idiom throughout; .get() on DaemonSet fields so a missing namespace yields None, not KeyError. New TestPickPrimaryMultusDaemonset + TestPickersAgree (both callers agree; Forge end-to-end picks kube-multus-ds).

m-3 (stale comment): rewrote the running_pods comment to the real computation — Running multus-named pods in the primary DaemonSet's own namespace only (a sibling in a different namespace is excluded; a co-located sibling on real OpenShift is included, so the count can exceed the DS ready number).

Verified: 228 passed (+11 new), mutation-tested (exact-match-then-first reds 4 tests; diverging the two pickers reds 2), ruff + mypy clean. Remaining nits (serial fetch outside the pool, RBAC-denial logging, dead analyze_sriov param) left for the class follow-ups (#210/#211). Re-requesting.

[jgruberf5]

CI status note

The code review above stands. The only red CI on this PR is the two repo-wide P4 security gates:

• P4 · Docker Build + Scan — Trivy CRITICAL CVE-2026-56854 (golang.org/x/crypto/ssh auth bypass) in the bundled helm 3.20.0 binary.
• P4 · Security Audit — gitpython 3.1.58 (PYSEC-2026-3785/86/87/88).

Both are environmental and repo-wide, not caused by this PR's code: the advisory/vuln DBs updated after staging last audited clean on 2026-08-24, so every open PR (and staging itself, if re-run) is red on them. Both are fixed in #215 (a documented .trivyignore suppression for the un-fixable-yet helm CVE + a gitpython patch bump). This PR goes green on P4 once #215 merges to staging and this branch rebases.

All P1/P2/P3 gates are green. Awaiting further review.

https://claude.ai/code/session_01UCsZXDxBsWV2s4kT47DwDW

[bonnyr-f5]

Review — review-discipline pipeline, round 2 (closure)

Re-reviewed at head 2677c04d. This round is a closure check of the round-1 findings, not a fresh audit: I checked each prior finding for closure and re-audited the changed regions as fresh code.

The M-1 gate is satisfied. #202's hardcoded-namespace pod fetch is a three-instance class; round 1 accepted either collapsing all three into the existing all-namespaces sweep or fixing multus and filing follow-ups for the rest. The PR took the latter: the multus consumer is fixed in-code and the class is tracked by two filed, open follow-ups — #210 (cert-manager → PARTIAL downgrade, the higher-severity sibling) and #211 (kamaji, low likelihood). Both verified OPEN against the tracker.

Round-1 findings — closure status

Finding	Status	Evidence
M-1 hardcoded-namespace class (1-of-3 fixed)	✅ resolved via follow-ups	fetch.py still queries the three literal namespaces, but the multus consumer is fixed and #210/#211 track cert-manager + kamaji
m-1 exact-multus match never fires on kube-multus-ds	✅ fixed	new shared pick_primary_multus_daemonset (prereqs.py:173-205) ranks multus(0) → kube-multus-ds(1) → sorted tiebreak; covered by order-reversal tests
m-2 two divergent pickers, divergent null-handling	✅ fixed	both fetch.py and analyze_multus now delegate to the one shared helper; agreement pinned by test_fetch_and_analyze_agree_*
m-4 _fetch_pods_in_ns swallows exceptions with no logging	⚠️ still open	fetch.py:281-282 except Exception: return [] unchanged — an RBAC denial still silently reproduces the #202 symptom
dead param analyze_sriov(…, kube_system_pods)	⚠️ still open	prereqs.py:222 param unused in the body; still passed at __init__.py:107

The new cross-module import (fetch.py → prereqs.py) is cycle-free, and the ["name"]→.get("name") changes are strict null-tolerance improvements. No new regressions.

m-4 and the dead param are non-blocking and outside the M-1 gate — worth a carry-forward (a follow-up commit or ticket), but they don't hold the PR.

Review Assessment

• Verdict: PASS
• Audit SHA: 2677c04d1db2d37b4796bd43e5a23ee0d6dd9a3a
• Cold Audit Performed: Yes — closure check in an independent context; cert-manager in a non-default namespace downgrades status to PARTIAL (hardcoded pod-fetch namespace, #202 class) #210/kamaji pod count uses hardcoded kamaji-system namespace (#202 class, low likelihood) #211 state verified against the live issue tracker
• Invariants Verified: INV-10 — fetch and analyze now select the same DaemonSet via one shared picker, so the namespace queried for pods always matches the reported DaemonSet; divergence can no longer silently zero running_pods
• Git & Harness Cleanliness: Clean (PR content)

Findings & Action Items

• Major (Blockers): none
• Minor (Non-blocking, carry-forward):
  • fetch.py:281: swallow-and-return-[] with no logging; add a logger.warning so an RBAC denial is distinguishable from absence
  • prereqs.py:222 / __init__.py:107: drop the unused kube_system_pods param from analyze_sriov

🤖 Generated with Claude Code