Report failed downloads in an installation report. #1301

pattivacek · 2019-08-19T09:32:53Z

This should inform the Director that the update failed.

This appears to fix the bug, but it is not perfect:

~~There is no test, because we don't have a way to fake a download failure. It's probably doable with libfiu and the fake package manager, though.~~ This is now covered by tests.
There is the same problem with empty targets metadata that occurs with installation failure. It ~~could be~~ has been handled the same way, ~~although~~ despite that I don't like the solution of dropping metadata.
The installation report does not contain any information about how, why, or which download failed. That could be improved if anyone wants to see that.

We might want to consider merging testing and merging this as is anyway just to get the fix out the door, but further consideration is necessary to address the above issues.

pattivacek · 2019-08-19T09:58:58Z

I brought over f6a5bd5 from #1296 and did the same style of Targets metadata dropping to address issue # 2 despite that I really don't like that solution.

lbonn · 2019-08-19T10:13:53Z

There is no test, because we don't have a way to fake a download failure. It's probably doable with libfiu and the fake package manager, though.

Don't we just need to fake some http error code with HttpFake or some or our python http scripts?

codecov-io · 2019-08-19T10:18:34Z

Codecov Report

Merging #1301 into master will decrease coverage by 0.07%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master    #1301      +/-   ##
==========================================
- Coverage   79.05%   78.97%   -0.08%     
==========================================
  Files         178      178              
  Lines       10475    10494      +19     
==========================================
+ Hits         8281     8288       +7     
- Misses       2194     2206      +12

