May Security Update #610
May Security Update #610
Conversation
willronchetti
commented
Jun 7, 2022
willronchetti
commented
- Rebase of May Security Update #602 on master
| @@ -17,7 +17,7 @@ on: | |||
| jobs: | |||
| # This workflow contains a single job called "build" | |||
| build: | |||
| name: Test Suite for cgap-portal (Python 3.7, Node 16) | |||
| name: Test Suite for cgap-portal (Python 3.8, Node 16) | |||
There was a problem hiding this comment.
Have we done a check to see if names can usefully have environment variables in them? It feels like that would save us a lot of little updates like this if it works. It might not, though.
| @@ -1,8 +1,6 @@ | |||
| # CGAP-Portal (Production) Dockerfile | |||
| # Take latest 3.7.12 Debian variant | |||
| FROM python:3.7.12-slim-buster | |||
| # bullseye seems to perform worse | |||
There was a problem hiding this comment.
It seems like this note about bullseye is worth mentioning, though it would have been more helpful if dated.
There was a problem hiding this comment.
I don't believe this comment is accurate as it pre-dates the performance optimizations. I suspect if I were to build with bullseye, performance would be comparable.
| @@ -61,5 +61,4 @@ timeout = 60 | |||
|
|
|||
| [filter:memlimit] | |||
| use = egg:encoded#memlimit | |||
| rss_limit = 500MB | |||
| rss_percent_limit = 20 | |||
There was a problem hiding this comment.
20% of what? I'm not sure what's going on here but it feels like 20% is a very low limit. Is this because we've got 4 processes that are each taking 25%? Or 5 that this allows 100% of each? (That seems questionable, but maybe it is discounted to 80% of the specified amount elsewhere.) If it's weird math like that, it seriously calls for a comment explaining how the value was chosen. But really, I'd like such a comment regardless.
There was a problem hiding this comment.
The functionality that uses this is based on psutil and does not work in Fargate, see giampaolo/psutil#2100. So in effect we are capping portal workers at 450MB of memory, while previously there was effectively no cap. If the cap is reached, the worker is killed and restarted by supervisor.
There was a problem hiding this comment.
I just what that info in a comment in the code.
| colorama = "0.3.3" | ||
| dcicpyvcf = "1.0.0" | ||
| dcicsnovault = "^5.4.0" | ||
| dcicsnovault = "^5.6.0" | ||
| dcicutils = "^3.9.0" |
There was a problem hiding this comment.
Even if not engaging the beta, it might be worth requiring "^3.13.1".
There was a problem hiding this comment.
3.13.1 is locked so I will keep this for now and bump when we introduce env utils.
