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

PS Improvements: ignore linked names and image lookup cache. #18177

Closed
wants to merge 1 commit into from

Conversation

calavera
Copy link
Contributor

  • Ignore linked container names in the api when they are not necessary.
    This avoids an O(N) traversal of the container tree when we're not
    going to display the linked names for containers. docker ps ignores
    those names by default, so we don't need them in 99% of the cases.
  • Cache image name lookup. Same image names are going to be resolved
    to the same id for a given list of containers. We can cache the result
    of the first operation avoiding an O(N) lookup of images.

Signed-off-by: David Calavera david.calavera@gmail.com

Since: r.Form.Get("since"),
Before: r.Form.Get("before"),
Filters: r.Form.Get("filters"),
IgnoreLinks: httputils.BoolValue(r, "ignoreLinks"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use trunc here too. For me it sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually implemented this at the beginning with trunc, but I think having a dedicated flag is better because we're not using it for anything else but ignoring the links.

Copy link
Contributor

Choose a reason for hiding this comment

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

I dunno, trunc sounds pretty fit for this and doesn't add anything to API. And it means same things from cli point of view. I'd better to try to keep it same if possible and now I don't see any requests for separate flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're not sending the trunc flag to the api currently, we need a new flag regardless. All truncating is client side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, funny. Okay.

@calavera
Copy link
Contributor Author

Some timings with a few big images:

root@docker-flames:~# time sh -c "./docker ps -a --format '{{.Image}}' | sort | uniq"
alpine
golang
quay.io/coreos/etcd
ubuntu

real    0m0.063s
user    0m0.076s
sys     0m0.004s

218 containers in that previous list:

root@docker-flames:~# time sh -c "./docker ps -a  | wc -l"
218

real    0m0.079s
user    0m0.100s
sys     0m0.028s

@cpuguy83
Copy link
Member

If we do #16032 then link lookup will be fast anyway.

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 24, 2015

@cpuguy83 yeah, maybe we should try to merge it :)

@calavera
Copy link
Contributor Author

@cpuguy83 I know. There is still no need to look for links when we don't even display those names by default.

- Ignore linked container names in the api when they are not necessary.
This avoids an O(N) traversal of the container tree when we're not
going to display the linked names for containers. `docker ps` ignores
those names by default, so we don't need them in 99% of the cases.

- Cache image name lookup. Same image names are going to be resolved
to the same id for a given list of containers. We can cache the result
of the first operation avoiding an O(N) lookup of images.

Signed-off-by: David Calavera <david.calavera@gmail.com>
@LK4D4
Copy link
Contributor

LK4D4 commented Nov 30, 2015

sounds ok for me

@tiborvass
Copy link
Contributor

@calavera instead of having ignoreLinks, we could also have a Name field returned by the API, and if people want all the names then they use --format to return .Names explicitly. WDYT?

@calavera
Copy link
Contributor Author

calavera commented Dec 1, 2015

@tiborvass we currently have that .Names field. The default ps format ignore the link names, the --no-trunc flag makes those names to show up. There is still no need to gather the names unless you explicitly want them. We make that explicit in our cli with --no-trunc.

@tiborvass
Copy link
Contributor

@calavera I was suggesting Name in addition to Names

I see what you mean. The right solution here would be to allow API clients to specify filters, a bit like --format in the CLI. But instead of using the go template for API filters something like a comma-separated list parameter (e.g. fields=id,name) would be more appropriate.

@calavera
Copy link
Contributor Author

calavera commented Dec 7, 2015

closing this until new order.

@calavera calavera closed this Dec 7, 2015
@LK4D4
Copy link
Contributor

LK4D4 commented Dec 7, 2015

@calavera Cache fix still can be used I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants