Gather cloud factsv2 #1869

GavinHeff · 2025-09-08T16:03:24Z

Adds playbook to gather more advanced facts about a multinode cloud

etc/kayobe/ansible/get-cloud-facts-v2.yml

etc/kayobe/stackhpc.yml

Alex-Welsh · 2025-09-12T16:04:18Z

tools/scan-images.sh

It appears to have picked up some stray changes from another PR. You might need another rebase

Alex-Welsh · 2025-09-12T16:05:04Z

etc/kayobe/ansible/filter_plugins/filters.py

+#!usr/bin/env python3
+
+from ansible.errors import AnsibleFilterError
+# from ansible.plugins.filter.core imprt FilterModule


Is this required?

Alex-Welsh · 2025-09-17T14:12:47Z

etc/kayobe/ansible/get-cloud-facts.yml

+          # 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'
          kayobe_managed_switches: "{{ groups['switches'] | length > 0 | bool }}"


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?

Alex-Welsh · 2025-09-17T14:17:51Z

etc/kayobe/ansible/get-cloud-facts.yml

+- 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 }}"


Two things to note here:

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.

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"

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

Alex-Welsh · 2025-09-18T08:49:03Z

etc/kayobe/ansible/get-cloud-facts.yml

+      register: kernel_version
+
+    - name: Get running contianers
+      ansible.builtin.command: "docker ps --format {% raw %}'{{.Names}}'{% endraw %}"


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

Alex-Welsh · 2025-09-18T08:50:12Z

etc/kayobe/ansible/get-cloud-facts.yml

      ansible.builtin.copy:
        content: "{{ cloud_facts | to_nice_json(sort_keys=false) }}"
-        dest: ~/cloud-facts.json
+        dest: ~/cloud-facts_2.json


Lets just keep this as cloud-facts.json

Alex-Welsh · 2025-09-18T08:51:59Z

etc/kayobe/ansible/filter_plugins/filters.py

+            self.group_hostvars_by_host
+        }
+
+    def group_hostvars_by_var(self, hostvars, var, stdout=None):


Could you add docstrings for these filters?

Alex-Welsh · 2025-09-18T08:53:50Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new playbook to gather more detailed facts about the cloud environment, supported by a new custom Ansible filter. While the overall direction is good, the new filter plugin in filters.py contains several issues, including a critical bug that silences errors, leftover debugging comments, and inconsistencies. The get-cloud-facts.yml playbook also has some potential problems, such as a misleading fact name and unsafe variable access that could lead to playbook failures. I have provided specific comments and code suggestions to address these issues and improve the robustness and clarity of the new code.

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

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


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}")

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

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


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]

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/get-cloud-facts.yml

+  tasks:
+    - name: Check if hugepages enabled
+      ansible.builtin.set_fact:
+        hugepages_enabled: "{{ ansible_facts.cmdline }}"


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 }}"

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/get-cloud-facts.yml

-          ansible_control_host_distribution: "{{ ansible_facts.distribution }}"
-          ansible_control_host_distribution_release: "{{ ansible_facts.distribution_release }}"
+          internet_connectivity: "{{ internet_connectivity }}"
+          seed_node_is_vm: "{{ hostvars[groups['seed'][0]]['seed_node_is_vm'] | default('N/A') }}"


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' }}"

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

@@ -0,0 +1,57 @@
+#!usr/bin/env python3


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

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

+#!usr/bin/env python3
+
+from ansible.errors import AnsibleFilterError
+# from ansible.plugins.filter.core imprt FilterModule


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.

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

+                if indiv_host_var not in result.keys():
+                    result[indiv_host_var] = [host]
+                else:
+                    result[indiv_host_var].append(host)


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)

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/filter_plugins/filters.py

+    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


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.

gemini-code-assist · 2025-09-18T08:55:35Z

etc/kayobe/ansible/get-cloud-facts.yml

+          # 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'


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

gemini-code-assist · 2025-09-18T08:55:36Z

etc/kayobe/ansible/get-cloud-facts.yml

      ansible.builtin.copy:
        content: "{{ cloud_facts | to_nice_json(sort_keys=false) }}"
-        dest: ~/cloud-facts.json
+        dest: ~/cloud-facts_2.json


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

Alex-Welsh reviewed Sep 9, 2025

View reviewed changes

etc/kayobe/ansible/get-cloud-facts-v2.yml Outdated Show resolved Hide resolved

Alex-Welsh reviewed Sep 9, 2025

View reviewed changes

etc/kayobe/stackhpc.yml Outdated Show resolved Hide resolved

GavinHeff force-pushed the gatherCloudFactsv2 branch 4 times, most recently from 97bc20e to d199d9a Compare September 12, 2025 10:39

GavinHeff temporarily deployed to SMS Lab September 12, 2025 10:41 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 12, 2025 10:41 — with GitHub Actions Inactive

GavinHeff marked this pull request as ready for review September 12, 2025 10:47

GavinHeff requested a review from a team as a code owner September 12, 2025 10:47

Alex-Welsh requested changes Sep 18, 2025

View reviewed changes

Alex-Welsh reviewed Sep 18, 2025

View reviewed changes

gemini-code-assist bot reviewed Sep 18, 2025

View reviewed changes

Add v2 of cloud facts playbook

1c70988

Alex-Welsh force-pushed the gatherCloudFactsv2 branch from d199d9a to 1c70988 Compare September 18, 2025 08:55

Alex-Welsh temporarily deployed to Leafcloud September 18, 2025 08:57 — with GitHub Actions Inactive

Alex-Welsh temporarily deployed to SMS Lab September 18, 2025 08:57 — with GitHub Actions Inactive

Alex-Welsh temporarily deployed to Leafcloud September 18, 2025 08:57 — with GitHub Actions Inactive

		# 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'

Gather cloud factsv2 #1869

Are you sure you want to change the base?

Gather cloud factsv2 #1869

Uh oh!

Conversation

GavinHeff commented Sep 8, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alex-Welsh commented Sep 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!