Impacted Files	Coverage Δ
src/libaktualizr/utilities/types.cc	`68.75% <ø> (ø)`	⬆️
src/libaktualizr/utilities/types.h	`83.92% <ø> (ø)`	⬆️
.../libaktualizr/package_manager/packagemanagerfake.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`91.59% <100%> (-0.53%)`	⬇️
...libaktualizr/package_manager/packagemanagerfake.cc	`91.22% <100%> (+1.43%)`	⬆️
src/libaktualizr/utilities/fault_injection.h	`94.87% <80%> (-2.19%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.h	`60% <0%> (-40%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.cc	`74.32% <0%> (-4.06%)`	⬇️
src/libaktualizr-posix/ipuptanesecondary.cc	`82.66% <0%> (-4%)`	⬇️
src/libaktualizr/uptane/directorrepository.cc	`95.71% <0%> (-1.43%)`	⬇️
... and 5 more

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 7971998...636acdb. Read the comment docs.

pattivacek · 2019-08-19T10:20:58Z

Don't we just need to fake some http error code with HttpFake or some or our python http scripts?

Oooh, nice idea. I was already halfway through setting up libfiu. I'll take that under consideration as well.

pattivacek · 2019-08-19T13:59:24Z

Fixed with tests using libfiu after all. It wasn't hard and the additional power is nice to have.

simao · 2019-08-19T14:11:46Z

This should inform the Director that the update failed.

This appears to fix the bug, but it is not perfect:

1. ~There is no test, because we don't have a way to fake a download failure. It's probably doable with libfiu and the fake package manager, though.~ This is now covered by tests.

2. There is the same problem with empty targets metadata that occurs with installation failure. It ~could be~ has been handled the same way, ~although~ despite that I don't like the solution of dropping metadata.

3. The installation report does not contain any information about how, why, or which download failed. That could be improved if anyone wants to see that.

What do you mean here? director would need at least the correlation id to make this work..

We might want to consider merging testing and merging this as is anyway just to get the fix out the door, but further consideration is necessary to address the above issues.

pattivacek · 2019-08-19T14:13:52Z

What do you mean here? director would need at least the correlation id to make this work..

Correlation ID is included. I just mean the error code/message is fairly generic. I've tested manually and it works, but the UI says:

Result code DOWNLOAD_FAILED
Target download failed

This should inform the Director that the update failed. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Otherwise this prevents being able to split updates into multiple individual calls to the Install API call. This still isn't my favorite fix for this problem, but in the short term, it's an improvement. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This is not an ideal solution! This makes it impossible to retry downloading via the API without setting up another update in the Director and rechecking for updates. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Raigi

Tests:

Device is not stuck after failed download - ok
Device can install new updates after failed download - ok
Creating devices and installing updates on them - ok

mike-sul · 2019-08-23T06:53:28Z

src/libaktualizr/primary/sotauptaneclient.cc

+      // Store installation report to inform Director of the download failure.
+      const std::string &correlation_id = director_repo.getCorrelationId();
+      data::InstallationResult device_installation_result =
+          data::InstallationResult(data::ResultCode::Numeric::kDownloadFailed, "Target download failed");


As far as I understand the mechanism of cancelling/removing of the new targets at Director is a sending manifest to it with a negative installation result in case if all targets downloads fails ?

If I'm understanding you correctly, yes. If we send an installation report with a failure code, the Director will respond by identifying the installation as a failure, and subsequent Targets metadata will be empty (to indicate that the device should take no further action, i.e. not retry until explicitly told to do so). Let me know if that doesn't make sense or you have further questions.

pattivacek requested review from simao, lbonn, EpicNuts, eu-siemann and Raigi August 19, 2019 09:32

pattivacek force-pushed the fix/OTA-3169/install-report-failed-download branch from 1d59acf to affb28f Compare August 19, 2019 09:55

pattivacek added 5 commits August 20, 2019 14:50

Report failed downloads in an installation report.

17e7e40

This should inform the Director that the update failed. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Also drop Targets metadata after a failed download.

b2f3e6e

This is not an ideal solution! This makes it impossible to retry downloading via the API without setting up another update in the Director and rechecking for updates. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Tests for download failure and subsequent empty targets handling.

da3b13a

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Update docs with fake_package_download failure point.

636acdb

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the fix/OTA-3169/install-report-failed-download branch from adbc70f to 636acdb Compare August 20, 2019 12:50

Raigi approved these changes Aug 21, 2019

View reviewed changes

pattivacek merged commit 38325b9 into master Aug 21, 2019

pattivacek deleted the fix/OTA-3169/install-report-failed-download branch August 21, 2019 11:30

mike-sul reviewed Aug 23, 2019

View reviewed changes

pattivacek mentioned this pull request Aug 30, 2019

Fix/ota 3060/check targets before download #1296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report failed downloads in an installation report. #1301

Report failed downloads in an installation report. #1301

pattivacek commented Aug 19, 2019 •

edited

Loading

pattivacek commented Aug 19, 2019

lbonn commented Aug 19, 2019 •

edited

Loading

codecov-io commented Aug 19, 2019 •

edited

Loading

pattivacek commented Aug 19, 2019

pattivacek commented Aug 19, 2019

simao commented Aug 19, 2019

pattivacek commented Aug 19, 2019

Raigi left a comment

mike-sul Aug 23, 2019

pattivacek Aug 23, 2019

Report failed downloads in an installation report. #1301

Report failed downloads in an installation report. #1301

Conversation

pattivacek commented Aug 19, 2019 • edited Loading

pattivacek commented Aug 19, 2019

lbonn commented Aug 19, 2019 • edited Loading

codecov-io commented Aug 19, 2019 • edited Loading

Codecov Report

pattivacek commented Aug 19, 2019

pattivacek commented Aug 19, 2019

simao commented Aug 19, 2019

pattivacek commented Aug 19, 2019

Raigi left a comment

Choose a reason for hiding this comment

mike-sul Aug 23, 2019

Choose a reason for hiding this comment

pattivacek Aug 23, 2019

Choose a reason for hiding this comment

pattivacek commented Aug 19, 2019 •

edited

Loading

lbonn commented Aug 19, 2019 •

edited

Loading

codecov-io commented Aug 19, 2019 •

edited

Loading