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

🐛 Fixes issues with dy-sidecar removal by director-v2 #3029

Merged
merged 16 commits into from
May 10, 2022

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented May 9, 2022

What do these changes do?

Fixes an issue where director-v2 was unable to remove dynamic-sidecars. If a node was delete from the database the status of the dynamic-sidecar could not be saved. This case is now correctly handled.

Extra:

  • reduces logging verbosity of sidecar

Related issue/s

How to test

Checklist

  • Unit tests for the changes exist

@GitHK GitHK added a:director-v2 issue related with the director-v2 service a:dynamic-sidecar dynamic-sidecar service changelog:🐛bugfix labels May 9, 2022
@GitHK GitHK self-assigned this May 9, 2022
@GitHK GitHK modified the milestones: Macarons, Croissant May 9, 2022
@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #3029 (b542997) into master (375cbd3) will increase coverage by 0.9%.
The diff coverage is 70.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3029     +/-   ##
========================================
+ Coverage    78.8%   79.7%   +0.9%     
========================================
  Files         698     698             
  Lines       29226   29245     +19     
  Branches     3758    3761      +3     
========================================
+ Hits        23040   23324    +284     
+ Misses       5361    5079    -282     
- Partials      825     842     +17     
Flag Coverage Δ
integrationtests 65.9% <53.3%> (+1.2%) ⬆️
unittests 75.4% <58.3%> (-0.1%) ⬇️

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

Impacted Files Coverage Δ
...dk/src/simcore_sdk/node_ports_common/exceptions.py 68.6% <0.0%> (+5.0%) ⬆️
..._director_v2/modules/dynamic_sidecar/client_api.py 75.7% <0.0%> (-1.6%) ⬇️
...ervice_dynamic_sidecar/api/containers_extension.py 89.1% <ø> (ø)
...tor_v2/modules/dynamic_sidecar/scheduler/events.py 90.1% <60.0%> (-2.1%) ⬇️
..._director_v2/modules/dynamic_sidecar/docker_api.py 87.1% <100.0%> (ø)
...vice_director_v2/modules/dynamic_sidecar/errors.py 100.0% <100.0%> (ø)
...imcore_service_dynamic_sidecar/core/application.py 96.2% <100.0%> (+0.1%) ⬆️
...ore_service_dynamic_sidecar/core/error_handlers.py 100.0% <100.0%> (ø)
...rvice_datcore_adapter/utils/requests_decorators.py 68.4% <0.0%> (-10.6%) ⬇️
... and 27 more

@GitHK GitHK marked this pull request as ready for review May 9, 2022 11:40
@GitHK GitHK requested review from sanderegg and pcrespov as code owners May 9, 2022 11:40
@GitHK GitHK changed the title 🐛 Fixes issues with service removal 🐛 Fixes issues with dy-sidecar removal by director-v2 May 9, 2022
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.

Please review more carefully error handling. Added some tips below ... can provide also some more concrete solutions later.

@@ -246,6 +247,10 @@ async def service_push_output_ports(
response = await self._client.post(
url, json=port_keys, timeout=self._save_restore_timeout
)
if response.status_code == status.HTTP_404_NOT_FOUND:
# check if the error of the dict is as expected an raise to skip!
if "node_not_found" in response.text:
Copy link
Member

@pcrespov pcrespov May 9, 2022

Choose a reason for hiding this comment

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

Please think carefully about error handling. Checking for a string in a text in order to classify an error is very unreliable and difficult to maintain!

We had a long discussion about how e.g. pydantic deals with error handling. Let's start using all those inputs ...

Some tips:

  • Transform NodeNotFound into http-error class but using structured json-response SEE e.g. http error handler in services/director-v2/src/simcore_service_director_v2/api/errors/http_error.py
  • Create a code system to classify errors (We even extended pydantic mechanism in )


try:
await logged_gather(*tasks)
except NodeportsDidNotFindNodeError:
Copy link
Member

Choose a reason for hiding this comment

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

We should also do a bigger effort in defining exceptions (e.g. with some sort of hierarchy or code)
If for every error we define a convoluted name for an exception, the number of combinations will not only explode but will be impossible to handle.
For instance, It would be nice to associate this type of exception with a node error, with a node id (e.g. the warning gets that info from the scheduler_data instead of the error itself ...)

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.

Pair reviewed

@sonarqubecloud
Copy link

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.5% 0.5% Duplication

@GitHK GitHK merged commit cd864d3 into ITISFoundation:master May 10, 2022
@GitHK GitHK deleted the dysdcr-fixes branch May 10, 2022 18:27
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:dynamic-sidecar dynamic-sidecar service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants