Merge lp://staging/~aramh/juju-core/107-state-watchers-units-unassignments6 into lp://staging/~juju/juju-core/trunk

Proposed by Aram Hăvărneanu
Status: Work in progress
Proposed branch: lp://staging/~aramh/juju-core/107-state-watchers-units-unassignments6
Merge into: lp://staging/~juju/juju-core/trunk
Diff against target: 324 lines (+95/-91)
2 files modified
state/machine_test.go (+67/-73)
state/watcher.go (+28/-18)
To merge this branch: bzr merge lp://staging/~aramh/juju-core/107-state-watchers-units-unassignments6
Reviewer Review Type Date Requested Status
The Go Language Gophers Pending
Review via email: mp+130118@code.staging.launchpad.net

Description of the change

state: machine units watcher reports unassignment

https://codereview.appspot.com/6720049/

To post a comment you must log in.
Revision history for this message
Aram Hăvărneanu (aramh) wrote :

Reviewers: mp+130118_code.launchpad.net,

Message:
Please take a look.

Description:
state: machine units watcher reports unassignment

https://code.launchpad.net/~aramh/juju-core/107-state-watchers-units-unassignments6/+merge/130118

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6720049/

Affected files:
   A [revision details]
   M state/machine_test.go
   M state/watcher.go

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Seems very complex, poorly tested, and quite problematic.

My recommendation is still the same: we do not depend on this logic.
Let's convert the watcher, and focus on getting the logic we do need
right now correctly. It'll be a while before we depend on unassignment
and reassignment.

https://codereview.appspot.com/6720049/diff/2001/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/6720049/diff/2001/state/machine_test.go#newcode824
state/machine_test.go:824: err := principal.UnassignFromMachine()
This seems extremely light compared to the amount of non-trivial logic
on that implementation.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1308
state/watcher.go:1308: isprincipal := doc.Principal == ""
If it wasn't found, that's not right.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1309
state/watcher.go:1309: w.known[unit] = doc.Life
If it wasn't found, that's certainly not right either.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1311
state/watcher.go:1311: if _, ok := w.known[doc.Principal]; !ok &&
!isprincipal {
Why are we looking for doc.Principal when it can be ""?

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1314
state/watcher.go:1314: w.st.watcher.Watch(w.st.units.Name, unit,
doc.TxnRevno, w.in)
If the document is !known && !found, we'll get here.

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1315
state/watcher.go:1315: pending = append(pending, unit)
So if the document was previously unknown, we don't care about the fact
it was just unassigned, or assigned to the wrong machine after we
decided to merge it?

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1319
state/watcher.go:1319: unassigned = (doc.MachineId != nil) &&
*doc.MachineId != w.machine.doc.Id || doc.MachineId == nil
doc.MachineId == nil || *doc.MachineId != w.machine.doc.Id

https://codereview.appspot.com/6720049/diff/2001/state/watcher.go#newcode1326
state/watcher.go:1326: if doc.Life != Dead && !hasString(pending, unit)
{
By now I'm finding pretty hard to recognize all the ins and outs of the
implementation. I can't tell with certainty if this is free from the
kind of issue I described above.

https://codereview.appspot.com/6720049/

Unmerged revisions

677. By Aram Hăvărneanu

state: go fmt

676. By Aram Hăvărneanu

state: make machine units report unassigment

675. By Roger Peppe

environs, juju: require admin-secret

The admin-secret configuration value is required
for bootstrapping and connecting to an environment,
but we can't make config.Config require it because
it should never be pushed into the state.

So we add checks to juju.NewConn and implementations
of Environ.Bootstrap to require that it be set.
This unfortunately means that all tests must
connect with authentication set, which means
the changes are necessarily bulky.

R=TheMue, dfc, fwereade, niemeyer
CC=
https://codereview.appspot.com/6653050

674. By Dave Cheney

cmd/juju: add remove-unit subcommand

R=fwereade, niemeyer, rog
CC=
https://codereview.appspot.com/6651060

673. By Gustavo Niemeyer

state: fix firewall-mode tests

Firewall mode default is now spelled as "".

R=
CC=
https://codereview.appspot.com/6699044

672. By Roger Peppe

environs/ec2: tear down suite properly

LiveTests wasn't destroying the environment.

Also simplify ec2.Destroy a little, now that the AllInstances
method exists.

R=fwereade, dfc, niemeyer
CC=
https://codereview.appspot.com/6646051

671. By Frank Mueller

environs: added global open/close/ports

Added the definition of OpenPorts(), ClosePorts() and Ports()
to Environ and changed the implementations of dummy and ec2.
dummy only supports instance firewall mode, ec2 both. The
common test for the global mode is skipped if the environment
does not support that mode.

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

670. By Frank Mueller

environs: ec2 and dummy default to instance

The validation of the configuration for the providers
ec2 and dummy now exchanges "default" with "instance".
The implementations of the global mode has been rolled
back to avoid failing tests. A later change will
prohibit opening and closing of ports on instances
in the global mode.

R=niemeyer, fwereade, <email address hidden>
CC=
https://codereview.appspot.com/6653043

669. By Roger Peppe

cmd/{jujud,juju}: no JujuConnSuite in btstrp test

The tests should not be running on an already-bootstrapped
environment, which is what JujuConnSuite provides.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6601067

668. By Roger Peppe

environs/jujutest: move fixtures

It's more conventional to have them next to the tests.

R=aram, TheMue
CC=
https://codereview.appspot.com/6651062

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches