juju-core

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

  1. add-machine-parent-getter
  2. Merge into trunk
Proposed by Ian Booth
Status: Merged
Merged at revision: 1273
Proposed branch: lp:~wallyworld/juju-core/add-machine-parent-getter
Merge into: lp:~juju/juju-core/trunk
Diff against target: 79 lines (+35/-3)
3 files modified
state/container.go (+12/-3)
state/machine.go (+6/-0)
state/machine_test.go (+17/-0)
To merge this branch: bzr merge lp:~wallyworld/juju-core/add-machine-parent-getter
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+168827@code.launchpad.net

Description of the change

Add ParentId() to Machine

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

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

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :
Download full text (3.3 KiB)

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(...

Read more...

Revision history for this message
Tim Penhey (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.

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

Revision history for this message
Tim Penhey (thumper) wrote :

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/

Revision history for this message
William Reade (fwereade) wrote :

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/

Revision history for this message
Ian Booth (wallyworld) wrote :

Please take a look.

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

Revision history for this message
Ian Booth (wallyworld) wrote :

*** 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/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/container.go'
2--- state/container.go 2013-06-11 20:56:00 +0000
3+++ state/container.go 2013-06-12 00:08:27 +0000
4@@ -88,6 +88,16 @@
5 return ops
6 }
7
8+// parentId returns the id of the host machine if machineId a container id, or ""
9+// if machineId is not for a container.
10+func parentId(machineId string) string {
11+ idParts := strings.Split(machineId, "/")
12+ if len(idParts) < 3 {
13+ return ""
14+ }
15+ return strings.Join(idParts[:len(idParts)-2], "/")
16+}
17+
18 // removeContainerRefOps returns the txn.Op's necessary to remove a machine container record.
19 // These include removing the record itself and updating the host machine's children property.
20 func removeContainerRefOps(st *State, machineId string) []txn.Op {
21@@ -98,11 +108,10 @@
22 Remove: true,
23 }
24 // If the machine is a container, figure out it's parent host.
25- idParts := strings.Split(machineId, "/")
26- if len(idParts) < 3 {
27+ parentId := parentId(machineId)
28+ if parentId == "" {
29 return []txn.Op{removeRefOp}
30 }
31- parentId := strings.Join(idParts[:len(idParts)-2], "/")
32 removeParentRefOp := txn.Op{
33 C: st.containerRefs.Name,
34 Id: parentId,
35
36=== modified file 'state/machine.go'
37--- state/machine.go 2013-06-11 22:22:23 +0000
38+++ state/machine.go 2013-06-12 00:08:27 +0000
39@@ -238,6 +238,12 @@
40 return nil, err
41 }
42
43+// ParentId returns the Id of the host machine if this machine is a container.
44+func (m *Machine) ParentId() (string, bool) {
45+ parentId := parentId(m.Id())
46+ return parentId, parentId != ""
47+}
48+
49 type HasContainersError struct {
50 MachineId string
51 ContainerIds []string
52
53=== modified file 'state/machine_test.go'
54--- state/machine_test.go 2013-06-11 22:22:23 +0000
55+++ state/machine_test.go 2013-06-12 00:08:27 +0000
56@@ -35,6 +35,23 @@
57 c.Assert(containers, DeepEquals, []string(nil))
58 }
59
60+func (s *MachineSuite) TestParentId(c *C) {
61+ parentId, ok := s.machine.ParentId()
62+ c.Assert(parentId, Equals, "")
63+ c.Assert(ok, Equals, false)
64+ params := state.AddMachineParams{
65+ ParentId: s.machine.Id(),
66+ ContainerType: state.LXC,
67+ Series: "series",
68+ Jobs: []state.MachineJob{state.JobHostUnits},
69+ }
70+ container, err := s.State.AddMachineWithConstraints(&params)
71+ c.Assert(err, IsNil)
72+ parentId, ok = container.ParentId()
73+ c.Assert(parentId, Equals, s.machine.Id())
74+ c.Assert(ok, Equals, true)
75+}
76+
77 func (s *MachineSuite) TestLifeJobManageEnviron(c *C) {
78 // A JobManageEnviron machine must never advance lifecycle.
79 m, err := s.State.AddMachine("series", state.JobManageEnviron)

Subscribers

People subscribed via source and target branches