Merge lp:~dimitern/juju-core/397-state-networking into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

state: Networking improvements, missed before

In this CL https://codereview.appspot.com/86010044/,
some of the suggestions regarding implementation
details of AddNetwork and AddNetworkInterface were
not included in time. This adds these suggestions.

https://codereview.appspot.com/87470044/
R=rogpeppe

state: Networking improvements, missed before

In this CL https://codereview.appspot.com/86010044/,
some of the suggestions regarding implementation
details of AddNetwork and AddNetworkInterface were
not included in time. This adds these suggestions.

https://codereview.appspot.com/87470044/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+215660_code.launchpad.net,

Message:
Please take a look.

Description:
state: Networking improvements, missed before

In this CL https://codereview.appspot.com/86010044/,
some of the suggestions regarding implementation
details of AddNetwork and AddNetworkInterface were
not included in time. This adds these suggestions.

https://code.launchpad.net/~dimitern/juju-core/397-state-networking/+merge/215660

Requires:
https://code.launchpad.net/~dimitern/juju-core/396-improve-errors/+merge/215652

(do not edit description out of merge proposal)

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

Affected files (+37, -35 lines):
   A [revision details]
   M state/machine.go
   M state/state.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:
<email address hidden>
+New revision:
<email address hidden>

Index: state/machine.go
=== modified file 'state/machine.go'
--- state/machine.go 2014-04-14 12:36:13 +0000
+++ state/machine.go 2014-04-14 13:09:55 +0000
@@ -1036,38 +1036,36 @@
    Insert: doc,
   }}

- for i := 0; i < 5; i++ {
- err = m.st.runTransaction(ops)
- switch err {
- case txn.ErrAborted:
- if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err == nil
{
- return nil, errors.AlreadyExistsf("interface with MAC address %q",
macAddress)
- }
- if _, err = m.st.Network(networkName); err != nil {
- return nil, err
- }
- if err = m.Refresh(); err != nil {
- return nil, err
- } else if m.doc.Life != Alive {
- return nil, fmt.Errorf("machine is not alive")
- } else if m.doc.Nonce != "" {
- msg := "machine already provisioned: dynamic network interfaces not
currently supported"
- return nil, fmt.Errorf(msg)
- }
- case nil:
- // For some reason when using unique indices with mgo, and
- // we have an index violation the error is nil, but the
- // document is not added. So we check if the supposedly
- // successful transaction did actually add the document.
- if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err != nil
{
- return nil, errors.AlreadyExistsf("%q on machine %q", interfaceName,
m.doc.Id)
- }
- return newNetworkInterface(m.st, doc), nil
- default:
- return nil, err
- }
+ err = m.st.runTransaction(ops)
+ switch err {
+ case txn.ErrAborted:
+ if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err == nil {
+ return nil, errors.AlreadyExistsf("interface with MAC address %q",
macAddress)
+ }
+ if _, err = m.st.Network(networkName); err != nil {
+ return nil, err
+ }
+ if err = m.Refresh(); err != nil {
+ return nil, err
+ } else if m.doc.Life != Alive {
+ return nil, fmt.Errorf("machine is not alive")
+ } else if m.doc.Nonce != "" {
+ msg := "machine already provisioned: dynamic network interfaces not
currently supported"
+ return nil, fmt.Errorf(msg)
+ }
+ case nil:
+ // We have a unique key restriction on the InterfaceName
+ // field, which will cause the insert to fail ...

[Roger Peppe (rogpeppe)]

On 2014/04/14 13:15:13, dimitern wrote:
> Please take a look.

LGTM, thanks.

https://codereview.appspot.com/87470044/

[Go Bot (go-bot)]

The prerequisite https://code.launchpad.net/~dimitern/juju-core/396-improve-errors has not yet been merged into https://code.launchpad.net/~go-bot/juju-core/trunk.

=== modified file 'state/machine.go'
--- state/machine.go	2014-04-14 13:14:11 +0000
+++ state/machine.go	2014-04-14 13:14:11 +0000
@@ -1036,38 +1036,36 @@
 		Insert: doc,
 	}}
 
-	for i := 0; i < 5; i++ {
-		err = m.st.runTransaction(ops)
-		switch err {
-		case txn.ErrAborted:
-			if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err == nil {
-				return nil, errors.AlreadyExistsf("interface with MAC address %q", macAddress)
-			}
-			if _, err = m.st.Network(networkName); err != nil {
-				return nil, err
-			}
-			if err = m.Refresh(); err != nil {
-				return nil, err
-			} else if m.doc.Life != Alive {
-				return nil, fmt.Errorf("machine is not alive")
-			} else if m.doc.Nonce != "" {
-				msg := "machine already provisioned: dynamic network interfaces not currently supported"
-				return nil, fmt.Errorf(msg)
-			}
-		case nil:
-			// For some reason when using unique indices with mgo, and
-			// we have an index violation the error is nil, but the
-			// document is not added. So we check if the supposedly
-			// successful transaction did actually add the document.
-			if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err != nil {
-				return nil, errors.AlreadyExistsf("%q on machine %q", interfaceName, m.doc.Id)
-			}
-			return newNetworkInterface(m.st, doc), nil
-		default:
-			return nil, err
-		}
+	err = m.st.runTransaction(ops)
+	switch err {
+	case txn.ErrAborted:
+		if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err == nil {
+			return nil, errors.AlreadyExistsf("interface with MAC address %q", macAddress)
+		}
+		if _, err = m.st.Network(networkName); err != nil {
+			return nil, err
+		}
+		if err = m.Refresh(); err != nil {
+			return nil, err
+		} else if m.doc.Life != Alive {
+			return nil, fmt.Errorf("machine is not alive")
+		} else if m.doc.Nonce != "" {
+			msg := "machine already provisioned: dynamic network interfaces not currently supported"
+			return nil, fmt.Errorf(msg)
+		}
+	case nil:
+		// We have a unique key restriction on the InterfaceName
+		// field, which will cause the insert to fail if there is
+		// another record with the same interface name in the table.
+		// The txn logic does not report insertion errors, so we check
+		// that the record has actually been inserted correctly before
+		// reporting success.
+		if err = m.st.networkInterfaces.FindId(macAddress).One(nil); err != nil {
+			return nil, errors.AlreadyExistsf("%q on machine %q", interfaceName, m.doc.Id)
+		}
+		return newNetworkInterface(m.st, doc), nil
 	}
-	return nil, ErrExcessiveContention
+	return nil, err
 }
 
 // CheckProvisioned returns true if the machine was provisioned with the given nonce.

=== modified file 'state/state.go'
--- state/state.go	2014-04-14 13:14:11 +0000
+++ state/state.go	2014-04-14 13:14:11 +0000
@@ -1059,10 +1059,12 @@
 			return nil, err
 		}
 	case nil:
-		// For some reason when using unique indices with mgo, and
-		// we have an index violation the error is nil, but the
-		// document is not added. So we check if the supposedly
-		// successful transaction did actually add the document.
+		// We have a unique key restriction on the ProviderId field,
+		// which will cause the insert to fail if there is another
+		// record with the same provider id in the table. The txn
+		// logic does not report insertion errors, so we check that
+		// the record has actually been inserted correctly before
+		// reporting success.
 		if _, err = st.Network(name); err != nil {
 			return nil, errors.AlreadyExistsf("network with provider id %q", providerId)
 		}