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. |
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 parsed value and set state
Renderer->>Renderer: validate positive integer
Renderer->>ControllerDeployment: render conditional controller flag
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors)
✅ Passed checks (20 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 |
|
[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
|
@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. |
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>
9a2f65e to
3d62149
Compare
openshift-ci-robot
removed
the
verified
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>
3d62149 to
2040ddf
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. |
danchild
changed the title
danchild
changed the title
danchild
changed the title
|
/pipeline required |
|
Scheduling required tests: Scheduling tests matching the |
|
/verified by @danchild |
|
@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-ipsec e2e-aws-ovn-fdp-qe e2e-ovn-ipsec-step-registry e2e-metal-ipi-ovn-dualstack-bgp e2e-aws-ovn-upgrade-ipsec |
|
/test 5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade |
|
/test e2e-aws-ovn-fdp-qe |
|
/test e2e-metal-ipi-ovn-dualstack-bgp |
|
/test e2e-aws-ovn-fdp-qe |
|
@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. |
| // from the cloud-network-config ConfigMap. | ||
| type OSMaxAllowedAddressPairs struct { | ||
| Value int | ||
| IsSet bool // used to differentiate between a zero value and an explicitly set zero value |
There was a problem hiding this comment.
I think you can use Value as a *int to differentiate between set and unset instead of adding a new field to the struct.
| type OSMaxAllowedAddressPairs struct { | ||
| Value int | ||
| IsSet bool // used to differentiate between a zero value and an explicitly set zero value | ||
| RawValue string // used for error reporting if user provides a non-numeric string |
There was a problem hiding this comment.
Do we need this? We use a typed flag and the user shouldn't be able to pass a non-integer value to this flag.
|
@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
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.