MAAS

Merge lp://staging/~newell-jensen/maas/fix-1596046 into lp://staging/~maas-committers/maas/trunk

  1. fix-1596046
  2. Merge into trunk
Proposed by Newell Jensen
Status: Merged
Approved by: Newell Jensen
Approved revision: no longer in the source branch.
Merged at revision: 5149
Proposed branch: lp://staging/~newell-jensen/maas/fix-1596046
Merge into: lp://staging/~maas-committers/maas/trunk
Diff against target: 264 lines (+94/-48)
4 files modified
src/provisioningserver/boot/powernv.py (+28/-16)
src/provisioningserver/boot/tests/test_powernv.py (+29/-29)
src/provisioningserver/pserv_services/tests/test_tftp.py (+25/-0)
src/provisioningserver/pserv_services/tftp.py (+12/-3)
To merge this branch: bzr merge lp://staging/~newell-jensen/maas/fix-1596046
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+298690@code.staging.launchpad.net

Commit message

This branch fixes a regression for powerNV where the "b'ppc64el/" was getting hardcoded into the dhcpd.conf. Additionally, it adds `path` to GetBootConfig (new in MAAS 2.0) on the rack, which was left out and needed.

To post a comment you must log in.
Revision history for this message
Newell Jensen (newell-jensen) wrote :

This was tested on powerNV hardware in 1ss. The nodes are now able to enlist, commission, and deploy with this fix.

Revision history for this message
Gavin Panella (allenap) wrote :

m.rpc.boot.get_config, the eventual receiver of the added "path"
argument, doesn't actually use it.

I'm about 95% sure that it should not be passed. The arguments to
GetBootConfig are those _derived_ from the TFTP request; a raw path
should not be amongst them.

I think the bug is that PowerNVBootMethod.get_params() should not be
returning a dict with a "path" key. What do you think?

review: Needs Information
Revision history for this message
Newell Jensen (newell-jensen) wrote :

After looking at it, I agree. I didn't originally write powernv.py but
when I looke through the other boot methods, I don't see them passing
"path" to the paraemters. Will change this and test again.

On Wed, Jun 29, 2016 at 11:41 AM, Gavin Panella <<email address hidden>
> wrote:

> Review: Needs Information
>
> m.rpc.boot.get_config, the eventual receiver of the added "path"
> argument, doesn't actually use it.
>
> I'm about 95% sure that it should not be passed. The arguments to
> GetBootConfig are those _derived_ from the TFTP request; a raw path
> should not be amongst them.
>
> I think the bug is that PowerNVBootMethod.get_params() should not be
> returning a dict with a "path" key. What do you think?
>
> --
> https://code.launchpad.net/~newell-jensen/maas/fix-1596046/+merge/298690
> You are the owner of lp:~newell-jensen/maas/fix-1596046.
>

Revision history for this message
Newell Jensen (newell-jensen) wrote :

Gavin,

So powerNV needs 'path' for its get_reader method. However, I was able to strip the need of modifying the RPC calls. Have another look. Thanks.

Revision history for this message
Gavin Panella (allenap) wrote :

This looks good but I suggest solving it by selecting arguments to pass to GetBootConfig instead of killing off errant keys.

PowerNV also trims the prefix in two different places, which seems wasteful, and confusing when reading the code.

I've fixed both of these in lp:~allenap/maas/newell-jensen--fix-1596046. If you pull that into your branch you can see what I've fixed in the last couple of revisions.

Revision history for this message
Newell Jensen (newell-jensen) wrote :

Gavin,

Merged your branch, tested it on the hardware, fixed the issues, and pushed again. Have a look again when you can, thanks.

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good. I'm glad you figured out why it wasn't working. One comment to check out; it won't hold you up long.

review: Approve

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.