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

Fix replacing "service:x" with "container:y" #10036

Merged
merged 2 commits into from Dec 2, 2022
Merged

Fix replacing "service:x" with "container:y" #10036

merged 2 commits into from Dec 2, 2022

Conversation

i-ky
Copy link
Contributor

@i-ky i-ky commented Dec 1, 2022

What I did
As mentioned in #9055 (comment) and #9055 (comment), updateServices() gets called from prepareRun() with an array of all containers belonging to the project and taking only first one of them is wrong in general case. For this use case we should construct a map from service name to ID of the first container belonging to that service. This map is then used ro replace service:x with container:y in network_mode:, ipc: and pid:. This change should not break the second use case of updateServices() when it is called from updateProject() with an array of containers already filtered by service (as pointed out in #9055 (comment)). In this case map will contain only one pair and code will behave exactly like before - only replacing mentions of one particular service and leaving all other service:x intact.

Related issue
Fixes #9055.

Signed-off-by: i-ky <gl.ivanovsky@gmail.com>
@ndeloof
Copy link
Contributor

ndeloof commented Dec 2, 2022

With this implemented in updateService you could remove

cnts := c.getObservedState(serviceName)

@codecov-commenter
Copy link

Codecov Report

Base: 75.86% // Head: 75.86% // No change to project coverage 👍

Coverage data is based on head (3ffed45) compared to base (7cf5940).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##               v2   #10036   +/-   ##
=======================================
  Coverage   75.86%   75.86%           
=======================================
  Files           2        2           
  Lines         232      232           
=======================================
  Hits          176      176           
  Misses         49       49           
  Partials        7        7           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: i-ky <gl.ivanovsky@gmail.com>
@i-ky
Copy link
Contributor Author

i-ky commented Dec 2, 2022

With this implemented in updateService you could remove

cnts := c.getObservedState(serviceName)

Do you mean removing filtering by serviceName there and passing all containers to updateServices()? Not sure about that as this would change behaviour. Currently when called from updateProject() updateServices() replaces only service:x (assuming serviceName equals "x"). If we pass all containers, it will replace not just service:x, but also service:y and service:z. Is this desirable? I guess there is a reason why updateProject() gets called with one serviceName at a time in very particular order:

return InDependencyOrder(ctx, project, func(ctx context.Context, name string) error {

@ndeloof ndeloof merged commit c74a77e into docker:v2 Dec 2, 2022
@i-ky i-ky deleted the 9055-fix-service-to-container-mapping branch December 2, 2022 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to run service with network_mode "service:NAME"
3 participants