Skip to content
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

Cleanup a deprecation warning (ipaddr filter) #10518

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Cleanup a deprecation warning (ipaddr filter) #10518

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
2 changes: 2 additions & 0 deletions galaxy.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ authors:
tags:
- infrastructure
repository: https://github.com/kubernetes-sigs/kubespray
dependencies:
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering what we should do about dependencies in general here, we have some check mostly on ansible core that is required to be 2.14.x currently. I'm wondering how we should handle these things in galaxy though. Do you have insight about that? Maybe we could also add ansible-core there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Arthur, long time no see indeed :)
I don't think ansible-core should go there.
From ansible docs on the subject, it looks like it's expected to treat ansible-core specially with the meta/runtime.yml file, and all other collections in the dependencies keys in galaxy.yml.

Note that they encourage collections authors to not depend on other collections, but I don't think kubespay should feel too concerned by that.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so I checked and I think ansible-core would be the same as the collection ansible.builtin. It seems to follow the same version (~ish). Could you add this to dependencies: ansible.builtin: ">= 2.14.0 <2.15.0 (there's a bug in ansible-core 2.15 preventing to run kubespray currently).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way the above documentation is written, it looks to me that ansible-core should be treated specially and not put in dependencies.

We recommend that collections work as standalone, independent units, depending only on ansible-core. However, if your collection must depend on features and functionality from another collection, list the other collection or collections under dependencies in your collection’s galaxy.yml file. Use the meta/runtime.yml file to set the ansible-core version that your collection depends on.

To me that reads like deps check for ansible-core specifically should go in meta/runtime.yml, as it is currently.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm ok but then I'm wondering it is really useful to have a requirement on ansible.utils, shouldn't the minimum version of Ansible implicitly include that already somehow ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently you can install ansible-core standalone, (but installing ansible would indeed have ansible.utils I think)

Copy link
Member

@MrFreezeex MrFreezeex Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okok well then nvm both are fine then, as you want 👍

Copy link
Member

Choose a reason for hiding this comment

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

And thanks for the valuable insights into this 🙏

ansible.utils: '>=2.5.0'
build_ignore:
- .github
- '*.tar.gz'
Expand Down
2 changes: 1 addition & 1 deletion roles/kubernetes/control-plane/tasks/kubeadm-setup.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@

- name: Kubeadm | Check apiserver.crt SANs
vars:
apiserver_ips: "{{ apiserver_sans | map('ipaddr') | reject('equalto', False) | list }}"
apiserver_ips: "{{ apiserver_sans | map('ansible.utils.ipaddr') | reject('equalto', False) | list }}"
apiserver_hosts: "{{ apiserver_sans | difference(apiserver_ips) }}"
when:
- kubeadm_already_run.stat.exists
Expand Down
2 changes: 1 addition & 1 deletion roles/kubernetes/preinstall/tasks/0020-set_facts.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@

- name: Get currently configured nameservers
set_fact:
configured_nameservers: "{{ resolvconf_slurp.content | b64decode | regex_findall('^nameserver\\s*(\\S*)', multiline=True) | ipaddr }}"
configured_nameservers: "{{ resolvconf_slurp.content | b64decode | regex_findall('^nameserver\\s*(\\S*)', multiline=True) | ansible.utils.ipaddr }}"
when: resolvconf_slurp.content is defined

- name: Stop if /etc/resolv.conf not configured nameservers
Expand Down
8 changes: 4 additions & 4 deletions roles/kubernetes/preinstall/tasks/0040-verify-settings.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,28 +166,28 @@
- name: "Check that kube_service_addresses is a network range"
assert:
that:
- kube_service_addresses | ipaddr('net')
- kube_service_addresses | ansible.utils.ipaddr('net')
msg: "kube_service_addresses = '{{ kube_service_addresses }}' is not a valid network range"
run_once: yes

- name: "Check that kube_pods_subnet is a network range"
assert:
that:
- kube_pods_subnet | ipaddr('net')
- kube_pods_subnet | ansible.utils.ipaddr('net')
msg: "kube_pods_subnet = '{{ kube_pods_subnet }}' is not a valid network range"
run_once: yes

- name: "Check that kube_pods_subnet does not collide with kube_service_addresses"
assert:
that:
- kube_pods_subnet | ipaddr(kube_service_addresses) | string == 'None'
- kube_pods_subnet | ansible.utils.ipaddr(kube_service_addresses) | string == 'None'
msg: "kube_pods_subnet cannot be the same network segment as kube_service_addresses"
run_once: yes

- name: "Check that IP range is enough for the nodes"
assert:
that:
- 2 ** (kube_network_node_prefix - kube_pods_subnet | ipaddr('prefix')) >= groups['k8s_cluster'] | length
- 2 ** (kube_network_node_prefix - kube_pods_subnet | ansible.utils.ipaddr('prefix')) >= groups['k8s_cluster'] | length
msg: "Not enough IPs are available for the desired node count."
when: kube_network_plugin != 'calico'
run_once: yes
Expand Down
4 changes: 2 additions & 2 deletions roles/network_plugin/calico/tasks/install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@

- name: Calico | Ensure that calico_pool_cidr is within kube_pods_subnet when defined
assert:
that: "[calico_pool_cidr] | ipaddr(kube_pods_subnet) | length == 1"
that: "[calico_pool_cidr] | ansible.utils.ipaddr(kube_pods_subnet) | length == 1"
msg: "{{ calico_pool_cidr }} is not within or equal to {{ kube_pods_subnet }}"
when:
- inventory_hostname == groups['kube_control_plane'][0]
Expand All @@ -111,7 +111,7 @@

- name: Calico | Ensure that calico_pool_cidr_ipv6 is within kube_pods_subnet_ipv6 when defined
assert:
that: "[calico_pool_cidr_ipv6] | ipaddr(kube_pods_subnet_ipv6) | length == 1"
that: "[calico_pool_cidr_ipv6] | ansible.utils.ipaddr(kube_pods_subnet_ipv6) | length == 1"
msg: "{{ calico_pool_cidr_ipv6 }} is not within or equal to {{ kube_pods_subnet_ipv6 }}"
when:
- inventory_hostname == groups['kube_control_plane'][0]
Expand Down