NE-2279: Add must-gather offline support for NIDS toolset - #362
Conversation
|
@bentito: This pull request references NE-2279 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThe PR adds netedge evaluation configurations, offline must-gather tasks and fixtures, exported provider access, and must-gather fallbacks for CoreDNS, EndpointSlice, and Route lookups. ChangesNetedge evaluation selection and execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant NetedgeTool
participant MustGather
participant Archive
participant DynamicClient
Task->>NetedgeTool: Invoke netedge lookup
NetedgeTool->>MustGather: GetProvider()
alt Provider available
MustGather-->>NetedgeTool: Provider
NetedgeTool->>Archive: Read or list resource
Archive-->>NetedgeTool: Resource data
else Provider unavailable
MustGather-->>NetedgeTool: Error
NetedgeTool->>DynamicClient: Get or list resource
DynamicClient-->>NetedgeTool: Resource data
end
NetedgeTool-->>Task: Formatted result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bentito The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/toolsets/netedge/coredns.go (1)
43-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLogic is correct; consider extracting the duplicated must-gather gating boilerplate.
The
if p, mgErr := mustgather.GetProvider(); mgErr == nil && p != nil { ... } else { dynamic client }pattern is now repeated nearly verbatim incoredns.go,endpoints.go, androutes.go. A small helper (e.g.mustgather.LoadedProvider() (*mg.Provider, bool)) would centralize the gate and keep the three handlers consistent if the contract changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolsets/netedge/coredns.go` around lines 43 - 57, The must-gather provider check is duplicated across CoreDNS-related handlers, so extract the repeated gating logic into a shared helper to keep the behavior consistent. Add a small helper around mustgather.GetProvider (for example in the netedge package) that returns the loaded provider and a boolean, then update coredns.go and the matching handlers in endpoints.go and routes.go to use it instead of inlining the same if/else pattern. Keep the existing behavior in CoreDNSHandler and related lookup paths unchanged while centralizing the must-gather fallback decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@evals/tasks/netedge/inspect_route/offline.yaml`:
- Around line 7-10: The offline verification is too weak because contains:
"console" can be satisfied by the prompt itself instead of the must-gather data.
Update the verify section in the offline task to assert on a route-specific
value from the fixture, such as the OpenShift console route host or TLS
termination type like reencrypt, so the check proves the answer was derived from
the offline data. Keep the existing mustgather_use flow and target the route
fields referenced by the task description.
In `@pkg/toolsets/mustgather/eval_offline_test.go`:
- Around line 23-24: The test currently hard-codes a developer-specific archive
path, so it will fail outside one machine. Update the fixture setup in
eval_offline_test.go to resolve the must-gather archive relative to the test
file or testdata location before calling mg.NewProvider, so the same archivePath
works in CI and on any developer environment.
- Around line 1-11: Move the test to package mustgather_test to avoid the import
cycle created by importing netedge from mustgather, and update it to exercise
the public offline tool entrypoint instead of calling setProvider directly. Use
the exported mustgather flow from mg and the tool API in api to load the archive
through the same path users use. Also replace the hard-coded /Users/... testdata
path with a repository-relative lookup so the test is portable across
environments.
---
Nitpick comments:
In `@pkg/toolsets/netedge/coredns.go`:
- Around line 43-57: The must-gather provider check is duplicated across
CoreDNS-related handlers, so extract the repeated gating logic into a shared
helper to keep the behavior consistent. Add a small helper around
mustgather.GetProvider (for example in the netedge package) that returns the
loaded provider and a boolean, then update coredns.go and the matching handlers
in endpoints.go and routes.go to use it instead of inlining the same if/else
pattern. Keep the existing behavior in CoreDNSHandler and related lookup paths
unchanged while centralizing the must-gather fallback decision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 14b9e86a-6641-4496-b1af-a328149b63a2
📒 Files selected for processing (22)
evals/gemini-agent/agent.yamlevals/gemini-agent/eval.yamlevals/tasks/netedge/get-coredns-config/offline.yamlevals/tasks/netedge/get_service_endpoints/offline.yamlevals/tasks/netedge/inspect_route/offline.yamlevals/testdata/must-gather/namespaces/openshift-console/routes.yamlevals/testdata/must-gather/namespaces/openshift-dns/configmaps.yamlevals/testdata/must-gather/namespaces/openshift-ingress/endpointslices.yamlevals/testdata/must-gather/timestampevals/testdata/must-gather/versionpkg/toolsets/mustgather/etcd.gopkg/toolsets/mustgather/eval_offline_test.gopkg/toolsets/mustgather/events.gopkg/toolsets/mustgather/helpers.gopkg/toolsets/mustgather/mcp_resources.gopkg/toolsets/mustgather/monitoring.gopkg/toolsets/mustgather/nodes.gopkg/toolsets/mustgather/pod_logs.gopkg/toolsets/mustgather/resources.gopkg/toolsets/netedge/coredns.gopkg/toolsets/netedge/endpoints.gopkg/toolsets/netedge/routes.go
|
/assign @swghosh |
|
/assign @Thealisyed |
|
/assign @candita |
|
@Cali0707 This can merge please. I think we have all the feedback (and have answered) we're going to get. |
touching up evals as discussed |
This commit implements offline must-gather support for the get_service_endpoints tool, using mustgather.GetProvider() to query the offline index when available. It also includes the necessary mock must-gather data and mcpchecker tasks to validate the implementation.
This commit exports GetProvider as a public function in pkg/toolsets/mustgather/helpers.go so that other packages (like netedge) can access the offline provider when needed. It updates all existing internal references to match the new casing.
This commit implements offline must-gather support for the get_coredns_config tool, using mustgather.GetProvider() to query the offline index when available. It also includes the necessary mock must-gather data and mcpchecker tasks to validate the implementation.
… separate package
…ting and strengthen task assertions
openshift-ci
Bot
added
the
needs-rebase
…ific configs Refactor NIDS/netedge evaluations to adopt the new core-eval-testing format. Standardize netedge task labels, remove deprecated global and nested eval configs, and sync build/evals.mk to support per-suite execution.
…c configs Update the mcpchecker.yaml workflow to run evaluations using the newly introduced decentralized suite-specific configurations under core-eval-testing/ instead of the legacy global files. Add support for the netedge suite.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
build/evals.mk (1)
11-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the ACP adapter version.
Defaulting to
latestand installing without a committed lockfile makes evaluation behavior non-reproducible and allows an unreviewed CLI change to break ACP runs. Pin a tested version and install from a locked dependency set.Also applies to: 48-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/evals.mk` at line 11, Update the CLAUDE_AGENT_ACP_VERSION configuration and the associated ACP installation commands to use a specific tested version instead of latest, and install from a committed lockfile or otherwise locked dependency set. Apply the same pinned, reproducible setup to the additional installation references noted in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/mcpchecker.yaml:
- Line 203: Add the missing evals/core-eval-testing/builtin-openai/eval-all.yaml
configuration for the supported all suite. Ensure it follows the schema and task
definitions expected by the existing suite eval files so the eval-config path
used by the workflow resolves for evaluation, baseline lookup, and result
publishing.
In `@build/evals.mk`:
- Around line 20-26: Update the EVAL_LABEL_SELECTOR assignment in the build
configuration so SUITE=all produces an empty selector, matching the behavior in
the CI workflow and allowing every suite to run; preserve suite=$(SUITE) for all
other suite values.
---
Nitpick comments:
In `@build/evals.mk`:
- Line 11: Update the CLAUDE_AGENT_ACP_VERSION configuration and the associated
ACP installation commands to use a specific tested version instead of latest,
and install from a committed lockfile or otherwise locked dependency set. Apply
the same pinned, reproducible setup to the additional installation references
noted in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6ade37d-c062-4469-b0c2-2caff3ff216e
📒 Files selected for processing (27)
.github/workflows/mcpchecker.yamlbuild/evals.mkevals/claude-code/agent.yamlevals/claude-code/eval.yamlevals/core-eval-testing/acp-anthropic/eval-netedge.yamlevals/core-eval-testing/acp-google/eval-netedge.yamlevals/core-eval-testing/builtin-anthropic/eval-netedge.yamlevals/core-eval-testing/builtin-google/eval-netedge.yamlevals/core-eval-testing/builtin-openai/eval-netedge.yamlevals/gemini-agent/agent.yamlevals/gemini-agent/eval.yamlevals/openai-agent/agent.yamlevals/openai-agent/eval.yamlevals/tasks/kubevirt/claude-code/eval.yamlevals/tasks/kubevirt/openai-agent/eval.yamlevals/tasks/netedge/get-coredns-config/offline.yamlevals/tasks/netedge/get-coredns-config/task.yamlevals/tasks/netedge/get-router-config/task.yamlevals/tasks/netedge/get-router-info/task.yamlevals/tasks/netedge/get-router-sessions/task.yamlevals/tasks/netedge/get_service_endpoints/offline.yamlevals/tasks/netedge/get_service_endpoints/task.yamlevals/tasks/netedge/inspect_route/offline.yamlevals/tasks/netedge/inspect_route/task.yamlevals/tasks/netedge/probe_dns_local/task.yamlevals/tasks/netedge/probe_http/task.yamlevals/tasks/netedge/query_prometheus/task.yaml
💤 Files with no reviewable changes (8)
- evals/tasks/kubevirt/openai-agent/eval.yaml
- evals/openai-agent/eval.yaml
- evals/claude-code/eval.yaml
- evals/tasks/kubevirt/claude-code/eval.yaml
- evals/openai-agent/agent.yaml
- evals/claude-code/agent.yaml
- evals/gemini-agent/eval.yaml
- evals/gemini-agent/agent.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- evals/tasks/netedge/get-coredns-config/offline.yaml
- evals/tasks/netedge/inspect_route/offline.yaml
- evals/tasks/netedge/get_service_endpoints/offline.yaml
a724046 to
4a17c89
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@bentito: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
done. seeing if I can check them with |
|
/run-mcpchecker netedge |
7 participants
Purpose
This PR adds offline must-gather support for the NIDS toolset. It enables users (like support and QE engineers) to perform static diagnostics against must-gather archives without requiring a live-cluster connection. Specifically, the following 3 tools have been modified to support must-gather offline mode:
inspect_routeget_coredns_configget_service_endpointsChanges
getProviderto exportedGetProvider()inpkg/toolsets/mustgather/helpers.goso that other toolsets (e.g.netedge/ NIDS) can access the loaded must-gather data structure.inspect_route: UpdatedinspectRouteinpkg/toolsets/netedge/routes.goto search for resources inside the must-gather provider first. Falls back to the dynamic client for live-cluster access only if no offline provider is active.get_coredns_config: Updatedget_coredns_configinpkg/toolsets/netedge/coredns.goto support offline querying.get_service_endpoints: Updatedget_service_endpointsinpkg/toolsets/netedge/endpoints.goto support offline querying.mcpcheckertask definitions (inspect_route/offline.yaml,get-coredns-config/offline.yaml, andget_service_endpoints/offline.yaml) alongside their mock must-gather data structures.Co-authored-by: gemini and bentito
Summary by CodeRabbit
/healthz).