Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #835 by revising the quota management report table in DeleteOldObservations.py. Instead of showing space usage and quotas in separate sections, it presents used space and quota side-by-side on the same line, along with a percentage indicator and divide-by-zero protection. It also adds a new row showing disk usage of the logical partition containing the RMS_data folder.
Changes:
- Refactored
quotaReport()to display usage, quota, and percentage on the same line for each category. - Added divide-by-zero protection for all percentage calculations.
- Added a logical partition information section using
shutil.disk_usage().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rep += (" bz2 files and archived directories : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(usedSpace(archived_dir), config.arch_dir_quota + config.bz2_files_quota, total_arch_pc)) | ||
| rep += ("\n") | ||
| rep += (" captured directories : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(usedSpace(captured_dir), capt_dir_quota, captured_dir_pc)) | ||
| rep += (" log files : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(usedSpace(log_dir),config.log_files_quota, log_files_pc)) | ||
| rep += (" ----------------------\n") | ||
| rep += (" total for RMS_data : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(usedSpace(config.data_dir), config.rms_data_quota, total_rms_data_pc)) |
There was a problem hiding this comment.
Several usedSpace() calls are made twice — once in the percentage calculation block and again in the format string for display. Specifically: usedSpace(archived_dir) (lines 85 and 122), usedSpace(captured_dir) (lines 95 and 124), usedSpace(log_dir) (lines 65 and 125), and usedSpace(config.data_dir) (lines 90 and 127). Since usedSpace() calls usedSpaceNoRecursion() which walks the filesystem, these redundant calls are expensive. Each value should be computed once and stored in a local variable, similar to continuous_capture_used_space.
| else: | ||
| total_arch_pc = 0 | ||
|
|
||
| if config.arch_dir_quota + config.bz2_files_quota != 0: |
There was a problem hiding this comment.
The guard condition on lines 89-92 checks config.arch_dir_quota + config.bz2_files_quota != 0 to protect against division by zero, but the actual division on line 90 divides by config.rms_data_quota, which is a completely different variable. This means:
- If
config.rms_data_quotais zero butconfig.arch_dir_quota + config.bz2_files_quotais non-zero, aZeroDivisionErrorwill be raised. - If
config.arch_dir_quota + config.bz2_files_quotais zero butconfig.rms_data_quotais non-zero, the percentage will be incorrectly set to 0 instead of being calculated.
The condition should be if config.rms_data_quota != 0: to properly guard the division by config.rms_data_quota.
| if config.arch_dir_quota + config.bz2_files_quota != 0: | |
| if config.rms_data_quota != 0: |
| rep += (" Available space on drive : {:7.02f}GB\n".format(availableSpace(config.data_dir) / (1024 ** 3))) | ||
| rep += ("-----------------------------------------------\n") | ||
| rep += (" logical partition information\n") | ||
| rep += (" partition containing RMS_data : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(usage.used / (1024 ** 3), usage.total / (1024 ** 3) , 100 * usage.used / usage.total)) |
There was a problem hiding this comment.
On line 130, usage.total is used as a divisor in 100 * usage.used / usage.total without any zero-division protection. While a disk with total == 0 is an unlikely edge case, all other percentage calculations in this function explicitly guard against division by zero. For consistency and robustness, this calculation should also be protected, e.g., by only computing the percentage if usage.total != 0.
| from RMS.ConfigReader import loadConfigFromDirectory | ||
| from RMS.Logger import LoggingManager, getLogger | ||
| from RMS.Misc import RmsDateTime, UTCFromTimestamp | ||
| from pathlib import Path |
There was a problem hiding this comment.
from pathlib import Path is imported on line 21 but Path is never used anywhere in the file. This is an unused import that was likely added by mistake. It should be removed to keep the imports clean.
| from pathlib import Path |
| rep += (" bz2 files : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(sizeBz2Files(config), config.bz2_files_quota, bz2_files_pc)) | ||
| rep += (" archived directories : {:7.02f}GB {:7.02f}GB {:3.0f}%\n".format(sizeArchivedDirs(config), config.arch_dir_quota, arch_dir_pc)) |
There was a problem hiding this comment.
The functions sizeBz2Files(config) and sizeArchivedDirs(config) are each called twice in the updated code: once during the percentage calculation block (lines 75 and 80) and again within the format string for the report (lines 119 and 120). Both functions iterate over the filesystem, making them potentially expensive. The values should be computed once and stored in local variables (similar to how continuous_capture_used_space is pre-computed), then reused in the format strings.
|
OK. I'll fix this up. |
Issue #835 identifies that the quota management table is not very easy to interpret. This PR modifies the table so that the usage, and the associated quota are all on the same line.
It adds a percentage used indication, with divide by zero protection, and information about the logical partition containing the RMS_data folder.