Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 etc/kayobe/ansible.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ inject_facts_as_vars = False
callbacks_enabled = ansible.posix.profile_tasks
# Silence warning about invalid characters found in group names
force_valid_group_names = ignore
# Default value plus custom filter plugins path
filter_plugins = $ANSIBLE_HOME/plugins/filter:/usr/share/ansible/plugins/filter:$KAYOBE_CONFIG_PATH/ansible/filter_plugins/

[inventory]
# Fail when any inventory source cannot be parsed.
Expand Down
57 changes: 57 additions & 0 deletions etc/kayobe/ansible/filter_plugins/filters.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
#!usr/bin/env python3

Choose a reason for hiding this comment

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

medium

The shebang is missing a leading /. While this file is a filter plugin and not directly executed, it's a best practice to have a correct shebang for consistency and to prevent issues if it's ever executed directly.

Suggested change
#!usr/bin/env python3
#!/usr/bin/env python3


from ansible.errors import AnsibleFilterError
# from ansible.plugins.filter.core imprt FilterModule
Copy link
Member

Choose a reason for hiding this comment

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

Is this required?

Choose a reason for hiding this comment

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

medium

This commented-out line appears to be a leftover from development. It includes a typo (imprt) and should be removed to keep the code clean.


class FilterModule(object):
def filters(self):
return {
'group_hostvars_by_var':
self.group_hostvars_by_var,
'group_hostvars_by_host':
self.group_hostvars_by_host
}

