update_engine: Pospone setgoodkernel after update check
Currently, if an update is forced, we first run setgoodkernel and then continue
with the update check. However, this delays the update check screen in the
OOBE. This patch adds a new Action, UpdateBootFlagsAction which is configured to
run AFTER an update check and response. However, now if the update check failed
due for example the response was invalid, then setgoodkernel will not
happen. But we already have a mechanism in SystemState where setgoodkernel is
always called (currently) 45 seconds after the boot. So there would no worries.
BUG=chromium:807976
TEST=unittests
TEST=powerwashed the device, and the check for update passed faster on OOBE.
Change-Id: I5845d7a4c1393aec568b6279dfbb7d3dfa31cdd8
Reviewed-on: https://chromium-review.googlesource.com/1011244
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>
diff --git a/Android.mk b/Android.mk
index acb98d0..1f0397b 100644
--- a/Android.mk
+++ b/Android.mk
@@ -318,6 +318,7 @@
proxy_resolver.cc \
real_system_state.cc \
update_attempter.cc \
+ update_boot_flags_action.cc \
update_manager/android_things_policy.cc \
update_manager/api_restricted_downloads_policy_impl.cc \
update_manager/boxed_value.cc \
@@ -1001,6 +1002,7 @@
payload_state_unittest.cc \
parcelable_update_engine_status_unittest.cc \
update_attempter_unittest.cc \
+ update_boot_flags_action_unittest.cc \
update_manager/android_things_policy_unittest.cc \
update_manager/boxed_value_unittest.cc \
update_manager/chromeos_policy.cc \
diff --git a/real_system_state.cc b/real_system_state.cc
index 8f4c731..9dab3a1 100644
--- a/real_system_state.cc
+++ b/real_system_state.cc
@@ -18,6 +18,7 @@
#include <memory>
#include <string>
+#include <utility>
#include <base/bind.h>
#include <base/files/file_util.h>
@@ -34,6 +35,7 @@
#include "update_engine/common/hardware.h"
#include "update_engine/common/utils.h"
#include "update_engine/metrics_reporter_omaha.h"
+#include "update_engine/update_boot_flags_action.h"
#if USE_DBUS
#include "update_engine/dbus_connection.h"
#endif // USE_DBUS
@@ -183,11 +185,14 @@
// Initiate update checks.
update_attempter_->ScheduleUpdates();
+ auto update_boot_flags_action =
+ std::make_unique<UpdateBootFlagsAction>(boot_control_.get());
+ processor_.EnqueueAction(std::move(update_boot_flags_action));
// Update boot flags after 45 seconds.
MessageLoop::current()->PostDelayedTask(
FROM_HERE,
- base::Bind(&UpdateAttempter::UpdateBootFlags,
- base::Unretained(update_attempter_.get())),
+ base::Bind(&ActionProcessor::StartProcessing,
+ base::Unretained(&processor_)),
base::TimeDelta::FromSeconds(45));
// Broadcast the update engine status on startup to ensure consistent system
diff --git a/real_system_state.h b/real_system_state.h
index f1cd38f..5239160 100644
--- a/real_system_state.h
+++ b/real_system_state.h
@@ -185,6 +185,8 @@
// rebooted. Important for tracking whether you are running instance of the
// update engine on first boot or due to a crash/restart.
bool system_rebooted_{false};
+
+ ActionProcessor processor_;
};
} // namespace chromeos_update_engine
diff --git a/update_attempter.cc b/update_attempter.cc
index 4f45a2a..517b0a8 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -61,6 +61,7 @@
#include "update_engine/payload_state_interface.h"
#include "update_engine/power_manager_interface.h"
#include "update_engine/system_state.h"
+#include "update_engine/update_boot_flags_action.h"
#include "update_engine/update_manager/policy.h"
#include "update_engine/update_manager/policy_utils.h"
#include "update_engine/update_manager/update_manager.h"
@@ -295,10 +296,7 @@
// checks in the case where a response is not received.
UpdateLastCheckedTime();
- // Just in case we didn't update boot flags yet, make sure they're updated
- // before any update processing starts.
- start_action_processor_ = true;
- UpdateBootFlags();
+ ScheduleProcessingStart();
}
void UpdateAttempter::RefreshDevicePolicy() {
@@ -597,6 +595,8 @@
system_state_, nullptr, std::move(update_check_fetcher), false);
auto response_handler_action =
std::make_unique<OmahaResponseHandlerAction>(system_state_);
+ auto update_boot_flags_action =
+ std::make_unique<UpdateBootFlagsAction>(system_state_->boot_control());
auto download_started_action = std::make_unique<OmahaRequestAction>(
system_state_,
new OmahaEvent(OmahaEvent::kTypeUpdateDownloadStarted),
@@ -647,6 +647,7 @@
processor_->EnqueueAction(std::move(update_check_action));
processor_->EnqueueAction(std::move(response_handler_action));
+ processor_->EnqueueAction(std::move(update_boot_flags_action));
processor_->EnqueueAction(std::move(download_started_action));
processor_->EnqueueAction(std::move(download_action));
processor_->EnqueueAction(std::move(download_finished_action));
@@ -708,11 +709,7 @@
SetStatusAndNotify(UpdateStatus::ATTEMPTING_ROLLBACK);
- // Just in case we didn't update boot flags yet, make sure they're updated
- // before any update processing starts. This also schedules the start of the
- // actions we just posted.
- start_action_processor_ = true;
- UpdateBootFlags();
+ ScheduleProcessingStart();
return true;
}
@@ -1223,38 +1220,6 @@
return true;
}
-void UpdateAttempter::UpdateBootFlags() {
- if (update_boot_flags_running_) {
- LOG(INFO) << "Update boot flags running, nothing to do.";
- return;
- }
- if (updated_boot_flags_) {
- LOG(INFO) << "Already updated boot flags. Skipping.";
- if (start_action_processor_) {
- ScheduleProcessingStart();
- }
- return;
- }
- // This is purely best effort. Failures should be logged by Subprocess. Run
- // the script asynchronously to avoid blocking the event loop regardless of
- // the script runtime.
- update_boot_flags_running_ = true;
- LOG(INFO) << "Marking booted slot as good.";
- if (!system_state_->boot_control()->MarkBootSuccessfulAsync(Bind(
- &UpdateAttempter::CompleteUpdateBootFlags, base::Unretained(this)))) {
- LOG(ERROR) << "Failed to mark current boot as successful.";
- CompleteUpdateBootFlags(false);
- }
-}
-
-void UpdateAttempter::CompleteUpdateBootFlags(bool successful) {
- update_boot_flags_running_ = false;
- updated_boot_flags_ = true;
- if (start_action_processor_) {
- ScheduleProcessingStart();
- }
-}
-
void UpdateAttempter::BroadcastStatus() {
UpdateEngineStatus broadcast_status;
// Use common method for generating the current status.
@@ -1372,7 +1337,6 @@
void UpdateAttempter::ScheduleProcessingStart() {
LOG(INFO) << "Scheduling an action processor start.";
- start_action_processor_ = false;
MessageLoop::current()->PostTask(
FROM_HERE,
Bind([](ActionProcessor* processor) { processor->StartProcessing(); },
diff --git a/update_attempter.h b/update_attempter.h
index 324f549..a405abc 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -480,20 +480,6 @@
ChromeBrowserProxyResolver chrome_proxy_resolver_;
#endif // USE_CHROME_NETWORK_PROXY
- // Originally, both of these flags are false. Once UpdateBootFlags is called,
- // |update_boot_flags_running_| is set to true. As soon as UpdateBootFlags
- // completes its asynchronous run, |update_boot_flags_running_| is reset to
- // false and |updated_boot_flags_| is set to true. From that point on there
- // will be no more changes to these flags.
- //
- // True if UpdateBootFlags has completed.
- bool updated_boot_flags_ = false;
- // True if UpdateBootFlags is running.
- bool update_boot_flags_running_ = false;
-
- // True if the action processor needs to be started by the boot flag updater.
- bool start_action_processor_ = false;
-
// Used for fetching information about the device policy.
std::unique_ptr<policy::PolicyProvider> policy_provider_;
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 4af5f8b..0267953 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -49,6 +49,7 @@
#include "update_engine/payload_consumer/install_plan.h"
#include "update_engine/payload_consumer/payload_constants.h"
#include "update_engine/payload_consumer/postinstall_runner_action.h"
+#include "update_engine/update_boot_flags_action.h"
using base::Time;
using base::TimeDelta;
@@ -442,15 +443,15 @@
namespace {
// Actions that will be built as part of an update check.
const string kUpdateActionTypes[] = { // NOLINT(runtime/string)
- OmahaRequestAction::StaticType(),
- OmahaResponseHandlerAction::StaticType(),
- OmahaRequestAction::StaticType(),
- DownloadAction::StaticType(),
- OmahaRequestAction::StaticType(),
- FilesystemVerifierAction::StaticType(),
- PostinstallRunnerAction::StaticType(),
- OmahaRequestAction::StaticType()
-};
+ OmahaRequestAction::StaticType(),
+ OmahaResponseHandlerAction::StaticType(),
+ UpdateBootFlagsAction::StaticType(),
+ OmahaRequestAction::StaticType(),
+ DownloadAction::StaticType(),
+ OmahaRequestAction::StaticType(),
+ FilesystemVerifierAction::StaticType(),
+ PostinstallRunnerAction::StaticType(),
+ OmahaRequestAction::StaticType()};
// Actions that will be built as part of a user-initiated rollback.
const string kRollbackActionTypes[] = { // NOLINT(runtime/string)
diff --git a/update_boot_flags_action.cc b/update_boot_flags_action.cc
new file mode 100644
index 0000000..97ef7f2
--- /dev/null
+++ b/update_boot_flags_action.cc
@@ -0,0 +1,68 @@
+//
+// Copyright (C) 2018 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.
+//
+
+#include "update_engine/update_boot_flags_action.h"
+
+#include <base/bind.h>
+#include <base/logging.h>
+
+#include "update_engine/common/boot_control.h"
+
+namespace chromeos_update_engine {
+
+bool UpdateBootFlagsAction::updated_boot_flags_ = false;
+bool UpdateBootFlagsAction::is_running_ = false;
+
+void UpdateBootFlagsAction::PerformAction() {
+ if (is_running_) {
+ LOG(INFO) << "Update boot flags running, nothing to do.";
+ processor_->ActionComplete(this, ErrorCode::kSuccess);
+ return;
+ }
+ if (updated_boot_flags_) {
+ LOG(INFO) << "Already updated boot flags. Skipping.";
+ processor_->ActionComplete(this, ErrorCode::kSuccess);
+ return;
+ }
+
+ // This is purely best effort. Failures should be logged by Subprocess. Run
+ // the script asynchronously to avoid blocking the event loop regardless of
+ // the script runtime.
+ is_running_ = true;
+ LOG(INFO) << "Marking booted slot as good.";
+ if (!boot_control_->MarkBootSuccessfulAsync(
+ base::Bind(&UpdateBootFlagsAction::CompleteUpdateBootFlags,
+ base::Unretained(this)))) {
+ CompleteUpdateBootFlags(false);
+ }
+}
+
+void UpdateBootFlagsAction::CompleteUpdateBootFlags(bool successful) {
+ is_running_ = false;
+ if (!successful) {
+ // We ignore the failure for now because if the updating boot flags is flaky
+ // or has a bug in a specific release, then blocking the update can cause
+ // devices to stay behind even though we could have updated the system and
+ // fixed the issue regardless of this failure.
+ //
+ // TODO(ahassani): Add new error code metric for kUpdateBootFlagsFailed.
+ LOG(ERROR) << "Updating boot flags failed, but ignoring its failure.";
+ }
+ updated_boot_flags_ = true;
+ processor_->ActionComplete(this, ErrorCode::kSuccess);
+}
+
+} // namespace chromeos_update_engine
diff --git a/update_boot_flags_action.h b/update_boot_flags_action.h
new file mode 100644
index 0000000..0d1125e
--- /dev/null
+++ b/update_boot_flags_action.h
@@ -0,0 +1,57 @@
+//
+// Copyright (C) 2018 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.
+//
+
+#include <string>
+
+#include "update_engine/common/action.h"
+#include "update_engine/common/boot_control_interface.h"
+
+namespace chromeos_update_engine {
+
+class UpdateBootFlagsAction : public AbstractAction {
+ public:
+ explicit UpdateBootFlagsAction(BootControlInterface* boot_control)
+ : boot_control_(boot_control) {}
+
+ void PerformAction() override;
+
+ static std::string StaticType() { return "UpdateBootFlagsAction"; }
+ std::string Type() const override { return StaticType(); }
+
+ void CompleteUpdateBootFlags(bool successful);
+
+ private:
+ FRIEND_TEST(UpdateBootFlagsActionTest, SimpleTest);
+ FRIEND_TEST(UpdateBootFlagsActionTest, DoubleActionTest);
+
+ // Originally, both of these flags are false. Once UpdateBootFlags is called,
+ // |is_running_| is set to true. As soon as UpdateBootFlags completes its
+ // asynchronous run, |is_running_| is reset to false and |updated_boot_flags_|
+ // is set to true. From that point on there will be no more changes to these
+ // flags.
+ //
+ // True if have updated the boot flags.
+ static bool updated_boot_flags_;
+ // True if we are still updating the boot flags.
+ static bool is_running_;
+
+ // Used for setting the boot flag.
+ BootControlInterface* boot_control_;
+
+ DISALLOW_COPY_AND_ASSIGN(UpdateBootFlagsAction);
+};
+
+} // namespace chromeos_update_engine
diff --git a/update_boot_flags_action_unittest.cc b/update_boot_flags_action_unittest.cc
new file mode 100644
index 0000000..1b2bfa5
--- /dev/null
+++ b/update_boot_flags_action_unittest.cc
@@ -0,0 +1,69 @@
+//
+// Copyright (C) 2018 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.
+//
+
+#include "update_engine/update_boot_flags_action.h"
+
+#include <memory>
+#include <utility>
+
+#include <base/bind.h>
+#include <gtest/gtest.h>
+
+#include "update_engine/fake_system_state.h"
+
+namespace chromeos_update_engine {
+
+class UpdateBootFlagsActionTest : public ::testing::Test {
+ public:
+ FakeSystemState fake_system_state_;
+};
+
+TEST_F(UpdateBootFlagsActionTest, SimpleTest) {
+ auto boot_control = fake_system_state_.fake_boot_control();
+ auto action = std::make_unique<UpdateBootFlagsAction>(boot_control);
+ ActionProcessor processor;
+ processor.EnqueueAction(std::move(action));
+
+ EXPECT_FALSE(UpdateBootFlagsAction::updated_boot_flags_);
+ EXPECT_FALSE(UpdateBootFlagsAction::is_running_);
+ processor.StartProcessing();
+ EXPECT_TRUE(UpdateBootFlagsAction::updated_boot_flags_);
+ EXPECT_FALSE(UpdateBootFlagsAction::is_running_);
+}
+
+TEST_F(UpdateBootFlagsActionTest, DoubleActionTest) {
+ // Reset the static flags.
+ UpdateBootFlagsAction::updated_boot_flags_ = false;
+ UpdateBootFlagsAction::is_running_ = false;
+
+ auto boot_control = fake_system_state_.fake_boot_control();
+ auto action1 = std::make_unique<UpdateBootFlagsAction>(boot_control);
+ auto action2 = std::make_unique<UpdateBootFlagsAction>(boot_control);
+ ActionProcessor processor1, processor2;
+ processor1.EnqueueAction(std::move(action1));
+ processor2.EnqueueAction(std::move(action2));
+
+ EXPECT_FALSE(UpdateBootFlagsAction::updated_boot_flags_);
+ EXPECT_FALSE(UpdateBootFlagsAction::is_running_);
+ processor1.StartProcessing();
+ EXPECT_TRUE(UpdateBootFlagsAction::updated_boot_flags_);
+ EXPECT_FALSE(UpdateBootFlagsAction::is_running_);
+ processor2.StartProcessing();
+ EXPECT_TRUE(UpdateBootFlagsAction::updated_boot_flags_);
+ EXPECT_FALSE(UpdateBootFlagsAction::is_running_);
+}
+
+} // namespace chromeos_update_engine
diff --git a/update_engine.gyp b/update_engine.gyp
index b09a401..20fb282 100644
--- a/update_engine.gyp
+++ b/update_engine.gyp
@@ -273,6 +273,7 @@
'real_system_state.cc',
'shill_proxy.cc',
'update_attempter.cc',
+ 'update_boot_flags_action.cc',
'update_manager/boxed_value.cc',
'update_manager/chromeos_policy.cc',
'update_manager/default_policy.cc',
@@ -568,6 +569,7 @@
'proxy_resolver_unittest.cc',
'testrunner.cc',
'update_attempter_unittest.cc',
+ 'update_boot_flags_action_unittest.cc',
'update_manager/boxed_value_unittest.cc',
'update_manager/chromeos_policy_unittest.cc',
'update_manager/evaluation_context_unittest.cc',