Skip to content

verbose info is missing for partial overlay ID#35989

Merged
thaJeztah merged 1 commit into
moby:masterfrom
dani-docker:orca-11380
Jan 25, 2018
Merged

verbose info is missing for partial overlay ID#35989
thaJeztah merged 1 commit into
moby:masterfrom
dani-docker:orca-11380

Conversation

@dani-docker
Copy link
Copy Markdown
Contributor

Signed-off-by: Dani Louca [email protected]

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
Copy Markdown
Contributor Author

ping @mavenugo @nishanttotla @thaJeztah

@fcrisciani
Copy link
Copy Markdown
Contributor

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

@thaJeztah
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@dani-docker there is an import issue

@dani-docker
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@dani-docker
Copy link
Copy Markdown
Contributor Author

comment format adjusted and PR updated

@fcrisciani
Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
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
Copy Markdown
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.

6 participants