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

verbose info is missing for partial overlay ID #35989

Merged
merged 1 commit into from Jan 25, 2018

Conversation

dani-docker
Copy link
Contributor

Signed-off-by: Dani Louca dani.louca@docker.com

The changes in 34302 ignores the network details already collected in the previous backend block and causes docker network inspect -v <overlay_partial_ID to miss the "extra details" provided by the verbose flag.
The same issue exists with the network inspect API when querying a network overlay name and swarm scope, ex: curl --unix-socket /var/run/docker.sock "http:/v1.30/networks/<overlay_name>?verbose=true&scope=swarm"

Steps To Reproduce:

  • create an overlay network and a service that uses it.
    docker network create -d overlay apps
    docker service create --mode global --name runev --network apps alpine watch date
  • get the network partial ID
    docker network ls -f driver=overlay -f name=apps -q
  • inspect the network using using the partial ID
    docker network inspect -v 6yiu2vo3obja

Expected Results:
return all the extra info provided by the verbose flag, including endpoints, services etc...

Actual Results:
verbose information is missing

Same behavior applies to:
curl --unix-socket /var/run/docker.sock "http:/v1.30/networks/apps?verbose=true&scope=swarm"

Fix:
The fix does not alter the behavior added in 34302, it checks if the networkID returned by n.cluster.GetNetwork(term) has an entry in listByPartialID or listByFullName and returns it, otherwise it returns the basic network detail.

@dani-docker
Copy link
Contributor Author

ping @mavenugo @nishanttotla @thaJeztah

@fcrisciani
Copy link
Contributor

@dani-docker can you add a simple test to avoid that this api will end up broken again?

@thaJeztah
Copy link
Member

This part of the code has really become way too complex; we should have a look at rewriting/refactoring

nwk = nwv
} else if nwv, ok := listByFullName[nwk.ID]; ok {
nwk = nwv
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to simplify these two conditions? The statements in these blocks are identical.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thoughts, I think this code is fine.

@dani-docker
Copy link
Contributor Author

@fcrisciani integration test is added.
It basically:

  • creates a swarm with an overlay network
  • deploy a service with 4 tasks
  • test the network inspect with verbose set on: (full network ID, partial network ID and network Name with scope=swarm) and make sure the response contains the tasks created by the service
  • cleanly tear things apart

btw, the swarm/service part is code copied from the service integration test.

@fcrisciani
Copy link
Contributor

@dani-docker there is an import issue

@dani-docker
Copy link
Contributor Author

@fcrisciani
Not sure why my local build/test didn't complain, anyhow, goimports the test file did the trick.
Thanks!

@@ -164,6 +164,13 @@ func (n *networkRouter) getNetwork(ctx context.Context, w http.ResponseWriter, r
// return the network. Skipped using isMatchingScope because it is true if the scope
// is not set which would be case if the client API v1.30
if strings.HasPrefix(nwk.ID, term) || (netconst.SwarmScope == scope) {
//If we have a previous match "backend", return it, we need verbose when enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit, can you put a space after the // of the comment?
applies to both lines and also the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, PR updated

Copy link
Contributor

@fcrisciani fcrisciani left a comment

Choose a reason for hiding this comment

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

small code style nits, rest LGTM

Signed-off-by: Dani Louca <dani.louca@docker.com>
@dani-docker
Copy link
Contributor Author

comment format adjusted and PR updated

@fcrisciani
Copy link
Contributor

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐼

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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

6 participants