OCPBUGS-49662: Prevent resetting masquerade subnet#2641
Conversation
ovnkube running in ovnkube-control-plane need to know about masquerade subnet as it is currently configurable at day 2. If ovnkube is not aware of custom masquerade subnet, then subnet overlap check at ovnkube side during startup would consider default masquerade subnet and overlap check would be inaccurate. Signed-off-by: Arnab Ghosh <arnabghosh89@gmail.com>
Prevent resetting masquerade subnet to default value(169.254.169.0/29) set at upstream ovn-kubernetes when ovnkube-node daemonset is removed from the cluster. If masquerade subnet is configured through network.operator custom resource then it takes precedence over default masquerade subnet. Signed-off-by: Arnab Ghosh <arnabghosh89@gmail.com>
arghosh93
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@arghosh93: This pull request references Jira Issue OCPBUGS-49662, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira-refresh |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@arghosh93: This pull request references Jira Issue OCPBUGS-49662, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/retest-required |
|
/retest |
|
|
||
| // set the default masquerade CIDR for new clusters while ignoring upgrades | ||
| if res.ControlPlaneUpdateStatus == nil && res.NodeUpdateStatus == nil { | ||
| if res.ControlPlaneUpdateStatus == nil || res.NodeUpdateStatus == nil { |
There was a problem hiding this comment.
What if someone removes both the controlplane and the dataplane components?
There was a problem hiding this comment.
If I remove both ovnkube-node DS and ovnkube-control-plane deployment then also masquerade subnet does not get reverted back to old default subnets.
Do you anticipate this change may override old default masquerade subnets during a upgrade? Then I would say this change is not sufficient to address the issue.
There was a problem hiding this comment.
I have tried to upgrade from one version to other(4.17-4.18) and I dont see ovnkube-node DS and ovnkube-control-plane deployment being deleted and instead all images get changed.
There was a problem hiding this comment.
@kyrtapz should we move forward with this change?
There was a problem hiding this comment.
Upgrades are not removing the daemonset either. I thought this fix is to address the corner case of users removing the ovn-k daemonset?
There was a problem hiding this comment.
yes, correct. Masquerade subnet gets reset when either ovnkube-node daemonset or ovnkube-control-plane deployment is removed manually.
|
/retest-required |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/lgtm NOTE: This PR does not address the case where both the control-plane and the dataplane components were removed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arghosh93, 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 |
openshift-ci
Bot
added
the
approved
|
@asood-rh can you please run a pre-merge test with this PR? Following is what we need to test:
The above test also makes sense after changing masquerade subnet at day 2.
Regarding upgrades. We are checking those in CI. |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
openshift-ci-robot
removed
the
jira/valid-bug
WalkthroughIntroduces new IPv4/IPv6 masquerade subnet configuration flags to OVN Kubernetes control plane manifests. Updates the default masquerade subnet assignment logic from requiring both update statuses nil to requiring either to be nil. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes.go (1)
1309-1309: Logic change correctly addresses the PR objective.The change from
&&to||ensures default masquerade subnets are configured when either the control-plane or node component is missing, preventing fallback to upstream defaults (169.254.169.0/29). This works in conjunction with GatewayConfig overrides (lines 141-146 inrenderOVNKubernetes) to preserve day-2 configurations.However, the comment on line 1308 could be clarified to reflect the expanded scope:
- // set the default masquerade CIDR for new clusters while ignoring upgrades + // set the default masquerade CIDR for new clusters or when components are missing, while ignoring upgrades if res.ControlPlaneUpdateStatus == nil || res.NodeUpdateStatus == nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yaml(2 hunks)bindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yaml(2 hunks)pkg/network/ovn_kubernetes.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yamlbindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yamlpkg/network/ovn_kubernetes.go
🔇 Additional comments (2)
bindata/network/ovn-kubernetes/managed/ovnkube-control-plane.yaml (1)
174-181: LGTM! Masquerade subnet configuration added correctly to control-plane.The implementation follows the established pattern for other configurable subnets (join subnet, transit switch subnet). These changes enable the control-plane to receive masquerade subnet configuration, ensuring day-2 changes propagate correctly and preventing fallback to upstream defaults when the node daemonset is removed.
Also applies to: 223-224
bindata/network/ovn-kubernetes/self-hosted/ovnkube-control-plane.yaml (1)
127-134: LGTM! Self-hosted control-plane correctly updated with masquerade subnet configuration.These changes mirror the managed deployment and ensure consistency across both deployment modes. The implementation follows the established pattern for subnet configuration and enables proper masquerade subnet handling for self-hosted deployments.
Also applies to: 173-174
|
/retest |
|
/tide refresh |
|
/retest 4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade |
|
@arghosh93: The The following commands are available to trigger optional jobs: Use 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. |
|
/test 4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade |
|
@arghosh93: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
/retest |
|
/tide refresh |
|
/override ci/prow/4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade |
|
@arghosh93: arghosh93 unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers openshift-sustaining-engineers. 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. |
|
/override ci/prow/4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade since this PR is very old its stuck on the older version jobs, overriding this to get this in |
|
@tssurya: Overrode contexts on behalf of tssurya: ci/prow/4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade 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. |
|
/retest-required |
3 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
@arghosh93: 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. |
|
/retest-required |
41c1ac3
into
openshift:master
|
@arghosh93: Jira Issue Verification Checks: Jira Issue OCPBUGS-49662 Jira Issue OCPBUGS-49662 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-03-15-203841 |
|
/cherry-pick release-4.21 release-4.20 |
|
@fedra197: only openshift org members may request cherry picks. If you are already part of the org, make sure to change your membership to public. Otherwise you can still do the cherry-pick manually. 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. |
|
/cherry-pick release-4.21 release-4.20 |
|
@fedra197: new pull request created: #3053 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. |
9 participants
Prevent resetting masquerade subnet to default value(169.254.169.0/29)
set at upstream ovn-kubernetes when ovnkube-node daemonset is removed
from the cluster.
This PR have an additional commit to update masquerade subnet to
ovnkube-control-plane deployment. This is required as masquerade
subnet is currently configurable at day 2.