Resolve ovnkube node encap IP in startup script#2998
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
Walkthrough
ChangesEncapsulation IP startup wiring
Estimated code review effort: 2 (Simple) | ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 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=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v0.8.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17357 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit 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" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SchSeba 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 544-547: Normalize and trim whitespace from the OVN_ENCAP_IP
environment variable before constructing ovn_encap_ip_flag: create a trimmed
value (e.g., strip leading/trailing whitespace from OVN_ENCAP_IP), check that
the trimmed value is non-empty, then set
ovn_encap_ip_flag="--encap-ip=${TRIMMED_OVN_ENCAP_IP}" and log the trimmed
value; update uses of OVN_ENCAP_IP to reference the trimmed variable so
accidental surrounding whitespace won't produce an invalid or split --encap-ip
argument.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ed7676fc-f101-424f-bb21-db5380f271b0
📒 Files selected for processing (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml
| if [[ -n "${OVN_ENCAP_IP}" ]]; then | ||
| log "encapip" "Using OVN_ENCAP_IP override ${OVN_ENCAP_IP}" | ||
| ovn_encap_ip_flag="--encap-ip=${OVN_ENCAP_IP}" | ||
| fi |
There was a problem hiding this comment.
Normalize OVN_ENCAP_IP before building the flag.
At Line 544, OVN_ENCAP_IP is used verbatim. Accidental whitespace in env overrides can produce an invalid/split --encap-ip argument at runtime.
Suggested patch
- if [[ -n "${OVN_ENCAP_IP}" ]]; then
- log "encapip" "Using OVN_ENCAP_IP override ${OVN_ENCAP_IP}"
- ovn_encap_ip_flag="--encap-ip=${OVN_ENCAP_IP}"
+ local encap_ip_override="${OVN_ENCAP_IP//[[:space:]]/}"
+ if [[ -n "${encap_ip_override}" ]]; then
+ log "encapip" "Using OVN_ENCAP_IP override ${encap_ip_override}"
+ ovn_encap_ip_flag="--encap-ip=${encap_ip_override}"
fi📝 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.
| if [[ -n "${OVN_ENCAP_IP}" ]]; then | |
| log "encapip" "Using OVN_ENCAP_IP override ${OVN_ENCAP_IP}" | |
| ovn_encap_ip_flag="--encap-ip=${OVN_ENCAP_IP}" | |
| fi | |
| local encap_ip_override="${OVN_ENCAP_IP//[[:space:]]/}" | |
| if [[ -n "${encap_ip_override}" ]]; then | |
| log "encapip" "Using OVN_ENCAP_IP override ${encap_ip_override}" | |
| ovn_encap_ip_flag="--encap-ip=${encap_ip_override}" | |
| fi |
🤖 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 `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml` around lines 544 -
547, Normalize and trim whitespace from the OVN_ENCAP_IP environment variable
before constructing ovn_encap_ip_flag: create a trimmed value (e.g., strip
leading/trailing whitespace from OVN_ENCAP_IP), check that the trimmed value is
non-empty, then set ovn_encap_ip_flag="--encap-ip=${TRIMMED_OVN_ENCAP_IP}" and
log the trimmed value; update uses of OVN_ENCAP_IP to reference the trimmed
variable so accidental surrounding whitespace won't produce an invalid or split
--encap-ip argument.
SchSeba
force-pushed
the
encap-ip-from-file-and-env
branch
from
55250eb to
71961a6
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
SchSeba
force-pushed
the
encap-ip-from-file-and-env
branch
from
71961a6 to
f596db7
Compare
|
Hi @tssurya thanks for the comment! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 481-490: The set-encap-ip-flag helper currently only reads
OVN_ENCAP_IP, leaving ovn_encap_ip_flag empty when that env var is unset, so
update this function to also resolve the encap IP from the
/etc/ovnk/encap_interface fallback before start-ovnkube-node builds the ovnkube
command. Use the existing set-encap-ip-flag and start-ovnkube-node symbols to
wire in the file-based lookup, and ensure the fallback sets --encap-ip when the
mounted encap file is present.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eec6ff95-f2f2-4481-a5e7-0627390ea41b
📒 Files selected for processing (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml
tssurya
left a comment
tssurya
left a comment
There was a problem hiding this comment.
- Please update the PR describe with the latest design details and remove references to our older approach or keep it for history but perhaps tell we didn't go with that approach.
- Attach the JIRA to the title
- Add a section on "How to test/verify this and how that was done" - we won't be able to set "verified" on this PR till that is done
- Add some unit tests to ensure the parsing works
- Address rabbit comments.
Main logic LGTM - looks simple
| ${gateway_mode_flags} \ | ||
| ${node_mgmt_port_netdev_flags} \ | ||
| ${ovnkube_node_mode} \ | ||
| ${ovn_encap_ip_flag} \ |
There was a problem hiding this comment.
is that it has to be a singe IP or comma of IPs is accepted for dualstack? what's the design for this config field upstream? I forget the details..
Teach the ovnkube node wrapper to derive --encap-ip from
the OVN_ENCAP_IP environment variable for per-node configuration.
The variable is expected to be set through the env-overrides ConfigMap
so that each node can receive its own encap IP. For example:
apiVersion: v1
kind: ConfigMap
metadata:
name: env-overrides
namespace: openshift-ovn-kubernetes
data:
ocp-virt-ctlplane-0.lab: |
OVN_ENCAP_IP=10.0.0.1
ocp-virt-worker-0.lab: |
OVN_ENCAP_IP=10.0.0.2
ocp-virt-worker-1.lab: |
OVN_ENCAP_IP=10.0.0.3
ocp-virt-worker-2.lab: |
OVN_ENCAP_IP=10.0.0.4
Signed-off-by: Sebastian Sch <sebassch@gmail.com>
SchSeba
force-pushed
the
encap-ip-from-file-and-env
branch
from
f596db7 to
08f19ed
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes_test.go (1)
4408-4433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test setup boilerplate.
The bootstrap/config setup (crd, config, bootstrapResult, featureGatesCNO, fakeClient, render call) is duplicated almost verbatim across both new tests. Consider extracting a shared helper (e.g.,
renderOVNKubernetesForEncapIPTest(t)) to reduce duplication and ease future maintenance.♻️ Suggested helper extraction
+func renderOVNKubernetesWithDefaults(g *WithT) []*uns.Unstructured { + crd := OVNKubernetesConfig.DeepCopy() + config := &crd.Spec + fillDefaults(config, nil) + + bootstrapResult := fakeBootstrapResult() + bootstrapResult.OVN = bootstrap.OVNBootstrapResult{ + ControlPlaneReplicaCount: 3, + OVNKubernetesConfig: &bootstrap.OVNConfigBoostrapResult{ + DpuHostModeLabel: OVN_NODE_SELECTOR_DEFAULT_DPU_HOST, + DpuModeLabel: OVN_NODE_SELECTOR_DEFAULT_DPU, + SmartNicModeLabel: OVN_NODE_SELECTOR_DEFAULT_SMART_NIC, + MgmtPortResourceName: "", + DpuNodeLeaseRenewInterval: DPU_NODE_LEASE_RENEW_INTERVAL_DEFAULT, + DpuNodeLeaseDuration: DPU_NODE_LEASE_DURATION_DEFAULT, + HyperShiftConfig: &bootstrap.OVNHyperShiftBootstrapResult{ + Enabled: false, + }, + }, + } + featureGatesCNO := getDefaultFeatureGates() + fakeClient := cnofake.NewFakeClient() + + objs, _, err := renderOVNKubernetes(config, bootstrapResult, manifestDirOvn, fakeClient, featureGatesCNO) + g.Expect(err).NotTo(HaveOccurred()) + return objs +}Also applies to: 4447-4473
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/network/ovn_kubernetes_test.go` around lines 4408 - 4433, The test setup in TestRenderOVNKubernetes_EncapIPEnvOverrideWiring is duplicated across the new encapsulation IP tests, including the crd/config bootstrapResult/fakeClient/featureGatesCNO/renderOVNKubernetes flow. Extract the repeated bootstrap and render setup into a shared helper such as renderOVNKubernetesForEncapIPTest so both tests can call it and only customize the scenario-specific assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4408-4433: The test setup in
TestRenderOVNKubernetes_EncapIPEnvOverrideWiring is duplicated across the new
encapsulation IP tests, including the crd/config
bootstrapResult/fakeClient/featureGatesCNO/renderOVNKubernetes flow. Extract the
repeated bootstrap and render setup into a shared helper such as
renderOVNKubernetesForEncapIPTest so both tests can call it and only customize
the scenario-specific assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1177293e-03f9-4bec-97ee-dbf03fa611ce
📒 Files selected for processing (2)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlpkg/network/ovn_kubernetes_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/network/ovn-kubernetes/common/008-script-lib.yaml
|
What about day2 changes? Are they supported? So once cluster comes up can users change this? |
|
@SchSeba: 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. |
zeeke
left a comment
zeeke
left a comment
There was a problem hiding this comment.
only one minor comment on my side
| // TestRenderOVNKubernetes_EncapIPEnvOverrideWiring verifies the rendered OVN | ||
| // script library sources per-node overrides and wires OVN_ENCAP_IP into the | ||
| // ovnkube-node command line. | ||
| func TestRenderOVNKubernetes_EncapIPEnvOverrideWiring(t *testing.T) { |
There was a problem hiding this comment.
I'd drop this test, as it makes noise without giving too value
3 participants
Teach the ovnkube node wrapper to derive --encap-ip from /etc/ovnk/encap_interface, including host-mounted lookups and dual-stack addresses, while allowing OVN_ENCAP_IP to override the resolved value for per-node configuration.
Summary by CodeRabbit
New Features
OVN_ENCAP_IPinto theovnkubestartup as--encap-ip=${OVN_ENCAP_IP}./env.Tests
OVN_ENCAP_IPwiring and theenv-overridesvolume/ConfigMap behavior.