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

integration-cli: add test for restarting entire swarm cluster #24465

Merged
merged 1 commit into from
Jul 12, 2016
Merged

integration-cli: add test for restarting entire swarm cluster #24465

merged 1 commit into from
Jul 12, 2016

Conversation

LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Jul 8, 2016

func (s *DockerSwarmSuite) TestApiSwarmRestartCluster(c *check.C) {
var nodes []*SwarmDaemon
d1 := s.AddDaemon(c, false, false)
c.Assert(d1.Init(swarm.InitRequest{
Copy link
Member

Choose a reason for hiding this comment

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

This is default for s.AddDaemon(c, true, true)

@thaJeztah thaJeztah added this to the 1.12.0 milestone Jul 9, 2016
// pollFunc is used to periodically execute a check function, it
// returns error after timeout.
func pollFunc(f func() error, timeout time.Duration) error {
if f() == nil {
Copy link
Member

@runcom runcom Jul 10, 2016

Choose a reason for hiding this comment

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

should be f not f() right? it'll call f() or panic if f is nil

Copy link
Member

Choose a reason for hiding this comment

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

I think it's checking for the error returned by f() 😝

Copy link
Member

@runcom runcom Jul 10, 2016

Choose a reason for hiding this comment

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

that's check on line 1579, I think the check here is for nilness - not the return

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this one is for not entering in the loop and start the timer 😉

Copy link
Member

Choose a reason for hiding this comment

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

what's the point of not entering the for with the timer if it returns nil and you exit the for and the function at line 1581? it's a duplicate check then? the function will always exit at that return nil if f() return nil so why not entering the for?

Copy link
Member

Choose a reason for hiding this comment

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

@LK4D4 ? 👼

Signed-off-by: Alexander Morozov <lk4d4@docker.com>
@LK4D4
Copy link
Contributor Author

LK4D4 commented Jul 11, 2016

@tonistiigi I removed polling overall as you suggested.

var leaderFound bool
totalMCount++
var mCount, wCount int
for _, n := range d.listNodes(c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API call gets proxied to the leader, so what we really see is the leader's view of the cluster.

I still think this loop is worth doing, because it confirms that API calls can be run against each of the managers. But it's important to understand that the information returned is really coming from the leader.

@aaronlehmann
Copy link
Contributor

LGTM

1 similar comment
@tonistiigi
Copy link
Member

LGTM

@tonistiigi tonistiigi merged commit 08a2fd7 into moby:master Jul 12, 2016
@LK4D4 LK4D4 deleted the restart_cluster_test branch July 12, 2016 23:12
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

9 participants