client: Use string concatenation instead of Sprintf by aaronlehmann · Pull Request #33536 · moby/moby

aaronlehmann · 2017-06-06T07:50:42Z

Despite my C background, I dislike Sprintf and try to avoid it when I can. I've seen too many printf format string bugs. I tolerate it when it's used for actual formatting, but for simple string concatenation, I much prefer the + operator. It makes code easier to read and allows more compile-time checking.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

AkihiroSuda

LGTM if green

boaz0

👍

thaJeztah · 2017-06-06T12:03:45Z

client/container_copy.go

 	}

-	apiPath := fmt.Sprintf("/containers/%s/archive", container)
+	apiPath := "/containers/" + container + "/archive"


Wondering if we should use path.Join() for these (although they're of course URL's, so probably won't make much of a difference)

I'd be okay with that. I don't have much of a preference either way.

I think for now this is ok, as it's closest to what the existing code does. We should do a whole scan of all URL's we generate, because there's no escaping / handling of special characters in most locations

thaJeztah

LGTM

thaJeztah · 2017-06-06T12:04:49Z

@tophj-ibm flaky test on z?

09:08:20 ----------------------------------------------------------------------
09:08:20 FAIL: docker_cli_swarm_test.go:1141: DockerSwarmSuite.TestSwarmLockUnlockCluster
09:08:20 
09:08:20 [d469126ad1a7e] waiting for daemon to start
09:08:20 [d469126ad1a7e] daemon started
09:08:20 
09:08:20 [d5e1278543061] waiting for daemon to start
09:08:20 [d5e1278543061] daemon started
09:08:20 
09:08:20 [d90eee32cea7e] waiting for daemon to start
09:08:20 [d90eee32cea7e] daemon started
09:08:20 
09:08:20 [d5e1278543061] exiting daemon
09:08:20 [d5e1278543061] waiting for daemon to start
09:08:20 [d5e1278543061] daemon started
09:08:20 
09:08:20 [d5e1278543061] exiting daemon
09:08:20 [d469126ad1a7e] exiting daemon
09:08:20 [d469126ad1a7e] waiting for daemon to start
09:08:20 [d469126ad1a7e] daemon started
09:08:20 
09:08:20 [d90eee32cea7e] exiting daemon
09:08:20 [d90eee32cea7e] waiting for daemon to start
09:08:20 [d90eee32cea7e] daemon started
09:08:20 
09:08:20 [d5e1278543061] waiting for daemon to start
09:08:20 [d5e1278543061] daemon started
09:08:20 
09:08:20 [d5e1278543061] exiting daemon
09:08:20 [d5e1278543061] waiting for daemon to start
09:08:20 [d5e1278543061] daemon started
09:08:20 
09:08:20 [d5e1278543061] exiting daemon
09:08:20 [d5e1278543061] waiting for daemon to start
09:08:20 [d5e1278543061] daemon started
09:08:20 
09:08:20 docker_cli_swarm_test.go:1187:
09:08:20     // d2 is now set to lock
09:08:20     checkSwarmUnlockedToLocked(c, d2)
09:08:20 docker_cli_swarm_test.go:1021:
09:08:20     c.Assert(getNodeStatus(c, d), checker.Equals, swarm.LocalNodeStateLocked)
09:08:20 ... obtained swarm.LocalNodeState = "active"
09:08:20 ... expected swarm.LocalNodeState = "locked"
09:08:20 
09:08:20 [d469126ad1a7e] exiting daemon
09:08:20 [d5e1278543061] exiting daemon
09:08:20 [d90eee32cea7e] exiting daemon

thaJeztah · 2017-06-06T12:05:25Z

Hm, and flaky test on Windows;

08:46:41 ----------------------------------------------------------------------
08:46:41 FAIL: docker_api_exec_test.go:159: DockerSuite.TestExecAPIStartValidCommand
08:46:41 
08:46:41 docker_api_exec_test.go:171:
08:46:41     c.Assert(inspectJSON.ExecIDs, checker.IsNil)
08:46:41 ... value []string = []string{"b133782d1203eb4d192fc9c66f33351d487b3fbbe207c14fab46c50fd586d33a"}
08:46:41

aaronlehmann · 2017-06-06T12:23:24Z

The logic in TestSwarmLockUnlockCluster looks sound to me, but I wonder if the 3 second sleep to allow the node time to catch up with the raft log and delete its KEK is not always enough. I can't think of why it would take longer than 3 seconds though - maybe spurious leader elections on restart?

I guess ideally the test could be rewritten to avoid sleeps and poll the PEM file on disk instead. Then if it still fails, we'd have more confidence that something is actually wrong. There's something very similar here:

moby/integration-cli/docker_api_swarm_test.go

Lines 214 to 224 in 376c75d

    
           waitAndAssert(c, defaultReconciliationTimeout, func(c *check.C) (interface{}, check.CommentInterface) { 
        
           	certBytes, err := ioutil.ReadFile(filepath.Join(d2.Folder, "root", "swarm", "certificates", "swarm-node.crt")) 
        
           	if err != nil { 
        
           		return "", check.Commentf("error: %v", err) 
        
           	} 
        
           	certs, err := helpers.ParseCertificatesPEM(certBytes) 
        
           	if err == nil && len(certs) > 0 && len(certs[0].Subject.OrganizationalUnit) > 0 { 
        
           		return certs[0].Subject.OrganizationalUnit[0], nil 
        
           	} 
        
           	return "", check.Commentf("could not get organizational unit from certificate") 
        
           }, checker.Equals, "swarm-worker")

I can give this a try if I have a chance.

cc @cyli

tophj-ibm · 2017-06-06T13:01:01Z

@thaJeztah it used to be, although I haven't seen it fail in a few months and thought it was fixed.

@aaronlehmann looking at the logs https://jenkins.dockerproject.org/job/Docker-PRs-s390x/3482/ in between the first and second restarts in checkSwarmUnlockedToLocked, the failing daemon starts up (is unlocked), starts its own leader election, and times out with "session initiation timed out". This happens a few times, it complains about not having a leader and then the 2nd restart occurs.

So if I'm reading everything correctly, it does look like a spurious leader election on restart, which makes it never read the logs from the leader and lock.

cyli · 2017-06-06T17:09:12Z

@aaronlehmann @tophj-ibm Thanks for finding that!

thaJeztah · 2017-06-06T19:28:42Z

Don't know what's wrong with WindowsRS1; https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/14661/console

15:18:48 ----------------------------------------------------------------------
15:18:48 FAIL: docker_cli_run_test.go:3992: DockerSuite.TestRunAttachFailedNoLeak
15:18:48 
15:18:48 docker_cli_run_test.go:3996:
15:18:48     runSleepingContainer(c, "--name=test", "-p", "8000:8000")
15:18:48 c:/gopath/src/github.com/docker/docker/pkg/testutil/cmd/command.go:64:
15:18:48     t.Fatalf("at %s:%d - %s", filepath.Base(file), line, err.Error())
15:18:48 ... Error: at cli.go:48 - 
15:18:48 Command:  d:\CI\CI-d5928228b\binary\docker.exe run -d --name=test -p 8000:8000 busybox sleep 240
15:18:48 ExitCode: 125
15:18:48 Error:    exit status 125
15:18:48 Stdout:   15051239cc9bc50337475d0d435ae874c1fb4e80629f9c589d24b0871281df0a
15:18:48 
15:18:48 Stderr:   d:\CI\CI-d5928228b\binary\docker.exe: Error response from daemon: failed to create endpoint test on network nat: HNS failed with error : The object already exists.
15:18:48 
15:18:48 
15:18:48 Failures:
15:18:48 ExitCode was 125 expected 0
15:18:48 Expected no error
15:18:48

thaJeztah · 2017-06-07T09:34:34Z

All green now 🎉

client: Use string concatenation instead of Sprintf

44c9ccc

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

GordonTheTurtle added the status/0-triage label Jun 6, 2017

AkihiroSuda approved these changes Jun 6, 2017

View reviewed changes

boaz0 approved these changes Jun 6, 2017

View reviewed changes

thaJeztah reviewed Jun 6, 2017

View reviewed changes

thaJeztah approved these changes Jun 6, 2017

View reviewed changes

thaJeztah added rebuild/windowsRS1 status/4-merge and removed status/0-triage labels Jun 6, 2017

GordonTheTurtle removed rebuild/windowsRS1 labels Jun 6, 2017

thaJeztah added the rebuild/windowsRS1 label Jun 6, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 6, 2017

thaJeztah merged commit d2a8386 into moby:master Jun 7, 2017

GordonTheTurtle added this to the 17.06.0 milestone Jun 7, 2017

thaJeztah mentioned this pull request Jul 11, 2017

Flaky test: DockerSwarmSuite.TestSwarmLockUnlockCluster #34051

Open

Conversation

aaronlehmann commented Jun 6, 2017

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jun 6, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 6, 2017

Uh oh!

thaJeztah commented Jun 6, 2017

Uh oh!

aaronlehmann commented Jun 6, 2017

Uh oh!

tophj-ibm commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyli commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jun 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tophj-ibm commented Jun 6, 2017 •

edited

Loading

cyli commented Jun 6, 2017 •

edited

Loading

thaJeztah commented Jun 6, 2017 •

edited

Loading