Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis pull request adds post-deployment external network infrastructure provisioning to the RHOSO Ansible playbook. A new ChangesExternal Network Infrastructure Setup
Sequence DiagramsequenceDiagram
participant Playbook as Ansible Playbook
participant VLANTask as VLAN Interface Task
participant NetworkTask as External Network Task
participant SubnetTask as External Subnet Task
participant OS as OpenStack API
Playbook->>VLANTask: Create VLAN interface with gateway IP
VLANTask->>VLANTask: Assign IP with CIDR prefix
Playbook->>NetworkTask: Create external network
NetworkTask->>OS: openstack network create (VLAN provider)
Playbook->>SubnetTask: Create external subnet
SubnetTask->>OS: openstack subnet create with allocation pool
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ansible/main.yml (1)
34-34: Consider making thepostrole opt-in.Because this role mutates host networking and external OpenStack resources, gating it behind a boolean reduces accidental side effects in environments that don’t want post-deploy external network creation.
Suggested change
- - post + - { role: post, when: external_network_post_deploy | default(false) | bool }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/main.yml` at line 34, Make the "post" role opt-in by gating its inclusion behind a boolean variable: add a variable like post_enabled (default false) in the role defaults and change the playbook include/import of the "post" role to only run when post_enabled is true (e.g., use import_role/include_role with a when: post_enabled | default(false)). Update any documentation/README to mention the new post_enabled toggle and ensure tests or CI invoke the playbook with post_enabled=true when external network mutation is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/post/tasks/main.yml`:
- Line 11: Remove the blanket ignore_errors: true from the three tasks and
instead register each task result and use a targeted failed_when/when check that
only treats “already exists” (or other known benign messages) as non-fatal; for
example, replace ignore_errors: true with register: <task_result> and
failed_when: not (('<benign substring>' in (task_result.stderr|default('') +
task_result.msg|default(''))) or task_result.rc == 0) so genuine errors still
fail the play while idempotent “already exists” outcomes are allowed. Ensure you
update each task that currently sets ignore_errors: true to use its own
registered result variable and a precise failed_when/when clause.
- Line 9: The task is assigning the actual gateway IP (external_net_gateway) to
the host VLAN interface which can conflict with routing; change the ip address
assignment to use a dedicated host-side VLAN IP variable (e.g.
external_net_host_ip or host_vlan_ip) instead of external_net_gateway: update
the ip command that uses external_net_gateway, external_net_cidr and
vlan_interface so it adds external_net_host_ip/{{
external_net_cidr.split('/')[1] }} on {{ vlan_interface }} and ensure the new
variable is defined in inventory/vars where appropriate.
---
Nitpick comments:
In `@ansible/main.yml`:
- Line 34: Make the "post" role opt-in by gating its inclusion behind a boolean
variable: add a variable like post_enabled (default false) in the role defaults
and change the playbook include/import of the "post" role to only run when
post_enabled is true (e.g., use import_role/include_role with a when:
post_enabled | default(false)). Update any documentation/README to mention the
new post_enabled toggle and ensure tests or CI invoke the playbook with
post_enabled=true when external network mutation is intended.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a97608e-3471-4948-80b5-57a7bc4cdfc8
📒 Files selected for processing (3)
ansible/group_vars/all.sample.ymlansible/main.ymlansible/roles/post/tasks/main.yml
| ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }} | ||
| ip link set dev {{ iface_2 }} up | ||
| ip link set dev {{ vlan_interface }} up | ||
| ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }} |
There was a problem hiding this comment.
Line 9 assigns the gateway IP to the host VLAN interface.
This uses external_net_gateway as the interface address, which can conflict with the actual gateway and break routing. Use a dedicated host-side VLAN IP variable instead.
Suggested fix
+- name: Validate external VLAN interface addressing
+ ansible.builtin.assert:
+ that:
+ - external_net_interface_ip is defined
+ - external_net_interface_ip != external_net_gateway
+ fail_msg: "Set external_net_interface_ip to a host IP different from external_net_gateway."
+
- name: Create vlan interface on external interface
vars:
vlan_interface: "{{ iface_2 }}.{{ external_network_vlan_id }}"
shell: |
ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }}
ip link set dev {{ iface_2 }} up
ip link set dev {{ vlan_interface }} up
- ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}
+ ip a a {{ external_net_interface_ip }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/roles/post/tasks/main.yml` at line 9, The task is assigning the
actual gateway IP (external_net_gateway) to the host VLAN interface which can
conflict with routing; change the ip address assignment to use a dedicated
host-side VLAN IP variable (e.g. external_net_host_ip or host_vlan_ip) instead
of external_net_gateway: update the ip command that uses external_net_gateway,
external_net_cidr and vlan_interface so it adds external_net_host_ip/{{
external_net_cidr.split('/')[1] }} on {{ vlan_interface }} and ensure the new
variable is defined in inventory/vars where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ansible/roles/post/tasks/main.yml (2)
21-29: 🛠️ Refactor suggestion | 🟠 MajorSubnet creation task is not idempotent.
Same issue as the network creation task—this will fail if the subnet already exists.
Suggested fix
- name: Create external subnet shell: | oc rsh -n openstack openstackclient openstack subnet create \ --network {{ external_net_name }} \ --subnet-range {{ external_net_cidr }} \ --gateway {{ external_net_gateway }} \ --allocation-pool start={{ external_net_alloc_start }},end={{ external_net_alloc_end }} \ --no-dhcp \ {{ external_net_name }}-subnet + register: subnet_create_result + failed_when: > + subnet_create_result.rc != 0 and + 'already exists' not in (subnet_create_result.stderr | default('') | lower)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/roles/post/tasks/main.yml` around lines 21 - 29, The "Create external subnet" shell task is not idempotent (it blindly runs "oc rsh ... openstack subnet create ...") and will fail if the subnet already exists; replace the shell invocation with the OpenStack Ansible module (openstack.cloud.subnet) and set state: present, name: "{{ external_net_name }}-subnet", network: "{{ external_net_name }}", cidr: "{{ external_net_cidr }}", gateway_ip: "{{ external_net_gateway }}", allocation_pools: [{"start":"{{ external_net_alloc_start }}","end":"{{ external_net_alloc_end }}"}], and enable_dhcp: false (or dhcp: no) so the task is safe to run repeatedly; locate the task by the "Create external subnet" name or the current "oc rsh -n openstack openstackclient openstack subnet create" shell command and replace it with the openstack.cloud.subnet usage to ensure idempotence.
12-19: 🛠️ Refactor suggestion | 🟠 MajorNetwork creation task is not idempotent.
This task will fail if the external network already exists. Register the result and allow "already exists" errors to pass while failing on real errors.
Suggested fix
- name: Create external network shell: | oc rsh -n openstack openstackclient openstack network create \ --external \ --provider-network-type vlan \ --provider-physical-network {{ external_net_physical_network }} \ --provider-segment {{ external_network_vlan_id }} \ {{ external_net_name }} + register: network_create_result + failed_when: > + network_create_result.rc != 0 and + 'already exists' not in (network_create_result.stderr | default('') | lower)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/roles/post/tasks/main.yml` around lines 12 - 19, The "Create external network" shell task is not idempotent; modify that task to register the command result (e.g., create_ext_net_result) and add failed_when/changed_when conditions so the task does not fail if the network "already exists" but does fail on other errors. Specifically, keep the existing shell invocation that uses {{ external_net_physical_network }}, {{ external_network_vlan_id }}, and {{ external_net_name }}, add register: create_ext_net_result, add failed_when: create_ext_net_result.rc != 0 and "'already exists' not in (create_ext_net_result.stderr | default('') + create_ext_net_result.stdout | default(''))", and set changed_when to create_ext_net_result.rc == 0 and "'already exists' not in (create_ext_net_result.stderr | default('') + create_ext_net_result.stdout | default(''))".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/roles/post/tasks/main.yml`:
- Around line 2-10: The VLAN creation task ("Create vlan interface on external
interface") is not idempotent because the shell runs "ip link add"
unconditionally; modify the task to first check whether the VLAN interface
(vlan_interface variable derived from iface_2 and external_network_vlan_id)
already exists and only run the add command if missing, then always run "ip link
set dev {{ iface_2 }} up" and "ip link set dev {{ vlan_interface }} up"; also
replace the IP assignment line to use "ip addr replace" (or the equivalent)
instead of "ip a a" so the address assignment is idempotent. Use the
vlan_interface variable and the same task name to locate and update the logic.
---
Duplicate comments:
In `@ansible/roles/post/tasks/main.yml`:
- Around line 21-29: The "Create external subnet" shell task is not idempotent
(it blindly runs "oc rsh ... openstack subnet create ...") and will fail if the
subnet already exists; replace the shell invocation with the OpenStack Ansible
module (openstack.cloud.subnet) and set state: present, name: "{{
external_net_name }}-subnet", network: "{{ external_net_name }}", cidr: "{{
external_net_cidr }}", gateway_ip: "{{ external_net_gateway }}",
allocation_pools: [{"start":"{{ external_net_alloc_start }}","end":"{{
external_net_alloc_end }}"}], and enable_dhcp: false (or dhcp: no) so the task
is safe to run repeatedly; locate the task by the "Create external subnet" name
or the current "oc rsh -n openstack openstackclient openstack subnet create"
shell command and replace it with the openstack.cloud.subnet usage to ensure
idempotence.
- Around line 12-19: The "Create external network" shell task is not idempotent;
modify that task to register the command result (e.g., create_ext_net_result)
and add failed_when/changed_when conditions so the task does not fail if the
network "already exists" but does fail on other errors. Specifically, keep the
existing shell invocation that uses {{ external_net_physical_network }}, {{
external_network_vlan_id }}, and {{ external_net_name }}, add register:
create_ext_net_result, add failed_when: create_ext_net_result.rc != 0 and
"'already exists' not in (create_ext_net_result.stderr | default('') +
create_ext_net_result.stdout | default(''))", and set changed_when to
create_ext_net_result.rc == 0 and "'already exists' not in
(create_ext_net_result.stderr | default('') + create_ext_net_result.stdout |
default(''))".
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 730aa3f6-d7fe-41b3-a58a-779bff3c71b7
📒 Files selected for processing (3)
ansible/group_vars/all.sample.ymlansible/main.ymlansible/roles/post/tasks/main.yml
✅ Files skipped from review due to trivial changes (2)
- ansible/main.yml
- ansible/group_vars/all.sample.yml
Comment on lines
+2
to
+10
| - name: Create vlan interface on external interface | ||
| vars: | ||
| vlan_interface: "{{ iface_2 }}.{{ external_network_vlan_id }}" | ||
| shell: | | ||
| ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }} | ||
| ip link set dev {{ iface_2 }} up | ||
| ip link set dev {{ vlan_interface }} up | ||
| ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }} | ||
| become: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
VLAN interface creation is not idempotent.
The ip link add command will fail if the interface already exists. Running this role twice will result in failure. Consider checking for interface existence first.
Suggested fix for idempotency
- name: Create vlan interface on external interface
vars:
vlan_interface: "{{ iface_2 }}.{{ external_network_vlan_id }}"
shell: |
+ if ip link show {{ vlan_interface }} >/dev/null 2>&1; then
+ echo "Interface {{ vlan_interface }} already exists, skipping creation"
+ else
+ ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }}
+ fi
- ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }}
ip link set dev {{ iface_2 }} up
ip link set dev {{ vlan_interface }} up
- ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}
+ ip addr replace {{ external_net_interface_ip }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}
become: trueNote: Using ip addr replace instead of ip addr add also improves idempotency for the IP assignment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/roles/post/tasks/main.yml` around lines 2 - 10, The VLAN creation
task ("Create vlan interface on external interface") is not idempotent because
the shell runs "ip link add" unconditionally; modify the task to first check
whether the VLAN interface (vlan_interface variable derived from iface_2 and
external_network_vlan_id) already exists and only run the add command if
missing, then always run "ip link set dev {{ iface_2 }} up" and "ip link set dev
{{ vlan_interface }} up"; also replace the IP assignment line to use "ip addr
replace" (or the equivalent) instead of "ip a a" so the address assignment is
idempotent. Use the vlan_interface variable and the same task name to locate and
update the logic.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ansible/roles/post/tasks/main.yml (2)
12-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake external network/subnet creation re-runnable.
These create commands fail if resources already exist, so repeat runs are brittle. Handle known idempotent outcomes explicitly.
Suggested fix
- name: Create external network shell: | oc rsh -n openstack openstackclient openstack network create \ --external \ --provider-network-type vlan \ --provider-physical-network {{ external_net_physical_network }} \ --provider-segment {{ external_network_vlan_id }} \ {{ external_net_name }} + register: external_network_create + failed_when: > + external_network_create.rc != 0 and + ('already exists' not in (external_network_create.stderr | default('') | lower)) - name: Create external subnet shell: | oc rsh -n openstack openstackclient openstack subnet create \ --network {{ external_net_name }} \ --subnet-range {{ external_net_cidr }} \ --gateway {{ external_net_gateway }} \ --allocation-pool start={{ external_net_alloc_start }},end={{ external_net_alloc_end }} \ --no-dhcp \ {{ external_net_name }}-subnet + register: external_subnet_create + failed_when: > + external_subnet_create.rc != 0 and + ('already exists' not in (external_subnet_create.stderr | default('') | lower))🤖 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 `@ansible/roles/post/tasks/main.yml` around lines 12 - 29, The network and subnet shell tasks ("Create external network" and "Create external subnet") are not idempotent; change them to first run the corresponding openstack show command (openstack network show {{ external_net_name }} and openstack subnet show {{ external_net_name }}-subnet) with failed_when: false and register their outputs, then conditionally run the create commands only when the show returned non-zero/not found; set changed_when: true on the create tasks and ensure the show tasks do not mark failure so repeated runs are safe. Use the existing oc rsh -n openstack openstackclient invocation for both show and create steps and reference the registered variables in the when clauses.
2-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not assign the gateway IP to the host VLAN interface, and make this task idempotent.
Line 9 sets
external_net_gatewayon the host interface, which can conflict with actual gateway routing. Also, Line 6/Line 9 fail on reruns (ip link add,ip a a).Suggested fix
+- name: Validate external VLAN host IP + ansible.builtin.assert: + that: + - external_net_interface_ip is defined + - external_net_interface_ip != external_net_gateway + fail_msg: "Set external_net_interface_ip to a host-side IP different from external_net_gateway." + - name: Create vlan interface on external interface vars: vlan_interface: "{{ iface_2 }}.{{ external_network_vlan_id }}" shell: | - ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }} + if ! ip link show {{ vlan_interface }} >/dev/null 2>&1; then + ip link add link {{ iface_2 }} name {{ vlan_interface }} type vlan id {{ external_network_vlan_id }} + fi ip link set dev {{ iface_2 }} up ip link set dev {{ vlan_interface }} up - ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }} + ip addr replace {{ external_net_interface_ip }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }} become: 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 `@ansible/roles/post/tasks/main.yml` around lines 2 - 10, The task currently force-assigns the gateway IP and runs non-idempotent commands; change it so it does NOT add external_net_gateway to the host VLAN interface and make creation idempotent: compute vlan_interface from iface_2 and external_network_vlan_id (same variable), then only run the ip link add if the interface does not already exist (use an existence test like `ip link show {{ vlan_interface }}` in a shell `if`/`then`), bring the parent and VLAN interfaces up if necessary, and remove the `ip a a {{ external_net_gateway }}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}` line (do not assign the gateway IP). Ensure commands reference vlan_interface, iface_2, external_network_vlan_id, external_net_gateway and external_net_cidr so the task remains clear and idempotent.
🤖 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 `@ansible/roles/post/tasks/main.yml`:
- Around line 12-29: The network and subnet shell tasks ("Create external
network" and "Create external subnet") are not idempotent; change them to first
run the corresponding openstack show command (openstack network show {{
external_net_name }} and openstack subnet show {{ external_net_name }}-subnet)
with failed_when: false and register their outputs, then conditionally run the
create commands only when the show returned non-zero/not found; set
changed_when: true on the create tasks and ensure the show tasks do not mark
failure so repeated runs are safe. Use the existing oc rsh -n openstack
openstackclient invocation for both show and create steps and reference the
registered variables in the when clauses.
- Around line 2-10: The task currently force-assigns the gateway IP and runs
non-idempotent commands; change it so it does NOT add external_net_gateway to
the host VLAN interface and make creation idempotent: compute vlan_interface
from iface_2 and external_network_vlan_id (same variable), then only run the ip
link add if the interface does not already exist (use an existence test like `ip
link show {{ vlan_interface }}` in a shell `if`/`then`), bring the parent and
VLAN interfaces up if necessary, and remove the `ip a a {{ external_net_gateway
}}/{{ external_net_cidr.split('/')[1] }} dev {{ vlan_interface }}` line (do not
assign the gateway IP). Ensure commands reference vlan_interface, iface_2,
external_network_vlan_id, external_net_gateway and external_net_cidr so the task
remains clear and idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73d987cf-ee81-412d-9916-ed4cf79304d4
📒 Files selected for processing (4)
ansible/group_vars/all.sample.ymlansible/main.ymlansible/roles/post/defaults/main.ymlansible/roles/post/tasks/main.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible/main.yml
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit