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

⚗️ Reject saving project while pipeline is running #2797

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Feb 4, 2022

What do these changes do?

Changes in node inputs (e.g. changing links or values) are not allowed to be saved while the pipelines are running.

NOTE: This PR complements the front-end PR #2795 that will gray out the workbench while pipelines run

Details

This is a temporary measure that we adopt in order to avoid an unstable state that occurs when a study is running a pipeline and the links between (newly added) nodes do not work anymore. In addition, this scenario has recently appeared more frequently due to other problems with the scheduler that would leave pipelines running forever.

The underline problem requires some more detail analysis of the underline nodeports library that could take a bit longer to fulfill. When that happens this limitation will be removed.

We agreed that the webserver will respond with 409: The request could not be completed due to a conflict with the current state of the target resource (i.e. pipeline is running). This code is used in situations where the user might be able to resolve the conflict and resubmit the request (front-end will show a pop-up with message below).

image

@odeimaiz you can instead use the message in the error response

{
  "data": null,
  "error": {
    "errors": [
      {
        "code": "HTTPConflict",
        "message": "Project cannot be modified while pipeline is still running.",
        "resource": null,
        "field": null
      }
    ],
    "status": 409,
    "message": "Project cannot be modified while pipeline is still running."
  }
}

Related issue/s

How to test

Checklist

  • Openapi changes? make openapi-specs, git commit ... and then make version-*)
  • Database migration script? cd packages/postgres-database, make setup-commit, sc-pg review -m "my changes"
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@pcrespov pcrespov self-assigned this Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #2797 (0602014) into master (10b14ad) will decrease coverage by 0.0%.
The diff coverage is 61.4%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2797     +/-   ##
========================================
- Coverage    78.8%   78.8%   -0.1%     
========================================
  Files         670     670             
  Lines       27200   27230     +30     
  Branches     2669    2677      +8     
========================================
+ Hits        21458   21475     +17     
- Misses       4985    4999     +14     
+ Partials      757     756      -1     
Flag Coverage Δ
integrationtests 65.8% <55.7%> (-0.1%) ⬇️
unittests 74.3% <31.5%> (-0.1%) ⬇️

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

Impacted Files Coverage Δ
...r/src/simcore_service_webserver/director_v2_api.py 100.0% <ø> (ø)
...re_service_webserver/projects/projects_handlers.py 81.1% <0.0%> (-1.0%) ⬇️
...mcore_service_webserver/projects/projects_utils.py 72.5% <17.6%> (-8.0%) ⬇️
...odels-library/src/models_library/projects_state.py 98.3% <50.0%> (-1.7%) ⬇️
...ore_service_director_v2/api/routes/computations.py 80.1% <80.0%> (-0.2%) ⬇️
...ages/models-library/src/models_library/projects.py 95.4% <100.0%> (ø)
...rc/simcore_service_director_v2/core/application.py 87.7% <100.0%> (+0.6%) ⬆️
...e_service_director_v2/models/schemas/comp_tasks.py 100.0% <100.0%> (ø)
.../simcore_service_director_v2/utils/computations.py 85.7% <100.0%> (ø)
.../src/simcore_service_webserver/director_v2_core.py 66.8% <100.0%> (+1.1%) ⬆️
... and 5 more

@pcrespov pcrespov marked this pull request as ready for review February 4, 2022 21:03
@pcrespov pcrespov requested review from sanderegg, GitHK and odeimaiz and removed request for sanderegg February 4, 2022 21:04
@pcrespov pcrespov changed the title WIP: ⚗️ Reject saving project while pipeline is running ⚗️ Reject saving project while pipeline is running Feb 4, 2022
@pcrespov pcrespov added a:director-v2 issue related with the director-v2 service a:webserver issue related to the webserver service labels Feb 4, 2022
@pcrespov pcrespov modified the milestone: Rudolph+1 Feb 4, 2022
@pcrespov pcrespov force-pushed the enh/avoid-saving-while-running branch from 3cd31f1 to 5354be2 Compare February 8, 2022 18:49
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

looking good.

Comment on lines +272 to +274
# TODO: how to ensure this list of "links types" is up-to-date!??
# These types represent links that node-ports need to handle
if isinstance(input_value, PortLink, DownloadLink, BaseFileLink):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be provided by some utility function in nodeports and have some tests which makes sure the list is up to date. But it will still not be prefect.

Copy link
Member Author

Choose a reason for hiding this comment

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

the tests are fine but I would rather have something closer to that line ... I added some assertions next to t but the check is for the moment limited.

@pcrespov pcrespov merged commit 306355e into ITISFoundation:master Feb 9, 2022
@pcrespov pcrespov deleted the enh/avoid-saving-while-running branch February 9, 2022 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants