Skip to content

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

Merged
ndeloof merged 2 commits intodocker:v2from
i-ky:9055-fix-service-to-container-mapping
Dec 2, 2022
Merged

Fix replacing "service:x" with "container:y"#10036
ndeloof merged 2 commits intodocker:v2from
i-ky:9055-fix-service-to-container-mapping

Conversation

@i-ky
Copy link
Copy Markdown
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.

@i-ky i-ky mentioned this pull request Dec 1, 2022
Closed
ndeloof
ndeloof reviewed Dec 2, 2022
View reviewed changes
Comment thread pkg/compose/convergence.go
@ndeloof
Copy link
Copy Markdown
Contributor

ndeloof commented Dec 2, 2022

With this implemented in updateService you could remove

compose/pkg/compose/convergence.go

Line 118 in 7cf5940

cnts := c.getObservedState(serviceName)

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 2, 2022

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.

@i-ky
Copy link
Copy Markdown
Contributor Author

i-ky commented Dec 2, 2022

With this implemented in updateService you could remove

compose/pkg/compose/convergence.go

Line 118 in 7cf5940

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:

compose/pkg/compose/convergence.go

Line 90 in 7cf5940

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

Reviewers

@ndeloof ndeloof ndeloof approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unable to run service with network_mode "service:NAME"

3 participants

@i-ky @ndeloof @codecov-commenter