Add check for requested networks compatibility

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://code.launchpad.net/~hduran-8/juju-core/add_state_network_check_on_specified_machine/+merge/215143

(do not edit description out of merge proposal)

[fwereade, 2014-04-11 09:48:47]

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
            m.doc.Clean = false
        }
    }()

    // 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.runTransaction(ops); err != txn.ErrAborted {
            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 ErrExcessiveContention
}

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

https://codereview.appspot.com/86430043/diff/20001/state/unit_test.go#newcode29
state/unit_test.go:29: type NetworkedUnitSuite UnitSuite
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://codereview.appspot.com/86430043/diff/20001/state/unit_test.go#newcode211
state/unit_test.go:211: excludeNetworks := []string{"127.0.0.2", "127.0.0.4",
"127.0.0.6"}
these are addresses, not network names

https://codereview.appspot.com/86430043/diff/20001/state/unit_test.go#newcode242
state/unit_test.go:242: }
I'd recommend putting these tests in assign_test.go, instead.

[fwereade, 2014-04-17 10:06:25]

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.

[hduran, 2014-04-18 23:17:14]

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, <fwereade@gmail.com> 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/
>