Check the available storage size before starting encryption migration.

Check the available storage size before starting encryption migration.

When the device storage is critically low (<10MB), we should not start migration.
Instead, we should tell the user that the migration needs some space, and let the
user log in to Desktop.
After the user signs in to the Desktop, the user will see an existing system
notification which says that the device storage is critically low.

BUG=706017
TEST=manually tested the new screen by modifying kMinimumAvailableStorage to a big value.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2811713002
Cr-Commit-Position: refs/heads/master@{#463935}
Committed: https://chromium.googlesource.com/chromium/src/+/607e4843c7c95c472dc00bb54127a8010fbbe826

[fukino, 2017-04-10 11:47:26]

Description was changed from

==========
Check the available storage size before starting encryption migration.

When the device storage is critically low (<10MB), we should not start
migration.
Instead, we should tell the user that the migration needs some space, and let
the
user log in to Desktop.
After the user signs in to the Desktop, the user will see an existing system
notification which says that the device storage is critically low.

BUG=706017
TEST=manually tested the new screen by modifying kMinimumAvailableStorage to a
big value.
==========

to

==========
Check the available storage size before starting encryption migration.

When the device storage is critically low (<10MB), we should not start
migration.
Instead, we should tell the user that the migration needs some space, and let
the
user log in to Desktop.
After the user signs in to the Desktop, the user will see an existing system
notification which says that the device storage is critically low.

BUG=706017
TEST=manually tested the new screen by modifying kMinimumAvailableStorage to a
big value.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-04-10 12:08:50]

fukino@chromium.org changed reviewers:
+ xiyuan@chromium.org

[fukino, 2017-04-10 12:08:51]

Xiyuan, could you take a look?
I'm sorry to take up your time by consecutive CLs.

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/encryption_migration.html (right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/encryption_migration.html:79: <template
is="dom-if" if="[[isNotEnoughSpace_(uiState)]]">
This screen's design is still temporary.
I'll update these screens to match the latest mock and make them
internationalized in a separate patch.

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:23:
constexpr char kCheckStoragePath[] = "/home";
I'd like to call statvfs to a path on the stateful partition.
I assume that "/home" is mounted on the partition, but it should not be
guaranteed. (To me, it's hard to imagine that it will change in the future,
though)

More robust alternative should be adding cryptohome API to ask the remaining
space, but not sure if this use case justifies adding a new API.

WDYT?

[xiyuan, 2017-04-10 17:24:55]

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/encryption_migration.html (right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/encryption_migration.html:79: <template
is="dom-if" if="[[isNotEnoughSpace_(uiState)]]">
On 2017/04/10 12:08:50, fukino wrote:
> This screen's design is still temporary.
> I'll update these screens to match the latest mock and make them
> internationalized in a separate patch.

Acknowledged.

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:23:
constexpr char kCheckStoragePath[] = "/home";
On 2017/04/10 12:08:50, fukino wrote:
> I'd like to call statvfs to a path on the stateful partition.
> I assume that "/home" is mounted on the partition, but it should not be
> guaranteed. (To me, it's hard to imagine that it will change in the future,
> though)
> 
> More robust alternative should be adding cryptohome API to ask the remaining
> space, but not sure if this use case justifies adding a new API.
> 
> WDYT?

I am fine with using hard-coded "/home". I am not an expert of the mount though.
You might want to double check with Gwendal to make sure this is the correct one
for checking space needed for migration.

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:81:
// TODO(fukino): Handle the following case.
Why not always start with CheckAvailableStorage() and move the |should_resume_|
check to call StartMigration() in OnGetAvailableStorage() ?

[xiyuan, 2017-04-10 17:28:02]

No big issues with the CL. So lgtm because of the ticking clock on branch day
and timezone difference.

[fukino, 2017-04-10 23:04:58]

fukino@chromium.org changed reviewers:
+ gwendal@chromium.org

[fukino, 2017-04-10 23:04:58]

Hi Gwendal, I have a question.

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:23:
constexpr char kCheckStoragePath[] = "/home";
I call statvfs on "/home" to check the available space in the stateful partition
before sign in.
I assume that we can expect that "/home" is mounted on the stateful partition.
Do you have any concerns on this approach? Should I add cryptohome API to check
the available space?

[gwendal1, 2017-04-11 07:53:45]

On 2017/04/10 23:04:58, fukino wrote:
> Hi Gwendal, I have a question.
> 
>
https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
> File
> chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
> (right):
> 
>
https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
>
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:23:
> constexpr char kCheckStoragePath[] = "/home";
> I call statvfs on "/home" to check the available space in the stateful
partition
> before sign in.
> I assume that we can expect that "/home" is mounted on the stateful partition.
> Do you have any concerns on this approach? Should I add cryptohome API to
check
> the available space?
There is already an API in cryptohomed to free some space. Can you use it to
confirm there is indeed enough space available to allow a migration? If there is
less than 10mb free in /home, we are in a critical condition, given /var is
oversubscribed in the stateful partition, which host /home.

[fukino, 2017-04-11 09:07:01]

On 2017/04/11 07:53:45, gwendal1 wrote:
> On 2017/04/10 23:04:58, fukino wrote:
> > Hi Gwendal, I have a question.
> > 
> >
>
https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
> > File
> >
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
> >
>
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:23:
> > constexpr char kCheckStoragePath[] = "/home";
> > I call statvfs on "/home" to check the available space in the stateful
> partition
> > before sign in.
> > I assume that we can expect that "/home" is mounted on the stateful
partition.
> > Do you have any concerns on this approach? Should I add cryptohome API to
> check
> > the available space?

> There is already an API in cryptohomed to free some space. Can you use it to
> confirm there is indeed enough space available to allow a migration? If there
is
> less than 10mb free in /home, we are in a critical condition, given /var is
> oversubscribed in the stateful partition, which host /home.

Do you mean "AsyncDoAutomaticFreeDiskSpaceControl"?
But I think we can not determine if there is 10MB or not.
If there is enough space for migration, we'd like to do the migration.
Does this approach make sense?

[fukino, 2017-04-12 01:31:51]

On 2017/04/11 07:53:45, gwendal1 wrote:
> There is already an API in cryptohomed to free some space. Can you use it to
> confirm there is indeed enough space available to allow a migration? If there
is
> less than 10mb free in /home, we are in a critical condition, given /var is
> oversubscribed in the stateful partition, which host /home.

I agree that we should free some space using DoAutomaticFreeDiskSpaceControl
API.
I filed crbug.com/710711 for this.

However, the API returns false if we can make 512MB available space.
Even when the remaining space is less than 512MB, we should do the migration as
long as we can do it, and the migration process is designed to be able to run if
there is 10MB.
(If it is less than 10MB, we don't do migration and ask the user to make some
space manually)
So, I think we still need to know the remaining space under '/home'.

Let me go with the current approach (Checking 10MB) with a try to FreeDiskSpace
for now.
If you have any concerns to refer hard coded '/home' directory, please let me
know. I'll fix it in a separate CL.

[fukino, 2017-04-12 01:43:05]

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
File
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
(right):

https://codereview.chromium.org/2811713002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc:81:
// TODO(fukino): Handle the following case.
On 2017/04/10 17:24:55, xiyuan wrote:
> Why not always start with CheckAvailableStorage() and move the
|should_resume_|
> check to call StartMigration() in OnGetAvailableStorage() ?

You are right.
It is much simpler to always start with CheckAvailableStorage().
I updated the code to do so, and in |should_resume_| situation, I replaced
"Skip" button with "Restart" button since we can not sign in to the current
user's session.
(Showing sign-in screen instead of Restart should be better, but I chose
"Restart" for now for simplicity for a very rare situation.)

[fukino, 2017-04-12 01:43:53]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 01:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 02:52:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-12 02:52:43]

Dry run: This issue passed the CQ dry run.

[fukino, 2017-04-12 05:18:03]

The CQ bit was checked by fukino@chromium.org

[fukino, 2017-04-12 05:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2811713002/#ps40001
(title: "Always check the available storage.")

[commit-bot: I haz the power, 2017-04-12 05:18:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 05:24:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491974283837260,
"parent_rev": "a91facddec70d3f7bd95075eb9f15fa892572d18", "commit_rev":
"607e4843c7c95c472dc00bb54127a8010fbbe826"}

[commit-bot: I haz the power, 2017-04-12 05:25:01]

Description was changed from

==========
Check the available storage size before starting encryption migration.

When the device storage is critically low (<10MB), we should not start
migration.
Instead, we should tell the user that the migration needs some space, and let
the
user log in to Desktop.
After the user signs in to the Desktop, the user will see an existing system
notification which says that the device storage is critically low.

BUG=706017
TEST=manually tested the new screen by modifying kMinimumAvailableStorage to a
big value.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Check the available storage size before starting encryption migration.

When the device storage is critically low (<10MB), we should not start
migration.
Instead, we should tell the user that the migration needs some space, and let
the
user log in to Desktop.
After the user signs in to the Desktop, the user will see an existing system
notification which says that the device storage is critically low.

BUG=706017
TEST=manually tested the new screen by modifying kMinimumAvailableStorage to a
big value.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2811713002
Cr-Commit-Position: refs/heads/master@{#463935}
Committed:
https://chromium.googlesource.com/chromium/src/+/607e4843c7c95c472dc00bb54127...
==========

[commit-bot: I haz the power, 2017-04-12 05:25:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/607e4843c7c95c472dc00bb54127...