comp_task requests are sequentially committed for each node in the project

[GitHK]

What do these changes do?

NOTE: Original PR code was scraped as it did not address the issue

Issue explained by @sanderegg

see original below

THE only one (service) inserting there is the director-v2. the sidecar/services are only updating.
Therefore, I think the problem is the following:

webclient -> PUT /projects/{uuid}
-> webserver saves project AND sends a /POST /computations/{uuid} to the director-v2
-> director-v2 inserts the new computation by deleting and then inserting
PROBLEM: if you move a node the webclient also triggers a PUT cause the node changed, therefore if the user moves his things/add nodes/modify nodes triggering a lot of PUTs, I think the director-v2 receives a string of requests (which are all async). so my guess is that the director-v2 is still treating the request when a nother arrives.

Approved implementation proposal:

see original below

• I create something similar to the run_sequentially_in_context decorator which will be applied to a function responsible for updating each individual node in the project
• This will put calls to this function in a queue which will guarantee FIFO thus applying all the requests from the frontend in sequence.
• The delete recreate logic will still continue to exist and I will remove the upset patter to detect future errors.

Related issue number

How to test

Checklist

• Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
• Unit tests for the changes exist
• Runs in the swarm
• Documentation reflects the changes
• New module? Add your github username to .github/CODEOWNERS

[codecov]

Codecov Report

Merging #2043 (e1ad6ce) into master (78b2e8c) will increase coverage by 0.0%.
The diff coverage is 87.7%.

@@          Coverage Diff           @@
##           master   #2043   +/-   ##
======================================
  Coverage    72.5%   72.6%           
======================================
  Files         404     405    +1     
  Lines       14794   14844   +50     
  Branches     1496    1502    +6     
======================================
+ Hits        10738   10782   +44     
- Misses       3659    3666    +7     
+ Partials      397     396    -1     

Flag	Coverage Δ
integrationtests	63.3% <87.7%> (+0.1%)	⬆️
unittests	66.6% <38.5%> (-0.2%)	⬇️

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

Impacted Files	Coverage Δ
...c/simcore_service_director_v2/utils/async_utils.py	85.4% <85.4%> (ø)
..._director_v2/modules/db/repositories/comp_tasks.py	92.5% <100.0%> (+0.2%)	⬆️
...erver/src/simcore_service_webserver/director_v2.py	85.8% <0.0%> (-2.7%)	⬇️
...ce_webserver/resource_manager/garbage_collector.py	77.5% <0.0%> (-1.0%)	⬇️
.../director/src/simcore_service_director/producer.py	61.1% <0.0%> (+0.6%)	⬆️
...webserver/computation_comp_tasks_listening_task.py	90.0% <0.0%> (+3.3%)	⬆️

[pcrespov]

this pattern is new to me. What is it about? Avoids the director-v2 getting angry? 😤 🤣 😉

[pcrespov]

Some minor optimization ;-)

[pcrespov]

MINOR optimization: export to dict only once

# ... let's upseRT (insert or update) so people do not get upset! ;-)

task_values: Dict[str,Any] = task_db.dict(by_alias=True, exclude_unset=True)
upsert_stmt = insert(comp_tasks).values(**task_values).on_conflict_do_update(
                index_elements=[comp_tasks.c.project_id, comp_tasks.c.node_id],
                set_=task_values,
            )

[sanderegg]

sorry but I am against that change for now.
the first line in the function deletes the comp_tasks so the problem lies somewhere else.

there should be no need to upsert here. the problem is probably more that this code gets called asynchronously several times. doing upserts here will certainly generate some undefined state in the DB.

[GitHK]

sorry but I am against that change for now.
the first line in the function deletes the comp_tasks so the problem lies somewhere else.

there should be no need to upsert here. the problem is probably more that this code gets called asynchronously several times. doing upserts here will certainly generate some undefined state in the DB.

You are correct about your assumption. I thought the same. But for some reason the another process/task is interacting with comp_task causing the item to already exist between the time you delete it and recreate it again, causing a duplicate key error. This happened yesterday on master.

Have a look at the attached log (the error is still present in the deployment).

unexpected_error.log

[sanderegg]

