MAAS

Merge lp://qastaging/~gmb/maas/commit-after-node-state-change-bug-1375664 into lp://qastaging/~maas-committers/maas/trunk

commit-after-node-state-change-bug-1375664
Merge into trunk

Proposed by Graham Binns on 2014-10-15

Status:

Merged

Approved by:

Graham Binns on 2014-10-15

Approved revision:

no longer in the source branch.

Merged at revision:

3258

Proposed branch:

lp://qastaging/~gmb/maas/commit-after-node-state-change-bug-1375664

Merge into:

lp://qastaging/~maas-committers/maas/trunk

Diff against target:

81 lines (+32/-0)

2 files modified

src/maasserver/models/node.py (+12/-0)
src/maasserver/models/tests/test_node.py (+20/-0)

To merge this branch:

bzr merge lp://qastaging/~gmb/maas/commit-after-node-state-change-bug-1375664

Critical

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Raphaël Badin (community)		Approve on 2014-10-15
Jason Hobbs (community)	2014-10-15	Approve on 2014-10-15
Review via email: mp+238489@code.qastaging.launchpad.net

Commit message

Node.start_deployment(), .end_deployment() and .release() now commit the transaction after changing the node's state.

Previously, they merely called node.save(), which was fine for individual node actions but unhelpful at best for bulk actions, because only some of the state changes got committed to the database. This should ensure that *all* state changes will be recorded properly.

Revision history for this message

Jason Hobbs (jason-hobbs) wrote on 2014-10-15:

One comment inline below.

review: Approve

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-15:

On 15 October 2014 20:57, Jason Hobbs <email address hidden> wrote:
>
> Why not commit immediately after self.save()? What if something goes awry between it and the commit here?
>

Pure copy and paste laziness :). Good catch.

Revision history for this message

Christian Reis (kiko) wrote on 2014-10-15:

Is it clear view as to why the commit() should be in the model and not in the callsite? I don't know the code so it could obviously be bong but I'm curious if it's been considered.

Revision history for this message

Graham Binns (gmb) wrote on 2014-10-15:

On 15 October 2014 22:34, Christian Reis <email address hidden> wrote:
> Is it clear view as to why the commit() should be in the model and not in the callsite? I don't know the code so it could obviously be bong but I'm curious if it's been considered.

Two reasons:

1. Guaranteed committing of state-changes — the commit() resides with
the state change because we don't want to rely on callsites to make
sure that the change gets committed.
2. We don't have to worry about introducing future callsites that
need to remember to commit the transaction after calling these
methods.