Merge lp:~axwalk/juju-core/initiate-replicaset-retry into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Retry replicaset.Initiate

Another attempt at retrying replicaset
initiation. The change I made in
https://codereview.appspot.com/98670045/
does not work well, as the error from
replSetInitiate may be spurious; it is
possible to receive an error despite the
config being written.

https://codereview.appspot.com/102000044/

Retry replicaset.Initiate

Another attempt at retrying replicaset
initiation. The change I made in
https://codereview.appspot.com/98670045/
does not work well, as the error from
replSetInitiate may be spurious; it is
possible to receive an error despite the
config being written.

https://codereview.appspot.com/102000044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+221475_code.launchpad.net,

Message:
Please take a look.

Description:
Retry replicaset.Initiate

Another attempt at retrying replicaset
initiation. The change I made in
https://codereview.appspot.com/98670045/
does not work well, as the error from
replSetInitiate may be spurious; it is
possible to receive an error despite the
config being written.

https://code.launchpad.net/~axwalk/juju-core/initiate-replicaset-retry/+merge/221475

(do not edit description out of merge proposal)

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

Affected files (+46, -23 lines):
   A [revision details]
   M replicaset/replicaset.go
   M worker/peergrouper/initiate.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-20140529113014-rt0lph0bic0c117z
+New revision: <email address hidden>

Index: replicaset/replicaset.go
=== modified file 'replicaset/replicaset.go'
--- replicaset/replicaset.go 2014-05-29 10:26:02 +0000
+++ replicaset/replicaset.go 2014-05-30 00:31:44 +0000
@@ -22,6 +22,11 @@
   // initiateAttemptDelay is the amount of time to sleep between failed
   // attempts to replSetInitiate.
   initiateAttemptDelay = 100 * time.Millisecond
+
+ // rsMembersUnreachableError is the error message returned from mongo
+ // when it thinks that replicaset members are unreachable. This can
+ // occur if replSetInitiate is executed shortly after starting up mongo.
+ rsMembersUnreachableError = "all members and seeds must be reachable to
initiate set"
  )

  var logger = loggo.GetLogger("juju.replicaset")
