launchpad-reviewers team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

> <bigjools> I am trying to be constructive on the review but you're going to
> hate me, sorry :(
> <roaksoax> hehe :)
> 
> Did you guys have any pre-implementation chat with one of us before starting
> this?  I can't approve this as it stands for quite a few fundamental
> architectural reasons, and also it's an 1100+ line diff without a single test
> change.  I did previously say I won't accept any more branches that have
> untested code, especially untested walls of shell :(
> 
> More below.
> 
> 25      === added file 'contrib/preseeds_v2/preseed_xinstall'
> 26      --- contrib/preseeds_v2/preseed_xinstall        1970-01-01 00:00:00
> +0000
> 27      +++ contrib/preseeds_v2/preseed_xinstall        2013-04-17 16:47:25
> +0000
> [snip]
> 36      +preseed = subprocess.check_output(["/usr/share/maas/xinstall/make-
> userdata-maas",
> 37      +    "--apt-proxy="+proxy,
> 38      +    "--dpkg-selections="+preseed_data,
> 39      +    "--finished-url="+node_disable_pxe_url,
> 40      +    "--finished-url-data="+node_disable_pxe_data,
> 41      +    "http://"+cluster_host+"/MAAS/static/images/"+node.architecture+";
> /"+node.distro_series+"/xinstall/root.tar.gz",
> 42      +    "/dev/sda"])
> 43      +}}
> 44      +{{preseed}}
> 
> This scares the bejesus out of me.  This is just a workaround so that the
> shell script make-userdata-maas can be used instead of doing it properly in
> Python, and tested.  It has completely sidestepped all of the code that exists

"doing it properly in python" is a general problem that maas has.   Maas knows too much.  The installer (xinstall) is not maas.  The proper way to do this is to not have the xinstall code in maas at all, and have maas know an interface to it.  We've pushed xinstall here as a temporary landing location.

What you have above is a sane way to interface between maas an an installer.

We've explicitly put this is a preseed as these templates are sold as user-configurable.  Some configuration may need to be done by an end user, and this is how they do that.  Making maas invoke that code internally means a user has to change maas python code to make a change.

> already in src/maasserver/preseed.py and src/maasserver/compose_preseed.py
> that calculates preseed_data based on the node state.
> 
> It is extremely fragile and it is likely to break, horribly, in many ways, as
> soon as someone changes anything elsewhere.

We have 2 components.  yes, one component can break another component.  A change to python can break maas.  A change to glibc can break python. a change to the kernel can break glibc.  Thats *good* design.

> At the very least, this template should not be calling out to the shell
> script.  If you can't re-write it in Python and test it in the time available,
> it should be called from the existing Python code that generates preseeds and
> returned in preseed_data for the template.  It can then be tested so we know
> the right preseed is getting returned for FPI.
> 
> 
> 1045    === modified file 'src/maasserver/api.py'
> 1061    === modified file 'src/maasserver/models/nodegroup.py'
> 1077    === modified file 'src/maasserver/preseed.py'
> 1096    === modified file 'src/provisioningserver/kernel_opts.py'
> 1135    === modified file 'src/provisioningserver/pxe/install_image.py'
> 
> The changes in all these files need tests.  I expect some tests are broken as
> a result but I can't run the test suite at the moment as I get this:
> 
> $ make build
> bin/buildout install database
> Traceback (most recent call last):
>   File "bin/buildout", line 5, in <module>
>     from pkg_resources import load_entry_point
>   File "/home/ed/canonical/maas/sandbox/local/lib/python2.7/site-
> packages/distribute-0.6.24-py2.7.egg/pkg_resources.py", line 16, in <module>
>     import sys, os, zipimport, time, re, imp, types
>   File "/home/ed/canonical/maas/sandbox/lib/python2.7/re.py", line 105, in
> <module>
>     import sre_compile
>   File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_compile.py", line
> 14, in <module>
>     import sre_parse
>   File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_parse.py", line 17,
> in <module>
>     from sre_constants import *
>   File "/home/ed/canonical/maas/sandbox/lib/python2.7/sre_constants.py", line
> 18, in <module>
>     from _sre import MAXREPEAT
> ImportError: cannot import name MAXREPEAT
> 
> 1109    === added file 'src/provisioningserver/pxe/config.xinstall.template'
> 1110    --- src/provisioningserver/pxe/config.xinstall.template 1970-01-01
> 00:00:00 +0000
> 1111    +++ src/provisioningserver/pxe/config.xinstall.template 2013-04-17
> 16:47:25 +0000
> [snip]
> 1119    +LABEL amd64
> 1120    +  SAY Booting (amd64) under MAAS direction...
> [snip]
> 1127    +LABEL i386
> 1128    +  SAY Booting (i386) under MAAS direction...
> 
> Don't bother with both SAY commands, they get parsed and executed before
> choosing the LABEL so both get printed.  You could fix the existing templates
> too.
> 
> I have not reviewed the changes in the contrib/ directory, shell is not my
> expertise.  Also, no tests :(

Its an installer. It should be separate.  Please just review this as if that code was not there.  We'll replace or move it at a later date.

> I realise that you need to get *something* in raring before FF tomorrow.
> Last-minute rushed changes are never good, but if you can at least add tests
> and make the change to the preseed template as I suggested then this could
> land with a view to improving more later.

I really prefer to use user-editable and modifable template files than maas python files for the interface between 'xinstall' and maas.

-- 
https://code.launchpad.net/~andreserl/maas/trunk-fpi/+merge/152039
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~andreserl/maas/trunk-fpi into lp:maas.