feat: Decouple disk wiping from LSO installation#825
Conversation
Disk preparation (wiping, partitioning, LVM setup) is now triggered by populating device lists alone, without requiring a separate boolean. LSO installation is controlled independently by the new setup_lso toggle, following the established setup_odf/setup_minio pattern. This enables three install paths: - Just install a cluster (default) - Install + wipe disks (populate device lists, no LSO) - Install + LSO (setup_lso: true, with or without disk wiping) Removes controlplane_localstorage_configuration and worker_localstorage_configuration variables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
akrzos
requested review from
mcornea
and removed request for
jtaleric and
rsevilla87
📝 WalkthroughWalkthroughThis PR refactors the local storage configuration model to separate disk preparation concerns from Local Storage Operator installation. Boolean configuration flags are removed in favor of device-list-based triggers for Ignition-driven disk setup, and a new ChangesLocal Storage Configuration Refactoring: Separate Disk Preparation from LSO Installation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml`:
- Line 218: The when condition uses raw vars and can mis-evaluate string values;
update the condition to coerce operands to booleans by applying the | bool
filter to migrate_monitoring, apply_cluster_monitoring_config and setup_lso so
the expression becomes: (migrate_monitoring | bool) or
((apply_cluster_monitoring_config | bool) and (setup_lso | bool)); this ensures
CLI-passed strings like "false" are treated correctly.
In `@ansible/roles/sno-post-cluster-install/tasks/main.yml`:
- Line 165: The when condition uses string-truthiness for
apply_cluster_monitoring_config and setup_lso; change it to use explicit boolean
coercion by adding the Jinja bool filter so the condition reads like: when:
apply_cluster_monitoring_config | bool and setup_lso | bool; this matches
adjacent checks (e.g., sno_metal3 | bool, setup_minio | bool) and prevents
extra-var string "false" from evaluating as true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 772ed91e-493a-476a-b3e0-1336288f254a
📒 Files selected for processing (10)
ansible/roles/configure-local-storage/defaults/main.ymlansible/roles/configure-local-storage/tasks/main.ymlansible/roles/configure-local-storage/templates/controlplane.ign.j2ansible/roles/mno-post-cluster-install/defaults/main/main.ymlansible/roles/mno-post-cluster-install/tasks/main.ymlansible/roles/mno-post-cluster-install/templates/localvolume-lvm.yml.j2ansible/roles/sno-post-cluster-install/defaults/main/main.ymlansible/roles/sno-post-cluster-install/tasks/main.ymldocs/local-storage.mddocs/odf.md
|
|
||
| - name: Apply OpenShift-Monitoring configuration options | ||
| when: migrate_monitoring or (apply_cluster_monitoring_config and (controlplane_localstorage_configuration or worker_localstorage_configuration)) | ||
| when: migrate_monitoring or (apply_cluster_monitoring_config and setup_lso) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find how these vars are passed in docs/scripts/inventory.
rg -n -C2 'setup_lso\s*='
rg -n -C2 'apply_cluster_monitoring_config\s*='
rg -n -C2 -- '--extra-vars|-e'Repository: redhat-performance/jetlag
Length of output: 50383
🏁 Script executed:
# Read the specific file and surrounding context
head -n 230 ansible/roles/mno-post-cluster-install/tasks/main.yml | tail -n 20Repository: redhat-performance/jetlag
Length of output: 1320
🏁 Script executed:
# Find where these variables are defined with defaults
fd -t f -e yml -e yaml | xargs rg -l "setup_lso|apply_cluster_monitoring_config" | grep -E "(defaults|vars)" | head -10Repository: redhat-performance/jetlag
Length of output: 193
🏁 Script executed:
# Check the role's defaults
rg -n "setup_lso|apply_cluster_monitoring_config" ansible/roles/mno-post-cluster-install/ --type yamlRepository: redhat-performance/jetlag
Length of output: 583
🏁 Script executed:
# Find variable documentation in tips-and-vars.md
rg -n "setup_lso|apply_cluster_monitoring_config" docs/Repository: redhat-performance/jetlag
Length of output: 1291
Cast monitoring gate operands to booleans.
Line 218 uses raw vars in when; when variables are passed via CLI extra-vars (e.g., -e setup_lso=false), they arrive as strings, and string truthiness will make this task run unexpectedly. Note that line 149 in this same file correctly applies the | bool filter to setup_lso.
Suggested fix
- name: Apply OpenShift-Monitoring configuration options
- when: migrate_monitoring or (apply_cluster_monitoring_config and setup_lso)
+ when: (migrate_monitoring | bool) or ((apply_cluster_monitoring_config | bool) and (setup_lso | bool))📝 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.
| when: migrate_monitoring or (apply_cluster_monitoring_config and setup_lso) | |
| - name: Apply OpenShift-Monitoring configuration options | |
| when: (migrate_monitoring | bool) or ((apply_cluster_monitoring_config | bool) and (setup_lso | bool)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ansible/roles/mno-post-cluster-install/tasks/main.yml` at line 218, The when
condition uses raw vars and can mis-evaluate string values; update the condition
to coerce operands to booleans by applying the | bool filter to
migrate_monitoring, apply_cluster_monitoring_config and setup_lso so the
expression becomes: (migrate_monitoring | bool) or
((apply_cluster_monitoring_config | bool) and (setup_lso | bool)); this ensures
CLI-passed strings like "false" are treated correctly.
|
|
||
| - name: Apply OpenShift-Monitoring configuration options | ||
| when: apply_cluster_monitoring_config and controlplane_localstorage_configuration | ||
| when: apply_cluster_monitoring_config and setup_lso |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification for bool-like extra-vars usage patterns.
rg -n -C2 'setup_lso\s*='
rg -n -C2 'apply_cluster_monitoring_config\s*='
rg -n -C2 -- '--extra-vars|-e'Repository: redhat-performance/jetlag
Length of output: 50383
🏁 Script executed:
# Read the specific file and line in question
wc -l ansible/roles/sno-post-cluster-install/tasks/main.ymlRepository: redhat-performance/jetlag
Length of output: 127
🏁 Script executed:
# Get context around line 165
sed -n '155,175p' ansible/roles/sno-post-cluster-install/tasks/main.ymlRepository: redhat-performance/jetlag
Length of output: 925
🏁 Script executed:
# Find where these variables are defined
rg -l "apply_cluster_monitoring_config|setup_lso" --type ymlRepository: redhat-performance/jetlag
Length of output: 97
🏁 Script executed:
# Check defaults for these variables
fd 'defaults' -t d ansible/roles/sno-post-cluster-install/ -exec find {} -name "*.yml" -o -name "*.yaml" \;Repository: redhat-performance/jetlag
Length of output: 300
🏁 Script executed:
# Check the defaults/main.yml for the sno-post-cluster-install role
find ansible/roles/sno-post-cluster-install -name "defaults" -o -name "vars"Repository: redhat-performance/jetlag
Length of output: 117
🏁 Script executed:
# List all files in the role directory
ls -la ansible/roles/sno-post-cluster-install/Repository: redhat-performance/jetlag
Length of output: 357
🏁 Script executed:
# Check for defaults/main.yml
cat ansible/roles/sno-post-cluster-install/defaults/main.yml 2>/dev/null || echo "No defaults file"Repository: redhat-performance/jetlag
Length of output: 86
🏁 Script executed:
# Search for where these variables are set in the entire codebase
rg "apply_cluster_monitoring_config\s*=" --type yamlRepository: redhat-performance/jetlag
Length of output: 51
🏁 Script executed:
# Check for extra-vars usage patterns in playbooks
rg "extra-vars|ansible-playbook.*-e" ansible/ | head -20Repository: redhat-performance/jetlag
Length of output: 261
🏁 Script executed:
# Check what's inside the defaults directory
ls -la ansible/roles/sno-post-cluster-install/defaults/Repository: redhat-performance/jetlag
Length of output: 231
🏁 Script executed:
# Check the main.yml in defaults
cat ansible/roles/sno-post-cluster-install/defaults/main.ymlRepository: redhat-performance/jetlag
Length of output: 158
🏁 Script executed:
# Search for where apply_cluster_monitoring_config is used across all playbooks
rg "apply_cluster_monitoring_config" ansible/ -B 2 -A 2Repository: redhat-performance/jetlag
Length of output: 2240
🏁 Script executed:
# Check how variables are typically passed in the main playbooks
rg "sno-post-cluster-install" ansible/ | head -5Repository: redhat-performance/jetlag
Length of output: 371
🏁 Script executed:
# Check setup_lso definition
rg "setup_lso\s*:" ansible/roles/sno-post-cluster-install/defaults/ -A 1Repository: redhat-performance/jetlag
Length of output: 421
🏁 Script executed:
# See the full context of the defaults file
cat ansible/roles/sno-post-cluster-install/defaults/main/main.yml | head -100Repository: redhat-performance/jetlag
Length of output: 1506
🏁 Script executed:
# Check if there are other uses of these variables without | bool filter
rg "when:.*setup_lso" ansible/Repository: redhat-performance/jetlag
Length of output: 467
Add explicit boolean coercion to monitoring condition.
Line 165 uses the same string-truthiness pattern as nearby tasks but without the | bool filter. When apply_cluster_monitoring_config or setup_lso are passed via extra-vars (e.g., -e apply_cluster_monitoring_config=false), they arrive as strings, and non-empty strings are truthy in Ansible conditions—causing the monitoring config to apply unintentionally. Other tasks in the same file (sno_metal3 | bool, setup_minio | bool, setup_lso | bool) consistently use the filter for this reason.
Suggested fix
- name: Apply OpenShift-Monitoring configuration options
- when: apply_cluster_monitoring_config and setup_lso
+ when: (apply_cluster_monitoring_config | bool) and (setup_lso | bool)📝 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.
| when: apply_cluster_monitoring_config and setup_lso | |
| - name: Apply OpenShift-Monitoring configuration options | |
| when: (apply_cluster_monitoring_config | bool) and (setup_lso | bool) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ansible/roles/sno-post-cluster-install/tasks/main.yml` at line 165, The when
condition uses string-truthiness for apply_cluster_monitoring_config and
setup_lso; change it to use explicit boolean coercion by adding the Jinja bool
filter so the condition reads like: when: apply_cluster_monitoring_config | bool
and setup_lso | bool; this matches adjacent checks (e.g., sno_metal3 | bool,
setup_minio | bool) and prevents extra-var string "false" from evaluating as
true.
1 participant
Disk preparation (wiping, partitioning, LVM setup) is now triggered by populating device lists alone, without requiring a separate boolean. LSO installation is controlled independently by the new setup_lso toggle, following the established setup_odf/setup_minio pattern.
This enables three install paths:
Removes controlplane_localstorage_configuration and worker_localstorage_configuration variables.
Summary by CodeRabbit
Release Notes
Refactor
setup_lsovariable independently controls Local Storage Operator installationDocumentation