Skip to content

Conversation

@mrnicegyu11
Copy link
Member

What do these changes do?

Turns out that in some internal calls the dv0 relies on the docker V1 registry protocol. The registry:3.0.0 seems to by default return v2 specs if the client to the docker registry does not specify what is acceptable. Via the usual Accept headers we can inform the docker registry even on version 3.0.0 that we would like to recieve only oci-image v1 data, which is compatible with the dv0 code.

Bonus:

Update registry to 3.0.0 everywhere

Related issue/s

How to test

Tested locally (make up-devel)

Dev-ops

@mrnicegyu11 mrnicegyu11 added this to the Voyager milestone Aug 20, 2025
@mrnicegyu11 mrnicegyu11 self-assigned this Aug 20, 2025
@mrnicegyu11 mrnicegyu11 added bug buggy, it does not work as expected a:director issue related with the director service labels Aug 20, 2025
@codecov
Copy link

codecov bot commented Aug 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.01%. Comparing base (d9aef02) to head (380e5d3).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8239      +/-   ##
==========================================
- Coverage   88.02%   88.01%   -0.01%     
==========================================
  Files        1916     1916              
  Lines       74053    74053              
  Branches     1301     1301              
==========================================
- Hits        65184    65180       -4     
- Misses       8478     8482       +4     
  Partials      391      391              
Flag Coverage Δ
integrationtests 63.98% <ø> (+0.02%) ⬆️
unittests 86.66% <ø> (-0.02%) ⬇️
Components Coverage Δ
pkg_aws_library 93.93% <ø> (ø)
pkg_celery_library 87.37% <ø> (ø)
pkg_dask_task_models_library 79.62% <ø> (ø)
pkg_models_library 93.04% <ø> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.02% <ø> (ø)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library 71.82% <ø> (ø)
pkg_settings_library 90.17% <ø> (ø)
pkg_simcore_sdk 85.03% <ø> (ø)
agent 93.90% <ø> (ø)
api_server 92.84% <ø> (ø)
autoscaling 95.89% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.37% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.14% <ø> (ø)
director_v2 90.97% <ø> (+0.05%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.05% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.18% <ø> (+0.05%) ⬆️
storage 86.63% <ø> (+0.08%) ⬆️
webclient ∅ <ø> (∅)
webserver 88.06% <ø> (-0.05%) ⬇️

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 d9aef02...380e5d3. 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

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.

Thanks!

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

let's see what others think as well

Comment on lines +86 to +87
"application/vnd.oci.image.manifest.v1+json",
"application/vnd.oci.image.index.v1+json",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not fully convinced about this approach. What about addressing the issue in the code?
I think it's better to use the default version 2 spec instead of requesting the older one I would expect this one to get deprecated at some point.

log_level=ERROR | log_timestamp=2025-08-20 09:01:21,941 | log_source=simcore_service_director.registry_proxy:log_catch(575) | log_uid=None | log_oec=None | log_trace_id=0 | log_span_id=0 | log_msg=Unhandled exception:
Traceback (most recent call last):
  File "/home/scu/.venv/lib/python3.11/site-packages/servicelib/logging_utils.py", line 570, in log_catch
    yield
  File "/home/scu/.venv/lib/python3.11/site-packages/simcore_service_director/registry_proxy.py", line 449, in get_repo_details
    if image_details := await image_details_future:
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/scu/.venv/lib/python3.11/site-packages/simcore_service_director/registry_proxy.py", line 402, in get_image_details
    labels, image_manifest_digest = await get_image_labels(
                                    ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/scu/.venv/lib/python3.11/site-packages/simcore_service_director/registry_proxy.py", line 384, in get_image_labels
    v1_compatibility_key = json_loads(request_result["history"][0]["v1Compatibility"])
                                      ~~~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'history'

Copy link
Member Author

@mrnicegyu11 mrnicegyu11 Aug 20, 2025

Choose a reason for hiding this comment

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

The issue is addressed in the code ;) These are code-changes after all. In case you want to do it differently, feel free to take it over in case ;) I interpret your comment as that I should not quickly get this in (I would have done it this way) but wait for some others. Must I wait for Sylvain, or even Pedro?

@mrnicegyu11 mrnicegyu11 requested a review from GitHK August 20, 2025 11:49
@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2025

🧪 CI Insights

Here's what we observed from your CI run for 380e5d3.

❌ Failed Jobs

Pipeline Job Health on base branch Retries 🔍 CI Insights 📄 Logs
CI [sys] e2e-playwright (3.11, ubuntu-24.04) Healthy 3 View View
system-tests Healthy 3 View View

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on base branch Retries 🔍 CI Insights 📄 Logs
CI unit-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@sanderegg
Copy link
Member

sanderegg commented Aug 20, 2025

@mrnicegyu11 just created #8240 to ensure the dv-0 is compatible with both types. This was adjusted to the newer schema version. Since multi-arch images might become a thing soonish, I guess the dv0 shall become compatible at some point as well. This would be a first step

@mrnicegyu11
Copy link
Member Author

Closing in favor of #8240 thanks a bunch @sanderegg , appreciated!

@sanderegg sanderegg removed this from the Voyager milestone Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:director issue related with the director service bug buggy, it does not work as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants