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

List Networks need not pull all the endpoints #30673

Merged
merged 1 commit into from Feb 4, 2017
Merged

Conversation

@mavenugo
Copy link
Contributor

mavenugo commented Feb 2, 2017

Pulling all the endpoints is a very resource heavy operation especially
for Global-scoped networks with a backing KVStore. Such heavy operations
can be fetched for individual network inspect. These are unneccessary
for a simple network list operation.

Signed-off-by: Madhu Venugopal madhu@docker.com

Copy link
Contributor

cpuguy83 left a comment

LGTM

Copy link
Member

vdemeester left a comment

LGTM 🐸

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Feb 2, 2017

Looks like this is a change to the API though 😞

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Feb 2, 2017

I'm going to pull this off merge, because the change indeed needs to be addressed in some way.

@mavenugo

This comment has been minimized.

Copy link
Contributor Author

mavenugo commented Feb 2, 2017

@cpuguy83 @thaJeztah i think the original implementation is a bug and we are fixing it the correct way here. I have seen other bugs filed for the exact same issue (I will point them when i find it). Also the inspect APIs are available to get the data. I don't think this change is going to break most of the users.

@vdemeester

This comment has been minimized.

Copy link
Member

vdemeester commented Feb 2, 2017

@mavenugo true, but this might break people and we should follow a depreciation policy as we do usually. We need, at least, to version this change (so using the same API version endpoint give the same result)

@mavenugo mavenugo force-pushed the mavenugo:nr branch from 406604c to 3560b3f Feb 3, 2017
@mavenugo

This comment has been minimized.

Copy link
Contributor Author

mavenugo commented Feb 3, 2017

@vdemeester @thaJeztah @cpuguy83 @tiborvass Added the version check. PTAL.

@mavenugo mavenugo force-pushed the mavenugo:nr branch from 3560b3f to 7eb1fa4 Feb 3, 2017
return r
}

func (n *networkRouter) buildNetworkDetailResources(nw libnetwork.Network) *types.NetworkResource {

This comment has been minimized.

Copy link
@tiborvass

tiborvass Feb 3, 2017

Collaborator

not tryin to bikeshed but i find the function name a bit weird. How about buildDetailedNetworkResource since NetworkResource is the normal one.

Pulling all the endpoints is a very resource heavy operation especially
for Global-scoped networks with a backing KVStore. Such heavy operations
can be fetched for individual network inspect. These are unneccessary
for a simple network list operation.

Signed-off-by: Madhu Venugopal <madhu@docker.com>
@mavenugo mavenugo force-pushed the mavenugo:nr branch from 7eb1fa4 to f388b7a Feb 3, 2017
@mavenugo

This comment has been minimized.

Copy link
Contributor Author

mavenugo commented Feb 3, 2017

@tiborvass done changing the function name. ptal.

} else {
nr = n.buildNetworkResource(nw)
}
list = append(list, *nr)
}

list, err = filterNetworks(list, netFilters)

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Feb 3, 2017

Member

As a follow up, it may be worth considering to add information not needed for filtering until after the networks have been filtered.

Perhaps create utility functions to append information such as containers and peers (e.g. appendEndpointInfo(list), appendPeerInfo(list))

Copy link
Member

thaJeztah left a comment

left a suggestion for possible improvement (follow up), but LGTM on this one

@tiborvass LGTY?

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Feb 4, 2017

LGTM

@tiborvass tiborvass merged commit 4dbc105 into moby:master Feb 4, 2017
4 checks passed
4 checks passed
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30303 has succeeded
Details
janky Jenkins build Docker-PRs 38917 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9966 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.