Only send device data if it has changed #1673
Conversation
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 <[email protected]>
Signed-off-by: Patrick Vacek <[email protected]>
pattivacek
force-pushed
the
feat/OTA-4882/cache-device-data
branch
from
d388961 to
15658ab
Compare
|
QEMU doesn't have frequency scaling, most of the real hardware will behave as your laptop. |
No, not critical, and yes, we could just ignore it, but that requires parsing something that we previously left untouched. We've since agreed to only send the device data once during initialization and only again if a user provides custom data through the new API function, so we don't have to worry about it anymore. I still need to update the PR, though. |
|
I also don't understand why the tests are failing. I haven't been able to reproduce locally yet. |
Codecov Report
@@ Coverage Diff @@
## master #1673 +/- ##
==========================================
+ Coverage 82.61% 82.85% +0.23%
==========================================
Files 190 190
Lines 12087 12142 +55
==========================================
+ Hits 9986 10060 +74
+ Misses 2101 2082 -19
Continue to review full report at Codecov.
|
config/sql/migration/migrate.25.sql
Outdated
| -- 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(unique_mark INTEGER PRIMARY KEY CHECK (unique_mark = 0), hardware_info_hash TEXT NOT NULL DEFAULT "", installed_packages_hash TEXT NOT NULL DEFAULT "", network_info_hash TEXT NOT NULL DEFAULT "", configuration_hash TEXT NOT NULL DEFAULT ""); |
There was a problem hiding this comment.
I'm starting to dislike this kind of one-row tables.
Maybe we can actually simplify things here with something like a key/value:
CREATE TABLE device_data(data_type TEXT PRIMARY KEY, hash TEXT NOT NULL);
And then, we don't even have to duplicate the load/store methods completely, they can call these with data_type as a constant:
void storeDeviceDataHash(const std::string &data_type, const std::string &hash);
bool loadDeviceDataHash(const std::string &data_type, std::string *hash);
Plus, each of these can just use INSERT OR REPLACE instead of checking if the table is empty first.
There was a problem hiding this comment.
Yeah, I was debating whether I should add these new items to the existing device_info table. I decided against it just because it seemed like two separate concepts. This seems like it would be more flexible (as in, fewer migrations necessary to add new items). Should we migrate some of the other similar things to that model as well?
There was a problem hiding this comment.
That's a fair question. Perhaps I was too vague about what felt wrong but it's probably that these hashes are pretty independent from each others and read/written at different times. And they have the same format, so it felt more natural to have them in different rows.
As far as other examples we have, the primary_keys table looks similar but I think the public/private keys are not so independent and we store both at once. It also looks like the other "one-row" tables are updated all at once, with fields of various types.
Also (again, opinion :D), the pain threshold we should have before changing the table schema should be slightly higher than changing regular code as they are not completely free to migrate (resources as much as risk). So I don't think we have anything urgent to change there right now.
pattivacek
force-pushed
the
feat/OTA-4882/cache-device-data
branch
from
451482b to
7cdfadc
Compare
| "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(); | ||
| return; |
There was a problem hiding this comment.
Should we throw an exception here?
There was a problem hiding this comment.
Although, to be fair, there are lots of other parts of the storage that don't throw given the same circumstances.
There was a problem hiding this comment.
Yes, I know. Still, will be fewer places to fix in the future ;)
|
LGTM, except one comment |
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 <[email protected]>
Custom hardware info will be sent if it is provided and it has changed from what was sent last. Signed-off-by: Patrick Vacek <[email protected]>
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 <[email protected]>
pattivacek
force-pushed
the
feat/OTA-4882/cache-device-data
branch
from
7cdfadc to
42c6ce8
Compare
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.
I also removed sending 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.