def group_hostvars_by_var(self, hostvars, var, stdout=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add docstrings for these filters?

result = {}

for host in hostvars.keys():
try:
indiv_host_all = hostvars[host]
indiv_host_var = indiv_host_all[var]
if stdout is not None:
indiv_host_var = indiv_host_var[stdout]
if indiv_host_var not in result.keys():
result[indiv_host_var] = [host]
else:
result[indiv_host_var].append(host)
Comment on lines +24 to +27

Choose a reason for hiding this comment

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

medium

The logic for adding hosts to the result dictionary can be simplified using dict.setdefault(). This makes the code more concise and is considered more Pythonic.

                result.setdefault(indiv_host_var, []).append(host)

except KeyError as e:
raise AnsibleFilterError(f"Variable {var} not found in hostvars: {e}")

return result

def group_hostvars_by_host(self, hostvars, var, stdout=None, hosts=None):
result = {}

### Currently returns blank dict, try block doesn't appear to be working

Choose a reason for hiding this comment

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

medium

This comment indicates that the code was not working during development. It should be removed before merging. The underlying issue seems to be in the except block below, which I've commented on separately.


if not hosts:
hosts = hostvars.keys()

for host in hosts:
try:
indiv_host_vars = hostvars[host][var]
if stdout is not None:
indiv_host_vars = indiv_host_vars['stdout_lines']

Choose a reason for hiding this comment

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

high

The stdout parameter is used as a flag to access the hardcoded key 'stdout_lines'. This is inconsistent with group_hostvars_by_var, where stdout is used as the key itself. To improve consistency and flexibility, this function should also use the value of the stdout parameter as the key. You will need to update the call in get-cloud-facts.yml to pass 'stdout_lines' instead of 'stdout'.

                    indiv_host_vars = indiv_host_vars[stdout]

if indiv_host_vars:
result[host] = indiv_host_vars
else:
result[host] = []

except KeyError as e:
AnsibleFilterError(f"Host {host} not found in hostvars.keys(): {e}")

Choose a reason for hiding this comment

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

critical

The AnsibleFilterError is instantiated but not raised, which means any KeyError is silently ignored. This is a critical bug and is likely the cause of the problem mentioned in the comment on line 36. The exception must be raised to signal an error. I've also improved the error message to be more specific.

                raise AnsibleFilterError(f"Variable '{var}' not found for host '{host}': {e}")



return result


98 changes: 63 additions & 35 deletions etc/kayobe/ansible/get-cloud-facts.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,68 @@
---
- name: Can the control host reach the internet?
hosts: localhost
gather_facts: true
tasks:
- name: Check internet connectivity
ansible.builtin.command: "ping stackhpc.com -c 3"
register: internet_connectivity
timeout: 5
ignore_errors: true

- name: Set internet connectivity fact
ansible.builtin.set_fact:
internet_connectivity: "{{ not internet_connectivity.failed }}"
Comment on lines +2 to +14
Copy link
Member

Choose a reason for hiding this comment

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

Two things to note here:

  1. it's probably useful to know this fact for all the hosts, not just localhost, so I'd move it to the "All" section later.
  2. It's not just checking internet connectivity, it's also resolving the DNS entry for stackhpc.com, and it's ICMP (ping) in particular. Could it be rephrased, so it's more specific about what it's doing? i.e. "pinging stackhpc.com"

Copy link
Member

Choose a reason for hiding this comment

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

If you want to extend it further, you could also add a curl command in, to check other protocols, ping an IP address (to sidestep DNS), or ping something that isn't stackhpc.com


- name: Get facts for seed node
hosts: seed
gather_facts: true
tasks:
- name: Is there a dedicated seed node?
ansible.builtin.set_fact:
seed_node_is_vm: "{{ ansible_facts.virtualization_role == 'guest' }}"

- name: Get facts
hosts: all
gather_facts: true
tasks:
- name: Get OS distribution
ansible.builtin.set_fact:
distribution: "{{ ansible_facts.distribution }}"

- name: Get OS distribution release
ansible.builtin.set_fact:
distribution_release: "{{ ansible_facts.distribution_release }}"

- name: Get kernel version
ansible.builtin.command: "uname -r"
register: kernel_version

- name: Get running contianers
ansible.builtin.command: "docker ps --format {% raw %}'{{.Names}}'{% endraw %}"
Copy link
Member

Choose a reason for hiding this comment

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

This could fail if it runs on a host that doesn't have docker installed, so needs to account for that

become: true
register: containers_list

- name: Get facts for computes
hosts: compute
gather_facts: true
tasks:
- name: Check if hugepages enabled
ansible.builtin.set_fact:
hugepages_enabled: "{{ ansible_facts.cmdline }}"

Choose a reason for hiding this comment

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

high

The fact hugepages_enabled is set to the value of ansible_facts.cmdline, which is a dictionary of all kernel command-line parameters. This is misleading, as the name suggests a boolean value. It would be clearer and less error-prone to set this fact to a boolean indicating whether hugepages are actually configured.

        hugepages_enabled: "{{ 'hugepages' in ansible_facts.cmdline }}"


- name: Gather Cloud Facts
hosts: localhost
gather_facts: true
tasks:
- name: Write facts to file
vars:
cloud_facts:
ansible_control_host_distribution: "{{ ansible_facts.distribution }}"
ansible_control_host_distribution_release: "{{ ansible_facts.distribution_release }}"
internet_connectivity: "{{ internet_connectivity }}"

Check warning on line 60 in etc/kayobe/ansible/get-cloud-facts.yml

View workflow job for this annotation

GitHub Actions / Ansible 2.18 lint with Python 3.12

jinja[spacing]

Jinja2 spacing could be improved: {{ hostvars | group_hostvars_by_var('kernel_version','stdout') }} -> {{ hostvars | group_hostvars_by_var('kernel_version', 'stdout') }}

Check warning on line 60 in etc/kayobe/ansible/get-cloud-facts.yml

View workflow job for this annotation

GitHub Actions / Ansible 2.17 lint with Python 3.10

jinja[spacing]

Jinja2 spacing could be improved: {{ hostvars | group_hostvars_by_var('kernel_version','stdout') }} -> {{ hostvars | group_hostvars_by_var('kernel_version', 'stdout') }}
seed_node_is_vm: "{{ hostvars[groups['seed'][0]]['seed_node_is_vm'] | default('N/A') }}"

Choose a reason for hiding this comment

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

high

Accessing groups['seed'][0] directly will cause the playbook to fail if the seed group is empty. It's safer to check for the group's existence before trying to access an element from it.

          seed_node_is_vm: "{{ hostvars[groups['seed'][0]]['seed_node_is_vm'] | default('N/A') if groups['seed'] else 'N/A' }}"

distributions: "{{ hostvars | group_hostvars_by_var('distribution') }}"
dist_releases: "{{ hostvars | group_hostvars_by_var('distribution_release') }}"
kernel_versions: "{{ hostvars | group_hostvars_by_var('kernel_version','stdout') }}"
deployed_containers: "{{ hostvars | group_hostvars_by_host('containers_list', 'stdout') }}"
openstack_release: "{{ openstack_release }}"
openstack_release_name: "{{ openstack_release_codename }}"
ansible_control_host_is_vm: "{{ ansible_facts.virtualization_role == 'guest' }}"
Expand All @@ -22,41 +77,11 @@
ceph_release: "{{ cephadm_ceph_release }}"
storage_hyperconverged: "{{ groups['controllers'] | intersect(groups['osds']) | length > 0 | bool }}"
wazuh_enabled: "{{ groups['wazuh-agent'] | length > 0 | bool }}"
# ngs_enabled_: "{{ 'genericswitch' in (kolla_neutron_ml2_mechanism_drivers | default([])) | bool }}"
# # ABOVE LEADS TO ERROR: argument of type ''NoneType'' is not iterable. argument of type ''NoneType'' is not iterable'
Comment on lines +80 to +81

Choose a reason for hiding this comment

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

medium

These commented-out lines and the associated error message appear to be leftovers from development and should be removed to keep the playbook clean.

kayobe_managed_switches: "{{ groups['switches'] | length > 0 | bool }}"
Comment on lines +80 to 82
Copy link
Member

Choose a reason for hiding this comment

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

I'd guess it's only defined if it's used, so could you add a check to first ensure kolla_neutron_ml2_mechanism_drivers is defined, and if it is, check the values it contains?

proxy_configured: "{{ http_proxy | bool or https_proxy | bool }}"
bifrost_version: "{{ kolla_bifrost_source_version }}"
barbican_enabled: "{{ kolla_enable_barbican }}"
nova_enabled: "{{ kolla_enable_nova }}"
neutron_enabled: "{{ kolla_enable_neutron }}"
ovs_enabled: "{{ kolla_enable_openvswitch }}"
ovn_enabled: "{{ kolla_enable_ovn }}"
glance_enabled: "{{ kolla_enable_glance }}"
cinder_enabled: "{{ kolla_enable_cinder }}"
keystone_enabled: "{{ kolla_enable_keystone }}"
horizon_enabled: "{{ kolla_enable_horizon }}"
fluentd_enabled: "{{ kolla_enable_fluentd }}"
rabbitmq_enabled: "{{ kolla_enable_rabbitmq }}"
mariadb_enabled: "{{ kolla_enable_mariadb }}"
mariabackup_enabled: "{{ kolla_enable_mariabackup }}"
memcached_enabled: "{{ kolla_enable_memcached }}"
haproxy_enabled: "{{ kolla_enable_haproxy }}"
keepalived_enabled: "{{ kolla_enable_keepalived }}"
octavia_enabled: "{{ kolla_enable_octavia }}"
designate_enabled: "{{ kolla_enable_designate }}"
manila_enabled: "{{ kolla_enable_manila }}"
magnum_enabled: "{{ kolla_enable_magnum }}"
heat_enabled: "{{ kolla_enable_heat }}"
ironic_enabled: "{{ kolla_enable_ironic }}"
skyline_enabled: "{{ kolla_enable_skyline }}"
blazar_enabled: "{{ kolla_enable_blazar }}"
pulp_enabled: "{{ seed_pulp_container_enabled }}"
opensearch_enabled: "{{ kolla_enable_opensearch }}"
opensearch_dashboards_enabled: "{{ kolla_enable_opensearch_dashboards }}"
influxdb_enabled: "{{ kolla_enable_influxdb }}"
grafana_enabled: "{{ kolla_enable_grafana }}"
prometheus_enabled: "{{ kolla_enable_prometheus }}"
cloudkitty_enabled: "{{ kolla_enable_cloudkitty }}"
telegraf_enabled: "{{ kolla_enable_telegraf }}"
internal_tls_enabled: "{{ kolla_enable_tls_internal }}"
external_tls_enabled: "{{ kolla_enable_tls_external }}"
firewalld_enabled_all: >-
Expand All @@ -82,6 +107,9 @@
stackhpc_package_repos_enabled: "{{ stackhpc_repos_enabled }}"
pulp_tls_enabled: "{{ pulp_enable_tls }}"
kolla_image_tags: "{{ kolla_image_tags }}"
gpu_passthrough_enabled: "{{ gpu_group_map | length > 0 | bool }}"
vGPU_enabled: "{{ groups['vgpu'] | length > 0 | bool }}"
hugepages_enabled: "{{ hostvars | group_hostvars_by_host('hugepages_enabled', hosts=['compute']) }}"
ansible.builtin.copy:
content: "{{ cloud_facts | to_nice_json(sort_keys=false) }}"
dest: ~/cloud-facts.json
dest: ~/cloud-facts_2.json
Copy link
Member

Choose a reason for hiding this comment

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

Lets just keep this as cloud-facts.json

Choose a reason for hiding this comment

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

medium

The destination filename ~/cloud-facts_2.json seems like a temporary name used for development or testing. Consider changing it back to the original ~/cloud-facts.json or a more suitable permanent name.

        dest: ~/cloud-facts.json

Loading