MAAS

Merge lp://qastaging/~julian-edwards/maas/race-for-staticip-bug-1387262 into lp://qastaging/~maas-committers/maas/trunk

race-for-staticip-bug-1387262
Merge into trunk

Proposed by Julian Edwards on 2014-11-05

Status:

Merged

Approved by:

Julian Edwards on 2014-11-05

Approved revision:

no longer in the source branch.

Merged at revision:

3337

Proposed branch:

lp://qastaging/~julian-edwards/maas/race-for-staticip-bug-1387262

Merge into:

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

Diff against target:

128 lines (+50/-11)

3 files modified

src/maasserver/locks.py (+3/-0)
src/maasserver/models/staticipaddress.py (+22/-11)
src/maasserver/models/tests/test_staticipaddress.py (+25/-0)

To merge this branch:

bzr merge lp://qastaging/~julian-edwards/maas/race-for-staticip-bug-1387262

Critical

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Graham Binns (community)		Approve on 2014-11-05
Newell Jensen (community)	2014-11-05	Approve on 2014-11-05
Review via email: mp+240669@code.qastaging.launchpad.net

Commit message

Use a lock in the critical section of the code that finds and records a new static IP address in the database. Some users have reported errors in the PG log file complaining about inserting duplicates, so there must be a race.

Description of the change

With reference to the attached bug, it looks like we need a lock around the section of code that acquires static IP addresses. At least, I can't see any other reason for errors like this in the PG log:

2014-10-28 15:40:35 UTC ERROR: duplicate key value violates unique constraint "maasserver_staticipaddress_ip_key"
2014-10-28 15:40:35 UTC DETAIL: Key (ip)=(10.245.0.201) already exists.
2014-10-28 15:40:35 UTC STATEMENT: INSERT INTO "maasserver_staticipaddress" ("created", "updated", "ip", "alloc_type", "user_id") VALUES ('2014-10-28 15:40:32.431728', '2014-10-28 15:40:32.431728', '10.245.0.201', 0, NULL) RETURNING "maasserver_staticipaddress"."id"

However the error that bubbles up to the appserver is a little odd (see the bug), you'd think it would be an IntegrityError. Maybe it's a consequence of the atomic context manager used in Node.claim_static_ip_addresses() ?

Revision history for this message

Newell Jensen (newell-jensen) wrote on 2014-11-05:

Approve. One question inline.

review: Approve

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2014-11-05:

On Wednesday 05 Nov 2014 04:22:23 you wrote:
> I assume you are using two calls to assertThat instead of expectThat because
> if the lock was not acquired then you know it wouldn't have been released,
> correct? I.e., you can't get the second test to error unless the first one
> passes. This stood out to me and I had to ask because I know you love
> expectThat!

Correct :)

Revision history for this message

Graham Binns (gmb) wrote on 2014-11-05:

LGTM. I wondered for a second whether we needed a lock in _attempt_allocation(), too, but I see upon looking at the code that we already handle IntegrityErrors there.

Nice work that man.

review: Approve