[ansible/tuning] Enable NUMA for Raspberry Pi 4 and 5 #219
Conversation
WalkthroughThe changes introduce two new tasks in the Ansible playbook for tuning configurations specific to Raspberry Pi devices. The first task, added to Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
ansible/roles/ovos_installer/tasks/tuning/numa.yml (1)
3-4: Consider validating the Pi model more strictlyThe current condition only checks for string presence. A more robust approach would be to use exact model matching to prevent false positives.
- _fake_number: "{{ '4' if 'Raspberry Pi 5' in ovos_installer_raspberrypi else '2' }}" + _fake_number: >- + {{ + '4' if ovos_installer_raspberrypi == 'Raspberry Pi 5' else + '2' if ovos_installer_raspberrypi == 'Raspberry Pi 4' else + ansible.builtin.fail('Unsupported Raspberry Pi model') + }}ansible/roles/ovos_installer/tasks/tuning/main.yml (1)
32-34: Consider standardizing RPi model detectionThe condition for including the NUMA task uses string matching similar to numa.yml. Consider extracting this logic into a variable or fact for consistency and reuse.
+- name: Set Raspberry Pi model facts + ansible.builtin.set_fact: + is_supported_pi: >- + {{ ovos_installer_raspberrypi in ['Raspberry Pi 4', 'Raspberry Pi 5'] }} + - name: Include tuning/numa.yml ansible.builtin.import_tasks: tuning/numa.yml - when: "'Raspberry Pi 4' in ovos_installer_raspberrypi or 'Raspberry Pi 5' in ovos_installer_raspberrypi" + when: is_supported_pi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ansible/roles/ovos_installer/tasks/tuning/main.yml(1 hunks)ansible/roles/ovos_installer/tasks/tuning/numa.yml(1 hunks)
🔇 Additional comments (1)
ansible/roles/ovos_installer/tasks/tuning/numa.yml (1)
10-12: Verify NUMA parameters are appropriate for RPi hardware
The NUMA configuration looks correct:
numa=fake={{ _fake_number }}: Creates virtual NUMA nodes matching CPU coresnuma_policy=interleave: Ensures memory allocation across nodes
✅ Verification successful
NUMA configuration is correctly tailored for RPi hardware
The NUMA configuration is specifically designed for Raspberry Pi 4 & 5:
- For RPi 5:
numa=fake=4(4 NUMA nodes) - For RPi 4:
numa=fake=2(2 NUMA nodes) numa_policy=interleaveis appropriately set for both- The configuration is only applied when the system is explicitly detected as RPi 4 or 5
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if these NUMA parameters are commonly used with RPi
rg -A 2 'numa=fake|numa_policy' --type md # Search docs/discussions
Length of output: 43
Script:
#!/bin/bash
# Search for NUMA configuration patterns in the codebase
rg -A 5 'numa=fake|numa_policy'
# Look for RPi or Raspberry Pi related configuration files
fd -e yml -e yaml | xargs rg -l 'raspberry|rpi' -i
# Check if there are any documentation files about system requirements or hardware
fd 'README|INSTALL|REQUIREMENTS|HARDWARE' -i
Length of output: 2149
Script:
#!/bin/bash
# Check the content of relevant README files and NUMA configuration context
cat ansible/roles/ovos_installer/README.md
cat ansible/roles/ovos_installer/tasks/tuning/numa.yml
cat ansible/roles/ovos_installer/tasks/tuning/main.yml
Length of output: 2120
| ansible.builtin.lineinfile: | ||
| path: "{{ _boot_directory }}/cmdline.txt" | ||
| regexp: "^((?!.*{{ item }}).*console.*)$" | ||
| line: '\1 {{ item }}' | ||
| backrefs: true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety measures for cmdline.txt modification
Consider adding backup and validation steps before modifying this critical boot file.
+ ansible.builtin.copy:
+ src: "{{ _boot_directory }}/cmdline.txt"
+ dest: "{{ _boot_directory }}/cmdline.txt.backup"
+ remote_src: true
+ force: false # Don't overwrite existing backup
+
+ ansible.builtin.stat:
+ path: "{{ _boot_directory }}/cmdline.txt"
+ register: cmdline_stat
+
+ ansible.builtin.fail:
+ msg: "Boot directory or cmdline.txt not found at {{ _boot_directory }}"
+ when: not cmdline_stat.stat.exists
+
ansible.builtin.lineinfile:
path: "{{ _boot_directory }}/cmdline.txt"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ansible.builtin.lineinfile: | |
| path: "{{ _boot_directory }}/cmdline.txt" | |
| regexp: "^((?!.*{{ item }}).*console.*)$" | |
| line: '\1 {{ item }}' | |
| backrefs: true | |
| ansible.builtin.copy: | |
| src: "{{ _boot_directory }}/cmdline.txt" | |
| dest: "{{ _boot_directory }}/cmdline.txt.backup" | |
| remote_src: true | |
| force: false # Don't overwrite existing backup | |
| ansible.builtin.stat: | |
| path: "{{ _boot_directory }}/cmdline.txt" | |
| register: cmdline_stat | |
| ansible.builtin.fail: | |
| msg: "Boot directory or cmdline.txt not found at {{ _boot_directory }}" | |
| when: not cmdline_stat.stat.exists | |
| ansible.builtin.lineinfile: | |
| path: "{{ _boot_directory }}/cmdline.txt" | |
| regexp: "^((?!.*{{ item }}).*console.*)$" | |
| line: '\1 {{ item }}' | |
| backrefs: true |
Summary by CodeRabbit