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

Fix issue with never-ending install retries #1192

Merged
merged 3 commits into from
Apr 26, 2019

Conversation

lbonn
Copy link
Contributor

@lbonn lbonn commented Apr 24, 2019

As backend sends empty director targets in some unexpected situations,
we've implemented a client-side mitigation in
02deeb5.

One unwanted side-effect is that after a failed installation, we'll keep
taking into account the targets containing the new version, hence
retrying installations in a loop until the backend asks to install
something new.

Second fix is to drop director targets after installation end, until
we've come to an agreement on a proper solution on both sides.

Note: would probably need an accompanying unit test, on the way :)

@pattivacek
Copy link
Collaborator

pattivacek commented Apr 24, 2019

I think we might need to move latest_targets to targets instead of just erasing latest_targets.

(Updated to fix typo)

To keep all the logic at the same place

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
As backend sends empty director targets in some unexpected situations,
we've implemented a client-side mitigation in
02deeb5.

One unwanted side-effect is that after a failed installation, we'll keep
taking into account the targets containing the new version, hence
retrying installations in a loop until the backend asks to install
something new.

Second fix is to drop director targets after installation end, until
we've come to an agreement on a proper solution on both sides.

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
@lbonn lbonn force-pushed the fix/OTA-2587/fix-failed-retries branch from 6d6a98e to 489b405 Compare April 25, 2019 09:56
@lbonn
Copy link
Contributor Author

lbonn commented Apr 25, 2019

Added a test for the fix. Since it works, I would vote for keeping the simple solution if it doesn't have major drawback, as it's easier to reason about and less likely (IMO) to break in the future.

@codecov-io
Copy link

Codecov Report

Merging #1192 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1192      +/-   ##
=========================================
+ Coverage   77.09%   77.1%   +<.01%     
=========================================
  Files         173     173              
  Lines       10116   10127      +11     
=========================================
+ Hits         7799    7808       +9     
- Misses       2317    2319       +2
Impacted Files Coverage Δ
src/libaktualizr/uptane/tuf.h 93.85% <ø> (ø) ⬆️
src/libaktualizr/primary/aktualizr.h 100% <ø> (ø) ⬆️
src/libaktualizr/uptane/directorrepository.h 100% <ø> (ø) ⬆️
src/libaktualizr/primary/sotauptaneclient.cc 91.29% <100%> (+0.05%) ⬆️
src/libaktualizr/uptane/directorrepository.cc 95.71% <100%> (+0.63%) ⬆️
src/libaktualizr/crypto/crypto.cc 86% <0%> (-0.8%) ⬇️

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 a1d8f1b...489b405. Read the comment docs.

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.

Seems legit. Shall we merge and then have @EpicNuts verify that he is happy with the results?

@pattivacek pattivacek merged commit 2a32803 into master Apr 26, 2019
@pattivacek pattivacek deleted the fix/OTA-2587/fix-failed-retries branch April 26, 2019 08:35
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.

3 participants