[WIP] OTE: Rebase K8s 1.36.2#3248
Open
pperiyasamy wants to merge 164 commits into
Open
Conversation
We only support single-node-zone so drop zone-name labeling. Signed-off-by: Mykola Yurchenko <myurchenko@nvidia.com>
With multi-node-per-zone removed the selection is simplified Signed-off-by: Mykola Yurchenko <myurchenko@nvidia.com>
zone-name label is no longer read by anything. kube node name is used directly Signed-off-by: Mykola Yurchenko <myurchenko@nvidia.com>
drop the helper as we only run single node per zone Signed-off-by: Mykola Yurchenko <myurchenko@nvidia.com>
Clean up leftover raft-related code that was missed by PR #6303 (Remove central mode support). The four shell variables (ovn_nb_raft_port, ovn_sb_raft_port, ovn_nb_raft_election_timer, ovn_sb_raft_election_timer) and their documentation comments are defined but never referenced anywhere in the codebase. Signed-off-by: Matteo Dallaglio <mdallagl@redhat.com>
The report was skipping : - indexing results (due to the rm of the perf-data) - capturing the baseline run and reporting the diff to the PR due to how I was capturing the PR #. Both issues are addressed. Signed-off-by: jtalerico <joe.talerico@gmail.com>
Signed-off-by: Lei Huang <leih@nvidia.com>
- Modify UDN/CUDN CRD schemas to allow adding multiple cluster-subents of the same IP family in Layer3 topology - Reconcile network controllers to add new subnets to NodeAllocator. Fixes #5377 Signed-off-by: Lei Huang <leih@nvidia.com>
This change adds test cases for: - Pods can perform east/west traffic between nodes on different CIDR - add subnet not affecting existing node subnet assignment - add bad subnet should not cause change on existing NAD Signed-off-by: Lei Huang <leih@nvidia.com>
Update ClusterNetworkConnect routing policy generation to create a single logical router policy per destination network and IP family. When a connected network has multiple same-family pod subnets, fold them into one OR match instead of creating multiple policies with the same DB identity. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Add unit coverage for the case where a primary UDN gains a new subnet that overlaps an existing EgressFirewall CIDRSelector. The test verifies that the initial ACL does not exclude the new subnet, then updates the primary network subnets and confirms the reconciled ACL adds the pod-subnet exclusion so east/west traffic is not affected by the EgressFirewall rule. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Update the OKEP to note that no RouteAdvertisement code change is required for Layer3 topology. FRR pod-subnet advertisements use node subnet annotations, not the NAD cluster subnet list directly, and node subnet annotation updates already trigger RA reconciliation. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Add unit coverage for overlay mode route import when a primary UDN has multiple cluster subnets. Verify that a BGP route contained in an additional UDN subnet is ignored and not imported into OVN. This confirms the existing route import logic accounts for all network subnets when filtering pod-network routes in overlay mode. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Queue advertised local nodes for reconciliation when a network subnet list changes. This ensures BGP Network Isolation ACLs are rebuilt when a primary Layer3 UDN gains an additional subnet. Add unit coverage for both the reconcile trigger and the advertised network isolation pass ACL update with the added subnet. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Trigger local node reconciliation when Egress IP is enabled and a primary Layer3 UDN adds or removes cluster subnets. Add unit coverage for the subnet-change reconcile path and verify Egress IP route/no-reroute policy generation covers every primary Layer3 UDN subnet. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
Update primary Layer3 UDN/CUDN e2e coverage to use multi-subnet configuration where the test is not specifically validating single-subnet behavior. This broadens runtime coverage for existing network segmentation, service, egress firewall, egress IP, endpoint slice mirroring, and route advertisement flows. Signed-off-by: Lei Huang <huang.f.lei@gmail.com>
OVN-Kubernetes renames the simulated management representor to ovn-k8s-mp0 and preserves the original rep0-X name as the link alias. Fall back to alias lookup so DPU-mode restarts and Helm rollouts can find the representor after it has been renamed. Signed-off-by: Tim Rozet <trozet@nvidia.com>
Remove dead raft variable definitions from ovnkube.sh
Fix performance report
Set OVN_ROUTE_ADVERTISEMENTS_ENABLE in the single-node-zone DPU chart so DPU-mode ovnkube-controller receives the Helm route-advertisements feature setting. Signed-off-by: Tim Rozet <trozet@nvidia.com>
DPU-host mode updates advertised UDN isolation nftables for primary UDNs, but the base sets were only configured from the full-mode NodePort watcher path. That left DPU-host nodes without the advertised UDN subnet sets when NodePort was disabled. Move the setup into default node controller initialization for full and DPU-host mode, and keep the existing route advertisements feature gate. Add a DPU-host test that covers NodePort-disabled startup. Signed-off-by: Tim Rozet <trozet@nvidia.com>
Resolve simulated DPU representors by alias
When a VM's spec.template.spec.hostname is set, KubeVirt copies it into the vm.kubevirt.io/name label on virt-launcher pods instead of the actual VM name. OVN-K used this label to find pods belonging to a VM (e.g. during live migration, or to tag OVN topology elements for cleanup). Since multiple VMs can share the same hostname, the label can no longer uniquely identify a VM's pods -- with 3+ VMs sharing a hostname, the 3rd VM fails with "unexpected live migration state at pods". This change switches to the kubevirt.io/domain annotation, which always reflects the real VM name. Since annotations can't be used in label selectors, we optimize: if the domain name matches the vm.kubevirt.io/name label (the common case), we use the label for lookup; otherwise we fall back to iterating virt-launcher pods in the namespace and matching by annotation. Note: KubeVirt >= 1.7 introduces a vm.kubevirt.io/id label that reliably identifies the VM, but it is not added retroactively to pods created with older versions. If KubeVirt adds a mutating webhook for that in the future, we can use it as a further optimization. Assisted-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
Signed-off-by: Enrique Llorente <ellorent@redhat.com>
Replace the fake virt-launcher pod with a proper VMI using fedoraWithTestToolingVMI and l2bridge binding plugin. This is needed because now ovnk requires the same labels as IPAM controller, so now IPAM controller is activated by this tests and fails since there is no vm. This also allow us to remove dependency on nmstate build image quay.io/nmstate/c10s-nmstate-dev:latest Signed-off-by: Enrique Llorente <ellorent@redhat.com>
kubevirt: Support handling vm hostname field
Pass route advertisement flag to DPU nodes
Configure advertised UDN nftables on node init
P-UDN: support multiple cluster-subnets for Layer3 topology
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Adding missing labels for UDN workloads
Drop manual zone-name node labeling
Sync reconcile pod-selector address sets at creation time
Add a "transport" label to the ovnkube_clustermanager_cluster_user_defined_networks gauge to distinguish CUDNs by transport type (Geneve, EVPN, NoOverlay). Empty transport (default OVN overlay) is mapped to "Geneve". Add seedCUDNCountsMetric to establish correct baselines at startup so the metric survives leader restarts where Inc/Dec from finalizer guards would not re-fire. Add dedicated CUDN metrics test context covering increment on create (Geneve and EVPN transports) and decrement on delete. Signed-off-by: Matteo Dallaglio <mdallagl@redhat.com>
- Use "Default" instead of "Geneve"/"Localnet" for the transport label when spec.Transport is empty. This avoids assuming a specific encapsulation protocol (OVN may use Geneve or VXLAN) and removes the conflation of Localnet topology with transport. - Invert cudnMetricTracker from map[name]key to map[key]sets.Set[name], giving O(1) count via set length and removing the O(n) countCUDNMetricKey scan. - Extract trackCUDN helper to deduplicate nil-check + insert logic between seedCUDNCountMetrics and cudnMetricCounted. Signed-off-by: Matteo Dallaglio <mdallagl@redhat.com>
Added optional CLI flags `--tls-min-version` and `--tls-cipher-suites` that will be used to configure TLS parameters for the webhook. Also added a `tls.ApplyConfigOptionsFor` function that will be used to parse the CLI flags and apply to a `tls.Config`. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Adds an `ApplyTLSOptions` field to `webhook.Config` that allows callers to customize TLS configuration (MinVersion, CipherSuites, etc.) for the webhook server. Includes unit tests to verify the server applies the custom TLS options during handshake. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
Add CLI flags for configuring TLS to ovnkube-identity
area-maintainers: introduce virtualization area with merge github action
During controller initialization, syncPodsForUserDefinedNetwork() calls GetActiveNetworkForNamespace() which can fail when a namespace is deleted (NotFound) or has the primary UDN label but no NAD yet (InvalidPrimaryNetworkError). Without error handling, these failures cause the factory retry loop to spin for 60 seconds until context deadline expires, blocking controller startup and preventing pods from getting their logical ports created. Add error handling to skip these pods during initial sync. Normal event-driven processing will handle them once their namespaces are ready. Signed-Off-By: Jamo Luhrsen <jluhrsen@gmail.com> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
DPU-host: skip LoadDeviceInfoFromDP for primary UDN
Update OVN-Kubernetes to build and test against Kubernetes 1.36.1. File changes: `.github/workflows/docker.yml`: use the Go 1.26 builder image for container image builds. `.github/workflows/performance-test.yml`: set CI K8S_VERSION to v1.36.1. `.github/workflows/test.yml`: set CI K8S_VERSION to v1.36.1 and update golangci-lint to v2.12.2. `contrib/kind-common.sh`: set the default kind Kubernetes version to v1.36.1 and keep frr-k8s install readiness compatible with the ovnk-bgp frr-k8s manifests used by the BGP lanes. `contrib/kind.yaml.j2`: enable RelaxedServiceNameValidation for the Kubernetes 1.36 kind API server so digit-prefixed Service names are accepted in CI. `dist/images/Makefile`: use Go 1.26 for the image build container. `docs/developer-guide/local_testing_guide.md`: update the local Kubernetes test binary example to v1.36.1. `docs/features/requirements.md`: update the master branch supported Kubernetes version to 1.36. `docs/installation/INSTALL.KUBEADM.md`: update kubeadm install examples from v1.35 to v1.36. `docs/installation/launching-ovn-kubernetes-on-kind.md`: update kind and kubectl examples from v1.35.0 to v1.36.1. `go-controller/.golangci.yml`: disable the new govet inline analyzer for this bump. `go-controller/Makefile`: use Go 1.26 for the go-controller build container. `go-controller/go.mod` and `go-controller/go.sum`: move the module to go 1.26/toolchain go1.26.3 and update Kubernetes, controller-runtime, OpenShift, Ginkgo/Gomega, knftables, and transitive dependencies for the Kubernetes 1.36.1 module set. `go-controller/hack/lint.sh`: update the local golangci-lint version to v2.12.2 to match CI. `go-controller/pkg/clustermanager/nooverlay/controller.go`, `go-controller/pkg/clustermanager/pod/allocator.go`, `go-controller/pkg/kube/healthcheck/healthcheck.go`, `go-controller/pkg/node/healthcheck_node.go`, `go-controller/pkg/node/udn_isolation.go`, `go-controller/pkg/ovn/base_network_controller.go`, and `go-controller/pkg/ovn/ovn.go`: pass explicit format strings to Eventf calls required by the updated lint/toolchain checks. `go-controller/pkg/clustermanager/status_manager/status_manager.go`: use FieldsV1.GetRawBytes with the updated Kubernetes API machinery type. `go-controller/pkg/csrapprover/approver_test.go`: remove stale unreachable error checks exposed by the updated lint/toolchain checks. `go-controller/pkg/libovsdb/ops/model.go` and `go-controller/pkg/node/controllers/egressip/egressip.go`: adjust fmt verbs for typed non-string values under the updated vet/lint checks. `go-controller/vendor/` and `go-controller/vendor/modules.txt`: regenerate vendored dependencies for the updated go-controller module. `LICENSES/`: regenerate third-party license metadata for the updated Go dependency graph. `test/conformance/go.mod` and `test/conformance/go.sum`: move the conformance module to go 1.26/toolchain go1.26.3 and update Kubernetes/client dependencies to the 1.36.1 set. `test/e2e/go.mod` and `test/e2e/go.sum`: move the e2e module to go 1.26/toolchain go1.26.3 and update Kubernetes/client/test dependencies to the 1.36.1 set. `test/scripts/install-kind.sh`: install Kubernetes v1.36.1 kubectl, e2e.test, and ginkgo, and build the Kubernetes v1.36.1 kind node image. `test/scripts/upgrade-ovn.sh`: refresh the e2e test binary to Kubernetes v1.36.1 during upgrade jobs. Commands used for the module bump: go-controller: `cd go-controller` `go get k8s.io/api@v0.36.1` `go get k8s.io/apimachinery@v0.36.1` `go get k8s.io/apiserver@v0.36.1` `go get k8s.io/client-go@v0.36.1` `go get k8s.io/component-base@v0.36.1` `go get k8s.io/component-helpers@v0.36.1` `go get k8s.io/kubelet@v0.36.1` `go get k8s.io/kubernetes@v1.36.1` `go get sigs.k8s.io/controller-runtime@v0.24.0` `go mod tidy` `go mod vendor` `make verify-go-mod-vendor` `make third-party-licenses` `make verify-third-party-licenses` test/e2e: `cd test/e2e` `go get k8s.io/api@v0.36.1` `go get k8s.io/apimachinery@v0.36.1` `go get k8s.io/client-go@v0.36.1` `go get k8s.io/kubernetes@v1.36.1` `go get k8s.io/pod-security-admission@v0.36.1` `go get k8s.io/kubectl@v0.36.1` `go get sigs.k8s.io/controller-runtime@v0.24.0` `go mod tidy` test/conformance: `cd test/conformance` `go get k8s.io/apimachinery@v0.36.1` `go get k8s.io/client-go@v0.36.1` `go get sigs.k8s.io/controller-runtime@v0.24.0` `go mod tidy` Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
This PR bumps OVN-Kubernetes CI to Kubernetes 1.36.1, but the kind bootstrap script still downloaded KIND v0.27.0. That older KIND version is not the right bootstrap version for building and running the Kubernetes v1.36.1 kind node image used by CI. The Kubernetes dependency bump updated kubectl, the e2e test binary, and the kind node image target to v1.36.1, but the KIND binary used to build and run that local cluster was left at v0.27.0. One visible compatibility problem is Kubernetes 1.36 ImageVolume coverage, which needs node runtime support from containerd 2.1.1 or newer. With the older KIND path, those tests can fail during node/runtime setup before they test OVN-Kubernetes behavior. The kind bootstrap script needs a KIND release that supports the Kubernetes v1.36.1 kind node image and provides the newer node runtime required by the Kubernetes 1.36.1 e2e and conformance surface. test/scripts/install-kind.sh now downloads KIND v0.31.0 and uses it to build the Kubernetes v1.36.1 kind node image before running the existing kind and Helm setup. CI builds and runs the Kubernetes 1.36.1 kind cluster with KIND tooling that supports that Kubernetes target, and the generated node image has a runtime new enough for Kubernetes 1.36.1 tests such as ImageVolume. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
Kubernetes 1.36 rejects CRD integer schemas when a field has a maximum above the int32 range but the generated OpenAPI schema still says format:int32. NetworkQoS bandwidth rate and burst are Go uint32 fields with a maximum of 4294967295. Without an explicit kubebuilder format marker, controller-gen emits format:int32, which is inconsistent with that maximum under the stricter Kubernetes 1.36 CRD validation. The NetworkQoS API type must remain uint32 while making the generated CRD schema use an integer format wide enough for the existing maximum. The rate and burst fields now include +kubebuilder:validation:Format=int64, and the committed Helm CRD is regenerated with format:int64 for both fields. Kubernetes 1.36 accepts the NetworkQoS CRD, and existing NetworkQoS values continue to use the same Go API type and value range. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
The KubeVirt stable release used by the kind helper does not include the CRD schema updates needed by Kubernetes 1.36 validation. Kubernetes 1.36 tightened CRD validation before a stable KubeVirt release containing the required schema fixes was available. The kv-live-migration lanes need to remain installable on Kubernetes 1.36.1 while preserving explicit KUBEVIRT_VERSION overrides for stable releases, specific versions, and specific nightly tags. The default kind KubeVirt install now uses the latest nightly release from kubevirt-prow, with a TODO to move back to stable once a Kubernetes 1.36-compatible stable release exists. The default CI KubeVirt install gets CRDs that Kubernetes 1.36 accepts, while callers can still override KUBEVIRT_VERSION when they need a specific release. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
After the Kubernetes 1.36 dependency update, go mod tidy no longer keeps gopkg.in/fsnotify/fsnotify.v1 in go-controller/go.mod. Remove the unused legacy module requirement so go-controller/go.mod matches the tidied dependency list. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
Kind setup failed with Kubernetes 1.36 because MetalLB v0.15.3 generated BGPPeer CRD schemas that Kubernetes 1.36 rejects. The affected BGPPeer ASN fields are spec.myASN, spec.peerASN, and spec.localASN. These fields need to support 4-byte ASN values up to 4294967295. MetalLB v0.15.3 generated the OpenAPI schema for those fields with format: int32. That is invalid because int32 cannot represent 4294967295. Kubernetes 1.36 validates this more strictly and rejects the CRD. There is also an FRR image replacement step in kind setup for BGP CI. MetalLB v0.16.1 manifests use quay.io/frrouting/frr:10.5.3. The BGP CI lanes use FRR_DEPLOYED_IMAGE, currently quay.io/frrouting/frr:10.6.0. The script must replace the MetalLB manifest image quay.io/frrouting/frr:10.5.3 with quay.io/frrouting/frr:10.6.0. Kubernetes 1.36 rejects CRD schemas where an int32 field allows values outside the int32 range. MetalLB v0.15.3 generated such a schema for BGPPeer ASN fields. MetalLB v0.16.1 contains the upstream schema fixes for those fields. The FRR replacement failed because the script must match the image value that appears in MetalLB manifests. MetalLB and frr-k8s are separate dependencies and can use different FRR image tags. For MetalLB v0.16.1, the image to match is quay.io/frrouting/frr:10.5.3. The kind install path needs a MetalLB release whose BGPPeer CRDs are accepted by Kubernetes 1.36. It also needs the FRR image replacement to be MetalLB-specific by matching quay.io/frrouting/frr:10.5.3 in MetalLB v0.16.1 manifests and replacing it with FRR_DEPLOYED_IMAGE. The kind install path now checks out MetalLB v0.16.1. It sets the MetalLB upstream FRR image to quay.io/frrouting/frr:10.5.3 and replaces that exact value in config/frr/speaker-patch.yaml, config/manifests/metallb-frr.yaml, and charts/metallb/values.yaml with FRR_DEPLOYED_IMAGE, currently quay.io/frrouting/frr:10.6.0. Kubernetes 1.36 accepts the MetalLB BGPPeer CRDs for spec.myASN, spec.peerASN, and spec.localASN. The BGP CI lanes deploy MetalLB with quay.io/frrouting/frr:10.6.0. If a future MetalLB release changes the FRR image in its manifests, kind setup fails at the replacement step instead of silently leaving MetalLB on an unexpected FRR image. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
Kubernetes 1.36 kind clusters are bootstrapped by kubeadm v1beta4, where component extraArgs are represented as name/value lists instead of the older map form. The existing kind template still emitted the old map-shaped extraArgs. With Kubernetes 1.36 this can cause kubeadm to ignore or misread control-plane and kubelet settings during custom kind node image startup. The kind template needs to be compatible with kubeadm v1beta4 while preserving the existing OVN-Kubernetes CI settings, including log verbosity, service-lb-controller disablement, kubelet verbosity, control-plane node labels, and RelaxedServiceNameValidation for the digit-prefixed Service conformance case. The template now declares kubeadm.k8s.io/v1beta4 in each kubeadm patch and converts extraArgs and kubeletExtraArgs to the v1beta4 name/value list format. Kubernetes 1.36 kind clusters receive the intended kubeadm settings using the schema kubeadm expects, so control-plane and kubelet arguments continue to be applied when CI builds custom kind node images. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com>
The KubeVirt "should keep ip" table under the "with user defined networks and persistent ips configured" test verifies that a VM keeps its persistent UDN allocation after restart or migration. The secondary localnet dual-stack entry timed out because the test waited for VMI status to report both IPv4 and IPv6. In the failure, VMI status reported only IPv4, while the virt-launcher pod network-status and OVN logs showed that both IPv4 and IPv6 were still allocated. For secondary KubeVirt interfaces, VMI status reflects the addresses visible or configured inside the guest. The secondary guest path is IPv4-only here because DHCPv6 is not supported for these secondary interfaces. Therefore, missing IPv6 in VMI status does not mean OVN lost the IPv6 allocation. The test still needs to prove that persistent IP allocation survives restart or migration, but it must check the full allocation from Multus and OVN state instead of relying only on VMI status for secondary interfaces. Primary UDNs continue using VMI status. Secondary UDNs now read the allocated IPs from the running virt-launcher pod Multus network-status and compare that allocation before and after restart or migration. The test still fails if OVN or Multus actually loses an allocated IP, including IPv6. It no longer fails only because KubeVirt VMI status reports the secondary interface as IPv4-only. Signed-off-by: Miheer Salunke <miheer.salunke@gmail.com> (cherry picked from commit a019fbdc0d2a2ebe652bd6250c5d51749da13ec3)
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughThis PR adds area-maintainer merge automation and governance, updates CI and kind tooling for Kubernetes 1.36 and DPU no-overlay paths, refactors controller/runtime auth and webhook plumbing, changes UDN and route-advertisement behavior, updates KubeVirt migration handling, and refreshes bundled license files. ChangesArea maintainer governance and merge automation
Platform, CI, and deployment environment refresh
Controller runtime, auth, and process plumbing
UDN, no-overlay, route advertisements, and network metrics
KubeVirt identity and EVPN migration handling
Third-party license corpus refresh
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pperiyasamy 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
go-controller/pkg/clustermanager/routeadvertisements/controller.go (1)
849-896:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope DPU next-hop override to default no-overlay advertisements only.
Line 890 currently applies
ToAdvertise.NextHopfor any matched advertisement on DPU-host/shared-gateway nodes. That broadens a default-network no-overlay workaround into other transports/VRFs and can rewrite next-hop semantics unexpectedly.Suggested fix
dpuHostGatewayNextHops, err := getDPUHostGatewayNextHops(node) if err != nil { return nil, err } + shouldOverrideNextHop := matchedNetwork == types.DefaultNetworkName && + selectedNetworks.networkTransport[matchedNetwork] == types.NetworkTransportNoOverlay && + (ra.Spec.TargetVRF == "auto" || len(selectedNetworks.networks) == 1) @@ - if nextHop := dpuHostGatewayNextHops[isIPV6]; nextHop != "" { - if isIPV6 { - neighbor.ToAdvertise.NextHop.IPv6 = nextHop - } else { - neighbor.ToAdvertise.NextHop.IPv4 = nextHop - } + if shouldOverrideNextHop { + if nextHop := dpuHostGatewayNextHops[isIPV6]; nextHop != "" { + if isIPV6 { + neighbor.ToAdvertise.NextHop.IPv6 = nextHop + } else { + neighbor.ToAdvertise.NextHop.IPv4 = nextHop + } + } }🤖 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 `@go-controller/pkg/clustermanager/routeadvertisements/controller.go` around lines 849 - 896, The DPU next-hop override being applied at the nextHop assignment block (starting around line 889 with the check for dpuHostGatewayNextHops) is currently scoped too broadly, affecting all matched advertisements across different transports and VRFs. Scope this override to only apply to default no-overlay advertisements by adding a condition that verifies the advertisement is a default network no-overlay type before assigning the NextHop values to neighbor.ToAdvertise.NextHop. This ensures the DPU-host/shared-gateway next-hop workaround is limited to the specific scenario it was designed for and doesn't unexpectedly rewrite next-hop semantics for other transports or VRFs.go-controller/pkg/config/config.go (1)
169-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
OvnNorthwith the northbound discriminator.
GetURL()now depends on privatenorthbound, but the package-levelOvnNorth/savedOvnNorthdefault never sets it. BeforebuildOvnAuth()runs — and afterPrepareTestConfig()—config.OvnNorth.GetURL()resolves tounix:/var/run/ovn/ovnsb_db.sock. Set the default bit and tighten tests to assert NB/SB URLs separately.🐛 Proposed fix
// OvnNorth holds northbound OVN database client and server authentication and location details OvnNorth = OvnAuthConfig{ + northbound: true, RunDir: "/var/run/ovn/", DbLocation: "/etc/ovn/ovnnb_db.db", }Also applies to: 609-609, 2869-2876
🤖 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 `@go-controller/pkg/config/config.go` around lines 169 - 173, The OvnNorth configuration struct initialization is missing the northbound discriminator field that GetURL() depends on, causing it to return the incorrect southbound URL before buildOvnAuth() runs. Add the northbound field to the OvnNorth initialization (the OvnAuthConfig struct with RunDir and DbLocation) and set it to true to properly identify this as a northbound configuration. Apply the same fix to the savedOvnNorth default initialization and any other OvnAuthConfig initializations mentioned in the review that represent northbound configurations.docs/installation/launching-ovn-kubernetes-on-kind.md (1)
432-440:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the sample
kubectl versionoutput with the updatedK8S_VERSION.Line [432] sets
K8S_VERSION=v1.36.1, but Line [439] still showsClient Version: v1.32.3. Please update the sample output to avoid confusion during local setup verification.🤖 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/installation/launching-ovn-kubernetes-on-kind.md` around lines 432 - 440, The sample output for the kubectl version command does not match the K8S_VERSION variable that was set earlier in the documentation. Update the Client Version output in the sample kubectl version --client result from v1.32.3 to v1.36.1 to align with the K8S_VERSION=v1.36.1 that was set at the beginning of the command sequence, ensuring readers see consistent and accurate version information during their setup verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/area-merge.js:
- Around line 134-141: The current implementation at lines 134-141 in
.github/scripts/area-merge.js uses a simple includes check for the
`/area-maintainer-approved` command, which can incorrectly match when another
user quotes or mentions a previous approval. Replace this loose string matching
with strict command parsing that only recognizes `/area-maintainer-approved` as
a standalone command. Additionally, persist the requester identity by storing it
in the bot's SHA marker comment when the approval is first processed. Then, at
the retry merge sites (lines 149-160 and 287-303), instead of re-parsing
comments to find the requester, read the requester identity from the persisted
SHA marker comment. This ensures that the original approver's identity is
maintained throughout retries and prevents false "unauthorized" blocks caused by
later mentions or quotes of the approval command.
In `@contrib/kind-common.sh`:
- Around line 1249-1251: The `pushd` and `popd` commands in the `clone_frr`
function are not guarded against failure. At line 1249, the `pushd frr-k8s`
command lacks error handling, at line 1251, the first `popd` command lacks error
handling, and at line 1257, another `popd` command lacks error handling. If any
of these directory change operations fail, subsequent commands will execute in
the wrong working directory, causing misleading failures. Add `|| return 1`
after each of these `pushd` and `popd` commands to ensure that the function
returns early with error status if any directory operation fails.
In `@docs/design/service-traffic-policy.md`:
- Around line 499-500: The markdown file contains multiple fenced code blocks
that are missing language identifiers, which triggers MD040 lint errors. In
docs/design/service-traffic-policy.md, add the `text` language identifier to all
fenced code blocks containing OpenFlow flow table rules. Specifically, modify
the opening triple backticks from ``` to ```text at lines 499-500 (anchor
location), and also at the sibling locations: lines 518-519, 527-528, 539-540,
and 548-549. Each code block should have its opening fence changed to include
the language tag to satisfy the markdown linting requirements.
In `@docs/developer-guide/areas.md`:
- Line 5: Fix the broken and stale links in docs/developer-guide/areas.md across
three locations. At line 5, replace the CODEOWNERS link that uses blob/master
with a repository-relative link format. At lines 9-10, replace the relative path
references (../governance/...) with repository-relative links that start from
the repository root to ensure they remain valid regardless of branch or
directory structure changes. At line 49, similarly replace any blob/master or
relative path references with repository-relative link syntax. Use consistent
repository-relative link formatting across all three locations so the
documentation remains stable across different branches and layout changes.
- Around line 16-18: The fenced code block opening with triple backticks is
missing a language identifier, which triggers the MD040 markdown linting rule.
Add the language identifier "text" immediately after the opening triple
backticks (change ``` to ```text) on line 16 of the
docs/developer-guide/areas.md file. This specifies the content type of the code
block and satisfies the linting requirement.
In `@go-controller/cmd/ovnkube-identity/webhook/webhook.go`:
- Around line 131-133: Replace the klog.Fatalf calls with proper error
propagation to avoid process termination from within the Run method. When the
certWatcher.Start() call fails and at any other similar failure point in the Run
method (such as around line 145-148), return the error to the caller instead of
calling klog.Fatalf. This allows the caller to handle the error appropriately
and enables graceful shutdown instead of abruptly terminating the process.
- Around line 95-98: The return value of cache.WaitForCacheSync is being ignored
on line 97. This call returns a boolean indicating whether the cache sync
succeeded. Check this return value and handle the failure case by logging an
error and exiting the process before serving any admission traffic. If the sync
fails or the context is canceled, the webhook must not proceed with an unsynced
nodeLister, so add a conditional check after the cache.WaitForCacheSync call
that verifies the operation succeeded and exits with an appropriate error if it
did not.
In `@go-controller/cmd/ovnkube/ovnkube.go`:
- Around line 247-250: The function `determineOvnkubeRunMode` currently
validates that cluster manager and ovnkube-controller cannot run together, but
does not validate the stated co-location requirement that ovnkube-controller
must run with node. Add validation logic similar to the existing cluster manager
check to reject the combination where `mode.ovnkubeController` is true but
`mode.node` is false, returning an error that clearly states ovnkube-controller
requires node to be enabled via the `--init-node` flag. This will enforce the
invariant documented in the comments at lines 209-210 and 335-336.
In `@go-controller/pkg/clustermanager/node/node_allocator.go`:
- Around line 136-143: The AddNetworkRange calls in the loop are not atomic,
leaving the clusterSubnetAllocator in a partially applied state if an error
occurs during a later iteration. Refactor the method to either stage all subnet
additions and validate them before committing, or implement rollback logic to
undo previously added ranges (via AddNetworkRange or a similar removal method)
when an error occurs during the loop. Ensure na.recordSubnetCount() is only
called after all subnets have been successfully added.
In `@go-controller/pkg/clustermanager/routeadvertisements/controller.go`:
- Around line 973-981: The default-VRF fallback path (when hasEVPN and
vrfASNs[""] is 0) populates vrfNeighbors[""] from the source router's neighbors
without validating the DualStackAddressFamily restriction, bypassing the
dual-stack enforcement for EVPN. Add a DualStackAddressFamily check in the loop
where neighbors are appended to vrfNeighbors[""] to reject any dual-stack
neighbors that should not be included in EVPN raw-config generation, consistent
with how this restriction is enforced elsewhere in the code.
In `@go-controller/pkg/kubevirt/pod.go`:
- Around line 75-80: The NewVMDescriptionFromPod() function can return (nil,
nil) without an error, but the code unconditionally dereferences vmDescription
by calling vmDescription.OwnedPods() without first checking if vmDescription is
nil. Add a nil check for vmDescription immediately after the error check on the
result of NewVMDescriptionFromPod() to guard against panics when the function
returns nil without error. Apply the same nil guard pattern at all other
locations where vmDescription is dereferenced after being returned from
NewVMDescriptionFromPod().
In `@go-controller/pkg/libovsdb/util/address_set.go`:
- Around line 84-87: The NAT scanning predicate function only calls
recordReferences on nat.Match, but NAT rows can now reference address sets
through AllowedExtIPs and ExemptedExtIPs fields as well. Update the predicate
function passed to FindNATsWithPredicate to add recordReferences calls for
nat.AllowedExtIPs and nat.ExemptedExtIPs in addition to the existing nat.Match
call. This ensures all address sets referenced by NAT records are properly
marked as live and not incorrectly deleted as stale.
In `@go-controller/pkg/node/controllers/evpn/evpn_pod_controller.go`:
- Around line 70-78: In the shouldDeleteNeighbors method, add a nil check for
migrationStatus.SourcePod before accessing its Spec.NodeName field to prevent a
panic when migration status is ready but SourcePod is nil. Additionally, fix the
deletion logic near line 115 to delete using the source pod key instead of the
current pod key when handling target-pod updates on the source node, ensuring
that source cached entries are properly cleaned up during migration.
---
Outside diff comments:
In `@docs/installation/launching-ovn-kubernetes-on-kind.md`:
- Around line 432-440: The sample output for the kubectl version command does
not match the K8S_VERSION variable that was set earlier in the documentation.
Update the Client Version output in the sample kubectl version --client result
from v1.32.3 to v1.36.1 to align with the K8S_VERSION=v1.36.1 that was set at
the beginning of the command sequence, ensuring readers see consistent and
accurate version information during their setup verification.
In `@go-controller/pkg/clustermanager/routeadvertisements/controller.go`:
- Around line 849-896: The DPU next-hop override being applied at the nextHop
assignment block (starting around line 889 with the check for
dpuHostGatewayNextHops) is currently scoped too broadly, affecting all matched
advertisements across different transports and VRFs. Scope this override to only
apply to default no-overlay advertisements by adding a condition that verifies
the advertisement is a default network no-overlay type before assigning the
NextHop values to neighbor.ToAdvertise.NextHop. This ensures the
DPU-host/shared-gateway next-hop workaround is limited to the specific scenario
it was designed for and doesn't unexpectedly rewrite next-hop semantics for
other transports or VRFs.
In `@go-controller/pkg/config/config.go`:
- Around line 169-173: The OvnNorth configuration struct initialization is
missing the northbound discriminator field that GetURL() depends on, causing it
to return the incorrect southbound URL before buildOvnAuth() runs. Add the
northbound field to the OvnNorth initialization (the OvnAuthConfig struct with
RunDir and DbLocation) and set it to true to properly identify this as a
northbound configuration. Apply the same fix to the savedOvnNorth default
initialization and any other OvnAuthConfig initializations mentioned in the
review that represent northbound configurations.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 729d30f2-5c7f-498d-8865-dc6eadf97087
⛔ Files ignored due to path filters (5)
dist/images/Makefileis excluded by!**/dist/**dist/images/ovn-config.shis excluded by!**/dist/**dist/images/ovn-run.shis excluded by!**/dist/**dist/images/ovnkube.shis excluded by!**/dist/**go-controller/go.sumis excluded by!**/*.sum
📒 Files selected for processing (295)
.github/scripts/area-merge.js.github/workflows/area-merge.yml.github/workflows/docker.yml.github/workflows/kind-dpu-offload.yml.github/workflows/performance-report.yml.github/workflows/performance-test.yml.github/workflows/test.ymlCODEOWNERSGOVERNANCE.mdLICENSES/packages/cyphar.com/go-pathrs/v0.2.2/COPYINGLICENSES/packages/github.com/alecthomas/units/v0.0.0-20240927000941-0f3dac36c52b/COPYINGLICENSES/packages/github.com/armon/circbuf/v0.0.0-20190214190532-5111143e8da2/LICENSELICENSES/packages/github.com/containerd/containerd/api/v1.10.0/LICENSELICENSES/packages/github.com/coredns/corefile-migration/v1.0.31/LICENSELICENSES/packages/github.com/coreos/go-oidc/v2.5.0+incompatible/LICENSELICENSES/packages/github.com/coreos/go-oidc/v2.5.0+incompatible/NOTICELICENSES/packages/github.com/coreos/go-systemd/v22/v22.7.0/LICENSELICENSES/packages/github.com/coreos/go-systemd/v22/v22.7.0/NOTICELICENSES/packages/github.com/cyphar/filepath-securejoin/v0.6.1/COPYING.mdLICENSES/packages/github.com/cyphar/filepath-securejoin/v0.6.1/LICENSE.BSDLICENSES/packages/github.com/cyphar/filepath-securejoin/v0.6.1/LICENSE.MPL-2.0LICENSES/packages/github.com/emicklei/go-restful/v3/v3.13.0/LICENSELICENSES/packages/github.com/go-openapi/swag/cmdutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/conv/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/fileutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/jsonname/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/jsonutils/fixtures_test/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/jsonutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/loading/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/mangling/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/netutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/stringutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/typeutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/swag/yamlutils/v0.25.4/LICENSELICENSES/packages/github.com/go-openapi/testify/enable/yaml/v2/v2.0.2/LICENSELICENSES/packages/github.com/go-openapi/testify/v2/v2.0.2/LICENSELICENSES/packages/github.com/go-openapi/testify/v2/v2.0.2/NOTICELICENSES/packages/github.com/go-openapi/testify/v2/v2.0.2/internal/difflib/LICENSELICENSES/packages/github.com/go-openapi/testify/v2/v2.0.2/internal/spew/LICENSELICENSES/packages/github.com/godbus/dbus/v5/v5.2.2/LICENSELICENSES/packages/github.com/golang-jwt/jwt/v5/v5.3.0/LICENSELICENSES/packages/github.com/google/cadvisor/v0.56.2/LICENSELICENSES/packages/github.com/google/pprof/v0.0.0-20260115054156-294ebfa9ad83/LICENSELICENSES/packages/github.com/google/pprof/v0.0.0-20260115054156-294ebfa9ad83/third_party/svgpan/LICENSELICENSES/packages/github.com/gregjones/httpcache/v0.0.0-20180305231024-9cad4c3443a7/LICENSE.txtLICENSES/packages/github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus/v1.1.0/LICENSELICENSES/packages/github.com/grpc-ecosystem/go-grpc-middleware/v2/v2.3.3/COPYRIGHTLICENSES/packages/github.com/grpc-ecosystem/go-grpc-middleware/v2/v2.3.3/LICENSELICENSES/packages/github.com/grpc-ecosystem/grpc-gateway/v2/v2.27.7/LICENSELICENSES/packages/github.com/grpc-ecosystem/grpc-gateway/v2/v2.27.7/internal/casing/LICENSE.mdLICENSES/packages/github.com/karrick/godirwalk/v1.17.0/LICENSELICENSES/packages/github.com/libopenstorage/openstorage/v1.0.0/LICENSELICENSES/packages/github.com/libopenstorage/openstorage/v1.0.0/pkg/jsonpb/LICENSELICENSES/packages/github.com/metallb/frr-k8s/v0.0.0-20260603082256-b43efcb206be/LICENSELICENSES/packages/github.com/mohae/deepcopy/v0.0.0-20170929034955-c48cc78d4826/LICENSELICENSES/packages/github.com/mrunalp/fileutils/v0.5.0/LICENSELICENSES/packages/github.com/onsi/ginkgo/v2/v2.27.4/LICENSELICENSES/packages/github.com/onsi/ginkgo/v2/v2.28.1/LICENSELICENSES/packages/github.com/onsi/gomega/v1.39.0/LICENSELICENSES/packages/github.com/onsi/gomega/v1.39.1/LICENSELICENSES/packages/github.com/opencontainers/cgroups/v0.0.6/LICENSELICENSES/packages/github.com/opencontainers/runtime-spec/v1.3.0/LICENSELICENSES/packages/github.com/opencontainers/selinux/v1.13.1/LICENSELICENSES/packages/github.com/openshift/api/v0.0.0-20260513233232-73d7ca93df6d/LICENSELICENSES/packages/github.com/openshift/client-go/v0.0.0-20260512113608-deb4dc54551a/LICENSELICENSES/packages/github.com/prometheus/common/v0.67.5/LICENSELICENSES/packages/github.com/prometheus/common/v0.67.5/NOTICELICENSES/packages/github.com/prometheus/procfs/v0.19.2/LICENSELICENSES/packages/github.com/prometheus/procfs/v0.19.2/NOTICELICENSES/packages/github.com/spf13/cobra/v1.10.2/LICENSE.txtLICENSES/packages/github.com/spf13/cobra/v1.9.1/LICENSE.txtLICENSES/packages/github.com/spf13/pflag/v1.0.10/LICENSELICENSES/packages/go.etcd.io/etcd/api/v3/v3.6.8/LICENSELICENSES/packages/go.etcd.io/etcd/client/pkg/v3/v3.6.8/LICENSELICENSES/packages/go.etcd.io/etcd/client/v3/v3.6.8/LICENSELICENSES/packages/go.etcd.io/etcd/pkg/v3/v3.6.8/LICENSELICENSES/packages/go.etcd.io/etcd/server/v3/v3.6.8/LICENSELICENSES/packages/go.opentelemetry.io/contrib/instrumentation/github.com/emicklei/go-restful/otelrestful/v0.65.0/LICENSELICENSES/packages/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/v0.65.0/LICENSELICENSES/packages/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/v0.65.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc/v1.40.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/exporters/otlp/otlptrace/v1.40.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/metric/v1.41.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/sdk/metric/v1.40.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/sdk/v1.34.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/sdk/v1.40.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/trace/v1.35.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/trace/v1.41.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/v1.35.0/LICENSELICENSES/packages/go.opentelemetry.io/otel/v1.41.0/LICENSELICENSES/packages/go.opentelemetry.io/proto/otlp/v1.5.0/LICENSELICENSES/packages/go.opentelemetry.io/proto/otlp/v1.9.0/LICENSELICENSES/packages/go.uber.org/zap/v1.27.1/LICENSELICENSES/packages/golang.org/x/crypto/v0.47.0/LICENSELICENSES/packages/golang.org/x/crypto/v0.48.0/LICENSELICENSES/packages/golang.org/x/exp/v0.0.0-20251219203646-944ab1f22d93/LICENSELICENSES/packages/golang.org/x/net/v0.49.0/LICENSELICENSES/packages/golang.org/x/net/v0.50.0/LICENSELICENSES/packages/golang.org/x/sys/v0.40.0/LICENSELICENSES/packages/golang.org/x/sys/v0.41.0/LICENSELICENSES/packages/golang.org/x/term/v0.38.0/LICENSELICENSES/packages/golang.org/x/term/v0.39.0/LICENSELICENSES/packages/golang.org/x/term/v0.40.0/LICENSELICENSES/packages/golang.org/x/text/v0.31.0/LICENSELICENSES/packages/golang.org/x/text/v0.32.0/LICENSELICENSES/packages/golang.org/x/text/v0.33.0/LICENSELICENSES/packages/golang.org/x/text/v0.34.0/LICENSELICENSES/packages/golang.org/x/time/v0.11.0/LICENSELICENSES/packages/golang.org/x/time/v0.14.0/LICENSELICENSES/packages/golang.org/x/time/v0.9.0/LICENSELICENSES/packages/golang.org/x/tools/go/expect/v0.1.1-deprecated/LICENSELICENSES/packages/golang.org/x/tools/v0.38.0/LICENSELICENSES/packages/golang.org/x/tools/v0.39.0/LICENSELICENSES/packages/golang.org/x/xerrors/v0.0.0-20191204190536-9bdfabe68543/LICENSELICENSES/packages/google.golang.org/genproto/googleapis/api/v0.0.0-20260128011058-8636f8732409/LICENSELICENSES/packages/google.golang.org/genproto/googleapis/rpc/v0.0.0-20260128011058-8636f8732409/LICENSELICENSES/packages/google.golang.org/genproto/googleapis/rpc/v0.0.0-20260504160031-60b97b32f348/LICENSELICENSES/packages/google.golang.org/grpc/v1.72.1/NOTICE.txtLICENSES/packages/google.golang.org/protobuf/v1.36.12-0.20260120151049-f2248ac996af/LICENSELICENSES/packages/google.golang.org/protobuf/v1.36.8/LICENSELICENSES/packages/k8s.io/api/v0.36.1/LICENSELICENSES/packages/k8s.io/apiextensions-apiserver/v0.36.0/LICENSELICENSES/packages/k8s.io/apiextensions-apiserver/v0.36.1/LICENSELICENSES/packages/k8s.io/apimachinery/v0.36.1/LICENSELICENSES/packages/k8s.io/apimachinery/v0.36.1/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/apiserver/v0.36.0/LICENSELICENSES/packages/k8s.io/apiserver/v0.36.1/LICENSELICENSES/packages/k8s.io/cli-runtime/v0.36.1/LICENSELICENSES/packages/k8s.io/client-go/v0.36.1/LICENSELICENSES/packages/k8s.io/client-go/v0.36.1/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/client-go/v0.36.1/third_party/forked/httpcache/LICENSELICENSES/packages/k8s.io/cloud-provider/v0.36.1/LICENSELICENSES/packages/k8s.io/code-generator/v0.36.1/LICENSELICENSES/packages/k8s.io/code-generator/v0.36.1/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/component-base/v0.36.0/LICENSELICENSES/packages/k8s.io/component-base/v0.36.1/LICENSELICENSES/packages/k8s.io/component-helpers/v0.36.1/LICENSELICENSES/packages/k8s.io/controller-manager/v0.36.1/LICENSELICENSES/packages/k8s.io/cri-api/v0.20.6/LICENSELICENSES/packages/k8s.io/gengo/v2/v2.0.0-20250604051438-85fd79dbfd9f/LICENSELICENSES/packages/k8s.io/klog/v2/v2.140.0/LICENSELICENSES/packages/k8s.io/kms/v0.36.1/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/internal/third_party/go-json-experiment/json/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/internal/third_party/govalidator/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/validation/errors/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/validation/spec/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/validation/strfmt/LICENSELICENSES/packages/k8s.io/kube-openapi/v0.0.0-20260512234627-ef417d054102/pkg/validation/validate/LICENSELICENSES/packages/k8s.io/kubectl/v0.36.1/LICENSELICENSES/packages/k8s.io/kubelet/v0.36.1/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/gonum/graph/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/gotestsum/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/gotestsum/NOTICELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/libcontainer/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/libcontainer/NOTICELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/shell2junit/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/forked/vishhstress/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/gimme/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/LICENSES/third_party/multiarch/qemu-user-static/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/logo/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/gonum/graph/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/gotestsum/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/gotestsum/NOTICELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/libcontainer/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/libcontainer/NOTICELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/shell2junit/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/forked/vishhstress/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/gimme/LICENSELICENSES/packages/k8s.io/kubernetes/v1.36.1/third_party/multiarch/qemu-user-static/LICENSELICENSES/packages/k8s.io/metrics/v0.36.1/LICENSELICENSES/packages/k8s.io/pod-security-admission/v0.36.1/LICENSELICENSES/packages/k8s.io/streaming/v0.36.1/LICENSELICENSES/packages/k8s.io/utils/v0.0.0-20260507154919-ff6756f316d2/LICENSELICENSES/packages/k8s.io/utils/v0.0.0-20260507154919-ff6756f316d2/inotify/LICENSELICENSES/packages/k8s.io/utils/v0.0.0-20260507154919-ff6756f316d2/internal/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/utils/v0.0.0-20260507154919-ff6756f316d2/third_party/forked/golang/LICENSELICENSES/packages/k8s.io/utils/v0.0.0-20260507154919-ff6756f316d2/third_party/forked/golang/btree/LICENSELICENSES/packages/sigs.k8s.io/apiserver-network-proxy/konnectivity-client/v0.34.0/LICENSELICENSES/packages/sigs.k8s.io/controller-runtime/v0.24.0/LICENSELICENSES/packages/sigs.k8s.io/knftables/v0.0.20/LICENSELICENSES/packages/sigs.k8s.io/knftables/v0.0.21/LICENSELICENSES/packages/sigs.k8s.io/kustomize/api/v0.21.1/LICENSELICENSES/packages/sigs.k8s.io/kustomize/kustomize/v5/v5.8.1/LICENSELICENSES/packages/sigs.k8s.io/kustomize/kyaml/v0.21.1/LICENSELICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.3.0/LICENSELICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.3.2-0.20260122202528-d9cc6641c482/LICENSELICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.3.2/LICENSELICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.4.0/LICENSEREVIEWING.mdcontrib/kind-common.shcontrib/kind-dpu-sim-lib.shcontrib/kind-dpu-sim-no-overlay.shcontrib/kind-dual-stack-conversion.shcontrib/kind-helm.shcontrib/kind.yaml.j2contrib/perf/get-pr-info.pycontrib/perf/workloads/udn-density-l2-pods-netpol.ymlcontrib/perf/workloads/udn-density-l3-pods-netpol.ymldocs/design/dpu-host-no-overlay-routing.mddocs/design/service-traffic-policy.mddocs/developer-guide/areas.mddocs/developer-guide/local_testing_guide.mddocs/features/requirements.mddocs/installation/INSTALL.KUBEADM.mddocs/installation/INSTALL.SSL.mddocs/installation/OVN-NORTHD.SSL.mddocs/installation/launching-ovn-kubernetes-on-kind.mddocs/installation/launching-ovn-kubernetes-with-dpu.mddocs/installation/launching-ovn-kubernetes-with-helm.mddocs/installation/ovnkube.1docs/observability/metrics.mddocs/okeps/okep-5259-no-overlay.mddocs/okeps/okep-5377-extend-udn-to-support-multiple-cluster-subnets-in-layer3-topology.mdgo-controller/.golangci.ymlgo-controller/Makefilego-controller/README.mdgo-controller/cmd/ovnkube-identity/ovnkubeidentity.gogo-controller/cmd/ovnkube-identity/webhook/webhook.gogo-controller/cmd/ovnkube-identity/webhook/webhook_test.gogo-controller/cmd/ovnkube-trace/ovnkube-trace.gogo-controller/cmd/ovnkube/ovnkube.gogo-controller/go.modgo-controller/hack/lint.shgo-controller/hack/update-third-party-licenses.shgo-controller/pkg/allocator/pod/macs.gogo-controller/pkg/clustermanager/clustermanager_suite_test.gogo-controller/pkg/clustermanager/clustermanager_test.gogo-controller/pkg/clustermanager/egressip_controller.gogo-controller/pkg/clustermanager/managedbgp/controller.gogo-controller/pkg/clustermanager/managedbgp/controller_test.gogo-controller/pkg/clustermanager/network_cluster_controller.gogo-controller/pkg/clustermanager/node/node_allocator.gogo-controller/pkg/clustermanager/nooverlay/controller.gogo-controller/pkg/clustermanager/pod/allocator.gogo-controller/pkg/clustermanager/pod/allocator_test.gogo-controller/pkg/clustermanager/routeadvertisements/controller.gogo-controller/pkg/clustermanager/routeadvertisements/controller_test.gogo-controller/pkg/clustermanager/routeadvertisements/evpn_rawconfig.gogo-controller/pkg/clustermanager/routeadvertisements/evpn_rawconfig_test.gogo-controller/pkg/clustermanager/routeadvertisements/rawconfig.gogo-controller/pkg/clustermanager/routeadvertisements/rawconfig_test.gogo-controller/pkg/clustermanager/status_manager/status_manager.gogo-controller/pkg/clustermanager/userdefinednetwork/controller.gogo-controller/pkg/clustermanager/userdefinednetwork/controller_metrics.gogo-controller/pkg/clustermanager/userdefinednetwork/controller_test.gogo-controller/pkg/clustermanager/userdefinednetwork/template/net-attach-def-template.gogo-controller/pkg/cni/cniserver.gogo-controller/pkg/cni/types/types.gogo-controller/pkg/cni/types/types_test.gogo-controller/pkg/config/config.gogo-controller/pkg/config/config_test.gogo-controller/pkg/controllermanager/controller_manager.gogo-controller/pkg/controllermanager/node_controller_manager.gogo-controller/pkg/controllermanager/node_controller_manager_test.gogo-controller/pkg/crd/networkqos/v1alpha1/types.gogo-controller/pkg/crd/userdefinednetwork/v1/apis/applyconfiguration/userdefinednetwork/v1/layer3config.gogo-controller/pkg/crd/userdefinednetwork/v1/cudn.gogo-controller/pkg/crd/userdefinednetwork/v1/shared.gogo-controller/pkg/crd/userdefinednetwork/v1/spec.gogo-controller/pkg/crd/userdefinednetwork/v1/udn.gogo-controller/pkg/csrapprover/approver_test.gogo-controller/pkg/factory/factory.gogo-controller/pkg/factory/factory_test.gogo-controller/pkg/kube/healthcheck/healthcheck.gogo-controller/pkg/kubevirt/dhcp.gogo-controller/pkg/kubevirt/pod.gogo-controller/pkg/kubevirt/pod_test.gogo-controller/pkg/kubevirt/router.gogo-controller/pkg/libovsdb/libovsdb.gogo-controller/pkg/libovsdb/ops/db_object_types.gogo-controller/pkg/libovsdb/ops/model.gogo-controller/pkg/libovsdb/ops/router.gogo-controller/pkg/libovsdb/ops/router_test.gogo-controller/pkg/libovsdb/util/address_set.gogo-controller/pkg/metrics/cluster_manager.gogo-controller/pkg/metrics/metrics.gogo-controller/pkg/metrics/ovnkube_controller.gogo-controller/pkg/metrics/recorders/duration.gogo-controller/pkg/metrics/recorders/duration_test.gogo-controller/pkg/metrics/server_test.gogo-controller/pkg/networkmanager/nad_controller.gogo-controller/pkg/networkmanager/nad_controller_test.gogo-controller/pkg/node/bridgeconfig/bridgeconfig.gogo-controller/pkg/node/bridgeconfig/bridgeconfig_testutil.gogo-controller/pkg/node/bridgeconfig/bridgeflows.gogo-controller/pkg/node/bridgeconfig/bridgeflows_test.gogo-controller/pkg/node/controllers/egressip/egressip.gogo-controller/pkg/node/controllers/evpn/evpn_pod_controller.gogo-controller/pkg/node/controllers/evpn/evpn_pod_controller_test.go
💤 Files with no reviewable changes (32)
- LICENSES/packages/go.opentelemetry.io/otel/v1.35.0/LICENSE
- LICENSES/packages/golang.org/x/time/v0.9.0/LICENSE
- LICENSES/packages/golang.org/x/text/v0.31.0/LICENSE
- LICENSES/packages/github.com/karrick/godirwalk/v1.17.0/LICENSE
- LICENSES/packages/google.golang.org/protobuf/v1.36.8/LICENSE
- LICENSES/packages/github.com/armon/circbuf/v0.0.0-20190214190532-5111143e8da2/LICENSE
- LICENSES/packages/google.golang.org/grpc/v1.72.1/NOTICE.txt
- LICENSES/packages/go.opentelemetry.io/proto/otlp/v1.5.0/LICENSE
- LICENSES/packages/github.com/mohae/deepcopy/v0.0.0-20170929034955-c48cc78d4826/LICENSE
- LICENSES/packages/golang.org/x/time/v0.11.0/LICENSE
- go-controller/pkg/clustermanager/routeadvertisements/evpn_rawconfig_test.go
- go-controller/pkg/clustermanager/routeadvertisements/evpn_rawconfig.go
- LICENSES/packages/golang.org/x/text/v0.32.0/LICENSE
- LICENSES/packages/github.com/libopenstorage/openstorage/v1.0.0/LICENSE
- LICENSES/packages/golang.org/x/term/v0.38.0/LICENSE
- LICENSES/packages/golang.org/x/xerrors/v0.0.0-20191204190536-9bdfabe68543/LICENSE
- LICENSES/packages/go.opentelemetry.io/otel/trace/v1.35.0/LICENSE
- LICENSES/packages/golang.org/x/tools/v0.39.0/LICENSE
- docs/installation/INSTALL.SSL.md
- LICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.3.2-0.20260122202528-d9cc6641c482/LICENSE
- LICENSES/packages/k8s.io/gengo/v2/v2.0.0-20250604051438-85fd79dbfd9f/LICENSE
- LICENSES/packages/golang.org/x/tools/v0.38.0/LICENSE
- docs/installation/OVN-NORTHD.SSL.md
- LICENSES/packages/go.opentelemetry.io/otel/sdk/v1.34.0/LICENSE
- LICENSES/packages/github.com/spf13/cobra/v1.9.1/LICENSE.txt
- LICENSES/packages/github.com/libopenstorage/openstorage/v1.0.0/pkg/jsonpb/LICENSE
- go-controller/pkg/metrics/server_test.go
- LICENSES/packages/sigs.k8s.io/knftables/v0.0.20/LICENSE
- LICENSES/packages/sigs.k8s.io/structured-merge-diff/v6/v6.3.0/LICENSE
- docs/installation/launching-ovn-kubernetes-with-helm.md
- go-controller/pkg/metrics/ovnkube_controller.go
- go-controller/pkg/csrapprover/approver_test.go
Comment on lines
+134
to
+141
| const mergeComment = [...allComments] | ||
| .reverse() | ||
| .find(c => c.body.includes('/area-maintainer-approved') && | ||
| c.user.login !== 'github-actions[bot]' && | ||
| c.user.login.toLowerCase() !== prAuthor); | ||
| if (mergeComment) { | ||
| requester = mergeComment.user.login.toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Bind check-suite retries to the original approver identity.
Line 136 currently picks the latest comment that merely contains /area-maintainer-approved. A later mention/quote by another user can replace requester and cause false “unauthorized” blocks during retry merges.
Use strict command parsing and persist requester in the bot’s SHA marker comment, then read that marker on check_suite retries.
Suggested patch
+ function isAreaApprovalCommand(body = '') {
+ return /^\s*\/area-maintainer-approved(?:\s|$)/.test(body);
+ }
+
// For check_suite retries, find who previously ran /area-maintainer-approved
let requester = commenter;
let allComments = null;
if (!requester) {
allComments = await fetchAllComments(prNumber);
const mergeComment = [...allComments]
.reverse()
- .find(c => c.body.includes('/area-maintainer-approved') &&
+ .find(c => isAreaApprovalCommand(c.body) &&
c.user.login !== 'github-actions[bot]' &&
c.user.login.toLowerCase() !== prAuthor);
if (mergeComment) {
requester = mergeComment.user.login.toLowerCase();
}
}
@@
if (pendingChecks.length > 0) {
@@
const shaMarker = `<!-- area-merge-sha:${ref} -->`;
+ const requesterMarker = `<!-- area-merge-requester:${requester} -->`;
@@
await github.rest.issues.createComment({
@@
- body: `${shaMarker}\nArea maintainer @${requester} has approved this PR via \`/area-maintainer-approved\`. ` +
+ body: `${shaMarker}\n${requesterMarker}\nArea maintainer @${requester} has approved this PR via \`/area-maintainer-approved\`. ` +
`Waiting on CI checks to complete — will merge automatically when all checks pass.\n\n` +Also applies to: 149-160, 287-303
🤖 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 @.github/scripts/area-merge.js around lines 134 - 141, The current
implementation at lines 134-141 in .github/scripts/area-merge.js uses a simple
includes check for the `/area-maintainer-approved` command, which can
incorrectly match when another user quotes or mentions a previous approval.
Replace this loose string matching with strict command parsing that only
recognizes `/area-maintainer-approved` as a standalone command. Additionally,
persist the requester identity by storing it in the bot's SHA marker comment
when the approval is first processed. Then, at the retry merge sites (lines
149-160 and 287-303), instead of re-parsing comments to find the requester, read
the requester identity from the persisted SHA marker comment. This ensures that
the original approver's identity is maintained throughout retries and prevents
false "unauthorized" blocks caused by later mentions or quotes of the approval
command.
Comment on lines
+1249
to
+1251
| pushd frr-k8s | ||
| git checkout --detach "$FRR_K8S_GIT_REF" | ||
| popd |
There was a problem hiding this comment.
Guard nested pushd/popd calls in clone_frr failure paths.
At Line [1249], Line [1251], and Line [1257], pushd/popd are unguarded. If directory changes fail, follow-up commands can run in the wrong working directory and produce misleading failures. Use || return 1 (or || exit 1) consistently.
Also applies to: 1257-1257
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 1249-1249: Use 'pushd ... || exit' or 'pushd ... || return' in case pushd fails.
(SC2164)
[warning] 1251-1251: Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
(SC2164)
🤖 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 `@contrib/kind-common.sh` around lines 1249 - 1251, The `pushd` and `popd`
commands in the `clone_frr` function are not guarded against failure. At line
1249, the `pushd frr-k8s` command lacks error handling, at line 1251, the first
`popd` command lacks error handling, and at line 1257, another `popd` command
lacks error handling. If any of these directory change operations fail,
subsequent commands will execute in the wrong working directory, causing
misleading failures. Add `|| return 1` after each of these `pushd` and `popd`
commands to ensure that the function returns early with error status if any
directory operation fails.
Source: Linters/SAST tools
Comment on lines
+499
to
+500
| ``` | ||
| table=0, priority=99,ip,in_port="patch-breth0_<udn>",dl_src=<bridge-mac>,nw_src=<node-ip> actions=ct(commit,zone=64000,nat(src=<node-ip>),exec(set_field:<udn-ct-mark>->ct_mark)),output:<physical-port> |
There was a problem hiding this comment.
Add language identifiers to new fenced code blocks.
These new fences are missing language tags and trigger MD040. Add text to keep docs lint-clean.
Suggested fix
-```
+```text
table=0, priority=99,ip,in_port="patch-breth0_<udn>",dl_src=<bridge-mac>,nw_src=<node-ip> actions=ct(commit,zone=64000,nat(src=<node-ip>),exec(set_field:<udn-ct-mark>->ct_mark)),output:<physical-port>
table=0, priority=100,ip,in_port="patch-breth0_<udn>",dl_src=<bridge-mac>,nw_src=<udn-gr-masq-ip> actions=ct(commit,zone=64000,nat(src=<node-ip>),exec(set_field:<udn-ct-mark>->ct_mark)),output:<physical-port>@@
- +text
table=0, priority=175,ip,in_port="patch-breth0_",nw_src= actions=ct(table=4,zone=64001,nat)
@@
-```
+```text
table=4, priority=100,in_port="patch-breth0_<udn>",ip,ip_dst=<udn-pod-cidr> actions=ct(commit,zone=64002,nat(src=<udn-gr-masq-ip>),exec(set_field:<udn-ct-mark>->ct_mark),table=3)
@@
- +text
table=0, priority=500,in_port=LOCAL,ip,ip_dst= actions=ct(zone=64002,nat,table=5)
@@
-```
+```text
table=5, priority=100,ip,ct_mark=<udn-ct-mark> actions=ct(commit,zone=64001,nat,exec(set_field:<udn-ct-mark>->ct_mark)),set_field:<bridge-mac>->eth_dst,output:"patch-breth0_<udn>"
</details>
Also applies to: 518-519, 527-528, 539-540, 548-549
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 499-499: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/design/service-traffic-policy.md around lines 499 - 500, The markdown
file contains multiple fenced code blocks that are missing language identifiers,
which triggers MD040 lint errors. In docs/design/service-traffic-policy.md, add
the text language identifier to all fenced code blocks containing OpenFlow
flow table rules. Specifically, modify the opening triple backticks from ``` to
lines 518-519, 527-528, 539-540, and 548-549. Each code block should have its
opening fence changed to include the language tag to satisfy the markdown
linting requirements.
Source: Linters/SAST tools
|
|
||
| ovn-kubernetes organises its codebase into **areas** — focused domains that are | ||
| owned by designated **Area Maintainers**. Each area has a clear scope of files | ||
| defined in [`CODEOWNERS`](https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/CODEOWNERS) |
There was a problem hiding this comment.
Fix broken/stale links in the area guide.
Lines 5, 9, and 49 currently point to unstable or incorrect targets (blob/master and ../governance/...). Use repository-relative links so docs don’t break with branch/layout changes.
Suggested patch
-defined in [`CODEOWNERS`](https://github.com/ovn-kubernetes/ovn-kubernetes/blob/master/CODEOWNERS)
+defined in [`CODEOWNERS`](../../CODEOWNERS)
-removal process — see [Area Maintainers](../governance/GOVERNANCE.md#area-maintainers)
+removal process — see [Area Maintainers](../../GOVERNANCE.md#area-maintainers)
- Maintainers (see [Governance](../governance/GOVERNANCE.md#area-maintainers)).
+ Maintainers (see [Governance](../../GOVERNANCE.md#area-maintainers)).Also applies to: 9-10, 49-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/developer-guide/areas.md` at line 5, Fix the broken and stale links in
docs/developer-guide/areas.md across three locations. At line 5, replace the
CODEOWNERS link that uses blob/master with a repository-relative link format. At
lines 9-10, replace the relative path references (../governance/...) with
repository-relative links that start from the repository root to ensure they
remain valid regardless of branch or directory structure changes. At line 49,
similarly replace any blob/master or relative path references with
repository-relative link syntax. Use consistent repository-relative link
formatting across all three locations so the documentation remains stable across
different branches and layout changes.
Comment on lines
+16
to
+18
| ``` | ||
| # Virtualization (Area Maintainer: @user) | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced example block.
Line 16 opens a fenced block without a language, which triggers MD040.
Suggested patch
- ```
+ ```text
# Virtualization (Area Maintainer: `@user`)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/developer-guide/areas.md` around lines 16 - 18, The fenced code block
opening with triple backticks is missing a language identifier, which triggers
the MD040 markdown linting rule. Add the language identifier "text" immediately
after the opening triple backticks (change ``` to ```text) on line 16 of the
docs/developer-guide/areas.md file. This specifies the content type of the code
block and satisfies the linting requirement.
Source: Linters/SAST tools
Comment on lines
+136
to
+143
| for _, subnet := range subnets { | ||
| if err := na.clusterSubnetAllocator.AddNetworkRange(subnet.CIDR, subnet.HostSubnetLength); err != nil { | ||
| return fmt.Errorf("failed to add network range %s/%d: %w", subnet.CIDR.String(), subnet.HostSubnetLength, err) | ||
| } | ||
| klog.V(5).Infof("Added new network range %s/%d to cluster subnet allocator for network %s", | ||
| subnet.CIDR.String(), subnet.HostSubnetLength, na.netInfo.GetNetworkName()) | ||
| } | ||
| na.recordSubnetCount() |
There was a problem hiding this comment.
Make AddSubnets atomic to avoid partial allocator mutation
At Line 137, subnets are committed one-by-one, and on a later failure the function returns without undoing earlier additions. That can leave clusterSubnetAllocator in a partially applied state for a failed update attempt.
Consider staging and committing only on full success (or rolling back already-added ranges on failure).
🤖 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 `@go-controller/pkg/clustermanager/node/node_allocator.go` around lines 136 -
143, The AddNetworkRange calls in the loop are not atomic, leaving the
clusterSubnetAllocator in a partially applied state if an error occurs during a
later iteration. Refactor the method to either stage all subnet additions and
validate them before committing, or implement rollback logic to undo previously
added ranges (via AddNetworkRange or a similar removal method) when an error
occurs during the loop. Ensure na.recordSubnetCount() is only called after all
subnets have been successfully added.
Comment on lines
+973
to
981
| if hasEVPN && vrfASNs[""] == 0 { | ||
| // Look for global router in the source FRRConfiguration, not in the filtered routers | ||
| for _, router := range source.Spec.BGP.Routers { | ||
| if router.VRF == "" { // default VRF | ||
| globalRouterASN = router.ASN | ||
| vrfASNs[""] = router.ASN | ||
| vrfNeighbors[""] = make([]string, 0, len(router.Neighbors)) | ||
| for _, neighbor := range router.Neighbors { | ||
| neighbors = append(neighbors, neighbor.Address) | ||
| vrfNeighbors[""] = append(vrfNeighbors[""], neighbor.Address) | ||
| } |
There was a problem hiding this comment.
Enforce dual-stack neighbor rejection in the default-VRF fallback path.
Lines 973-981 populate vrfNeighbors[""] from the source default router without checking DualStackAddressFamily. That bypasses the new dual-stack restriction for EVPN raw-config generation when the default router is reached only through this fallback.
Suggested fix
for _, router := range source.Spec.BGP.Routers {
if router.VRF == "" { // default VRF
vrfASNs[""] = router.ASN
vrfNeighbors[""] = make([]string, 0, len(router.Neighbors))
for _, neighbor := range router.Neighbors {
+ if neighbor.DualStackAddressFamily {
+ return nil, fmt.Errorf("%w: DualStackAddressFamily==true not supported, seen on FRRConfiguration %s/%s neighbor %s",
+ errConfig,
+ source.Namespace,
+ source.Name,
+ neighbor.Address,
+ )
+ }
vrfNeighbors[""] = append(vrfNeighbors[""], neighbor.Address)
}
break
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if hasEVPN && vrfASNs[""] == 0 { | |
| // Look for global router in the source FRRConfiguration, not in the filtered routers | |
| for _, router := range source.Spec.BGP.Routers { | |
| if router.VRF == "" { // default VRF | |
| globalRouterASN = router.ASN | |
| vrfASNs[""] = router.ASN | |
| vrfNeighbors[""] = make([]string, 0, len(router.Neighbors)) | |
| for _, neighbor := range router.Neighbors { | |
| neighbors = append(neighbors, neighbor.Address) | |
| vrfNeighbors[""] = append(vrfNeighbors[""], neighbor.Address) | |
| } | |
| if hasEVPN && vrfASNs[""] == 0 { | |
| // Look for global router in the source FRRConfiguration, not in the filtered routers | |
| for _, router := range source.Spec.BGP.Routers { | |
| if router.VRF == "" { // default VRF | |
| vrfASNs[""] = router.ASN | |
| vrfNeighbors[""] = make([]string, 0, len(router.Neighbors)) | |
| for _, neighbor := range router.Neighbors { | |
| if neighbor.DualStackAddressFamily { | |
| return nil, fmt.Errorf("%w: DualStackAddressFamily==true not supported, seen on FRRConfiguration %s/%s neighbor %s", | |
| errConfig, | |
| source.Namespace, | |
| source.Name, | |
| neighbor.Address, | |
| ) | |
| } | |
| vrfNeighbors[""] = append(vrfNeighbors[""], neighbor.Address) | |
| } | |
| break | |
| } | |
| } |
🤖 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 `@go-controller/pkg/clustermanager/routeadvertisements/controller.go` around
lines 973 - 981, The default-VRF fallback path (when hasEVPN and vrfASNs[""] is
0) populates vrfNeighbors[""] from the source router's neighbors without
validating the DualStackAddressFamily restriction, bypassing the dual-stack
enforcement for EVPN. Add a DualStackAddressFamily check in the loop where
neighbors are appended to vrfNeighbors[""] to reject any dual-stack neighbors
that should not be included in EVPN raw-config generation, consistent with how
this restriction is enforced elsewhere in the code.
Comment on lines
+75
to
+80
| vmDescription, err := NewVMDescriptionFromPod(pod) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(vmPods) == 0 { | ||
| return []*corev1.Pod{}, nil | ||
| vmPods, err := vmDescription.OwnedPods(client.PodCoreInformer().Lister()) | ||
| if err != nil { |
There was a problem hiding this comment.
Guard vmDescription == nil before dereference to prevent panics.
NewVMDescriptionFromPod() can return nil, nil, but Line 79 and Line 174 dereference it unconditionally. This can panic on reachable paths that call these helpers with non-VM pods (or partially-labeled pods).
💡 Proposed fix
func findVMRelatedPods(client *factory.WatchFactory, pod *corev1.Pod) ([]*corev1.Pod, error) {
vmDescription, err := NewVMDescriptionFromPod(pod)
if err != nil {
return nil, err
}
+ if vmDescription == nil {
+ return nil, nil
+ }
vmPods, err := vmDescription.OwnedPods(client.PodCoreInformer().Lister())
if err != nil {
return nil, err
}
...
}
func AllVMPodsAreCompleted(podLister listersv1.PodLister, pod *corev1.Pod) (bool, error) {
if !util.PodCompleted(pod) {
return false, nil
}
vmDescription, err := NewVMDescriptionFromPod(pod)
if err != nil {
return false, err
}
+ if vmDescription == nil {
+ return false, nil
+ }
vmPods, err := vmDescription.OwnedPods(podLister)
...
}Also applies to: 169-175
🤖 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 `@go-controller/pkg/kubevirt/pod.go` around lines 75 - 80, The
NewVMDescriptionFromPod() function can return (nil, nil) without an error, but
the code unconditionally dereferences vmDescription by calling
vmDescription.OwnedPods() without first checking if vmDescription is nil. Add a
nil check for vmDescription immediately after the error check on the result of
NewVMDescriptionFromPod() to guard against panics when the function returns nil
without error. Apply the same nil guard pattern at all other locations where
vmDescription is dereferenced after being returned from
NewVMDescriptionFromPod().
Comment on lines
+84
to
+87
| _, err = libovsdbops.FindNATsWithPredicate(nbClient, func(nat *nbdb.NAT) bool { | ||
| recordReferences(nat.Match) | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Include NAT AllowedExtIPs/ExemptedExtIPs when detecting live address sets.
This cleanup only scans nat.Match, but NAT rows can now reference address sets through AllowedExtIPs and existing ExemptedExtIPs. Those live address sets would be treated as stale and deleted.
Proposed fix
_, err = libovsdbops.FindNATsWithPredicate(nbClient, func(nat *nbdb.NAT) bool {
recordReferences(nat.Match)
+ if nat.AllowedExtIPs != nil {
+ recordReferences(*nat.AllowedExtIPs)
+ }
+ if nat.ExemptedExtIPs != nil {
+ recordReferences(*nat.ExemptedExtIPs)
+ }
return false
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _, err = libovsdbops.FindNATsWithPredicate(nbClient, func(nat *nbdb.NAT) bool { | |
| recordReferences(nat.Match) | |
| return false | |
| }) | |
| _, err = libovsdbops.FindNATsWithPredicate(nbClient, func(nat *nbdb.NAT) bool { | |
| recordReferences(nat.Match) | |
| if nat.AllowedExtIPs != nil { | |
| recordReferences(*nat.AllowedExtIPs) | |
| } | |
| if nat.ExemptedExtIPs != nil { | |
| recordReferences(*nat.ExemptedExtIPs) | |
| } | |
| return false | |
| }) |
🤖 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 `@go-controller/pkg/libovsdb/util/address_set.go` around lines 84 - 87, The NAT
scanning predicate function only calls recordReferences on nat.Match, but NAT
rows can now reference address sets through AllowedExtIPs and ExemptedExtIPs
fields as well. Update the predicate function passed to FindNATsWithPredicate to
add recordReferences calls for nat.AllowedExtIPs and nat.ExemptedExtIPs in
addition to the existing nat.Match call. This ensures all address sets
referenced by NAT records are properly marked as live and not incorrectly
deleted as stale.
Comment on lines
+70
to
+78
| func (c *Controller) shouldDeleteNeighbors(migrationStatus *kubevirt.LiveMigrationStatus, pod *corev1.Pod) bool { | ||
| if util.PodCompleted(pod) { | ||
| return true | ||
| } | ||
| if migrationStatus == nil || !migrationStatus.IsTargetDomainReady() { | ||
| return nil | ||
| } | ||
| if migrationStatus.SourcePod.Spec.NodeName != c.nodeName { | ||
| return nil | ||
| return false | ||
| } | ||
| return migrationStatus.SourcePod.Spec.NodeName == c.nodeName | ||
| } |
There was a problem hiding this comment.
Fix migration-source deletion path and nil SourcePod handling.
Line 77 can panic when migration is ready but SourcePod is nil. Also, Line 115 deletes by current pod key; during target-pod updates on source node this can miss source cached entries.
💡 Proposed fix
func (c *Controller) shouldDeleteNeighbors(migrationStatus *kubevirt.LiveMigrationStatus, pod *corev1.Pod) bool {
if util.PodCompleted(pod) {
return true
}
if migrationStatus == nil || !migrationStatus.IsTargetDomainReady() {
return false
}
+ if migrationStatus.SourcePod == nil {
+ return false
+ }
return migrationStatus.SourcePod.Spec.NodeName == c.nodeName
}
func (c *Controller) reconcilePod(key string) error {
...
if c.shouldDeleteNeighbors(migrationStatus, pod) {
- return c.deletePodNeighbors(key)
+ deleteKey := key
+ if migrationStatus != nil && migrationStatus.SourcePod != nil {
+ deleteKey = migrationStatus.SourcePod.Namespace + "/" + migrationStatus.SourcePod.Name
+ }
+ return c.deletePodNeighbors(deleteKey)
}
...
}Also applies to: 114-116
🤖 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 `@go-controller/pkg/node/controllers/evpn/evpn_pod_controller.go` around lines
70 - 78, In the shouldDeleteNeighbors method, add a nil check for
migrationStatus.SourcePod before accessing its Spec.NodeName field to prevent a
panic when migration status is ready but SourcePod is nil. Additionally, fix the
deletion logic near line 115 to delete using the source pod key instead of the
current pod key when handling target-pod updates on the source node, ensuring
that source cached entries are properly cleaned up during migration.
Contributor
|
@pperiyasamy: The following tests failed, say
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. |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
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
20 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.
Depends on ovn-kubernetes/ovn-kubernetes#6417, openshift/origin#31237 and openshift/kubernetes#2653.
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Documentation
Chores