Crostini no longer fails to start when disk space is low.

The previous container restart flow would return an error if there was
insufficient disk. Free disk size is not relevant if there's an existing disk
image.

Bug: 891165
Change-Id: I34ed750263182e920ab96befc8b00f1f2b2db89e
Reviewed-on: https://chromium-review.googlesource.com/c/1328482Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607490}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -185,9 +185,49 @@ class CrostiniManager::CrostiniRestarter
       FinishRestart(result);
       return;
     }
+
+    crostini_manager_->ListVmDisks(
+        base::BindOnce(&CrostiniRestarter::ListVmDisksFinished, this));
+  }
+
+  void ListVmDisksFinished(CrostiniResult result, int64_t disk_space_taken) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (is_aborted_)
+      return;
+    if (result != CrostiniResult::SUCCESS) {
+      LOG(ERROR) << "Failed to list disk images.";
+      FinishRestart(result);
+      return;
+    }
+    base::PostTaskWithTraitsAndReplyWithResult(
+        FROM_HERE, {base::MayBlock()},
+        base::BindOnce(&base::SysInfo::AmountOfFreeDiskSpace,
+                       base::FilePath(kHomeDirectory)),
+        base::BindOnce(&CrostiniRestarter::CreateDiskImageAfterSizeCheck, this,
+                       disk_space_taken));
+  }
+
+  void CreateDiskImageAfterSizeCheck(int64_t disk_space_taken,
+                                     int64_t free_disk_bytes) {
+    int64_t disk_size_available = (free_disk_bytes * 9) / 10;
+    // If there's no existing disk image, need enough space to create one.
+    if (disk_space_taken == 0) {
+      // Don't enforce minimum disk size on dev box or trybots because
+      // base::SysInfo::AmountOfFreeDiskSpace returns zero in testing.
+      if (disk_size_available < kMinimumDiskSize &&
+          base::SysInfo::IsRunningOnChromeOS()) {
+        LOG(ERROR) << "Insufficient disk available. Need to free "
+                   << kMinimumDiskSize - disk_size_available << " bytes";
+        FinishRestart(CrostiniResult::INSUFFICIENT_DISK);
+        return;
+      }
+    }
+    // If we have an already existing disk, CreateDiskImage will just return its
+    // path so we can pass it to StartTerminaVm.
     crostini_manager_->CreateDiskImage(
         base::FilePath(vm_name_),
         vm_tools::concierge::StorageLocation::STORAGE_CRYPTOHOME_ROOT,
+        disk_size_available,
         base::BindOnce(&CrostiniRestarter::CreateDiskImageFinished, this));
   }
 
