Merge lp://staging/~julian-edwards/maas/dup-boot-source-selections-bug-1360280 into lp://staging/~maas-committers/maas/trunk

Proposed by Julian Edwards
Status: Merged
Approved by: Julian Edwards
Approved revision: no longer in the source branch.
Merged at revision: 3361
Proposed branch: lp://staging/~julian-edwards/maas/dup-boot-source-selections-bug-1360280
Merge into: lp://staging/~maas-committers/maas/trunk
Diff against target: 867 lines (+832/-0)
4 files modified
src/maasserver/migrations/0115_unique_boot_source_selections.py (+433/-0)
src/maasserver/migrations/0116_unique_boot_source_selections.py (+373/-0)
src/maasserver/models/bootsourceselection.py (+1/-0)
src/maasserver/tests/test_forms_bootsourceselection.py (+25/-0)
To merge this branch: bzr merge lp://staging/~julian-edwards/maas/dup-boot-source-selections-bug-1360280
Reviewer Review Type Date Requested Status
Graham Binns (community) Approve
Review via email: mp+241235@code.staging.launchpad.net

Commit message

Make os, release and boot_source unique for BootSourceSelection to stop duplicate entries.

Description of the change

Add a unique index across boot_source, os and release on BootSourceSelections.

This will obviously potentially fail a migration if someone has dupes already, but this really needs to go in. Since nobody was around to do a pre-imp I'm throwing this up as-is to discuss ideas:

 1. Land it as-is and if it fails add a release note telling someone to remove the dupes manually (so they get to choose which to keep)
 2. Add code to the migration to force-delete all but the newest entry for each boot source.

Anything else?

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote :

ISTR Blake saying that we had to coalesce the entries for one OS/release rather than just dropping all-but-one, because the entries may all have different arches, subarches and labels — all of which are perfectly valid choices, but which need to be recorded in a single row, not several.

So, if we don't want to have settings go away surprisingly, (1) is a good option.

That said, could we add a persistent, user-clearable error* after doing (2) that says something like "MAAS's records of your boot source settings may have changed; check the Images tab to ensure that your boot image settings are correct."

*Not sure we actually have the facility to do this; I was thinking of the persistent error stuff, but that all seems to be auto-cleared or not at all.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

On Monday 10 Nov 2014 07:18:40 you wrote:
> ISTR Blake saying that we had to coalesce the entries for one OS/release
> rather than just dropping all-but-one, because the entries may all have
> different arches, subarches and labels — all of which are perfectly valid
> choices, but which need to be recorded in a single row, not several.
>
> So, if we don't want to have settings go away surprisingly, (1) is a good
> option.
>
> That said, could we add a persistent, user-clearable error* after doing (2)
> that says something like "MAAS's records of your boot source settings may
> have changed; check the Images tab to ensure that your boot image settings
> are correct."
>
> *Not sure we actually have the facility to do this; I was thinking of the
> persistent error stuff, but that all seems to be auto-cleared or not at
> all.

Actually I hadn't considered coalescing them - good point! I'll write some
runes to do that later.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Right, coalescing migration added, care to take another look?

I have tested this reasonably thoroughly in the dev environment, I used this script to poke values in http://paste.ubuntu.com/8935209/ and the end result looked exactly right (see comments in that script).

Note that there's two migrations as you can't do data and schema changes in the same migration.

Revision history for this message
Graham Binns (gmb) wrote :

Tip top. Really nice work, Jools!

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :
Download full text (20.7 KiB)

The attempt to merge lp:~julian-edwards/maas/dup-boot-source-selections-bug-1360280 into lp:maas failed. Below is the output from the failed tests.

Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://security.ubuntu.com trusty-security InRelease
Get:1 http://security.ubuntu.com trusty-security Release.gpg [933 B]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:3 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates Release [62.0 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Get:5 http://security.ubuntu.com trusty-security/main Sources [49.5 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [136 kB]
Get:7 http://security.ubuntu.com trusty-security/universe Sources [13.1 kB]
Get:8 http://security.ubuntu.com trusty-security/main amd64 Packages [153 kB]
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [89.5 kB]
Get:10 http://security.ubuntu.com trusty-security/universe amd64 Packages [60.5 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [356 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [217 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Fetched 1,200 kB in 2s (411 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 authbind bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools gjs ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make pep8 postgresql pyflakes python-amqplib python-bzrlib python-celery python-convoy python-crochet python-cssselect python-curtin python-dev python-distro-info python-django python-django-piston python-django-south python-djorm-ext-pgarray python-docutils python-extras python-fixtures python-flake8 python-formencode python-hivex python-httplib2 python-jinja2 python-jsonschema python-lockfile python-lxml python-mimeparse python-mock python-netaddr python-netifaces python-nose python-oauth python-oops python-oops-amqp python-oops-datedir-repo python-oops-twisted p...

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.