Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct datadir disk usage reporting in lighthouse.rs #6741

Open
wants to merge 6 commits into
base: stable
Choose a base branch
from

Conversation

crStiv
Copy link

@crStiv crStiv commented Dec 26, 2024

Fix datadir disk usage reporting in lighthouse.rs

Description

This pull request updates lighthouse.rs to fix the disk usage metrics. Previously, the disk usage was calculated for the root directory (/). The new implementation correctly retrieves and uses the LIGHTHOUSE_DATADIR environment variable to calculate disk usage for the appropriate data directory. If the environment variable is not set, it defaults to /.

Fixes #6687

Proposed Changes

  • Modify the disk_usage calculation to use the LIGHTHOUSE_DATADIR environment variable.
  • Add a fallback to the root directory if LIGHTHOUSE_DATADIR is not specified.
  • Update the error handling to provide clearer error messages if disk usage information cannot be retrieved.

Additional Info

  • This change introduces a behavior that depends on the LIGHTHOUSE_DATADIR environment variable. Reviewers should ensure the variable is correctly set in environments where this change is deployed.
  • Future consideration: Test cases could be added to validate behavior when LIGHTHOUSE_DATADIR is missing or set to invalid paths.

Allow edits by maintainers

Yes.


Please ensure contributions adhere to the repository's contributing guidelines and security policy.

Fix disk usage metrics to show datadir usage instead of root
@michaelsproul
Copy link
Member

This should not require the addition of a new environment variable. Lighthouse already knows the data directory from the --datadir CLI flag.

@michaelsproul michaelsproul added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Dec 26, 2024
@crStiv
Copy link
Author

crStiv commented Dec 26, 2024

@michaelsproul this one seems to be better

@michaelsproul
Copy link
Member

That doesn't work @crStiv. Are you writing this PR with AI?

@crStiv
Copy link
Author

crStiv commented Dec 27, 2024

That doesn't work @crStiv. Are you writing this PR with AI?

Made an update, this one should be alright. I used chatgpt to help me create a pull request description

/// Gets the datadir which should be used.
fn get_data_dir() -> PathBuf {
// Try to get datadir from command line arguments
let args: Vec<String> = std::env::args().collect();
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be re-parsing the CLI args like this. They are already parsed by clap and available in the config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author The reviewer has suggested changes and awaits thier implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Remote Monitoring to Report Disk Usage for Datadir Mount Point
2 participants