Track bytes received across multiple update files
When downloading the packages that comprise a multi-package or multi-app
update, the UpdateAttempter receives BytesReceived() callbacks with
bytes_received resetting to 0 for each file. This causes the progress
calculations to be incorrect.
This change tracks the total of the previously downloaded packages
within the DownloadAction, so that it properly tracks. Resumed
downloads will jump ahead over skipped data, when the payload is
incremented.
Bug: 65451460
Tests: Added unit tests to directly test the accumulation and the the
transition from the previous state to UpdateStatus::DOWNLOADING when the
first bytes are received.
Change-Id: I3b540df16b9a664b09f53ee3ec962e2cbc8adf1b
(cherry picked from commit d6f869dbd9952be8a926e80c4f1e172845ab8d5f)
diff --git a/mock_file_writer.h b/mock_file_writer.h
index 72d6a86..26cd45d 100644
--- a/mock_file_writer.h
+++ b/mock_file_writer.h
@@ -24,7 +24,8 @@
class MockFileWriter : public FileWriter {
public:
- MOCK_METHOD2(Write, ssize_t(const void* bytes, size_t count));
+ MOCK_METHOD2(Write, bool(const void* bytes, size_t count));
+ MOCK_METHOD3(Write, bool(const void* bytes, size_t count, ErrorCode* error));
MOCK_METHOD0(Close, int());
};
diff --git a/mock_service_observer.h b/mock_service_observer.h
new file mode 100644
index 0000000..032f55a
--- /dev/null
+++ b/mock_service_observer.h
@@ -0,0 +1,38 @@
+//
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#ifndef UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
+#define UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
+
+#include <gmock/gmock.h>
+#include "update_engine/service_observer_interface.h"
+
+namespace chromeos_update_engine {
+
+class MockServiceObserver : public ServiceObserverInterface {
+ public:
+ MOCK_METHOD5(SendStatusUpdate,
+ void(int64_t last_checked_time,
+ double progress,
+ update_engine::UpdateStatus status,
+ const std::string& new_version,
+ int64_t new_size));
+ MOCK_METHOD1(SendPayloadApplicationComplete, void(ErrorCode error_code));
+};
+
+} // namespace chromeos_update_engine
+
+#endif // UPDATE_ENGINE_MOCK_SERVICE_OBSERVER_H_
diff --git a/payload_consumer/download_action.cc b/payload_consumer/download_action.cc
index c3a5016..cd25962 100644
--- a/payload_consumer/download_action.cc
+++ b/payload_consumer/download_action.cc
@@ -173,6 +173,7 @@
install_plan_.Dump();
bytes_received_ = 0;
+ bytes_received_previous_payloads_ = 0;
bytes_total_ = 0;
for (const auto& payload : install_plan_.payloads)
bytes_total_ += payload.size;
@@ -317,8 +318,10 @@
}
bytes_received_ += length;
+ uint64_t bytes_downloaded_total =
+ bytes_received_previous_payloads_ + bytes_received_;
if (delegate_ && download_active_) {
- delegate_->BytesReceived(length, bytes_received_, bytes_total_);
+ delegate_->BytesReceived(length, bytes_downloaded_total, bytes_total_);
}
if (writer_ && !writer_->Write(bytes, length, &code_)) {
if (code_ != ErrorCode::kSuccess) {
@@ -349,13 +352,16 @@
void DownloadAction::TransferComplete(HttpFetcher* fetcher, bool successful) {
if (writer_) {
LOG_IF(WARNING, writer_->Close() != 0) << "Error closing the writer.";
- writer_ = nullptr;
+ if (delta_performer_.get() == writer_) {
+ // no delta_performer_ in tests, so leave the test writer in place
+ writer_ = nullptr;
+ }
}
download_active_ = false;
ErrorCode code =
successful ? ErrorCode::kSuccess : ErrorCode::kDownloadTransferError;
- if (code == ErrorCode::kSuccess && delta_performer_.get()) {
- if (!payload_->already_applied)
+ if (code == ErrorCode::kSuccess) {
+ if (delta_performer_ && !payload_->already_applied)
code = delta_performer_->VerifyPayload(payload_->hash, payload_->size);
if (code != ErrorCode::kSuccess) {
LOG(ERROR) << "Download of " << install_plan_.download_url
@@ -365,10 +371,12 @@
CloseP2PSharingFd(true);
} else if (payload_ < &install_plan_.payloads.back() &&
system_state_->payload_state()->NextPayload()) {
+ LOG(INFO) << "Incrementing to next payload";
// No need to reset if this payload was already applied.
- if (!payload_->already_applied)
+ if (delta_performer_ && !payload_->already_applied)
DeltaPerformer::ResetUpdateProgress(prefs_, false);
// Start downloading next payload.
+ bytes_received_previous_payloads_ += payload_->size;
payload_++;
install_plan_.download_url =
system_state_->payload_state()->GetCurrentUrl();
diff --git a/payload_consumer/download_action.h b/payload_consumer/download_action.h
index d0e6000..786db20 100644
--- a/payload_consumer/download_action.h
+++ b/payload_consumer/download_action.h
@@ -166,7 +166,8 @@
// For reporting status to outsiders
DownloadActionDelegate* delegate_;
- uint64_t bytes_received_{0};
+ uint64_t bytes_received_{0}; // per file/range
+ uint64_t bytes_received_previous_payloads_{0};
uint64_t bytes_total_{0};
bool download_active_{false};
diff --git a/payload_consumer/download_action_unittest.cc b/payload_consumer/download_action_unittest.cc
index 7d3ac6c..f42b1d8 100644
--- a/payload_consumer/download_action_unittest.cc
+++ b/payload_consumer/download_action_unittest.cc
@@ -41,6 +41,7 @@
#include "update_engine/common/utils.h"
#include "update_engine/fake_p2p_manager_configuration.h"
#include "update_engine/fake_system_state.h"
+#include "update_engine/mock_file_writer.h"
#include "update_engine/payload_consumer/mock_download_action.h"
#include "update_engine/update_manager/fake_update_manager.h"
@@ -56,6 +57,7 @@
using testing::AtLeast;
using testing::InSequence;
using testing::Return;
+using testing::SetArgPointee;
using testing::_;
class DownloadActionTest : public ::testing::Test { };
@@ -242,6 +244,92 @@
false); // use_download_delegate
}
+TEST(DownloadActionTest, MultiPayloadProgressTest) {
+ std::vector<brillo::Blob> payload_datas;
+ // the first payload must be the largest, as it's the actual payload used by
+ // the MockHttpFetcher for all downloaded data.
+ payload_datas.emplace_back(4 * kMockHttpFetcherChunkSize + 256);
+ payload_datas.emplace_back(2 * kMockHttpFetcherChunkSize);
+ brillo::FakeMessageLoop loop(nullptr);
+ loop.SetAsCurrent();
+ FakeSystemState fake_system_state;
+ EXPECT_CALL(*fake_system_state.mock_payload_state(), NextPayload())
+ .WillOnce(Return(true));
+
+ MockFileWriter mock_file_writer;
+ EXPECT_CALL(mock_file_writer, Close()).WillRepeatedly(Return(0));
+ EXPECT_CALL(mock_file_writer, Write(_, _, _))
+ .WillRepeatedly(
+ DoAll(SetArgPointee<2>(ErrorCode::kSuccess), Return(true)));
+
+ InstallPlan install_plan;
+ uint64_t total_expected_download_size{0};
+ for (const auto& data : payload_datas) {
+ uint64_t size = data.size();
+ install_plan.payloads.push_back(
+ {.size = size, .type = InstallPayloadType::kFull});
+ total_expected_download_size += size;
+ }
+ ObjectFeederAction<InstallPlan> feeder_action;
+ feeder_action.set_obj(install_plan);
+ MockPrefs prefs;
+ MockHttpFetcher* http_fetcher = new MockHttpFetcher(
+ payload_datas[0].data(), payload_datas[0].size(), nullptr);
+ // takes ownership of passed in HttpFetcher
+ DownloadAction download_action(&prefs,
+ fake_system_state.boot_control(),
+ fake_system_state.hardware(),
+ &fake_system_state,
+ http_fetcher);
+ download_action.SetTestFileWriter(&mock_file_writer);
+ BondActions(&feeder_action, &download_action);
+ MockDownloadActionDelegate download_delegate;
+ {
+ InSequence s;
+ download_action.set_delegate(&download_delegate);
+ // these are hand-computed based on the payloads specified above
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 2,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 3,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 4,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(256,
+ kMockHttpFetcherChunkSize * 4 + 256,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ kMockHttpFetcherChunkSize * 5 + 256,
+ total_expected_download_size));
+ EXPECT_CALL(download_delegate,
+ BytesReceived(kMockHttpFetcherChunkSize,
+ total_expected_download_size,
+ total_expected_download_size));
+ }
+ ActionProcessor processor;
+ processor.EnqueueAction(&feeder_action);
+ processor.EnqueueAction(&download_action);
+
+ loop.PostTask(
+ FROM_HERE,
+ base::Bind(
+ [](ActionProcessor* processor) { processor->StartProcessing(); },
+ base::Unretained(&processor)));
+ loop.Run();
+ EXPECT_FALSE(loop.PendingTasks());
+}
+
namespace {
class TerminateEarlyTestProcessorDelegate : public ActionProcessorDelegate {
public:
diff --git a/update_attempter.h b/update_attempter.h
index b4e2f60..9dd844d 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -252,17 +252,20 @@
FRIEND_TEST(UpdateAttempterTest, ActionCompletedDownloadTest);
FRIEND_TEST(UpdateAttempterTest, ActionCompletedErrorTest);
FRIEND_TEST(UpdateAttempterTest, ActionCompletedOmahaRequestTest);
+ FRIEND_TEST(UpdateAttempterTest, BootTimeInUpdateMarkerFile);
+ FRIEND_TEST(UpdateAttempterTest, BroadcastCompleteDownloadTest);
+ FRIEND_TEST(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest);
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventTest);
FRIEND_TEST(UpdateAttempterTest, CreatePendingErrorEventResumedTest);
FRIEND_TEST(UpdateAttempterTest, DisableDeltaUpdateIfNeededTest);
+ FRIEND_TEST(UpdateAttempterTest, DownloadProgressAccumulationTest);
FRIEND_TEST(UpdateAttempterTest, MarkDeltaUpdateFailureTest);
FRIEND_TEST(UpdateAttempterTest, PingOmahaTest);
+ FRIEND_TEST(UpdateAttempterTest, ReportDailyMetrics);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionNoEventTest);
FRIEND_TEST(UpdateAttempterTest, ScheduleErrorEventActionTest);
- FRIEND_TEST(UpdateAttempterTest, UpdateTest);
- FRIEND_TEST(UpdateAttempterTest, ReportDailyMetrics);
- FRIEND_TEST(UpdateAttempterTest, BootTimeInUpdateMarkerFile);
FRIEND_TEST(UpdateAttempterTest, TargetVersionPrefixSetAndReset);
+ FRIEND_TEST(UpdateAttempterTest, UpdateTest);
// CertificateChecker::Observer method.
// Report metrics about the certificate being checked.
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 4ebc85c..911f664 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -48,6 +48,7 @@
#include "update_engine/fake_system_state.h"
#include "update_engine/mock_p2p_manager.h"
#include "update_engine/mock_payload_state.h"
+#include "update_engine/mock_service_observer.h"
#include "update_engine/payload_consumer/filesystem_verifier_action.h"
#include "update_engine/payload_consumer/install_plan.h"
#include "update_engine/payload_consumer/payload_constants.h"
@@ -219,6 +220,7 @@
EXPECT_CALL(*prefs_, GetInt64(kPrefsDeltaUpdateFailures, _)).Times(0);
attempter_.ActionCompleted(nullptr, &action, ErrorCode::kSuccess);
EXPECT_EQ(UpdateStatus::FINALIZING, attempter_.status());
+ EXPECT_EQ(0.0, attempter_.download_progress_);
ASSERT_EQ(nullptr, attempter_.error_event_.get());
}
@@ -232,6 +234,79 @@
ASSERT_NE(nullptr, attempter_.error_event_.get());
}
+TEST_F(UpdateAttempterTest, DownloadProgressAccumulationTest) {
+ // Simple test case, where all the values match (nothing was skipped)
+ uint64_t bytes_progressed_1 = 1024 * 1024; // 1MB
+ uint64_t bytes_progressed_2 = 1024 * 1024; // 1MB
+ uint64_t bytes_received_1 = bytes_progressed_1;
+ uint64_t bytes_received_2 = bytes_received_1 + bytes_progressed_2;
+ uint64_t bytes_total = 20 * 1024 * 1024; // 20MB
+
+ double progress_1 =
+ static_cast<double>(bytes_received_1) / static_cast<double>(bytes_total);
+ double progress_2 =
+ static_cast<double>(bytes_received_2) / static_cast<double>(bytes_total);
+
+ EXPECT_EQ(0.0, attempter_.download_progress_);
+ // This is set via inspecting the InstallPlan payloads when the
+ // OmahaResponseAction is completed
+ attempter_.new_payload_size_ = bytes_total;
+ NiceMock<MockServiceObserver> observer;
+ EXPECT_CALL(observer,
+ SendStatusUpdate(
+ _, progress_1, UpdateStatus::DOWNLOADING, _, bytes_total));
+ EXPECT_CALL(observer,
+ SendStatusUpdate(
+ _, progress_2, UpdateStatus::DOWNLOADING, _, bytes_total));
+ attempter_.AddObserver(&observer);
+ attempter_.BytesReceived(bytes_progressed_1, bytes_received_1, bytes_total);
+ EXPECT_EQ(progress_1, attempter_.download_progress_);
+ // This iteration validates that a later set of updates to the variables are
+ // properly handled (so that |getStatus()| will return the same progress info
+ // as the callback is receiving.
+ attempter_.BytesReceived(bytes_progressed_2, bytes_received_2, bytes_total);
+ EXPECT_EQ(progress_2, attempter_.download_progress_);
+}
+
+TEST_F(UpdateAttempterTest, ChangeToDownloadingOnReceivedBytesTest) {
+ // The transition into UpdateStatus::DOWNLOADING happens when the
+ // first bytes are received.
+ uint64_t bytes_progressed = 1024 * 1024; // 1MB
+ uint64_t bytes_received = 2 * 1024 * 1024; // 2MB
+ uint64_t bytes_total = 20 * 1024 * 1024; // 300MB
+ attempter_.status_ = UpdateStatus::CHECKING_FOR_UPDATE;
+ // This is set via inspecting the InstallPlan payloads when the
+ // OmahaResponseAction is completed
+ attempter_.new_payload_size_ = bytes_total;
+ EXPECT_EQ(0.0, attempter_.download_progress_);
+ NiceMock<MockServiceObserver> observer;
+ EXPECT_CALL(
+ observer,
+ SendStatusUpdate(_, _, UpdateStatus::DOWNLOADING, _, bytes_total));
+ attempter_.AddObserver(&observer);
+ attempter_.BytesReceived(bytes_progressed, bytes_received, bytes_total);
+ EXPECT_EQ(UpdateStatus::DOWNLOADING, attempter_.status_);
+}
+
+TEST_F(UpdateAttempterTest, BroadcastCompleteDownloadTest) {
+ // There is a special case to ensure that at 100% downloaded,
+ // download_progress_ is updated and that value broadcast. This test confirms
+ // that.
+ uint64_t bytes_progressed = 0; // ignored
+ uint64_t bytes_received = 5 * 1024 * 1024; // ignored
+ uint64_t bytes_total = 5 * 1024 * 1024; // 300MB
+ attempter_.status_ = UpdateStatus::DOWNLOADING;
+ attempter_.new_payload_size_ = bytes_total;
+ EXPECT_EQ(0.0, attempter_.download_progress_);
+ NiceMock<MockServiceObserver> observer;
+ EXPECT_CALL(
+ observer,
+ SendStatusUpdate(_, 1.0, UpdateStatus::DOWNLOADING, _, bytes_total));
+ attempter_.AddObserver(&observer);
+ attempter_.BytesReceived(bytes_progressed, bytes_received, bytes_total);
+ EXPECT_EQ(1.0, attempter_.download_progress_);
+}
+
TEST_F(UpdateAttempterTest, ActionCompletedOmahaRequestTest) {
unique_ptr<MockHttpFetcher> fetcher(new MockHttpFetcher("", 0, nullptr));
fetcher->FailTransfer(500); // Sets the HTTP response code.