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

♻️ dynamic-sidecar rpc interfce namespace is now tied to the node_id #6614

Merged

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Oct 28, 2024

What do these changes do?

Refactor the RPC interface of the dynamic-sidecar. Each sidecar registers its interface tied to its node_id. This way each individual sidecar can be targeted.

To send DiskUsage for some paths mounted by the dynamic-sidecar, from the efs-guardian service:

from servicelib.rabbitmq.rpc_interfaces.dynamic_sidecar.disk_usage import update_disk_usage

node_id = get_service_node_id()
usage = {
    ".data_assets": DiskUsage.from_efs_guardian(used=10, total=100),
    "home_user_workspace": DiskUsage.from_efs_guardian(used=10, total=100)
}
await disk_usage.update_disk_usage(rpc_client, node_id=node_id, usage=usage)

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK self-assigned this Oct 28, 2024
@GitHK GitHK added the a:dynamic-sidecar dynamic-sidecar service label Oct 28, 2024
@GitHK GitHK added this to the MartinKippenberger milestone Oct 28, 2024
@GitHK GitHK marked this pull request as ready for review October 28, 2024 12:52
@GitHK GitHK requested a review from giancarloromeo October 28, 2024 12:52
Copy link

codecov bot commented Oct 28, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.75%. Comparing base (e3b3673) to head (b6e84df).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6614      +/-   ##
==========================================
+ Coverage   86.17%   87.75%   +1.57%     
==========================================
  Files        1562     1395     -167     
  Lines       62760    58950    -3810     
  Branches     2085     1578     -507     
==========================================
- Hits        54085    51729    -2356     
+ Misses       8357     6969    -1388     
+ Partials      318      252      -66     
Flag Coverage Δ
integrationtests 64.83% <100.00%> (+4.04%) ⬆️
unittests 85.56% <62.50%> (-0.43%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 92.14% <ø> (-0.01%) ⬇️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 71.44% <ø> (ø)
pkg_service_library 76.68% <0.00%> (-0.02%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.27% <ø> (ø)
agent 97.16% <ø> (ø)
api_server 89.82% <ø> (ø)
autoscaling 95.26% <ø> (ø)
catalog 89.51% <ø> (ø)
clusters_keeper 98.85% <ø> (ø)
dask_sidecar 91.30% <ø> (ø)
datcore_adapter 94.02% <ø> (ø)
director 58.38% <ø> (ø)
director_v2 90.84% <ø> (+14.61%) ⬆️
dynamic_scheduler 96.62% <ø> (ø)
dynamic_sidecar 89.71% <100.00%> (+0.04%) ⬆️
efs_guardian 87.03% <ø> (ø)
invitations 93.47% <ø> (ø)
osparc_gateway_server 85.15% <ø> (ø)
payments 93.08% <ø> (ø)
resource_usage_tracker 87.36% <ø> (ø)
storage 89.73% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 89.30% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3b3673...b6e84df. Read the comment docs.

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.

Thanks 👍

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

so if understand correctly we will create 1 exchange per node running?
any thoughts about if this is a good design or not?
1 TOPIC exchange vs X exchanges?

@GitHK
Copy link
Contributor Author

GitHK commented Oct 28, 2024

so if understand correctly we will create 1 exchange per node running? any thoughts about if this is a good design or not? 1 TOPIC exchange vs X exchanges?

So the only hit we get here is slightly bigger resource usage to handle the queues. Also after 10000 queues the performance degrades slightly.

In theory I see the point of routing via differente exchanges. The advantage of this is that each sidecar has its own channel with messages dedicated to it. Which can make debugging simpler in the future, when something goes wrong.

@GitHK GitHK enabled auto-merge (squash) October 29, 2024 07:37
@GitHK GitHK requested a review from pcrespov October 29, 2024 07:37
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.

thx

Copy link

@GitHK GitHK modified the milestones: MartinKippenberger, Caveman Oct 29, 2024
@GitHK GitHK merged commit fc1e2a7 into ITISFoundation:master Oct 29, 2024
85 of 88 checks passed
@GitHK GitHK deleted the pr-osparc-sidecar-rpc-interface-change branch October 29, 2024 11:18
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Oct 29, 2024
57 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:dynamic-sidecar dynamic-sidecar service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants