Add IPv6 address support for mongodb > 2.7.4 #3

macgreagoir · 2016-11-22T16:32:46Z

MongoDB of versions greater than 2.7.4 can use the correct bracketed
IPv6 format ([addr]:port). This change adds support for that format.

frobware · 2016-11-23T11:52:17Z

replicaset.go

-			Address: fixIpv6Address(address),
-			Tags:    tags,
-		}},
+	// We don't know the mongod's ability to use a correct IPv6 addr format


Drop "know the" for "know".

frobware · 2016-11-23T11:52:55Z

replicaset.go

-		}},
+	// We don't know the mongod's ability to use a correct IPv6 addr format
+	// until the server is started, but we need to know before we can start
+	// it. Try the older, incorrect format if the correct format fails.


Try the older, incorrect format, if the correct format fails.

jameinel

Overall I think there are a couple tweaks. I question if we really want to support <=2.6 anymore, but if it isn't hard to do so, we can go with this. One question it raises is why the Initiate failed, given we already had a loop there, it could easily be failing for reasons that aren't the IPv6 address string.

jameinel · 2016-11-23T11:37:39Z

replicaset.go

+	// We don't know the mongod's ability to use a correct IPv6 addr format
+	// until the server is started, but we need to know before we can start
+	// it. Try the older, incorrect format if the correct format fails.
+	cfg := []Config{


configs ? something plural is probably useful since its now a list.

I also wonder if we really want this, or if it would be better to just always use the proper IPv6 form, and drop support for 2.4 + IPv6.

jameinel · 2016-11-23T11:41:08Z

replicaset.go

 	var err error
+	logger.Infof("Initiating replicaset with config %#v", cfg)


should this actually be moved into the loop so we only report 1 config at a time?

jameinel · 2016-11-23T11:46:58Z

replicaset.go

+				time.Sleep(initiateAttemptDelay)
+				continue
+			}
+			break Initiate


I wonder if using labels and break is clearer than a boolean "successful". Or if we'd actually like to pull this out into a separate function and just have that "return".

It does feel like this overall function is getting a little bit big.

Agreed. I actually pulled the change locally to make sure I understood the exit conditions.

jameinel · 2016-11-23T11:58:44Z

replicaset_test.go

 	assertAddRemoveSet(c, root, ipv6GetAddr)
 }

 func (s *MongoIPV6Suite) TestAddressFixing(c *gc.C) {
+	c.Skip("Skipping test until rewritten for good and bad IPv6 addr formats")


can you add a comment as to why this isn't working right now? Maybe just explain directly, but a comment lets someone else come back to address it.

What changed that we can no longer run the tests?

In fact, I'm not sure we now need to skip this, with the fix in the Initiate function. Thanks for the catch.

frobware · 2016-11-23T11:57:34Z

replicaset.go

 	}
-	logger.Infof("Initiating replicaset with config %#v", cfg)
+
 	var err error


This can go inside the for loop as err :=.

This is the var eventually returned by the function, so I think we need it in this higher scope.

frobware · 2016-11-23T12:01:58Z

replicaset.go

