Skip to content

allow user to define controlplane_network_gateway #694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

allow user to define controlplane_network_gateway #694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ansible/roles/create-inventory/defaults/main/networks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ controlplane_network_interface_idx: 0
controlplane_network_interface: eth0
controlplane_network: 198.18.0.0/16
controlplane_network_prefix: 16
controlplane_network_gateway: "{{ controlplane_network | ansible.utils.nthhost(1) }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately removing this when the file is widely symlinked between many roles will result in potential breakage in at least 2 roles

  • bastion-gogs
  • bastion-disconnected-haproxy

Since those roles have vars that depend on controlplane_network_gateway. You might ask why do these roles "import" by symlink the variables in ansible/roles/create-inventory/defaults/main/networks.yml, well the reason is a long time ago when jetlag was first being developed we attempted to have a single vars file (all.yml) and for it to have every possible var required therefore all vars were global, however the length of all.yml became very large and folks would muck with vars that rarely needed to be adjusted or changed. There was also very few variables that needed to be modified per deployment for a basic deployment. The end result was we could "localize" vars more into each role but this meant repeating many variables across different roles. We left the most needed variables in all.yml and put a "spot" on the bottom of the file to override variables that had been "localized" to the role. The solution we landed on at the time was to make "related" vars files that could be symlinked into roles to reduce repetition of vars and ideally improve maintainability. The symlinked vars files have both pros and cons, as a pro it means we have a single file to modify for a variable's existent, and a con is that when it comes time to modify that variable, it can have a much more significant effect than intended if it is not well understood what consumes this variable when it is symlinked. If we stuck to repeating each variable in each role's default variables file, then we would end up have many spots to potential modify for a new default variable value. As you can see both of these solutions have pros and cons. To this day, no one has taken on the task to attempting to improve the vars maintenance further. This means there can be instances where it seems like the appropriate solution is to remove a variable from a vars file however it will effect many more roles than you may have intended.

That being said, one potential solution here would be to change which var is referenced here:

From gogs_host: "{{ controlplane_network_gateway }}" to gogs_host: "{{ bastion_controlplane_ip }}"

I think that would solve the dependency issue we see here however I do not have the extensive means to test this at this time. Another potential solution would be adding yet another special variable just for your use case of using public_vlan for auto configuration but running a custom controlplane_network_gateway which I would prefer not to add yet another variable just for a single edge case.

Copy link
Contributor Author

@venkataanil venkataanil Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately removing this when the file is widely symlinked between many roles will result in potential breakage in at least 2 roles

  • bastion-gogs
  • bastion-disconnected-haproxy

Since those roles have vars that depend on controlplane_network_gateway. You might ask why do these roles "import" by symlink the variables in ansible/roles/create-inventory/defaults/main/networks.yml, well the reason is a long time ago when jetlag was first being developed we attempted to have a single vars file (all.yml) and for it to have every possible var required therefore all vars were global, however the length of all.yml became very large and folks would muck with vars that rarely needed to be adjusted or changed. There was also very few variables that needed to be modified per deployment for a basic deployment. The end result was we could "localize" vars more into each role but this meant repeating many variables across different roles. We left the most needed variables in all.yml and put a "spot" on the bottom of the file to override variables that had been "localized" to the role. The solution we landed on at the time was to make "related" vars files that could be symlinked into roles to reduce repetition of vars and ideally improve maintainability. The symlinked vars files have both pros and cons, as a pro it means we have a single file to modify for a variable's existent, and a con is that when it comes time to modify that variable, it can have a much more significant effect than intended if it is not well understood what consumes this variable when it is symlinked. If we stuck to repeating each variable in each role's default variables file, then we would end up have many spots to potential modify for a new default variable value. As you can see both of these solutions have pros and cons. To this day, no one has taken on the task to attempting to improve the vars maintenance further. This means there can be instances where it seems like the appropriate solution is to remove a variable from a vars file however it will effect many more roles than you may have intended.

That being said, one potential solution here would be to change which var is referenced here:

From gogs_host: "{{ controlplane_network_gateway }}" to gogs_host: "{{ bastion_controlplane_ip }}"

Agree.

I think that would solve the dependency issue we see here however I do not have the extensive means to test this at this time.

We can wait for this change until someone can verify "gogs" roles. Is it possible to request any folks who use "gogs" role to verify later when they get thechance.

Another potential solution would be adding yet another special variable just for your use case of using public_vlan for auto configuration but running a custom controlplane_network_gateway which I would prefer not to add yet another variable just for a single edge case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can wait for this change until someone can verify "gogs" roles. Is it possible to request any folks who use "gogs" role to verify later when they get thechance.

I'm not sure I understand, you can or can't wait? I suggested a fix after reviewing the code and following the vars, did you want to implement it or keep the patch in limbo?

controlplane_network_api: "{{ controlplane_network | ansible.utils.nthhost(3) }}"
controlplane_network_ingress: "{{ controlplane_network | ansible.utils.nthhost(4) }}"
# Sets "bastion_controlplane_ip" as primary dns
Expand Down
12 changes: 12 additions & 0 deletions ansible/roles/create-inventory/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@
set_fact:
machine_type: "{{ (ocpinventory.json.nodes[0].pm_addr.split('.')[0]).split('-')[-1] }}"

- name: Set Controlplane network gateway
set_fact:
controlplane_network_gateway: "{{ controlplane_network | ansible.utils.nthhost(1) }}"
when:
- controlplane_network_gateway is not defined
- not public_vlan

- name: Public VLAN autoconfiguration
when: public_vlan
block:
Expand Down Expand Up @@ -84,7 +91,12 @@
set_fact:
controlplane_network: "{{ quads_assignment.json.vlan.ip_range }}"
controlplane_network_prefix: "{{ quads_assignment.json.vlan.ip_range | ipaddr('prefix') }}"

- name: Public VLAN - Set Controlplane network gateway
set_fact:
controlplane_network_gateway: "{{ quads_assignment.json.vlan.gateway }}"
when:
- controlplane_network_gateway is not defined

- name: Public VLAN - Set cluster_name if was set as default
set_fact:
Expand Down