OSASINFRA-4400: make openstack allowed address pairs configurable - #230
OSASINFRA-4400: make openstack allowed address pairs configurable#230danchild wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@danchild: This pull request references OSASINFRA-4400 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 story 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. |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds an OpenStack-specific max allowed address-pairs flag, stores the value in cloud provider configuration, applies positive overrides to egress capacity calculations, and expands related test coverage. ChangesOpenStack capacity override
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant CloudProviderConfig
participant OpenStack
participant EgressConfiguration
CLI->>CloudProviderConfig: Set OSMaxAllowedAddressPairs
OpenStack->>CloudProviderConfig: Read configured capacity
OpenStack->>OpenStack: Select configured or default ceiling
OpenStack->>EgressConfiguration: Compute and validate egress IP capacity
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danchild 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 |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/cloud-network-config-controller/main.go (1)
282-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse constant for platform type comparison.
Consider replacing the hardcoded
"OpenStack"string with thecloudprovider.PlatformTypeOpenStackconstant that is already used on line 205 of this file. This helps avoid magic strings and aligns with existing code patterns.♻️ Proposed refactor
// determine if platform-os-max-allowed-address-pairs was set explicitly with a // command line flag - if platformCfg.PlatformType == "OpenStack" { + if platformCfg.PlatformType == cloudprovider.PlatformTypeOpenStack { flag.Visit(func(f *flag.Flag) { if f.Name == "platform-os-max-allowed-address-pairs" { platformCfg.OSMaxAllowedAddressPairs.SetByFlag = true } }) }🤖 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 `@cmd/cloud-network-config-controller/main.go` around lines 282 - 291, In the platform type check surrounding flag.Visit, replace the hardcoded "OpenStack" comparison with the existing cloudprovider.PlatformTypeOpenStack constant. Keep the explicit flag detection and SetByFlag assignment unchanged.
🤖 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 `@cmd/cloud-network-config-controller/main.go`:
- Around line 282-291: In the platform type check surrounding flag.Visit,
replace the hardcoded "OpenStack" comparison with the existing
cloudprovider.PlatformTypeOpenStack constant. Keep the explicit flag detection
and SetByFlag assignment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 725d7b3c-7cab-4f58-84c3-6aafb21435c6
📒 Files selected for processing (4)
cmd/cloud-network-config-controller/main.gopkg/cloudprovider/cloudprovider.gopkg/cloudprovider/openstack.gopkg/cloudprovider/openstack_test.go
|
/assign @arghosh93 |
9aa6812 to
d0174f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/cloud-network-config-controller/main.go (1)
267-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly reject negative values for the flag.
While negative values are gracefully ignored during the capacity calculation (falling back to the default of 10), explicitly rejecting them during initialization prevents silent misconfigurations and strictly aligns with the PR objective ("zero or negative values are rejected"). As per path instructions, bounds-check user-supplied sizes.
Consider adding validation after
flag.Parse():// Verify required arguments if secretName == "" || platformCfg.PlatformType == "" { klog.Exit("-secret-name or -platform-type is empty, cannot initialize controller") } if platformCfg.OSMaxAllowedAddressPairs < 0 { klog.Exit("-platform-os-max-allowed-address-pairs cannot be negative") }🤖 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 `@cmd/cloud-network-config-controller/main.go` at line 267, Add initialization validation after flag.Parse and the existing required-argument checks to reject negative platformCfg.OSMaxAllowedAddressPairs via klog.Exit with a clear flag-specific message. Preserve zero handling and the existing validation flow.Source: Path instructions
🤖 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 `@pkg/cloudprovider/openstack.go`:
- Around line 611-613: Update the error message format string in the
address-pair capacity check to remove parentheses around all integer format
specifiers, while preserving the existing values, wording, and argument order.
---
Nitpick comments:
In `@cmd/cloud-network-config-controller/main.go`:
- Line 267: Add initialization validation after flag.Parse and the existing
required-argument checks to reject negative platformCfg.OSMaxAllowedAddressPairs
via klog.Exit with a clear flag-specific message. Preserve zero handling and the
existing validation flow.
🪄 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: 31799181-b9e8-493f-a5b4-8e0c8b2db34f
📒 Files selected for processing (4)
cmd/cloud-network-config-controller/main.gopkg/cloudprovider/cloudprovider.gopkg/cloudprovider/openstack.gopkg/cloudprovider/openstack_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cloudprovider/openstack_test.go
d0174f3 to
d1a1b28
Compare
|
/test e2e-azure-serial-2of2 |
|
/test e2e-gcp-ovn |
|
/verified by @danchild |
openshift-ci-robot
added
the
verified
|
@danchild: 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. |
Currently, the maxmium allowed address pairs per port is hardcoded to OpenStack's default of 10. The `platform-os-max-allowed-address-pairs` flag allows users to customize the configuration, mirroring the setting in their installation's neutron.conf. The patch allows any custom configuration greater than zero. Finally, the default of 10 is still used as a fallback if the user doesn't configure `platform-os-max-allowed-address-pairs` Signed-off-by: Dan Childers <[email protected]>
d1a1b28 to
62be2d7
Compare
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cloudprovider/openstack_test.go (1)
1084-1164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a test case to verify negative value fallback behavior.
The PR summary states that negative values are rejected, which the implementation handles by falling back to the default capacity (since the condition
OSMaxAllowedAddressPairs > 0evaluates to false). Consider explicitly adding a test case for a negative value to ensure this fallback behavior remains validated and prevents future regressions.💡 Proposed addition to the test cases
You can append this case to the
tcsslice:{ name: "negative flag uses default capacity", maxAllowedAddrPairs: -5, port: portMap["9ab428d4-58f8-42d7-9672-90c3f5641f83"], nodeEgressIPConfig: NodeEgressIPConfiguration{ Interface: "9ab428d4-58f8-42d7-9672-90c3f5641f83", IFAddr: ifAddr{ IPv4: "192.0.2.0/24", IPv6: "2000::/64", }, Capacity: capacity{ IP: ptr.To(defaultOpenstackMaxCapacity - 5), }, }, },🤖 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/cloudprovider/openstack_test.go` around lines 1084 - 1164, Add a table-driven case to TestGetNeutronPortNodeEgressIPConfigurationMaxAllowedAddressPairs with a negative OSMaxAllowedAddressPairs value, such as -5, and expect the same default capacity configuration as the existing “unset flag uses default capacity” case.
🤖 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/cloudprovider/openstack_test.go`:
- Around line 1084-1164: Add a table-driven case to
TestGetNeutronPortNodeEgressIPConfigurationMaxAllowedAddressPairs with a
negative OSMaxAllowedAddressPairs value, such as -5, and expect the same default
capacity configuration as the existing “unset flag uses default capacity” case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f9a0247c-1a73-4da0-b3d3-16fbca255aaa
📒 Files selected for processing (4)
cmd/cloud-network-config-controller/main.gopkg/cloudprovider/cloudprovider.gopkg/cloudprovider/openstack.gopkg/cloudprovider/openstack_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/cloud-network-config-controller/main.go
- pkg/cloudprovider/openstack.go
62be2d7 to
d68adfb
Compare
d68adfb to
e70977a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/cloudprovider/openstack_test.go (1)
1084-1179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a combined override + existing-cloud-IPs case.
All four new subtests use
cloudPrivateIPConfigsunset (cloudPrivateIPsCount == 0). Consider adding a case where a positiveOSMaxAllowedAddressPairsoverride is combined with non-emptycloudPrivateIPConfigs, mirroring the existing default-path coverage inTestGetNeutronPortNodeEgressIPConfiguration(lines 1043-1057), to fully exercise theopenstackMaxCapacity + cloudPrivateIPsCount - len(...)formula under override.🤖 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/cloudprovider/openstack_test.go` around lines 1084 - 1179, Add a table case to TestGetNeutronPortNodeEgressIPConfigurationMaxAllowedAddressPairs combining a positive maxAllowedAddrPairs override with non-empty cloudPrivateIPConfigs, using the expected capacity from the override plus cloud-private-IP count minus managed addresses. Mirror the existing default-path scenario while preserving the current cases and 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/cloudprovider/openstack_test.go`:
- Around line 1084-1179: Add a table case to
TestGetNeutronPortNodeEgressIPConfigurationMaxAllowedAddressPairs combining a
positive maxAllowedAddrPairs override with non-empty cloudPrivateIPConfigs,
using the expected capacity from the override plus cloud-private-IP count minus
managed addresses. Mirror the existing default-path scenario while preserving
the current cases and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b5241aed-3534-4511-9331-73cf19d2f99f
📒 Files selected for processing (1)
pkg/cloudprovider/openstack_test.go
e70977a to
fa17726
Compare
…irs arg Coverage includes testing the following combinations of conditions: - custom positive value overrides default - unset flag uses default capacity - custom value is too small - negative values are overriden by default value Signed-off-by: Dan Childers <[email protected]>
fa17726 to
6fd36a0
Compare
|
/verified by @danchild |
openshift-ci-robot
added
the
verified
|
@danchild: 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. |
|
/test hypershift-e2e-aks |
|
@danchild: 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. |
3 participants
Currently, the maxmium allowed address pairs per port configuration
is hardcoded to OpenStack's default of 10. The new
platform-os-max-allowed-address-pairsflag allowsusers to customize the configuration, mirroring the
setting in their installation's neutron.conf.
The patch allows any custom configuration greater than
zero and disallows anything less than or equal to zero.
Finally, the default of 10 is still used as a fallback
if the user doesn't configure
platform-os-max-allowed-address-pairs