From 7b36696a41b6400e069db857ed4a554d32624d7d Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Tue, 19 May 2020 11:18:54 +0200 Subject: [PATCH 1/5] Do not send the manifest as part of device data. There's no reason to; it is always sent as part of checking for updates, and there is never a time that device data is sent that isn't immediately followed by checking for updates. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/aktualizr_test.cc | 2 +- src/libaktualizr/primary/sotauptaneclient.cc | 1 - src/libaktualizr/uptane/uptane_test.cc | 7 +++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libaktualizr/primary/aktualizr_test.cc b/src/libaktualizr/primary/aktualizr_test.cc index 6c550c4684..1f92a61307 100644 --- a/src/libaktualizr/primary/aktualizr_test.cc +++ b/src/libaktualizr/primary/aktualizr_test.cc @@ -1176,7 +1176,7 @@ TEST(Aktualizr, AutoRebootAfterUpdate) { storage->loadPrimaryInstalledVersions(¤t_target, &pending_target); EXPECT_TRUE(!!current_target); EXPECT_FALSE(!!pending_target); - EXPECT_EQ(http->manifest_sends, 4); + EXPECT_EQ(http->manifest_sends, 3); } } diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 3b5c646de8..15cc6546d2 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -766,7 +766,6 @@ void SotaUptaneClient::sendDeviceData(const Json::Value &custom_hwinfo) { reportInstalledPackages(); reportNetworkInfo(); reportAktualizrConfiguration(); - putManifestSimple(); sendEvent(); } diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index 914c45b180..5fd45bbd42 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -890,7 +890,7 @@ class HttpFakeProv : public HttpFake { std::string file_secondary; std::string hash_primary; std::string hash_secondary; - if (manifest_count <= 2) { + if (manifest_count <= 1) { file_primary = "unknown"; file_secondary = "noimage"; // Check for default initial value of packagemanagerfake. @@ -990,14 +990,13 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->ecus_count, 1); EXPECT_NO_THROW(up->sendDeviceData()); - EXPECT_EQ(http->manifest_count, 1); EXPECT_EQ(http->installed_count, 1); EXPECT_EQ(http->system_info_count, 1); EXPECT_EQ(http->network_count, 1); result::UpdateCheck update_result = up->fetchMeta(); EXPECT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable); - EXPECT_EQ(http->manifest_count, 2); + EXPECT_EQ(http->manifest_count, 1); // Test installation to make sure the metadata put to the server is correct. result::Download download_result = up->downloadImages(update_result.updates); @@ -1009,7 +1008,7 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->devices_count, 1); EXPECT_EQ(http->ecus_count, 1); - EXPECT_EQ(http->manifest_count, 3); + EXPECT_EQ(http->manifest_count, 2); EXPECT_EQ(http->installed_count, 1); EXPECT_EQ(http->system_info_count, 1); EXPECT_EQ(http->network_count, 1); From 0c22dba788c43e1a04ace5bc1e024e061a586c93 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Tue, 19 May 2020 12:03:57 +0200 Subject: [PATCH 2/5] Test sending libaktualizr configuration. Signed-off-by: Patrick Vacek --- src/libaktualizr/uptane/uptane_test.cc | 27 +++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index 5fd45bbd42..4a164712ec 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -774,8 +774,24 @@ TEST(Uptane, UptaneSecondaryMisconfigured) { */ class HttpFakeProv : public HttpFake { public: - HttpFakeProv(const boost::filesystem::path &test_dir_in, std::string flavor = "") - : HttpFake(test_dir_in, std::move(flavor)) {} + HttpFakeProv(const boost::filesystem::path &test_dir_in, std::string flavor, Config &config_in) + : HttpFake(test_dir_in, std::move(flavor)), config(config_in) {} + + HttpResponse post(const std::string &url, const std::string &content_type, const std::string &data) override { + std::cout << "post " << url << "\n"; + + if (url.find("/system_info/config") != std::string::npos) { + /* Send libaktualizr configuration to the server. */ + config_count++; + std::stringstream conf_ss; + config.writeToStream(conf_ss); + EXPECT_EQ(data, conf_ss.str()); + EXPECT_EQ(content_type, "application/toml"); + } else { + EXPECT_EQ(0, 1) << "Unexpected post to URL: " << url; + } + return HttpFake::post(url, content_type, data); + } HttpResponse post(const std::string &url, const Json::Value &data) override { std::cout << "post " << url << "\n"; @@ -940,8 +956,10 @@ class HttpFakeProv : public HttpFake { int installed_count{0}; int system_info_count{0}; int network_count{0}; + int config_count{0}; private: + Config &config; int primary_download_start{0}; int primary_download_complete{0}; int secondary_download_start{0}; @@ -957,7 +975,7 @@ TEST(Uptane, ProvisionOnServer) { RecordProperty("zephyr_key", "OTA-984,TST-149"); TemporaryDirectory temp_dir; Config config("tests/config/basic.toml"); - auto http = std::make_shared(temp_dir.Path(), "hasupdates"); + auto http = std::make_shared(temp_dir.Path(), "hasupdates", config); const std::string &server = http->tls_server; config.provision.server = server; config.tls.server = server; @@ -980,6 +998,7 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->installed_count, 0); EXPECT_EQ(http->system_info_count, 0); EXPECT_EQ(http->network_count, 0); + EXPECT_EQ(http->config_count, 0); EXPECT_NO_THROW(up->initialize()); EcuSerials serials; @@ -993,6 +1012,7 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->installed_count, 1); EXPECT_EQ(http->system_info_count, 1); EXPECT_EQ(http->network_count, 1); + EXPECT_EQ(http->config_count, 1); result::UpdateCheck update_result = up->fetchMeta(); EXPECT_EQ(update_result.status, result::UpdateStatus::kUpdatesAvailable); @@ -1012,6 +1032,7 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->installed_count, 1); EXPECT_EQ(http->system_info_count, 1); EXPECT_EQ(http->network_count, 1); + EXPECT_EQ(http->config_count, 1); EXPECT_EQ(http->events_seen, 8); } From 2b19fe0ef1a5f97dd13c062115463af4c2a8e8b9 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Tue, 19 May 2020 15:16:31 +0200 Subject: [PATCH 3/5] Only send device data if it has changed. We now stored hashes of the hardware/system information, list of installed packages, network information, and configuration in the database. The hardware and network data were previously stored locally in SotaUptaneClient, but this persists over restarts, so it saves even more bandwidth. Note that at least on my laptop, one field in the output of lshw changes every time I call it, so the hardware info gets sent every time. Thankfully, I could not reproduce that problem on qemu. Signed-off-by: Patrick Vacek --- config/sql/migration/migrate.25.sql | 9 +++ config/sql/rollback/rollback.25.sql | 9 +++ config/sql/schema.sql | 3 +- src/libaktualizr/primary/sotauptaneclient.cc | 67 +++++++++++++++----- src/libaktualizr/primary/sotauptaneclient.h | 2 - src/libaktualizr/storage/invstorage.h | 4 ++ src/libaktualizr/storage/sqlstorage.cc | 42 ++++++++++++ src/libaktualizr/storage/sqlstorage.h | 4 ++ src/libaktualizr/uptane/uptane_test.cc | 40 ++++++++++-- 9 files changed, 156 insertions(+), 24 deletions(-) create mode 100644 config/sql/migration/migrate.25.sql create mode 100644 config/sql/rollback/rollback.25.sql diff --git a/config/sql/migration/migrate.25.sql b/config/sql/migration/migrate.25.sql new file mode 100644 index 0000000000..cdd03ee7bc --- /dev/null +++ b/config/sql/migration/migrate.25.sql @@ -0,0 +1,9 @@ +-- Don't modify this! Create a new migration instead--see docs/ota-client-guide/modules/ROOT/pages/schema-migrations.adoc +SAVEPOINT MIGRATION; + +CREATE TABLE device_data(data_type TEXT PRIMARY KEY, hash TEXT NOT NULL); + +DELETE FROM version; +INSERT INTO version VALUES(25); + +RELEASE MIGRATION; diff --git a/config/sql/rollback/rollback.25.sql b/config/sql/rollback/rollback.25.sql new file mode 100644 index 0000000000..540f1a4185 --- /dev/null +++ b/config/sql/rollback/rollback.25.sql @@ -0,0 +1,9 @@ +-- Don't modify this! Create a new migration instead--see docs/ota-client-guide/modules/ROOT/pages/schema-migrations.adoc +SAVEPOINT ROLLBACK_MIGRATION; + +DROP TABLE device_data; + +DELETE FROM version; +INSERT INTO version VALUES(24); + +RELEASE ROLLBACK_MIGRATION; diff --git a/config/sql/schema.sql b/config/sql/schema.sql index 6ecb19f868..cf3cf98d77 100644 --- a/config/sql/schema.sql +++ b/config/sql/schema.sql @@ -1,5 +1,5 @@ CREATE TABLE version(version INTEGER); -INSERT INTO version(rowid,version) VALUES(1,24); +INSERT INTO version(rowid,version) VALUES(1,25); CREATE TABLE device_info(unique_mark INTEGER PRIMARY KEY CHECK (unique_mark = 0), device_id TEXT, is_registered INTEGER NOT NULL DEFAULT 0 CHECK (is_registered IN (0,1))); CREATE TABLE ecus(id INTEGER PRIMARY KEY, serial TEXT UNIQUE, hardware_id TEXT NOT NULL, is_primary INTEGER NOT NULL DEFAULT 0 CHECK (is_primary IN (0,1))); CREATE TABLE secondary_ecus(serial TEXT PRIMARY KEY, sec_type TEXT, public_key_type TEXT, public_key TEXT, extra TEXT, manifest TEXT); @@ -26,3 +26,4 @@ CREATE TABLE rollback_migrations(version_from INT PRIMARY KEY, migration TEXT NO CREATE TABLE delegations(meta BLOB NOT NULL, role_name TEXT NOT NULL, UNIQUE(role_name)); CREATE TABLE ecu_report_counter(ecu_serial TEXT NOT NULL PRIMARY KEY, counter INTEGER NOT NULL DEFAULT 0); CREATE TABLE report_events(id INTEGER PRIMARY KEY, json_string TEXT NOT NULL); +CREATE TABLE device_data(data_type TEXT PRIMARY KEY, hash TEXT NOT NULL); diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 15cc6546d2..3ce0e525e1 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -173,43 +173,78 @@ void SotaUptaneClient::reportHwInfo(const Json::Value &custom_hwinfo) { } const Json::Value &hw_info = custom_hwinfo.empty() ? system_info : custom_hwinfo; - if (hw_info != last_hw_info_reported) { - if (http->put(config.tls.server + "/system_info", hw_info).isOk()) { - last_hw_info_reported = hw_info; + const Hash new_hash = Hash::generate(Hash::Type::kSha256, Utils::jsonToCanonicalStr(hw_info)); + std::string stored_hash; + if (!(storage->loadDeviceDataHash("hardware_info", &stored_hash) && + new_hash == Hash(Hash::Type::kSha256, stored_hash))) { + LOG_DEBUG << "Reporting hardware information"; + const HttpResponse response = http->put(config.tls.server + "/system_info", hw_info); + if (response.isOk()) { + storage->storeDeviceDataHash("hardware_info", new_hash.HashString()); } + } else { + LOG_TRACE << "Not reporting hardware information because it has not changed"; } } void SotaUptaneClient::reportInstalledPackages() { - http->put(config.tls.server + "/core/installed", package_manager_->getInstalledPackages()); + const Json::Value packages = package_manager_->getInstalledPackages(); + const Hash new_hash = Hash::generate(Hash::Type::kSha256, Utils::jsonToCanonicalStr(packages)); + std::string stored_hash; + if (!(storage->loadDeviceDataHash("installed_packages", &stored_hash) && + new_hash == Hash(Hash::Type::kSha256, stored_hash))) { + LOG_DEBUG << "Reporting installed packages"; + const HttpResponse response = http->put(config.tls.server + "/core/installed", packages); + if (response.isOk()) { + storage->storeDeviceDataHash("installed_packages", new_hash.HashString()); + } + } else { + LOG_TRACE << "Not reporting installed packages because they have not changed"; + } } void SotaUptaneClient::reportNetworkInfo() { - if (config.telemetry.report_network) { + if (!config.telemetry.report_network) { + LOG_TRACE << "Not reporting network information because telemetry is disabled"; + return; + } + + const Json::Value network_info = Utils::getNetworkInfo(); + const Hash new_hash = Hash::generate(Hash::Type::kSha256, Utils::jsonToCanonicalStr(network_info)); + std::string stored_hash; + if (!(storage->loadDeviceDataHash("network_info", &stored_hash) && + new_hash == Hash(Hash::Type::kSha256, stored_hash))) { LOG_DEBUG << "Reporting network information"; - Json::Value network_info = Utils::getNetworkInfo(); - if (network_info != last_network_info_reported) { - HttpResponse response = http->put(config.tls.server + "/system_info/network", network_info); - if (response.isOk()) { - last_network_info_reported = network_info; - } + const HttpResponse response = http->put(config.tls.server + "/system_info/network", network_info); + if (response.isOk()) { + storage->storeDeviceDataHash("network_info", new_hash.HashString()); } - } else { - LOG_DEBUG << "Not reporting network information because telemetry is disabled"; + LOG_TRACE << "Not reporting network information because it has not changed"; } } void SotaUptaneClient::reportAktualizrConfiguration() { if (!config.telemetry.report_config) { - LOG_DEBUG << "Not reporting network information because telemetry is disabled"; + LOG_TRACE << "Not reporting libaktualizr configuration because telemetry is disabled"; return; } - LOG_DEBUG << "Reporting libaktualizr configuration"; std::stringstream conf_ss; config.writeToStream(conf_ss); - http->post(config.tls.server + "/system_info/config", "application/toml", conf_ss.str()); + const std::string conf_str = conf_ss.str(); + const Hash new_hash = Hash::generate(Hash::Type::kSha256, conf_str); + std::string stored_hash; + if (!(storage->loadDeviceDataHash("configuration", &stored_hash) && + new_hash == Hash(Hash::Type::kSha256, stored_hash))) { + LOG_DEBUG << "Reporting libaktualizr configuration"; + const HttpResponse response = http->post(config.tls.server + "/system_info/config", "application/toml", conf_str); + if (response.isOk()) { + storage->storeDeviceDataHash("configuration", new_hash.HashString()); + } + } else { + LOG_TRACE << "Not reporting libaktualizr configuration because it has not changed"; + } } Json::Value SotaUptaneClient::AssembleManifest() { diff --git a/src/libaktualizr/primary/sotauptaneclient.h b/src/libaktualizr/primary/sotauptaneclient.h index fe3cbf4a69..6d5fa64b5a 100644 --- a/src/libaktualizr/primary/sotauptaneclient.h +++ b/src/libaktualizr/primary/sotauptaneclient.h @@ -169,8 +169,6 @@ class SotaUptaneClient { std::shared_ptr package_manager_; std::shared_ptr uptane_fetcher; std::unique_ptr report_queue; - Json::Value last_network_info_reported; - Json::Value last_hw_info_reported; std::shared_ptr events_channel; boost::signals2::scoped_connection conn; std::exception_ptr last_exception; diff --git a/src/libaktualizr/storage/invstorage.h b/src/libaktualizr/storage/invstorage.h index e1ba8b9665..5f83dccc96 100644 --- a/src/libaktualizr/storage/invstorage.h +++ b/src/libaktualizr/storage/invstorage.h @@ -215,6 +215,10 @@ class INvStorage { virtual bool loadReportEvents(Json::Value* report_array, int64_t* id_max) const = 0; virtual void deleteReportEvents(int64_t id_max) = 0; + virtual void storeDeviceDataHash(const std::string& data_type, const std::string& hash) = 0; + virtual bool loadDeviceDataHash(const std::string& data_type, std::string* hash) const = 0; + virtual void clearDeviceData() = 0; + virtual bool checkAvailableDiskSpace(uint64_t required_bytes) const = 0; virtual boost::optional> checkTargetFile(const Uptane::Target& target) const = 0; diff --git a/src/libaktualizr/storage/sqlstorage.cc b/src/libaktualizr/storage/sqlstorage.cc index ed867ec5cd..0fc3ed86c9 100644 --- a/src/libaktualizr/storage/sqlstorage.cc +++ b/src/libaktualizr/storage/sqlstorage.cc @@ -1648,6 +1648,48 @@ void SQLStorage::clearInstallationResults() { db.commitTransaction(); } +void SQLStorage::storeDeviceDataHash(const std::string& data_type, const std::string& hash) { + SQLite3Guard db = dbConnection(); + + auto statement = db.prepareStatement( + "INSERT OR REPLACE INTO device_data(data_type,hash) VALUES (?,?);", data_type, hash); + if (statement.step() != SQLITE_DONE) { + LOG_ERROR << "Can't set " << data_type << " hash: " << db.errmsg(); + throw std::runtime_error("Can't set " + data_type + " hash: " + db.errmsg()); + } +} + +bool SQLStorage::loadDeviceDataHash(const std::string& data_type, std::string* hash) const { + SQLite3Guard db = dbConnection(); + + auto statement = + db.prepareStatement("SELECT hash FROM device_data WHERE data_type = ? LIMIT 1;", data_type); + + int result = statement.step(); + if (result == SQLITE_DONE) { + LOG_TRACE << data_type << " hash not present in db"; + return false; + } else if (result != SQLITE_ROW) { + LOG_ERROR << "Can't get " << data_type << " hash: " << db.errmsg(); + return false; + } + + if (hash != nullptr) { + *hash = statement.get_result_col_str(0).value(); + } + + return true; +} + +void SQLStorage::clearDeviceData() { + SQLite3Guard db = dbConnection(); + + if (db.exec("DELETE FROM device_data;", nullptr, nullptr) != SQLITE_OK) { + LOG_ERROR << "Can't clear device_data: " << db.errmsg(); + return; + } +} + bool SQLStorage::checkAvailableDiskSpace(const uint64_t required_bytes) const { struct statvfs stvfsbuf {}; const int stat_res = statvfs(dbPath().c_str(), &stvfsbuf); diff --git a/src/libaktualizr/storage/sqlstorage.h b/src/libaktualizr/storage/sqlstorage.h index d14363f37c..0f615aef0f 100644 --- a/src/libaktualizr/storage/sqlstorage.h +++ b/src/libaktualizr/storage/sqlstorage.h @@ -96,6 +96,10 @@ class SQLStorage : public SQLStorageBase, public INvStorage { void deleteReportEvents(int64_t id_max) override; void clearInstallationResults() override; + void storeDeviceDataHash(const std::string& data_type, const std::string& hash) override; + bool loadDeviceDataHash(const std::string& data_type, std::string* hash) const override; + void clearDeviceData() override; + bool checkAvailableDiskSpace(uint64_t required_bytes) const override; std::unique_ptr allocateTargetFile(const Uptane::Target& target) override; diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index 4a164712ec..4d9d9f930d 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -937,11 +937,15 @@ class HttpFakeProv : public HttpFake { } else if (url.find("/system_info") != std::string::npos) { /* Send hardware info to the server. */ system_info_count++; - Json::Value hwinfo = Utils::getHardwareInfo(); - EXPECT_EQ(hwinfo["id"].asString(), data["id"].asString()); - EXPECT_EQ(hwinfo["description"].asString(), data["description"].asString()); - EXPECT_EQ(hwinfo["class"].asString(), data["class"].asString()); - EXPECT_EQ(hwinfo["product"].asString(), data["product"].asString()); + if (system_info_count <= 1) { + Json::Value hwinfo = Utils::getHardwareInfo(); + EXPECT_EQ(hwinfo["id"].asString(), data["id"].asString()); + EXPECT_EQ(hwinfo["description"].asString(), data["description"].asString()); + EXPECT_EQ(hwinfo["class"].asString(), data["class"].asString()); + EXPECT_EQ(hwinfo["product"].asString(), data["product"].asString()); + } else { + EXPECT_EQ(custom_hw_info, data); + } } else { EXPECT_EQ(0, 1) << "Unexpected put to URL: " << url; } @@ -957,6 +961,7 @@ class HttpFakeProv : public HttpFake { int system_info_count{0}; int network_count{0}; int config_count{0}; + Json::Value custom_hw_info; private: Config &config; @@ -1034,6 +1039,31 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->network_count, 1); EXPECT_EQ(http->config_count, 1); EXPECT_EQ(http->events_seen, 8); + + // Try sending device data again. Set hardware info to a custom value, because + // on some systems lshw returns inconsistent results. Nothing else should have + // changed. + http->custom_hw_info["hardware"] = "test-hw"; + EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); + EXPECT_EQ(http->installed_count, 1); + EXPECT_EQ(http->system_info_count, 2); + EXPECT_EQ(http->network_count, 1); + EXPECT_EQ(http->config_count, 1); + + // Try sending device data again to confirm hardware info isn't resent. + EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); + EXPECT_EQ(http->installed_count, 1); + EXPECT_EQ(http->system_info_count, 2); + EXPECT_EQ(http->network_count, 1); + EXPECT_EQ(http->config_count, 1); + + // Clear the stored values and resend to verify the data is resent. + storage->clearDeviceData(); + EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); + EXPECT_EQ(http->installed_count, 2); + EXPECT_EQ(http->system_info_count, 3); + EXPECT_EQ(http->network_count, 2); + EXPECT_EQ(http->config_count, 2); } /* Migrate from the legacy filesystem storage. */ From fd94e54ca9c1e4b4958d6300b0ae59fc07389337 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 20 May 2020 15:06:40 +0200 Subject: [PATCH 4/5] Only send default hardware info once (ever). Custom hardware info will be sent if it is provided and it has changed from what was sent last. Signed-off-by: Patrick Vacek --- src/libaktualizr/primary/sotauptaneclient.cc | 21 ++++++++++--- src/libaktualizr/uptane/uptane_test.cc | 32 ++++++++++++-------- 2 files changed, 37 insertions(+), 16 deletions(-) diff --git a/src/libaktualizr/primary/sotauptaneclient.cc b/src/libaktualizr/primary/sotauptaneclient.cc index 3ce0e525e1..14c32b0acc 100644 --- a/src/libaktualizr/primary/sotauptaneclient.cc +++ b/src/libaktualizr/primary/sotauptaneclient.cc @@ -161,10 +161,21 @@ data::InstallationResult SotaUptaneClient::PackageInstallSetResult(const Uptane: return result; } +/* Hardware info is treated differently than the other device data. The default + * info (supplied via lshw) is only sent once and never again, even if it + * changes. (Unfortunately, it can change often due to CPU frequency scaling.) + * However, users can provide custom info via the API, and that will be sent if + * it has changed. */ void SotaUptaneClient::reportHwInfo(const Json::Value &custom_hwinfo) { Json::Value system_info; + std::string stored_hash; + storage->loadDeviceDataHash("hardware_info", &stored_hash); if (custom_hwinfo.empty()) { + if (!stored_hash.empty()) { + LOG_TRACE << "Not reporting default hardware information because it has already been reported"; + return; + } system_info = Utils::getHardwareInfo(); if (system_info.empty()) { LOG_WARNING << "Unable to fetch hardware information from host system."; @@ -174,10 +185,12 @@ void SotaUptaneClient::reportHwInfo(const Json::Value &custom_hwinfo) { const Json::Value &hw_info = custom_hwinfo.empty() ? system_info : custom_hwinfo; const Hash new_hash = Hash::generate(Hash::Type::kSha256, Utils::jsonToCanonicalStr(hw_info)); - std::string stored_hash; - if (!(storage->loadDeviceDataHash("hardware_info", &stored_hash) && - new_hash == Hash(Hash::Type::kSha256, stored_hash))) { - LOG_DEBUG << "Reporting hardware information"; + if (new_hash != Hash(Hash::Type::kSha256, stored_hash)) { + if (custom_hwinfo.empty()) { + LOG_DEBUG << "Reporting default hardware information"; + } else { + LOG_DEBUG << "Reporting custom hardware information"; + } const HttpResponse response = http->put(config.tls.server + "/system_info", hw_info); if (response.isOk()) { storage->storeDeviceDataHash("hardware_info", new_hash.HashString()); diff --git a/src/libaktualizr/uptane/uptane_test.cc b/src/libaktualizr/uptane/uptane_test.cc index 4d9d9f930d..145e39f746 100644 --- a/src/libaktualizr/uptane/uptane_test.cc +++ b/src/libaktualizr/uptane/uptane_test.cc @@ -937,7 +937,7 @@ class HttpFakeProv : public HttpFake { } else if (url.find("/system_info") != std::string::npos) { /* Send hardware info to the server. */ system_info_count++; - if (system_info_count <= 1) { + if (system_info_count <= 2) { Json::Value hwinfo = Utils::getHardwareInfo(); EXPECT_EQ(hwinfo["id"].asString(), data["id"].asString()); EXPECT_EQ(hwinfo["description"].asString(), data["description"].asString()); @@ -992,6 +992,7 @@ TEST(Uptane, ProvisionOnServer) { config.provision.primary_ecu_hardware_id = "primary_hw"; config.storage.path = temp_dir.Path(); UptaneTestCommon::addDefaultSecondary(config, temp_dir, "secondary_ecu_serial", "secondary_hw"); + logger_set_threshold(boost::log::trivial::trace); auto storage = INvStorage::newStorage(config.storage); auto events_channel = std::make_shared(); @@ -1040,25 +1041,32 @@ TEST(Uptane, ProvisionOnServer) { EXPECT_EQ(http->config_count, 1); EXPECT_EQ(http->events_seen, 8); - // Try sending device data again. Set hardware info to a custom value, because - // on some systems lshw returns inconsistent results. Nothing else should have - // changed. - http->custom_hw_info["hardware"] = "test-hw"; + // Try sending device data again to confirm that it isn't resent if it hasn't + // changed (and hardware info is only sent once). EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); EXPECT_EQ(http->installed_count, 1); - EXPECT_EQ(http->system_info_count, 2); + EXPECT_EQ(http->system_info_count, 1); EXPECT_EQ(http->network_count, 1); EXPECT_EQ(http->config_count, 1); - // Try sending device data again to confirm hardware info isn't resent. + // Clear the stored values and resend to verify the data is resent. + storage->clearDeviceData(); EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); - EXPECT_EQ(http->installed_count, 1); + EXPECT_EQ(http->installed_count, 2); EXPECT_EQ(http->system_info_count, 2); - EXPECT_EQ(http->network_count, 1); - EXPECT_EQ(http->config_count, 1); + EXPECT_EQ(http->network_count, 2); + EXPECT_EQ(http->config_count, 2); - // Clear the stored values and resend to verify the data is resent. - storage->clearDeviceData(); + // Set hardware info to a custom value and send device data again. + http->custom_hw_info["hardware"] = "test-hw"; + EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); + EXPECT_EQ(http->installed_count, 2); + EXPECT_EQ(http->system_info_count, 3); + EXPECT_EQ(http->network_count, 2); + EXPECT_EQ(http->config_count, 2); + + // Try once again; nothing should be resent. + http->custom_hw_info["hardware"] = "test-hw"; EXPECT_NO_THROW(up->sendDeviceData(http->custom_hw_info)); EXPECT_EQ(http->installed_count, 2); EXPECT_EQ(http->system_info_count, 3); From 42c6ce89d6a010c692ef8438eebe9a3f3515bd89 Mon Sep 17 00:00:00 2001 From: Patrick Vacek Date: Wed, 20 May 2020 17:29:04 +0200 Subject: [PATCH 5/5] Fix flaky test by waiting for Secondary to start up. The Primary is now more efficient and I guess the Secondary needs some extra time to catch up now. I wish there was a better solution here. I fixed a typo and tried to make the expiry tests a bit more consistent with each other since that was originally what I thought might be a cause of the failure. Signed-off-by: Patrick Vacek --- tests/test_update.py | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/tests/test_update.py b/tests/test_update.py index bbe9f149e9..1cb0fda29a 100755 --- a/tests/test_update.py +++ b/tests/test_update.py @@ -29,7 +29,7 @@ @with_sysroot() @with_aktualizr(start=False, run_mode='once', output_logs=True) def test_primary_ostree_secondary_file_updates(uptane_repo, secondary, aktualizr, director, sysroot, - treehub, uptane_server, **kwargs): + treehub, **kwargs): target_rev = treehub.revision # add an ostree update for Primary uptane_repo.add_ostree_target(aktualizr.id, target_rev) @@ -71,16 +71,17 @@ def test_primary_ostree_secondary_file_updates(uptane_repo, secondary, aktualizr but are expired after that and before Secondary is rebooted, we still expect that the installed update is applied in this case """ -@with_treehub() @with_uptane_backend() @with_director() +@with_treehub() @with_sysroot() @with_secondary(start=False) @with_aktualizr(start=False, run_mode='once', output_logs=True) -def test_secodary_ostree_update_if_metadata_expires(uptane_repo, secondary, aktualizr, treehub, sysroot, director, **kwargs): +def test_secondary_ostree_update_if_metadata_expires(uptane_repo, secondary, aktualizr, director, sysroot, treehub, **kwargs): target_rev = treehub.revision expires_within_sec = 10 + # add an OSTree update for Secondary uptane_repo.add_ostree_target(secondary.id, target_rev, expires_within_sec=expires_within_sec) start_time = time.time() @@ -88,30 +89,34 @@ def test_secodary_ostree_update_if_metadata_expires(uptane_repo, secondary, aktu with aktualizr: aktualizr.wait_for_completion() + # check the Primary update, must be in pending state since it requires reboot pending_rev = aktualizr.get_current_pending_image_info(secondary.id) - if pending_rev != target_rev: - logger.error("Pending version {} != the target one {}".format(pending_rev, target_rev)) + logger.error("Pending version {} != the target version {}".format(pending_rev, target_rev)) return False # wait until the target metadata are expired time.sleep(max(0, expires_within_sec - (time.time() - start_time))) + # emulate reboot and run aktualizr once more sysroot.update_revision(pending_rev) secondary.emulate_reboot() with secondary: + # Wait for Secondary to initialize. wait_for_completion won't work; it + # times out. + time.sleep(5) with aktualizr: aktualizr.wait_for_completion() + # check the Secondary update after reboot if not director.get_install_result(): logger.error("Installation result is not successful") return False installed_rev = aktualizr.get_current_image_info(secondary.id) - if installed_rev != target_rev: - logger.error("Installed version {} != the target one {}".format(installed_rev, target_rev)) + logger.error("Installed version {} != the target version {}".format(installed_rev, target_rev)) return False return True @@ -124,16 +129,16 @@ def test_secodary_ostree_update_if_metadata_expires(uptane_repo, secondary, aktu but are expired after that and before Primary is rebooted, we still expect that the installed update is applied in this case """ -@with_uptane_backend(start_generic_server=True) +@with_uptane_backend() @with_director() @with_treehub() @with_sysroot() @with_aktualizr(start=False, run_mode='once', output_logs=True) -def test_primary_ostree_update_if_metadata_expires(uptane_repo, aktualizr, director, sysroot, treehub, uptane_server, **kwargs): +def test_primary_ostree_update_if_metadata_expires(uptane_repo, aktualizr, director, sysroot, treehub, **kwargs): target_rev = treehub.revision expires_within_sec = 10 - # add an ostree update for Primary + # add an OSTree update for Primary uptane_repo.add_ostree_target(aktualizr.id, target_rev, expires_within_sec=expires_within_sec) start_time = time.time() @@ -157,8 +162,16 @@ def test_primary_ostree_update_if_metadata_expires(uptane_repo, aktualizr, direc aktualizr.wait_for_completion() # check the Primary update after reboot - result = director.get_install_result() and (target_rev == aktualizr.get_current_primary_image_info()) - return result + if not director.get_install_result(): + logger.error("Installation result is not successful") + return False + + installed_rev = aktualizr.get_current_primary_image_info() + if installed_rev != target_rev: + logger.error("Installed version {} != the target version {}".format(installed_rev, target_rev)) + return False + + return True if __name__ == "__main__": @@ -176,7 +189,7 @@ def test_primary_ostree_update_if_metadata_expires(uptane_repo, aktualizr, direc test_suite = [ test_primary_ostree_secondary_file_updates, - test_secodary_ostree_update_if_metadata_expires, + test_secondary_ostree_update_if_metadata_expires, test_primary_ostree_update_if_metadata_expires ]