Make OpenStack max_allowed_address_pairs configurable#236
Conversation
Read the value from the "max-allowed-address-pairs" key in the already-mounted kube-cloud-config ConfigMap, falling back to the Neutron default of 10 when absent. This lets admins whose deployments use a non-default max_allowed_address_pairs report accurate capacity without a code change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jluhrsen 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 |
Summary by CodeRabbit
WalkthroughOpenStack egress capacity is now read from ChangesOpenStack egress capacity
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OpenStack
participant ConfigDir
participant NeutronPortConfig
OpenStack->>ConfigDir: Read max-allowed-address-pairs
ConfigDir-->>OpenStack: Return configured value or error
OpenStack->>OpenStack: Validate positive integer or use default
OpenStack->>NeutronPortConfig: Calculate capacity with runtime maximum
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cloudprovider/openstack.go (2)
541-557: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct unit tests for
getMaxCapacity().The new function has multiple code paths (valid value, missing file, non-numeric content, zero, negative) but no dedicated test coverage. Existing tests only exercise the fallback implicitly because
o.cfg.ConfigDiris unset. Consider adding table-driven tests that write temp files with various contents and verify the return value and log output.🤖 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 541 - 557, Add direct table-driven unit tests for OpenStack.getMaxCapacity covering valid, missing, non-numeric, zero, and negative file contents. Configure each test with a temporary ConfigDir, verify the returned capacity against the configured value or openstackDefaultMaxCapacity, and capture/assert the expected warning or info logging where applicable.
541-557: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the
getMaxCapacity()result to avoid repeated disk I/O on the hot path.
getMaxCapacity()performsos.ReadFileon every call, and it's invoked fromgetNeutronPortNodeEgressIPConfigurationwhich runs per-port per-node. ConfigMap-mounted files only change on pod restart, so the value is effectively immutable at runtime. Consider reading it once (e.g., during provider initialization or viasync.OnceValue) and caching the result.♻️ Suggested caching with sync.OnceValue
+var maxCapacityCache sync.OnceValue[int] + func (o *OpenStack) getMaxCapacity() int { + return maxCapacityCache.Do(func() int { data, err := os.ReadFile(filepath.Join(o.cfg.ConfigDir, "max-allowed-address-pairs")) if err != nil { return openstackDefaultMaxCapacity } v, err := strconv.Atoi(strings.TrimSpace(string(data))) if err != nil || v <= 0 { klog.Warningf("Ignoring invalid max-allowed-address-pairs value %q, using default %d", string(data), openstackDefaultMaxCapacity) return openstackDefaultMaxCapacity } klog.Infof("Using configured max-allowed-address-pairs: %d", v) return v + }) }🤖 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 541 - 557, Cache the value produced by OpenStack.getMaxCapacity so the ConfigMap file is read only once instead of on each hot-path invocation from getNeutronPortNodeEgressIPConfiguration. Initialize or memoize the result during provider setup or with a concurrency-safe one-time mechanism, while preserving the existing default and validation behavior.
🤖 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.go`:
- Around line 541-557: Add direct table-driven unit tests for
OpenStack.getMaxCapacity covering valid, missing, non-numeric, zero, and
negative file contents. Configure each test with a temporary ConfigDir, verify
the returned capacity against the configured value or
openstackDefaultMaxCapacity, and capture/assert the expected warning or info
logging where applicable.
- Around line 541-557: Cache the value produced by OpenStack.getMaxCapacity so
the ConfigMap file is read only once instead of on each hot-path invocation from
getNeutronPortNodeEgressIPConfiguration. Initialize or memoize the result during
provider setup or with a concurrency-safe one-time mechanism, while preserving
the existing default and validation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dabcb41e-a91f-4696-84cd-420019b5a1eb
📒 Files selected for processing (2)
pkg/cloudprovider/openstack.gopkg/cloudprovider/openstack_test.go
|
@jluhrsen: 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. |
1 participant
Read the value from the "max-allowed-address-pairs" key in the already-mounted kube-cloud-config ConfigMap, falling back to the Neutron default of 10 when absent. This lets admins whose deployments use a non-default max_allowed_address_pairs report accurate capacity without a code change.