lae / ansible-role-proxmox

IaC for Proxmox VE clusters.
MIT License
496 stars 144 forks source link

Default to IPv6 if host does not have IPv4 #197

Closed junousi closed 2 years ago

junousi commented 2 years ago

Otherwise on host with IPv6-only mgmt/corosync this will fail with error:

{{ ansible_default_ipv4.address }}: 'dict object' has no attribute 'address'

junousi commented 2 years ago

Please note that per precedence rules it is not possible to override this variable by setting pve_cluster_addr0 or _pve_cluster_addr0 in the group/host vars. (Role variables are at better precedence level 15, versus host/group variables at levels 4-10.)

It could maybe be possible to override this with include_vars, but that seems kind of a hack.

I admit this might not be the best approach to introduce/enhance IPv6 support. I don't mind closing this PR, or reiterating if there's better ideas, like relocating the variable from role vars/ to role defaults/. But for now, just needed to push this somewhere so that provisioning isn't broken for certain clusters.

lae commented 2 years ago

Did a bit of sleuthing.

_pve_cluster_addr0 originated in https://github.com/lae/ansible-role-proxmox/commit/8ffa6810e3f2410132bfe5e0c92bb5fb6fba1c13#diff-af68359e5cd55e633c68306dfcbef53158b06722ec89a7988316e010aee23108

and landed in https://github.com/lae/ansible-role-proxmox/releases/tag/v1.6.2

Given the notes, I was supposed to remove them in 1.7.0 but I guess I forgot (it happened over 2 years later lol). So we can just move this into pve_cluster_addr0 in the role defaults and get rid of the _-prefixed variable, and remove the legacy tasks. Would you like to do this?

junousi commented 2 years ago

more complex setups the administrator sets the actual pve_cluster_addr0 to a specific interface's address.

For me the big question is how the administrator should do this as long as the variable is sat in vars/; e.g. all my attempts such as:

$ cat inventory/host_vars/clusternode1.yml
---
_pve_cluster_addr0: 2001:xyz:xyz::xyz
pve_cluster_addr0: 2001:xyz:xyz::xyz
$ cat playbook.yml
...
  roles:
    - role: ansible-role-proxmox
      vars:
        _pve_cluster_addr0: 2001:xyz:xyz::xyz
        pve_cluster_addr0: 2001:xyz:xyz::xyz

etc. have ended up generating the {{ ansible_default_ipv4.address }}: 'dict object' has no attribute 'address' error on a system without IPv4. So seems Ansible really really wants to evaluate this variable early on regardless if it uses it or not?

junousi commented 2 years ago

So we can just move this into pve_cluster_addr0 in the role defaults and get rid of the _-prefixed variable, and remove the legacy tasks. Would you like to do this?

I don't mind, but if it requires reading All Of The Code and thinking of various edge cases, I probably won't have possibility today time-wise. Could do tomorrow, next week at the very latest. Definitely don't mind closing this PR if you can accommodate the removal from role vars to role defaults on your side, with better knowledge of the role internals.

lae commented 2 years ago

if it requires reading All Of The Code and thinking of various edge cases

Nah, it's basically just reverting that particular commit that I linked. _pve_cluster_addr0 was just a temporary addition to support existing users without requiring them to update their role variables immediately (i.e. it was only to let me be able to make a backwards-compatible release and deprecate the old variables later).

junousi commented 2 years ago

Okie, but still, it can't be a 1:1 revert because there's some stretch stuff there which I presume shouldn't be added back. So yes, if you don't mind the wait, I'll reiterate this PR and do some standard testing in IPv4-only + IPv6-only systems, in the timetable mentioned above.

lae commented 2 years ago

I went ahead and made a PR dealing with it and including the change from this, so will close this one.

junousi commented 2 years ago

I went ahead and made a PR dealing with it and including the change from this, so will close this one.

Thank you! Can report this latest change now allowed provisioning both IPv4-only and IPv6-only clusters just fine.