Merge lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine into lp:~go-bot/juju-core/trunk

[Horacio Durán]

Add check for requested networks compatibility

This is still work in progress, the idea is to
check if the requested networks for a unit and
for a machine are compatible.
So far check looks that there is no requesting
to inlude nets excluded in the other and vice
versa.

https://codereview.appspot.com/86430043/

[lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine]

2580. By Horacio Durán on 2014-04-10

Changed the way networks are obtained.
Fixed the corresponding tests
Added a draft of check for changes

2581. By Horacio Durán on 2014-04-10

Go Formatted, added extra comments for context

[William Reade (fwereade)]

firehose of suggestions :)

https://codereview.appspot.com/86430043/diff/20001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode539
state/unit.go:539: return readRequestedNetworks(u.st, u.globalKey())
don't think we need this method now

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode833
state/unit.go:833: machineIncludedNetworks, machineExcludedNetworks, _
:= m.RequestedNetworks()
Basically never ignore errors. Certainly not here ;p.

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode839
state/unit.go:839: unitIncludedNetworks, unitExcludedNetworks, _ :=
networked_service.Networks()
readRequestedNetworks(u.st, serviceGlobalKey(u.doc.Service))

and don't ignore errors :).

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode852
state/unit.go:852: return fmt.Errorf("The requested network %q is
explicitly excluded in the requested machine %q ", includedNetwork,
m.Id())
We tend not to capitalise error messages

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode859
state/unit.go:859: }
This all might be easier with the methods on utils/set.Strings

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode864
state/unit.go:864: Assert: bson.D{{"txn-revno",
networked_service.doc.TxnRevno}},
We definitely don't want to assert on the service doc's txn-revno -- we
care about changes to the networks, which are on a different doc.

Ofc, you don't have access to them at the moment. I'd add a TxnRevno
field to the requestedNetworks doc, and have readRequestedNetworks
return that too; the RequestedNetworks methods can ignore it, and this
method can use it in txns.

Make sure you return -1 if it's missing, though, and assert DocMissing
:).

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode866
state/unit.go:866: err = u.st.runTransaction(ops)
When building this sort of construction:

if err := u.st.runTransaction(ops); err != txn.ErrAborted {
     return err
}

...replaces this line and the subsequent two blocks.

