Better system_disk discovery function #43
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughThe pull request modifies the logic used for determining disk configuration in three chart templates. Two templates now remove conditional checks by directly assigning the quoted output of the disk name lookup. The third template introduces a conditional structure: it first attempts to retrieve the system disk via a lookup; if none is found, it defaults to "/dev/sda" and iterates over available disks to find one with a specified identifier before outputting the result. No changes were made to the exported or public entity declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant T as talm.discovered.system_disk_name
participant L as SystemDisk Lookup
participant D as Disks Lookup
T->>L: Call lookup() for system disk
alt system disk found
L-->>T: Return systemDisk (spec.devPath)
T->>T: Output systemDisk.spec.devPath
else system disk not found
T->>T: Set disk = "/dev/sda"
T->>D: Call lookup() for disks
loop For each disk in Disks
D-->>T: Return disk item
alt disk has wwid
T->>T: Set disk = disk.dev_path and break
end
end
T->>T: Output the determined disk value
end
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (2)
charts/talm/templates/_helpers.tpl (2)
2-14: Enhanced system disk discovery with better fallback mechanismThis implementation significantly improves the system disk discovery function by:
- First attempting to use a specifically designated system disk
- Falling back to the first disk with a WWID (World Wide Identifier) if no system disk is found
- Using "/dev/sda" only as a last resort
The approach is more robust for environments with multiple disks and provides better reliability.
Consider adding a comment explaining the fallback strategy for future maintenance. Also, you might want to add logging or annotations when falling back to default options to help with troubleshooting:
{{- define "talm.discovered.system_disk_name" }} +{{- /* First try to use designated system disk, then disk with WWID, finally default to /dev/sda */ -}} {{- $systemDisk := (lookup "systemdisk" "" "system-disk") }} {{- if $systemDisk }} {{- $systemDisk.spec.devPath }} {{- else }} +{{- /* No system disk found, checking for disks with WWID */ -}} {{- $disk := "/dev/sda" }} {{- range (lookup "disks" "" "").items }} {{- if .spec.wwid }} {{- $disk = .spec.dev_path }} {{- break }} {{- end }} {{- end }} +{{- /* Return selected disk path */ -}} {{- $disk }} {{- end }}
7-11: Consider prioritization criteria for multiple disks with WWIDsThe current implementation selects the first disk with a WWID and breaks the loop. This may not be optimal if multiple disks have WWIDs, as the selection depends on the order in which disks are returned by the lookup function.
Consider adding criteria to select the most appropriate disk when multiple disks have WWIDs, such as preferring SSDs, larger disks, or disks with specific labels:
{{- $disk := "/dev/sda" }} +{{- /* Variables to track the best disk candidate */ -}} +{{- $bestDisk := $disk }} +{{- $bestScore := 0 }} {{- range (lookup "disks" "" "").items }} {{- if .spec.wwid }} -{{- $disk = .spec.dev_path }} -{{- break }} +{{- /* Calculate score based on preferred criteria (example: SSD > HDD, larger sizes preferred) */ -}} +{{- $score := 1 }} +{{- if .spec.rotational | default true | not }}{{- $score = add $score 10 }}{{- end }} +{{- if gt ($bestScore) 0 }} +{{- $bestDisk = .spec.dev_path }} +{{- $bestScore = $score }} +{{- end }} {{- end }} {{- end }} +{{- $disk = $bestDisk }}Note: This is a simplified example. The actual implementation would depend on what disk attributes are available and what prioritization criteria make sense for your environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/presets.gois excluded by!**/generated/**
📒 Files selected for processing (3)
charts/cozystack/templates/_helpers.tpl(1 hunks)charts/generic/templates/_helpers.tpl(1 hunks)charts/talm/templates/_helpers.tpl(1 hunks)
🔇 Additional comments (2)
charts/generic/templates/_helpers.tpl (1)
10-10: Simplification of disk configuration improves code readabilityThe direct use of
include "talm.discovered.system_disk_name"with quoting streamlines the code by removing conditional logic that was previously needed here. This change properly leverages the enhanced system disk discovery implementation in the talm helpers.charts/cozystack/templates/_helpers.tpl (1)
37-37: Improved code maintainability through consistent disk handlingSimilar to the change in generic templates, this simplification removes redundant conditional logic by leveraging the enhanced system disk discovery implementation. The consistent approach across templates reduces code duplication and improves maintainability.
| {{- $disk := "/dev/sda" }} | ||
| {{- range (lookup "disks" "" "").items }} | ||
| {{- if .spec.wwid }} | ||
| {{- $disk = .spec.dev_path }} | ||
| {{- break }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- $disk }} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Error resilience for empty disk results
The current implementation assumes that the lookup for disks will always return a valid result. While the default to "/dev/sda" provides some safety, it might be helpful to add explicit handling for when the lookup returns no items.
Verify the behavior when no disks are found:
🏁 Script executed:
#!/bin/bash
# Check if the lookup function is used elsewhere with empty results
rg -A 5 "lookup \"disks\"" charts/ | grep -B 3 -A 3 "items"
# Check if there's any error handling for empty disk results in other parts of the code
rg -A 5 "if.*\.items" charts/ | grep -B 5 -A 5 "empty"Length of output: 710
Explicit error handling for empty disk lookup results
Currently, the template in charts/talm/templates/_helpers.tpl defaults to "/dev/sda" if no disk with a valid wwid is found. However, our verification confirms that there is no explicit check for an empty lookup result. This could lead to silent failures or unintended behavior if the disks lookup returns an empty set.
- Verify that the lookup returns a non-empty set before proceeding with further logic.
- Consider adding explicit handling (e.g., logging a warning or issuing an error) when no disk items are found.
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit