Merge lp:~andreserl/maas/trunk-fpi into lp:maas/trunk

Proposed by Andres Rodriguez on 2013-03-06
Status: Merged
Approved by: Diogo Matsubara on 2013-05-14
Approved revision: 1476
Merged at revision: 1477
Proposed branch: lp:~andreserl/maas/trunk-fpi
Merge into: lp:maas/trunk
Diff against target: 501 lines (+231/-16)
17 files modified
contrib/maas-cluster-http.conf (+6/-0)
contrib/preseeds_v2/generic (+4/-0)
contrib/preseeds_v2/preseed_xinstall (+1/-0)
scripts/maas-import-ephemerals (+6/-1)
scripts/uec2roottar (+56/-0)
setup.py (+4/-0)
src/maasserver/api.py (+4/-1)
src/maasserver/models/nodegroup.py (+5/-0)
src/maasserver/preseed.py (+7/-1)
src/maasserver/tests/test_api.py (+3/-0)
src/maasserver/tests/test_preseed.py (+13/-5)
src/provisioningserver/kernel_opts.py (+5/-2)
src/provisioningserver/pxe/config.xinstall.template (+20/-0)
src/provisioningserver/pxe/install_image.py (+12/-2)
src/provisioningserver/pxe/tests/test_config.py (+10/-2)
src/provisioningserver/pxe/tests/test_install_image.py (+35/-0)
src/provisioningserver/tests/test_kernel_opts.py (+40/-2)
To merge this branch: bzr merge lp:~andreserl/maas/trunk-fpi
Reviewer Review Type Date Requested Status
Andres Rodriguez Approve on 2013-05-10
Gavin Panella (community) 2013-03-06 Abstain on 2013-05-10
Julian Edwards (community) Approve on 2013-05-10
Review via email: mp+152039@code.launchpad.net

Commit message

Fast Path Installer

To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :
Download full text (4.1 KiB)

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

Read more...

Julian Edwards (julian-edwards) wrote :

So based on the conversation I just had with Andres he explained that the FPI script stuff here is also going to be used on the CD installer so they wanted to share the code.

The maas-make-userdata script is used to generate a template, and the intention is that in the future people should be able to pass extra scripts to run in preinst and postinst time in the installer.

I am strongly of the opinion that that template should be static in MAAS and any data that it requires comes from MAAS (it does now anyway - it's just indirect!). MAAS's templating system will handle this requirement with ease and vastly reduce this complexity. It is fragile and will be very very hard to debug problems.

Julian Edwards (julian-edwards) wrote :
Download full text (32.5 KiB)

To fix the devel env, you need to merge trunk and then "make distclean".

The following tests are all broken in your branch:

======================================================================
ERROR: maasserver.tests.test_preseed.TestPreseedMethods.test_get_preseed_returns_default_preseed
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/ed/canonical/maas/sandbox/src/maasserver/tests/test_preseed.py", line 525, in test_get_preseed_returns_default_preseed
    preseed = get_preseed(node)
  File "/home/ed/canonical/maas/sandbox/src/maasserver/preseed.py", line 80, in get_preseed
    release=node.get_distro_series())
  File "/home/ed/canonical/maas/sandbox/src/maasserver/preseed.py", line 306, in render_preseed
    return template.substitute(**context)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 177, in substitute
    result = self._interpret_inherit(result, defs, inherit, ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 204, in _interpret_inherit
    return templ.substitute(ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 173, in substitute
    result, defs, inherit = self._interpret(ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 184, in _interpret
    self._interpret_codes(self._parsed, ns, out=parts, defs=defs)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 212, in _interpret_codes
    self._interpret_code(item, ns, out, defs)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 218, in _interpret_code
    self._exec(code[2], ns, pos)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 312, in _exec
    exec code in self.default_namespace, ns
  File "<string>", line 12, in <module>
TypeError: coercing to Unicode: need string or buffer, NoneType found at line 1 column 3 in file /home/ed/canonical/maas/sandbox/contrib/preseeds_v2/preseed_xinstall

======================================================================
ERROR: maasserver.tests.test_preseed.TestPreseedProxy.test_preseed_uses_configured_proxy
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/ed/canonical/maas/sandbox/src/maasserver/tests/test_preseed.py", line 513, in test_preseed_uses_configured_proxy
    factory.make_node(), PRESEED_TYPE.DEFAULT, "precise")
  File "/home/ed/canonical/maas/sandbox/src/maasserver/preseed.py", line 306, in render_preseed
    return template.substitute(**context)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 177, in substitute
    result = self._interpret_inherit(result, defs, inherit, ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 204, in _interpret_inherit
    return templ.substitute(ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 173, in substitute
    result, defs, inherit = self._interpret(ns)
  File "/usr/lib/python2.7/dist-packages/tempita/__init__.py", line 184, in _interpret
    self._interpret_codes(self._parsed, ns, out=parts, defs=defs)
  File "/usr...

Scott Moser (smoser) wrote :
Download full text (5.5 KiB)

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

Read more...

Julian Edwards (julian-edwards) wrote :
Download full text (3.7 KiB)

Hi Scott,

On 19/04/13 03:09, Scott Moser wrote:
> "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.

I don't think maas itself knows too much - all the installer specific
stuff is in templates, exactly where it should be. However I would love
for the xinstall code to not be in MAAS too. Fortunately there's an
easy solution, we just use the template feature that MAAS already has.

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

With respect, in no way do I think that is sane and you'll get the same
response from the rest of the guys in my team. I've explained already
where it's faults are and how to do it better, so I won't do so again.

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

Right now with this branch you have a situation like this:
- Maas renders a preseed template, passing in some data to it
- The template shells out to a script, passing it the data, that
generates another template

This can be vastly simplified as:
 - Maas renders a template, passing in some data to it

where the template is the one that the script previously generated.

The same configuration and flexibility is still there, without the
intermediate step. The template system we chose is very powerful.

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

I think that's a strawman. It's not good to have things break. You're
also falsely assuming the likelihood of Python/glibc/kernel breaking is
of the same likelihood as a change here. This change is fragile and it
will break easily.

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

The fact that it is here and you know it should be separate is enough of
a warning that this approach is wrong.

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

I was never suggesting modifiable python files. The existing preseed
setup is tot...

Read more...

Gavin Panella (allenap) wrote :
Download full text (3.3 KiB)

On 18 April 2013 18:09, Scott Moser <email address hidden> wrote:
...
> "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.

I say more later, but if it's not MAAS then it shouldn't land in MAAS;
it should exist as a patch in the packaging.

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

It's a nitpick, but that's not good design, that's just a property of
interrelating systems.

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

There's no one actively driving MAAS development at this very moment,
but I guess we - the Red Squad - still feel the most responsible, and
we still care about the thing we've created. Our criteria for landing
a change to any product that we're working on is: does this improve
the product? This change does not.

It satisfies a short-term goal, that of getting something to work for
Raring, but it's a horrible change: it contains a moderately complex
program written in shell-script (meaning it's now highly complex); the
script has been cargo-culted from somewhere else, and so can diverge
unchecked; paths have been hard-coded into the template; there are no
tests; and so on.

If MAAS's limitations have meant that this was all that was possible
in the time available, then more time needs to be made available:
changing MAAS *needs* to be part of the engineering effort.

This change should *not* land on trunk. Previous changes to trunk in
the same vein as this - especially those without tests - should
probably not have landed either. This change should be carried as a
patch in the packaging.

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

Fwiw, the template files are user-editable. There's meant to be an
inheritance/override mechanism so that the templates can be installed
by the package but selectively customised in, say, /etc/maas. I'm not
sure if that's effective in practice right now, but that's the
intention. Throwing that away instead of improving it - especially by
replacing it with untested shell-script! - is not doing anyone any
favours.

If you really, definitely want to land large amounts of shell-script
to MAAS, it needs to be tested. We can help by integratiing, say,
ShUnit <http://shunit.s...

Read more...

review: Disapprove
Scott Moser (smoser) wrote :

We have 2 completely separate components here:
 a.) maas
 b.) a fast path installer

There is a problem in that the fast path installer doesn't "live" anywhere. That will be addressed at a later date, and would then be addressed here by removal of any 'xinstall' reference in maas. We'll re-submit this branch with such a removal.

Maas will interface with this fast path installer via popen. Essentially running a command like:
  make-userdata-maas "--apt-proxy={{proxy}}" \
     "--dpkg-selections={{dpkg_set_selections_here}}" \
     "--finished-url={{node_disable_pxe_url}}" \
     "--finished-url-data={{node_disable_pxe_data}}" \
     "{{url_of_thing_to_install}}" "{{target_disk}}"

Having maas invoke that command itself, and making the output available in a preseed variable named 'xinstall_data' which is then rendered in a preseed wholly with '{{xinstall_data}}' is pointlessly complex and not modifiable by the end user.

Julian Edwards (julian-edwards) wrote :

The installer should not live in MAAS upstream code, that is certain. Make a separate package for it and pull it in if you need it.

The problem with temporary fixes is that they become permanent. Especially if they go out in a release - look at how long it's taken to remove Cobbler from 12.04.

>Having maas invoke that command itself, and making the output available in a preseed variable named 'xinstall_data'
>which is then rendered in a preseed wholly with '{{xinstall_data}}' is pointlessly complex and not modifiable by the
>end user.

Yet this is exactly what you are doing via the template that shells out!

If you want FPI to land in MAAS you need to add the FPI template that it needs and populate it with data from MAAS, as both Gavin and I have said above. I don't understand why a separate script is needed.

review: Disapprove
Julian Edwards (julian-edwards) wrote :

Much better without the xinstall stuff in here. It's obviously incomplete but I have no fundamental objections any more. Provided the xinstall preseed is sane in the future, it's looking ok.

As you said, you need tests for all the changes here. Some other comments below:

70 +Usage() {
71 +cat <<EOF
72 + Usage: ${0##*/} uec-tarball [output]
73 +EOF

Can you put a description in here so people know what this script is doing. Also, *sadface* another Wall of Shell™
:(

201 def compose_purpose_opts(params):
202 """Return the list of the purpose-specific kernel options."""
203 - if params.purpose == "commissioning":
204 + if params.purpose == "commissioning" or params.purpose == "xinstall":

Woa, really? Why is this needed?

Andres Rodriguez (andreserl) wrote :

Hi Julian,

Thanks for the review. We will be shipping the xinstall stuff in packaging so won't really be missing anything. I'll however provide a sample preseed to be merged on trunk.

Now, as far as the latter, the 'xinstall' uses exactly the same kernel arguments as the ones the 'commissioning' step uses, with the exception of 'ds=nocloud-net', which is being added in the template file. If you believe is better to add this additional parameter on the code above, I'm also fine doing that.

224 + APPEND ds=nocloud-net {{kernel_params(arch="amd64") | kernel_command}}

Cheers.

Julian Edwards (julian-edwards) wrote :

On 04/05/13 00:51, Andres Rodriguez wrote:
> Hi Julian,
>
> Thanks for the review. We will be shipping the xinstall stuff in packaging so won't really be missing anything. I'll however provide a sample preseed to be merged on trunk.

How can the "sample" one diverge from the xinstall one? And why would
they ever be different?

>
> Now, as far as the latter, the 'xinstall' uses exactly the same kernel arguments as the ones the 'commissioning' step uses, with the exception of 'ds=nocloud-net', which is being added in the template file. If you believe is better to add this additional parameter on the code above, I'm also fine doing that.
>
> 224 + APPEND ds=nocloud-net {{kernel_params(arch="amd64") | kernel_command}}
>
> Cheers.
>

How are you defining the tag that indicates to use the FPI? Because
tags support extra kernel options and that's where ds=nocloud-net should
be. Once it's in tag.kernel_opts, everything will DTRT. Having custom
code is not the right way here.

Cheers.

Andres Rodriguez (andreserl) wrote :

Hi Julian,

The ds=nocloud-net is a required kernel option for xinstall and should not be set when creating/assigning the tag. Since the 'xinstall' is another purpose, aside from commissioning and install, all the required kernel opts should be within MAAS as it is for the other purposes. I have updated the branch for it.

Thanks

Julian Edwards (julian-edwards) wrote :

Ok fair enough on the tag. I guess you want to disassociate the notion of FPI from *how* FPI works (the xinstaller)?

Why did you add the tests in a separate branch? They should be in here!

review: Needs Fixing
Julian Edwards (julian-edwards) wrote :
Download full text (3.3 KiB)

Thanks for the tests! Ok let's review:

154 if node.netboot:
155 - return "install"
156 + if node.should_use_traditional_installer():
157 + return "install"
158 + else:
159 + return "xinstall"

There's no test for this change, can you add one please.

171 + def get_any_interface(self):

This needs its own test please.

206 - def test_preseed_context_null_cluster_host_if_unmanaged(self):
207 + def test_preseed_context_not_null_cluster_host_if_unmanaged(self):
208 # If the nodegroup has no managed interface recorded, which is
209 # possible in the data model but would be a bit weird, the
210 # cluster_host context variable is present, but None.
211 @@ -378,7 +378,7 @@
212 interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
213 interface.save()
214 context = get_preseed_context(release, nodegroup)
215 - self.assertIsNone(context["cluster_host"])
216 + self.assertIsNotNone(context["cluster_host"])

This should check for the actual expected interface rather than it not being None. Also, the test's comment is now wrong, fix that please!

243 + kernel_params = []
244 # These are kernel parameters read by the ephemeral environment.
245 tname = "%s:%s" % (ISCSI_TARGET_NAME_PREFIX,
246 get_ephemeral_name(params.release, params.arch))
247 - return [
248 + if params.purpose == "xinstall":
249 + kernel_params += ["ds=nocloud-net"]
250 + kernel_params += [

You can simplify this code a bit.

Remove the "kernel_params = []" bit, and assign straight to kernel_params instead of the first +=. Then you can add these lines:

    if params.purpose == "xinstall":
        kernel_params.append("ds=nocloud-net")

Much simpler!

274 + SAY Booting (amd64) under MAAS direction...
275 + SAY {{kernel_params(arch="amd64") | kernel_command}}
276 + KERNEL {{kernel_params(arch="amd64") | kernel_path }}
277 + INITRD {{kernel_params(arch="amd64") | initrd_path }}
278 + APPEND {{kernel_params(arch="amd64") | kernel_command}}
279 + IPAPPEND 2
280 +
281 +LABEL i386
282 + SAY Booting (i386) under MAAS direction...
283 + SAY {{kernel_params(arch="i386") | kernel_command}}

Since the "SAY" gets evaluated regardless of the LABEL it's executing, this will repeat 4 lots of SAY. I think we can just make it dump the kernel params as information, just the once.

341 + def test_render_pxe_config_for_xinstall(self):
342 + # The xinstall config uses an extra PXELINUX module to auto
343 + # select between i386 and amd64.
344 + get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
345 + get_ephemeral_name.return_value = factory.make_name("ephemeral")
346 + options = {
347 + "kernel_params":
348 + make_kernel_parameters(purpose="xinstall"),

.. [snip]...

Given that this new test is identical to test_render_pxe_config_for_commissioning except for one piece of data, let's introduce a test scenario into it.

I've done that for you, so just revert your changes t...

Read more...

Andres Rodriguez (andreserl) wrote :

Hi Julian,

I have addressed all of your comments but the SAY one. SO something like?

LABEL execute
  SAY Booting under MAAS direction...
  SAY {{kernel_params() | kernel_command}}
  KERNEL ifcpu64.c32
  APPEND amd64 -- i386

Gavin Panella (allenap) wrote :

On 9 May 2013 10:04, Andres Rodriguez <email address hidden> wrote:
> Hi Julian,
>
> I have addressed all of your comments but the SAY one. SO something like?
>
> LABEL execute
> SAY Booting under MAAS direction...
> SAY {{kernel_params() | kernel_command}}

These SAY commands are processed as the file is parsed, not when the
particular label is chosen. You can't use SAY to show which label was
actually chosen.

For example:

  SAY hello
  LABEL foo
    SAY foo
  LABEL bar
    SAY bar

will echo:

  hello
  foo
  bar

regardless.

Julian Edwards (julian-edwards) wrote :

Andres, looks great! I'll echo what Gavin said about SAY. Just put them only at the TOP level like this:

SAY Booting under MAAS direction...
SAY {{kernel_params() | kernel_command}}

review: Approve
MAAS Lander (maas-lander) wrote :

No commit message specified.

MAAS Lander (maas-lander) wrote :

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0. Got: 1 Approve, 1 Disapprove.

Gavin Panella (allenap) :
review: Abstain
review: Approve
review: Approve
MAAS Lander (maas-lander) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-trunk/255/console reported an error when processing this lp:~andreserl/maas/trunk-fpi branch.
Not merging it.

Andres Rodriguez (andreserl) wrote :

The branch has been rebased against latest trunk.

MAAS Lander (maas-lander) wrote :

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-trunk/275/console reported an error when processing this lp:~andreserl/maas/trunk-fpi branch.
Not merging it.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'contrib/maas-cluster-http.conf'
2--- contrib/maas-cluster-http.conf 1970-01-01 00:00:00 +0000
3+++ contrib/maas-cluster-http.conf 2013-05-11 18:43:26 +0000
4@@ -0,0 +1,6 @@
5+# Server static files for tftp images as FPI
6+# installer needs them
7+Alias /MAAS/static/images/ /var/lib/maas/tftp/
8+<Directory /var/lib/maas/tftp/>
9+ SetHandler None
10+</Directory>
11
12=== modified file 'contrib/preseeds_v2/generic'
13--- contrib/preseeds_v2/generic 2012-11-14 10:32:25 +0000
14+++ contrib/preseeds_v2/generic 2013-05-11 18:43:26 +0000
15@@ -1,4 +1,8 @@
16+{{if node.should_use_traditional_installer() }}
17 {{inherit "preseed_master"}}
18+{{else}}
19+{{inherit "preseed_xinstall"}}
20+{{endif}}
21
22 {{def proxy}}
23 d-i mirror/country string manual
24
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-05-11 18:43:26 +0000
28@@ -0,0 +1,1 @@
29+# Disabled by default
30
31=== modified file 'scripts/maas-import-ephemerals'
32--- scripts/maas-import-ephemerals 2012-11-23 11:00:04 +0000
33+++ scripts/maas-import-ephemerals 2013-05-11 18:43:26 +0000
34@@ -239,6 +239,9 @@
35 tar -Sxzf - -C "$exdir" < "$tarball" ||
36 { error "failed to extract tarball from $furl"; return 1; }
37
38+ # Create root disk
39+ uec2roottar "$tarball" || { error "failed to create root image"; return 1; }
40+
41 # Look for our files in the extracted tarball.
42 local x="" img="" kernel="" initrd=""
43 for x in "$exdir/"*.img; do
44@@ -342,12 +345,14 @@
45 return 1
46 copy_first_available "$src/initrd.gz" "$src/initrd" "$tmpdir/initrd.gz" ||
47 return 1
48+ copy_first_available "$src/dist-root.tar.gz" "" "$tmpdir/root.tar.gz" ||
49+ return 1
50 fi
51
52 local cmd out=""
53 cmd=( maas-provision install-pxe-image
54 "--arch=$arch" "--subarch=$subarch" "--release=$release"
55- --purpose="commissioning" --image="$tmpdir" )
56+ --purpose="commissioning" --image="$tmpdir" --symlink="xinstall")
57 debug 2 "${cmd[@]}"
58 out=$("${cmd[@]}" 2>&1) ||
59 { error "cmd failed:" "${cmd[@]}"; error "$out"; return 1; }
60
61=== added file 'scripts/uec2roottar'
62--- scripts/uec2roottar 1970-01-01 00:00:00 +0000
63+++ scripts/uec2roottar 2013-05-11 18:43:26 +0000
64@@ -0,0 +1,56 @@
65+#!/bin/sh
66+
67+TEMP_D=""
68+MP=""
69+
70+Usage() {
71+cat <<EOF
72+ ${0##*/} converts an ubuntu ephemeral image into a root
73+ disk image tarball.
74+
75+ Usage: ${0##*/} uec-tarball [output]
76+EOF
77+}
78+error() { echo "$(date -R):" "$@" 1>&2; }
79+fail() { [ $# -eq 0 ] || error "$@"; exit 1; }
80+cleanup() {
81+ [ -z "$MP" ] || umount "$MP"
82+ [ -z "$TEMP_D" -o ! -d "$TEMP_D" ] || rm -Rf "${TEMP_D}";
83+}
84+
85+[ "$1" = "-h" -o "$1" = "--help" ] && { Usage; exit 0; }
86+[ $# -eq 1 -o $# -eq 2 ] || { Usage 1>&2; exit 1; }
87+[ -f "$1" ] || { Usage 1>&2; error "$1: not a file"; exit 1; }
88+
89+[ "$(id -u)" = "0" ] || fail "must be root"
90+
91+TEMP_D=$(mktemp -d "${TMPDIR:-/tmp}/${0##*/}.XXXXXX") || fail "failed mktemp"
92+trap cleanup EXIT
93+
94+uec="$1"
95+output=${2}
96+
97+[ -n "$output" ] || output="${uec%.tar.gz}-root.tar.gz";
98+error "converting ${uec} to ${output}"
99+
100+error "extracting *.img from ${uec}"
101+tar -C ${TEMP_D} --wildcards "*.img" -Sxvzf "$uec" ||
102+ fail "failed extract $uec"
103+
104+img=""
105+for cand in "${TEMP_D}/"*.img; do
106+ [ -z "$img" ] || fail "multiple .img files in $uec"
107+ [ -f "$cand" ] && img="$cand"
108+done
109+
110+[ -n "$img" ] || fail "failed to find image in $uec"
111+
112+mkdir "${TEMP_D}/mp" && mount -o ro "$img" "${TEMP_D}/mp" &&
113+ MP="$TEMP_D/mp" || fail "failed to mount $img"
114+
115+error "copying contents of ${img#${TEMP_D}/} in ${uec} to ${output}"
116+tar -C "$MP" -cpSzf "${output}" --numeric-owner . ||
117+ fail "failed to create ${output}"
118+
119+error "finished. wrote to ${output}"
120+
121
122=== modified file 'setup.py'
123--- setup.py 2012-12-18 17:06:43 +0000
124+++ setup.py 2013-05-11 18:43:26 +0000
125@@ -62,6 +62,7 @@
126 'etc/maas/import_ephemerals',
127 'etc/maas/import_pxe_files',
128 'contrib/maas-http.conf',
129+ 'contrib/maas-cluster-http.conf',
130 'contrib/maas_local_settings.py']),
131 ('/usr/share/maas',
132 ['contrib/wsgi.py',
133@@ -73,10 +74,13 @@
134 'contrib/preseeds_v2/enlist',
135 'contrib/preseeds_v2/generic',
136 'contrib/preseeds_v2/enlist_userdata',
137+ 'contrib/preseeds_v2/preseed_xinstall',
138 'contrib/preseeds_v2/preseed_master']),
139 ('/usr/sbin',
140 ['scripts/maas-import-ephemerals',
141 'scripts/maas-import-pxe-files']),
142+ ('/usr/bin',
143+ ['scripts/uec2roottar']),
144 ],
145
146 install_requires=[
147
148=== modified file 'src/maasserver/api.py'
149--- src/maasserver/api.py 2013-05-08 23:03:51 +0000
150+++ src/maasserver/api.py 2013-05-11 18:43:26 +0000
151@@ -1753,7 +1753,10 @@
152 elif node.status == NODE_STATUS.ALLOCATED:
153 # Install the node if netboot is enabled, otherwise boot locally.
154 if node.netboot:
155- return "install"
156+ if node.should_use_traditional_installer():
157+ return "install"
158+ else:
159+ return "xinstall"
160 else:
161 return "local" # TODO: Investigate.
162 else:
163
164=== modified file 'src/maasserver/models/nodegroup.py'
165--- src/maasserver/models/nodegroup.py 2012-11-23 09:54:47 +0000
166+++ src/maasserver/models/nodegroup.py 2013-05-11 18:43:26 +0000
167@@ -204,6 +204,11 @@
168 self.api_key = api_token.key
169 return super(NodeGroup, self).save(*args, **kwargs)
170
171+ def get_any_interface(self):
172+ for interface in self.nodegroupinterface_set.all():
173+ return interface
174+ return None
175+
176 def get_managed_interface(self):
177 """Return the interface for which MAAS managed the DHCP service.
178
179
180=== modified file 'src/maasserver/preseed.py'
181--- src/maasserver/preseed.py 2013-04-08 19:56:00 +0000
182+++ src/maasserver/preseed.py 2013-05-11 18:43:26 +0000
183@@ -232,7 +232,13 @@
184 else:
185 base_url = nodegroup.maas_url
186 cluster_if = nodegroup.get_managed_interface()
187- cluster_host = None if cluster_if is None else cluster_if.ip
188+ any_cluster_if = nodegroup.get_any_interface()
189+ cluster_host = None
190+ if cluster_if is None:
191+ if any_cluster_if is not None:
192+ cluster_host = any_cluster_if.ip
193+ else:
194+ cluster_host = cluster_if.ip
195 return {
196 'main_archive_hostname': main_archive_hostname,
197 'main_archive_directory': main_archive_directory,
198
199=== modified file 'src/maasserver/tests/test_api.py'
200--- src/maasserver/tests/test_api.py 2013-05-08 23:03:51 +0000
201+++ src/maasserver/tests/test_api.py 2013-05-11 18:43:26 +0000
202@@ -3776,11 +3776,14 @@
203 ("poweroff", {"status": NODE_STATUS.READY}),
204 ("poweroff", {"status": NODE_STATUS.RESERVED}),
205 ("install", {"status": NODE_STATUS.ALLOCATED, "netboot": True}),
206+ ("xinstall", {"status": NODE_STATUS.ALLOCATED, "netboot": True}),
207 ("local", {"status": NODE_STATUS.ALLOCATED, "netboot": False}),
208 ("poweroff", {"status": NODE_STATUS.RETIRED}),
209 ]
210 node = factory.make_node()
211 for purpose, parameters in options:
212+ if purpose == "xinstall":
213+ node.use_fastpath_installer()
214 for name, value in parameters.items():
215 setattr(node, name, value)
216 self.assertEqual(purpose, api.get_boot_purpose(node))
217
218=== modified file 'src/maasserver/tests/test_preseed.py'
219--- src/maasserver/tests/test_preseed.py 2013-02-21 23:32:20 +0000
220+++ src/maasserver/tests/test_preseed.py 2013-05-11 18:43:26 +0000
221@@ -368,17 +368,19 @@
222 nodegroup.get_managed_interface().ip,
223 context["cluster_host"])
224
225- def test_preseed_context_null_cluster_host_if_unmanaged(self):
226- # If the nodegroup has no managed interface recorded, which is
227- # possible in the data model but would be a bit weird, the
228- # cluster_host context variable is present, but None.
229+ def test_preseed_context_cluster_host_if_unmanaged(self):
230+ # If the nodegroup has no managed interface recorded, the cluster_host
231+ # context variable is still present and derived from the nodegroup.
232 release = factory.getRandomString()
233 nodegroup = factory.make_node_group(maas_url=factory.getRandomString())
234 for interface in nodegroup.nodegroupinterface_set.all():
235 interface.management = NODEGROUPINTERFACE_MANAGEMENT.UNMANAGED
236 interface.save()
237 context = get_preseed_context(release, nodegroup)
238- self.assertIsNone(context["cluster_host"])
239+ self.assertIsNotNone(context["cluster_host"])
240+ self.assertEqual(
241+ nodegroup.get_any_interface().ip,
242+ context["cluster_host"])
243
244 def test_preseed_context_null_cluster_host_if_does_not_exist(self):
245 # If there's no nodegroup, the cluster_host context variable is
246@@ -525,6 +527,12 @@
247 preseed = get_preseed(node)
248 self.assertIn('preseed/late_command', preseed)
249
250+ def test_get_preseed_returns_xinstall_preseed(self):
251+ node = factory.make_node()
252+ node.use_fastpath_installer()
253+ preseed = get_preseed(node)
254+ self.assertIn('# Disabled by default', preseed)
255+
256 def test_get_enlist_preseed_returns_enlist_preseed(self):
257 preseed = get_enlist_preseed()
258 self.assertTrue(preseed.startswith('#cloud-config'))
259
260=== modified file 'src/provisioningserver/kernel_opts.py'
261--- src/provisioningserver/kernel_opts.py 2013-02-01 01:07:25 +0000
262+++ src/provisioningserver/kernel_opts.py 2013-05-11 18:43:26 +0000
263@@ -124,11 +124,11 @@
264
265 def compose_purpose_opts(params):
266 """Return the list of the purpose-specific kernel options."""
267- if params.purpose == "commissioning":
268+ if params.purpose == "commissioning" or params.purpose == "xinstall":
269 # These are kernel parameters read by the ephemeral environment.
270 tname = "%s:%s" % (ISCSI_TARGET_NAME_PREFIX,
271 get_ephemeral_name(params.release, params.arch))
272- return [
273+ kernel_params = [
274 # Read by the open-iscsi initramfs code.
275 "iscsi_target_name=%s" % tname,
276 "iscsi_target_ip=%s" % params.fs_host,
277@@ -145,6 +145,9 @@
278 # Read by cloud-init.
279 "cloud-config-url=%s" % params.preseed_url,
280 ]
281+ if params.purpose == "xinstall":
282+ kernel_params.append("ds=nocloud-net")
283+ return kernel_params
284 else:
285 # These are options used by the Debian Installer.
286 return [
287
288=== added file 'src/provisioningserver/pxe/config.xinstall.template'
289--- src/provisioningserver/pxe/config.xinstall.template 1970-01-01 00:00:00 +0000
290+++ src/provisioningserver/pxe/config.xinstall.template 2013-05-11 18:43:26 +0000
291@@ -0,0 +1,20 @@
292+DEFAULT execute
293+
294+SAY Booting under MAAS direction...
295+SAY {{kernel_params() | kernel_command}}
296+
297+LABEL execute
298+ KERNEL ifcpu64.c32
299+ APPEND amd64 -- i386
300+
301+LABEL amd64
302+ KERNEL {{kernel_params(arch="amd64") | kernel_path }}
303+ INITRD {{kernel_params(arch="amd64") | initrd_path }}
304+ APPEND {{kernel_params(arch="amd64") | kernel_command}}
305+ IPAPPEND 2
306+
307+LABEL i386
308+ KERNEL {{kernel_params(arch="i386") | kernel_path }}
309+ INITRD {{kernel_params(arch="i386") | initrd_path }}
310+ APPEND {{kernel_params(arch="i386") | kernel_command}}
311+ IPAPPEND 2
312
313=== modified file 'src/provisioningserver/pxe/install_image.py'
314--- src/provisioningserver/pxe/install_image.py 2012-08-31 17:21:01 +0000
315+++ src/provisioningserver/pxe/install_image.py 2013-05-11 18:43:26 +0000
316@@ -70,7 +70,7 @@
317 return False
318
319
320-def install_dir(new, old):
321+def install_dir(new, old, symlink=None):
322 """Install directory `new`, replacing directory `old` if it exists.
323
324 This works as atomically as possible, but isn't entirely. Moreover,
325@@ -121,6 +121,13 @@
326 # Now delete the old image directory at leisure.
327 rmtree('%s.old' % old, ignore_errors=True)
328
329+ # Symlink the new image directory to 'symlink'.
330+ if symlink is not None:
331+ sdest = "%s/%s" % (os.path.dirname(old), symlink)
332+ if os.path.exists(sdest) or os.path.islink(sdest):
333+ os.unlink(sdest)
334+ os.symlink(old, sdest)
335+
336
337 def add_arguments(parser):
338 parser.add_argument(
339@@ -138,6 +145,9 @@
340 parser.add_argument(
341 '--image', dest='image', default=None,
342 help="Netboot image directory, containing kernel & initrd.")
343+ parser.add_argument(
344+ '--symlink', dest='symlink', default=None,
345+ help="Destination directory to symlink the installed images to.")
346
347
348 def run(args):
349@@ -154,5 +164,5 @@
350 tftproot, args.arch, args.subarch, args.release, args.purpose)
351 if not are_identical_dirs(destination, args.image):
352 # Image has changed. Move the new version into place.
353- install_dir(args.image, destination)
354+ install_dir(args.image, destination, args.symlink)
355 rmtree(args.image, ignore_errors=True)
356
357=== modified file 'src/provisioningserver/pxe/tests/test_config.py'
358--- src/provisioningserver/pxe/tests/test_config.py 2012-09-19 13:51:15 +0000
359+++ src/provisioningserver/pxe/tests/test_config.py 2013-05-11 18:43:26 +0000
360@@ -223,14 +223,22 @@
361 self.assertIn("chain.c32", output)
362 self.assertNotIn("LOCALBOOT", output)
363
364- def test_render_pxe_config_for_commissioning(self):
365+class TestRenderPXEConfigScenarios(TestCase):
366+ """Tests for `provisioningserver.pxe.config.render_pxe_config`."""
367+
368+ scenarios = [
369+ ("commissioning", dict(purpose="commissioning")),
370+ ("xinstall", dict(purpose="xinstall")),
371+ ]
372+
373+ def test_render_pxe_config_scenarios(self):
374 # The commissioning config uses an extra PXELINUX module to auto
375 # select between i386 and amd64.
376 get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
377 get_ephemeral_name.return_value = factory.make_name("ephemeral")
378 options = {
379 "kernel_params":
380- make_kernel_parameters(purpose="commissioning"),
381+ make_kernel_parameters(purpose=self.purpose),
382 }
383 output = render_pxe_config(**options)
384 config = parse_pxe_config(output)
385
386=== modified file 'src/provisioningserver/pxe/tests/test_install_image.py'
387--- src/provisioningserver/pxe/tests/test_install_image.py 2012-08-31 15:41:18 +0000
388+++ src/provisioningserver/pxe/tests/test_install_image.py 2013-05-11 18:43:26 +0000
389@@ -209,3 +209,38 @@
390 self.assertEqual(
391 "rw-r--r--",
392 target_dir.child("image").getPermissions().shorthand())
393+
394+ def test_install_dir_moves_dir_into_place_with_symlink(self):
395+ download_image = os.path.join(self.make_dir(), 'download-image')
396+ published_image = os.path.join(self.make_dir(), 'published-image')
397+ base_path = os.path.dirname(published_image)
398+ symlink_dest = 'xinstall'
399+ contents = factory.getRandomString()
400+ os.makedirs(download_image)
401+ sample_file = factory.make_file(download_image, contents=contents)
402+ install_dir(download_image, published_image, symlink_dest)
403+ self.assertThat(
404+ os.path.join(published_image, os.path.basename(sample_file)),
405+ FileContains(contents))
406+ self.assertThat(
407+ os.path.join(base_path, symlink_dest, os.path.basename(sample_file)),
408+ FileContains(contents))
409+
410+ def test_install_dir_replaces_existing_dir_with_symlink(self):
411+ download_image = os.path.join(self.make_dir(), 'download-image')
412+ published_image = os.path.join(self.make_dir(), 'published-image')
413+ base_path = os.path.dirname(published_image)
414+ symlink_dest = 'xinstall'
415+ os.makedirs(download_image)
416+ sample_file = factory.make_file(download_image)
417+ os.makedirs(published_image)
418+ os.symlink(published_image, os.path.join(base_path, symlink_dest))
419+ obsolete_file = factory.make_file(published_image)
420+ install_dir(download_image, published_image, symlink_dest)
421+ self.assertThat(
422+ os.path.join(published_image, os.path.basename(sample_file)),
423+ FileExists())
424+ self.assertThat(obsolete_file, Not(FileExists()))
425+ self.assertThat(
426+ os.path.join(base_path, symlink_dest, os.path.basename(sample_file)),
427+ FileExists())
428
429=== modified file 'src/provisioningserver/tests/test_kernel_opts.py'
430--- src/provisioningserver/tests/test_kernel_opts.py 2013-02-01 01:07:25 +0000
431+++ src/provisioningserver/tests/test_kernel_opts.py 2013-05-11 18:43:26 +0000
432@@ -118,6 +118,22 @@
433 "netcfg/choose_interface=auto",
434 compose_kernel_command_line(params))
435
436+ def test_xinstall_compose_kernel_command_line_inc_purpose_opts(self):
437+ # The result of compose_kernel_command_line includes the purpose
438+ # options for a non "xinstall" node.
439+ get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
440+ get_ephemeral_name.return_value = "RELEASE-ARCH"
441+ params = make_kernel_parameters(purpose="xinstall")
442+ cmdline = compose_kernel_command_line(params)
443+ self.assertThat(
444+ cmdline,
445+ ContainsAll([
446+ "ds=nocloud-net",
447+ "root=/dev/disk/by-path/ip-",
448+ "iscsi_initiator=",
449+ "overlayroot=tmpfs",
450+ "ip=::::%s:BOOTIF" % params.hostname]))
451+
452 def test_commissioning_compose_kernel_command_line_inc_purpose_opts(self):
453 # The result of compose_kernel_command_line includes the purpose
454 # options for a non "commissioning" node.
455@@ -147,8 +163,8 @@
456 self.assertNotIn(cmdline, "None")
457
458 def test_compose_kernel_command_line_inc_common_opts(self):
459- # Test that some kernel arguments appear on both commissioning
460- # and install command lines.
461+ # Test that some kernel arguments appear on commissioning, install
462+ # and xinstall command lines.
463 get_ephemeral_name = self.patch(kernel_opts, "get_ephemeral_name")
464 get_ephemeral_name.return_value = "RELEASE-ARCH"
465 expected = ["nomodeset"]
466@@ -159,6 +175,11 @@
467 self.assertThat(cmdline, ContainsAll(expected))
468
469 params = make_kernel_parameters(
470+ purpose="xinstall", arch="i386")
471+ cmdline = compose_kernel_command_line(params)
472+ self.assertThat(cmdline, ContainsAll(expected))
473+
474+ params = make_kernel_parameters(
475 purpose="install", arch="i386")
476 cmdline = compose_kernel_command_line(params)
477 self.assertThat(cmdline, ContainsAll(expected))
478@@ -182,6 +203,23 @@
479 factory.make_file(
480 ephemeral_dir, name='info', contents=ephemeral_info)
481
482+ def test_compose_kernel_command_line_inc_purpose_opts_xinstall_node(self):
483+ # The result of compose_kernel_command_line includes the purpose
484+ # options for a "xinstall" node.
485+ ephemeral_name = factory.make_name("ephemeral")
486+ params = make_kernel_parameters(purpose="xinstall")
487+ self.create_ephemeral_info(
488+ ephemeral_name, params.arch, params.release)
489+ self.assertThat(
490+ compose_kernel_command_line(params),
491+ ContainsAll([
492+ "ds=nocloud-net",
493+ "iscsi_target_name=%s:%s" % (
494+ ISCSI_TARGET_NAME_PREFIX, ephemeral_name),
495+ "iscsi_target_port=3260",
496+ "iscsi_target_ip=%s" % params.fs_host,
497+ ]))
498+
499 def test_compose_kernel_command_line_inc_purpose_opts_comm_node(self):
500 # The result of compose_kernel_command_line includes the purpose
501 # options for a "commissioning" node.