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.

Sign up for GitHub

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

Jump to bottom

✨ adding docker-api-proxy service ⚠️ #7070

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

✨ adding docker-api-proxy service ⚠️ #7070

wants to merge 49 commits into from

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Jan 23, 2025 •
edited
Loading

What do these changes do?

Why yet another service? It allows to use the docker API from a manager node.
Why is this important? Some services like the dynamic-scheduler require access to it in order to create and remove services and overlay networks (swarm scope network).

Bonuses

  • added a way to combine multiple lifespan generators
  • added shared single docker client tied to the app's lifecycle
  • connected the dynamic-scheduler to the docker-api-proxy

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK self-assigned this Jan 23, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 35.82090% with 43 lines in your changes missing coverage. Please review.

Project coverage is 86.12%. Comparing base (d4d8e65) to head (8f062d2).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7070      +/-   ##
==========================================
- Coverage   87.66%   86.12%   -1.54%     
==========================================
  Files        1636     1631       -5     
  Lines       63966    63839     -127     
  Branches     1179     1087      -92     
==========================================
- Hits        56076    54982    -1094     
- Misses       7578     8545     +967     
  Partials      312      312
Flag Coverage Δ
integrationtests 64.67% <ø> (-10.12%) ⬇️
unittests 84.19% <35.82%> (-1.76%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 94.02% <ø> (ø)
pkg_dask_task_models_library 97.09% <ø> (ø)
pkg_models_library 91.51% <100.00%> (+<0.01%) ⬆️
pkg_notifications_library 84.57% <ø> (ø)
pkg_postgres_database 88.39% <ø> (ø)
pkg_service_integration 70.03% <ø> (ø)
pkg_service_library 73.52% <0.00%> (-0.57%) ⬇️
pkg_settings_library 90.61% <85.71%> (-0.10%) ⬇️
pkg_simcore_sdk 85.50% <ø> (ø)
agent 96.46% <ø> (ø)
api_server 90.55% <ø> (ø)
autoscaling 96.08% <ø> (ø)
catalog 90.33% <ø> (ø)
clusters_keeper 99.24% <ø> (ø)
dask_sidecar 91.26% <ø> (ø)
datcore_adapter 93.19% <ø> (ø)
director 77.01% <ø> (+0.08%) ⬆️
director_v2 91.29% <ø> (ø)
dynamic_scheduler 97.19% <100.00%> (+0.01%) ⬆️
dynamic_sidecar 89.75% <ø> (ø)
efs_guardian 90.25% <ø> (ø)
invitations 93.28% <ø> (ø)
osparc_gateway_server ∅ <ø> (∅)
payments 92.66% <ø> (ø)
resource_usage_tracker 88.62% <ø> (-0.33%) ⬇️
storage 89.57% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 81.18% <ø> (-5.04%) ⬇️

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 d4d8e65...8f062d2. Read the comment docs.

sanderegg
sanderegg reviewed Jan 23, 2025
View reviewed changes
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.

some early comments

.github/workflows/ci-testing-deploy.yml Outdated Show resolved Hide resolved
services/docker-api-proxy/tests/integration/test_docker_api_proxy.py Outdated Show resolved Hide resolved
services/docker-api-proxy/tests/integration/test_docker_api_proxy_autenticated.py Outdated Show resolved Hide resolved
services/docker-api-proxy/tests/integration/test_docker_api_proxy_autenticated.py Outdated Show resolved Hide resolved
services/docker-api-proxy/Dockerfile Outdated Show resolved Hide resolved
services/docker-api-proxy/tests/integration/test_docker_api_proxy.py Outdated Show resolved Hide resolved
@GitHK GitHK requested a review from sanderegg January 23, 2025 13:27
@GitHK GitHK added this to the Singularity milestone Jan 23, 2025
sanderegg
sanderegg reviewed Jan 23, 2025
View reviewed changes
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.

much better thanks!
There are a few things to still fix/think about:

  • dockerfile in root mode
  • who closes the aiohttp client?

packages/service-library/src/servicelib/docker_utils.py Outdated Show resolved Hide resolved
services/docker-api-proxy/requirements/_test.in Outdated Show resolved Hide resolved
services/docker-api-proxy/requirements/ci.txt Show resolved Hide resolved
services/docker-api-proxy/requirements/dev.txt Show resolved Hide resolved
services/docker-api-proxy/Dockerfile Outdated Show resolved Hide resolved
@GitHK GitHK added 🤖-automerge marks PR as ready to be merged for Mergify and removed a:dynamic-scheduler labels Jan 30, 2025
@GitHK GitHK changed the title ✨ adding docker-api-proxy service ✨ adding docker-api-proxy service ⚠️ Jan 31, 2025
pcrespov
pcrespov approved these changes Jan 31, 2025
View reviewed changes
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.

Q: What do we do with the services that already use docker client ? Should they all be migrated to use this other approach

packages/service-library/src/servicelib/docker_utils.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/docker_utils.py Outdated Show resolved Hide resolved
packages/service-library/src/servicelib/fastapi/lifespan_utils.py Outdated Show resolved Hide resolved
@GitHK GitHK mentioned this pull request Jan 31, 2025
Merged
1 task
YuryHrytsuk
YuryHrytsuk requested changes Feb 4, 2025
View reviewed changes
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.

Thanks for your work!

Let's discuss security implications before going ahead.

@GitHK GitHK requested a review from YuryHrytsuk February 4, 2025 13:20
YuryHrytsuk
YuryHrytsuk approved these changes Feb 4, 2025
View reviewed changes
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.

Thanks!

@GitHK please create a ticket to add this service to ops repository and assign to me

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Feb 4, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanderegg sanderegg sanderegg left review comments

@pcrespov pcrespov pcrespov approved these changes

@YuryHrytsuk YuryHrytsuk YuryHrytsuk approved these changes

@giancarloromeo giancarloromeo giancarloromeo approved these changes

@mrnicegyu11 mrnicegyu11 Awaiting requested review from mrnicegyu11 mrnicegyu11 is a code owner

@matusdrobuliak66 matusdrobuliak66 Awaiting requested review from matusdrobuliak66 matusdrobuliak66 is a code owner

Assignees

@GitHK GitHK

Labels
🤖-automerge marks PR as ready to be merged for Mergify
Projects
None yet
Milestone
Singularity
5 participants
@GitHK @giancarloromeo @pcrespov @sanderegg @YuryHrytsuk