Skip to content

cli/command/formatter: sort published ports numerically by IP - #7144

Open
hirehamir wants to merge 1 commit into
docker:masterfrom
hirehamir:fix/ps-port-sort
Open

cli/command/formatter: sort published ports numerically by IP#7144
hirehamir wants to merge 1 commit into
docker:masterfrom
hirehamir:fix/ps-port-sort

Conversation

@hirehamir

Copy link
Copy Markdown
Contributor

- What I did

Fixed #7143 - docker ps ordering published ports by the string form of the host IP rather than numerically, so 10.0.0.2 was listed before 9.0.0.1.

- How I did it

comparePorts compared IPs with

i.IP.String() < j.IP.String()

PortSummary.IP is a netip.Addr, which provides Less for exactly this purpose, so this uses that instead.

It also avoids formatting both addresses as strings on every comparison.

- How to verify it

docker run -d --name porttest -p 127.0.0.9:8081:80 -p 127.0.0.10:8080:80 nginx:alpine
docker ps --format '{{.Ports}}'

Before: 127.0.0.10:8080->80/tcp, 127.0.0.9:8081->80/tcp
After: 127.0.0.9:8081->80/tcp, 127.0.0.10:8080->80/tcp

A new case in TestDisplayablePorts covers this.

ports before after
64 1540 allocs/op 394 allocs/op
256 5364 allocs/op 1548 allocs/op

That's a ~74% (3.9x) reduction in allocations in Docker CLI's port-sorting path.

- Human readable description for the release notes

Fixed `docker ps` sorting published ports lexicographically instead of numerically.

`comparePorts` compared host IPs with `i.IP.String() < j.IP.String()`, which
sorts them lexicographically, so "10.0.0.2" ordered before "9.0.0.1" in
`docker ps` output.

This also avoids formatting both addresses as strings on every comparison.
Measured with the added benchmark, which ties every port on the container port:

ports    before               after
64       1540 allocs/op       394 allocs/op
256      5364 allocs/op       1548 allocs/op

That's a ~74% (3.9x) reduction in allocations in Docker CLI's port-sorting path.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

docker ps sorts IPs lexicographically instead of numerically

2 participants