MAAS

Merge lp://staging/~stefankrupop/maas/recs into lp://staging/~maas-committers/maas/trunk

recs
Merge into trunk

Proposed by Stefan Krupop on 2017-05-19

Status:	Merged
Approved by:	Newell Jensen on 2017-06-21
Approved revision:	no longer in the source branch.
Merged at revision:	6103
Proposed branch:	lp://staging/~stefankrupop/maas/recs
Merge into:	lp://staging/~maas-committers/maas/trunk
Diff against target:	1092 lines (+932/-15) 8 files modified src/maasserver/api/machines.py (+14/-11) src/maasserver/api/tests/test_machines.py (+7/-4) src/maasserver/static/js/angular/controllers/add_hardware.js (+38/-0) src/provisioningserver/drivers/power/recs.py (+345/-0) src/provisioningserver/drivers/power/registry.py (+2/-0) src/provisioningserver/drivers/power/tests/test_recs.py (+465/-0) src/provisioningserver/rpc/clusterservice.py (+7/-0) src/provisioningserver/rpc/tests/test_clusterservice.py (+54/-0)
To merge this branch:	bzr merge lp://staging/~stefankrupop/maas/recs
Related bugs:	Link a bug report

Reviewer	Date Requested	Status
Newell Jensen (community)		Approve on 2017-06-21
Stefan Krupop (community)		Needs Resubmitting on 2017-06-21
Mike Pontillo (community)	2017-05-19	Needs Information on 2017-06-07
Review via email: mp+324310@code.staging.launchpad.net

Commit message

adds power management support for christmann RECS|Box servers (https://embedded.christmann.info/).

Description of the change

This branch adds power management support for christmann RECS|Box servers (https://embedded.christmann.info/). Supports adding a chassis and controlling power of nodes.

Currently, trying to add a chassis via the WebGUI runs into bug #1572060 (https://bugs.launchpad.net/maas/+bug/1572060). I was unsure whether I should remove the port field because of this, but left it in for now.

Originally, I developed the change last year against some older MAAS version and based it on the Seamicro 15k driver. As I am normally not a Python developer, I hope I did not mess up the update too much. Please bear with me ;)

Revision history for this message

Mike Pontillo (mpontillo) wrote on 2017-05-19:

Thanks for your contribution. Unfortunately there is legal due diligence that must happen before we can consider merging your work. Would you please follow the instructions here in order to sign our contributor agreement?

https://www.ubuntu.com/legal/contributors

We would be happy to consider your contributions after the agreement is in place.

With regard to the code, (since Python is an interpreted language) the MAAS team coding standards require that each line of code be covered by unit tests. If you look in the `tests/` directory you should find `test_*.py` files that roughly correspond to the Python source in `../*.py`. That is us to accept this proposal, you would need to amend the tests for any files that have changed, and also create a new test file for your new driver.

As an aside, it would be tricky for the MAAS team to officially support this type of server, since we do not have test hardware that would allow us to ensure that your contribution works in the future, as more changes are made to MAAS and/or the RECS|Box APIs. I don't think we should block your contribution based on that, but this is also something that deserves discussion; perhaps our organizations' respective business development teams should talk to see if we can better collaborate.

review: Needs Fixing

Revision history for this message

Stefan Krupop (stefankrupop) wrote on 2017-06-07:

Ok, I have signed the agreement and also added the required tests.
Coverage for the new parts should be 100 %.
I also improved error handling a bit and made sure there are no lint errors left.

Kind regards,
Stefan

review: Needs Resubmitting

Revision history for this message

Mike Pontillo (mpontillo) wrote on 2017-06-07:

Thanks for the fixes! I see that the tests are passing after I merge your branch to trunk.

I think the next step is to talk to the team about how to support christmann RECS|Box in MAAS, given that we have no way to test it directly. As it stands now, we risk breaking your code unintentionally. What is your role with regard to the christmann RECS|Box product, and can we expect that this code will be maintained long-term?

My other question is, I notice that the power type is just 'recs', is that specific enough for 'christmann RECS|Box' or might it be confused with another meaning of "RECS"?

If you could e-mail me directly so we can start that conversation, that would be appreciated. You can find my @canonical.com address on my Launchpad profile[1]. Thanks in advance!

[1]: https://launchpad.net/~mpontillo

review: Needs Information

Revision history for this message

Newell Jensen (newell-jensen) wrote on 2017-06-07:

Stefan,

See my inline comments for some minor fixes. Additionally, I see that you have a provisioningserver/drivers/hardware/recs.py and a provisioningserver/drivers/power/recs.py. We have been making an effort to put everything into power/* and the only reason there is still code in hardware/* is because we just haven't gotten around to migrating that last code over.

Please move the code that you have in the hardware directory into the power directory. There are other drivers in power directory that you can look at combine all functionality into one.

After this consolidation I will take a closer look at your code.

review: Needs Fixing

Revision history for this message

Stefan Krupop (stefankrupop) wrote on 2017-06-20:

I now moved driver and tests from drivers/hardware/ to drivers/power/. I also changed the power type to "recs_box".

Kind regards,
Stefan

review: Needs Resubmitting

Revision history for this message

Newell Jensen (newell-jensen) wrote on 2017-06-20:

Looks good. Thanks for the changes (moving from hardware to driver folder). I have one inline comment and once you fix this I will set to approved.

review: Needs Fixing

Revision history for this message