@@ -659,6 +699,7 @@ void CrostiniManager::OnStopConcierge(StopConciergeCallback callback,
 void CrostiniManager::CreateDiskImage(
     const base::FilePath& disk_path,
     vm_tools::concierge::StorageLocation storage_location,
+    int64_t disk_size_bytes,
     CreateDiskImageCallback callback) {
   std::string disk_path_string = disk_path.AsUTF8Unsafe();
   if (disk_path_string.empty()) {
@@ -687,34 +728,8 @@ void CrostiniManager::CreateDiskImage(
     return;
   }
   request.set_storage_location(storage_location);
-
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&base::SysInfo::AmountOfFreeDiskSpace,
-                     base::FilePath(kHomeDirectory)),
-      base::BindOnce(&CrostiniManager::CreateDiskImageAfterSizeCheck,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(request),
-                     std::move(callback)));
-}
-
-void CrostiniManager::CreateDiskImageAfterSizeCheck(
-    vm_tools::concierge::CreateDiskImageRequest request,
-    CreateDiskImageCallback callback,
-    int64_t free_disk_size) {
-  int64_t disk_size = (free_disk_size * 9) / 10;
-  // Skip disk size check on dev box or trybots because
-  // base::SysInfo::AmountOfFreeDiskSpace returns zero in testing.
-  if (disk_size < kMinimumDiskSize && base::SysInfo::IsRunningOnChromeOS()) {
-    LOG(ERROR) << "Insufficient disk available. Need to free "
-               << kMinimumDiskSize - disk_size << " bytes";
-    std::move(callback).Run(
-        CrostiniResult::CLIENT_ERROR,
-        vm_tools::concierge::DiskImageStatus::DISK_STATUS_UNKNOWN,
-        base::FilePath());
-    return;
-  }
   // The logical size of the new disk image, in bytes.
-  request.set_disk_size(std::move(disk_size));
+  request.set_disk_size(std::move(disk_size_bytes));
 
   GetConciergeClient()->CreateDiskImage(
       std::move(request),

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -33,6 +33,7 @@ enum class CrostiniResult {
   SUCCESS,
   DBUS_ERROR,
   UNPARSEABLE_RESPONSE,
+  INSUFFICIENT_DISK,
   CREATE_DISK_IMAGE_FAILED,
   VM_START_FAILED,
   VM_STOP_FAILED,
@@ -234,6 +235,8 @@ class CrostiniManager : public KeyedService,
       const base::FilePath& disk_path,
       // The storage location for the disk image
       vm_tools::concierge::StorageLocation storage_location,
+      // The logical size of the disk image, in bytes
+      int64_t disk_size_bytes,
       CreateDiskImageCallback callback);
 
   // Checks the arguments for destroying a named Termina VM disk image.
@@ -541,12 +544,6 @@ class CrostiniManager : public KeyedService,
   // checking component registration code may block.
   void MaybeUpgradeCrostiniAfterChecks();
 
-  // Helper for CrostiniManager::CreateDiskImage. Separated so it can be run
-  // off the main thread.
-  void CreateDiskImageAfterSizeCheck(
-      vm_tools::concierge::CreateDiskImageRequest request,
-      CreateDiskImageCallback callback,
-      int64_t free_disk_size);
 
   void FinishRestart(CrostiniRestarter* restarter, CrostiniResult result);
 

chrome/browser/chromeos/crostini/crostini_manager_unittest.cc

@@ -24,6 +24,7 @@ namespace crostini {
 namespace {
 const char kVmName[] = "vm_name";
 const char kContainerName[] = "container_name";
+constexpr int64_t kDiskSizeBytes = 4ll * 1024 * 1024 * 1024;  // 4 GiB
 }  // namespace
 
 class CrostiniManagerTest : public testing::Test {
@@ -177,7 +178,7 @@ TEST_F(CrostiniManagerTest, CreateDiskImageNameError) {
   const base::FilePath& disk_path = base::FilePath("");
 
   crostini_manager()->CreateDiskImage(
-      disk_path, vm_tools::concierge::STORAGE_CRYPTOHOME_ROOT,
+      disk_path, vm_tools::concierge::STORAGE_CRYPTOHOME_ROOT, kDiskSizeBytes,
       base::BindOnce(&CrostiniManagerTest::CreateDiskImageClientErrorCallback,
                      base::Unretained(this), run_loop()->QuitClosure()));
   run_loop()->Run();
@@ -189,6 +190,7 @@ TEST_F(CrostiniManagerTest, CreateDiskImageStorageLocationError) {
   crostini_manager()->CreateDiskImage(
       disk_path,
       vm_tools::concierge::StorageLocation_INT_MIN_SENTINEL_DO_NOT_USE_,
+      kDiskSizeBytes,
       base::BindOnce(&CrostiniManagerTest::CreateDiskImageClientErrorCallback,
                      base::Unretained(this), run_loop()->QuitClosure()));
   run_loop()->Run();
@@ -199,6 +201,7 @@ TEST_F(CrostiniManagerTest, CreateDiskImageSuccess) {
 
   crostini_manager()->CreateDiskImage(
       disk_path, vm_tools::concierge::STORAGE_CRYPTOHOME_DOWNLOADS,
+      kDiskSizeBytes,
       base::BindOnce(&CrostiniManagerTest::CreateDiskImageSuccessCallback,
                      base::Unretained(this), run_loop()->QuitClosure()));
   run_loop()->Run();