feat: allow wiping disks when adding to ceph#655
Conversation
7cc45ec to
3815160
Compare
3815160 to
7af77be
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds support for wiping disks before adding them as OSDs to MicroCeph. Users can enable this dangerous operation on a per-machine basis via the manifest configuration using the flag dangerous_i_acknowledge_i_will_lose_data_wipe_disks.
Changes:
- Added a new boolean field
dangerous_i_acknowledge_i_will_lose_data_wipe_disksto the manifest's_HostMicroCephConfigmodel - Extended
ConfigureMicrocephOSDStepandMaasConfigureMicrocephOSDStepto read the wipe flag from the manifest and pass it to the microcephadd-osdaction - Added comprehensive test coverage for the wipe functionality in both step implementations
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sunbeam-python/sunbeam/core/manifest.py | Added dangerous_i_acknowledge_i_will_lose_data_wipe_disks boolean field to _HostMicroCephConfig model with default value False |
| sunbeam-python/sunbeam/steps/microceph.py | Added wipe instance variable and logic to read wipe flag from manifest and pass it to the add-osd action |
| sunbeam-python/sunbeam/provider/maas/steps.py | Extended MaasConfigureMicrocephOSDStep to accept manifest parameter, added _wipe_requested() helper method, and updated run() to pass wipe parameter to add-osd action |
| sunbeam-python/sunbeam/provider/maas/commands.py | Updated instantiation of MaasConfigureMicrocephOSDStep to pass the manifest parameter |
| sunbeam-python/tests/unit/sunbeam/steps/test_microceph.py | Added tests for wipe=True and wipe=False scenarios, updated existing tests to set the wipe attribute |
| sunbeam-python/tests/unit/sunbeam/provider/maas/test_maas.py | Added tests for _wipe_requested() method and wipe flag handling in run(), updated fixture to include manifest parameter, fixed tests to set unit_to_hostname mapping |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
The tests are missing coverage for the prompt() method of ConfigureMicrocephOSDStep, specifically for testing that the wipe flag is correctly read from the manifest. This would have caught the naming inconsistency bug in lines 322-324. Consider adding a test that verifies the prompt() method correctly reads the dangerous_i_acknowledge_i_will_lose_data_wipe_disks field from the manifest and sets self.wipe appropriately.
| def test_prompt_reads_wipe_flag_true_from_manifest(self, cclient, jhelper): | |
| """Ensure prompt() sets wipe=True when manifest flag is True.""" | |
| step = ConfigureMicrocephOSDStep(cclient, "test-0", jhelper, "test-model") | |
| # Simulate manifest containing the dangerous wipe flag set to True. | |
| manifest = { | |
| "dangerous_i_acknowledge_i_will_lose_data_wipe_disks": True, | |
| } | |
| # Set possible manifest attributes used by the implementation. | |
| step.manifest = manifest | |
| step._manifest = manifest | |
| console = Mock() | |
| step.prompt(console) | |
| assert step.wipe is True | |
| def test_prompt_reads_wipe_flag_false_from_manifest(self, cclient, jhelper): | |
| """Ensure prompt() sets wipe=False when manifest flag is False.""" | |
| step = ConfigureMicrocephOSDStep(cclient, "test-0", jhelper, "test-model") | |
| manifest = { | |
| "dangerous_i_acknowledge_i_will_lose_data_wipe_disks": False, | |
| } | |
| step.manifest = manifest | |
| step._manifest = manifest | |
| console = Mock() | |
| step.prompt(console) | |
| assert step.wipe is False |
| # Preseed can have osd_devices as list. If so, change to comma separated str | ||
| osd_devices = microceph_config.get(self.node_name, {}).get("osd_devices") | ||
| wipe_disks = microceph_config.get(self.node_name, {}).get( | ||
| "dangerous-i-acknowledge-i-will-lose-data-wipe-disks", False |
There was a problem hiding this comment.
The field name in the manifest model uses underscores (dangerous_i_acknowledge_i_will_lose_data_wipe_disks) but this code is looking for hyphens (dangerous-i-acknowledge-i-will-lose-data-wipe-disks) in the dumped dictionary. Since the _HostMicroCephConfig model doesn't define an alias for this field, when model_dump(by_alias=True) is called on line 313, the dictionary will use the field name with underscores, not hyphens. This will cause the wipe_disks variable to always be False regardless of the actual manifest value, breaking the wipe functionality. Either add an alias to the manifest field or change the key lookup here to use underscores instead of hyphens.
| "dangerous-i-acknowledge-i-will-lose-data-wipe-disks", False | |
| "dangerous_i_acknowledge_i_will_lose_data_wipe_disks", False |
Add a per machine flag to tell microceph to wipe disks before adding them as OSD. The flag will be: `dangerous_i_acknowledge_i_will_lose_data_wipe_disks` Signed-off-by: Guillaume Boutry <guillaume.boutry@canonical.com>
7af77be to
eb084f6
Compare
Add a per machine flag to tell microceph to wipe disks before adding them as OSD.
The flag will be:
dangerous_i_acknowledge_i_will_lose_data_wipe_disksExample: