lp:~fwereade/pyjuju/fix-charm-upgrade

Branch merges

Propose for merging

No branches dependent on this one.

Merged into lp:pyjuju at revision 458

Kapil Thangavelu (community): Approve on 2012-02-21

Jim Baker (community): Approve on 2012-01-09

Diff: 3535 lines (+1424/-808)

15 files modified

docs/source/internals/unit-agent-persistence.rst (+138/-0)
juju/agents/tests/test_unit.py (+79/-141)
juju/agents/unit.py (+59/-136)
juju/control/tests/test_resolved.py (+16/-8)
juju/control/tests/test_status.py (+11/-37)
juju/control/tests/test_upgrade_charm.py (+4/-2)
juju/errors.py (+11/-1)
juju/lib/statemachine.py (+105/-28)
juju/lib/tests/test_statemachine.py (+214/-76)
juju/state/service.py (+10/-10)
juju/tests/test_errors.py (+31/-9)
juju/unit/lifecycle.py (+122/-24)
juju/unit/tests/test_lifecycle.py (+121/-50)
juju/unit/tests/test_workflow.py (+435/-227)
juju/unit/workflow.py (+68/-59)

api

Related source package recipes

No recipes using this branch. (?)

Create packaging recipe

Related snap packages

Related bugs

Bug #903018: charm upgrade is dangerous

Undecided

Fix Released

Link a bug report

Related blueprints

458. By William Reade on 2012-02-21

merge parent

457. By William Reade on 2012-02-21

break up synchronize test to avoid occasional timeout during full test runs

456. By William Reade on 2012-02-21

WorkflowState now (1) has a synchronize method that re-runs inflight

transitions after restart, and (2) requires external locking on state-
changing methods.

(1) is a required change due to the verdict on fix-charm-upgrade (that is:
we don't want additional states for the upgrade process, and we should use
write-ahead logging to maintain state across process death); (2) is
consequently required because of the interaction between the upgrade flag
watch and the resolved watch, both of which take `state == "started"` to
mean "it's safe to fire a transition". This was a pre-existing bug, but was
IMO exacerbated severely by the fact that we now never leave the "started"
state while upgrading a charm -- enough so that I'm not comfortable
proposing a patch that fails to address this issue.

This diff is *big*, but the vast majority of the changes are as a result of
the WorkflowState locking; in particular, many many tests set workflow state
in one way or another, and the necessary changes add a *lot* of noise. In
hindsight, the locking changes could have been made independently, but I
don't think the resulting pair of branches would actually be significantly
easier to deal with.

Various explanatory notes follow:

* External WorkflowState locking (rather than automagic internal locking) was
  chosen for simplicity's sake on the part of the client as well: when one
  to fire a transition conditional on some state being active, it makes
  sense to grab the lock, check, and fire the transition, in the certain
  knowledge that nobody can have changed state underneath you.

* The obvious problem with DeferredLock (that you can tell it's locked, but
  not who owns the lock) has minimal impact, thanks to the unit tests.
  Consider the following scenarios:

  * Code tries to lock, test not holding lock: we're fine.
  * Code tries to lock, test is holding lock: bad test; deadlocks.
  * Code fails to lock, test not holding lock: bad code: asserts.
  * Code fails to lock, test is holding lock: bad code AND bad test.

  The final scenario is the only dangerous one, but it's just a special
  case of the fact that you can *always* write a bad test that passes bad
  code; I think attempting to solve this is out of scope for this universe.

* WorkflowState.fire_transition sets the inflight transition once it knows
  the requested transition is valid, and only clears it explicitly if the
  transition fails without an error transition to follow up (when a
  transition succeeds, set_state implicitly clears inflight).

* WorkflowState.synchronize (1) detects and re-runs inflight transitions and
  (2) has taken over responsibility for the automatic transitions (eg
  None->installed->started) which had previously been handled in UWS/RWS.
  The overlapping concept of success_transition_id has been dropped;
  transitions can now be marked as "automatic".

* WorkflowState.set_inflight really wants to be private, but is exposed for
  testing's sake; get_inflight needs to be exposed so that UWS.synchronize
  doesn't inappropriately start the executor when recovering from mid-
  upgrade process death.

* Er...

* That's it.

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

Merged branch lp:~fwereade/pyjuju/state-machine-sync

455. By William Reade on 2012-02-08

merge parent

454. By William Reade on 2012-02-02

remove UL.upgrade_charm's first_attempt kwarg

453. By William Reade on 2012-02-02

manually back out r448 without screwing up future merges, I hope

452. By William Reade on 2012-01-30

merge parent

451. By William Reade on 2012-01-23

merge parent

450. By William Reade on 2012-01-16

address final review point

449. By William Reade on 2012-01-16

address review point