-
Notifications
You must be signed in to change notification settings - Fork 61
Add automatic removal of old installed targets #1318
Conversation
4b30ec9
to
784f4bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main problem with this version is that it will probably make problems with rollbacks.
Agreed. I like the idea, but I think we need to keep the current and previous targets and only get rid of anything older than that.
Also, we'd probably want to have something similar for ostree.
Agreed, but does OSTree do that automatically? I've never seen ostree admin status
return more than two commits. Is it hiding older commits or silently cleaning them up?
Codecov Report
@@ Coverage Diff @@
## master #1318 +/- ##
=========================================
+ Coverage 79.72% 79.83% +0.1%
=========================================
Files 177 178 +1
Lines 10475 10547 +72
=========================================
+ Hits 8351 8420 +69
- Misses 2124 2127 +3
Continue to review full report at Codecov.
|
Ok, I'll push the installation log on top then and will think about a nice api to reuse that easily.
Hm ok I didn't remember about that. Makes sense |
784f4bb
to
3bd213d
Compare
e40b89e
to
a284818
Compare
Ok I think it's close to the end. Was more complicated than expected... |
a284818
to
f2c95ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty cool!
|
||
// only two targets are left: dummy_firmware has been cleaned up | ||
std::vector<Uptane::Target> targets = aktualizr.GetStoredTargets(); | ||
ASSERT_EQ(targets.size(), 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty awesome test, but do you think it's worth checking that the two remaining are the two we expect at the very end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add that.
My only nit is that I had to really read commit eb510df to get an understanding of what was going on here. "Feat/ota 3073" doesn't give readers much context. I think the intent of this is something along the lines of "The installed_versions table grows each time an update is applied. This change prunes out old rows from the table that are no longer needed" |
Fair points, I opened the PR long before it was finally ready. I've updated the opening description. There should be some doc bits missing as well, actually. |
After further tests, the installation log actually doesn't work properly... Will push some fixes soon. |
95db449
to
3267861
Compare
There was a bug in the previous version. It should now be fixed and also simpler and better tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool, still just trying to make sure I understand it all.
/** | ||
* Get log of installations. The log is indexed for every ECU and contains | ||
* every change of versions ordered by time. It may contain duplicates in | ||
* case of rollbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still true? Won't anything rolled back erase the prior generation number for that target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you describe is close to the old behavior.
It will now create a new generation and have another entry in the table.
ALTER TABLE installed_versions RENAME TO installed_versions_old; | ||
CREATE TABLE installed_versions(id INTEGER PRIMARY KEY, ecu_serial TEXT NOT NULL, sha256 TEXT NOT NULL, name TEXT NOT NULL, hashes TEXT NOT NULL, length INTEGER NOT NULL DEFAULT 0, correlation_id TEXT NOT NULL DEFAULT '', generation INTEGER, is_current INTEGER NOT NULL CHECK (is_current IN (0,1)) DEFAULT 0, is_pending INTEGER NOT NULL CHECK (is_pending IN (0,1)) DEFAULT 0, was_installed INTEGER NOT NULL CHECK (was_installed IN (0,1)) DEFAULT 0); | ||
CREATE TRIGGER trig_installed_versions_gen AFTER INSERT ON installed_versions FOR EACH ROW BEGIN UPDATE installed_versions SET generation = (SELECT IFNULL(MAX(generation), 0) FROM installed_versions WHERE ecu_serial = NEW.ecu_serial)+1 WHERE id = NEW.id; END; | ||
INSERT INTO installed_versions(ecu_serial, sha256, name, hashes, length, correlation_id, is_current, is_pending, was_installed) SELECT installed_versions_old.ecu_serial, installed_versions_old.sha256, installed_versions_old.name, installed_versions_old.hashes, installed_versions_old.length, installed_versions_old.correlation_id, installed_versions_old.is_current, installed_versions_old.is_pending, 1 FROM installed_versions_old ORDER BY rowid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is was_installed
actually used for? I'm not quite following why that is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installed_versions has is_current and is_pending columns to distinguish between what's currently running and staged from the rest.
was_installed is used to distinguish historical versions that were never current from the rest. ie: you set something to pending and end up installing some other version. The actual log would only contain versions that have actually been installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional note: we assume that all versions have been installed when migrating (hence, the 1). It seems to be better than reporting none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just determine if it was installed if the generation is > 0 (or >= 0)? Or does the generation value get set even if the target is pending, such that if it isn't actually installed, you'd still want to know that it was pending at one point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea but in this code, generation was also incremented for pending version.
In the end I realized it was still complex so I removed the generation field entirely and relies on id which should have sufficient guarantees for this purpose.
was_installed is still there in the new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even realize that id
would probably work fine. Makes sense if you're copying entries for rollbacks/reinstalls. I think that means this is good to go, then.
@@ -346,6 +363,72 @@ TEST(sqlstorage, DbMigration18to19) { | |||
FAIL() << "Too many rows"; | |||
} | |||
} | |||
|
|||
TEST(sqlstorage, DbMigration19to20) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is this test covering? Is it just making sure we don't erase things that were installed with version 19 after we migrate to version 20 since they don't have generation numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's testing the migration with realistic data indeed. It gives a bit more confidence than just checking the table shapes, like schema_migration_test.sh does.
3267861
to
5efa6fe
Compare
Signed-off-by: Laurent Bonnans <[email protected]>
Signed-off-by: Laurent Bonnans <[email protected]>
This function returns empty json for failure. Throwing here ended up masking the real issue. Signed-off-by: Laurent Bonnans <[email protected]>
Signed-off-by: Laurent Bonnans <[email protected]>
Most of the time it was used to get current and pending versions. For the other cases, provide loadInstallationLog which also returns the ordered log of installation. Signed-off-by: Laurent Bonnans <[email protected]>
Put it in a separate source so it can be more easily tested and won't pollute aktualizr_primary's main too much. Only deletes until the second last version. Signed-off-by: Laurent Bonnans <[email protected]>
Signed-off-by: Laurent Bonnans <[email protected]>
5efa6fe
to
33d1973
Compare
This adds a mechanism to remove old downloaded targets.
Here, "old" is what was before the current and the previous installed version and the cleaning is done at the end of the installation.
It's only enabled when using aktualizr's executable but can be added through the API as well.
This first version is simple. I was planning to add a more complex logic based on the previous log of installed versions (spinned off here: https://github.com/advancedtelematic/aktualizr/tree/feat/install-log) but now I'm not sure it's worth the trouble.Main problem with this version is that it will probably make problems with rollbacks.Also, we'd probably want to have something similar for ostree. (already here?)