Always powerwash on channel change if arbitrary channel allowed.
Merged is_powerwash_allowed() and to_more_stable_channel() into
ShouldPowerwash().
Bug: 73082835
Test: update_engine_unittests
Change-Id: I6b7af0d1dac28d5fa9cddf4391bbd9cdf2acb57b
diff --git a/mock_omaha_request_params.h b/mock_omaha_request_params.h
index 5d5d47b..6d8d3d8 100644
--- a/mock_omaha_request_params.h
+++ b/mock_omaha_request_params.h
@@ -32,9 +32,6 @@
// Delegate all calls to the parent instance by default. This helps the
// migration from tests using the real RequestParams when they should have
// use a fake or mock.
- ON_CALL(*this, to_more_stable_channel())
- .WillByDefault(testing::Invoke(
- this, &MockOmahaRequestParams::fake_to_more_stable_channel));
ON_CALL(*this, GetAppId())
.WillByDefault(testing::Invoke(
this, &MockOmahaRequestParams::FakeGetAppId));
@@ -44,27 +41,22 @@
ON_CALL(*this, UpdateDownloadChannel())
.WillByDefault(testing::Invoke(
this, &MockOmahaRequestParams::FakeUpdateDownloadChannel));
- ON_CALL(*this, is_powerwash_allowed())
+ ON_CALL(*this, ShouldPowerwash())
.WillByDefault(testing::Invoke(
- this, &MockOmahaRequestParams::fake_is_powerwash_allowed));
+ this, &MockOmahaRequestParams::FakeShouldPowerwash));
}
- MOCK_CONST_METHOD0(to_more_stable_channel, bool(void));
MOCK_CONST_METHOD0(GetAppId, std::string(void));
MOCK_METHOD3(SetTargetChannel, bool(const std::string& channel,
bool is_powerwash_allowed,
std::string* error));
MOCK_METHOD0(UpdateDownloadChannel, void(void));
- MOCK_CONST_METHOD0(is_powerwash_allowed, bool(void));
MOCK_CONST_METHOD0(IsUpdateUrlOfficial, bool(void));
+ MOCK_CONST_METHOD0(ShouldPowerwash, bool(void));
private:
// Wrappers to call the parent class and behave like the real object by
// default. See "Delegating Calls to a Parent Class" in gmock's documentation.
- bool fake_to_more_stable_channel() const {
- return OmahaRequestParams::to_more_stable_channel();
- }
-
std::string FakeGetAppId() const {
return OmahaRequestParams::GetAppId();
}
@@ -81,8 +73,8 @@
return OmahaRequestParams::UpdateDownloadChannel();
}
- bool fake_is_powerwash_allowed() const {
- return OmahaRequestParams::is_powerwash_allowed();
+ bool FakeShouldPowerwash() const {
+ return OmahaRequestParams::ShouldPowerwash();
}
};
diff --git a/omaha_request_action.cc b/omaha_request_action.cc
index a8b8a94..d58612c 100644
--- a/omaha_request_action.cc
+++ b/omaha_request_action.cc
@@ -239,9 +239,7 @@
// If we are downgrading to a more stable channel and we are allowed to do
// powerwash, then pass 0.0.0.0 as the version. This is needed to get the
// highest-versioned payload on the destination channel.
- bool is_potential_downgrade =
- params->to_more_stable_channel() && params->is_powerwash_allowed();
- if (is_potential_downgrade) {
+ if (params->ShouldPowerwash()) {
LOG(INFO) << "Passing OS version as 0.0.0.0 as we are set to powerwash "
<< "on downgrading to the version in the more stable channel";
app_versions = "version=\"0.0.0.0\" from_version=\"" +
@@ -291,7 +289,7 @@
}
string product_components_args;
- if (!is_potential_downgrade && !app_data.product_components.empty()) {
+ if (!params->ShouldPowerwash() && !app_data.product_components.empty()) {
brillo::KeyValueStore store;
if (store.LoadFromString(app_data.product_components)) {
for (const string& key : store.GetKeys()) {
diff --git a/omaha_request_action_unittest.cc b/omaha_request_action_unittest.cc
index 87dc404..2f466dd 100644
--- a/omaha_request_action_unittest.cc
+++ b/omaha_request_action_unittest.cc
@@ -2188,8 +2188,7 @@
params.set_current_channel("canary-channel");
EXPECT_TRUE(params.SetTargetChannel("stable-channel", true, nullptr));
params.UpdateDownloadChannel();
- EXPECT_TRUE(params.to_more_stable_channel());
- EXPECT_TRUE(params.is_powerwash_allowed());
+ EXPECT_TRUE(params.ShouldPowerwash());
ASSERT_FALSE(TestUpdateCheck(¶ms,
"invalid xml>",
-1,
@@ -2223,8 +2222,7 @@
params.set_current_channel("stable-channel");
EXPECT_TRUE(params.SetTargetChannel("canary-channel", false, nullptr));
params.UpdateDownloadChannel();
- EXPECT_FALSE(params.to_more_stable_channel());
- EXPECT_FALSE(params.is_powerwash_allowed());
+ EXPECT_FALSE(params.ShouldPowerwash());
ASSERT_FALSE(TestUpdateCheck(¶ms,
"invalid xml>",
-1,
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index 8301d7b..9e78a93 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -222,18 +222,24 @@
return -1;
}
-bool OmahaRequestParams::to_more_stable_channel() const {
+bool OmahaRequestParams::ToMoreStableChannel() const {
int current_channel_index = GetChannelIndex(image_props_.current_channel);
int download_channel_index = GetChannelIndex(download_channel_);
- // If any of the two channels are arbitrary channels, stability is unknown, so
- // always powerwash if allowed.
- if (current_channel_index < 0 || download_channel_index < 0)
- return true;
-
return download_channel_index > current_channel_index;
}
+bool OmahaRequestParams::ShouldPowerwash() const {
+ if (!mutable_image_props_.is_powerwash_allowed)
+ return false;
+ // If arbitrary channels are allowed, always powerwash on channel change.
+ if (image_props_.allow_arbitrary_channels)
+ return image_props_.current_channel != download_channel_;
+ // Otherwise only powerwash if we are moving from less stable (higher version)
+ // to more stable channel (lower version).
+ return ToMoreStableChannel();
+}
+
string OmahaRequestParams::GetAppId() const {
return download_channel_ == "canary-channel" ? image_props_.canary_product_id
: image_props_.product_id;
diff --git a/omaha_request_params.h b/omaha_request_params.h
index a9215ae..60619f9 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -198,10 +198,6 @@
return max_update_checks_allowed_;
}
- // True if we're trying to update to a more stable channel.
- // i.e. index(target_channel) > index(current_channel).
- virtual bool to_more_stable_channel() const;
-
// Returns the app id corresponding to the current value of the
// download channel.
virtual std::string GetAppId() const;
@@ -238,9 +234,8 @@
// or Init is called again.
virtual void UpdateDownloadChannel();
- virtual bool is_powerwash_allowed() const {
- return mutable_image_props_.is_powerwash_allowed;
- }
+ // Returns whether we should powerwash for this update.
+ virtual bool ShouldPowerwash() const;
// Check if the provided update URL is official, meaning either the default
// autoupdate server or the autoupdate autotest server.
@@ -259,7 +254,10 @@
FRIEND_TEST(OmahaRequestParamsTest, ChannelIndexTest);
FRIEND_TEST(OmahaRequestParamsTest, CollectECFWVersionsTest);
FRIEND_TEST(OmahaRequestParamsTest, IsValidChannelTest);
+ FRIEND_TEST(OmahaRequestParamsTest, SetIsPowerwashAllowedTest);
FRIEND_TEST(OmahaRequestParamsTest, SetTargetChannelInvalidTest);
+ FRIEND_TEST(OmahaRequestParamsTest, SetTargetChannelTest);
+ FRIEND_TEST(OmahaRequestParamsTest, ShouldPowerwashTest);
FRIEND_TEST(OmahaRequestParamsTest, ToMoreStableChannelFlagTest);
// Returns true if |channel| is a valid channel, otherwise write error to
@@ -273,6 +271,10 @@
// Returns the index of the given channel.
int GetChannelIndex(const std::string& channel) const;
+ // True if we're trying to update to a more stable channel.
+ // i.e. index(target_channel) > index(current_channel).
+ bool ToMoreStableChannel() const;
+
// Returns True if we should store the fw/ec versions based on our hwid_.
// Compares hwid to a set of whitelisted prefixes.
bool CollectECFWVersions() const;
diff --git a/omaha_request_params_unittest.cc b/omaha_request_params_unittest.cc
index 2de45de..ce77f31 100644
--- a/omaha_request_params_unittest.cc
+++ b/omaha_request_params_unittest.cc
@@ -117,12 +117,12 @@
params.set_root(tempdir_.GetPath().value());
EXPECT_TRUE(params.Init("", "", false));
EXPECT_TRUE(params.SetTargetChannel("canary-channel", false, nullptr));
- EXPECT_FALSE(params.is_powerwash_allowed());
+ EXPECT_FALSE(params.mutable_image_props_.is_powerwash_allowed);
}
params_.set_root(tempdir_.GetPath().value());
EXPECT_TRUE(params_.Init("", "", false));
EXPECT_EQ("canary-channel", params_.target_channel());
- EXPECT_FALSE(params_.is_powerwash_allowed());
+ EXPECT_FALSE(params_.mutable_image_props_.is_powerwash_allowed);
}
TEST_F(OmahaRequestParamsTest, SetIsPowerwashAllowedTest) {
@@ -131,12 +131,12 @@
params.set_root(tempdir_.GetPath().value());
EXPECT_TRUE(params.Init("", "", false));
EXPECT_TRUE(params.SetTargetChannel("canary-channel", true, nullptr));
- EXPECT_TRUE(params.is_powerwash_allowed());
+ EXPECT_TRUE(params.mutable_image_props_.is_powerwash_allowed);
}
params_.set_root(tempdir_.GetPath().value());
EXPECT_TRUE(params_.Init("", "", false));
EXPECT_EQ("canary-channel", params_.target_channel());
- EXPECT_TRUE(params_.is_powerwash_allowed());
+ EXPECT_TRUE(params_.mutable_image_props_.is_powerwash_allowed);
}
TEST_F(OmahaRequestParamsTest, SetTargetChannelInvalidTest) {
@@ -151,12 +151,12 @@
params.SetTargetChannel("dogfood-channel", true, &error_message));
// The error message should include a message about the valid channels.
EXPECT_NE(string::npos, error_message.find("stable-channel"));
- EXPECT_FALSE(params.is_powerwash_allowed());
+ EXPECT_FALSE(params.mutable_image_props_.is_powerwash_allowed);
}
params_.set_root(tempdir_.GetPath().value());
EXPECT_TRUE(params_.Init("", "", false));
EXPECT_EQ("stable-channel", params_.target_channel());
- EXPECT_FALSE(params_.is_powerwash_allowed());
+ EXPECT_FALSE(params_.mutable_image_props_.is_powerwash_allowed);
}
TEST_F(OmahaRequestParamsTest, IsValidChannelTest) {
@@ -232,13 +232,25 @@
TEST_F(OmahaRequestParamsTest, ToMoreStableChannelFlagTest) {
params_.image_props_.current_channel = "canary-channel";
params_.download_channel_ = "stable-channel";
- EXPECT_TRUE(params_.to_more_stable_channel());
+ EXPECT_TRUE(params_.ToMoreStableChannel());
params_.image_props_.current_channel = "stable-channel";
- EXPECT_FALSE(params_.to_more_stable_channel());
+ EXPECT_FALSE(params_.ToMoreStableChannel());
params_.download_channel_ = "beta-channel";
- EXPECT_FALSE(params_.to_more_stable_channel());
- params_.download_channel_ = "some-channel";
- EXPECT_TRUE(params_.to_more_stable_channel());
+ EXPECT_FALSE(params_.ToMoreStableChannel());
+}
+
+TEST_F(OmahaRequestParamsTest, ShouldPowerwashTest) {
+ params_.mutable_image_props_.is_powerwash_allowed = false;
+ EXPECT_FALSE(params_.ShouldPowerwash());
+ params_.mutable_image_props_.is_powerwash_allowed = true;
+ params_.image_props_.allow_arbitrary_channels = true;
+ params_.image_props_.current_channel = "foo-channel";
+ params_.download_channel_ = "bar-channel";
+ EXPECT_TRUE(params_.ShouldPowerwash());
+ params_.image_props_.allow_arbitrary_channels = false;
+ params_.image_props_.current_channel = "canary-channel";
+ params_.download_channel_ = "stable-channel";
+ EXPECT_TRUE(params_.ShouldPowerwash());
}
TEST_F(OmahaRequestParamsTest, CollectECFWVersionsTest) {
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index 9c5fb4a..2d6105a 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -139,7 +139,7 @@
system_state_->prefs()->SetString(current_channel_key,
params->download_channel());
- if (params->to_more_stable_channel() && params->is_powerwash_allowed())
+ if (params->ShouldPowerwash())
install_plan_.powerwash_required = true;
TEST_AND_RETURN(HasOutputPipe());
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 4e9c03f..9e2cdd1 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -393,8 +393,7 @@
#endif // __ANDROID__
EXPECT_TRUE(params.SetTargetChannel("stable-channel", true, nullptr));
params.UpdateDownloadChannel();
- EXPECT_TRUE(params.to_more_stable_channel());
- EXPECT_TRUE(params.is_powerwash_allowed());
+ EXPECT_TRUE(params.ShouldPowerwash());
fake_system_state_.set_request_params(¶ms);
InstallPlan install_plan;
@@ -426,8 +425,7 @@
#endif // __ANDROID__
EXPECT_TRUE(params.SetTargetChannel("canary-channel", false, nullptr));
params.UpdateDownloadChannel();
- EXPECT_FALSE(params.to_more_stable_channel());
- EXPECT_FALSE(params.is_powerwash_allowed());
+ EXPECT_FALSE(params.ShouldPowerwash());
fake_system_state_.set_request_params(¶ms);
InstallPlan install_plan;