Merge lp:~axwalk/juju-core/apiclient-open-parallel-fixes into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2526
Proposed branch: lp:~axwalk/juju-core/apiclient-open-parallel-fixes
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 141 lines (+48/-12)
3 files modified
state/api/apiclient.go (+33/-11)
state/api/apiclient_test.go (+11/-1)
state/api/export_test.go (+4/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/apiclient-open-parallel-fixes
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+213569@code.launchpad.net

Commit message

state/api: fixes to parallel api.Open

With these changes, api.Open will now
do the right thing if one of the dialers
succeeds before all are started. We also
introduce a short delay between starting
dialers, to avoid unnecessary dial
attempts.

https://codereview.appspot.com/82900045/

Description of the change

state/api: fixes to parallel api.Open

With these changes, api.Open will now
do the right thing if one of the dialers
succeeds before all are started. We also
introduce a short delay between starting
dialers, to avoid unnecessary dial
attempts.

https://codereview.appspot.com/82900045/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (5.1 KiB)

Reviewers: mp+213569_code.launchpad.net,

Message:
Please take a look.

Description:
state/api: fixes to parallel api.Open

With these changes, api.Open will now
do the right thing if one of the dialers
succeeds before all are started. We also
introduce a short delay between starting
dialers, to avoid unnecessary dial
attempts.

https://code.launchpad.net/~axwalk/juju-core/apiclient-open-parallel-fixes/+merge/213569

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/82900045/

Affected files (+49, -12 lines):
   A [revision details]
   M state/api/apiclient.go
   M state/api/apiclient_test.go
   M state/api/export_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140401002201-jdbrb1jd9gomhhci
+New revision: <email address hidden>

Index: state/api/apiclient.go
=== modified file 'state/api/apiclient.go'
--- state/api/apiclient.go 2014-03-31 14:43:24 +0000
+++ state/api/apiclient.go 2014-04-01 03:13:49 +0000
@@ -6,6 +6,7 @@
  import (
   "crypto/tls"
   "crypto/x509"
+ "fmt"
   "io"
   "time"

@@ -84,6 +85,10 @@
  // DialOpts holds configuration parameters that control the
  // Dialing behavior when connecting to a state server.
  type DialOpts struct {
+ // DialAddressInterval is the amount of time to wait
+ // before starting to dial another address.
+ DialAddressInterval time.Duration
+
   // Timeout is the amount of time to wait contacting
   // a state server.
   Timeout time.Duration
@@ -97,8 +102,9 @@
  // parameters for contacting a state server.
  func DefaultDialOpts() DialOpts {
   return DialOpts{
- Timeout: 10 * time.Minute,
- RetryDelay: 2 * time.Second,
+ DialAddressInterval: 50 * time.Millisecond,
+ Timeout: 10 * time.Minute,
+ RetryDelay: 2 * time.Second,
   }
  }

@@ -114,9 +120,16 @@
   try := parallel.NewTry(maxParallelDial, nil)
   defer try.Kill()
   for _, addr := range info.Addrs {
- if err := dialWebsocket(addr, opts, pool, try); err != nil {
+ err := dialWebsocket(addr, opts, pool, try)
+ if err == parallel.ErrStopped {
+ break
+ } else if err != nil {
     return nil, err
    }
+ select {
+ case <-time.After(opts.DialAddressInterval):
+ case <-try.Dead():
+ }
   }
   try.Close()
   result, err := try.Result()
@@ -160,28 +173,36 @@
    RootCAs: rootCAs,
    ServerName: "anything",
   }
+ return try.Start(newWebsocketDialer(cfg, opts))
+}
+
+// new WebsocketDialler returns a function that
+// can be passed to utils/parallel.Try.Start.
+func newWebsocketDialer(cfg *websocket.Config, opts DialOpts) func(<-chan
struct{}) (io.Closer, error) {
   openAttempt := utils.AttemptStrategy{
    Total: opts.Timeout,
    Delay: opts.RetryDelay,
   }
- return try.Start(func(stop <-chan struct{}) (io.Closer, error) {
- err := parallel.ErrStopped
+ return func(stop <-chan struct{}) (io.Closer, error) {
    for a := openAttempt.Start(); a.Next(); {
     select {
     case <-stop:
- break
+ return nil, parallel.ErrStopped
     default:
     }...

Read more...

Revision history for this message
William Reade (fwereade) wrote :

LGTM

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient_test.go
File state/api/apiclient_test.go (right):

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient_test.go#newcode79
state/api/apiclient_test.go:79: c.Assert(err, gc.ErrorMatches, `timed
out connecting to "wss://.*/"`)
much nicer, thanks

https://codereview.appspot.com/82900045/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM, thanks

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go#newcode126
state/api/apiclient.go:126: } else if err != nil {
s/else//

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go#newcode199
state/api/apiclient.go:199: logger.Debugf("error dialing API server,
will retry: %v", err)
perhaps include the cfg.Location here, otherwise when we've got a few of
these happening concurrently, we won't be able to tell which address is
giving which error.

https://codereview.appspot.com/82900045/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go#newcode126
state/api/apiclient.go:126: } else if err != nil {
On 2014/04/01 07:42:07, rog wrote:
> s/else//

Done.

https://codereview.appspot.com/82900045/diff/1/state/api/apiclient.go#newcode199
state/api/apiclient.go:199: logger.Debugf("error dialing API server,
will retry: %v", err)
On 2014/04/01 07:42:07, rog wrote:
> perhaps include the cfg.Location here, otherwise when we've got a few
of these
> happening concurrently, we won't be able to tell which address is
giving which
> error.

Done.

https://codereview.appspot.com/82900045/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (55.2 KiB)

The attempt to merge lp:~axwalk/juju-core/apiclient-open-parallel-fixes into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.098s
ok launchpad.net/juju-core/agent/mongo 0.536s
ok launchpad.net/juju-core/agent/tools 0.182s
ok launchpad.net/juju-core/bzr 5.406s
ok launchpad.net/juju-core/cert 3.323s
ok launchpad.net/juju-core/charm 0.410s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.028s
ok launchpad.net/juju-core/cloudinit/sshinit 0.854s
ok launchpad.net/juju-core/cmd 0.171s
ok launchpad.net/juju-core/cmd/charm-admin 0.805s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 202.385s
ok launchpad.net/juju-core/cmd/jujud 62.235s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 8.193s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.174s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.022s
ok launchpad.net/juju-core/container 0.029s
ok launchpad.net/juju-core/container/factory 0.036s
ok launchpad.net/juju-core/container/kvm 0.169s
ok launchpad.net/juju-core/container/kvm/mock 0.032s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.326s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.273s
ok launchpad.net/juju-core/environs 2.515s
ok launchpad.net/juju-core/environs/bootstrap 9.638s
ok launchpad.net/juju-core/environs/cloudinit 0.477s
ok launchpad.net/juju-core/environs/config 2.177s
ok launchpad.net/juju-core/environs/configstore 0.028s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.682s
ok launchpad.net/juju-core/environs/imagemetadata 0.434s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.034s
ok launchpad.net/juju-core/environs/jujutest 0.215s
ok launchpad.net/juju-core/environs/manual 12.647s
ok launchpad.net/juju-core/environs/simplestreams 0.288s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.915s
ok launchpad.net/juju-core/environs/storage 0.839s
ok launchpad.net/juju-core/environs/sync 41.917s
ok launchpad.net/juju-core/environs/testing 0.131s
ok launchpad.net/juju-core/environs/tools 4.616s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.018s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 20.536s
ok launchpad.net/juju-core/juju/arch ...

Revision history for this message
Go Bot (go-bot) wrote :

The attempt to merge lp:~axwalk/juju-core/apiclient-open-parallel-fixes into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.037s
ok launchpad.net/juju-core/agent/mongo 0.599s
ok launchpad.net/juju-core/agent/tools 0.209s
ok launchpad.net/juju-core/bzr 5.333s
ok launchpad.net/juju-core/cert 2.896s
ok launchpad.net/juju-core/charm 0.469s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.034s
ok launchpad.net/juju-core/cloudinit/sshinit 0.795s
ok launchpad.net/juju-core/cmd 0.227s
ok launchpad.net/juju-core/cmd/charm-admin 0.732s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

warning: building out-of-date packages:
 launchpad.net/goamz/ec2/ec2test
 launchpad.net/goamz/s3/s3test
 github.com/joyent/gomanta/localservices/hook
 github.com/joyent/gomanta/localservices
 github.com/joyent/gomanta/localservices/manta
 github.com/joyent/gosdc/localservices/hook
 github.com/joyent/gosdc/localservices
 github.com/joyent/gosdc/localservices/cloudapi
 launchpad.net/goose/testservices/hook
 launchpad.net/goose/testservices/identityservice
 launchpad.net/goose/testservices
 launchpad.net/goose/testservices/novaservice
 launchpad.net/goose/testservices/swiftservice
 launchpad.net/goose/testservices/openstackservice
installing these packages with 'go test -i ./...' will speed future tests.

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.9 KiB)

The attempt to merge lp:~axwalk/juju-core/apiclient-open-parallel-fixes into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.012s
ok launchpad.net/juju-core/agent 1.198s
ok launchpad.net/juju-core/agent/mongo 0.616s
ok launchpad.net/juju-core/agent/tools 0.283s
ok launchpad.net/juju-core/bzr 5.194s
ok launchpad.net/juju-core/cert 2.605s
ok launchpad.net/juju-core/charm 0.538s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.900s
ok launchpad.net/juju-core/cmd 0.175s
ok launchpad.net/juju-core/cmd/charm-admin 0.789s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 210.121s
ok launchpad.net/juju-core/cmd/jujud 67.035s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 10.774s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.143s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.020s
ok launchpad.net/juju-core/container 0.044s
ok launchpad.net/juju-core/container/factory 0.036s
ok launchpad.net/juju-core/container/kvm 0.208s
ok launchpad.net/juju-core/container/kvm/mock 0.040s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.340s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.249s
ok launchpad.net/juju-core/environs 2.367s
ok launchpad.net/juju-core/environs/bootstrap 11.687s
ok launchpad.net/juju-core/environs/cloudinit 0.477s
ok launchpad.net/juju-core/environs/config 1.551s
ok launchpad.net/juju-core/environs/configstore 0.031s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.673s
ok launchpad.net/juju-core/environs/imagemetadata 0.442s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.042s
ok launchpad.net/juju-core/environs/jujutest 0.173s
ok launchpad.net/juju-core/environs/manual 12.089s
ok launchpad.net/juju-core/environs/simplestreams 0.271s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.020s
ok launchpad.net/juju-core/environs/storage 0.860s
ok launchpad.net/juju-core/environs/sync 42.921s
ok launchpad.net/juju-core/environs/testing 0.149s
ok launchpad.net/juju-core/environs/tools 4.550s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.021s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 19.288s
ok launchpad.net/juju-core/juju/arc...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/api/apiclient.go'
2--- state/api/apiclient.go 2014-03-31 14:43:24 +0000
3+++ state/api/apiclient.go 2014-04-01 07:50:42 +0000
4@@ -6,6 +6,7 @@
5 import (
6 "crypto/tls"
7 "crypto/x509"
8+ "fmt"
9 "io"
10 "time"
11
12@@ -84,6 +85,10 @@
13 // DialOpts holds configuration parameters that control the
14 // Dialing behavior when connecting to a state server.
15 type DialOpts struct {
16+ // DialAddressInterval is the amount of time to wait
17+ // before starting to dial another address.
18+ DialAddressInterval time.Duration
19+
20 // Timeout is the amount of time to wait contacting
21 // a state server.
22 Timeout time.Duration
23@@ -97,8 +102,9 @@
24 // parameters for contacting a state server.
25 func DefaultDialOpts() DialOpts {
26 return DialOpts{
27- Timeout: 10 * time.Minute,
28- RetryDelay: 2 * time.Second,
29+ DialAddressInterval: 50 * time.Millisecond,
30+ Timeout: 10 * time.Minute,
31+ RetryDelay: 2 * time.Second,
32 }
33 }
34
35@@ -114,9 +120,17 @@
36 try := parallel.NewTry(maxParallelDial, nil)
37 defer try.Kill()
38 for _, addr := range info.Addrs {
39- if err := dialWebsocket(addr, opts, pool, try); err != nil {
40+ err := dialWebsocket(addr, opts, pool, try)
41+ if err == parallel.ErrStopped {
42+ break
43+ }
44+ if err != nil {
45 return nil, err
46 }
47+ select {
48+ case <-time.After(opts.DialAddressInterval):
49+ case <-try.Dead():
50+ }
51 }
52 try.Close()
53 result, err := try.Result()
54@@ -160,28 +174,36 @@
55 RootCAs: rootCAs,
56 ServerName: "anything",
57 }
58+ return try.Start(newWebsocketDialer(cfg, opts))
59+}
60+
61+// new WebsocketDialler returns a function that
62+// can be passed to utils/parallel.Try.Start.
63+func newWebsocketDialer(cfg *websocket.Config, opts DialOpts) func(<-chan struct{}) (io.Closer, error) {
64 openAttempt := utils.AttemptStrategy{
65 Total: opts.Timeout,
66 Delay: opts.RetryDelay,
67 }
68- return try.Start(func(stop <-chan struct{}) (io.Closer, error) {
69- err := parallel.ErrStopped
70+ return func(stop <-chan struct{}) (io.Closer, error) {
71 for a := openAttempt.Start(); a.Next(); {
72 select {
73 case <-stop:
74- break
75+ return nil, parallel.ErrStopped
76 default:
77 }
78 logger.Infof("dialing %q", cfg.Location)
79- var conn *websocket.Conn
80- conn, err = websocket.DialConfig(cfg)
81+ conn, err := websocket.DialConfig(cfg)
82 if err == nil {
83 return conn, nil
84 }
85- logger.Debugf("error dialing API server, will retry: %v", err)
86+ if a.HasNext() {
87+ logger.Debugf("error dialing %q, will retry: %v", cfg.Location, err)
88+ } else {
89+ return nil, fmt.Errorf("timed out connecting to %q", cfg.Location)
90+ }
91 }
92- return nil, err
93- })
94+ panic("unreachable")
95+ }
96 }
97
98 func (s *State) heartbeatMonitor() {
99
100=== modified file 'state/api/apiclient_test.go'
101--- state/api/apiclient_test.go 2014-03-31 13:03:59 +0000
102+++ state/api/apiclient_test.go 2014-04-01 07:50:42 +0000
103@@ -11,6 +11,7 @@
104
105 jujutesting "launchpad.net/juju-core/juju/testing"
106 "launchpad.net/juju-core/state/api"
107+ "launchpad.net/juju-core/utils/parallel"
108 )
109
110 type apiclientSuite struct {
111@@ -75,5 +76,14 @@
112 addr := listener.Addr().String()
113 info.Addrs = []string{addr, addr, addr}
114 _, err = api.Open(info, api.DialOpts{})
115- c.Assert(err, gc.ErrorMatches, "websocket.Dial .*: read tcp .*: connection reset by peer")
116+ c.Assert(err, gc.ErrorMatches, `timed out connecting to "wss://.*/"`)
117+}
118+
119+func (s *apiclientSuite) TestDialWebsocketStopped(c *gc.C) {
120+ stopped := make(chan struct{})
121+ f := api.NewWebsocketDialer(nil, api.DialOpts{})
122+ close(stopped)
123+ result, err := f(stopped)
124+ c.Assert(err, gc.Equals, parallel.ErrStopped)
125+ c.Assert(result, gc.IsNil)
126 }
127
128=== modified file 'state/api/export_test.go'
129--- state/api/export_test.go 2013-12-17 15:30:13 +0000
130+++ state/api/export_test.go 2014-04-01 07:50:42 +0000
131@@ -3,6 +3,10 @@
132
133 package api
134
135+var (
136+ NewWebsocketDialer = newWebsocketDialer
137+)
138+
139 // SetServerRoot allows changing the URL to the internal API server
140 // that AddLocalCharm uses in order to test NotImplementedError.
141 func SetServerRoot(c *Client, root string) {

Subscribers

People subscribed via source and target branches

to status/vote changes: