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

Don't fetch Snapshot or Targets metadata if we already have the latest. #1503

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

pattivacek
Copy link
Collaborator

@pattivacek pattivacek commented Dec 30, 2019

Also test that we don't fetch anything from the Image repo if the Director does not indicate that there are updates.

This should probably wait until #1501 is merged, since that will change how root metadata is fetched.

I did some manual testing to verify that the bandwidth savings are real. Note that my account has a rather large targets.json in the Image repo, so this demonstrates it well. I did a rather naive comparison by counting the bytes dumped to the log for each server response. (This is flawed for many reasons but should still give a reasonable idea of the benefit.) I did this three times: once for initialization (no updates, only Director metadata fetched), again for an update (all metadata fetched), and a third time for a second, pre-existing update (all metadata fetched on master, but with this PR, the Snapshot and Targets from the Image repo are skipped).

On current master:

$ grep "response:" dump_init.txt | wc -c
34
$ grep "response:" dump_first.txt | wc -c
190934
$ grep "response:" dump_second.txt | wc -c
184349

With these changes:

$ grep "response:" dump_init.txt | wc -c
34
$ grep "response:" dump_first.txt | wc -c
190934
$ grep "response:" dump_second.txt | wc -c
8419

FYI @doanac this hopefully helps address #1446.

@pattivacek
Copy link
Collaborator Author

@doanac
Copy link
Collaborator

doanac commented Jan 4, 2020

Thanks! I'm just getting back from holidays and have only glanced at this. One thing I was wondering - would it be a worth a LOG_DEBUG when skipping a remote fetch, or is this pretty easy to detect based on the current logging (ie no HTTP_GET is done, so it got skipped)?

@pattivacek pattivacek requested review from eu-siemann and lbonn January 6, 2020 08:30
@pattivacek
Copy link
Collaborator Author

FYI the standard has been updated accordingly to acknowledge (most of) these optimizations: uptane/uptane-standard#151

@pattivacek pattivacek force-pushed the feat/OTA-3995/dont-download-redundantly branch from 515b2fb to d9494ee Compare January 6, 2020 14:52
@codecov-io
Copy link

codecov-io commented Jan 6, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1503      +/-   ##
==========================================
+ Coverage   80.74%   80.76%   +0.02%     
==========================================
  Files         184      184              
  Lines       11141    11154      +13     
==========================================
+ Hits         8996     9009      +13     
  Misses       2145     2145
Impacted Files Coverage Δ
src/libaktualizr/uptane/imagesrepository.h 100% <ø> (ø) ⬆️
src/libaktualizr/uptane/imagesrepository.cc 88.6% <100%> (+1.02%) ⬆️
src/libaktualizr/storage/sqlstorage_base.h 60% <0%> (-40%) ⬇️
src/libaktualizr/storage/sqlstorage_base.cc 74.32% <0%> (-4.73%) ⬇️
src/libaktualizr/primary/sotauptaneclient.cc 88.85% <0%> (+0.14%) ⬆️
src/libaktualizr/storage/sqlstorage.cc 77.45% <0%> (+0.89%) ⬆️

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 77df64c...b4537ee. Read the comment docs.

@pattivacek
Copy link
Collaborator Author

Rebased on latest master.

would it be a worth a LOG_DEBUG when skipping a remote fetch, or is this pretty easy to detect based on the current logging (ie no HTTP_GET is done, so it got skipped)?

@doanac I think it's easy enough to determine with loglevel 1 from watching the GETs (not) made.

@lbonn
Copy link
Contributor

lbonn commented Jan 6, 2020

Looking good! @doanac's comment is still valid though, adding a LOG_DEBUG should be pretty cheap.

Also test that we don't fetch anything from the Image repo if the
Director does not indicate that there are updates.

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
@pattivacek pattivacek force-pushed the feat/OTA-3995/dont-download-redundantly branch from d9494ee to b4537ee Compare January 7, 2020 08:22
@pattivacek
Copy link
Collaborator Author

Looking good! @doanac's comment is still valid though, adding a LOG_DEBUG should be pretty cheap.

Two votes is enough to convince me. Done.

@pattivacek pattivacek merged commit dee1cd6 into master Jan 7, 2020
@pattivacek pattivacek deleted the feat/OTA-3995/dont-download-redundantly branch January 7, 2020 09:17
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.

4 participants