*but* asserting at this point does us no good -- we have to build the
correct asserts and include them in the transaction, below, that
actually does the assignment.

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode887
state/unit.go:887: assert := append(isAliveDoc, bson.D{
(rename this unitAssert for clarity's sake)

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode893
state/unit.go:893: massert := isAliveDoc
(and this to machineAssert)

https://codereview.appspot.com/86430043/diff/20001/state/unit.go#newcode932
state/unit.go:932: }
I would recommend starting off by refactoring the method to have the
shape we want, so it can handle retries, and making sure all original
tests still pass; and then adding in the extra bits. So, something like:

func (u *Unit) assignToMachine(m *Machine, unused bool) (err error) {
     // On the expectation that we'll have more than one possible nil
return,
     // defer the updates to local state.
     defer func() {
         if err != nil {
             u.doc.MachineId = m.doc.Id...

[lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine]

2582. By Horacio Durán on 2014-04-14

Refactored assignToMachine Added proper tests in separate file, added clone to unit and machine

2583. By Horacio Durán on 2014-04-14

Go formatted

2584. By Horacio Durán on 2014-04-14

Go formatte

2585. By Horacio Durán on 2014-04-14

Ensured there is a retry when txnrevno of unit does not match

[William Reade (fwereade)]

couple of comments, let's chat live

https://codereview.appspot.com/86430043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode869
state/unit.go:869: if unit.doc.MachineId != machine.Id() {
This in particular needs to go inside the loop -- unit and machine
assignments can change.

(also, should we be checking that the machine's principals are empty if
unused is true? I think we should...)

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode881
state/unit.go:881: if err := unit.st.supportsUnitPlacement(); err != nil
{
I don't think this is mutable state, is it? we can check it outside the
loop.

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode923
state/unit.go:923: }
Everything from here up to the top of the loop feels like it should all
be inside the assignToMachineOps method -- calculating the ops and
verifying that they're sane should go hand in hand. (this applies to
stuff like the life checks too -- check them while building the txn,
don't just build it blind, wait for it to fail, and then look up why.)

https://codereview.appspot.com/86430043/diff/40001/state/unit.go#newcode940
state/unit.go:940: case unit.doc.Life != Alive:
You shouldn't need this code here. It should all be checked as we
construct the transaction -- we have access to latest-known-state up at
the top of the loop, and we can and should check unit life etc *before*
we try to run a transaction asserting that it's the case.

Apart from anything else, you're inspecting old state here -- it doesn't
necessarily have any bearing on why the txn mechanism rejected the ops.

https://codereview.appspot.com/86430043/

[Horacio Durán (hduran-8)]

Wow i was not aware it was so bad, right now i am on holiday and on a
touristic place very far from a pc. ill Ping you on monday as soon as i
begin my day

El jueves, 17 de abril de 2014, <email address hidden> escribió:

> couple of comments, let's chat live
>
>
> https://codereview.appspot.com/86430043/diff/40001/state/unit.go
> File state/unit.go (right):
>
> https://codereview.appspot.com/86430043/diff/40001/state/
> unit.go#newcode869
> state/unit.go:869: if unit.doc.MachineId != machine.Id() {
> This in particular needs to go inside the loop -- unit and machine
> assignments can change.
>
> (also, should we be checking that the machine's principals are empty if
> unused is true? I think we should...)
>
> https://codereview.appspot.com/86430043/diff/40001/state/
> unit.go#newcode881
> state/unit.go:881: if err := unit.st.supportsUnitPlacement(); err != nil
> {
> I don't think this is mutable state, is it? we can check it outside the
> loop.
>
> https://codereview.appspot.com/86430043/diff/40001/state/
> unit.go#newcode923
> state/unit.go:923: }
> Everything from here up to the top of the loop feels like it should all
> be inside the assignToMachineOps method -- calculating the ops and
> verifying that they're sane should go hand in hand. (this applies to
> stuff like the life checks too -- check them while building the txn,
> don't just build it blind, wait for it to fail, and then look up why.)
>
> https://codereview.appspot.com/86430043/diff/40001/state/
> unit.go#newcode940
> state/unit.go:940: case unit.doc.Life != Alive:
> You shouldn't need this code here. It should all be checked as we
> construct the transaction -- we have access to latest-known-state up at
> the top of the loop, and we can and should check unit life etc *before*
> we try to run a transaction asserting that it's the case.
>
> Apart from anything else, you're inspecting old state here -- it doesn't
> necessarily have any bearing on why the txn mechanism rejected the ops.
>
> https://codereview.appspot.com/86430043/
>

Unmerged revisions

2585. By Horacio Durán on 2014-04-14

Ensured there is a retry when txnrevno of unit does not match

2584. By Horacio Durán on 2014-04-14

Go formatte

2583. By Horacio Durán on 2014-04-14

Go formatted

2582. By Horacio Durán on 2014-04-14

Refactored assignToMachine Added proper tests in separate file, added clone to unit and machine

2581. By Horacio Durán on 2014-04-10

Go Formatted, added extra comments for context

2580. By Horacio Durán on 2014-04-10

Changed the way networks are obtained.
Fixed the corresponding tests
Added a draft of check for changes

2579. By Horacio Durán on 2014-04-10

Ran go fmt

2578. By Horacio Durán on 2014-04-10

Added rough check for network compatibility when adding to machine

=== modified file 'state/machine.go'
--- state/machine.go	2014-04-13 11:06:42 +0000
+++ state/machine.go	2014-04-14 23:52:31 +0000
@@ -134,6 +134,48 @@
 	return machine
 }
 
+func (m *Machine) Clone() *Machine {
+	machineClone := &Machine{
+		st:  m.st,
+		doc: m.doc,
+	}
+
+	machinePrincipals := make([]string, len(m.doc.Principals))
+	for key, value := range m.doc.Principals {
+		machinePrincipals[key] = value
+	}
+	machineClone.doc.Principals = machinePrincipals
+	machineJobs := make([]MachineJob, len(m.doc.Jobs))
+	for key, value := range m.doc.Jobs {
+		machineJobs[key] = value
+	}
+	machineClone.doc.Jobs = machineJobs
+	machineAddresses := make([]address, len(m.doc.Addresses))
+	for key, value := range m.doc.Addresses {
+		machineAddresses[key] = value
+	}
+	machineClone.doc.Addresses = machineAddresses
+	machineMachineAddresses := make([]address, len(m.doc.MachineAddresses))
+	for key, value := range m.doc.MachineAddresses {
+		machineMachineAddresses[key] = value
+	}
+	machineClone.doc.MachineAddresses = machineMachineAddresses
+
+	machineSupportedContainers := make([]instance.ContainerType, len(m.doc.SupportedContainers))
+	for key, value := range m.doc.SupportedContainers {
+		machineSupportedContainers[key] = value
+	}
+	machineClone.doc.SupportedContainers = machineSupportedContainers
+
+	machineClone.annotator = annotator{
+		globalKey: machineClone.globalKey(),
+		tag:       machineClone.Tag(),
+		st:        machineClone.st,
+	}
+	return machineClone
+
+}
+
 // Id returns the machine id.
 func (m *Machine) Id() string {
 	return m.doc.Id
 
=== added file 'state/network_assign_test.go'
--- state/network_assign_test.go	1970-01-01 00:00:00 +0000
+++ state/network_assign_test.go	2014-04-14 23:52:31 +0000
@@ -0,0 +1,88 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package state_test
+
+import (
+	"fmt"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/state"
+)
+
+type NetworkedUnitSuite struct {
+	UnitSuite
+}
+
+var _ = gc.Suite(&NetworkedUnitSuite{})
+
+func (s *NetworkedUnitSuite) SetUpTest(c *gc.C) {
+	includeNetworks := []string{"net1", "net3", "net5"}
+	excludeNetworks := []string{"net2", "net4", "net6"}
+	s.ConnSuite.SetUpTest(c)
+	s.charm = s.AddTestingCharm(c, "wordpress")
+	var err error
+	s.service = s.AddTestingServiceWithNetworks(c, "wordpress", s.charm, includeNetworks, excludeNetworks)
+	c.Assert(err, gc.IsNil)
+	s.unit, err = s.service.AddUnit()
+	c.Assert(err, gc.IsNil)
+	c.Assert(s.unit.Series(), gc.Equals, "quantal")
+}
+
+func (s *NetworkedUnitSuite) TestIncludeExcludeNetworksCompatibilityCheckPasses(c *gc.C) {
+	includeNetworks := []string{"net1", "net3", "net5"}
+	excludeNetworks := []string{"net2", "net4", "net6"}
+
+	template := state.MachineTemplate{
+		Series:          "quantal",
+		Jobs:            []state.MachineJob{state.JobHostUnits},
+		IncludeNetworks: includeNetworks,
+		ExcludeNetworks: excludeNetworks,
+	}
+
+	machine, err := s.State.AddOneMachine(template)
+	c.Assert(err, gc.IsNil)
+
+	// TODO: (hduran-8) I really need to find a way to trigger reload
+	// So I can test that there is a retry but could not think
+	// a proper transactionhook
+
+	err = s.unit.AssignToMachine(machine)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *NetworkedUnitSuite) TestExcludeNetworksCompatibilityCheckFails(c *gc.C) {
+	includeNetworks := []string{"net2", "net4", "net6"}
+	excludeNetworks := []string{"net1", "net3", "net5"}
+
+	template := state.MachineTemplate{
+		Series:          "quantal",
+		Jobs:            []state.MachineJob{state.JobHostUnits},
+		IncludeNetworks: includeNetworks,
+		ExcludeNetworks: excludeNetworks,
+	}
+	machine, err := s.State.AddOneMachine(template)
+	c.Assert(err, gc.IsNil)
+	err = s.unit.AssignToMachine(machine)
+	expected_error := fmt.Errorf("cannot assign unit \"wordpress/0\" to machine 0: the requested networks [\"net1\" \"net3\" \"net5\"] are explicitly excluded in the requested machine \"0\" ")
+	c.Assert(err, gc.DeepEquals, expected_error)
+
+}
+
+func (s *NetworkedUnitSuite) TestInvludeNetworksCompatibilityCheckFails(c *gc.C) {
+	includeNetworks := []string{"net2", "net4", "net6"}
+	excludeNetworks := []string{"net7", "net9", "net11"}
+
+	template := state.MachineTemplate{
+		Series:          "quantal",
+		Jobs:            []state.MachineJob{state.JobHostUnits},
+		IncludeNetworks: includeNetworks,
+		ExcludeNetworks: excludeNetworks,
+	}
+	machine, err := s.State.AddOneMachine(template)
+	c.Assert(err, gc.IsNil)
+	err = s.unit.AssignToMachine(machine)
+	expected_error := fmt.Errorf("cannot assign unit \"wordpress/0\" to machine 0: the requested networks [\"net2\" \"net4\" \"net6\"] are explicitly included in the requested machine \"0\" ")
+	c.Assert(err, gc.DeepEquals, expected_error)
+}
 
=== modified file 'state/unit.go'
--- state/unit.go	2014-04-12 05:53:58 +0000
+++ state/unit.go	2014-04-14 23:52:31 +0000
@@ -22,6 +22,7 @@
 	"launchpad.net/juju-core/state/presence"
 	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/set"
 	"launchpad.net/juju-core/version"
 )
 
@@ -102,6 +103,32 @@
 	return unit
 }
 
+// Provides a reazonably deep copy of the given unit
+func (u *Unit) Clone() *Unit {
+	// The following fields have been omitted from explicit copy
+	// because I did not find reason to deep copy them
+	// CharmURL     *charm.URL
+	// Tools        *tools.Tools `bson:",omitempty"`
+	unitClone := &Unit{st: u.st, doc: u.doc}
+	unitSubordinates := make([]string, len(u.doc.Subordinates))
+	for key, value := range u.doc.Subordinates {
+		unitSubordinates[key] = value
+	}
+	unitClone.doc.Subordinates = unitSubordinates
+	unitPorts := make([]instance.Port, len(u.doc.Ports))
+	for key, value := range u.doc.Ports {
+		unitPorts[key] = value
+	}
+	unitClone.doc.Ports = unitPorts
+
+	unitClone.annotator = annotator{
+		globalKey: unitClone.globalKey(),
+		tag:       unitClone.Tag(),
+		st:        unitClone.st,
+	}
+	return unitClone
+}
+
 // Service returns the service.
 func (u *Unit) Service() (*Service, error) {
 	return u.st.Service(u.doc.Service)
@@ -269,7 +296,7 @@
 			u.doc.Life = Dying
 		}
 	}()
-	unit := &Unit{st: u.st, doc: u.doc}
+	unit := u.Clone()
 	for i := 0; i < 5; i++ {
 		switch ops, err := unit.destroyOps(); err {
 		case errRefresh:
@@ -441,7 +468,7 @@
 
 	// Now we're sure we haven't left any scopes occupied by this unit, we
 	// can safely remove the document.
-	unit := &Unit{st: u.st, doc: u.doc}
+	unit := u.Clone()
 	for i := 0; i < 5; i++ {
 		switch ops, err := unit.removeOps(isDeadDoc); err {
 		case errRefresh:
@@ -816,6 +843,33 @@
 	inUseErr           = stderrors.New("machine is not unused")
 )
 
+func (u *Unit) assignToMachineOps(m *Machine, unused bool) []txn.Op {
+	assert := append(isAliveDoc, bson.D{
+		{"$or", []bson.D{
+			{{"machineid", ""}},
+			{{"machineid", m.Id()}},
+		}},
+	}...)
+	massert := isAliveDoc
+	if unused {
+		massert = append(massert, bson.D{{"clean", bson.D{{"$ne", false}}}}...)
+	}
+	assert = append(assert, bson.D{{"txn-revno", u.doc.TxnRevno}}...)
+	ops := []txn.Op{
+		{
+			C:      u.st.units.Name,
+			Id:     u.doc.Name,
+			Assert: assert,
+			Update: bson.D{{"$set", bson.D{{"machineid", m.doc.Id}}}},
+		}, {
+			C:      u.st.machines.Name,
+			Id:     m.doc.Id,
+			Assert: massert,
+			Update: bson.D{{"$addToSet", bson.D{{"principals", u.doc.Name}}}, {"$set", bson.D{{"clean", false}}}},
+		}}
+	return ops
+}
+
 // assignToMachine is the internal version of AssignToMachine,
 // also used by AssignToUnusedMachine. It returns specific errors
 // in some cases:
@@ -824,80 +878,106 @@
 // - alreadyAssignedErr when the unit has already been assigned
 // - inUseErr when the machine already has a unit assigned (if unused is true)
 func (u *Unit) assignToMachine(m *Machine, unused bool) (err error) {
-	if u.doc.Series != m.doc.Series {
+
+	unit := u.Clone()
+	machine := m.Clone()
+
+	// Static checking
+	if unit.doc.Series != machine.doc.Series {
 		return fmt.Errorf("series does not match")
 	}
-	if u.doc.MachineId != "" {
-		if u.doc.MachineId != m.Id() {
+	if unit.doc.MachineId != "" {
+		if unit.doc.MachineId != machine.Id() {
 			return alreadyAssignedErr
 		}
 		return nil
 	}
-	if u.doc.Principal != "" {
+	if unit.doc.Principal != "" {
 		return fmt.Errorf("unit is a subordinate")
 	}
-	canHost := false
-	for _, j := range m.doc.Jobs {
-		if j == JobHostUnits {
-			canHost = true
-			break
-		}
-	}
-	if !canHost {
-		return fmt.Errorf("machine %q cannot host units", m)
-	}
-	// assignToMachine implies assignment to an existing machine,
-	// which is only permitted if unit placement is supported.
-	if err := u.st.supportsUnitPlacement(); err != nil {
-		return err
-	}
-	assert := append(isAliveDoc, bson.D{
-		{"$or", []bson.D{
-			{{"machineid", ""}},
-			{{"machineid", m.Id()}},
-		}},
-	}...)
-	massert := isAliveDoc
-	if unused {
-		massert = append(massert, bson.D{{"clean", bson.D{{"$ne", false}}}}...)
-	}
-	ops := []txn.Op{{
-		C:      u.st.units.Name,
-		Id:     u.doc.Name,
-		Assert: assert,
-		Update: bson.D{{"$set", bson.D{{"machineid", m.doc.Id}}}},
-	}, {
-		C:      u.st.machines.Name,
-		Id:     m.doc.Id,
-		Assert: massert,
-		Update: bson.D{{"$addToSet", bson.D{{"principals", u.doc.Name}}}, {"$set", bson.D{{"clean", false}}}},
-	}}
-	err = u.st.runTransaction(ops)
-	if err == nil {
-		u.doc.MachineId = m.doc.Id
-		m.doc.Clean = false
-		return nil
-	}
-	if err != txn.ErrAborted {
-		return err
-	}
-	u0, err := u.st.Unit(u.Name())
-	if err != nil {
-		return err
-	}
-	m0, err := u.st.Machine(m.Id())
-	if err != nil {
-		return err
-	}
-	switch {
-	case u0.Life() != Alive:
-		return unitNotAliveErr
-	case m0.Life() != Alive:
-		return machineNotAliveErr
-	case u0.doc.MachineId != "" || !unused:
-		return alreadyAssignedErr
-	}
-	return inUseErr
+	for i := 0; i < 3; i++ {
+
+		// assignToMachine implies assignment to an existing machine,
+		// which is only permitted if unit placement is supported.
+		if err := unit.st.supportsUnitPlacement(); err != nil {
+			return err
+		}
+
+		// Jobs
+		canHost := false
+		for _, j := range m.doc.Jobs {
+			if j == JobHostUnits {
+				canHost = true
+				break
+			}
+		}
+		if !canHost {
+			return fmt.Errorf("machine %q cannot host units", m)
+		}
+
+		// Check non colliding network settings
+		machineIncludedNetworks, machineExcludedNetworks, err := machine.RequestedNetworks()
+		if err != nil {
+			return err
+		}
+
+		unitIncludedNetworks, unitExcludedNetworks, err := readRequestedNetworks(unit.st, serviceGlobalKey(unit.doc.Service))
+		if err != nil {
+			return err
+		}
+
+		machineExcludedSet := set.NewStrings(machineExcludedNetworks...)
+		machineIncludedSet := set.NewStrings(machineIncludedNetworks...)
+		unitExcludedSet := set.NewStrings(unitExcludedNetworks...)
+		unitIncludedSet := set.NewStrings(unitIncludedNetworks...)
+
+		mExcludeUIncludeIntersection := unitIncludedSet.Intersection(machineExcludedSet)
+
+		if mExcludeUIncludeIntersection.Size() > 0 {
+			return fmt.Errorf("the requested networks %q are explicitly excluded in the requested machine %q ", mExcludeUIncludeIntersection.Values(), machine.Id())
+		}
+
+		mIncludeUExcludeIntersection := unitExcludedSet.Intersection(machineIncludedSet)
+
+		if mIncludeUExcludeIntersection.Size() > 0 {
+			return fmt.Errorf("the requested networks %q are explicitly included in the requested machine %q ", mIncludeUExcludeIntersection.Values(), machine.Id())
+		}
+
+		// ops
+		ops := unit.assignToMachineOps(machine, unused)
+		err = u.st.runTransaction(ops)
+
+		if err == nil {
+			u.doc.MachineId = m.doc.Id
+			m.doc.Clean = false
+			return nil
+		}
+
+		if i > 1 {
+			if err != txn.ErrAborted {
+				return err
+			}
+
+			switch {
+			case unit.doc.Life != Alive:
+				return unitNotAliveErr
+			case machine.doc.Life != Alive:
+				return machineNotAliveErr
+			case u.doc.MachineId != "" || !unused:
+				return alreadyAssignedErr
+			}
+		}
+
+		if err := unit.Refresh(); err != nil {
+			return err
+		}
+		if err := machine.Refresh(); err != nil {
+			return err
+		}
+
+	}
+
+	return ErrExcessiveContention
 }
 
 func assignContextf(err *error, unit *Unit, target string) {