From 14d6afa2b1aed8733c3522e7715a425510ead034 Mon Sep 17 00:00:00 2001 From: Mike Sul Date: Mon, 4 Sep 2023 12:47:55 +0200 Subject: [PATCH] rootfsmanager: Add usage stat if no pre-pull check error - Print and add usage statistic to the "download complete" event if the "no space" error occurs during ostree pull (the pre-pull check is successful). - Correct corresponding tests. - Extend the tests to cover the new functionality. Signed-off-by: Mike Sul --- src/rootfstreemanager.cc | 22 +++++--- src/rootfstreemanager.h | 2 +- src/storage/stat.cc | 1 + tests/nospace_test.cc | 118 ++++++++++++++++++++++++++++----------- 4 files changed, 101 insertions(+), 42 deletions(-) diff --git a/src/rootfstreemanager.cc b/src/rootfstreemanager.cc index 425984e3..a84e59a4 100644 --- a/src/rootfstreemanager.cc +++ b/src/rootfstreemanager.cc @@ -52,12 +52,15 @@ DownloadResult RootfsTreeManager::Download(const TufTarget& target) { setRemote(remote.name, remote.baseUrl, remote.keys); } - storage::Volume::UsageInfo pre_pull_usage_info{getUsageInfo()}; + DeltaStat delta_stat{}; + bool delta_stat_avail{getDeltaStatIfAvailable(target, remote, delta_stat)}; + + storage::Volume::UsageInfo pre_pull_usage_info{getUsageInfo(!delta_stat_avail)}; if (!pre_pull_usage_info.isOk()) { LOG_ERROR << "Failed to obtain storage usage statistic: " << pre_pull_usage_info.err; } - DeltaStat delta_stat{}; - if (getDeltaStatIfAvailable(target, remote, delta_stat)) { + + if (delta_stat_avail) { if (pre_pull_usage_info.isOk()) { LOG_INFO << "Checking if update can fit on a disk..."; if (pre_pull_usage_info.available.first < delta_stat.uncompressedSize) { @@ -83,7 +86,7 @@ DownloadResult RootfsTreeManager::Download(const TufTarget& target) { pull_err = OstreeManager::pull(config.sysroot, remote.baseUrl, keys_, Target::fromTufTarget(target), nullptr, prog_cb, remote.isRemoteSet ? nullptr : remote.name.c_str(), remote.headers); - storage::Volume::UsageInfo post_pull_usage_info{getUsageInfo()}; + storage::Volume::UsageInfo post_pull_usage_info{getUsageInfo(!delta_stat_avail)}; if (post_pull_usage_info.isOk()) { LOG_INFO << "Post pull storage usage info; " << post_pull_usage_info; } else { @@ -91,7 +94,8 @@ DownloadResult RootfsTreeManager::Download(const TufTarget& target) { } if (pull_err.isSuccess()) { res = {DownloadResult::Status::Ok, - "before ostree pull; " + pre_pull_usage_info.str() + "\nafter ostree pull; " + post_pull_usage_info.str()}; + "before ostree pull; " + pre_pull_usage_info.str() + "\nafter ostree pull; " + post_pull_usage_info.str(), + sysroot_->repoPath()}; break; } @@ -104,7 +108,9 @@ DownloadResult RootfsTreeManager::Download(const TufTarget& target) { // not enough storage space in the case of a static delta pull (pulling the delta parts/files) (pull_err.description.find("Delta requires") != std::string::npos && pull_err.description.find("free space, but only") != std::string::npos)) { - res = {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available; " + pull_err.description, + res = {DownloadResult::Status::DownloadFailed_NoSpace, + "Insufficient storage available; " + pull_err.description + "\nbefore ostree pull; " + + pre_pull_usage_info.str() + "\nafter ostree pull; " + post_pull_usage_info.str(), sysroot_->repoPath()}; break; } @@ -402,12 +408,12 @@ bool RootfsTreeManager::findDeltaStatForUpdate(const Json::Value& delta_stats, c return true; } -storage::Volume::UsageInfo RootfsTreeManager::getUsageInfo() const { +storage::Volume::UsageInfo RootfsTreeManager::getUsageInfo(bool just_reserved_by_ostree) const { unsigned int reserved_percentage{sysroot_->reservedStorageSpacePercentageDelta()}; std::string reserved_by{OSTree::Sysroot::Config::ReservedStorageSpacePercentageDeltaParamName}; const unsigned int reserved_by_ostree{sysroot_->reservedStorageSpacePercentageOstree()}; - if (reserved_percentage < reserved_by_ostree) { + if (just_reserved_by_ostree || reserved_percentage < reserved_by_ostree) { reserved_percentage = reserved_by_ostree; reserved_by = OSTree::Sysroot::Config::ReservedStorageSpacePercentageOstreeParamName; } diff --git a/src/rootfstreemanager.h b/src/rootfstreemanager.h index f127db34..389e45af 100644 --- a/src/rootfstreemanager.h +++ b/src/rootfstreemanager.h @@ -71,7 +71,7 @@ class RootfsTreeManager : public OstreeManager, public Downloader { void setRemote(const std::string& name, const std::string& url, const boost::optional& keys); data::InstallationResult verifyBootloaderUpdate(const Uptane::Target& target) const; bool getDeltaStatIfAvailable(const TufTarget& target, const Remote& remote, DeltaStat& delta_stat) const; - storage::Volume::UsageInfo getUsageInfo() const; + storage::Volume::UsageInfo getUsageInfo(bool just_reserved_by_ostree) const; static bool getDeltaStatsRef(const Json::Value& json, DeltaStatsRef& ref); static Json::Value downloadDeltaStats(const DeltaStatsRef& ref, const Remote& remote); diff --git a/src/storage/stat.cc b/src/storage/stat.cc index 3adb8a77..27a08dd7 100644 --- a/src/storage/stat.cc +++ b/src/storage/stat.cc @@ -92,6 +92,7 @@ ostream& operator<<(ostream& os, const storage::Volume::UsageInfo& t) { if (t.required.first > 0) { os << "required: " << t.required.first << "B unknown%"; } + os << "; fstatvfs err: " << t.err; return os; } diff --git a/tests/nospace_test.cc b/tests/nospace_test.cc index 67243ed4..6bc425e7 100644 --- a/tests/nospace_test.cc +++ b/tests/nospace_test.cc @@ -127,7 +127,7 @@ TEST(StorageStat, UsageInfo) { usage_info.withRequired(7); ASSERT_EQ(7, usage_info.required.first); ASSERT_EQ(0, usage_info.required.second); - ASSERT_EQ("required: 7B unknown%", usage_info.str()); + ASSERT_TRUE(std::string::npos != usage_info.str().find("required: 7B unknown%")) << usage_info; } { unsigned int block_size{4096}; @@ -213,6 +213,11 @@ TEST_F(NoSpaceTest, OstreeUpdateNoSpace) { update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + const auto event_err_msg{getEventContext("EcuDownloadCompleted")}; + ASSERT_TRUE(std::string::npos != event_err_msg.find("min-free-space-size 1048576MB would be exceeded")) + << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find("before ostree pull; available:")) << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find("after ostree pull; available:")) << event_err_msg; // reboot device reboot(client); @@ -230,62 +235,109 @@ TEST_F(NoSpaceTest, OstreeUpdateNoSpaceIfWatermarkParamIsSet) { ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); auto new_target = createTarget(); - update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, - {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); - ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); - // Now, let's decrease the required minimum space to 40%, since the update size is < 9 blocks, - // then the libostree should be happy. - // We need to "reboot" in order to recreate the client instance so the new watermark is applied. - setMinFreeSpace("40"); - reboot(client); - ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); - update(*client, getInitialTarget(), new_target); - reboot(client); - ASSERT_TRUE(targetsMatch(client->getCurrent(), new_target)); + { + update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, + {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); + ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + const auto event_err_msg{getEventContext("EcuDownloadCompleted")}; + ASSERT_TRUE(std::string::npos != event_err_msg.find("min-free-space-percent '50%' would be exceeded")) + << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find("available: 4096B 1%")) << event_err_msg; + } + { + // Now, let's decrease the required minimum space to 40%, since the update size is < 9 blocks, + // then the libostree should be happy. + // We need to "reboot" in order to recreate the client instance so the new watermark is applied. + setMinFreeSpace("40"); + reboot(client); + ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + update(*client, getInitialTarget(), new_target); + reboot(client); + ASSERT_TRUE(targetsMatch(client->getCurrent(), new_target)); + } UnsetFreeBlockNumb(); } TEST_F(NoSpaceTest, OstreeUpdateNoSpaceIfStaticDelta) { auto client = createLiteClient(); ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + // Delta size is 3 + 1 = 4 blocks setGenerateStaticDelta(3); auto new_target = createTarget(); { - // Not enough space/block, 3 is required, we got only 1. + // The delta-based update if there is no stat/info about the delta, so the pre-pull verification + // of the update size is not possible. Thus, the error originates in libostree; libostree does NOT + // apply any threshold/reserved when checking if there is enough storage to store a delta file, + // it just checks for the overall storage capacity. + // + // required 4%, free 2%, available 0%, no pre-pull check -> libostree generates the error "Delta requires..." // The expected libostree error: // "Error while pulling image: 0 Delta requires 13.8 kB free space, but only 4.1 kB available" - SetFreeBlockNumb(1, 10); + SetFreeBlockNumb(2, 100); update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + const auto event_err_msg{getEventContext("EcuDownloadCompleted")}; + ASSERT_TRUE(std::string::npos != event_err_msg.find("Delta requires")) << event_err_msg; + // libostree converts 2*4096=8192B by dividing it to 1000 not 1024. + ASSERT_TRUE(std::string::npos != event_err_msg.find("free space, but only 8.2 kB available")) << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find("available: 0B 0%")) << event_err_msg; + ASSERT_TRUE(std::string::npos != + event_err_msg.find(OSTree::Sysroot::Config::ReservedStorageSpacePercentageOstreeParamName)) + << event_err_msg; } { - // There is enough space to accommodate the 3-block delta since we have 3 free blocks. - // But, at the last step of the delta processing libostree calls dispatch_close() -> - // _ostree_repo_bare_content_commit(). And, the _ostree_repo_bare_content_commit() checks the - // `min-free-space-percent`/`min-free-space-size` watermark. In our case we have 3 out of 100 blocks free, - // which is enough to fit the delta, but less than the `min-free-space-percent` of overall storage - // becomes free after the update, so libostree rejects it. - // It doesn't make sense to reject the update after its content is pulled and already written to a disk, - // but this is the way libostree works, so we have to adjust...(we bypass this issue by using the delta stats) - // Expected error message is: + // The delta-based update if there is no stat/info about the delta, so the pre-pull verification + // of the update size is not possible. Thus, the error originates in libostree; libostree does NOT + // apply any threshold/reserved when checking if there is enough storage to store a delta file, + // it just checks for the overall storage capacity. + // In this case, there is enough free storage to accommodate the delta file. + // But, during committing the ostree objects extracted from the delta file, libostree checks + // if there is enough free storage is available taking into account the + // `min-free-space-percent`/`min-free-space-size` threshold -> reserved storage. By default, the libostree sets it + // to 3%. + // + // required 4%, free 5%, reserved 3%, available 2%, no pre-pull check -> libostree generates the error "would be + // exceeded, at least..." Expected error message is: // "Error while pulling image: 0 opcode close: min-free-space-percent '3%' would be exceeded, at least 13 bytes // requested" - SetFreeBlockNumb(3, 100); + SetFreeBlockNumb(5, 100); + update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, + {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); + ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + const auto event_err_msg{getEventContext("EcuDownloadCompleted")}; + ASSERT_TRUE(std::string::npos != event_err_msg.find("min-free-space-percent '3%' would be exceeded, at least")) + << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find("available: 8192B 2%")) << event_err_msg; + ASSERT_TRUE(std::string::npos != + event_err_msg.find(OSTree::Sysroot::Config::ReservedStorageSpacePercentageOstreeParamName)); + } + { + // required 4%, free 8%, reserved 3%, available 8% - 3% = 5% -> should be ok since 5 > 4, + // but there is a moment during the delta-based pull when libostree has the delta file + // on a file system + extracted files while it commits the extracted files to the repo. + // So, it takes into account the delta file size + extracted objects during ostree objects committing, + // therefore we need 4% + ~ 6% (required)+ 3% (reserved) ~ 9% (at least 9 free blocks + // is required, but just 8 is available) + // + storage::Volume::UsageInfo usage_info{.size = {100 * 4096, 100}, .available = {(8 - 3) * 4096, 8 - 3}}; + std::stringstream expected_available; + expected_available << usage_info.available; + SetFreeBlockNumb(8, 100); update(*client, getInitialTarget(), new_target, data::ResultCode::Numeric::kDownloadFailed, {DownloadResult::Status::DownloadFailed_NoSpace, "Insufficient storage available"}); ASSERT_TRUE(targetsMatch(client->getCurrent(), getInitialTarget())); + const auto event_err_msg{getEventContext("EcuDownloadCompleted")}; + ASSERT_TRUE(std::string::npos != event_err_msg.find("min-free-space-percent '3%' would be exceeded, at least")) + << event_err_msg; + ASSERT_TRUE(std::string::npos != event_err_msg.find(expected_available.str())) << event_err_msg; + ASSERT_TRUE(std::string::npos != + event_err_msg.find(OSTree::Sysroot::Config::ReservedStorageSpacePercentageOstreeParamName)); } { - // Now, we have 15 blocks out of 100 free, so 15-3 = 12 - 12% of storage will be free after the update, - // so libostree should be happy. - // NOTE: 7 blocks (7%) should suffice, but for some reason libostree requires 15%. - // TODO: Check the following assumption. - // Most likely there is a moment during download at which 2x of the update/delta size is required. - // Specifically, in this case, 3% for downloaded delta and then 3% to commit it to the repo - hence > 6% is - // needed. - SetFreeBlockNumb(15, 100); + // required 4%, free 15%, reserved 3%, available 15% - 3% = 12% -> ok + SetFreeBlockNumb(14, 100); update(*client, getInitialTarget(), new_target); reboot(client); ASSERT_TRUE(targetsMatch(client->getCurrent(), new_target));