daemon/links: assorted bug fixes and cleanup#49300
Merged
thaJeztah merged 8 commits intomoby:masterfrom Jan 20, 2025
Merged
Conversation
thaJeztah
added
status/2-code-review
area/networking
Member
Author
|
Looks like I still had this in a local branch, and forgot to push after #49232 was merged 😂 |
thaJeztah
commented
Jan 19, 2025
Comment on lines
31
to
32
| // NewLink initializes a new Link struct with the provided options. | ||
| func NewLink(parentIP, childIP, name string, env []string, exposedPorts map[nat.Port]struct{}) *Link { |
Member
Author
There was a problem hiding this comment.
I might open a PR to deprecate and/or move these internal (NewLink and Link) because nothing is using these, and they're only there to assist the "EnvVars" function now
robmry
approved these changes
Jan 20, 2025
| "6379/tcp", | ||
| "6376/udp", | ||
| "6380/tcp", | ||
| "6376/scp", |
Contributor
There was a problem hiding this comment.
Not that it makes much difference - but these should probably be sctp?
Member
Author
There was a problem hiding this comment.
oh, good catch; yes, they should've been.
Let me change those to prevent confusing ourselves!
| } else { | ||
| i++ | ||
|
|
||
| // These env-vars are produces for every port, regardless if they're |
Contributor
There was a problem hiding this comment.
Sub-nit - produces -> produced.
…f not used This function was unconditionally trying to fetch linked container, even if the container was not using the default bridge (the only network that supports legacy links). Also removing the intermediate variable, as daemon.children, through daemon.linkindex.children already returns a variable with a copy of these links. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Encapsulate the "create link -> link.ToEnv" process. Signed-off-by: Sebastiaan van Stijn <[email protected]>
go test -v -bench ^\QBenchmarkLinkMultipleEnv\E$ -run ^$
goos: darwin
goarch: arm64
pkg: github.com/docker/docker/daemon/links
cpu: Apple M1 Pro
BenchmarkLinkMultipleEnv
BenchmarkLinkMultipleEnv-10 92817 12072 ns/op 8516 B/op 316 allocs/op
PASS
Signed-off-by: Sebastiaan van Stijn <[email protected]>
There's no need to loop and sort multiple times; this code picked the first port after sorting, which we already did in this function. Signed-off-by: Sebastiaan van Stijn <[email protected]>
The intent of this sorting was twofold; - the "default" port of the container to be the first TCP port (if present) - consecutive port-mappings with the same protocol to be together so that port-ranges would produce an env-var describing the range. The current sorting would sort TCP ports before UDP (or SCTP) port, but only if they had the same port-number. This could result in range-detection incorrectly combining TCP and UDP (or SCTP) ports in the same range. Signed-off-by: Sebastiaan van Stijn <[email protected]>
The code incorrectly created env-vars for consecutive port numbers with a different protocol; we should only consider ports to be part of a range if they have consecutive port-numbers and have the same protocol. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Rewrite the range-detection logic to prevent duplicate env-vars,
and to avoid looping over the same values multiple times.
Benchmark before / after:
goos: darwin
goarch: arm64
pkg: github.com/docker/docker/daemon/links
cpu: Apple M1 Pro
BenchmarkLinkMultipleEnv
BenchmarkLinkMultipleEnvOld-10 92817 12072 ns/op 8516 B/op 316 allocs/op
BenchmarkLinkMultipleEnvNew-10 149493 7792 ns/op 6435 B/op 213 allocs/op
PASS
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
links_fixes
branch
from
4aa8453 to
76a496a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
daemon: Daemon.setupLinkedContainers: don't fetch linked containers if not used
This function was unconditionally trying to fetch linked container, even
if the container was not using the default bridge (the only network that
supports legacy links).
Also removing the intermediate variable, as daemon.children, through
daemon.linkindex.children already returns a variable with a copy of these
links.
daemon/links: NewLink: simplify map to string conversion
daemon/links: add EnvVars function
Encapsulate the "create link -> link.ToEnv" process.
daemon/links: add BenchmarkLinkMultipleEnv
daemon/links: fix port-sorting with mixed protocols
The intent of this sorting was twofold;
that port-ranges would produce an env-var describing the range.
The current sorting would sort TCP ports before UDP (or SCP) port, but
only if they had the same port-number. This could result in range-detection
incorrectly combining TCP and UDP (or SCP) ports in the same range.
daemon/links: fix port-ranges with mixed protocols
The code incorrectly created env-vars for consecutive port numbers with
a different protocol; we should only consider ports to be part of a range
if they have consecutive port-numbers and have the same protocol.
daemon/links: fix duplicate env-vars and cleanup range-detection
Rewrite the range-detection logic to prevent duplicate env-vars,
and to avoid looping over the same values multiple times.
Benchmark before / after:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)