USHIFT-7267: fix conformance networking issues#7052
Conversation
The upstream OVN-K binary removed the --nb-address/--sb-address flags, changing the connection model from TCP to unix sockets. With TCP, each connection attempt was fresh and a missing SBDB caused a clean retry. With unix sockets on a shared hostPath, the socket file persists between container restarts. If ovn-controller connects to a leftover socket from a previous SBDB instance, it caches a stale raft commit index. When the new SBDB starts fresh, ovn-controller sees the lower index as "stale data" and enters an infinite reconnect loop, blocking all pod networking. Fix this with a two-part approach: - SBDB/NBDB containers remove stale sockets before starting ovsdb-server, which then creates fresh ones. - ovn-controller waits for the fresh SBDB socket to appear before connecting, ensuring it never grabs a leftover socket. Applied to both single-node and multi-node DaemonSets.
The northd container used template-rendered OVN_NB_DB_LIST and OVN_SB_DB_LIST with the node IP baked into the DaemonSet spec. In multinode, each node rendered the DaemonSet with its own IP, causing spec changes on every reconcile and triggering rolling updates that restarted the SBDB — the root cause of the stale data issue. Replace the baked-in template variables with the runtime env var K8S_NODE_IP, which is resolved per-pod from status.hostIP. This makes the DaemonSet spec identical regardless of which node renders it, eliminating spurious rolling updates. Remove the now-unused Controlplane field from MultiNodeConfig and the OVN_NB_DB_LIST/OVN_SB_DB_LIST render parameters.
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request references USHIFT-7267 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 bug 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. |
WalkthroughOVN multi-node database connectivity now uses per-node TCP endpoints, while database startup removes stale Unix sockets. OVN controllers wait for the southbound database socket before starting. The multinode configuration no longer stores a control-plane address. ChangesOVN readiness and database connectivity
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=warning msg="The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2." ... [truncated 31032 characters] ... elet: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-cli-plugin: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-controller: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
assets/components/ovn/single-node/node/daemonset.yaml (1)
63-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent infinite hanging.
These
whileloops lack a timeout. If the socket fails to materialize, the containers will hang permanently without exiting, masking the failure and complicating remediation. Add a timeout or retry limit.
assets/components/ovn/single-node/node/daemonset.yaml#L63-L63: add a timeout to the socket wait loop.assets/components/ovn/multi-node/node/daemonset.yaml#L63-L63: add a timeout to the socket wait loop.🤖 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 `@assets/components/ovn/single-node/node/daemonset.yaml` at line 63, The socket wait loops in the daemonsets can hang indefinitely when the OVN socket never appears. Update the loop at assets/components/ovn/single-node/node/daemonset.yaml:63-63 and the corresponding loop at assets/components/ovn/multi-node/node/daemonset.yaml:63-63 to enforce a finite timeout or retry limit, exiting with failure when the limit is reached.
🤖 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 `@assets/components/ovn/multi-node/master/daemonset.yaml`:
- Around line 71-72: Revert the manual edits in the OVN daemonset manifests at
assets/components/ovn/multi-node/master/daemonset.yaml lines 71-72,
assets/components/ovn/single-node/master/daemonset.yaml lines 136-137,
assets/components/ovn/single-node/node/daemonset.yaml lines 58-65, and
assets/components/ovn/multi-node/node/daemonset.yaml lines 58-65. Apply the
intended changes through the upstream patching or templating mechanism instead
of editing rebase-managed assets directly.
---
Nitpick comments:
In `@assets/components/ovn/single-node/node/daemonset.yaml`:
- Line 63: The socket wait loops in the daemonsets can hang indefinitely when
the OVN socket never appears. Update the loop at
assets/components/ovn/single-node/node/daemonset.yaml:63-63 and the
corresponding loop at assets/components/ovn/multi-node/node/daemonset.yaml:63-63
to enforce a finite timeout or retry limit, exiting with failure when the limit
is reached.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9a318813-d3f4-4795-bcb2-a3cad9b1e8ba
📒 Files selected for processing (6)
assets/components/ovn/multi-node/master/daemonset.yamlassets/components/ovn/multi-node/node/daemonset.yamlassets/components/ovn/single-node/master/daemonset.yamlassets/components/ovn/single-node/node/daemonset.yamlpkg/components/networking.gopkg/config/multinode.go
💤 Files with no reviewable changes (1)
- pkg/config/multinode.go
| --ovnnb-db "tcp:${K8S_NODE_IP}:{{.OVN_NB_PORT}}" \ | ||
| --ovnsb-db "tcp:${K8S_NODE_IP}:{{.OVN_SB_PORT}}" \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚖️ Poor tradeoff
Do not manually edit asset manifests.
As per coding guidelines, upstream component manifests under assets/ are rebase-managed and must not be edited manually. Ensure changes are applied via the correct patching or templating mechanism upstream.
assets/components/ovn/multi-node/master/daemonset.yaml#L71-L72: revert or migrate this manual edit.assets/components/ovn/single-node/master/daemonset.yaml#L136-L137: revert or migrate this manual edit.assets/components/ovn/single-node/node/daemonset.yaml#L58-L65: revert or migrate this manual edit.assets/components/ovn/multi-node/node/daemonset.yaml#L58-L65: revert or migrate this manual edit.
📍 Affects 4 files
assets/components/ovn/multi-node/master/daemonset.yaml#L71-L72(this comment)assets/components/ovn/single-node/master/daemonset.yaml#L136-L137assets/components/ovn/single-node/node/daemonset.yaml#L58-L65assets/components/ovn/multi-node/node/daemonset.yaml#L58-L65
🤖 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 `@assets/components/ovn/multi-node/master/daemonset.yaml` around lines 71 - 72,
Revert the manual edits in the OVN daemonset manifests at
assets/components/ovn/multi-node/master/daemonset.yaml lines 71-72,
assets/components/ovn/single-node/master/daemonset.yaml lines 136-137,
assets/components/ovn/single-node/node/daemonset.yaml lines 58-65, and
assets/components/ovn/multi-node/node/daemonset.yaml lines 58-65. Apply the
intended changes through the upstream patching or templating mechanism instead
of editing rebase-managed assets directly.
Source: Coding guidelines
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eslutsky, pacevedom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: This PR has been marked as verified by 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. |
|
/override ci/prow/e2e-aws-tests-bootc-el9 Unrelated error. |
|
@pacevedom: Overrode contexts on behalf of pacevedom: ci/prow/e2e-aws-tests-bootc-el9 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 kubernetes-sigs/prow repository. |
|
@pacevedom: 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. |
|
/cherry-pick release-4.22 |
|
@pacevedom: new pull request created: #7056 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 kubernetes-sigs/prow repository. |
4 participants
Summary by CodeRabbit