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

Add disk monitoring #232

Closed
wants to merge 10 commits into from

Conversation

perllaghu
Copy link
Contributor

Adds disk monitoring.

I've also done a couple of tweaks - like moving the UI labelling to indicators to their individual View file, and use these labels as the main reference.

(I didn't try and get the react code to take them from the schema file, but maybe a future task)

Copy link

welcome bot commented Mar 15, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@perllaghu
Copy link
Contributor Author

I am unclear as to what is failing the linting... everything passes on my workstation

Copy link
Contributor

@iandesj iandesj left a comment

Choose a reason for hiding this comment

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

I've added a comment for what might be a potential logic error when calculating disk warning state.

limits["disk"] = {"disk": disk_info.total}
if config.disk_warning_threshold != 0:
limits["disk"]["warn"] = (disk_info.total - disk_info.used) < (
disk_info.total * config.cpu_warning_threshold
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be

disk_info.total * config.disk_warning_threshold

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it should!

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, the warning state calculated is never handled. It looks like the current client-side code handles warning state for memory only. I'm playing with this right now to show the disk warning!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More than happy for you to contribute....
Question: does the CPU have a parallel warning state that should be handled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great question! I'm more than happy to contribute to your branch and close PR #233, which I've pulled in your changes to and did the React work for the warnings. I was in a flow state this AM and wasn't sure how active this PR was going to be, so I threw those changes together.

Check it out and let me know what you think!

@iandesj iandesj mentioned this pull request Apr 26, 2024
@perllaghu
Copy link
Contributor Author

This PR has been superseded by #233

@perllaghu perllaghu closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants