Started Machine integration into the state package

A first raw Machine type has been added to add the Machine
related methods to unit. Additionally all Machine related
methods in topology have been added. Beside this major topic
Unit has been extended by few further methods.

https://code.launchpad.net/~themue/juju/go-state-first-machine-integration/+merge/92111

(do not edit description out of merge proposal)

[TheMue, 2012-02-08 19:17:32]

Please take a look.

[niemeyer, 2012-02-08 20:30:14]

Very good overall. Thanks Frank.

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

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode19
state/machine.go:19: // Key returns the public key of the machine.
// Id returns the machine id.

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode20
state/machine.go:20: func (m *Machine) Key() string {
s/Key/Id/
s/string/int/

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode25
state/machine.go:25: func (m *Machine) InternalKey() string {
Please drop this method. This is private information of the package.

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode41
state/machine.go:41: func publicMachineKey(internalKey string) string {
// machineId returns the machine id corresponding to machineKey.
func machineId(machineKey string) (id int) {
    if machineKey == "" {
        panic("machineId: empty machine key")
    }
    i := strings.Index(machineKey, "-")
    var err error
    if i >= 0 {
        id, err = strconv.ParseInt(machineKey[i+1:], 10, 32)
    }
    if i < 0 || err != nil {
        panic("machineId: invalid machine key: " + machineKey)
    }
    return int(id)
}

https://codereview.appspot.com/5647055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode140
state/state_test.go:140: charm := state.CharmMock("local:myseries/mytest-1")
It'd be great to get rid of CharmMock next. This branch is increasing the
dependency on logic that has to be removed.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode163
state/state_test.go:163: // URL validation is done by charm.ParseURL().
Drop second sentence. Sure, ParseURL does validation, but why is that relevant
here?

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode245
state/state_test.go:245: // named unit fails nicely.
It was right before.. s/fails/fail/. You can do s/or/and/ as well, to be more
clear. We're talking about both.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode343
state/state_test.go:343: // URL validation is done by charm.ParseURL().
Please drop second sentence too.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode539
state/state_test.go:539: // Create root machine that shouldn't be useds.
// Create machine 0, that shouldn't be used.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode542
state/state_test.go:542: // Check that assigning without unuused machine fails.
s/unuused/unused/

https://codereview.appspot.com/5647055/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode87
state/topology.go:87: if t.HasMachine(key) {
else if? It Machines == nil it can't have a machine.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode93
state/topology.go:93: 
// Comment here.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode95
state/topology.go:95: err := t.assertMachine(key)
No need to assert here. MachineHasUnits will check already.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode106
state/topology.go:106: // Machine exists and as no units, so remove it.
s/as/has/

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode121
state/topology.go:121: // HasMachine returns true if a machine with the given
key exists.
// HasMachine returns whether a machine with key exists.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode126
state/topology.go:126: // MachineHasUnits returns true if one of the units is
assigned tp the machine.
// MachineHasUnits returns whether the machine with key has any units assigned
to it.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode295
state/topology.go:295: // AssigneUnitToMachine assigns a unit to a machine. The
unit must exist and
s/Assigne/Assign/

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode296
state/topology.go:296: // yet be unassigned.
Drop second sentence, and replace by:

// It is an error to reassign a unit that is already assigned.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode308
state/topology.go:308: return fmt.Errorf("service unit %q in service %q already
assigned to a machine", unitKey, serviceKey)
We can as well say which machine: "... to machine %q"

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode330
state/topology.go:330: return fmt.Errorf("machine with key %q cannot be found",
machineKey)
Can we please replace all of these errors with:

s/cannot be found/not found/

https://codereview.appspot.com/5647055/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode36
state/unit.go:36: // is unassigned, or its unit agent hasn't started this value
it may be empty.
Second sentence reads incorrectly, and it also sounds like a bad idea. If we're
unable to serve a public address, this method should return an error since what
the developer asked for wasn't feasible.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode63
state/unit.go:63: // is unassigned, or its unit agent hasn't started this value
it may be empty.
Ditto.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode121
state/unit.go:121: func (u *Unit) AssignedMachineKey() (string, error) {
s/Key/Id/
s/string/int/

Keys are internal node names in ZooKeeper, which should never be exposed through
the API of the state package.

Machine ids are integers.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode132
state/unit.go:132: }
return machineId(machineKey), nil

Drop logic below.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode150
state/unit.go:150: if err = t.AssignUnitToMachine(u.serviceKey, u.key,
machine.key); err != nil {
return t.Assign...

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode158
state/unit.go:158: return fmt.Errorf("unit %q already assigned to a machine",
u.Name())
return fmt.Errorf("unit %q already assigned to machine %d", u.Name(),
machineId(machineKey))

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode166
state/unit.go:166: // creation and assigning of a new machine.
Please use this comment instead:

// AssignToUnusedMachine assigns u to a machine without other units.
// If there are no unused machines besides machine 0, an error is returned.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode168
state/unit.go:168: unusedMachineKeyWrapper := ""
The algorithm in this method is more involved than necessary. I'll walk you over
a simpler version.

Start by replacing the line above by:

machineKey := ""

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode177
state/unit.go:177: // be removed.
This comment is bogus. Please drop it.

We don't reuse machine 0 by choice, because we don't want to have other units
living with zk and then be unable to kill the unit cleanly and keep the rest of
the cluster working. All the allocation information is properly reflected. This
may be changed once we have containers working well in EC2.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode179
state/unit.go:179: unusedMachineKeys := []string{}
Please drop both lines above.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode180
state/unit.go:180: for _, machineKey := range t.MachineKeys() {
s/:=/=/

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode181
state/unit.go:181: if machineKey != rootMachine {
if machineId(machineKey) != 0 {

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode182
state/unit.go:182: ok, err := t.MachineHasUnits(machineKey)
s/ok/hasUnits/

Using "!ok" is confusing.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode186
state/unit.go:186: if !ok {
if !hasUnits {
    break
}

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode191
state/unit.go:191: if len(unusedMachineKeys) == 0 {
if machineKey == ""

[TheMue, 2012-02-09 15:56:55]

Please take a look.

[TheMue, 2012-02-09 15:57:19]

PTAL

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

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode19
state/machine.go:19: // Key returns the public key of the machine.
On 2012/02/08 20:30:14, niemeyer wrote:
> // Id returns the machine id.

Done.

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode20
state/machine.go:20: func (m *Machine) Key() string {
On 2012/02/08 20:30:14, niemeyer wrote:
> s/Key/Id/
> s/string/int/

Done.

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode25
state/machine.go:25: func (m *Machine) InternalKey() string {
On 2012/02/08 20:30:14, niemeyer wrote:
> Please drop this method. This is private information of the package.

Done. But added zkKey() like we've done for Service and Unit.

https://codereview.appspot.com/5647055/diff/1/state/machine.go#newcode41
state/machine.go:41: func publicMachineKey(internalKey string) string {
On 2012/02/08 20:30:14, niemeyer wrote:
> // machineId returns the machine id corresponding to machineKey.
> func machineId(machineKey string) (id int) {
>     if machineKey == "" {
>         panic("machineId: empty machine key")
>     }
>     i := strings.Index(machineKey, "-")
>     var err error
>     if i >= 0 {
>         id, err = strconv.ParseInt(machineKey[i+1:], 10, 32)
>     }
>     if i < 0 || err != nil {
>         panic("machineId: invalid machine key: " + machineKey)
>     }
>     return int(id)
> }

Done.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode140
state/state_test.go:140: charm := state.CharmMock("local:myseries/mytest-1")
On 2012/02/08 20:30:14, niemeyer wrote:
> It'd be great to get rid of CharmMock next. This branch is increasing the
> dependency on logic that has to be removed.

OK, will be the focus of the next branch.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode163
state/state_test.go:163: // URL validation is done by charm.ParseURL().
On 2012/02/08 20:30:14, niemeyer wrote:
> Drop second sentence. Sure, ParseURL does validation, but why is that relevant
> here?

Done.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode245
state/state_test.go:245: // named unit fails nicely.
On 2012/02/08 20:30:14, niemeyer wrote:
> It was right before.. s/fails/fail/. You can do s/or/and/ as well, to be more
> clear. We're talking about both.

Done.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode343
state/state_test.go:343: // URL validation is done by charm.ParseURL().
On 2012/02/08 20:30:14, niemeyer wrote:
> Please drop second sentence too.

Done.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode539
state/state_test.go:539: // Create root machine that shouldn't be useds.
On 2012/02/08 20:30:14, niemeyer wrote:
> // Create machine 0, that shouldn't be used.

Done.

https://codereview.appspot.com/5647055/diff/1/state/state_test.go#newcode542
state/state_test.go:542: // Check that assigning without unuused machine fails.
On 2012/02/08 20:30:14, niemeyer wrote:
> s/unuused/unused/

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode87
state/topology.go:87: if t.HasMachine(key) {
On 2012/02/08 20:30:14, niemeyer wrote:
> else if? It Machines == nil it can't have a machine.

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode93
state/topology.go:93: 
On 2012/02/08 20:30:14, niemeyer wrote:
> // Comment here.

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode95
state/topology.go:95: err := t.assertMachine(key)
On 2012/02/08 20:30:14, niemeyer wrote:
> No need to assert here. MachineHasUnits will check already.

Done. Ah, yes, found it. Had it from the Py code. So I have to double-check in
future.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode106
state/topology.go:106: // Machine exists and as no units, so remove it.
On 2012/02/08 20:30:14, niemeyer wrote:
> s/as/has/

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode121
state/topology.go:121: // HasMachine returns true if a machine with the given
key exists.
On 2012/02/08 20:30:14, niemeyer wrote:
> // HasMachine returns whether a machine with key exists.

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode126
state/topology.go:126: // MachineHasUnits returns true if one of the units is
assigned tp the machine.
On 2012/02/08 20:30:14, niemeyer wrote:
> // MachineHasUnits returns whether the machine with key has any units assigned
> to it.

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode295
state/topology.go:295: // AssigneUnitToMachine assigns a unit to a machine. The
unit must exist and
On 2012/02/08 20:30:14, niemeyer wrote:
> s/Assigne/Assign/

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode296
state/topology.go:296: // yet be unassigned.
On 2012/02/08 20:30:14, niemeyer wrote:
> Drop second sentence, and replace by:
> 
> // It is an error to reassign a unit that is already assigned.

Done.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode308
state/topology.go:308: return fmt.Errorf("service unit %q in service %q already
assigned to a machine", unitKey, serviceKey)
On 2012/02/08 20:30:14, niemeyer wrote:
> We can as well say which machine: "... to machine %q"

Done. Hehe, funnily had this as a first approach too, but then oriented back to
the Py version.

https://codereview.appspot.com/5647055/diff/1/state/topology.go#newcode330
state/topology.go:330: return fmt.Errorf("machine with key %q cannot be found",
machineKey)
On 2012/02/08 20:30:14, niemeyer wrote:
> Can we please replace all of these errors with:
> 
> s/cannot be found/not found/

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode36
state/unit.go:36: // is unassigned, or its unit agent hasn't started this value
it may be empty.
On 2012/02/08 20:30:14, niemeyer wrote:
> Second sentence reads incorrectly, and it also sounds like a bad idea. If
we're
> unable to serve a public address, this method should return an error since
what
> the developer asked for wasn't feasible.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode63
state/unit.go:63: // is unassigned, or its unit agent hasn't started this value
it may be empty.
On 2012/02/08 20:30:14, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode121
state/unit.go:121: func (u *Unit) AssignedMachineKey() (string, error) {
On 2012/02/08 20:30:14, niemeyer wrote:
> s/Key/Id/
> s/string/int/
> 
> Keys are internal node names in ZooKeeper, which should never be exposed
through
> the API of the state package.
> 
> Machine ids are integers.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode132
state/unit.go:132: }
On 2012/02/08 20:30:14, niemeyer wrote:
> return machineId(machineKey), nil
> 
> Drop logic below.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode150
state/unit.go:150: if err = t.AssignUnitToMachine(u.serviceKey, u.key,
machine.key); err != nil {
On 2012/02/08 20:30:14, niemeyer wrote:
> return t.Assign...

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode158
state/unit.go:158: return fmt.Errorf("unit %q already assigned to a machine",
u.Name())
On 2012/02/08 20:30:14, niemeyer wrote:
> return fmt.Errorf("unit %q already assigned to machine %d", u.Name(),
> machineId(machineKey))

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode166
state/unit.go:166: // creation and assigning of a new machine.
On 2012/02/08 20:30:14, niemeyer wrote:
> Please use this comment instead:
> 
> // AssignToUnusedMachine assigns u to a machine without other units.
> // If there are no unused machines besides machine 0, an error is returned.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode168
state/unit.go:168: unusedMachineKeyWrapper := ""
On 2012/02/08 20:30:14, niemeyer wrote:
> The algorithm in this method is more involved than necessary. I'll walk you
over
> a simpler version.
> 
> Start by replacing the line above by:
> 
> machineKey := ""

That's fine, already found the original Py method with its structure and those
comments below not very maintainable.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode177
state/unit.go:177: // be removed.
On 2012/02/08 20:30:14, niemeyer wrote:
> This comment is bogus. Please drop it.
> 
> We don't reuse machine 0 by choice, because we don't want to have other units
> living with zk and then be unable to kill the unit cleanly and keep the rest
of
> the cluster working. All the allocation information is properly reflected.
This
> may be changed once we have containers working well in EC2.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode179
state/unit.go:179: unusedMachineKeys := []string{}
On 2012/02/08 20:30:14, niemeyer wrote:
> Please drop both lines above.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode180
state/unit.go:180: for _, machineKey := range t.MachineKeys() {
On 2012/02/08 20:30:14, niemeyer wrote:
> s/:=/=/

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode181
state/unit.go:181: if machineKey != rootMachine {
On 2012/02/08 20:30:14, niemeyer wrote:
> if machineId(machineKey) != 0 {

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode182
state/unit.go:182: ok, err := t.MachineHasUnits(machineKey)
On 2012/02/08 20:30:14, niemeyer wrote:
> s/ok/hasUnits/
> 
> Using "!ok" is confusing.

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode186
state/unit.go:186: if !ok {
On 2012/02/08 20:30:14, niemeyer wrote:
> if !hasUnits {
>     break
> }

Done.

https://codereview.appspot.com/5647055/diff/1/state/unit.go#newcode191
state/unit.go:191: if len(unusedMachineKeys) == 0 {
On 2012/02/08 20:30:14, niemeyer wrote:
> if machineKey == ""

Done.

[niemeyer, 2012-02-09 16:23:44]

Thanks for the fixes.. just a few additional details and it's ready for merging.

https://codereview.appspot.com/5647055/diff/7001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/topology.go#newcode92
state/topology.go:92: // RemoveMachine removes a machine from the topology.
s/a machine/the machine with key/

https://codereview.appspot.com/5647055/diff/7001/state/topology.go#newcode281
state/topology.go:281: // key means there is no machine assigned.
Documentation is wrong now.

https://codereview.appspot.com/5647055/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/unit.go#newcode173
state/unit.go:173: machineKey = ""
Please invert the logic to make the intention more clear. At the top of the for
loop:

if machineId(machineKey) == 0 {
    machineKey = ""
    continue
}
... rest ...

https://codereview.appspot.com/5647055/diff/7001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/util.go#newcode19
state/util.go:19: unitNotAssigned = errors.New("unit not assigned to machine")
Please move that right above the respective function in topology.go. It doesn't
help to have this error out of context here.

[TheMue, 2012-02-09 18:40:17]

*** Submitted:

Started Machine integration into the state package

A first raw Machine type has been added to add the Machine
related methods to unit. Additionally all Machine related
methods in topology have been added. Beside this major topic
Unit has been extended by few further methods.

R=niemeyer
CC=
https://codereview.appspot.com/5647055

https://codereview.appspot.com/5647055/diff/7001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/topology.go#newcode92
state/topology.go:92: // RemoveMachine removes a machine from the topology.
On 2012/02/09 16:23:44, niemeyer wrote:
> s/a machine/the machine with key/

Done.

https://codereview.appspot.com/5647055/diff/7001/state/topology.go#newcode281
state/topology.go:281: // key means there is no machine assigned.
On 2012/02/09 16:23:44, niemeyer wrote:
> Documentation is wrong now.

Done.

https://codereview.appspot.com/5647055/diff/7001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/unit.go#newcode173
state/unit.go:173: machineKey = ""
On 2012/02/09 16:23:44, niemeyer wrote:
> Please invert the logic to make the intention more clear. At the top of the
for
> loop:
> 
> if machineId(machineKey) == 0 {
>     machineKey = ""
>     continue
> }

Doesn't work, machineKey has to be set empty at the end of each loop. Otherwise
the last set key will be taken.
> ... rest ...

https://codereview.appspot.com/5647055/diff/7001/state/util.go
File state/util.go (right):

https://codereview.appspot.com/5647055/diff/7001/state/util.go#newcode19
state/util.go:19: unitNotAssigned = errors.New("unit not assigned to machine")
On 2012/02/09 16:23:44, niemeyer wrote:
> Please move that right above the respective function in topology.go. It
doesn't
> help to have this error out of context here.

Done.