Add enable-dynamic-udn-allocation config override#2986
Add enable-dynamic-udn-allocation config override#2986kyrtapz wants to merge 1 commit intoopenshift:masterfrom
Conversation
Support --enable-dynamic-udn-allocation flag for both ovnkube-cluster-manager and ovnkube-controller via the ovn-kubernetes-config-overrides configmap. Signed-off-by: Patryk Diak <pdiak@redhat.com>
|
Skipping CI for Draft Pull Request. |
WalkthroughThis change introduces support for dynamic UDN allocation across OVN-Kubernetes configurations. It adds conditional flag construction in YAML manifests, reads the configuration override from OVN-Kubernetes config with validation, and includes a test to verify the feature's behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kyrtapz 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes_test.go (1)
4453-4514: ⚡ Quick winCover the control-plane flag path in this new override test.
Lines 4453-4514 only validate the script-lib rendering path. Since this PR targets both
ovnkube-controllerandovnkube-cluster-manager, add control-plane assertions too so both render paths are protected from regressions.Proposed test addition
func TestRenderOVNKubernetes_EnableDynamicUDNAllocationOverride(t *testing.T) { @@ t.Run("with invalid enable-dynamic-udn-allocation override", func(t *testing.T) { ovnkubeScriptLib := renderWithOverrides(map[string]string{"enable-dynamic-udn-allocation": "notabool"}) g.Expect(ovnkubeScriptLib).To(ContainSubstring(` if [[ "" != "" ]]; then enable_dynamic_udn_allocation_flag="--enable-dynamic-udn-allocation=" fi`)) }) + + t.Run("control-plane includes enable-dynamic-udn-allocation=true", func(t *testing.T) { + script := renderControlPlaneWithOverrides(t, "self-hosted", map[string]interface{}{ + "enable-dynamic-udn-allocation": "true", + }) + g.Expect(script).To(ContainSubstring(`--enable-dynamic-udn-allocation=true`)) + }) + + t.Run("control-plane renders empty value for invalid enable-dynamic-udn-allocation", func(t *testing.T) { + script := renderControlPlaneWithOverrides(t, "self-hosted", map[string]interface{}{ + "enable-dynamic-udn-allocation": "notabool", + }) + g.Expect(script).To(ContainSubstring(`--enable-dynamic-udn-allocation=`)) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/network/ovn_kubernetes_test.go` around lines 4453 - 4514, The test only asserts the node-agent/script-lib path (using extractOVNScriptLib) but doesn't cover the control-plane rendering path; update TestRenderOVNKubernetes_EnableDynamicUDNAllocationOverride by changing renderWithOverrides to also extract the control-plane script (use the existing renderOVNKubernetes call and call the control-plane extractor, e.g. extractOVNControlPlaneScriptLib or the appropriate helper used elsewhere in tests) and return both libs (or return objs so callers can extract both); then for each subtest duplicate the same ContainSubstring assertions against the control-plane script variable (ovnkube-controller / ovnkube-cluster-manager path) so the enable-dynamic-udn-allocation flag is validated for both extractOVNScriptLib and the control-plane extractor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/network/ovn_kubernetes_test.go`:
- Around line 4453-4514: The test only asserts the node-agent/script-lib path
(using extractOVNScriptLib) but doesn't cover the control-plane rendering path;
update TestRenderOVNKubernetes_EnableDynamicUDNAllocationOverride by changing
renderWithOverrides to also extract the control-plane script (use the existing
renderOVNKubernetes call and call the control-plane extractor, e.g.
extractOVNControlPlaneScriptLib or the appropriate helper used elsewhere in
tests) and return both libs (or return objs so callers can extract both); then
for each subtest duplicate the same ContainSubstring assertions against the
control-plane script variable (ovnkube-controller / ovnkube-cluster-manager
path) so the enable-dynamic-udn-allocation flag is validated for both
extractOVNScriptLib and the control-plane extractor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0643f30f-f75b-471a-b289-09b33480162f
📒 Files selected for processing (5)
bindata/network/ovn-kubernetes/common/008-script-lib.yamlbindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yamlbindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yamlpkg/network/ovn_kubernetes.gopkg/network/ovn_kubernetes_test.go
Support --enable-dynamic-udn-allocation flag for both ovnkube-cluster-manager and ovnkube-controller via the ovn-kubernetes-config-overrides configmap.
Example:
should automatically re-deploy ovn-k.
Summary by CodeRabbit
Release Notes
New Features
Tests