MGMT-23733: add PublicIPPool AAP playbooks and MetalLB L2 role - #238
Conversation
|
@akshaynadkarni: This pull request references MGMT-23733 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 "4.22.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. |
|
Skipping CI for Draft Pull Request. |
|
@akshaynadkarni: This pull request references MGMT-23733 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 "4.22.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. |
|
@akshaynadkarni: This pull request references MGMT-23733 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 "4.22.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. |
2b2a275 to
713eb81
Compare
|
@akshaynadkarni: This pull request references MGMT-23733 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 "4.22.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 Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis pull request introduces a complete feature set for managing public IP pools in MetalLB L2 implementations. It adds Ansible Automation Platform controller job templates for create and delete operations, a new MetalLB L2 role with argument specifications and task files for provisioning and deprovisioning MetalLB resources (IPAddressPool and L2Advertisement), corresponding Ansible playbooks that consume event-driven payloads, and event-driven rules that trigger the jobs based on incoming EDA events. The implementation follows a consistent pattern for both create and delete workflows across all layers. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
playbook_osac_create_public_ip_pool.yml (1)
10-12: Consider handling missing annotation gracefully.If the
osac.openshift.io/implementation-strategyannotation is missing from the payload, this will raise aKeyErrorand fail without a clear error message. Consider using| default('')with a validation task, or ensuring the operator always sets this annotation before sending the event.♻️ Proposed fix with default and validation
implementation_strategy: >- - {{ ansible_eda.event.payload.metadata.annotations - ['osac.openshift.io/implementation-strategy'] }} + {{ ansible_eda.event.payload.metadata.annotations + ['osac.openshift.io/implementation-strategy'] | default('') }} template_parameters: {} pre_tasks: - name: Show EDA Event ansible.builtin.debug: var: ansible_eda.event.payload + + - name: Validate implementation strategy is set + ansible.builtin.fail: + msg: "Missing required annotation 'osac.openshift.io/implementation-strategy'" + when: implementation_strategy | length == 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbook_osac_create_public_ip_pool.yml` around lines 10 - 12, The playbook currently directly accesses ansible_eda.event.payload.metadata.annotations['osac.openshift.io/implementation-strategy'] which will raise a KeyError if the annotation is missing; change the template to use the Jinja2 default filter (e.g., | default('')) when assigning implementation_strategy and add a short validation task that fails with a clear message if implementation_strategy is empty, referencing implementation_strategy and the annotations mapping to locate the change.playbook_osac_delete_public_ip_pool.yml (1)
10-13: Same annotation handling concern as create playbook.Similar to the create playbook, consider handling a missing
osac.openshift.io/implementation-strategyannotation gracefully with a default value and validation task.Additionally,
template_parametersis defined but not used by the delete operation (it's not in the argument specs fordelete_public_ip_pool). Consider removing it for clarity unless it's intended for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbook_osac_delete_public_ip_pool.yml` around lines 10 - 13, Handle the missing annotation by reading osac.openshift.io/implementation-strategy via a defaulted lookup (e.g., use a default value like "none" or "standard" when ansible_eda.event.payload.metadata.annotations[...] is absent) and add a short validation task that fails fast if the resolved implementation_strategy is invalid; update the playbook section that sets implementation_strategy to perform this safe lookup and validation. Also remove the unused template_parameters top-level key (or explicitly document/attach it to delete_public_ip_pool if intended for future use) so that template_parameters is not left defined but unused in conjunction with the delete_public_ip_pool operation.collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/delete_public_ip_pool.yaml (1)
21-34: Consider adding retry logic for transient errors.The create task includes retry logic (60 retries, 10s delay) to handle webhook readiness. While delete operations are generally more reliable, transient network issues or API server hiccups could cause failures. Consider adding minimal retry logic for consistency and robustness, especially in automated pipelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/delete_public_ip_pool.yaml` around lines 21 - 34, The delete task "Delete L2Advertisement" can fail on transient API/network hiccups; update the kubernetes.core.k8s task that deletes the L2Advertisement (name "{{ pool_name }}-l2adv", namespace metallb-system) to add minimal retry logic (e.g., retries: 3 and delay: 10) and keep the existing register (l2adv_delete_result) and failed_when checks (including the "'NotFound' not in (l2adv_delete_result.msg | default(''))") so transient failures are retried but NotFound remains treated as non-fatal.collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/create_public_ip_pool.yaml (2)
52-68: Consider adding retry logic to L2Advertisement creation.The IPAddressPool creation has retry logic for webhook readiness, but L2Advertisement creation doesn't. If the MetalLB webhook applies to both resource types, transient failures could still occur here. Consider adding similar retry logic for consistency.
♻️ Proposed addition of retry logic
register: l2advertisement_result + retries: 60 + delay: 10 + until: l2advertisement_result is successful🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/create_public_ip_pool.yaml` around lines 52 - 68, The L2Advertisement task "Create L2Advertisement" using kubernetes.core.k8s (registered as l2advertisement_result) needs the same transient-failure retry logic used for the IPAddressPool: wrap the kubernetes.core.k8s invocation in an Ansible retry pattern (register the result, add until: condition checking success/changed or absence of API errors, with retries and delay) so the task will retry while the MetalLB webhook is not yet ready; ensure the task name "Create L2Advertisement" and the registered variable l2advertisement_result are preserved for traceability.
32-34: Hardcoded labels could use defaults variable.The labels are hardcoded here, but
defaults/main.yamldefinesdefault_publicippool_labels. Consider using the defaults variable combined with any resource-specific labels for better maintainability:♻️ Proposed refactor to use defaults
labels: - osac.openshift.io/publicippool: "{{ pool_name }}" - osac.io/managed-by: osac-fulfillment + osac.openshift.io/publicippool: "{{ pool_name }}" + {{ default_publicippool_labels | to_nice_yaml | indent(10) }}Or alternatively, update
defaults/main.yamlto include both labels and use it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/create_public_ip_pool.yaml` around lines 32 - 34, Replace the hardcoded labels in the labels block of the create_public_ip_pool task with a merge of the defaults variable and the pool-specific label: use the defaults variable default_publicippool_labels combined with a resource-specific mapping that sets osac.openshift.io/publicippool to the pool_name and ensures osac.io/managed-by is present; update the labels evaluation in the template (labels: ...) to combine default_publicippool_labels with the per-resource labels so defaults are used and can be overridden for this pool.collections/ansible_collections/osac/templates/roles/metallb_l2/defaults/main.yaml (1)
1-4: Unused default variable defined but not referenced in tasks.The
default_publicippool_labelsvariable is defined indefaults/main.yamlbut never used. The create task hardcodes labels inline at lines 32-34 (IPAddressPool) and 62-64 (L2Advertisement) instead of referencing this default. Consider removing the unused default or refactoring the tasks to use it for consistency and DRY compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/osac/templates/roles/metallb_l2/defaults/main.yaml` around lines 1 - 4, The default variable default_publicippool_labels is declared but unused; either remove it or update the MetalLB resource creation tasks to reference it instead of hardcoding labels. Modify the tasks that create the IPAddressPool and L2Advertisement resources to use the default_publicippool_labels variable (e.g., labels: "{{ default_publicippool_labels }}") so both resources reuse the same label map, or delete the default from defaults/main.yaml if you prefer the hardcoded labels approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@collections/ansible_collections/osac/templates/roles/metallb_l2/defaults/main.yaml`:
- Around line 1-4: The default variable default_publicippool_labels is declared
but unused; either remove it or update the MetalLB resource creation tasks to
reference it instead of hardcoding labels. Modify the tasks that create the
IPAddressPool and L2Advertisement resources to use the
default_publicippool_labels variable (e.g., labels: "{{
default_publicippool_labels }}") so both resources reuse the same label map, or
delete the default from defaults/main.yaml if you prefer the hardcoded labels
approach.
In
`@collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/create_public_ip_pool.yaml`:
- Around line 52-68: The L2Advertisement task "Create L2Advertisement" using
kubernetes.core.k8s (registered as l2advertisement_result) needs the same
transient-failure retry logic used for the IPAddressPool: wrap the
kubernetes.core.k8s invocation in an Ansible retry pattern (register the result,
add until: condition checking success/changed or absence of API errors, with
retries and delay) so the task will retry while the MetalLB webhook is not yet
ready; ensure the task name "Create L2Advertisement" and the registered variable
l2advertisement_result are preserved for traceability.
- Around line 32-34: Replace the hardcoded labels in the labels block of the
create_public_ip_pool task with a merge of the defaults variable and the
pool-specific label: use the defaults variable default_publicippool_labels
combined with a resource-specific mapping that sets
osac.openshift.io/publicippool to the pool_name and ensures osac.io/managed-by
is present; update the labels evaluation in the template (labels: ...) to
combine default_publicippool_labels with the per-resource labels so defaults are
used and can be overridden for this pool.
In
`@collections/ansible_collections/osac/templates/roles/metallb_l2/tasks/delete_public_ip_pool.yaml`:
- Around line 21-34: The delete task "Delete L2Advertisement" can fail on
transient API/network hiccups; update the kubernetes.core.k8s task that deletes
the L2Advertisement (name "{{ pool_name }}-l2adv", namespace metallb-system) to
add minimal retry logic (e.g., retries: 3 and delay: 10) and keep the existing
register (l2adv_delete_result) and failed_when checks (including the "'NotFound'
not in (l2adv_delete_result.msg | default(''))") so transient failures are
retried but NotFound remains treated as non-fatal.
In `@playbook_osac_create_public_ip_pool.yml`:
- Around line 10-12: The playbook currently directly accesses
ansible_eda.event.payload.metadata.annotations['osac.openshift.io/implementation-strategy']
which will raise a KeyError if the annotation is missing; change the template to
use the Jinja2 default filter (e.g., | default('')) when assigning
implementation_strategy and add a short validation task that fails with a clear
message if implementation_strategy is empty, referencing implementation_strategy
and the annotations mapping to locate the change.
In `@playbook_osac_delete_public_ip_pool.yml`:
- Around line 10-13: Handle the missing annotation by reading
osac.openshift.io/implementation-strategy via a defaulted lookup (e.g., use a
default value like "none" or "standard" when
ansible_eda.event.payload.metadata.annotations[...] is absent) and add a short
validation task that fails fast if the resolved implementation_strategy is
invalid; update the playbook section that sets implementation_strategy to
perform this safe lookup and validation. Also remove the unused
template_parameters top-level key (or explicitly document/attach it to
delete_public_ip_pool if intended for future use) so that template_parameters is
not left defined but unused in conjunction with the delete_public_ip_pool
operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 465cfed3-6741-4cb2-84e6-4299c36f7038
📒 Files selected for processing (9)
collections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.ymlcollections/ansible_collections/osac/templates/roles/metallb_l2/defaults/main.yamlcollections/ansible_collections/osac/templates/roles/metallb_l2/meta/argument_specs.yamlcollections/ansible_collections/osac/templates/roles/metallb_l2/meta/osac.yamlcollections/ansible_collections/osac/templates/roles/metallb_l2/tasks/create_public_ip_pool.yamlcollections/ansible_collections/osac/templates/roles/metallb_l2/tasks/delete_public_ip_pool.yamlplaybook_osac_create_public_ip_pool.ymlplaybook_osac_delete_public_ip_pool.ymlrulebooks/cluster_fulfillment.yml
|
/hold |
Code Review: PublicIPPool AAP Playbooks & MetalLB L2 Role✅ Overall AssessmentClean Ansible role implementation that integrates well with the existing OSAC AAP patterns. E2E testing on edge22 confirms the provisioning flow works correctly. Strengths:
💡 Suggested Improvement 1: Retry Logic ConsistencyFiles:
Observation: IPAddressPool creation has retry logic for MetalLB webhook readiness (60 retries, 10s delay), but L2Advertisement creation doesn't: - name: Create IPAddressPool
kubernetes.core.k8s:
# ...
register: ipaddresspool_result
retries: 60 # ✅ Has retry logic
delay: 10
until: ipaddresspool_result is successful
- name: Create L2Advertisement
kubernetes.core.k8s:
# ...
register: l2advertisement_result
# ❌ Missing retry logicRationale: Both resources use MetalLB webhooks, so both could encounter transient webhook-not-ready errors during operator installation. Suggested Fix: - name: Create L2Advertisement
kubernetes.core.k8s:
kubeconfig: "{{ remote_cluster_kubeconfig | default(omit) }}"
state: present
definition:
# ... existing definition ...
register: l2advertisement_result
retries: 60 # Add
delay: 10 # Add
until: l2advertisement_result is successful # Add💡 Suggested Improvement 2: Missing Annotation ValidationFiles:
Problem: Both playbooks access the implementation_strategy: >-
{{ ansible_eda.event.payload.metadata.annotations
['osac.openshift.io/implementation-strategy'] }}Impact: If the annotation is missing (e.g., due to operator bug or old CR version), the playbook raises a Suggested Fix: vars:
public_ip_pool: "{{ ansible_eda.event.payload }}"
public_ip_pool_name: "{{ ansible_eda.event.payload.metadata.name }}"
implementation_strategy: >-
{{ ansible_eda.event.payload.metadata.annotations
['osac.openshift.io/implementation-strategy'] | default('') }}
template_parameters: {}
pre_tasks:
- name: Show EDA Event
ansible.builtin.debug:
var: ansible_eda.event.payload
- name: Validate implementation strategy is set
ansible.builtin.fail:
msg: "Missing required annotation 'osac.openshift.io/implementation-strategy'"
when: implementation_strategy | length == 0Note: The operator always sets this annotation (see osac-operator PR #176, lines 66-74), so this is a defensive check for robustness. 💡 Suggested Improvement 3: Unused Defaults VariableFile: Observation: The defaults file defines default_publicippool_labels:
osac.io/managed-by: osac-fulfillmentBut the tasks hardcode labels instead: labels:
osac.openshift.io/publicippool: "{{ pool_name }}"
osac.io/managed-by: osac-fulfillmentOptions:
Either approach is fine—just pick one for consistency. 💡 Suggested Improvement 4: Delete Retry LogicFile: Observation: Delete tasks could benefit from minimal retry logic for transient API/network errors: - name: Delete L2Advertisement
kubernetes.core.k8s:
# ... existing params ...
register: l2adv_delete_result
retries: 3 # Add minimal retry for transient errors
delay: 10 # Add
failed_when:
- l2adv_delete_result.failed | default(false)
- "'NotFound' not in (l2adv_delete_result.msg | default(''))"📋 SummaryAll suggestions are non-blocking enhancements for robustness and consistency. The current implementation works correctly (verified in E2E), but these improvements would make it more resilient to edge cases. Priority:
Recommendation: Consider applying improvements 1 and 2 before merge for better production resilience. Improvements 3 and 4 can be follow-up polish. |
…stration
Create the metallb_l2 Ansible role under osac.templates with tasks for
provisioning IPAddressPool (autoAssign: false) and L2Advertisement on
target clusters via remote kubeconfig. Add create/delete playbooks that
dispatch to the role based on the implementation_strategy annotation,
with a replace('-', '_') filter for Ansible role name compatibility.
Register two new job templates in config-as-code (create-public-ip-pool,
delete-public-ip-pool) using the networking-operations inventory and
instance group. Add corresponding EDA rulebook rules for webhook
endpoint dispatch.
ansible-lint passes on all new and modified files.
Assisted-by: Cursor/Claude
Add retry logic to L2Advertisement creation (matching IPAddressPool) and to both delete tasks for transient API error resilience. Validate the implementation-strategy annotation defensively using .get() with a default and an explicit fail task, preventing unclear KeyError if the annotation is ever missing. Remove unused default_publicippool_labels variable from role defaults (labels are defined inline in task files). Assisted-by: Cursor/Claude
713eb81 to
127bc82
Compare
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
✅ LGTM
Solid MetalLB L2 role implementation with E2E verification on edge22.
CodeRabbit Comments Review
All CodeRabbit comments are nitpicks — none blocking:
- ✅ Unused defaults variable: harmless, can be used later
- ✅ Hardcoded labels: explicit and testable, refactor if needed later
⚠️ Missing retry on L2Advertisement create: low risk (webhook ready after IPAddressPool), but would be good defensive coding- ✅ Missing annotation default: operator always sets it, but defensive coding would add
| default('metallb-l2') - ✅ Unused template_parameters in delete: can be removed for clarity
Highlights
- Role structure correct (
osac.templatesnamespace, argument specs, meta/osac.yaml) - MetalLB resources correct (autoAssign: false, labels, L2Advertisement → IPAddressPool delete order)
- IPAddressPool creation has 60 retries for webhook readiness (good defensive coding)
- EDA integration correct (rulebook rules, job templates use networking-operations inventory)
- E2E tested on edge22: AAP jobs 10186 (create, succeeded), 10188 (delete, succeeded)
- ansible-lint passing (0 failures, 0 warnings)
Integration with osac-operator #176
✅ Correctly aligned: operator sets osac.openshift.io/implementation-strategy annotation → AAP reads it → dispatches to osac.templates.{{ implementation_strategy | replace('-', '_') }}
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, eranco74 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
a5c2950
into
osac-project:main
3 participants
Summary
MGMT-23733: Add AAP playbooks and MetalLB L2 Ansible role for PublicIPPool
provisioning. When the osac-operator triggers an AAP job for a PublicIPPool CR,
these playbooks dispatch to the
metallb_l2role, which creates anIPAddressPool (
autoAssign: false) and L2Advertisement on the target cluster.New files:
playbook_osac_create_public_ip_pool.yml/playbook_osac_delete_public_ip_pool.yml: EDA-dispatched playbooksosac.templates.metallb_l2role: create/delete tasks for MetalLB resourcescreate-public-ip-pool/delete-public-ip-poolendpointsnetworking-operationsinventory/IGKey design decisions:
osac.templates(notosac.service) to match theinclude_roledispatch patternreplace('-', '_')filter onimplementation_strategyannotation since Ansible role names cannot contain hyphensget_remote_cluster_kubeconfig(same ascudn_net) because MetalLB resources must be on the target clusterTesting
ansible-lint
Result:
Passed: 0 failure(s), 0 warning(s) in 8 files processedE2E on edge22 (2026-04-09)
Full end-to-end test on edge22 (SNO, OCP 4.20, AAP Direct provider):
Verified:
autoAssign: falseand correct addresses fromspec.cidrsosac.openshift.io/publicippoolandosac.io/managed-bylabelsFull E2E results document attached to MGMT-23733.
Unit tests
Role-level unit tests for
metallb_l2tracked as follow-up: MGMT-23823 (depends on PR #239 merging first to establish test conventions).Related PRs
Ticket
MGMT-23733
Assisted-by: Cursor/Claude
Summary by CodeRabbit