Merge lp://qastaging/~blake-rouse/maas/commissioning-get-disk-info into lp://qastaging/~maas-committers/maas/trunk

Proposed by Blake Rouse
Status: Merged
Approved by: Blake Rouse
Approved revision: no longer in the source branch.
Merged at revision: 3443
Proposed branch: lp://qastaging/~blake-rouse/maas/commissioning-get-disk-info
Merge into: lp://qastaging/~maas-committers/maas/trunk
Prerequisite: lp://qastaging/~blake-rouse/maas/physical-block-device-model
Diff against target: 532 lines (+467/-0)
2 files modified
src/metadataserver/models/commissioningscript.py (+121/-0)
src/metadataserver/models/tests/test_noderesults.py (+346/-0)
To merge this branch: bzr merge lp://qastaging/~blake-rouse/maas/commissioning-get-disk-info
Reviewer Review Type Date Requested Status
Raphaël Badin (community) Approve
Review via email: mp+245585@code.qastaging.launchpad.net

Commit message

Populates the PhysicalBlockDevice model for a node when commissioning. Uses a combination of lsblk, udevadm, and blockdev to gather the information from the node during commissioning.

To post a comment you must log in.
Revision history for this message
Raphaël Badin (rvb) wrote :

This looks generally good. I've got a bunch of comments but more importantly, I'm wondering about the failure modes for this. See below.

This uses a lot of commands and a fair share of parsing. All of which could go wrong at some point and break the commissioning. I think it's worth taking a step back and thinking about the failure modes here:
- which part of the code are likely to fail (command failure, parsing failure, etc)? Can we make this more robust against non-fatal error?
- what's the failure mode? In other words, what is going to happen when this fails, what will the consequences be, how easy will it be to the user to get information about what's wrong?

What do you think?

review: Needs Information
Revision history for this message
Blake Rouse (blake-rouse) wrote :

I think at the moment we should leave it how it is. I understand your concern, but the issue is that all the calls that are made are required and not optional. Currently if this script fails then all of commissioning fails, which would allow the user to view the output where the exception would be outputted. The only way I can think around this is to remove disks as commands for that disk fail, which might be even more confusing for the user.

Revision history for this message
Raphaël Badin (rvb) wrote :

> Currently if this script fails then all of commissioning fails, which would allow the user to
> view the output where the exception would be outputted. The only way I can think around this is
> to remove disks as commands for that disk fail, which might be even more confusing for the user.

I'm a bit concerned about releasing code that might prevent the nodes from being commissioned at all if the parsing fails (given that the commands we run and the parsing we do are non-trivial). Now, I'm fine with releasing this as is as long as we preform extensive QA on this and test it on all the machines that we have: the lab, the garage MAAS, Jason's lab, OIL, etc. Can you please do this?

fwiw, I really think this ugly code could be best done with a regular expression but I'm not going to block this branch for it :)

+ info_line = info_line.strip()
+ if info_line == "":
+ continue
+ _, info = info_line.split(" ", 1)
+ if "=" not in info:
+ continue
+ k, v = info.split("=", 1)

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

The attempt to merge lp:~blake-rouse/maas/commissioning-get-disk-info into lp:maas failed. Below is the output from the failed tests.

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 InRelease
Get:2 http://security.ubuntu.com trusty-security Release [62.0 kB]
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
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]
Get:5 http://security.ubuntu.com trusty-security/main Sources [59.7 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [17.4 kB]
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [187 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
Hit http://nova.clouds.archive.ubuntu.com trusty/universe amd64 Packages
Get:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [82.2 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Translation-en
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [152 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [96.5 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [392 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [236 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,348 kB in 3s (441 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 python-oops-wsg...

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.