Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨Personalized resource limits: add DB table for max resources per user 🗃️ #4335

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Jun 8, 2023

What do these changes do?

  • adds a new table in the DB called services_limitations that allows one to define limitations in terms of RAM, #CPU, VRAM and #GPU to a group/cluster
  • adds a bunch of tests in the postgres-database package to check that all this works as intended

We can currently override the resources a service uses per group ID via the services_specifications table. It has a number of drawbacks:

  • all the instances of the overriden service will use the overriden specifications (if the user once needed a 96GB service, then all instances of that service will now require that amount)
  • a bit complex to set up

Nevertheless it saved us when the dynamic sidecar was not stable and allowed the autoscaling to come up switftly.

This new table is the beginning of:

  • project defined resources (the user may select more resources based on what he/she is allowed to use - which is defined in this new table)

Related issue/s

How to test

cd packages/postgres-database
make install-dev
make tests

DevOps Checklist

@sanderegg sanderegg added the a:database associated to postgres service and postgres-database package label Jun 8, 2023
@sanderegg sanderegg added this to the Watermelon milestone Jun 8, 2023
@sanderegg sanderegg self-assigned this Jun 8, 2023
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #4335 (58c7cf0) into master (13e95f6) will increase coverage by 2.6%.
The diff coverage is 98.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4335     +/-   ##
========================================
+ Coverage    84.1%   86.7%   +2.6%     
========================================
  Files         976     840    -136     
  Lines       41881   37085   -4796     
  Branches      983     449    -534     
========================================
- Hits        35247   32183   -3064     
+ Misses       6424    4794   -1630     
+ Partials      210     108    -102     
Flag Coverage Δ
integrationtests 68.1% <ø> (+16.1%) ⬆️
unittests 83.9% <98.9%> (+0.8%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/src/simcore_postgres_database/models/clusters.py 100.0% <ø> (ø)
...re_postgres_database/utils_services_limitations.py 98.7% <98.7%> (ø)
...se/src/simcore_postgres_database/models/_common.py 100.0% <100.0%> (ø)
...core_postgres_database/models/cluster_to_groups.py 100.0% <100.0%> (ø)
...e_postgres_database/models/services_limitations.py 100.0% <100.0%> (ø)

... and 179 files with indirect coverage changes

@sanderegg sanderegg force-pushed the personalized-resource-limits/add-group-table branch from b5adcfe to d8cd193 Compare June 8, 2023 14:33
@sanderegg sanderegg changed the title ✨Personalized resource limits: add DB table for max resources per user ✨Personalized resource limits: add DB table for max resources per user 🗃️ Jun 8, 2023
@sanderegg sanderegg marked this pull request as ready for review June 8, 2023 14:44
Copy link
Collaborator

@elisabettai elisabettai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds a big improvement! So this new table will make the changes in resources temporary? So this will not happen anymore?

all the instances of the overriden service will use the overriden specifications (if the user once needed a 96GB service, then all instances of that service will now require that amount)

Minor: in the PR description, I guess you wanted to add the direct link to the issue (and not the link via the Github project board).

@sanderegg
Copy link
Member Author

Sounds a big improvement! So this new table will make the changes in resources temporary? So this will not happen anymore?

all the instances of the overriden service will use the overriden specifications (if the user once needed a 96GB service, then all instances of that service will now require that amount)

Minor: in the PR description, I guess you wanted to add the direct link to the issue (and not the link via the Github project board).

well hold a bit the horses ;) it's not yet complete. that is only the table.

Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read it briefly, very cool stuff, thanks!

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Code is spotless!

@sanderegg sanderegg force-pushed the personalized-resource-limits/add-group-table branch from 46ae7a7 to 940c6e1 Compare June 9, 2023 06:19
@codeclimate
Copy link

codeclimate bot commented Jun 9, 2023

Code Climate has analyzed commit 58c7cf0 and detected 0 issues on this pull request.

View more on Code Climate.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sanderegg sanderegg merged commit d3a78c6 into ITISFoundation:master Jun 9, 2023
@sanderegg sanderegg deleted the personalized-resource-limits/add-group-table branch June 9, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:database associated to postgres service and postgres-database package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants