Merge lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine into lp:~go-bot/juju-core/trunk
Status: | Work in progress |
---|---|
Proposed branch: | lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine |
Merge into: | lp:~go-bot/juju-core/trunk |
Diff against target: |
415 lines (+278/-68) 3 files modified
state/machine.go (+42/-0) state/network_assign_test.go (+88/-0) state/unit.go (+148/-68) |
To merge this branch: | bzr merge lp:~hduran-8/juju-core/add_state_network_check_on_specified_machine |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+215143@code.launchpad.net |
Description of the change
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.
Unmerged revisions
- 2585. By Horacio Durán
-
Ensured there is a retry when txnrevno of unit does not match
- 2584. By Horacio Durán
-
Go formatte
- 2583. By Horacio Durán
-
Go formatted
- 2582. By Horacio Durán
-
Refactored assignToMachine Added proper tests in separate file, added clone to unit and machine
- 2581. By Horacio Durán
-
Go Formatted, added extra comments for context
- 2580. By Horacio Durán
-
Changed the way networks are obtained.
Fixed the corresponding tests
Added a draft of check for changes - 2579. By Horacio Durán
-
Ran go fmt
- 2578. By Horacio Durán
-
Added rough check for network compatibility when adding to machine
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 tworks( u.st, u.globalKey())
state/unit.go:539: return readRequestedNe
don't think we need this method now
https:/ /codereview. appspot. com/86430043/ diff/20001/ state/unit. go#newcode833 Networks, machineExcluded Networks, _ orks()
state/unit.go:833: machineIncluded
:= m.RequestedNetw
Basically never ignore errors. Certainly not here ;p.
https:/ /codereview. appspot. com/86430043/ diff/20001/ state/unit. go#newcode839 works, unitExcludedNet works, _ := service. Networks( ) tworks( u.st, serviceGlobalKe y(u.doc. Service) )
state/unit.go:839: unitIncludedNet
networked_
readRequestedNe
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 {"txn-revno" , service. doc.TxnRevno} },
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 tworks
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:/ /codereview. appspot. com/86430043/ diff/20001/ state/unit. go#newcode866 tion(ops)
state/unit.go:866: err = u.st.runTransac
When building this sort of construction:
if err := u.st.runTransac tion(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) {
u. doc.MachineId = m.doc.Id...
// On the expectation that we'll have more than one possible nil
return,
// defer the updates to local state.
defer func() {
if err != nil {