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.

Sign up for GitHub

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

Jump to bottom

Add information for Manager Addresses in the output of docker info #28042

Merged
merged 1 commit into from
Nov 11, 2016
Merged

Add information for Manager Addresses in the output of docker info #28042

merged 1 commit into from
Nov 11, 2016

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Nov 3, 2016 •
edited
Loading

- What I did
As is specified in #28018, it would be useful to know the manager's addresses even in a worker node. This is especially useful when there are many worker nodes in a big cluster.

The information is available in info.Swarm.RemoteManagers.

This fix add the information of Manager Addresses to the output of docker info, to explicitly show it.

- How I did it

- How to verify it

A test has been added for this fix.

Also the new output:

$ docker info
...
Cgroup Driver: cgroupfs
Plugins: 
 Volume: local
 Network: bridge host macvlan null overlay
Swarm: active
 NodeID: bu6m8b45fxu8ac4w4wx26iw19
 Is Manager: false
 Node Address: 127.0.0.1
 Manager Addresses: 127.0.0.1:2477
Runtimes: runc
Default Runtime: runc
...

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cute_funny_animals_21

This fix fixes #28018.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang yongtang mentioned this pull request Nov 3, 2016
Closed
allencloud
allencloud reviewed Nov 3, 2016
View reviewed changes
cli/command/system/info.go
}
sort.Strings(managers)
if len(managers) > 0 {
fmt.Fprintf(dockerCli.Out(), " Manager Addresses: %s\n", strings.Join(managers, " "))
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if there is lots of managers, this line could be some kind of unreadable. Is it OK for separate line for each one? Just my thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @allencloud. The PR has been updated with separated lines.

@allencloud
Copy link
Contributor

Or just show the address of leader manager?

@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from a40642a to e8317cc Compare November 4, 2016 00:36
@yongtang
Copy link
Member Author

yongtang commented Nov 4, 2016

@allencloud To show the lead manager is slightly tricky because it needs to query other managers to get the leader. If there is an error while querying managers, not sure we should return an error (error for docker info), or we should just ignore.

@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from e8317cc to a7c2881 Compare November 4, 2016 04:21
@vdemeester
Copy link
Member

/cc @aluzzardi @stevvooe @aaronlehmann

@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from a7c2881 to db93ca7 Compare November 8, 2016 03:36
@stevvooe
Copy link
Contributor

stevvooe commented Nov 8, 2016

Or just show the address of leader manager?

Also, the utility of knowing the leader is limited, since it may change at any time. From the perspective of a client, the managers should be treated equally. The current leader should only be of interest the quorum.

@allencloud
Copy link
Contributor

Thanks @stevvooe
I think we need to think more about whether to bring this or how to bring this better.
If we have lots of managers, docker info would be not so reasonable than before with a list of manager address. I got your point of leader's meaningless for client.

In addition, a rebase needed, though. @yongtang

@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from db93ca7 to 3434635 Compare November 9, 2016 02:37
@yongtang
Copy link
Member Author

yongtang commented Nov 9, 2016

Thanks @allencloud, rebased.

@thaJeztah
Copy link
Member

Sorry @yongtang rebase needed again 😢 (it's busy before code freeze 😄)

@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from 3434635 to 7083b29 Compare November 10, 2016 17:18
@yongtang
Copy link
Member Author

Thanks @thaJeztah. The PR has been rebased. 😅

@aaronlehmann
Copy link
Contributor

LGTM

@tonistiigi
Copy link
Member

@yongtang needs rebase

LGTM

@yongtang
Add information for Manager Addresses in the output of docker info
828bd44 
As is specified in 28018, it would be useful to know the manager's addresses
even in a worker node. This is especially useful when there are many
worker nodes in a big cluster.

The information is available in `info.Swarm.RemoteManagers`.

This fix add the information of `Manager Addresses` to the output
of `docker info`, to explicitly show it.

A test has been added for this fix.

This fix fixes 28018.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the 28018-docker-info-swarm-managers branch from 7083b29 to 828bd44 Compare November 11, 2016 15:39
@yongtang
Copy link
Member Author

Thanks @tonistiigi @aaronlehmann for the review. The PR has been rebased.

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 11, 2016
@thaJeztah
Copy link
Member

I added this to the 1.13 milestone, @yongtang can you do a follow up PR for docs?

@thaJeztah thaJeztah mentioned this pull request Nov 11, 2016
Merged
@vieux vieux merged commit 6f3720c into moby:master Nov 11, 2016
@yongtang yongtang mentioned this pull request Nov 11, 2016
Merged
@yongtang
Copy link
Member Author

Thanks @thaJeztah . Since this PR only changes the output of docker info and there is no API changes, I only updated the docs/man for docker info in PR #28316.

@thaJeztah
Copy link
Member

Perfect, thanks! i'll review later

@yongtang yongtang deleted the 28018-docker-info-swarm-managers branch November 11, 2016 17:56
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
@vieux
Merge pull request moby#28042 from yongtang/28018-docker-info-swarm-m…
460a7e2 
…anagers

Add information for `Manager Addresses` in the output of `docker info`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allencloud allencloud allencloud left review comments

Assignees
No one assigned
Labels
impact/changelog impact/cli impact/documentation status/4-merge
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

[Question] Can worker in Swarm get address of managers?
9 participants
@yongtang @allencloud @vdemeester @stevvooe @thaJeztah @aaronlehmann @tonistiigi @vieux @GordonTheTurtle