CORENET-7137, CORENET-7138: cni-diagnostics MCP tools for kernel and network-tools layer#375
Open
arkadeepsen wants to merge 5 commits into
Open
CORENET-7137, CORENET-7138: cni-diagnostics MCP tools for kernel and network-tools layer#375arkadeepsen wants to merge 5 commits into
arkadeepsen wants to merge 5 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CORENET-7137 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. This pull request references CORENET-7138 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CNI diagnostics MCP tools for kernel inspection, packet capture, and eBPF tracing, with validation, execution limits, documentation, configuration references, toolset registration coverage, and end-to-end evaluation workflows. Also updates node-debug timeout handling to preserve existing context deadlines. ChangesCNI Diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant MCPServer
participant DiagnosticHandler
participant DebugNode
Agent->>MCPServer: invoke diagnostic tool
MCPServer->>DiagnosticHandler: validate parameters and build command
DiagnosticHandler->>DebugNode: execute command
DebugNode-->>DiagnosticHandler: return stdout and stderr
DiagnosticHandler-->>Agent: return bounded diagnostic 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: arkadeepsen 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: 7
♻️ Duplicate comments (1)
pkg/toolsets/ovnkubernetes/network-tools/tools.go (1)
265-296: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame dead negative-check bug as
tcpdumpHandler(Line 114/165).
timeoutSecs(Line 265) is cast touint32before thetimeoutSecs >= 0check (Line 292), making the check always true and letting negative input silently wrap and get clamped to the max timeout instead of returning the validation error.🐛 Proposed fix
- timeoutSecs := uint32(p.OptionalInt64("timeout_seconds", 0)) + timeoutSecs := p.OptionalInt64("timeout_seconds", 0)// Validate timeout if timeoutSecs >= 0 { - upstreamParams.TimeoutParams.TimeoutSeconds = min(timeoutSecs, uint32(timeout.MaxTimeout.Seconds())) + upstreamParams.TimeoutParams.TimeoutSeconds = min(uint32(timeoutSecs), uint32(timeout.MaxTimeout.Seconds())) } else { return api.NewToolCallResult("", fmt.Errorf("timeout_seconds must be greater than or equal to 0")), nil }🤖 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/ovnkubernetes/network-tools/tools.go` around lines 265 - 296, The timeout validation in `tools.go` has the same dead negative-check issue as `tcpdumpHandler`: `timeoutSecs` is converted to `uint32` before validation, so `timeoutSecs >= 0` can never fail and negative input wraps silently. Move the validation to use the original `p.OptionalInt64("timeout_seconds", 0)` result before casting, then only assign to `upstreamParams.TimeoutParams.TimeoutSeconds` after confirming it is non-negative and clamping it with `timeout.MaxTimeout` as needed.
🧹 Nitpick comments (7)
pkg/toolsets/ovnkubernetes/adapter/adapter_test.go (1)
9-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer plain
TestXxxfunctions over a suite for this trivial test file.This file has two ~2-line assertions with no shared setup/teardown. A
testify/suiteadds ceremony without benefit here.♻️ Proposed simplification
-type AdapterSuite struct { - suite.Suite -} - -func (s *AdapterSuite) TestNewRunDebugNodeCommand() { - s.Run("returns function with correct signature", func() { - fn := NewRunDebugNodeCommand(nil) - s.NotNil(fn, "adapter should return a non-nil function") - }) -} - -func (s *AdapterSuite) TestNewRunPodExecCommand() { - s.Run("returns function with correct signature", func() { - fn := NewRunPodExecCommand(nil) - s.NotNil(fn, "adapter should return a non-nil function") - }) -} - -func TestAdapter(t *testing.T) { - suite.Run(t, new(AdapterSuite)) -} +func TestNewRunDebugNodeCommand(t *testing.T) { + fn := NewRunDebugNodeCommand(nil) + require.NotNil(t, fn, "adapter should return a non-nil function") +} + +func TestNewRunPodExecCommand(t *testing.T) { + fn := NewRunPodExecCommand(nil) + require.NotNil(t, fn, "adapter should return a non-nil function") +}Based on learnings, "In Go *_test.go files, avoid using testify/suite for small/simple tests...implement it as a direct
func TestXxx(t *testing.T)", reserving suite for files with meaningful shared setup or multiple related subtests.🤖 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/ovnkubernetes/adapter/adapter_test.go` around lines 9 - 29, The adapter test file is using testify/suite for two trivial assertions with no shared setup, so simplify it to plain TestXxx-style tests. Replace the AdapterSuite methods and suite.Run usage with direct testing.T functions that call NewRunDebugNodeCommand and NewRunPodExecCommand, keeping the same assertions while removing the suite boilerplate.Source: Learnings
pkg/toolsets/ovnkubernetes/network-tools/tools_test.go (2)
64-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTests call unexported
initTcpdumpTool()/initPwruTool()directly instead of the publicInitNetworkTools()API.
TestInitTcpdump/TestInitPwru(and the handler tests below) reach into the private constructors rather than exercising the package through its exported surface.As per coding guidelines, "Test the public API only - tests should be black-box and not access internal/private functions." Consider deriving
toolby filtering the slice returned fromInitNetworkTools()byTool.Nameinstead.Also applies to: 212-254
🤖 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/ovnkubernetes/network-tools/tools_test.go` around lines 64 - 126, The tests are reaching into private constructors instead of the package’s public API. Update TestInitTcpdump, TestInitPwru, and the related handler tests to obtain tools from InitNetworkTools() and select the matching entry by Tool.Name rather than calling initTcpdumpTool() or initPwruTool() directly. Keep the existing assertions, but make them run against the exported API surface so the tests remain black-box.Source: Coding guidelines
128-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing negative
timeout_secondstest case — would have caught the validation bug intools.go.
TestTcpdumpHandler/TestPwruHandlercover negativepacket_count/snaplen/output_limit_lines, but there's no equivalent"negative timeout_seconds returns validation error"case. Because of theuint32cast bug flagged intools.go, such a test would currently fail and expose the issue.Also applies to: 256-296
🤖 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/ovnkubernetes/network-tools/tools_test.go` around lines 128 - 210, Add a missing validation test for negative timeout_seconds in the handler tests to catch the uint32 cast bug in tools.go. Extend TestTcpdumpHandler and the corresponding TestPwruHandler case by calling tool.Handler with a negative timeout_seconds value and asserting a validation error message like the existing packet_count/snaplen/output_limit_lines checks. Use the existing handler setup in TestTcpdumpHandler/TestPwruHandler so the new case stays consistent with the other parameter-validation subtests.pkg/toolsets/ovnkubernetes/kernel/tools.go (1)
155-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated boilerplate for image resolution and MCP-server construction across all four handlers.
Consider extracting a small helper (e.g.
resolveImage(params) stringandnewKernelServer(params, image) (*ovnkmcp.Server, error)) to avoid the 4x duplication of image lookup +ovnkmcp.NewMCPServerconstruction.Also applies to: 317-331, 473-487, 637-651
🤖 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/ovnkubernetes/kernel/tools.go` around lines 155 - 169, The image resolution and ovnkmcp.NewMCPServer setup are duplicated across all four kernel handlers, so extract shared helpers to centralize this logic. Add a small resolveImage(params) helper for the config.DefaultNetshootImage / ovnkCfg.KernelDebugImage lookup, and a newKernelServer(params, image) helper that wraps adapter.NewRunDebugNodeCommand and ovnkmcp.NewMCPServer. Update each handler to call these helpers so the repeated boilerplate is removed while preserving the existing error handling.pkg/toolsets/ovnkubernetes/kernel/tools_test.go (1)
121-168: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTests validate the bugged behaviors in
tools.goas if they were correct — negative timeout is not exercised.The suite tests
negative headandnegative tailvalidation but has no equivalent test fornegative timeout_seconds, which (per the flagged issue intools.go) currently silently succeeds instead of erroring. Adding this test would have caught the bug.🤖 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/ovnkubernetes/kernel/tools_test.go` around lines 121 - 168, Add a new negative validation test in ToolsSuite.TestConntrackHandler for timeout_seconds, since the current suite only covers head and tail and misses the bug in initConntrackTool/Handler where a negative timeout can silently pass. Use s.handlerParams with node plus timeout_seconds set to a negative value, then assert tool.Handler returns a result error with the expected validation message, matching the style of the existing negative head and negative tail cases.evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh (1)
13-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the image tag instead of using
:latest.Using
:latestfor the test pod image can cause non-deterministic eval failures if the upstream image changes unexpectedly.♻️ Pin the image version
- image: quay.io/nginx/nginx-unprivileged:latest + image: quay.io/nginx/nginx-unprivileged:1.27🤖 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 `@evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh` around lines 13 - 25, The test pod in setup.sh uses quay.io/nginx/nginx-unprivileged:latest, which can make the eval unstable. Update the Pod manifest in the kubectl apply block for test-client/nginx to use a fixed, pinned image tag instead of latest so the environment is deterministic.evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnpinned
:latestimage tag.Using
quay.io/quay/busybox:latestrisks non-deterministic eval runs if the tag changes upstream. Consider pinning to a specific digest or version tag for reproducibility.🤖 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 `@evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh` around lines 20 - 23, The busybox container image is using an unpinned latest tag, which can make runs non-deterministic. Update the container definition in setup.sh to use a specific version tag or immutable digest instead of quay.io/quay/busybox:latest. Keep the change localized to the busybox image reference in the containers spec so the eval remains reproducible.
🤖 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 `@docs/openshift/ovn-kubernetes.md`:
- Around line 453-474: The workflow examples under the packet-path and DNS
diagnostics sections are duplicating the `-L` flag by putting it in both the
`command` field and `filter_parameters`, which makes the samples misleading.
Update the examples so `command` remains the source of `-L` and remove the extra
`-L` from each affected `get-iptables` example in the documentation snippet,
keeping the other filter arguments unchanged.
- Line 174: The documentation table entry for filter_parameters currently shows
an inherited default of filter, which is incorrect and implies a nonexistent
positional argument. Update the row in the ovn-kubernetes docs so
filter_parameters defaults to empty/no default, and keep the description aligned
with the actual accepted iptables flags; use the filter_parameters symbol in the
table to locate and correct this entry.
In `@evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh`:
- Around line 30-31: Update the DNS lookup in setup.sh so it works on OpenShift
as well as Kubernetes: the current kubectl query for kube-dns can leave DNS_IP
empty on OpenShift. Adjust the DNS_IP assignment logic to first try the
OpenShift DNS service (openshift-dns/dns-default) and fall back to the existing
kube-dns lookup if needed, keeping the echo of DNS_IP in place. Use the existing
DNS_IP setup block as the target for this change.
In `@evals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yaml`:
- Around line 11-16: The verifier in the task definition currently only checks
for a tcpdump capture with a BPF filter, so it can pass without meeting the
packet-count requirement. Update the verify.contains check in the task YAML so
it explicitly requires capturing 50 packets, keeping the existing TCP/BPF and
worker-node context tied to the tcpdump-based task prompt.
In `@pkg/toolsets/ovnkubernetes/kernel/tools.go`:
- Around line 111-118: `timeout_seconds` validation is currently bypassed
because `tools.go` converts the parsed value to `uint32` too early and then
checks it as if it could be negative. Update the parsing/validation flow in
`kernelHandler`, `iptablesHandler`, `nftHandler`, and `ipHandler` so
`OptionalInt64("timeout_seconds", 0)` is validated as a signed value first,
rejecting negatives before any unsigned conversion. Only convert to `uint32`
after validation, and keep the timeout clamping logic using the validated
non-negative value.
In `@pkg/toolsets/ovnkubernetes/network-tools/tools.go`:
- Around line 114-169: The timeout validation in the tcpdump tool is incorrect
because `timeout_seconds` is converted to `uint32` before checking for
negativity, so the `timeoutSecs >= 0` guard in the `tools.go` flow can never
catch invalid negative input. Keep the parsed `timeout_seconds` value signed in
the parameter handling around `p.OptionalInt64`, validate it before any cast,
and only convert to `uint32` when assigning to
`upstreamParams.TimeoutParams.TimeoutSeconds` in the tcpdump setup.
In `@vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.go`:
- Around line 30-46: The warning filter in filterWarnings is too broad because
it removes any line containing the word warning in any case, which can hide
legitimate diagnostics. Update filterWarnings to match only the known warning
prefix/banner used by the tool instead of using strings.Contains on the whole
line, and keep the logic localized to the existing filterWarnings function in
util.go so only actual warning output is suppressed.
---
Duplicate comments:
In `@pkg/toolsets/ovnkubernetes/network-tools/tools.go`:
- Around line 265-296: The timeout validation in `tools.go` has the same dead
negative-check issue as `tcpdumpHandler`: `timeoutSecs` is converted to `uint32`
before validation, so `timeoutSecs >= 0` can never fail and negative input wraps
silently. Move the validation to use the original
`p.OptionalInt64("timeout_seconds", 0)` result before casting, then only assign
to `upstreamParams.TimeoutParams.TimeoutSeconds` after confirming it is
non-negative and clamping it with `timeout.MaxTimeout` as needed.
---
Nitpick comments:
In `@evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh`:
- Around line 20-23: The busybox container image is using an unpinned latest
tag, which can make runs non-deterministic. Update the container definition in
setup.sh to use a specific version tag or immutable digest instead of
quay.io/quay/busybox:latest. Keep the change localized to the busybox image
reference in the containers spec so the eval remains reproducible.
In `@evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh`:
- Around line 13-25: The test pod in setup.sh uses
quay.io/nginx/nginx-unprivileged:latest, which can make the eval unstable.
Update the Pod manifest in the kubectl apply block for test-client/nginx to use
a fixed, pinned image tag instead of latest so the environment is deterministic.
In `@pkg/toolsets/ovnkubernetes/adapter/adapter_test.go`:
- Around line 9-29: The adapter test file is using testify/suite for two trivial
assertions with no shared setup, so simplify it to plain TestXxx-style tests.
Replace the AdapterSuite methods and suite.Run usage with direct testing.T
functions that call NewRunDebugNodeCommand and NewRunPodExecCommand, keeping the
same assertions while removing the suite boilerplate.
In `@pkg/toolsets/ovnkubernetes/kernel/tools_test.go`:
- Around line 121-168: Add a new negative validation test in
ToolsSuite.TestConntrackHandler for timeout_seconds, since the current suite
only covers head and tail and misses the bug in initConntrackTool/Handler where
a negative timeout can silently pass. Use s.handlerParams with node plus
timeout_seconds set to a negative value, then assert tool.Handler returns a
result error with the expected validation message, matching the style of the
existing negative head and negative tail cases.
In `@pkg/toolsets/ovnkubernetes/kernel/tools.go`:
- Around line 155-169: The image resolution and ovnkmcp.NewMCPServer setup are
duplicated across all four kernel handlers, so extract shared helpers to
centralize this logic. Add a small resolveImage(params) helper for the
config.DefaultNetshootImage / ovnkCfg.KernelDebugImage lookup, and a
newKernelServer(params, image) helper that wraps adapter.NewRunDebugNodeCommand
and ovnkmcp.NewMCPServer. Update each handler to call these helpers so the
repeated boilerplate is removed while preserving the existing error handling.
In `@pkg/toolsets/ovnkubernetes/network-tools/tools_test.go`:
- Around line 64-126: The tests are reaching into private constructors instead
of the package’s public API. Update TestInitTcpdump, TestInitPwru, and the
related handler tests to obtain tools from InitNetworkTools() and select the
matching entry by Tool.Name rather than calling initTcpdumpTool() or
initPwruTool() directly. Keep the existing assertions, but make them run against
the exported API surface so the tests remain black-box.
- Around line 128-210: Add a missing validation test for negative
timeout_seconds in the handler tests to catch the uint32 cast bug in tools.go.
Extend TestTcpdumpHandler and the corresponding TestPwruHandler case by calling
tool.Handler with a negative timeout_seconds value and asserting a validation
error message like the existing packet_count/snaplen/output_limit_lines checks.
Use the existing handler setup in TestTcpdumpHandler/TestPwruHandler so the new
case stays consistent with the other parameter-validation subtests.
🪄 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: dd7791e9-d164-464d-babc-7887d1b0a1bc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (64)
docs/openshift/README.mddocs/openshift/ovn-kubernetes.mdevals/claude-code/eval.yamlevals/gemini-agent/eval.yamlevals/openai-agent/eval.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/ovn-kubernetes/kernel-ip-routes/setup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/ovn-kubernetes/kernel-iptables-list/setup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.shevals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yamlevals/tasks/ovn-kubernetes/kernel-nft-list/setup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.shevals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.shevals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yamlevals/tasks/ovn-kubernetes/network-pwru-trace/setup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.shgo.modpkg/cluster-diagnostics/nodesdebug/nodes_debug.gopkg/mcp/modules.gopkg/mcp/testdata/toolsets-ovn-kubernetes-tools.jsonpkg/mcp/toolsets_test.gopkg/toolsets/ovnkubernetes/adapter/adapter.gopkg/toolsets/ovnkubernetes/adapter/adapter_test.gopkg/toolsets/ovnkubernetes/config/config.gopkg/toolsets/ovnkubernetes/config/config_test.gopkg/toolsets/ovnkubernetes/kernel/tools.gopkg/toolsets/ovnkubernetes/kernel/tools_test.gopkg/toolsets/ovnkubernetes/network-tools/tools.gopkg/toolsets/ovnkubernetes/network-tools/tools_test.gopkg/toolsets/ovnkubernetes/toolset.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSEvendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.govendor/modules.txt
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 7, 2026 14:25
9991e34 to
dfea2c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/toolsets/ovnkubernetes/utils/utils.go (1)
75-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate near-identical image-lookup helpers.
KernelDebugImage,TcpdumpImage, andPwruImagediffer only by config field and default constant. A small generic helper would remove the duplication if more image types are added later.♻️ Possible consolidation
+func imageOrDefault(params api.ToolHandlerParams, defaultImage string, pick func(*config.Config) string) string { + image := defaultImage + if ovnkCfg := config.GetConfig(params); ovnkCfg != nil { + image = pick(ovnkCfg) + } + return image +} + func KernelDebugImage(params api.ToolHandlerParams) string { - image := config.DefaultNetshootImage - if ovnkCfg := config.GetConfig(params); ovnkCfg != nil { - image = ovnkCfg.KernelDebugImage - } - return image + return imageOrDefault(params, config.DefaultNetshootImage, func(c *config.Config) string { return c.KernelDebugImage }) }🤖 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/ovnkubernetes/utils/utils.go` around lines 75 - 103, Consolidate the repeated image-selection logic in KernelDebugImage, TcpdumpImage, and PwruImage by extracting a shared helper that takes the default image and the matching config field selector from config.GetConfig(params). Each function should then delegate to that helper while preserving its current default fallback behavior, so adding new image lookups later only requires one place to maintain.evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentical cleanup script duplicated across multiple eval tasks.
This exact script (same shebang,
setflags, and delete command) is repeated verbatim inkernel-conntrack-list,kernel-iptables-list, and (with an extra line)kernel-network-namespace-diagnosis/network-packet-drop-diagnosis. Consider extracting a shared helper script sourced by each task to avoid drift if the cleanup logic needs to change later.🤖 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 `@evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh` around lines 1 - 4, The cleanup logic in the cleanup.sh script is duplicated verbatim across multiple eval tasks, so refactor it into a shared helper script and have each task invoke or source that common entrypoint instead of maintaining separate copies. Keep the node-debug pod deletion behavior in the shared script, and update the task-specific cleanup.sh files (including the ones with the extra line) to reference the shared helper so future changes stay in sync. Refer to the cleanup.sh entrypoint and the kubectl delete pods command when relocating the logic.pkg/toolsets/ovnkubernetes/adapter/adapter_test.go (1)
9-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests only verify non-nil, not actual constructor behavior.
Both tests assert
require.NotNilon the returned closure but never invoke it or assert anything about its "correct signature" as the subtest name claims. This provides minimal coverage of the adapters' real behavior. Consider invoking the returned function (with a fake/real resource where feasible) and asserting on the resulting command, per the guideline to aim for high coverage of the public API including edge cases.🤖 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/ovnkubernetes/adapter/adapter_test.go` around lines 9 - 21, The adapter tests only check that NewRunDebugNodeCommand and NewRunPodExecCommand return a non-nil closure, so they do not verify the constructors’ real behavior. Update TestNewRunDebugNodeCommand and TestNewRunPodExecCommand to invoke the returned functions from those constructors (with a suitable fake or minimal valid input where possible) and assert the resulting command/output matches the expected behavior. Keep the checks focused on the public API of the adapter functions so the tests validate the actual signature and command generation rather than just existence.Source: Path instructions
🤖 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 `@pkg/toolsets/ovnkubernetes/adapter/adapter_test.go`:
- Around line 1-7: Change the test package in adapter_test.go from package
adapter to package adapter_test so the tests run as black-box tests. Update the
imports in the test file to explicitly import the adapter package and keep using
the exported APIs NewRunDebugNodeCommand and NewRunPodExecCommand from that
package. The existing direct testing style with testing and require is fine;
just ensure no internal package symbols are referenced after the package change.
---
Nitpick comments:
In `@evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh`:
- Around line 1-4: The cleanup logic in the cleanup.sh script is duplicated
verbatim across multiple eval tasks, so refactor it into a shared helper script
and have each task invoke or source that common entrypoint instead of
maintaining separate copies. Keep the node-debug pod deletion behavior in the
shared script, and update the task-specific cleanup.sh files (including the ones
with the extra line) to reference the shared helper so future changes stay in
sync. Refer to the cleanup.sh entrypoint and the kubectl delete pods command
when relocating the logic.
In `@pkg/toolsets/ovnkubernetes/adapter/adapter_test.go`:
- Around line 9-21: The adapter tests only check that NewRunDebugNodeCommand and
NewRunPodExecCommand return a non-nil closure, so they do not verify the
constructors’ real behavior. Update TestNewRunDebugNodeCommand and
TestNewRunPodExecCommand to invoke the returned functions from those
constructors (with a suitable fake or minimal valid input where possible) and
assert the resulting command/output matches the expected behavior. Keep the
checks focused on the public API of the adapter functions so the tests validate
the actual signature and command generation rather than just existence.
In `@pkg/toolsets/ovnkubernetes/utils/utils.go`:
- Around line 75-103: Consolidate the repeated image-selection logic in
KernelDebugImage, TcpdumpImage, and PwruImage by extracting a shared helper that
takes the default image and the matching config field selector from
config.GetConfig(params). Each function should then delegate to that helper
while preserving its current default fallback behavior, so adding new image
lookups later only requires one place to maintain.
🪄 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: 82d3ba8c-9977-4122-8177-0160a94b544c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (62)
docs/openshift/README.mddocs/openshift/ovn-kubernetes.mdevals/claude-code/eval.yamlevals/gemini-agent/eval.yamlevals/openai-agent/eval.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/ovn-kubernetes/kernel-ip-routes/setup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/ovn-kubernetes/kernel-iptables-list/setup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.shevals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yamlevals/tasks/ovn-kubernetes/kernel-nft-list/setup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.shevals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.shevals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yamlevals/tasks/ovn-kubernetes/network-pwru-trace/setup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.shgo.modpkg/mcp/testdata/toolsets-ovn-kubernetes-tools.jsonpkg/toolsets/ovnkubernetes/adapter/adapter.gopkg/toolsets/ovnkubernetes/adapter/adapter_test.gopkg/toolsets/ovnkubernetes/config/config.gopkg/toolsets/ovnkubernetes/config/config_test.gopkg/toolsets/ovnkubernetes/kernel/tools.gopkg/toolsets/ovnkubernetes/kernel/tools_test.gopkg/toolsets/ovnkubernetes/network-tools/tools.gopkg/toolsets/ovnkubernetes/network-tools/tools_test.gopkg/toolsets/ovnkubernetes/toolset.gopkg/toolsets/ovnkubernetes/utils/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSEvendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.govendor/modules.txt
✅ Files skipped from review due to trivial changes (9)
- evals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yaml
- docs/openshift/README.md
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSE
- evals/tasks/ovn-kubernetes/kernel-ip-routes/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml
- docs/openshift/ovn-kubernetes.md
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.go
- vendor/modules.txt
🚧 Files skipped from review as they are similar to previous changes (41)
- evals/openai-agent/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yaml
- evals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yaml
- pkg/toolsets/ovnkubernetes/toolset.go
- evals/tasks/ovn-kubernetes/kernel-nft-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yaml
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.go
- pkg/toolsets/ovnkubernetes/adapter/adapter.go
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.go
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yaml
- pkg/toolsets/ovnkubernetes/config/config_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.go
- evals/claude-code/eval.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/setup.sh
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh
- pkg/toolsets/ovnkubernetes/config/config.go
- evals/tasks/ovn-kubernetes/kernel-iptables-list/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.go
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.go
- go.mod
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.go
- pkg/toolsets/ovnkubernetes/network-tools/tools_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.go
- evals/gemini-agent/eval.yaml
- pkg/mcp/testdata/toolsets-ovn-kubernetes-tools.json
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.go
- pkg/toolsets/ovnkubernetes/network-tools/tools.go
- pkg/toolsets/ovnkubernetes/kernel/tools_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.go
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 7, 2026 15:47
dfea2c8 to
45aa97b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh (2)
2-7: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider
pipefailfor the node check.Without
set -o pipefail, if thekubectl get nodescall itself fails (e.g., RBAC/connectivity error), the pipeline's exit status comes fromgrep, so the real error is masked and misreported as "No non-control-plane nodes found."♻️ Optional hardening
#!/usr/bin/env bash -set -e +set -euo pipefail🤖 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 `@evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh` around lines 2 - 7, The node-check pipeline in setup.sh can mask failures from kubectl get nodes because only set -e is enabled. Update the script setup to enable pipefail alongside the existing error handling so the if ! kubectl get nodes ... | grep -q . check surfaces kubectl/RBAC/connectivity errors instead of misreporting "No non-control-plane nodes found."
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the busybox image tag.
quay.io/quay/busybox:latestis a floating tag; pinning improves reproducibility of eval runs.🤖 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 `@evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh` at line 47, The busybox image reference is using the floating tag, so update the setup logic that defines the busybox image to use a pinned, immutable tag instead. Locate the image declaration in setup.sh and replace the current busybox reference with a specific versioned tag so eval runs remain reproducible.
🤖 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.
Nitpick comments:
In `@evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh`:
- Around line 2-7: The node-check pipeline in setup.sh can mask failures from
kubectl get nodes because only set -e is enabled. Update the script setup to
enable pipefail alongside the existing error handling so the if ! kubectl get
nodes ... | grep -q . check surfaces kubectl/RBAC/connectivity errors instead of
misreporting "No non-control-plane nodes found."
- Line 47: The busybox image reference is using the floating tag, so update the
setup logic that defines the busybox image to use a pinned, immutable tag
instead. Locate the image declaration in setup.sh and replace the current
busybox reference with a specific versioned tag so eval runs remain
reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 05fb10cb-59c3-4bef-af2b-ffb3e017adfc
📒 Files selected for processing (30)
evals/claude-code/eval.yamlevals/gemini-agent/eval.yamlevals/openai-agent/eval.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/ovn-kubernetes/kernel-ip-routes/setup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/ovn-kubernetes/kernel-iptables-list/setup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.shevals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yamlevals/tasks/ovn-kubernetes/kernel-nft-list/setup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.shevals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.shevals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yamlevals/tasks/ovn-kubernetes/network-pwru-trace/setup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh
✅ Files skipped from review due to trivial changes (1)
- evals/openai-agent/eval.yaml
🚧 Files skipped from review as they are similar to previous changes (27)
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yaml
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh
- evals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.sh
- evals/claude-code/eval.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yaml
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-iptables-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-nft-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/setup.sh
- evals/gemini-agent/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-ip-routes/setup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh
|
/assign |
bentito
reviewed
Jul 9, 2026
| Type: "string", | ||
| Description: "Namespace of the debug pod on which the command is expected to be executed (optional, defaults to 'default')", | ||
| }, | ||
| "bpf_filter": { |
There was a problem hiding this comment.
Should --filter-track-skb be an option so that an agent can trace through NAT?
Member
Author
There was a problem hiding this comment.
For now, we're not providing support for all the flags. Based on customer feedback we may introduce support for additional flags.
bentito
reviewed
Jul 9, 2026
| github.com/opencontainers/image-spec v1.1.1/go.mod h1:qpqAh3Dmcf36wStyyWU+kCeDgrGnAve2nCC8+7h8Q0M= | ||
| github.com/os-observability/redhat-opentelemetry-collector/configschemas v0.0.0-20260603165435-b2d908d32435 h1:QxOm58H4ez7Loxcx0PXYcmux+2nXQjym43Ym9Riu5G4= | ||
| github.com/os-observability/redhat-opentelemetry-collector/configschemas v0.0.0-20260603165435-b2d908d32435/go.mod h1:ThsbZTM538g95wb3Aj/+XHfFfpW/YBHdO5o6W0f+IRw= | ||
| github.com/ovn-kubernetes/ovn-kubernetes-mcp v0.0.0-20260626131912-c16a1ad4a657 h1:AirHF65bOk6GLJNfnoMvSr7AR1h30joR7YS77E7l1lY= |
There was a problem hiding this comment.
Seems like this would be better to have as a standard SemVer tag, so if possible, cut a release tag and point at it
Member
Author
There was a problem hiding this comment.
I have responded to a similar question here: #375 (comment)
|
Just had one network and one go comment. Other than that, this PR looks great! |
Cali0707
reviewed
Jul 9, 2026
| github.com/google/uuid v1.6.0 | ||
| github.com/miekg/dns v1.1.72 | ||
| github.com/modelcontextprotocol/go-sdk v1.6.1 | ||
| github.com/ovn-kubernetes/ovn-kubernetes-mcp v0.0.0-20260626131912-c16a1ad4a657 |
There was a problem hiding this comment.
Is it possible to get a release of this repo so we don't have to pin to a prerelease like this?
Member
Author
There was a problem hiding this comment.
I'll have to check with the upstream maintainers regarding this. For the main ovn-kubernetes repo, the next release cut is on Aug 31 and the plan is to have a release of ovn-kubernetes-mcp repo together.
Comment on lines
+37
to
+52
| "node_name": { | ||
| Type: "string", | ||
| Description: "Name of the node (required when target_type is 'node')", | ||
| }, | ||
| "node_pod_namespace": { | ||
| Type: "string", | ||
| Description: "Namespace of the debug pod on which the command is expected to be executed when target_type is 'node' (optional, defaults to 'default')", | ||
| }, | ||
| "pod_name": { | ||
| Type: "string", | ||
| Description: "Name of the pod (required when target_type is 'pod')", | ||
| }, | ||
| "pod_namespace": { | ||
| Type: "string", | ||
| Description: "Namespace of the pod (required when target_type is 'pod')", | ||
| }, |
There was a problem hiding this comment.
Can these be condensed into just name/namespace parameters, and have the description explain it is for pod or node depending on the target_type?
| fn := NewRunPodExecCommand(nil) | ||
| require.NotNil(t, fn, "adapter should return a non-nil function") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Can we testify/suite as per the project conventions?
Member
Author
|
Currently working on the following upstream ovnk-mcp repo PRs which this PR depends on. Will work on the PR review once these 2 get merged: |
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 14, 2026 11:01
45aa97b to
c80bfe8
Compare
openshift-ci
Bot
added
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/toolsets/ovnkubernetes/utils/utils.go`:
- Around line 51-59: Update ValidateTimeout so min compares timeoutSecs and the
timeout maximum using int64 values before converting the selected result to
uint32. Preserve the existing nonnegative validation and assignment to
tp.TimeoutSeconds, ensuring oversized inputs are capped at timeout.MaxTimeout
rather than wrapping.
🪄 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: f81ff76a-eca1-4aca-90c5-1e9037d9601f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (62)
docs/openshift/README.mddocs/openshift/ovn-kubernetes.mdevals/claude-code/eval.yamlevals/gemini-agent/eval.yamlevals/openai-agent/eval.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/ovn-kubernetes/kernel-ip-routes/setup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/ovn-kubernetes/kernel-iptables-list/setup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.shevals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yamlevals/tasks/ovn-kubernetes/kernel-nft-list/setup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.shevals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.shevals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yamlevals/tasks/ovn-kubernetes/network-pwru-trace/setup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.shgo.modpkg/mcp/testdata/toolsets-ovn-kubernetes-tools.jsonpkg/toolsets/ovnkubernetes/adapter/adapter.gopkg/toolsets/ovnkubernetes/adapter/adapter_test.gopkg/toolsets/ovnkubernetes/config/config.gopkg/toolsets/ovnkubernetes/config/config_test.gopkg/toolsets/ovnkubernetes/kernel/tools.gopkg/toolsets/ovnkubernetes/kernel/tools_test.gopkg/toolsets/ovnkubernetes/network-tools/tools.gopkg/toolsets/ovnkubernetes/network-tools/tools_test.gopkg/toolsets/ovnkubernetes/toolset.gopkg/toolsets/ovnkubernetes/utils/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSEvendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.govendor/modules.txt
🚧 Files skipped from review as they are similar to previous changes (53)
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.sh
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yaml
- evals/tasks/ovn-kubernetes/kernel-iptables-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yaml
- evals/tasks/ovn-kubernetes/network-pwru-trace/setup.sh
- docs/openshift/README.md
- evals/tasks/ovn-kubernetes/kernel-ip-routes/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml
- evals/openai-agent/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yaml
- evals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.sh
- vendor/modules.txt
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yaml
- pkg/toolsets/ovnkubernetes/toolset.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.go
- evals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.go
- evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.go
- evals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.go
- pkg/toolsets/ovnkubernetes/config/config.go
- evals/gemini-agent/eval.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.go
- evals/claude-code/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.go
- pkg/toolsets/ovnkubernetes/kernel/tools_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.go
- pkg/toolsets/ovnkubernetes/adapter/adapter.go
- pkg/mcp/testdata/toolsets-ovn-kubernetes-tools.json
- pkg/toolsets/ovnkubernetes/kernel/tools.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.go
- pkg/toolsets/ovnkubernetes/network-tools/tools.go
- pkg/toolsets/ovnkubernetes/config/config_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.go
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.go
- pkg/toolsets/ovnkubernetes/network-tools/tools_test.go
- docs/openshift/ovn-kubernetes.md
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 14, 2026 11:09
c80bfe8 to
5fce9fe
Compare
openshift-ci
Bot
removed
the
needs-rebase
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 14, 2026 11:23
5fce9fe to
52ec1ff
Compare
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
2 times, most recently
from
July 14, 2026 13:00
4fcf982 to
59f69a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
pkg/toolsets/ovnkubernetes/network-tools/tools_test.go (2)
269-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the advertised validation matrix.
Add
max+1cases for packet count, snap length, and output limits. PWru also lacks invalid-type coverage fornode_pod_namespace,bpf_filter, andtimeout_seconds, plus a negative timeout case.As per coding guidelines, tests should cover edge cases including invalid indices, type mismatches, and malformed input.
Also applies to: 391-410
🤖 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/ovnkubernetes/network-tools/tools_test.go` around lines 269 - 300, Expand the validation tests around the existing packet_count and snaplen cases, plus the output-limit tests, to cover max+1 values and assert the expected validation errors. Add PWru handler cases for invalid types of node_pod_namespace, bpf_filter, and timeout_seconds, and add a negative timeout_seconds case with the corresponding validation error assertions.Source: Coding guidelines
138-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd a valid invocation test for each handler.
Both suites exercise only rejected inputs, leaving the adapter, execution, and successful-result paths untested. Add real implementation-backed cases or verify equivalent integration/eval coverage exists elsewhere.
As per coding guidelines, “Avoid mocks in tests; prefer real implementations and integration-style testing where possible” and “Aim for high coverage of the public API.”
Also applies to: 371-373
🤖 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/ovnkubernetes/network-tools/tools_test.go` around lines 138 - 140, Add successful invocation coverage for each tool handler in the relevant test suites, including TestTcpdumpHandler and the corresponding handler tests. Use valid inputs with real implementations to exercise adapter execution and successful results, or reuse existing integration/evaluation coverage if it already verifies these paths; retain the rejected-input cases.Source: Coding guidelines
🤖 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/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yaml`:
- Line 12: The criterion text must require evidence that routing information
covers every routing table on the worker node, not merely route entries, table
identifiers, or default routes from the main table. Update the contains
criterion in kernel-ip-routes.yaml to explicitly require all tables and their
route entries, while retaining the existing worker-node context.
In `@evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.sh`:
- Around line 19-24: Update the busybox pod specification in the generated
manifest to require scheduling on a worker node, using node affinity or the
cluster’s established worker-node label. Preserve the existing container
configuration and ensure the fixture cannot schedule onto control-plane nodes.
In `@pkg/toolsets/ovnkubernetes/network-tools/tools_test.go`:
- Around line 107-111: Update the “target_type parameter has supported values”
test to assert that the enum contains exactly two entries, while retaining
checks for “node” and “pod”; use the existing enum variable and add an
exact-length assertion so extra values fail.
- Around line 47-56: Update ToolsSuite.toolByName to use a fatal test failure
when no matching tool is found, replacing s.Failf with s.T().Fatalf so execution
stops before returning the empty ServerTool and invoking a nil handler.
---
Nitpick comments:
In `@pkg/toolsets/ovnkubernetes/network-tools/tools_test.go`:
- Around line 269-300: Expand the validation tests around the existing
packet_count and snaplen cases, plus the output-limit tests, to cover max+1
values and assert the expected validation errors. Add PWru handler cases for
invalid types of node_pod_namespace, bpf_filter, and timeout_seconds, and add a
negative timeout_seconds case with the corresponding validation error
assertions.
- Around line 138-140: Add successful invocation coverage for each tool handler
in the relevant test suites, including TestTcpdumpHandler and the corresponding
handler tests. Use valid inputs with real implementations to exercise adapter
execution and successful results, or reuse existing integration/evaluation
coverage if it already verifies these paths; retain the rejected-input cases.
🪄 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: 499fd64e-2259-451c-aaf0-d4641540fd1d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
docs/openshift/README.mddocs/openshift/ovn-kubernetes.mdevals/claude-code/eval.yamlevals/gemini-agent/eval.yamlevals/openai-agent/eval.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.shevals/tasks/ovn-kubernetes/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/ovn-kubernetes/kernel-ip-routes/setup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/ovn-kubernetes/kernel-iptables-list/setup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/setup.shevals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.shevals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yamlevals/tasks/ovn-kubernetes/kernel-nft-list/setup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.shevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.shevals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.shevals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yamlevals/tasks/ovn-kubernetes/network-pwru-trace/setup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.shevals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.shgo.modpkg/cluster-diagnostics/nodesdebug/nodes_debug.gopkg/mcp/modules.gopkg/mcp/testdata/toolsets-ovn-kubernetes-tools.jsonpkg/mcp/toolsets_test.gopkg/toolsets/ovnkubernetes/adapter/adapter.gopkg/toolsets/ovnkubernetes/adapter/adapter_test.gopkg/toolsets/ovnkubernetes/config/config.gopkg/toolsets/ovnkubernetes/config/config_test.gopkg/toolsets/ovnkubernetes/kernel/tools.gopkg/toolsets/ovnkubernetes/kernel/tools_test.gopkg/toolsets/ovnkubernetes/network-tools/tools.gopkg/toolsets/ovnkubernetes/network-tools/tools_test.gopkg/toolsets/ovnkubernetes/toolset.gopkg/toolsets/ovnkubernetes/utils/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSEvendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.govendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.govendor/modules.txt
🚧 Files skipped from review as they are similar to previous changes (60)
- evals/tasks/ovn-kubernetes/kernel-nft-list/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/cleanup.sh
- docs/openshift/README.md
- pkg/mcp/modules.go
- evals/claude-code/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-iptables-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-nft-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/cleanup.sh
- evals/tasks/ovn-kubernetes/kernel-ip-routes/setup.sh
- evals/tasks/ovn-kubernetes/kernel-iptables-list/kernel-iptables-list.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/setup.sh
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/cleanup.sh
- pkg/toolsets/ovnkubernetes/adapter/adapter_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/commandbuilder/command_builder.go
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/cleanup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/LICENSE
- evals/gemini-agent/eval.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/setup.sh
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/network-tcpdump-basic.yaml
- evals/tasks/ovn-kubernetes/kernel-conntrack-list/kernel-conntrack-list.yaml
- evals/tasks/ovn-kubernetes/kernel-nft-list/kernel-nft-list.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/pwru.go
- evals/tasks/ovn-kubernetes/network-pwru-trace/cleanup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/tcpdump.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/types/manifest.go
- evals/tasks/ovn-kubernetes/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml
- evals/openai-agent/eval.yaml
- go.mod
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/util.go
- evals/tasks/ovn-kubernetes/kernel-conntrack-filter/kernel-conntrack-filter.yaml
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/mcp.go
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yaml
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/cleanup.sh
- pkg/toolsets/ovnkubernetes/toolset.go
- pkg/toolsets/ovnkubernetes/config/config_test.go
- pkg/toolsets/ovnkubernetes/network-tools/tools.go
- vendor/modules.txt
- pkg/cluster-diagnostics/nodesdebug/nodes_debug.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/utils.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/ip.go
- pkg/toolsets/ovnkubernetes/kernel/tools.go
- evals/tasks/ovn-kubernetes/network-tcpdump-basic/setup.sh
- pkg/toolsets/ovnkubernetes/kernel/tools_test.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/timeout/timeout.go
- evals/tasks/ovn-kubernetes/kernel-iptables-list/cleanup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/utils.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/utils/headtail/head_tail.go
- pkg/toolsets/ovnkubernetes/adapter/adapter.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/nft.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/types/types.go
- evals/tasks/ovn-kubernetes/network-pwru-trace/network-pwru-trace.yaml
- evals/tasks/ovn-kubernetes/network-packet-drop-diagnosis/setup.sh
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/iptables.go
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/network-tools/mcp/mcp.go
- pkg/toolsets/ovnkubernetes/config/config.go
- docs/openshift/ovn-kubernetes.md
- pkg/mcp/toolsets_test.go
- pkg/mcp/testdata/toolsets-ovn-kubernetes-tools.json
- vendor/github.com/ovn-kubernetes/ovn-kubernetes-mcp/pkg/kernel/mcp/conntrack.go
- evals/tasks/ovn-kubernetes/network-pwru-trace/setup.sh
|
/lgtm |
openshift-ci
Bot
added
needs-rebase
Member
Author
openshift-ci
Bot
added
the
do-not-merge/hold
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 17, 2026 13:38
56f93b9 to
21b4a62
Compare
|
New changes are detected. LGTM label has been removed. |
openshift-ci
Bot
removed
the
needs-rebase
Cali0707
reviewed
Jul 21, 2026
Cali0707
left a comment
Cali0707
left a comment
There was a problem hiding this comment.
Overall LGTM, added a few nits but none of them are blocking merging this when we update the dep.
IMO these can either be tackled here or in a follow up later, and none block backporting this into the TP2 branch (but they should probably be done at some point).
| Enum: []interface{}{"ip", "ip6", "inet", "arp", "bridge", "netdev"}, | ||
| }, | ||
| } | ||
| utils.AddHeadTailProperties(props, "Return only first N lines") |
There was a problem hiding this comment.
nit: do we not want to have the same Default %d lines if tail is not specified message here as the other tools in the package have?
Member
Author
There was a problem hiding this comment.
Thanks for pointing that out. Missed it.
Comment on lines
+179
to
+180
| // HasOVNKubernetes returns true if the target has OVN-Kubernetes resources. | ||
| func HasOVNKubernetes(p api.FilteringProvider) bool { |
There was a problem hiding this comment.
nit: it looks like this wasn't wired up into the toolset.go GetTools method? IMO we either want to:
- use this to filter the tools
- Drop this and not have the unused func
Member
Author
There was a problem hiding this comment.
I had initially added that for the tools in kernel and netwrok-tools, then on a second thought removed it from the TargetCompatibilityFilters field of the tools since these tools can be used even when the cluster is not using ovnk CNI. I have removed the function.
However, for the tools which are dependent on the ovnk CNI, instead of checking whether certain GVKs are available in the cluster or not to verify if ovnk CNI is being used, I think it's best to check if the ovnk pod are actually running in the cluster or not. The GVKs being available in the cluster may not always mean that the CNI is also being used. We can probably check like this:
func HasOVNKubernetes(p api.FilteringProvider) bool {
k8sProvider, ok := p.(kubernetes.Provider)
if !ok {
return false
}
k8s, err := k8sProvider.GetDerivedKubernetes(context.TODO(), k8sProvider.GetDefaultTarget())
if err != nil {
return false
}
pods, err := k8s.CoreV1().Pods("").List(context.TODO(), metav1.ListOptions{
LabelSelector: "app=ovnkube-node",
})
if err != nil {
return false
}
return len(pods.Items) > 0
}There was a problem hiding this comment.
if some tools do not rely on the cluster using ovnk CNI, should they be moved out of the ovnk toolset? As otherwise people would only have them if enabling ovnk?
Member
Author
There was a problem hiding this comment.
The tools are used for diagnosing networking issues, including ovnk CNI issues. We normally use it for diagnosing ovnk problems, that's why I added it under the ovnk toolset. But, I guess, it can be added to a different toolset and not club them together with other ovnk tools.
There was a problem hiding this comment.
Yeah that's fair - I guess my thinking was just that the main part of toolsets from a user perspective is that they are groups of tools that can be enabled/disabled without needing to list individual tools
So if these are useful outside of ovnk, maybe they could make sense separately. You are the domain expert though, so I defer to you on whether you think these would make sense for users outside of ovnk
| timeoutCtx, cancel = context.WithTimeout(ctx, timeout) | ||
| } | ||
| if cancel != nil { | ||
| defer cancel() |
There was a problem hiding this comment.
nit: this is a little fragile, IMO it would be better if we defer the cancel at each spot we create it, just to be safe in these different paths if someone adds an intermediate return in the future
| }, | ||
| "command": { | ||
| Type: "string", | ||
| Description: `These options specify the desired action to perform. Only one of them can be specified on the command line unless otherwise stated below. |
There was a problem hiding this comment.
nit: maybe add the options as an enum here?
Member
Author
There was a problem hiding this comment.
The values also have a variable [chain] part. Not sure if we can add enum for that.
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 22, 2026 06:18
21b4a62 to
250bbc4
Compare
Member
Author
|
/retest |
… set and the context has a deadline Signed-off-by: arkadeepsen <arsen@redhat.com>
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
2 times, most recently
from
July 23, 2026 18:19
4a188f4 to
50969c8
Compare
arkadeepsen
changed the title
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/openshift/ovn-kubernetes.md (4)
653-655: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the OVN command name.
The troubleshooting example says
ovs-nbctl, but the OVN command isovn-nbctl.🤖 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 `@docs/openshift/ovn-kubernetes.md` around lines 653 - 655, Update the troubleshooting symptom text under the “no record” errors from ovn_get section to use the correct ovn-nbctl command name instead of ovs-nbctl.
512-512: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to the fenced workflow examples.
Use a language such as
textfor these three fences to satisfy Markdown linting and improve rendering.Also applies to: 530-530, 555-555
🤖 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 `@docs/openshift/ovn-kubernetes.md` at line 512, Update the three fenced workflow examples in the documentation to specify a language tag, using text for each fence, including the fences around the referenced sections. Leave the example contents unchanged.Source: Linters/SAST tools
40-67: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse namespaced RBAC for the documented access.
This
ClusterRoleBindinggrants pod listing andpods/execaccess across every namespace, while the toolset is documented foropenshift-ovn-kubernetes. Also,ClusterRoleBindingcannot be namespace-scoped as suggested on Line 81. Use aRole/RoleBindingin the target namespace, or bind a reusableClusterRolewith a namespacedRoleBinding, to avoid cluster-wide exec access.Also applies to: 78-82
🤖 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 `@docs/openshift/ovn-kubernetes.md` around lines 40 - 67, Replace the cluster-wide RBAC resources in the documented access configuration with namespaced authorization for openshift-ovn-kubernetes: use a Role and RoleBinding in that namespace, or bind the existing permissions through a namespaced RoleBinding. Ensure the ServiceAccount retains pod discovery and pods/exec access only within the target namespace, and remove any ClusterRoleBinding-based scope.
662-666: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLink to the OpenShift intro when meant.
../README.mdcorrectly resolves todocs/README.md, but the OpenShift documentation hub isdocs/openshift/README.md; useREADME.mdif that is the intended target.🤖 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 `@docs/openshift/ovn-kubernetes.md` around lines 662 - 666, Update the Core Toolset link in the Related Documentation section to target the OpenShift documentation hub in the current directory by using README.md instead of ../README.md; leave the other links unchanged.
🧹 Nitpick comments (1)
evals/tasks/cni-diagnostics/network-tcpdump-basic/setup.sh (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the image tag for reproducibility.
Uses
nginx-unprivileged:latestwhile the siblingnetwork-packet-drop-diagnosis/setup.shpins the same image to:1.29. Unpinned tags can cause flaky eval results if the upstream image changes.♻️ Proposed fix
- image: quay.io/nginx/nginx-unprivileged:latest + image: quay.io/nginx/nginx-unprivileged:1.29🤖 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 `@evals/tasks/cni-diagnostics/network-tcpdump-basic/setup.sh` at line 26, Replace the :latest tag in the setup image configuration with the pinned nginx-unprivileged:1.29 tag, matching the sibling network-packet-drop-diagnosis setup and preserving the existing image repository.
🤖 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 `@docs/configuration.md`:
- Around line 350-370: Re-run the make update-readme-tools target to regenerate
the Available Toolsets, Resources, and Resource Templates sections in both
README.md and docs/configuration.md. Do not edit the generated AVAILABLE-*
sections manually; retain the generated output to keep toolset listings
synchronized.
In `@evals/core-eval-testing/acp-anthropic/eval-cni-diagnostics.yaml`:
- Around line 20-25: The CNI eval assertions use a generic Kubernetes wildcard
instead of enforcing the intended diagnostic tool. In
evals/core-eval-testing/acp-anthropic/eval-cni-diagnostics.yaml:20-25,
evals/core-eval-testing/acp-google/eval-cni-diagnostics.yaml:20-25,
evals/core-eval-testing/builtin-anthropic/eval-cni-diagnostics.yaml:20-25,
evals/core-eval-testing/builtin-google/eval-cni-diagnostics.yaml:20-25, and
evals/core-eval-testing/builtin-openai/eval-cni-diagnostics.yaml:20-25, replace
the toolsUsed server and toolPattern with the concrete diagnostic tool/server
pattern used by these tasks while preserving the existing minToolCalls and
maxToolCalls limits.
In `@evals/tasks/cni-diagnostics/kernel-conntrack-filter/setup.sh`:
- Around line 4-13: The setup scripts must create deterministic
worker-to-API-server TCP traffic before evaluation and wait until the connection
is established. Update both
evals/tasks/cni-diagnostics/kernel-conntrack-filter/setup.sh (lines 4-13) and
evals/tasks/cni-diagnostics/kernel-conntrack-list/setup.sh (lines 4-15) to
launch or reuse a stable worker-side connection to the API server identified by
the API_SERVER value, retaining the worker-node validation and ensuring the flow
is ready before exiting successfully.
In `@evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/cleanup.sh`:
- Line 4: Scope node-debug pod cleanup to the specific task/run instead of
deleting every pod with the shared component label. Update cleanup.sh in
evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/cleanup.sh at
line 4, evals/tasks/cni-diagnostics/kernel-nft-list/cleanup.sh at line 4, and
evals/tasks/cni-diagnostics/network-packet-drop-diagnosis/cleanup.sh at line 4
to use the existing component and node-specific name labels assigned by
createDebugPod, or introduce and consistently use a task/run-scoped label.
In
`@evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml`:
- Around line 11-12: Replace the generated meta-sentence assertions in
verify.contains with stable evidence terms. In
evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yaml
lines 11-12, assert relevant output such as POSTROUTING or MASQUERADE; in
evals/tasks/cni-diagnostics/kernel-nft-list/kernel-nft-list.yaml lines 11-12,
assert stable nftables evidence such as inet. Ensure the prompts and verifiers
require the same observable terms.
In `@evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/setup.sh`:
- Around line 27-29: Update the busybox fixture in the setup configuration to
use a tested immutable version tag or image digest instead of
quay.io/quay/busybox:latest, while preserving the existing sleep command and
container behavior.
- Around line 14-40: Update setup.sh after the external-test pod becomes Ready
to deterministically apply a reversible node-level egress fault involving
routing, NAT, or nftables, and verify the fault is active before the diagnosis
begins. Preserve the existing pod setup and wait behavior, and record enough
state or provide cleanup data so the injected fault can be reverted safely.
- Around line 11-12: Generate a unique run/task identifier and apply it
consistently to the Kubernetes fixture namespace/resource names and cleanup
targets. Update setup.sh lines 11-12 to create and use the scoped
kernel-network-namespace fixture name; update cleanup.sh lines 5-5 in
evals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis and
evals/tasks/cni-diagnostics/network-packet-drop-diagnosis to delete the same
run-scoped names, preserving matching references across setup, fixtures, and
cleanup.
---
Outside diff comments:
In `@docs/openshift/ovn-kubernetes.md`:
- Around line 653-655: Update the troubleshooting symptom text under the “no
record” errors from ovn_get section to use the correct ovn-nbctl command name
instead of ovs-nbctl.
- Line 512: Update the three fenced workflow examples in the documentation to
specify a language tag, using text for each fence, including the fences around
the referenced sections. Leave the example contents unchanged.
- Around line 40-67: Replace the cluster-wide RBAC resources in the documented
access configuration with namespaced authorization for openshift-ovn-kubernetes:
use a Role and RoleBinding in that namespace, or bind the existing permissions
through a namespaced RoleBinding. Ensure the ServiceAccount retains pod
discovery and pods/exec access only within the target namespace, and remove any
ClusterRoleBinding-based scope.
- Around line 662-666: Update the Core Toolset link in the Related Documentation
section to target the OpenShift documentation hub in the current directory by
using README.md instead of ../README.md; leave the other links unchanged.
---
Nitpick comments:
In `@evals/tasks/cni-diagnostics/network-tcpdump-basic/setup.sh`:
- Line 26: Replace the :latest tag in the setup image configuration with the
pinned nginx-unprivileged:1.29 tag, matching the sibling
network-packet-drop-diagnosis setup and preserving the existing image
repository.
🪄 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: 615c22bf-2534-4df6-90fa-50d43acfef7d
📒 Files selected for processing (37)
README.mddocs/configuration.mddocs/openshift/README.mddocs/openshift/cni-diagnostics.mddocs/openshift/ovn-kubernetes.mdevals/core-eval-testing/acp-anthropic/eval-cni-diagnostics.yamlevals/core-eval-testing/acp-google/eval-cni-diagnostics.yamlevals/core-eval-testing/builtin-anthropic/eval-cni-diagnostics.yamlevals/core-eval-testing/builtin-google/eval-cni-diagnostics.yamlevals/core-eval-testing/builtin-openai/eval-cni-diagnostics.yamlevals/tasks/cni-diagnostics/kernel-conntrack-filter/cleanup.shevals/tasks/cni-diagnostics/kernel-conntrack-filter/kernel-conntrack-filter.yamlevals/tasks/cni-diagnostics/kernel-conntrack-filter/setup.shevals/tasks/cni-diagnostics/kernel-conntrack-list/cleanup.shevals/tasks/cni-diagnostics/kernel-conntrack-list/kernel-conntrack-list.yamlevals/tasks/cni-diagnostics/kernel-conntrack-list/setup.shevals/tasks/cni-diagnostics/kernel-ip-routes/cleanup.shevals/tasks/cni-diagnostics/kernel-ip-routes/kernel-ip-routes.yamlevals/tasks/cni-diagnostics/kernel-ip-routes/setup.shevals/tasks/cni-diagnostics/kernel-iptables-list/cleanup.shevals/tasks/cni-diagnostics/kernel-iptables-list/kernel-iptables-list.yamlevals/tasks/cni-diagnostics/kernel-iptables-list/setup.shevals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/cleanup.shevals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/kernel-network-namespace-diagnosis.yamlevals/tasks/cni-diagnostics/kernel-network-namespace-diagnosis/setup.shevals/tasks/cni-diagnostics/kernel-nft-list/cleanup.shevals/tasks/cni-diagnostics/kernel-nft-list/kernel-nft-list.yamlevals/tasks/cni-diagnostics/kernel-nft-list/setup.shevals/tasks/cni-diagnostics/network-packet-drop-diagnosis/cleanup.shevals/tasks/cni-diagnostics/network-packet-drop-diagnosis/network-packet-drop-diagnosis.yamlevals/tasks/cni-diagnostics/network-packet-drop-diagnosis/setup.shevals/tasks/cni-diagnostics/network-pwru-trace/cleanup.shevals/tasks/cni-diagnostics/network-pwru-trace/network-pwru-trace.yamlevals/tasks/cni-diagnostics/network-pwru-trace/setup.shevals/tasks/cni-diagnostics/network-tcpdump-basic/cleanup.shevals/tasks/cni-diagnostics/network-tcpdump-basic/network-tcpdump-basic.yamlevals/tasks/cni-diagnostics/network-tcpdump-basic/setup.sh
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 23, 2026 19:22
50969c8 to
9b37232
Compare
Cali0707
reviewed
Jul 24, 2026
| }, | ||
| Required: []string{"target_type", "name"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Network Packet Capture", false), |
There was a problem hiding this comment.
do we want a OVN-K prefix on this CNI tool?
Member
Author
There was a problem hiding this comment.
Copy-paste issue. Posting an update shortly.
Member
Author
There was a problem hiding this comment.
Updated all annotations.
Member
Author
There was a problem hiding this comment.
Updated the tests as well.
| }, | ||
| Required: []string{"node_name"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Network eBPF Packet Tracing", false), |
| Properties: props, | ||
| Required: []string{"node"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Kernel Connection Tracking", true), |
| Properties: props, | ||
| Required: []string{"node"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Kernel IPtables", true), |
| Properties: props, | ||
| Required: []string{"node", "command"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Kernel NFtables", true), |
| Properties: props, | ||
| Required: []string{"node", "command"}, | ||
| }, | ||
| Annotations: utils.ReadOnlyAnnotations("OVN-K: Kernel IP Command", true), |
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 24, 2026 14:15
9b37232 to
ba749e1
Compare
…s toolset Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: arkadeepsen <arsen@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 24, 2026 14:35
ba749e1 to
bb26a67
Compare
… cni-diagnostics Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: arkadeepsen <arsen@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: arkadeepsen <arsen@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: arkadeepsen <arsen@redhat.com>
arkadeepsen
force-pushed
the
ovnk-kernel-network-tools
branch
from
July 24, 2026 14:50
bb26a67 to
4d802f4
Compare
|
@arkadeepsen: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the MCP tools for kernel and network-tools layer to the cni-diagnostics toolset.
The tools are added using the toolset-design skill and the EP to add the upstream ovn-kubernetes MCP tools.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation