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

🎨 reroute dynamic service stop via dynamic-scheduler #5192

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Dec 20, 2023

What do these changes do?

Reroute the stop command for dynamic services via the dynamic-scheduler.

BONUS:

  • RPCServerError now properly transfers the traceback instead of just the error name. This helps trace where the error originated (see below)
ERROR: [2024-01-02 12:42:38,290/MainProcess] [servicelib.rabbitmq.rpc_interfaces.dynamic_scheduler.services:stop_dynamic_service(183)]  -  Exception: Raised 'servicelib.fastapi.http_client_thin.ClientHttpError' while running 'stop_dynamic_service' method:   File "/home/scu/.venv/lib/python3.10/site-packages/servicelib/rabbitmq/_rpc_router.py", line 55, in _wrapper
    return await func(*args, **kwargs)
  File "/home/scu/.venv/lib/python3.10/site-packages/simcore_service_dynamic_scheduler/api/rpc/_services.py", line 47, in stop_dynamic_service
    return await director_v2_client.stop_dynamic_service(
  File "/home/scu/.venv/lib/python3.10/site-packages/simcore_service_dynamic_scheduler/services/director_v2/_public_client.py", line 80, in stop_dynamic_service
    await self.thin_client.delete_dynamic_service(
  File "/home/scu/.venv/lib/python3.10/site-packages/simcore_service_dynamic_scheduler/services/director_v2/_thin_client.py", line 101, in delete_dynamic_service
    return await _(self)
  File "/home/scu/.venv/lib/python3.10/site-packages/servicelib/fastapi/http_client_thin.py", line 155, in request_wrapper
    raise ClientHttpError(error=e) from e

To check before merging

  • set the DYNAMIC_SCHEDULER_STOP_SERVICE_TIMEOUT to a lower amount and verify that it times out as expected

Related issue/s

How to test

Dev Checklist

  • No ENV changes or I properly updated ENV (read the instruction)
  • dynamic-scheduler now explicitly captures DYNAMIC_SCHEDULER_STOP_SERVICE_TIMEOUT

DevOps Checklist

@GitHK GitHK marked this pull request as ready for review December 20, 2023 13:52
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 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, thanks!
Q: why the naming: _thin_client? what is meant by the "thin"? thanks

@GitHK
Copy link
Contributor Author

GitHK commented Jan 3, 2024

Looking good, thanks! Q: why the naming: _thin_client? what is meant by the "thin"? thanks

@matusdrobuliak66

The thin part of the client is a convenient way to apply retry and enforce http status expectations. It must always return a response. The response it returns can be safely parsed since it will not contain any unexpected errors due to transport or connectivity. This is the idea behind this pattern.

Copy link

sonarqubecloud bot commented Jan 3, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.1% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@YuryHrytsuk YuryHrytsuk left a comment

Choose a reason for hiding this comment

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

Could you link the ENV PR from osparc-config repo?

@GitHK
Copy link
Contributor Author

GitHK commented Jan 3, 2024

Could you link the ENV PR from osparc-config repo?

@YuryHrytsuk added in the PR body above. After it's review and merge I will proceed in merging this PR

@GitHK GitHK merged commit 8824cbd into ITISFoundation:master Jan 3, 2024
54 checks passed
@GitHK GitHK deleted the pr-osparc-redirect-service-stop-via-dynamic-scheduler branch January 3, 2024 10:32
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Feb 14, 2024
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dynamic-scheduler a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants