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

[DDW-256] No disk space overlay #1157

Conversation

thedanheller
Copy link
Member

@thedanheller thedanheller commented Nov 1, 2018

This PR Implements better handling for no-diskspace error

Low disk space check solution:

  • Daedalus checks for free disk space on startup and every 10 minutes while running
  • if this space is lower than 2 GB - 10% we don't start the node or stop it if it is running, we show the red screen explaining that Daedalus requires at least 2 GB of free space but we strongly recommend having more free space (as recommended by OS manufacturer)
  • while on red screen, we check for free disk space every 10 seconds
  • we hide the red screen and restart node when disk space is more than 2 GB
  • final check intervals are implemented as follows:
Available disk space Check interval
>= 0 && < 2 GB 10 sec
>= 2 && < 4 GB 1 min
>= 4 GB 10 min

Todo:

  • No disk space overlay
  • Container functions and variables
  • IPC communication
  • Detect "no space" error and suppress alert
  • Calculate space required
  • Stop/Start Cardano
  • English copy - @darko-mijic
  • Japanese translation - @darko-mijic
  • New copy with the meeting conclusion changes - @darko-mijic
  • New Japanese translation - @darko-mijic
  • Implement acceptance tests
  • Implement dynamic disk-space-check intervals

Screenshots:

Before

image

After

image


Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (yarn run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn run package / CI builds)
  • There are no flow errors or warnings (yarn run flow:test)
  • There are no lint errors or warnings (yarn run lint)
  • Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running yarn run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn run storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

@nikolaglumac
Copy link
Contributor

@daniloprates looks like you forgot to add prettysize plugin into package.json and yarn.lockfile?
screen shot 2018-11-06 at 10 56 54

@thedanheller thedanheller removed the WIP label Jan 8, 2019
@nikolaglumac
Copy link
Contributor

nikolaglumac commented Jan 11, 2019

We'll need to improve this feature...
The problem we are seeing while running manual tests is that the 10-min pollers we run when there's enough space are not good enough. Given that user starts with exactly 2GB of free disk space and Daedalus has just started syncing it will run out of disk space much sooner than our next disk-space check is run. The problem in this scenario is that things start to fall apart when we reach very low disk space - before the ENOSPC error is thrown... This means Daedalus will crash before we even show the not-enough-disk-space error screen.
In order to avoid this from happening we should implement dynamic disk-space-check polling. Such which would run more frequently as we get closer to the 2GB of free disk space.
The exact intervals are yet to be decided but a logic such as this one should be good enough:

  • if there is more than 100GB of free disk space the check should be run every 10 mins,
  • if there is more than 20 GB and less than 100GB of free disk space the check should run every 5 mins,
  • if there is more than 10 GB and less than 20 GB of free disk space the check should run every 2 mins,
  • if there is more than 5 GB and less than 10 GB of free disk space the check should run every 1 min,
  • if there is more than 2 GB and less than 5 GB of free disk space the check should run every 20 sec.

These intervals should be updated on every disk-space-check run. This means that as free disk space decreases and our checks notice this happening they will start running more frequently.
Also, in case the free disk space increases the checks would run less frequently.

cc @daniloprates @DominikGuzei @darko-mijic @DmitriiGaico

@DominikGuzei
Copy link
Member

DominikGuzei commented Jan 11, 2019

Is checking the disk space such a costly operation or why do we need 10 different intervals?

@nikolaglumac
Copy link
Contributor

@DominikGuzei as I have pointed out in my comment - this was just a suggestion.
Regardless of the disk space operation cost, I would prefer not to run this check too often if there is no reason to do so (aka. if there is a bunch of free space). Of course, we don't need to have 10 different intervals. We could probably get by with having 3...

@DominikGuzei
Copy link
Member

i was just asking because you have proposed a complicated logic for the interval based on free disc space … now i wondered why 😉

@nikolaglumac
Copy link
Contributor

I still think we should have it like that - just simpler ;)

@nikolaglumac nikolaglumac removed the WIP label Jan 14, 2019
@nikolaglumac
Copy link
Contributor

nikolaglumac commented Jan 14, 2019