@@ -52,10 +57,8 @@
   logger.Infof("Initiating replicaset with config %#v", cfg)
   var err error
   for i := 0; i < maxInitiateAttempts; i++ {
- monotonicSession.Refresh()
    err = monotonicSession.Run(bson.D{{"replSetInitiate", cfg}}, nil)
- if err != nil {
- logger.Debugf("replSetInitiate failed: %v", err)
+ if err != nil && err.Error() == rsMembersUnreachableError {
     time.Sleep(initiateAttemptDelay)
     continue
    }

Index: worker/peergrouper/initiate.go
=== modified file 'worker/peergrouper/initiate.go'
--- worker/peergrouper/initiate.go 2014-04-18 04:39:58 +0000
+++ worker/peergrouper/initiate.go 2014-05-30 00:31:44 +0000
@@ -5,14 +5,21 @@

  import (
   "fmt"
+ "time"

   "labix.org/v2/mgo"

   "launchpad.net/juju-core/agent"
   "launchpad.net/juju-core/agent/mongo"
   "launchpad.net/juju-core/replicaset"
+ "launchpad.net/juju-core/utils"
  )

+var initiateAttemptStrategy = utils.AttemptStrategy{
+ Total: 30 * time.Second,
+ Delay: 1 * time.Second,
+}
+
  // InitiateMongoParams holds parameters for the MaybeInitiateMongo call.
  type InitiateMongoParams struct {
   // DialInfo specifies how to connect to the mongo server.
@@ -53,24 +60,35 @@
   }
   defer session.Close()

- cfg, err := replicaset.CurrentConfig(session)
- if err == nil && len(cfg.Members) > 0 {
- logger.Infof("replica set configuration already found: %#v", cfg)
- return nil
- }
- if err != nil && err != mgo.ErrNotFound {
- return fmt.Errorf("cannot get replica ...

[Ian Booth (wallyworld)]

I'd prefer if we didn't have to do a string equality check for the
error. Can we fix that before landing?

https://codereview.appspot.com/102000044/diff/1/replicaset/replicaset.go
File replicaset/replicaset.go (right):

https://codereview.appspot.com/102000044/diff/1/replicaset/replicaset.go#newcode29
replicaset/replicaset.go:29: rsMembersUnreachableError = "all members
and seeds must be reachable to initiate set"
I'd like to think there's a error code we can check. I think we can cast
the error to a mongo error struct and look at the error code? Like is
done for IsUnauthorized() in testing/mgo.go

https://codereview.appspot.com/102000044/

[Andrew Wilkins (axwalk)]

On 2014/05/30 00:43:43, wallyworld wrote:
> I'd prefer if we didn't have to do a string equality check for the
error. Can we
> fix that before landing?

https://codereview.appspot.com/102000044/diff/1/replicaset/replicaset.go
> File replicaset/replicaset.go (right):

https://codereview.appspot.com/102000044/diff/1/replicaset/replicaset.go#newcode29
> replicaset/replicaset.go:29: rsMembersUnreachableError = "all members
and seeds
> must be reachable to initiate set"
> I'd like to think there's a error code we can check. I think we can
cast the
> error to a mongo error struct and look at the error code? Like is done
for
> IsUnauthorized() in testing/mgo.go

AFAICT there is no corresponding error code for this error.
Even in testing/mgo.go, isUnauthorized has to resort to comparing error
strings.

https://codereview.appspot.com/102000044/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/102000044/

=== modified file 'replicaset/replicaset.go'
--- replicaset/replicaset.go	2014-05-29 10:26:02 +0000
+++ replicaset/replicaset.go	2014-05-30 00:35:11 +0000
@@ -22,6 +22,11 @@
 	// initiateAttemptDelay is the amount of time to sleep between failed
 	// attempts to replSetInitiate.
 	initiateAttemptDelay = 100 * time.Millisecond
+
+	// rsMembersUnreachableError is the error message returned from mongo
+	// when it thinks that replicaset members are unreachable. This can
+	// occur if replSetInitiate is executed shortly after starting up mongo.
+	rsMembersUnreachableError = "all members and seeds must be reachable to initiate set"
 )
 
 var logger = loggo.GetLogger("juju.replicaset")
@@ -52,10 +57,8 @@
 	logger.Infof("Initiating replicaset with config %#v", cfg)
 	var err error
 	for i := 0; i < maxInitiateAttempts; i++ {
-		monotonicSession.Refresh()
 		err = monotonicSession.Run(bson.D{{"replSetInitiate", cfg}}, nil)
-		if err != nil {
-			logger.Debugf("replSetInitiate failed: %v", err)
+		if err != nil && err.Error() == rsMembersUnreachableError {
 			time.Sleep(initiateAttemptDelay)
 			continue
 		}

=== modified file 'worker/peergrouper/initiate.go'
--- worker/peergrouper/initiate.go	2014-04-18 04:39:58 +0000
+++ worker/peergrouper/initiate.go	2014-05-30 00:35:11 +0000
@@ -5,14 +5,21 @@
 
 import (
 	"fmt"
+	"time"
 
 	"labix.org/v2/mgo"
 
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/agent/mongo"
 	"launchpad.net/juju-core/replicaset"
+	"launchpad.net/juju-core/utils"
 )
 
+var initiateAttemptStrategy = utils.AttemptStrategy{
+	Total: 30 * time.Second,
+	Delay: 1 * time.Second,
+}
+
 // InitiateMongoParams holds parameters for the MaybeInitiateMongo call.
 type InitiateMongoParams struct {
 	// DialInfo specifies how to connect to the mongo server.
@@ -53,24 +60,35 @@
 	}
 	defer session.Close()
 
-	cfg, err := replicaset.CurrentConfig(session)
-	if err == nil && len(cfg.Members) > 0 {
-		logger.Infof("replica set configuration already found: %#v", cfg)
-		return nil
-	}
-	if err != nil && err != mgo.ErrNotFound {
-		return fmt.Errorf("cannot get replica set configuration: %v", err)
-	}
-	err = replicaset.Initiate(
-		session,
-		p.MemberHostPort,
-		mongo.ReplicaSetName,
-		map[string]string{
-			jujuMachineTag: agent.BootstrapMachineId,
-		},
-	)
-	if err != nil {
-		return fmt.Errorf("cannot initiate replica set: %v", err)
-	}
-	return nil
+	// Initiate may fail while mongo is initialising, so we retry until
+	// we succssfully populate the replicaset config.
+	for attempt := initiateAttemptStrategy.Start(); attempt.Next(); {
+		var cfg *replicaset.Config
+		cfg, err = replicaset.CurrentConfig(session)
+		if err == nil && len(cfg.Members) > 0 {
+			logger.Infof("replica set configuration already found: %#v", cfg)
+			return nil
+		}
+		if err != nil && err != mgo.ErrNotFound {
+			return fmt.Errorf("cannot get replica set configuration: %v", err)
+		}
+		err = replicaset.Initiate(
+			session,
+			p.MemberHostPort,
+			mongo.ReplicaSetName,
+			map[string]string{
+				jujuMachineTag: agent.BootstrapMachineId,
+			},
+		)
+		if err == nil {
+			logger.Infof("replica set initiated")
+			return nil
+		}
+		if attempt.HasNext() {
+			logger.Debugf("replica set initiation failed, will retry: %v", err)
+		}
+		// Release sockets, which may have been closed by mgo.
+		session.Refresh()
+	}
+	return fmt.Errorf("cannot initiate replica set: %v", err)
 }