Merge lp://staging/~rvb/maas/dhcp-stop-bug-1328656 into lp://staging/~maas-committers/maas/trunk

Don't feel guilty for not tracking the state of the DHCP server: with these things you sort of have to assume that you'll get it wrong anyway, what with restarts and failures and race conditions. The only truth is reality itself — whether the service is actually running.

I do think the check for the error message is still a bit brittle. For starters, make sure the upstart command runs in the “C” locale so we don't get a translated version of the message. Also I'd at least normalise whitespace: ' '.join(output.strip().split()). It may even be worth a dedicated test.

You may also want to say that (apparently) the error ends in a colon, with nothing to follow. It looks weird, and reading it I sort of expected a "message begins with" check.

The comments about the upstart error (lines 16—17 and 33—34 of the diff) strike me as being the wrong way around. From the reader's perspective, the fact that Upstart issues this error is the first thing. After that it may or may not be worth saying that we use these variables to recognise that situation, or that we ignore it, respectively.

Thanks for the review.

> For starters, make sure the upstart command runs in the “C” locale so we don't get a translated
> version of the message.

How do you go about doing that?

You do that by setting environment variable LC_ALL to “C”.

> Also I'd at least normalise whitespace:
> ' '.join(output.strip().split()). It may even be worth a dedicated test.

Why do you think this is worth the trouble? I mean, this error string is present in upstart's source code.

> You may also want to say that (apparently) the error ends in a colon, with
> nothing to follow. It looks weird, and reading it I sort of expected a
> "message begins with" check.

I know it looks weird, but that's what the error looks like. I could relax the condition a bit (by just checking that the error contains "Unknown instance") but I think it's much more important not to hide legitimate errors so I'd rather keep a very precise check. The worse that can happen is that the user will see misleading stacktraces: painful but not as misleading as missing valid errors.

> You do that by setting environment variable LC_ALL to “C”.

Oh, right. Sorry for being thick, I thought you meant something else.

> > Also I'd at least normalise whitespace:
> > ' '.join(output.strip().split()). It may even be worth a dedicated test.
>
> Why do you think this is worth the trouble? I mean, this error string is
> present in upstart's source code.

Because nobody will think twice about changing the handling of whitespace, not just in Upstart but also in how we process error output. For example we might reduce stripping, and suddenly get a newline at the end. I don't care much about the internal whitespace.

> > You may also want to say that (apparently) the error ends in a colon, with
> > nothing to follow. It looks weird, and reading it I sort of expected a
> > "message begins with" check.
>
> I know it looks weird, but that's what the error looks like. I could relax
> the condition a bit (by just checking that the error contains "Unknown
> instance") but I think it's much more important not to hide legitimate errors
> so I'd rather keep a very precise check. The worse that can happen is that
> the user will see misleading stacktraces: painful but not as misleading as
> missing valid errors.

I'm not asking you to change the code, only to document that the error is weird.

> > Why do you think this is worth the trouble? I mean, this error string is
> > present in upstart's source code.
>
> Because nobody will think twice about changing the handling of whitespace, not
> just in Upstart but also in how we process error output. For example we might
> reduce stripping, and suddenly get a newline at the end. I don't care much
> about the internal whitespace.

Okay, makes sense, done.

> > > You may also want to say that (apparently) the error ends in a colon, with
> > > nothing to follow. It looks weird, and reading it I sort of expected a
> > > "message begins with" check.
> >
> > I know it looks weird, but that's what the error looks like. I could relax
> > the condition a bit (by just checking that the error contains "Unknown
> > instance") but I think it's much more important not to hide legitimate
> errors
> > so I'd rather keep a very precise check. The worse that can happen is that
> > the user will see misleading stacktraces: painful but not as misleading as
> > missing valid errors.
>
> I'm not asking you to change the code, only to document that the error is
> weird.

k, done.

The attempt to merge lp:~rvb/maas/dhcp-stop-bug-1328656 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 [58.5 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 [58.5 kB]
Get:5 http://security.ubuntu.com trusty-security/main Sources [17.9 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main Sources
Get:6 http://security.ubuntu.com trusty-security/universe Sources [4,212 B]
Hit http://nova.clouds.archive.ubuntu.com trusty/universe Sources
Get:7 http://security.ubuntu.com trusty-security/main amd64 Packages [56.3 kB]
Hit http://nova.clouds.archive.ubuntu.com trusty/main amd64 Packages
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:8 http://security.ubuntu.com trusty-security/universe amd64 Packages [17.9 kB]
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
Ign http://security.ubuntu.com trusty-security/main Translation-en_US
Ign http://security.ubuntu.com trusty-security/universe Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty/universe Translation-en_US
Get:9 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [60.1 kB]
Get:10 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [30.1 kB]
Get:11 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [149 kB]
Get:12 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [81.0 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-updates/main Translation-en_US
Ign http://nova.clouds.archive.ubuntu.com trusty-updates/universe Translation-en_US
Fetched 536 kB in 0s (1,980 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
     --no-install-recommends install apache2 bind9 bind9utils build-essential bzr-builddeb curl daemontools debhelper dh-apport distro-info dnsutils firefox freeipmi-tools ipython isc-dhcp-common libjs-raphael libjs-yui3-full libjs-yui3-min libpq-dev make postgresql 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-formencode python-httplib2 python-jinja2 python-j...