Code review comment for lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine
Revision history for this message
| William Reade (fwereade) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
35a32c5
(Get the code!)
firehose of suggestions :)
https:/
File state/unit.go (right):
https:/
state/unit.go:539: return readRequestedNe
don't think we need this method now
https:/
state/unit.go:833: machineIncluded
:= m.RequestedNetw
Basically never ignore errors. Certainly not here ;p.
https:/
state/unit.go:839: unitIncludedNet
networked_
readRequestedNe
and don't ignore errors :).
https:/
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:/
state/unit.go:859: }
This all might be easier with the methods on utils/set.Strings
https:/
state/unit.go:864: Assert: bson.D{
networked_
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 readRequestedNe
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:/
state/unit.go:866: err = u.st.runTransac
When building this sort of construction:
if err := u.st.runTransac
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:/
state/unit.go:887: assert := append(isAliveDoc, bson.D{
(rename this unitAssert for clarity's sake)
https:/
state/unit.go:893: massert := isAliveDoc
(and this to machineAssert)
https:/
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 {
}
}()
// Clone u and m for working with and possible subsequent refreshes.
// If we don't already have clone methods we probably should, but if
// you're going to write them please fix the places we do it dumbly.
// (And write the clone method smartly: copy *all* fields, make sure
// maps/slices in doc fields are copied, etc)
unit := &Unit{st: u.st, doc: doc: u.doc}
machine := &Machine{st: u.st, doc: m.doc}
// It's ok to validate immutable stuff outside as early as you like:
if unit.doc.Series != machine.doc.Series ...
if unit.doc.Principal != "" ...
// But everything else, including Jobs -- that may be immutable now
but will
// not be forever -- we should always check against fresh state.
for i := 0; i < 3; i++ {
if unit.doc.Life != Alive ...
if machine.doc.Life != Alive ...
if unit.doc.MachineId != "" ...
// etc...
// Build transaction as before (subsequently inserting
network-related
// lookups/ops). Note: I would recommend factoring out a method
called
// assignToMachineOps at this point, this is getting pretty
unwieldy.
ops := ...
// Run transaction, and return on success, or surprising
failure.
if err := u.st.runTransac
return err
}
// Now we're here, all we know is that some assert failed. But
if we
// reload the unit and the machine, we'll go back to the top of
the
// loop and go all the way through all the state checking again,
and
// be able to skip the nasty duplication we have here.
if err := unit.Refresh(); err != nil {
return err
}
if err := machine.Refresh(); err != nil {
return err
}
}
// This is the usual error when we fail out of a retry loop. It
*says*
// "changing too fast" but what it usually *means* is "a programmer
// screwed up"
return ErrExcessiveCon
}
https:/
File state/unit_test.go (right):
https:/
state/unit_
This is interesting. It *works* but it's a bit surprising. I think I'd
usually rather see the common stuff embedded in a separate type, as is
ConnSuite.
I'm heartily +1 on small separate test suites, though, good call there.
https:/
state/unit_
"127.0.0.4", "127.0.0.6"}
these are addresses, not network names
https:/
state/unit_
I'd recommend putting these tests in assign_test.go, instead.
https:/