Merge lp://staging/~wallyworld/juju-core/add-machine-parent-getter into lp://staging/~juju/juju-core/trunk

Reviewers: mp+168827_code.launchpad.net,

Message:
Please take a look.

Description:
Add Parent() to Machine

This branch provides the ability to ask a container for its parent
machine.

https://code.launchpad.net/~wallyworld/juju-core/add-machine-parent-getter/+merge/168827

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/container.go
   M state/machine.go
   M state/machine_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: <email address hidden>
+New revision: <email address hidden>

Index: state/container.go
=== modified file 'state/container.go'
--- state/container.go 2013-06-11 20:56:00 +0000
+++ state/container.go 2013-06-11 22:40:34 +0000
@@ -88,6 +88,16 @@
   return ops
  }

+// parentId returns the id of the host machine if machineId a container
id, or ""
+// if machineId is not for a container.
+func parentId(machineId string) string {
+ idParts := strings.Split(machineId, "/")
+ if len(idParts) < 3 {
+ return ""
+ }
+ return strings.Join(idParts[:len(idParts)-2], "/")
+}
+
  // removeContainerRefOps returns the txn.Op's necessary to remove a
machine container record.
  // These include removing the record itself and updating the host
machine's children property.
  func removeContainerRefOps(st *State, machineId string) []txn.Op {
@@ -98,11 +108,10 @@
    Remove: true,
   }
   // If the machine is a container, figure out it's parent host.
- idParts := strings.Split(machineId, "/")
- if len(idParts) < 3 {
+ parentId := parentId(machineId)
+ if parentId == "" {
    return []txn.Op{removeRefOp}
   }
- parentId := strings.Join(idParts[:len(idParts)-2], "/")
   removeParentRefOp := txn.Op{
    C: st.containerRefs.Name,
    Id: parentId,

Index: state/machine.go
=== modified file 'state/machine.go'
--- state/machine.go 2013-06-11 20:56:00 +0000
+++ state/machine.go 2013-06-11 22:40:34 +0000
@@ -235,6 +235,14 @@
   return nil, err
  }

+func (m *Machine) Parent() (*Machine, error) {
+ parentId := parentId(m.Id())
+ if parentId == "" {
+ return nil, nil
+ }
+ return m.st.Machine(parentId)
+}
+
  type HasContainersError struct {
   MachineId string
   ContainerIds []string

Index: state/machine_test.go
=== modified file 'state/machine_test.go'
--- state/machine_test.go 2013-06-11 02:15:31 +0000
+++ state/machine_test.go 2013-06-11 22:40:34 +0000
@@ -35,6 +35,23 @@
   c.Assert(containers, DeepEquals, []string(nil))
  }

+func (s *MachineSuite) TestParent(c *C) {
+ parent, err := s.machine.Parent()
+ c.Assert(err, IsNil)
+ c.Assert(parent, IsNil)
+ params := state.AddMachineParams{
+ ParentId: s.machine.Id(),
+ ContainerType: state.LXC,
+ Series: "series",
+ Jobs: []state.MachineJob{state.JobHostUnits},
+ }
+ container, err := s.State.AddMachineWithConstraints(&params)
+ c.Assert(err, IsNil)
+ parent, err = container.Parent()
+ c.Assert(err, IsNil)
+ c.Assert(parent.Id(...

https://codereview.appspot.com/10203043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode243
state/machine.go:243: return m.st.Machine(parentId)
Unfortunately this is making an assumption that doesn't always hold.

If I am the provisioner, and I have a state.Machine instance, I won't
necessarily have access to state.

Can we have something that returns ParentId() ?

While this method is OK now, I'm wondering how the API changes are going
to impact it.

https://codereview.appspot.com/10203043/

On 2013/06/11 22:47:06, thumper wrote:
> https://codereview.appspot.com/10203043/diff/1/state/machine.go
> File state/machine.go (right):

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode243
> state/machine.go:243: return m.st.Machine(parentId)
> Unfortunately this is making an assumption that doesn't always hold.

> If I am the provisioner, and I have a state.Machine instance, I won't
> necessarily have access to state.

> Can we have something that returns ParentId() ?

> While this method is OK now, I'm wondering how the API changes are
going to
> impact it.

LGTM - any change to the api will need a bucket load of changes.

https://codereview.appspot.com/10203043/

LGTM with ParentId as suggested.

https://codereview.appspot.com/10203043/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode241
state/machine.go:241: return nil, nil
There's a general expectation that a func returning (*Foo, error) will
give you an error iff it can't give you a non-nil *Foo, so I don't
really like this.

https://codereview.appspot.com/10203043/diff/1/state/machine.go#newcode243
state/machine.go:243: return m.st.Machine(parentId)
On 2013/06/11 22:47:06, thumper wrote:
> Unfortunately this is making an assumption that doesn't always hold.

> If I am the provisioner, and I have a state.Machine instance, I won't
> necessarily have access to state.

> Can we have something that returns ParentId() ?

> While this method is OK now, I'm wondering how the API changes are
going to
> impact it.

Ideally, I think, `ParentId() (string, bool)`, in the name of convention
wrt the state package.

https://codereview.appspot.com/10203043/

Please take a look.

https://codereview.appspot.com/10203043/

*** Submitted:

Add ParentId() to Machine

This branch provides the ability to ask a container for its parent
machine's id.

R=thumper, fwereade
CC=
https://codereview.appspot.com/10203043

https://codereview.appspot.com/10203043/