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

Increase size of targets metadata file accepted #1476

Merged

Conversation

bclouser
Copy link

@bclouser bclouser commented Dec 6, 2019

Updated after reading contributing guidelines.

Hey All,
I am curious what you think of this PR.

We have run into an issue where we are pushing a whole lot of images and run over the 1MB limit on the targets.json metadata file. We don't anticipate it growing much more than 1MB as we are pruning our nightly images from the previous month, but we kept having issues at the end of each month where we would bump against the limit.

Additionally, I am more than happy to learn that we are doing something stupid :) Perhaps there is a better way to handle large numbers of images???

Thanks,
Ben

Signed-off-by: Ben Clouser <ben.clouser@toradex.com>
@codecov-io
Copy link

codecov-io commented Dec 6, 2019

Codecov Report

Merging #1476 into master will increase coverage by 1.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1476      +/-   ##
==========================================
+ Coverage   80.51%   81.89%   +1.38%     
==========================================
  Files         184      184              
  Lines       11083    11756     +673     
==========================================
+ Hits         8923     9628     +705     
+ Misses       2160     2128      -32
Impacted Files Coverage Δ
src/libaktualizr/uptane/fetcher.h 100% <ø> (ø) ⬆️
src/libaktualizr/http/httpinterface.h 100% <0%> (ø) ⬆️
src/libaktualizr/primary/aktualizr.h 100% <0%> (ø) ⬆️
src/libaktualizr/uptane/directorrepository.h 100% <0%> (ø) ⬆️
src/libaktualizr/primary/reportqueue.cc 100% <0%> (ø) ⬆️
src/libaktualizr/http/httpclient.h 100% <0%> (ø) ⬆️
...c/aktualizr_secondary/aktualizr_secondary_common.h 100% <0%> (ø) ⬆️
src/libaktualizr/primary/sotauptaneclient.h 100% <0%> (ø) ⬆️
src/libaktualizr/telemetry/telemetryconfig.cc 100% <0%> (ø) ⬆️
... and 16 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 6b1da3e...efc95eb. Read the comment docs.

@eu-siemann
Copy link
Contributor

It seems to me that we should consider making it configurable, I don't think there is a "one size fits all" solution here.

@pattivacek
Copy link
Collaborator

As a stop-gap solution, this is fine. It's been a point of discussion how we can make this more user-friendly going forward, since it's not very nice that there isn't any sort of warning about approaching the size limit nor any great solution for what to do once it's reached. The options are to use delegations or remove something from your Images metadata.

I think the backend should probably have the same hardcoded limits as aktualizr, if that's the route we want to keep going down. @simao @koshelev any feelings on that?

@bclouser
Copy link
Author

Makes enough sense to me. I had already updated the nginx conf of our deployment to arbitrarily use 50M

/configmap.yaml
index db2c48d..7ab10d7 100755
--- a/charts/reverse-proxy/templates/configmap.yaml
+++ b/charts/reverse-proxy/templates/configmap.yaml
@@ -22,6 +22,7 @@ data:
        proxy_pass              http://ota-treehub-internal/;
      }
      location /repo/ {
+        client_max_body_size 50M;
        auth_request     /auth-token;
        proxy_set_header x-ats-namespace "default";
        proxy_pass              http://ota-tuf-reposerver-internal/;

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.

Merging as an interim solution until we sort something better out.

@pattivacek pattivacek merged commit 2896c3f into advancedtelematic:master Dec 18, 2019
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

4 participants