[2.0b2] DNS zone serials are not stable
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
MAAS |
Fix Released
|
Critical
|
Gavin Panella |
Bug Description
Revision 4852 introduced a race into maasserver.
makes it no longer reliable. The new code ostensibly allows a current
value of a sequence to be obtained without the side-effects associated
with calling nextval() [1].
However, it does have side-effects. The code in question:
cursor.execute(
"SELECT setval(%s, nextval(%s) - %s);",
[self.name, self.name, self.increment])
Sequences work outside of transactions, and are mutated immediately, so
there's a moment between nextval() and setval() above where the sequence
has been incremented. This is visible to other database sessions. This
can be demonstrated with a short example [2].
Calls to current() from different database sessions that occur close in
time — for example, when the database notifies all regiond listeners
that an IP address has been allocated — are at real risk of getting
different values out of current(). This means that zones might be
generated with inconsistent serials, or even a serial _before_ the
previous one.
In practice this _may_ work out okay, or it may not. We MUST, however,
remove this code from Sequence.current(). maasserver.Sequence is an OO
abstraction around PostgreSQL's sequences and is intended to share
essentially the same behaviour.
I can suggest two ways to fix this:
1. Subclass m.Sequence to something specific for DNS serials. Only the
subclass would contain the problematic setval(nextval()) code, but
would add an explicit non-transactional advisory database lock around
it to eliminate the race.
2. In the database triggers that drive DNS serials — like
sys_
side-effects alone, capture its result and include it in the
pg_notify() payload. The listening code would then never use
Sequence.
Of these I _strongly_ suggest #2; #1 would still be hack, albeit a
lesser one.
[1] Normally, in a database session, there is no current value of a
sequence until nextval() has been called on it.
[2] http://
DJANGO_
bin/database --preserve run -- bin/py script.py
and leave it running for at least a minute.
Related branches
- Blake Rouse (community): Approve
-
Diff: 68 lines (+9/-25)2 files modifiedsrc/maasserver/sequence.py (+6/-22)
src/maasserver/tests/test_sequence.py (+3/-3)
- Blake Rouse (community): Approve
-
Diff: 348 lines (+203/-59)4 files modifiedsrc/maasserver/migrations/builtin/maasserver/0055_dns_publications.py (+28/-0)
src/maasserver/models/__init__.py (+17/-59)
src/maasserver/models/dnspublication.py (+74/-0)
src/maasserver/models/tests/test_dnspublication.py (+84/-0)
- Mike Pontillo (community): Approve
-
Diff: 274 lines (+99/-26)12 files modifiedsrc/maasserver/api/domains.py (+2/-4)
src/maasserver/api/tests/test_domains.py (+1/-1)
src/maasserver/dns/config.py (+1/-11)
src/maasserver/migrations/builtin/maasserver/0026_create_zone_serial_sequence.py (+4/-2)
src/maasserver/migrations/builtin/maasserver/0056_zone_serial_ownership.py (+30/-0)
src/maasserver/migrations/builtin/maasserver/0057_merge.py (+13/-0)
src/maasserver/migrations/south/migrations/0011_add_dns_zone_serial_sequence.py (+22/-3)
src/maasserver/models/dnspublication.py (+11/-1)
src/maasserver/triggers/system.py (+1/-1)
src/maasserver/triggers/tests/helper.py (+1/-1)
src/maasserver/triggers/tests/test_system.py (+1/-1)
utilities/check-imports (+12/-1)
- LaMont Jones (community): Approve
- Mike Pontillo (community): Approve
-
Diff: 1539 lines (+549/-189)12 files modifiedsrc/maasserver/api/tests/test_domains.py (+33/-13)
src/maasserver/dns/config.py (+3/-5)
src/maasserver/dns/tests/test_config.py (+25/-40)
src/maasserver/migrations/builtin/maasserver/0057_initial_dns_publication.py (+37/-0)
src/maasserver/migrations/builtin/maasserver/0058_bigger_integer_for_dns_publication_serial.py (+22/-0)
src/maasserver/migrations/builtin/maasserver/0059_merge.py (+13/-0)
src/maasserver/models/dnspublication.py (+19/-5)
src/maasserver/models/tests/test_dnspublication.py (+41/-5)
src/maasserver/triggers/system.py (+103/-31)
src/maasserver/triggers/testing.py (+49/-11)
src/maasserver/triggers/tests/test_system_listener.py (+203/-78)
src/maasserver/triggers/tests/test_websocket_listener.py (+1/-1)
- Gavin Panella (community): Abstain
- Blake Rouse (community): Approve
- Mike Pontillo (community): Abstain
-
Diff: 366 lines (+266/-5)7 files modifiedsrc/maasserver/api/tests/test_interfaces.py (+3/-3)
src/maasserver/dns/publication.py (+72/-0)
src/maasserver/dns/tests/test_publication.py (+126/-0)
src/maasserver/eventloop.py (+10/-0)
src/maasserver/models/dnspublication.py (+7/-2)
src/maasserver/models/tests/test_dnspublication.py (+47/-0)
src/maasserver/tests/test_plugin.py (+1/-0)
Changed in maas: | |
importance: | Undecided → High |
milestone: | none → 2.0.0 |
status: | New → Triaged |
summary: |
- DNS zone serials are not stable + [2.0b2] DNS zone serials are not stable |
Changed in maas: | |
importance: | High → Critical |
Changed in maas: | |
status: | Triaged → In Progress |
assignee: | nobody → Gavin Panella (allenap) |
Changed in maas: | |
status: | In Progress → Fix Committed |
Changed in maas: | |
status: | Fix Committed → Fix Released |