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

etc/kayobe/ansible/filter_plugins/filters.py

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

etc/kayobe/ansible/filter_plugins/filters.py

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.

etc/kayobe/ansible/filter_plugins/filters.py

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

etc/kayobe/ansible/filter_plugins/filters.py

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

etc/kayobe/ansible/filter_plugins/filters.py

Alex-Welsh · 2025-10-08T13:33:24Z

etc/kayobe/ansible/filter_plugins/filters.py

+                indiv_host_vars = hostvars[host][var]
+                for key in indiv_host_vars.keys():
+                    if key == "skipped":
+                        return {}


I think return here is just going to instantly exit the function completely and return an empty dictionary. I think you want continue. May have to refactor the loop a bit to get that to work properly though.

Just to be clear, what's the intention with this bit of the loop?

Its a way to handle hosts not having Docker installed (or any other service it might be checking). If the docker ps command is skipped then Ansible saves "skipped" to containers_list. This part of the loop catches that and returns an empty dict as the value for that host

Ah, okay have you tested it? I'm concerned that return is going to exit the entire function and just ignore any future iterations of the loop

Here's a quick example of what I mean:

def testfunc(): for i in range(10): if not i == 3: print(i) else: return i return "complete" output = testfunc() print("Final output") print(output)

alex@alex-framework:/tmp$ python3 testo.py 0 1 2 Final output 3

The loop never gets past 3 because it returns too early

Whereas continue will just skip the element:

alex@alex-framework:/tmp$ python3 testo.py 0 1 2 4 5 6 7 8 9 Final output complete

etc/kayobe/ansible/filter_plugins/filters.py

Alex-Welsh

Looking good, just a few minor tweaks

Alex-Welsh · 2025-10-21T13:40:53Z

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

+      ansible.builtin.set_fact:
+        seed_node_is_vm: "{{ ansible_facts.virtualization_role == 'guest' }}"
+
+- name: Get facts


Just to be extra clear

Suggested change

- name: Get facts

- name: Set facts for all hosts

Alex-Welsh · 2025-10-21T13:42:40Z

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

+- name: Get facts
+  hosts: all
+  gather_facts: true
+  tasks:


Needs a pass on spacing between these tasks. Sometimes there's no gap, sometimes there's two lines.
Should be one empty line between each task (no gap between the first task and the tasks: line)

Alex-Welsh · 2025-10-21T13:49:00Z

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

+    - name: Set internet connectivity fact
+      ansible.builtin.set_fact:
+        internet_connectivity: "{{ not successful_ping.failed }}"
+    - 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 }}"


I went looking for ways to deduplicate this and it turns out you can specify multiple facts in one task, see docs

So you can do:

Suggested change

- name: Set internet connectivity fact

ansible.builtin.set_fact:

internet_connectivity: "{{ not successful_ping.failed }}"

- 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: Set facts

ansible.builtin.set_fact:

internet_connectivity: "{{ not successful_ping.failed }}"

distribution: "{{ ansible_facts.distribution }}"

distribution_release: "{{ ansible_facts.distribution_release }}"

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

(note I've also added hugepages_enabled to my example)

Alex-Welsh · 2025-10-21T13:56:13Z

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

+      ansible.builtin.set_fact:
+        hugepages_enabled: "{{ 'hugepages' in ansible_facts.cmdline }}"
+
+- name: Gather Cloud Facts


The name of this play is now a bit similar to the previous "gather facts" plays. I'd rename it to be a bit more clear

Suggested change

- name: Gather Cloud Facts

- name: Write cloud facts file

Alex-Welsh · 2025-10-21T14:00:40Z

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

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


Just by convention, lets keep all vars lowercase

Suggested change

vGPU_enabled: "{{ groups['vgpu'] | length > 0 | bool }}"

vgpu_enabled: "{{ groups['vgpu'] | length > 0 | bool }}"

Alex-Welsh · 2025-10-21T14:02:46Z

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

@@ -0,0 +1,124 @@
+---
+- name: Get facts for control host


nit: Set facts rather than Get facts

Suggested change

- name: Get facts for control host

- name: Set facts for control host

Applies to subsequent plays as well

Alex-Welsh · 2025-10-21T14:03:39Z

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

+      ansible.builtin.command: "uname -r"
+      register: kernel_version
+
+    - name: Check if docker is installed


nit: capitalisation

Suggested change

- name: Check if docker is installed

- name: Check if Docker is installed

Alex-Welsh · 2025-10-21T14:05:54Z

You'll also need to rebase this on stackhpc/2025.1. In a recent change, all the ansible playbooks were moved to separate sub-directories, so there's a conflict where the playbook moved

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

etc/kayobe/ansible/filter_plugins/filters.py Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Sep 18, 2025

View reviewed changes

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

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:33 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:33 — with GitHub Actions Inactive

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:33 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:33 — with GitHub Actions Inactive

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:36 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:36 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:41 — with GitHub Actions Inactive

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

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:41 — with GitHub Actions Inactive

GavinHeff force-pushed the gatherCloudFactsv2 branch 2 times, most recently from 9949f32 to 8b1311e Compare September 25, 2025 14:53

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:55 — with GitHub Actions Inactive

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:55 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:55 — with GitHub Actions Inactive

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:55 — with GitHub Actions Inactive

GavinHeff force-pushed the gatherCloudFactsv2 branch from 8b1311e to 5cb8111 Compare September 25, 2025 14:55

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:58 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:58 — with GitHub Actions Inactive

GavinHeff temporarily deployed to SMS Lab September 25, 2025 14:58 — with GitHub Actions Inactive

GavinHeff temporarily deployed to Leafcloud September 25, 2025 14:58 — with GitHub Actions Inactive

GavinHeff requested a review from Alex-Welsh September 30, 2025 10:50

Alex-Welsh requested changes Oct 8, 2025

View reviewed changes

Add v2 of cloud facts playbook

1bfbb2b

GavinHeff force-pushed the gatherCloudFactsv2 branch from 5cb8111 to 1bfbb2b Compare October 9, 2025 12:41

Alex-Welsh requested changes Oct 21, 2025

View reviewed changes

	vGPU_enabled: "{{ groups['vgpu'] \| length > 0 \| bool }}"
	vgpu_enabled: "{{ groups['vgpu'] \| length > 0 \| bool }}"

	- name: Get facts for control host
	- name: Set facts for control host

	- name: Check if docker is installed
	- name: Check if Docker is installed

Gather cloud factsv2 #1869

Are you sure you want to change the base?

Gather cloud factsv2 #1869

Conversation

GavinHeff commented Sep 8, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Alex-Welsh Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alex-Welsh left a comment

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 Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Alex-Welsh Oct 9, 2025 •

edited

Loading