@DominikGuzei I have added only one more interval: 23de0d7

Here's the new setup:

Available disk space Check interval
>= 0 && < 2 GB 10 sec
>= 2 && < 4 GB 1 min
>= 4 GB 10 min

@DominikGuzei
Copy link
Member

Looks good @nikolaglumac 👍

@DominikGuzei
Copy link
Member

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 14, 2019
1157: [DDW-256] No disk space overlay r=DominikGuzei a=daniloprates

This PR Implements better handling for no-diskspace error

Low disk space check solution:
- Daedalus checks for free disk space on startup and every 10 minutes while running
- if this space is lower than 2 GB - 10% we don't start the node or stop it if it is running, we show the red screen explaining that Daedalus requires at least 2 GB of free space but we strongly recommend having more free space (as recommended by OS manufacturer)
- while on red screen, we check for free disk space every 10 seconds
- we hide the red screen and restart node when disk space is more than 2 GB
- final check intervals are implemented as follows:

| Available disk space | Check interval |
|----------------------|:--------:|
| >= 0 && < 2 GB       |  10 sec  |
| >= 2 && < 4 GB       |   1 min  |
| >= 4 GB              |  10 min  |

## Todo:

- [x] No disk space overlay
- [x] Container functions and variables
- [x] IPC communication
- [x] Detect "no space" error and suppress alert
- [x] Calculate space required
- [x] Stop/Start Cardano
- [x] English copy - @darko-mijic 
- [x] Japanese translation - @darko-mijic 
- [x] New copy with the meeting conclusion changes - @darko-mijic 
- [x] New Japanese translation - @darko-mijic 
- [x] Implement acceptance tests
- [x] Implement dynamic disk-space-check intervals

## Screenshots:

### Before

![image](https://user-images.githubusercontent.com/1504716/47876280-aafa6f80-ddf7-11e8-9989-b6850891531c.png)

### After

![image](https://user-images.githubusercontent.com/1504716/49522366-a7428880-f88e-11e8-884b-faac8e1394db.png)



---

## Review Checklist:

### Basics

- [ ] PR is updated to the most recent version of target branch (and there are no conflicts)
- [ ] PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
- [ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub
- [ ] Automated tests: All acceptance tests are passing (`yarn run test`)
- [ ] Manual tests (minimum tests should cover newly added feature/fix): App works correctly in *development* build (`yarn run dev`)
- [ ] Manual tests (minimum tests should cover newly added feature/fix): App works correctly in *production* build (`yarn run package` / CI builds)
- [ ] There are no *flow* errors or warnings (`yarn run flow:test`)
- [ ] There are no *lint* errors or warnings (`yarn run lint`)
- [ ] Text changes are proofread and approved (Jane Wild)
- [ ] There are no missing translations (running `yarn run manage:translations` produces no changes)
- [ ] UI changes look good in all themes (Alexander Rukin)
- [ ] Storybook works and no stories are broken (`yarn run storybook`)
- [ ] In case of dependency changes `yarn.lock` file is updated

### Code Quality
- [ ] Important parts of the code are properly documented and commented
- [ ] Code is properly typed with flow
- [ ] React components are split-up enough to avoid unnecessary re-rendering
- [ ] Any code that only works in Electron is neatly separated from components

### Testing
- [ ] New feature / change is covered by acceptance tests
- [ ] All existing acceptance tests are still up-to-date
- [ ] New feature / change is covered by Daedalus Testing scenario
- [ ] All existing Daedalus Testing scenarios are still up-to-date

### After Review:
- [ ] Merge PR
- [ ] Delete source branch
- [ ] Move ticket to `done` on the Youtrack board


Co-authored-by: Danilo Prates <daniloprates@gmail.com>
@iohk-bors iohk-bors bot merged commit 66bbf7b into develop Jan 14, 2019
@iohk-bors iohk-bors bot deleted the feature/ddw-256-implement-better-handling-for-no-diskspace-error branch January 14, 2019 14:26
@nikolaglumac nikolaglumac mentioned this pull request Feb 22, 2019
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.

None yet

4 participants