sorry but I am against that change for now.
the first line in the function deletes the comp_tasks so the problem lies somewhere else.
there should be no need to upsert here. the problem is probably more that this code gets called asynchronously several times. doing upserts here will certainly generate some undefined state in the DB.

You are correct about your assumption. I thought the same. But for some reason the another process/task is interacting with comp_task causing the item to already exist between the time you delete it and recreate it again, causing a duplicate key error. This happened yesterday on master.

Have a look at the attached log (the error is still present in the deployment).

unexpected_error.log

yes. as I said. THE only one inserting there is the director-v2. the sidecar/services are only updating.
Therefore, I think the problem is the following:

• webclient -> PUT /projects/{uuid}
• -> webserver saves project AND sends a /POST /computations/{uuid} to the director-v2
• -> director-v2 inserts the new computation by deleting and then inserting
  PROBLEM: if you move a node the webclient also triggers a PUT cause the node changed, therefore if the user moves his things/add nodes/modify nodes triggering a lot of PUTs, I think the director-v2 receives a string of requests (which are all async). so my guess is that the director-v2 is still treating the request when a nother arrives.

[GitHK]

sorry but I am against that change for now.
the first line in the function deletes the comp_tasks so the problem lies somewhere else.
there should be no need to upsert here. the problem is probably more that this code gets called asynchronously several times. doing upserts here will certainly generate some undefined state in the DB.

You are correct about your assumption. I thought the same. But for some reason the another process/task is interacting with comp_task causing the item to already exist between the time you delete it and recreate it again, causing a duplicate key error. This happened yesterday on master.
Have a look at the attached log (the error is still present in the deployment).
unexpected_error.log

yes. as I said. THE only one inserting there is the director-v2. the sidecar/services are only updating.
Therefore, I think the problem is the following:

• webclient -> PUT /projects/{uuid}
• -> webserver saves project AND sends a /POST /computations/{uuid} to the director-v2
• -> director-v2 inserts the new computation by deleting and then inserting
  PROBLEM: if you move a node the webclient also triggers a PUT cause the node changed, therefore if the user moves his things/add nodes/modify nodes triggering a lot of PUTs, I think the director-v2 receives a string of requests (which are all async). so my guess is that the director-v2 is still treating the request when a nother arrives.

That sounds really possible. Would you something like this working here:

• I create something similar to therun_sequentially_in_context decorator which will be applied to a function responsible for updating each individual node in the project
• This will put calls to this function in a queue which will guarantee FIFO thus applying all the requests from the frontend in sequence.
• The delete recreate logic will still continue to exist and I will remove the upset patter to detect future errors.

[sanderegg]

sorry but I am against that change for now.
the first line in the function deletes the comp_tasks so the problem lies somewhere else.
there should be no need to upsert here. the problem is probably more that this code gets called asynchronously several times. doing upserts here will certainly generate some undefined state in the DB.

You are correct about your assumption. I thought the same. But for some reason the another process/task is interacting with comp_task causing the item to already exist between the time you delete it and recreate it again, causing a duplicate key error. This happened yesterday on master.
Have a look at the attached log (the error is still present in the deployment).
unexpected_error.log

yes. as I said. THE only one inserting there is the director-v2. the sidecar/services are only updating.
Therefore, I think the problem is the following:

• webclient -> PUT /projects/{uuid}
• -> webserver saves project AND sends a /POST /computations/{uuid} to the director-v2
• -> director-v2 inserts the new computation by deleting and then inserting
  PROBLEM: if you move a node the webclient also triggers a PUT cause the node changed, therefore if the user moves his things/add nodes/modify nodes triggering a lot of PUTs, I think the director-v2 receives a string of requests (which are all async). so my guess is that the director-v2 is still treating the request when a nother arrives.

That sounds really possible. Would you something like this working here:

• I create something similar to therun_sequentially_in_context decorator which will be applied to a function responsible for updating each individual node in the project
• This will put calls to this function in a queue which will guarantee FIFO thus applying all the requests from the frontend in sequence.
• The delete recreate logic will still continue to exist and I will remove the upset patter to detect future errors.

this sounds like a good proposal.

[sanderegg]

see comment. I think something from master went away

[sanderegg]

all good!