Skip to content

Conversation

@GitHK
Copy link
Contributor

@GitHK GitHK commented Jun 5, 2025

What do these changes do?

Inside nodeports, used only by the dynamic-sidecar, the port download was not behaving as expected.
Replaced aiohttp client with httpx which addresses #7821 that was reported on AWS master.
The issue was very hard to reduce and could only be tested agains AWS master deployment. Also in a certain setup it appeared 100% of cases. In other setups it took a few tries before it manifested.

Related issue/s

How to test

Dev-ops

@GitHK GitHK self-assigned this Jun 5, 2025
@GitHK GitHK added bug buggy, it does not work as expected a:dynamic-sidecar dynamic-sidecar service labels Jun 5, 2025
@codecov
Copy link

codecov bot commented Jun 5, 2025

Codecov Report

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

Project coverage is 88.11%. Comparing base (3d8ae69) to head (d9af07c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7824      +/-   ##
==========================================
+ Coverage   86.74%   88.11%   +1.36%     
==========================================
  Files        1850     1257     -593     
  Lines       71934    53961   -17973     
  Branches     1218      176    -1042     
==========================================
- Hits        62399    47547   -14852     
+ Misses       9193     6356    -2837     
+ Partials      342       58     -284     
Flag Coverage Δ
integrationtests 64.25% <66.66%> (-0.01%) ⬇️
unittests 87.80% <22.22%> (+1.30%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.10% <66.66%> (+0.13%) ⬆️
agent 96.29% <ø> (ø)
api_server 91.73% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.07% <ø> (+0.04%) ⬆️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.08% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 88.87% <ø> (-0.17%) ⬇️
storage 87.78% <ø> (+0.07%) ⬆️
webclient ∅ <ø> (∅)
webserver 83.75% <ø> (ø)

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 3d8ae69...d9af07c. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 5, 2025

@GitHK GitHK added this to the Bazinga! milestone Jun 5, 2025
@GitHK GitHK changed the title 🐛 use aiohttp to download output ports 🐛 use httpx to download output ports Jun 5, 2025
@GitHK GitHK marked this pull request as ready for review June 5, 2025 08:50
Copy link
Collaborator

@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.

👍

@GitHK GitHK added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 5, 2025
@GitHK
Copy link
Contributor Author

GitHK commented Jun 5, 2025

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Jun 5, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 2f0a89d

@mergify mergify bot merged commit 2f0a89d into ITISFoundation:master Jun 5, 2025
95 of 96 checks passed
@GitHK GitHK deleted the pr-osparc-refactor-simcore-sdk branch June 5, 2025 08:56
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.

Please check my comments

response = await stack.enter_async_context(session.get(url))
if response.status == status.HTTP_404_NOT_FOUND:
client = await stack.enter_async_context(
httpx.AsyncClient(
Copy link
Member

Choose a reason for hiding this comment

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

creating a new client everytime is costly and wasting resources.
here you are changing the behavior. I don't want to stop this now, but please take care of that in a next PR, thank you.
Create an issue and let's fix this calmly.

@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 6, 2025
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:dynamic-sidecar dynamic-sidecar service bug buggy, it does not work as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to download output port when starting dynamic-sidecar

4 participants