-// Turn "bad format" ipv6 addresses ("::1:port"), that mongo uses,  into good
-// format addresses ("[::1]:port").
-func unFixIpv6Address(address string) string {
+// unBreakIpv6Address turns the "bad format" IPv6 addresses ("<addr>:<port>")


suggestion: func formatIPv6AddressWithBrackets.

frobware

It would be good if we could simplify the loop that now has a label.

frobware · 2016-11-23T12:06:03Z

replicaset.go

-func fixIpv6Address(address string) string {
+// breakIpv6Address turns correctly formatted IPv6 addresses into the "bad
+// format" (without brackets around the address) that mongo <2.7 require use.
+func breakIpv6Address(address string) string {


suggestion: func formatIPv6AddressWithoutBrackets.

Fair enough. formatIPv6AddressWith{,out}Brackets are potentially harder to read for their length and difference in the middle (not start) of the names, but certainly more descriptive.

frobware · 2016-11-23T12:06:41Z

replicaset_test.go

 	assertAddRemoveSet(c, root, ipv6GetAddr)
 }

 func (s *MongoIPV6Suite) TestAddressFixing(c *gc.C) {
+	c.Skip("Skipping test until rewritten for good and bad IPv6 addr formats")


What changed that we can no longer run the tests?

jameinel

LGTM, subject to fixing the "break" out of the for loop.

jameinel · 2016-11-24T12:55:15Z

replicaset.go

-		err = monotonicSession.Run(bson.D{{"replSetInitiate", cfg}}, nil)
-		if err != nil && err.Error() == rsMembersUnreachableError {
+		err = attemptInitiate(monotonicSession, cfg)
+		if err != nil {


I think this becomes
if err != nil {
time.Sleep()
} else {
break
}

Otherwise this is triggering maxInitiateAttempts always.

Cheers. In fact, else } break is probably more readable than the continue and fall through to break used in other parts of the function, so I'll fix this missing break and update the other uses.

hoenirvili

LGTM, subject to fixing the "else" stmt.

hoenirvili · 2016-11-24T15:26:15Z

replicaset.go

+		err = monotonicSession.Run(bson.D{{"replSetInitiate", c}}, nil)
+		if err != nil {
+			logger.Infof("Unsuccessful attempt to initiate replicaset: %v", err)
+		} else {


I think you should remove the else and make it like this.

if err = monotonicSession.Run(bson.D{{"replSetInitiate", c}}, nil); err != nil { logger.Infof("Unsuccessful attempt to initiate replicaset: %v", err) } return nil // code

I've very recently added the else for clarity around the early return on success.

@frobware @jameinel what are your thoughts about this?

frobware · 2016-11-24T16:09:32Z

replicaset_test.go

 	root := newServer(c)
 	defer root.Destroy()
 	dialAndTestInitiate(c, root, ipv6GetAddr(root))
+


Drop the blank line. The fewer lines I have to review, the quicker I can review. 👍

frobware · 2016-11-24T16:34:03Z

replicaset_test.go

@@ -127,7 +139,7 @@ func loadData(session *mgo.Session, c *gc.C) {
 	}

 	for col := 0; col < 10; col++ {
-		foos := make([]foo, 10000)
+		foos := make([]interface{}, 10000)


This change seems unnecessary. I tried this locally and it runs/tests OK.

frobware · 2016-11-25T15:02:47Z

replicaset_test.go

@@ -127,7 +139,13 @@ func loadData(session *mgo.Session, c *gc.C) {
 	}

 	for col := 0; col < 10; col++ {
-		foos := make([]foo, 10000)
+		// Testing with mongodb3.2 showed the need to make foos a slice
+		// if interface{} (Insert expects a slice not an empty


of interface. Insert expects a ....

frobware

Minor commentary fix up in loaddata. but LGTM.

MongoDB of versions greater than 2.7.4 can use the correct bracketed IPv6 format ([addr]:port). This change adds support for that format.

macgreagoir · 2016-11-25T15:38:55Z

@jameinel Your issue is now fixed too, so I'll $$merge$$

macgreagoir · 2016-11-25T15:42:31Z

$$merge$$

jujubot · 2016-11-25T16:00:08Z

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-replicaset

frobware reviewed Nov 23, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from c635a3e to 75b082a Compare November 23, 2016 11:58

jameinel reviewed Nov 23, 2016

View reviewed changes

frobware reviewed Nov 23, 2016

View reviewed changes

frobware suggested changes Nov 23, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from 75b082a to 3aacfeb Compare November 23, 2016 16:02

jameinel requested changes Nov 24, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from 3aacfeb to 51945c8 Compare November 24, 2016 13:23

hoenirvili suggested changes Nov 24, 2016

View reviewed changes

frobware reviewed Nov 24, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from 51945c8 to 5969b23 Compare November 24, 2016 17:52

hoenirvili approved these changes Nov 25, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from 5969b23 to 33fff94 Compare November 25, 2016 11:52

frobware reviewed Nov 25, 2016

View reviewed changes

frobware approved these changes Nov 25, 2016

View reviewed changes

macgreagoir force-pushed the ipv6 branch from 33fff94 to 35875ce Compare November 25, 2016 15:18

Add IPv6 address support for mongodb > 2.7.4

3613ba1

MongoDB of versions greater than 2.7.4 can use the correct bracketed IPv6 format ([addr]:port). This change adds support for that format.

macgreagoir force-pushed the ipv6 branch from 35875ce to 3613ba1 Compare November 25, 2016 15:22

jujubot merged commit 6b5becf into juju:master Nov 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPv6 address support for mongodb > 2.7.4 #3

Add IPv6 address support for mongodb > 2.7.4 #3

macgreagoir commented Nov 22, 2016

frobware Nov 23, 2016

frobware Nov 23, 2016

jameinel left a comment

jameinel Nov 23, 2016

jameinel Nov 23, 2016

macgreagoir Nov 23, 2016

jameinel Nov 23, 2016

frobware Nov 23, 2016

macgreagoir Nov 23, 2016

jameinel Nov 23, 2016

frobware Nov 23, 2016

macgreagoir Nov 23, 2016

frobware Nov 23, 2016

macgreagoir Nov 23, 2016

frobware Nov 23, 2016

macgreagoir Nov 23, 2016

frobware left a comment

frobware Nov 23, 2016

macgreagoir Nov 23, 2016

frobware Nov 23, 2016

jameinel left a comment

jameinel Nov 24, 2016

macgreagoir Nov 24, 2016

hoenirvili left a comment

hoenirvili Nov 24, 2016

macgreagoir Nov 24, 2016

hoenirvili Nov 24, 2016

frobware Nov 24, 2016

frobware Nov 24, 2016

frobware Nov 25, 2016

frobware left a comment

macgreagoir commented Nov 25, 2016

macgreagoir commented Nov 25, 2016

jujubot commented Nov 25, 2016

		var err error
		logger.Infof("Initiating replicaset with config %#v", cfg)

Add IPv6 address support for mongodb > 2.7.4 #3

Add IPv6 address support for mongodb > 2.7.4 #3

Conversation

macgreagoir commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameinel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameinel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoenirvili left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware left a comment

Choose a reason for hiding this comment

macgreagoir commented Nov 25, 2016

macgreagoir commented Nov 25, 2016

jujubot commented Nov 25, 2016