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

Give warning if runpath disk space is close to full on ert startup #9193

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

larsevj
Copy link
Contributor

@larsevj larsevj commented Nov 12, 2024

Issue
Resolves #8921

The following is presented if disk is "close" to full:
image

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.79%. Comparing base (b5d3671) to head (da3bd16).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9193      +/-   ##
==========================================
+ Coverage   90.77%   90.79%   +0.02%     
==========================================
  Files         352      352              
  Lines       21935    21958      +23     
==========================================
+ Hits        19911    19937      +26     
+ Misses       2024     2021       -3     
Flag Coverage Δ
cli-tests 39.29% <89.47%> (+0.04%) ⬆️
gui-tests 71.79% <89.47%> (+0.04%) ⬆️
performance-tests 49.41% <89.47%> (+0.04%) ⬆️
unit-tests 79.70% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@larsevj
Copy link
Contributor Author

larsevj commented Nov 12, 2024

initial attempt. Very open for suggestions regarding the text.

@larsevj
Copy link
Contributor Author

larsevj commented Nov 12, 2024

Second alternative:
image

@@ -37,6 +48,9 @@ def str_to_datetime(date_str: str) -> datetime:
DEFAULT_JOBNAME_FORMAT = "<CONFIG_FILE>-<IENS>"
DEFAULT_ECLBASE_FORMAT = "ECLBASE<IENS>"

FULL_DISK_PERCENTAGE_THRESHOLD = 0.97
MINIMUM_BYTES_LEFT_ON_DISK = 200 * 1024**3 # 200 GB required
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, we will get some complaints from all the Raspberry Pi users out there with a 16Gb memory card.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we in some way leave this number for ert-configurations to set?

Copy link
Contributor

Choose a reason for hiding this comment

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

The 0.97 is easier to accept as a general rule in Erts open source code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed the 200 GB should only matter if you are using more than 97 % of your disk, and guards against the case were you have a very large disks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a comment in the source code on this?

Copy link
Contributor

@berland berland left a comment

Choose a reason for hiding this comment

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

👍🏻

@eivindjahren
Copy link
Contributor

Should this check be on startup or on experiment start?

@larsevj
Copy link
Contributor Author

larsevj commented Nov 15, 2024

Should this check be on startup or on experiment start?

Might be better to be on experiment startup, seems like a lot of users ignore warnings on startup. Maybe we also should check space left on enspath?

@eivindjahren
Copy link
Contributor

Should this check be on startup or on experiment start?

Might be better to be on experiment startup, seems like a lot of users ignore warnings on startup. Maybe we also should check space left on enspath?

Yes, we should check there also, we have seen a lot of errors from missing capacity in enspath. Also, for enspath we could estimate how much space is necessary, but I suggest we keep that for another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider displaying the status of free disk spacefor runpath in the Ert gui
4 participants