Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

chore(storage-monitor): improve free_space calculation and cli default value #14133

Merged
merged 6 commits into from
May 12, 2023
Merged

chore(storage-monitor): improve free_space calculation and cli default value #14133

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ac8e05f
chore(storage-monitor): fix free_space calculation
yjhmelody May 12, 2023
b4da357
add `Result` type
yjhmelody May 12, 2023
4fe0f04
add docs
yjhmelody May 12, 2023
df253bb
update `polling_period` default value to `6`
yjhmelody May 12, 2023
21f7293
Update client/storage-monitor/src/lib.rs
yjhmelody May 12, 2023
88c5a6e
Apply suggestions from code review
bkchr May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions client/storage-monitor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,19 @@ pub struct StorageMonitorParams {
#[arg(long = "db-storage-threshold", value_name = "MB", default_value_t = 1000)]
pub threshold: u64,

/// How often available space is polled.
/// How often available space is polled in seconds.
yjhmelody marked this conversation as resolved.
Show resolved Hide resolved
#[arg(long = "db-storage-polling-period", value_name = "SECONDS", default_value_t = 5, value_parser = clap::value_parser!(u32).range(1..))]
davxy marked this conversation as resolved.
Show resolved Hide resolved
pub polling_period: u32,
}

/// Storage monitor service: checks the available space for the filesystem for fiven path.
/// Storage monitor service: checks the available space for the filesystem for given path.
pub struct StorageMonitorService {
/// watched path
path: PathBuf,
/// number of megabytes that shall be free on the filesystem for watched path
threshold: u64,
/// storage space polling period (seconds)
polling_period: u32,
/// storage space polling period
polling_period: Duration,
}

impl StorageMonitorService {
Expand Down Expand Up @@ -92,7 +92,7 @@ impl StorageMonitorService {
let storage_monitor_service = StorageMonitorService {
path: path.to_path_buf(),
threshold,
polling_period: parameters.polling_period,
polling_period: Duration::from_secs(parameters.polling_period.into()),
};

spawner.spawn_essential(
Expand All @@ -108,7 +108,7 @@ impl StorageMonitorService {
/// below threshold.
async fn run(self) {
loop {
tokio::time::sleep(Duration::from_secs(self.polling_period.into())).await;
tokio::time::sleep(self.polling_period).await;
if Self::check_free_space(&self.path, self.threshold).is_err() {
break
};
Expand All @@ -117,10 +117,10 @@ impl StorageMonitorService {

/// Returns free space in MB, or error if statvfs failed.
fn free_space(path: &Path) -> Result<u64, Error> {
Ok(fs4::available_space(path).map(|s| s / 1_000_000)?)
Ok(fs4::available_space(path).map(|s| s / 1024 / 1024)?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the returned value was purposely returned in MB and not in MiB.
But maybe @michalkucharczyk knows better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I made this decision basing on https://en.wikipedia.org/wiki/Megabyte being 10^6 not 1024^2.
Feel free to change it if you really feel like we need to use MiB (Mebibyte). But please also adjust clap args documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return the value in MiB then also change the docs (MB -> MiB) please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also: #13466 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think we should use MiB(1024 * 1024 Byte).
This is more accurate for node consumption of storage resources.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, for me. but please update docs/comments wherever needed.

}

/// Checks if the amount of free space for given `path` is above given `threshold`.
/// Checks if the amount of free space for given `path` is above given `threshold` in MB.
/// If it dropped below, error is returned.
/// System errors are silently ignored.
fn check_free_space(path: &Path, threshold: u64) -> Result<(), Error> {
Expand All @@ -139,7 +139,7 @@ impl StorageMonitorService {
}
},
Err(e) => {
log::error!(target: LOG_TARGET, "Could not read available space: {:?}.", e);
log::error!(target: LOG_TARGET, "Could not read available space: {e:?}.");
Err(e)
},
}
Expand Down