utils.disk: Added comprehensive disk cleanup utilities - #6312
utils.disk: Added comprehensive disk cleanup utilities#6312maramsmurthy wants to merge 1 commit into
Conversation
|
Verified these changes by adding it to tests in a CR flow TestSuite TestRun Summary host_io_disk_cleanup_bonnie_bonnie Run Successfully executed host_io_disk_cleanup_rawread_rawread Run Successfully executed host_io_disk_cleanup_tiobench_tiobench Run Successfully executed host_io_disk_cleanup_disktest_disktest Run Successfully executed host_io_disk_cleanup_fiotest_fio Run Successfully executed host_io_disk_cleanup_softwareraid_softwareraid Run Successfully executed host_io_disk_cleanup_ltp_fsstress_ltp_fsstress Run Successfully executed host_io_disk_cleanup_ltp_fs_ltp_fs Run Successfully executed host_io_disk_cleanup_parallel_dd_parallel_dd Run Successfully executed host_io_disk_cleanup_fs_mark_fs_mark Run Successfully executed host_io_disk_cleanup_ioping_ioping Run Successfully executed host_io_disk_cleanup_iozone_iozone Run Successfully executed host_io_disk_cleanup_lvsetup_lvsetup Run Successfully executed No issues observed during cleanup and no breakage to the CR flow. |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive disk management and cleanup utilities in avocado/utils/disk.py, including multipath normalization, dependency resolution, LVM/RAID teardown, and metadata wiping. The review feedback highlights several critical issues, including incorrect kernel name mapping for RAID slaves, dangerous substring matching during device unmounting, ordering bugs in LVM and RAID cleanup sequences, broken retry logic in metadata wiping due to stderr redirection, and potential multi-line parsing issues when retrieving volume groups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6312 +/- ##
==========================================
- Coverage 71.69% 70.47% -1.22%
==========================================
Files 206 207 +1
Lines 23480 24011 +531
==========================================
+ Hits 16834 16922 +88
- Misses 6646 7089 +443 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dcffe4e to
6f8341a
Compare
|
Hi @maramsmurthy is it a good idea to perhaps contribute this directly to https://github.com/avocado-framework/aautils/tree/main/autils? This adds a lot of code that is meant to be migrated to begin with. What do you think? |
Hi @pevogam, given that this change is currently part of our CR flow, we have a dependency on the existing implementation at this point. That said, I’m open to migrating it to aautils in the future if needed. Please let me know as part of the migration plan if you expect any changes in this code, so we can align early and avoid rework later. |
I assume by your projects depending on this implementation you mean that you use a patch or modified package in some way? Because the pull request is not merged in the repo and as such is not deployed with the default packages built here. If this then integrated as a patch or anything modular then perhaps it maybe we adjusted later on to simply patch on top of aautils?
Glad to hear this, I would say at this point the aautils is still fairly unexplored in terms of standardization so I would say bolder changes like this will also be merged less conservatively there (meaning that I don't expect any further changes to be needed unless you could of course provide some unit tests which would be much appreciated). |
Hi @pevogam, Thank you for the detailed feedback and for being open to future migration possibilities. I'd like to proceed with avocado.utils for this PR, as it maintains consistency with the current codebase and avoids introducing dependencies on aautils while it's still in the exploration phase. I completely understand your perspective on standardization. Before we finalize the approach, I'd also like to get @PraveenPenguin opinion on this, especially regarding the long-term direction and whether there are any specific migration considerations we should keep in mind. In the meantime, if you'd like me to make any other improvements to strengthen this implementation, please let me know and I'll be happy to incorporate them. Looking forward to hearing from both of you! |
|
Alright, let's also hear the opinion of @PraveenPenguin on this. Maybe also same for @harvey0100 but if they are too busy and turn around within enough given time we may find another way forward together on this. |
6f8341a to
79117d3
Compare
Hi @maramsmurthy and @pevogam, Thanks for the discussion. Given that, it may be beneficial to:
For now, I'd lean towards proceeding with Thanks again for the constructive discussion. Regards |
Interesting because I would have argued the same but with the exact opposite implications - it is the fact that aautils is so new and unestablished that would make contributions there much easier with much more lenient maintainers and maintainer reviews. Every addition of code into a functionality that is meant to be deprecated or migrated imposes additional technical debt on the project harboring such additions. Would you mind elaborating a bit more on what precisely you are referring regarding "maturity"? Is it that the aautils project has not enough test coverage or not easily deployable or something else?
What does such strong dependency entail? Your CI/CD pipeline is using a patched avocado version with these changes or do you mean something else? Could you elaborate a bit more on this sentence so I can understand your exact technical restrictions.
I would say we want to hold aautils to the same level of API stability as the other projects and most of the migration entails expansionist direction where each successive change is mostly additive.
I would not mind moving on with this at least for an initial pull request, it is just that I don't see much test coverage and it seems to me as a technical debt that will be offloaded to the avocado core maintainers instead of adjusting a hopefully easy enough to patch workflow. |
Hi @pevogam, Thanks for the clarification. Let me expand a bit on my earlier point. I completely agree that we should avoid creating long-term technical debt. My suggestion is mainly to give downstream consumers some time to align and then perform a more coordinated migration to aautils rather than treating avocado.utils as the permanent home for this functionality. Regards |
|
Hi again @PraveenPenguin and thanks for following up on this as well!
This is fully understandable and from what I gather the current PR is not merged, i.e. you are not using this functionality unless you have patched the avocado core code base. Note this PR is just about the current addition and previously used imports can be found in the same place as before, at least for now.
If I understand this right you mean the avocado-misc-tests repo needs adaptation to start using aautils in addition to avocado. Such adaptation has not yet taken place and the current PR contains code that whose use has also not yet taken place in avocado-misc-tests. So what this would imply is that you want to use the functionality of this PR now but not import aautils yet. Is the effort of importing aautils into avocado-misc-tests considered major at this point? I can see that it is already already available on PyPI (https://pypi.org/project/aautils/) but it may not yet be available as a regular RPM. Is this the problem you point out?
Could you point me to any open issue there so I can see the details about this revamp?
Agreed on all points, my main points above are that:
|
Hi @pevogam, 1-Continue evolving disk.py within avocado.utils for the time being. We are already considering broader updates in some of these downstream areas, so my preference is mainly about migration timing and reducing churn rather than avoiding aautils. Regards |
I see, I guess what you mean is that this PR is concerned with the larger direction we are trying to establish. However referring particularly to the current PR here, @maramsmurthy I have pushed a migrating pull request at avocado-framework/aautils#106 so feel free to review it. We may then push a mirror PR there and add a deprecation warning to this one if we are about to merge it.
Exactly, and it will only require more imports and fixes also on the side of avocado-mist-tests. In other words, waiting for this will introduce additional technical debt not just for maintainers here like me but also for maintainers there like you.
Feel free to also review the PR above, as you can see an effort about this already underway.
I just don't see how migrating things now and not accepting any further technical debt on either side makes things worse for avocado-mist-tests. Note for instance that the current disk module can still be freely used by avocado-misc-tests and will not be dropped fast giving you plenty of time to move to the formal migration stage you wish to. The only difference for avocado-misc-tests right now is that it will not be able to use new features until it migrates and from your words above it is clear that it doesn't use new features like the ones introduced by this PR. |
pevogam
left a comment
There was a problem hiding this comment.
There are some original changes above I asked for if possible so let's not hurry to merge this if possible. @PraveenPenguin I left some further comments that so far are unaddressed, let me know what you think.
This enables consistent disk cleanup across all storage validation tests. Functions added: - cleanup_disks(): Main API for disk cleanup operations - normalize_multipath_devices(): Map devices to multipath - build_device_dependencies(): Build cleanup dependency graph - cleanup_raid_arrays(): Remove software RAID arrays - cleanup_lvm_volumes(): Remove LVM logical volumes - cleanup_lvm_groups(): Remove LVM volume groups - cleanup_lvm_physical_volumes(): Remove LVM physical volumes - unmount_filesystems(): Unmount all filesystems on devices - wipe_disk_metadata(): Clear filesystem and RAID metadata - zero_disk_start(): Zero first 100MB of disks - And 8 additional helper functions Code Quality Improvements (addressing review feedback): Exception Handling: - Replaced bare 'except Exception:' with specific exception types (OSError, ValueError, KeyError, TimeoutError) in 4 locations - Added debug logging for caught exceptions to improve troubleshooting Magic Numbers: - Extracted 20 hardcoded values to module-level constants (timeouts, retry counts, buffer sizes) for better maintainability - Updated 15+ locations to use named constants instead of magic numbers Robustness Improvements: - Fixed variable shadowing in normalize_multipath_devices() - Eliminated TOCTOU race conditions in 3 locations - Added proper error handling for filesystem operations - Improved handling of permission errors and missing paths All improvements maintain zero functional impact for normal operation while significantly improving code robustness and maintainability. Testing: - Validated with 13 test suites in avocado-misc-tests - 203 tests PASS (bonnie, rawread, tiobench, disktest, fiotest, softwareraid, ltp_fsstress, ltp_fs, parallel_dd, fs_mark, ioping, lvsetup) - Zero regressions introduced - Code quality improvements verified with no behavior changes Signed-off-by: Maram Srimannarayana Murthy <msmurthy@linux.vnet.ibm.com>
79117d3 to
4b26676
Compare
|
Hi @maramsmurthy,
Could you confirm you have understood the changes I suggest and have pushed them here?
Could you also provide your own review for this pull request and create a follow up mirror PR there? |
|
Hi @maramsmurthy, Once this PR is merged, we can pursue the corresponding changes in aautils through a mirror/follow-on PR and gradually transition users there. Adding the deprecation notice now will provide a clear migration path and avoid any disruption for existing users. |
|
Hi @PraveenPenguin, now that we have the disk.py available in aautils I would prefer not to merge this PR as it is not a functionality anyone uses (not merged thus never available on the main repo) and can be easily introduced in aautils for those of us that really want to use it. We cannot perform changes on migrated code which can lead to various continuity violations which will be a nightmare to untangle. |
This enables consistent disk cleanup across all storage validation tests.
Functions added: