MAAS

Code review comment for lp:~andreserl/maas/trunk-fpi

trunk-fpi
Merge into trunk

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2013-04-18:

<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 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.

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 :(

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.

<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 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.

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:

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 :(

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.

« Back to merge proposal