The holy grail of PRs #1
Conversation
| when: | ||
| - kernel_module.force_build | default(False) or module_type == "missing" | ||
| - name: Build and load the module | ||
| when: insert_kernel_module_data | check_cpu_constraints | bool |
There was a problem hiding this comment.
The logic should be:
- no kernel module is inserted if the cpu constraints are not met.
- thus, this check (check_cpu_constraints) should be done right after checking role parameters and if false, don't do any action inside this role.
There was a problem hiding this comment.
Thank you for the feedback. Should be addressed in 33036e3.
|
|
||
|
|
||
| def get_cpu_info(): | ||
| info = cpuinfo.get_cpu_info() |
There was a problem hiding this comment.
2 points:
- ansible filters are for data manipulation. They are not intended to execute any actions / data collection on hosts
- code of filters are executed centrally on ansible controller, not on target hosts. Hence this will not work
cpu information will be needed to collecto on role level, like:
handle_module: "{{ cpu_constraints | check_cpu_constraints(cpu_family, cpu_model) }}"
There was a problem hiding this comment.
Thank you for the feedback. Should be addressed in 33036e3
| raise ValueError( | ||
| "Constraints needs to be int or list of ranges!" | ||
| ) | ||
| if is_in_range(value, range.get("start", 0), range.get("end", 255)): |
There was a problem hiding this comment.
Do we need to cap the range to 255? It looks like unnecessary conctraint to me.
There was a problem hiding this comment.
If i understood correctly, the model number is one byte (0x00 to 0xFF) - thus the range should be from 0 to 255.
But I can make additional logic that would be used in case the range is open either from left or right side, so we don't need to cap the range.
There was a problem hiding this comment.
Addressed in ff0daa1 . The limits of 0 and 255 were replaced by -inf and inf
| # Sometimes it might be preffered to use the off-tree module instead of the module | ||
| # that is already shipped with the kernel. When a force_build flag is set for | ||
| # a module a build_module.yml will be called instead of loading it. | ||
| # NOTE(r-krcek): This needs testing, I am not sure if it works as-is!! |
There was a problem hiding this comment.
to be tested and this line to be removed then
There was a problem hiding this comment.
Tested by Roman, needs testing by third party
|
|
||
| - fail: | ||
| - name: "Fail on incorrect input : {{ insert_kernel_module_data.name }}" |
There was a problem hiding this comment.
What happens when insert_kernel_module_data.name is not defined here? This is what is being checked in this task, right? Won't it fail earlier on the task name then?
There was a problem hiding this comment.
It would not work, it would fail before displaying the correct text. Fixed in bb75c55
| # Detect what type of kernel module it is - builtin, loadable, or missing | ||
| - import_tasks: detect_module.yml | ||
| - name: "Import CPU info tasks" | ||
| ansible.builtin.import_tasks: cpu_info.yml |
There was a problem hiding this comment.
I'm wondering whether it is worth it to separate this into an included file. Drawbacks: one need to take a look to a different file just for few tasks, and it's not clear from where insert_kernel_module_cpu_family and insert_kernel_module_cpu_model come from.
There was a problem hiding this comment.
The tasks were moved to main in 9202f63
| @@ -0,0 +1,81 @@ | |||
| #!/usr/bin/python3 | |||
There was a problem hiding this comment.
filename: insert_kernel_modules.py is misleading. Better cpu_constraints.py?
| @@ -1,50 +1,45 @@ | |||
| --- | |||
There was a problem hiding this comment.
can we do it in more generic way?
- deliver list of needed boot options as an input to the role (configured in group_vars/all.yml)
- the role will assure that the options are inserted
| success_msg: "BIOS CPPC setting is correct." | ||
| - name: Add or modify amd_pstate parameter in GRUB | ||
| ansible.builtin.lineinfile: | ||
| path: /etc/default/grub |
There was a problem hiding this comment.
we need to make sure that it is valid path for all supported linux distributions.
Also, set this path as a constant in role's vars. It is used multiple times in main.yml
| - cpuinfo_cppc_output.stdout | length > 0 | ||
| fail_msg: "BIOS CPPC setting is not correct. Enable CPPC in BIOS." | ||
| success_msg: "BIOS CPPC setting is correct." | ||
| - name: Add or modify amd_pstate parameter in GRUB |
There was a problem hiding this comment.
Before doing this, make sure that the GRUB_CMDLINE_LINUX_DEFAULT is there.
| ansible.builtin.lineinfile: | ||
| path: /etc/default/grub | ||
| regexp: '^{{ grub_cmdline }}=' | ||
| line: '{{ grub_cmdline }}="amd_pstate=passive"' |
There was a problem hiding this comment.
- we need to keep options that was there before
- think also about edge conditions (file does not exist -> create? GRUB_CMDLINE_LINUX_DEFAULT not present -> add it?
| fail_msg: "BIOS Monitor and Mwait setting is not correct. Enable Monitor and Mwait in BIOS." | ||
| success_msg: "BIOS Monitor and Mwait setting is correct." | ||
| - name: Fail if reboot is required | ||
| ansible.builtin.fail: |
There was a problem hiding this comment.
fail is not correct in this case. I'd suggest:
- issue a warning instructing the user to do reboot after playbook finish
- provide an option to proceed with the node reboot as a part of this role at the end (flag in /group_vars.all.yml)
No description provided.