Merge lp://staging/~julian-edwards/maas/api-for-power-query 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: 3045
Proposed branch: lp://staging/~julian-edwards/maas/api-for-power-query
Merge into: lp://staging/~maas-committers/maas/trunk
Diff against target: 297 lines (+160/-5)
6 files modified
src/maasserver/api/nodes.py (+61/-0)
src/maasserver/api/tests/test_doc.py (+1/-0)
src/maasserver/api/tests/test_nodes.py (+84/-0)
src/maasserver/exceptions.py (+11/-0)
src/maasserver/models/node.py (+1/-4)
src/maasserver/models/tests/test_node.py (+2/-1)
To merge this branch: bzr merge lp://staging/~julian-edwards/maas/api-for-power-query
Reviewer Review Type Date Requested Status
Jeroen T. Vermeulen (community) Approve
Review via email: mp+235422@code.staging.launchpad.net

Commit message

Add an API call Node.power_state() which asks the cluster controller to query the state of the Node's power and returns the result. This is mainly intended for use in the web UI as a "test power parameters" button. It will hold up the appserver thread for up to 30 seconds waiting for the power query to finish, so use sparingly.

Description of the change

I also removed a duplicate UnknownPowerAction exception...

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Thorough work. I do have a few questions:

.

Half a minute seems generous... Are we really likely to need that long? It's an awfully long time to hold up an app server thread.

And if calling this method is likely to have a noticeable impact on the system, I wouldn't name it as if it were just a getter. Why not call it query_power_state to make it clear that it's “doing things” and not just “returning data”?

.

Not sure it matters but it looks to me as if 503: Service Unavailable was meant to signify that the web service as such is down, not just the resource that's being accessed.

.

What does “Power state is not queryable” mean? Is there anything more that can be said about that kind of error?

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

On Monday 22 September 2014 05:25:47 you wrote:
> Review: Approve
>
> Thorough work. I do have a few questions:
>
> .
>
> Half a minute seems generous... Are we really likely to need that long?
> It's an awfully long time to hold up an app server thread.

Remember that this is the *maximum*. If the power method is quicker then it
won't last that long. 30 seconds is backstop.

> And if calling this method is likely to have a noticeable impact on the
> system, I wouldn't name it as if it were just a getter. Why not call it
> query_power_state to make it clear that it's “doing things” and not just
> “returning data”?

Good point, I'll change it, thanks.

> Not sure it matters but it looks to me as if 503: Service Unavailable was
> meant to signify that the web service as such is down, not just the
> resource that's being accessed.

I used 503 when we're out of IPs as well. I'm not sure there's a better one.
Here's the blurb for 503:

"The Web server (running the Web site) is currently unable to handle the HTTP
request due to a temporary overloading or maintenance of the server. The
implication is that this is a temporary condition which will be alleviated
after some delay."

I didn't want to use BAD_REQUEST as that implies you must not send the request
again without modifying it.

> What does “Power state is not queryable” mean? Is there anything more that
> can be said about that kind of error?

That's a tough one. At the moment, it means that the power type is WoL, but
that is abstracted away through the method it's calling. I'm not sure what
else to say in the error.

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

The attempt to merge lp:~julian-edwards/maas/api-for-power-query into lp:maas failed. Below is the output from the failed tests.

Ign http://security.ubuntu.com trusty-security InRelease
Hit http://security.ubuntu.com trusty-security Release.gpg
Hit http://security.ubuntu.com trusty-security Release
Ign http://nova.clouds.archive.ubuntu.com trusty InRelease
Ign http://nova.clouds.archive.ubuntu.com trusty-updates InRelease
Hit http://nova.clouds.archive.ubuntu.com trusty Release.gpg
Get:1 http://nova.clouds.archive.ubuntu.com trusty-updates Release.gpg [933 B]
Hit http://nova.clouds.archive.ubuntu.com trusty Release
Get:2 http://nova.clouds.archive.ubuntu.com trusty-updates Release [59.7 kB]
Hit http://security.ubuntu.com trusty-security/main Sources
Hit http://security.ubuntu.com trusty-security/universe Sources
Hit http://security.ubuntu.com trusty-security/main amd64 Packages
Hit http://security.ubuntu.com trusty-security/universe amd64 Packages
Hit http://security.ubuntu.com trusty-security/main Translation-en
Hit http://security.ubuntu.com trusty-security/universe Translation-en
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
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:3 http://nova.clouds.archive.ubuntu.com trusty-updates/main Sources [120 kB]
Get:4 http://nova.clouds.archive.ubuntu.com trusty-updates/universe Sources [84.7 kB]
Get:5 http://nova.clouds.archive.ubuntu.com trusty-updates/main amd64 Packages [320 kB]
Get:6 http://nova.clouds.archive.ubuntu.com trusty-updates/universe amd64 Packages [203 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 788 kB in 0s (1,876 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 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-wsgi python-openssl python-paramiko python-pexpect python-pip python-pocket-lint pyt...

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.