From d90545bc33e82f67a51f3198136c4fb7cb816b61 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 5 May 2023 11:37:36 -0700 Subject: [PATCH] libsnapshot: Fix flaky low-space tests. These tests have been a plague of flakiness forever, and while we've made improvements, the test is fundamentally flaky, and keeps showing up in presubmit. This change removes the code that attempts to fill /data until it's almost full, which is unreliable and destructive. Instead, attempt to allocate a file that is one block smaller than the maximum file size. This should always fail with ENOSPC, as opposed to the maximum file size which would fail with EFBIG. Bug: N/A Test: vts_libsnapshot_test Change-Id: I4652b83e31c50975ddb50b6cbe846e102ba0f322 --- .../include_test/libsnapshot/test_helpers.h | 23 ------ fs_mgr/libsnapshot/snapshot.cpp | 11 ++- fs_mgr/libsnapshot/snapshot_test.cpp | 77 +++++++++++-------- fs_mgr/libsnapshot/test_helpers.cpp | 62 --------------- 4 files changed, 56 insertions(+), 117 deletions(-) diff --git a/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h b/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h index 24c91a85b..f45d4ed2a 100644 --- a/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h +++ b/fs_mgr/libsnapshot/include_test/libsnapshot/test_helpers.h @@ -214,29 +214,6 @@ void SetSize(PartitionUpdate* partition_update, uint64_t size); // Get partition size from update package metadata. uint64_t GetSize(PartitionUpdate* partition_update); -// Util class for test cases on low space scenario. These tests assumes image manager -// uses /data as backup device. -class LowSpaceUserdata { - public: - // Set the maximum free space allowed for this test. If /userdata has more space than the given - // number, a file is allocated to consume space. - AssertionResult Init(uint64_t max_free_space); - - uint64_t free_space() const; - uint64_t available_space() const; - uint64_t bsize() const; - - private: - AssertionResult ReadUserdataStats(); - - static constexpr const char* kUserDataDevice = "/data"; - std::unique_ptr big_file_; - bool initialized_ = false; - uint64_t free_space_ = 0; - uint64_t available_space_ = 0; - uint64_t bsize_ = 0; -}; - bool IsVirtualAbEnabled(); #define SKIP_IF_NON_VIRTUAL_AB() \ diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 64637c284..26614827b 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -3133,6 +3133,7 @@ static Return AddRequiredSpace(Return orig, for (auto&& [name, status] : all_snapshot_status) { sum += status.cow_file_size(); } + LOG(INFO) << "Calculated needed COW space: " << sum << " bytes"; return Return::NoSpace(sum); } @@ -3279,7 +3280,10 @@ Return SnapshotManager::CreateUpdateSnapshots(const DeltaArchiveManifest& manife auto ret = CreateUpdateSnapshotsInternal(lock.get(), manifest, &cow_creator, &created_devices, &all_snapshot_status); - if (!ret.is_ok()) return ret; + if (!ret.is_ok()) { + LOG(ERROR) << "CreateUpdateSnapshotsInternal failed: " << ret.string(); + return ret; + } auto exported_target_metadata = target_metadata->Export(); if (exported_target_metadata == nullptr) { @@ -3496,7 +3500,10 @@ Return SnapshotManager::CreateUpdateSnapshotsInternal( // Create the backing COW image if necessary. if (snapshot_status.cow_file_size() > 0) { auto ret = CreateCowImage(lock, name); - if (!ret.is_ok()) return AddRequiredSpace(ret, *all_snapshot_status); + if (!ret.is_ok()) { + LOG(ERROR) << "CreateCowImage failed: " << ret.string(); + return AddRequiredSpace(ret, *all_snapshot_status); + } } LOG(INFO) << "Successfully created snapshot for " << name; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 1fbfaf750..22731e7b7 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -2371,20 +2372,44 @@ TEST_F(SnapshotUpdateTest, Overflow) { << "FinishedSnapshotWrites should detect overflow of CoW device."; } -TEST_F(SnapshotUpdateTest, LowSpace) { - static constexpr auto kMaxFree = 10_MiB; - auto userdata = std::make_unique(); - ASSERT_TRUE(userdata->Init(kMaxFree)); +// Get max file size and free space. +std::pair GetBigFileLimit() { + struct statvfs fs; + if (statvfs("/data", &fs) < 0) { + PLOG(ERROR) << "statfs failed"; + return {0, 0}; + } + + auto fs_limit = static_cast(fs.f_blocks) * (fs.f_bsize - 1); + auto fs_free = static_cast(fs.f_bfree) * fs.f_bsize; + + LOG(INFO) << "Big file limit: " << fs_limit << ", free space: " << fs_free; + + return {fs_limit, fs_free}; +} + +TEST_F(SnapshotUpdateTest, LowSpace) { + // To make the low space test more reliable, we force a large cow estimate. + // However legacy VAB ignores the COW estimate and uses InstallOperations + // to compute the exact size required for dm-snapshot. It's difficult to + // make this work reliably (we'd need to somehow fake an extremely large + // super partition, and we don't have that level of dependency injection). + // + // For now, just skip this test on legacy VAB. + if (!snapuserd_required_) { + GTEST_SKIP() << "Skipping test on legacy VAB"; + } + + auto fs = GetBigFileLimit(); + ASSERT_NE(fs.first, 0); - // Grow all partitions to 10_MiB, total 30_MiB. This requires 30 MiB of CoW space. After - // using the empty space in super (< 1 MiB), it uses 30 MiB of /userdata space. constexpr uint64_t partition_size = 10_MiB; SetSize(sys_, partition_size); SetSize(vnd_, partition_size); SetSize(prd_, partition_size); - sys_->set_estimate_cow_size(partition_size); - vnd_->set_estimate_cow_size(partition_size); - prd_->set_estimate_cow_size(partition_size); + sys_->set_estimate_cow_size(fs.first); + vnd_->set_estimate_cow_size(fs.first); + prd_->set_estimate_cow_size(fs.first); AddOperationForPartitions(); @@ -2393,8 +2418,12 @@ TEST_F(SnapshotUpdateTest, LowSpace) { auto res = sm->CreateUpdateSnapshots(manifest_); ASSERT_FALSE(res); ASSERT_EQ(Return::ErrorCode::NO_SPACE, res.error_code()); - ASSERT_GE(res.required_size(), 14_MiB); - ASSERT_LT(res.required_size(), 40_MiB); + + // It's hard to predict exactly how much free space is needed, since /data + // is writable and the test is not the only process running. Divide by two + // as a rough lower bound, and adjust this in the future as necessary. + auto expected_delta = fs.first - fs.second; + ASSERT_GE(res.required_size(), expected_delta / 2); } TEST_F(SnapshotUpdateTest, AddPartition) { @@ -2776,6 +2805,7 @@ class ImageManagerTest : public SnapshotTest { void TearDown() override { RETURN_IF_NON_VIRTUAL_AB(); CleanUp(); + SnapshotTest::TearDown(); } void CleanUp() { if (!image_manager_) { @@ -2789,26 +2819,13 @@ class ImageManagerTest : public SnapshotTest { }; TEST_F(ImageManagerTest, CreateImageNoSpace) { - bool at_least_one_failure = false; - for (uint64_t size = 1_MiB; size <= 512_MiB; size *= 2) { - auto userdata = std::make_unique(); - ASSERT_TRUE(userdata->Init(size)); + auto fs = GetBigFileLimit(); + ASSERT_NE(fs.first, 0); - uint64_t to_allocate = userdata->free_space() + userdata->bsize(); - - auto res = image_manager_->CreateBackingImage(kImageName, to_allocate, - IImageManager::CREATE_IMAGE_DEFAULT); - if (!res) { - at_least_one_failure = true; - } else { - ASSERT_EQ(res.error_code(), FiemapStatus::ErrorCode::NO_SPACE) << res.string(); - } - - CleanUp(); - } - - ASSERT_TRUE(at_least_one_failure) - << "We should have failed to allocate at least one over-sized image"; + auto res = image_manager_->CreateBackingImage(kImageName, fs.first, + IImageManager::CREATE_IMAGE_DEFAULT); + ASSERT_FALSE(res); + ASSERT_EQ(res.error_code(), FiemapStatus::ErrorCode::NO_SPACE) << res.string(); } bool Mkdir(const std::string& path) { diff --git a/fs_mgr/libsnapshot/test_helpers.cpp b/fs_mgr/libsnapshot/test_helpers.cpp index 9f1d6764f..a224f6b58 100644 --- a/fs_mgr/libsnapshot/test_helpers.cpp +++ b/fs_mgr/libsnapshot/test_helpers.cpp @@ -214,68 +214,6 @@ uint64_t GetSize(PartitionUpdate* partition_update) { return partition_update->mutable_new_partition_info()->size(); } -AssertionResult LowSpaceUserdata::Init(uint64_t max_free_space) { - auto res = ReadUserdataStats(); - if (!res) return res; - - // Try to fill up the disk as much as possible until free_space_ <= max_free_space. - big_file_ = std::make_unique(); - if (big_file_->fd == -1) { - return AssertionFailure() << strerror(errno); - } - if (!android::base::StartsWith(big_file_->path, kUserDataDevice)) { - return AssertionFailure() << "Temp file allocated to " << big_file_->path << ", not in " - << kUserDataDevice; - } - uint64_t next_consume = std::min(std::max(available_space_, max_free_space) - max_free_space, - (uint64_t)std::numeric_limits::max()); - off_t allocated = 0; - while (next_consume > 0 && free_space_ > max_free_space) { - int status = fallocate(big_file_->fd, 0, allocated, next_consume); - if (status == -1 && errno == ENOSPC) { - next_consume /= 2; - continue; - } - if (status == -1) { - return AssertionFailure() << strerror(errno); - } - allocated += next_consume; - - res = ReadUserdataStats(); - if (!res) return res; - } - - LOG(INFO) << allocated << " bytes allocated to " << big_file_->path; - initialized_ = true; - return AssertionSuccess(); -} - -AssertionResult LowSpaceUserdata::ReadUserdataStats() { - struct statvfs buf; - if (statvfs(kUserDataDevice, &buf) == -1) { - return AssertionFailure() << strerror(errno); - } - bsize_ = buf.f_bsize; - free_space_ = bsize_ * buf.f_bfree; - available_space_ = bsize_ * buf.f_bavail; - return AssertionSuccess(); -} - -uint64_t LowSpaceUserdata::free_space() const { - CHECK(initialized_); - return free_space_; -} - -uint64_t LowSpaceUserdata::available_space() const { - CHECK(initialized_); - return available_space_; -} - -uint64_t LowSpaceUserdata::bsize() const { - CHECK(initialized_); - return bsize_; -} - bool IsVirtualAbEnabled() { return android::base::GetBoolProperty("ro.virtual_ab.enabled", false); }