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: 48 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: Enterprise Run ID: 📒 Files selected for processing (1)
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: 2 (Simple) | ~15 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant CloudProviderConfig
participant OpenStack
CLI->>CloudProviderConfig: Set OSMaxAllowedAddressPairs
OpenStack->>CloudProviderConfig: Read configured capacity
OpenStack->>OpenStack: Select configured or default capacity
OpenStack->>OpenStack: Compute egress IP capacity
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
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 <dchilder@redhat.com>
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
| return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC", | ||
| openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove parentheses around the format specifiers.
The formatting (%d) around the integer counts produces unusual output (e.g., "which already has (5) allowed address pairs"). Removing the parentheses improves readability. Additionally, this avoids breaking test assertions that check for unparenthesized substrings, as observed in the accompanying tests.
💡 Proposed fix
- return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC",
+ return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has %d allowed address pairs and %d managed by CNCC",
openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount)📝 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.
| return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has (%d) allowed address pairs and (%d) managed by CNCC", | |
| openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount) | |
| } | |
| return nil, fmt.Errorf("max allowed address pairs (%d) is too small for port %s which already has %d allowed address pairs and %d managed by CNCC", | |
| openstackMaxCapacity, p.ID, len(p.AllowedAddressPairs), cloudPrivateIPsCount) | |
| } |
🤖 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.go` around lines 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.
…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 Signed-off-by: Dan Childers <dchilder@redhat.com>
d0174f3 to
d1a1b28
Compare
|
@danchild: 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. |
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