WIP: 🐛 fix max num of allowed dynamic services #4030
Conversation
…ect running nodes
matusdrobuliak66
requested review from
pcrespov,
sanderegg and
GitHK
as code owners
Codecov Report
@@ Coverage Diff @@
## master #4030 +/- ##
=========================================
- Coverage 83.0% 64.9% -18.2%
=========================================
Files 946 394 -552
Lines 40979 20369 -20610
Branches 851 137 -714
=========================================
- Hits 34034 13221 -20813
- Misses 6736 7099 +363
+ Partials 209 49 -160
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| project_active_running_nodes = [ | ||
| node | ||
| for node in project_running_nodes | ||
| if node["service_state"] not in ["failed", "completed"] |
There was a problem hiding this comment.
so we also have completed ones??? that is very interesting...
@GitHK what is the concept here. I think this calls return the director-v2 tracked services correct? why do we see completed? I had the feeling they were removed from the list at some point, is that not true?
I'm ok for now with the fix, but I am a bit afraid that we can get failed services with a still running sidecar that are eating resources.
There was a problem hiding this comment.
The name of this function is so outdated, with respect to what it returns. It's a list of states for the dynamic services (legacy and modern). Nothing more.
Had a chat with Matus and I think in this case he should only consider services which are started/starting when computing the count.
services/web/server/src/simcore_service_webserver/projects/projects_api.py
Outdated
Show resolved
Hide resolved
services/web/server/tests/unit/with_dbs/02/test_projects_handlers__open_close.py
Show resolved
Hide resolved
…tusdrobuliak66/osparc-simcore into is836/max-num-of-dynamic-services-fix
|
Code Climate has analyzed commit 340f10c and detected 0 issues on this pull request. View more on Code Climate. |
|
Kudos, SonarCloud Quality Gate passed!
|
|
I will be taking over this @matusdrobuliak66
|
There was a problem hiding this comment.
Both of the options below are valid considerations to be made. This PR changes from one of these behaviours to the other.
- do we want the user to always be able to start up to 5 services?
- or do we want the system to always be able to have max 5 running services like it is now?
We need to decide on what we want to obtain here.
yes you can review it, but as mentioned by previous comment Andrei will firstly have a look at #4034 |
What do these changes do?
When calling director v2 with list_dynamic_services(), we receive information about all currently running services and their respective states. Currently, this function may return services that have failed (as seen in the reported issue), but in theory, it is possible for a service in the completed state to also be returned. This pull request aims to filter out both of these states when calculating the maximum number of dynamic services that a user can run, for an improved user experience.
We can discuss whether this is a way to go, or we will fix it in the director v2. Then I will add additional unit tests.
Related issue/s
How to test