OSASINFRA-4400: Add support for max_allowed_address_pairs OpenStack - #3058
OSASINFRA-4400: Add support for max_allowed_address_pairs OpenStack#3058danchild wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe change reads ChangesOpenStack address-pair configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant KubernetesAPI
participant Renderer
participant ControllerDeployment
Bootstrap->>KubernetesAPI: fetch cloud-network-config ConfigMap
KubernetesAPI-->>Bootstrap: return address-pair configuration
Bootstrap->>Renderer: provide OpenStackMaxAllowedAddressPairs
Renderer->>ControllerDeployment: render conditional controller flag
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 2 warnings)
✅ Passed checks (16 passed)
✨ 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.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/Masterminds/semver@v1.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/Masterminds/sprig/v3@v3.2.3: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/containernetworking/cni@v1.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ghodss/yaml@v1.0.1-0.20190212211648-25d852aebe32: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-bindata/go-bindata@v3.1.2+incompatible: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/onsi/gomega@v1.39.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/ope ... [truncated 17377 characters] ... red in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/gengo/v2@v2.0.0-20251215205346-5ee0d033ba5b: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kms@v0.35.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tk8s.io/kube-aggregator@v0.35.1: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/randfill@v1.0.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tsigs.k8s.io/structured-merge-diff/v6@v6.3.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" 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 |
|
[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 |
c0144b5 to
666ef2c
Compare
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.
Actionable comments posted: 2
🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml`:
- Around line 50-54: Convert the container args flow sequence around the
existing "-secret-name" and OSMaxAllowedAddressPairs template entries into a
YAML block sequence using list-item syntax. Preserve the current argument values
and conditional inclusion controlled by OSMaxAllowedAddressPairsIsSet and
OSMaxAllowedAddressPairs.
In `@pkg/network/bootstrap.go`:
- Around line 76-107: Update cloudNetworkConfigBootstrap to return the API error
alongside CloudNetworkConfigBootstrapResult, propagating transient errors from
cl.Get instead of logging and returning an empty result. Adjust Bootstrap and
its caller handling to receive the error and return it so reconciliation
requeues with backoff; preserve the existing not-found and successful
configuration behavior.
🪄 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: 10ffe051-5f03-4806-86dd-a122e8f1e1f3
📒 Files selected for processing (7)
bindata/cloud-network-config-controller/managed/controller.yamlbindata/cloud-network-config-controller/self-hosted/controller.yamlpkg/bootstrap/types.gopkg/network/bootstrap.gopkg/network/bootstrap_test.gopkg/network/cloud_network.gopkg/network/cloud_network_test.go
62b0134 to
1a3f781
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. |
1a3f781 to
e5abd93
Compare
openshift-ci-robot
removed
the
verified
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bindata/cloud-network-config-controller/self-hosted/controller.yaml (1)
45-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnresolved:
argsflow sequence + Go template conditional breaks YAML lint.This is the same pattern flagged in an earlier review round (YAMLlint:
expected ',' or ']', but got '{'at this location) — the fix (converting to a block sequence) was not applied. The raw template still mixes a flow sequenceargs: [...]with an injected{{- if ... }}/{{- end }}block, which a YAML parser cannot parse as valid flow-sequence syntax on the raw (pre-render) template.🛠️ Proposed fix to use block sequence
- args: [ "-platform-type", "{{.PlatformType}}", - "-platform-region={{.PlatformRegion}}", - "-platform-api-url={{.PlatformAPIURL}}", - "-platform-aws-ca-override={{.PlatformAWSCAPath}}", - "-platform-azure-environment={{.PlatformAzureEnvironment}}", - "-secret-name", "cloud-credentials" -{{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }} - ,"-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" -{{- end }} - ] + args: + - "-platform-type" + - "{{.PlatformType}}" + - "-platform-region={{.PlatformRegion}}" + - "-platform-api-url={{.PlatformAPIURL}}" + - "-platform-aws-ca-override={{.PlatformAWSCAPath}}" + - "-platform-azure-environment={{.PlatformAzureEnvironment}}" + - "-secret-name" + - "cloud-credentials" +{{- if and .OSMaxAllowedAddressPairsIsSet (gt .OSMaxAllowedAddressPairs 0) }} + - "-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" +{{- end }}🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml` around lines 45 - 54, Convert the args value in the controller manifest from a flow sequence to a block sequence, placing each argument as its own list item. Preserve the conditional OSMaxAllowedAddressPairs argument and its existing condition, while ensuring the raw Go template remains valid YAML before rendering.
🤖 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.
Duplicate comments:
In `@bindata/cloud-network-config-controller/self-hosted/controller.yaml`:
- Around line 45-54: Convert the args value in the controller manifest from a
flow sequence to a block sequence, placing each argument as its own list item.
Preserve the conditional OSMaxAllowedAddressPairs argument and its existing
condition, while ensuring the raw Go template remains valid YAML before
rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d5b997b4-8f82-40b0-b3db-5ad96b111c37
📒 Files selected for processing (7)
bindata/cloud-network-config-controller/managed/controller.yamlbindata/cloud-network-config-controller/self-hosted/controller.yamlpkg/bootstrap/types.gopkg/network/bootstrap.gopkg/network/bootstrap_test.gopkg/network/cloud_network.gopkg/network/cloud_network_test.go
e5abd93 to
8247a2b
Compare
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/verified by @danchild |
|
@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. |
openshift-ci-robot
added
the
verified
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/network/bootstrap_test.go (1)
390-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant test-helper comments.
Both comments repeat what
toPtrand the_test.gofile already state.
pkg/network/bootstrap_test.go#L390-L393: Delete the comment abovetoPtr.pkg/network/cloud_network_test.go#L494-L497: Delete the comment abovetoPtr.As per coding guidelines, new comments must add genuine value.
🤖 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/network/bootstrap_test.go` around lines 390 - 393, Remove the redundant comment above the toPtr test helper in pkg/network/bootstrap_test.go at lines 390-393 and pkg/network/cloud_network_test.go at lines 494-497; leave the toPtr implementations unchanged.Source: Coding guidelines
🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml`:
- Around line 54-56: Fix the Helm template conditional around
OSMaxAllowedAddressPairs by placing the if/end actions inside YAML comments
without trim markers, while keeping the argument entry valid YAML in the
rendered output. Validate both the branch where OSMaxAllowedAddressPairs is set
and the branch where it is absent, using the existing template structure in
controller.yaml.
In `@pkg/network/bootstrap.go`:
- Around line 86-93: Update Bootstrap to accept a context.Context parameter,
thread that context through cloudNetworkConfigBootstrap, and replace
context.TODO() in its cl.Get ConfigMap request with the provided reconciliation
context.
---
Nitpick comments:
In `@pkg/network/bootstrap_test.go`:
- Around line 390-393: Remove the redundant comment above the toPtr test helper
in pkg/network/bootstrap_test.go at lines 390-393 and
pkg/network/cloud_network_test.go at lines 494-497; leave the toPtr
implementations unchanged.
🪄 Autofix
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: Pro Plus
Run ID: 63994a86-248e-45bf-87f7-e1fe3267b375
📒 Files selected for processing (7)
bindata/cloud-network-config-controller/managed/controller.yamlbindata/cloud-network-config-controller/self-hosted/controller.yamlpkg/bootstrap/types.gopkg/network/bootstrap.gopkg/network/bootstrap_test.gopkg/network/cloud_network.gopkg/network/cloud_network_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- bindata/cloud-network-config-controller/managed/controller.yaml
c860305 to
7d46b28
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/verified by @danchild |
|
/pipeline required |
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. |
|
Scheduling required tests: Scheduling tests matching the |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bindata/cloud-network-config-controller/self-hosted/controller.yaml (1)
54-56:⚠️ Potential issue | 🔴 CriticalFix the remaining YAML template syntax error.
Line 54 and Line 56 place Go-template actions outside YAML comments. YAMLlint reports
could not find expected ':'at Line 55. Use YAML comments without trim markers, then render both branches. This is the same unresolved issue reported in the previous review.Proposed fix
-{{- if .OSMaxAllowedAddressPairs }} +# {{ if .OSMaxAllowedAddressPairs }} - "-platform-os-max-allowed-address-pairs={{ .OSMaxAllowedAddressPairs }}" -{{- end }} +# {{ end }}🤖 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 `@bindata/cloud-network-config-controller/self-hosted/controller.yaml` around lines 54 - 56, Fix the conditional block around OSMaxAllowedAddressPairs by placing the Go-template actions inside YAML comments without trim markers, while preserving rendering of both the enabled and disabled branches. Ensure the resulting template remains valid YAML for YAMLlint and continues to emit the platform argument only when .OSMaxAllowedAddressPairs is set.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In `@bindata/cloud-network-config-controller/self-hosted/controller.yaml`:
- Around line 54-56: Fix the conditional block around OSMaxAllowedAddressPairs
by placing the Go-template actions inside YAML comments without trim markers,
while preserving rendering of both the enabled and disabled branches. Ensure the
resulting template remains valid YAML for YAMLlint and continues to emit the
platform argument only when .OSMaxAllowedAddressPairs is set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a3766d38-afb3-4568-964c-e173a4b38ca2
📒 Files selected for processing (7)
bindata/cloud-network-config-controller/managed/controller.yamlbindata/cloud-network-config-controller/self-hosted/controller.yamlpkg/bootstrap/types.gopkg/network/bootstrap.gopkg/network/bootstrap_test.gopkg/network/cloud_network.gopkg/network/cloud_network_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- bindata/cloud-network-config-controller/managed/controller.yaml
- pkg/network/cloud_network.go
- pkg/bootstrap/types.go
- pkg/network/bootstrap_test.go
- pkg/network/cloud_network_test.go
- pkg/network/bootstrap.go
|
/test e2e-aws-ovn-upgrade |
|
/test e2e-gcp-ovn-upgrade |
|
/test 5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade |
Following `openshift/cloud-network-config-controller` openshift#230, a mechanism is required to pass the configured `max_allowed_address_pairs` as a command line argument to the controller. New feature includes: - Add a bootstrap result type for a new `cloud-network-config` ConfigMap, reading it if the platform type is OpenStack - Validate `platform-os-max-allowed-address-pairs`, returning an error if it is negative, explicitly set to 0, or an invalid number (e.g. alphabetic string) - Pass platform-os-max-allowed-address-pairs to the `cloud-network-config-controller` as a command line argument - Check for changes to `cloud-network-config`, restarting the cloud-network-config-controller if there's a diff Signed-off-by: Dan Childers <dchilder@redhat.com>
Exercise managed and self-hosted template rendering for the new max_allowed_address_pairs flag, including zero, negative, non-integer, valid, and unset cases. Also error propogation related to fetching the cloud-network-config ConfigMap is also tested. Signed-off-by: Dan Childers <dchilder@redhat.com>
7d46b28 to
7e57523
Compare
openshift-ci-robot
removed
the
verified
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/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 e2e-metal-ipi-ovn-ipv6 |
|
/test e2e-gcp-ovn-upgrade |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
@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. |
4 participants
Following work in openshift/cloud-network-config-controller/pull/230, a mechanism is required to pass the configured
max_allowed_address_pairsas a command line argument to the controller.New feature includes:
cloud-network-configConfigMap,reading it if the platform type is OpenStack
platform-os-max-allowed-address-pairs, returning an errorif it is negative, explicitly set to 0, or an invalid number (e.g.
alphabetic string)
platform-os-max-allowed-address-pairsto thecloud-network-config-controlleras a command line argumentcloud-network-config, restarting thecloud-network-config-controller if there's a diff
Please note that openshift/cloud-network-config-controller/pull/230 must be accepted and merged before accepting these changes.