Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Store installed version as "non current" before installing #1402

Merged
merged 1 commit into from
Oct 10, 2019

Conversation

lbonn
Copy link
Contributor

@lbonn lbonn commented Oct 8, 2019

So that the metadata can be re-accessed even if the process is interrupted before the end of installation.

In the infamous "error 20" scenario, the update now shows as successful with error code "0". The reason is that aktualizr doesn't report any installation report but backend accepts it because we now report the correct new version.

It's also maybe not ideal as we would maybe want to report some kind of error (should director error on empty installation reports?), but I think it's a step in the good direction.

Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh-oh, the signals test failed.

This is probably fine, but strikes me as an imperfect compromise. Perhaps we should leave some comments to explain the decisions?

@@ -160,6 +160,8 @@ void SotaUptaneClient::finalizeAfterReboot() {
data::InstallationResult SotaUptaneClient::PackageInstallSetResult(const Uptane::Target &target) {
data::InstallationResult result;
Uptane::EcuSerial ecu_serial = uptane_manifest.getPrimaryEcuSerial();
// load the target in installed versions so that it can be re-accessed later
storage->saveInstalledVersion(ecu_serial.ToString(), target, InstalledVersionUpdateMode::kNone);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to put that in the installedVersions table if it hasn't been installed yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The api has been designed to allow that. But adding a non-current/non-pending entry via kNone is indeed poorly tested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, just checking. Seems fine, then.

@lbonn
Copy link
Contributor Author

lbonn commented Oct 9, 2019

Yes the signal tests sometimes hangs like that... The issue is hard to reproduce though.
Where do you think we should add a comment?

@pattivacek
Copy link
Collaborator

Where do you think we should add a comment?

Probably in SotaUptaneClient::PackageInstallSetResult(), just to clarify how we expect to "recover" if the installation is interrupted and the potential risk if more logic is added after the OSTree deploy that might get skipped.

Is it reasonable to add a test for this scenario? How have you been simulating it, anyway?

@lbonn
Copy link
Contributor Author

lbonn commented Oct 10, 2019

Is it reasonable to add a test for this scenario? How have you been simulating it, anyway?

Yes, testing is a bit tricky though. For verification, I've built a qemu image with an aktualizr that just throws after the ostree step.

@pattivacek
Copy link
Collaborator

For verification, I've built a qemu image with an aktualizr that just throws after the ostree step.

That's probably fine for now. Maybe for now we should just make a tech debt task to figure out how to test it, and then if you add a small comment about the current risks of the installation interruption, I'll approve.

So that the metadata can be re-accessed even if the process is
interrupted before the end of installation.

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
@codecov-io
Copy link

Codecov Report

Merging #1402 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
+ Coverage   80.39%   80.41%   +0.02%     
==========================================
  Files         181      181              
  Lines       10630    10637       +7     
==========================================
+ Hits         8546     8554       +8     
+ Misses       2084     2083       -1
Impacted Files Coverage Δ
src/libaktualizr/primary/sotauptaneclient.cc 89.1% <100%> (+0.01%) ⬆️
src/libaktualizr/package_manager/ostreemanager.cc 80.22% <100%> (+3.18%) ⬆️
src/libaktualizr/storage/sqlstorage.cc 77.24% <0%> (-0.69%) ⬇️
src/libaktualizr/crypto/keymanager.cc 88.74% <0%> (-0.67%) ⬇️
src/libaktualizr/storage/sqlstorage_base.cc 79.72% <0%> (+0.67%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d13ff1c...67c5c58. Read the comment docs.

@lbonn lbonn merged commit bb2b7c0 into master Oct 10, 2019
@lbonn lbonn deleted the bug/OTA-2725/bad-version branch October 10, 2019 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants