Merge lp:~javier.collado/utah/bug1050976 into lp:utah

[Javier Collado]

This branch moves the "apt-get install" commands in the late command to etc/rc.local

This is to workaround the networking problems when executing the late command since
"apt-get install" and "apt-install" don't seem to be working.

In addition to this, this branch:
- removes the hardcoded ssh timeout of 3 seconds (config.checktimeout is used instead)
- changes bootimeout and checktimeout values in config to some values that work for me
- removes the check for the utah client return code

Regarding the update to the timeout values in the configuration, I think that boottimeout
is being used for multiple things at the same time. One problem I had with the old values
once the hardcoded ssh timeout was removed, was that if checktimeout was greater than
boottimeout, no more retry attempts were made. Hence, in the new values boottimeout (90)
is greater than checktimeout (15). I'm not sure if this might have an effect on some other
use cases, but I've tested this with 64bit desktop precise and quantal images and worked fine
(with the old values I got ssh timeout).

Regarding the utah client return code. The server was not writing the yaml file because the client
returns a failure code when some test case failed. To really fix this, it should be clear
which codes returns the client on a test case failure and on an internal failure. In the meantime
is preferable to write whatever the client prints to stdout to a yaml file.

[Javier Collado (javier.collado)]

One thing that concerns me is that I think that the "apt-get install" commands in the late command install
the packages from the image itself (is that correct?), but after the move to /etc/rc.local, they need
a network connection to work fine, so this change will break the scenarios without network connection.

[Max Brustkern (nuclearbob)]

That's exactly what I was going to say. I almost think it might be good to have the option to do one or the other.

I definitely agree about a need to standardize client return codes and update the server accordingly. I still don't like that we return a failure code from the server if the client fails a tests with no errors, though, so we may have dissenting opinions on that.

Also, if you needed to update your timeouts to get things working, maybe we should make those the new defaults? I set things kind of aggressively low when first developing them because I wanted to see if things worked quickly, but for production use it's probably better to err on the high side.

[lp:~javier.collado/utah/bug1050976]

708. By Javier Collado on 2012-10-10

Added sshserverinstall configuration variable to decide when to install the ssh server

Valid values are 'before reboot' and 'after reboot'

[Javier Collado (javier.collado)]

@Max

I've added a new configuration variable named sshserverinstall that can have two values: 'before reboot' and 'after reboot'. Based on this configuration variable value, the command to execute in the chroot is selected, that is, when 'before reboot' is set the apt-get install is executed directly and when 'after reboot' is set, the apt-get install is added to /etc/rc.local

The variable name isn't really very good because gdebi package is installed aside from openssh-server. If you've got a better name, please let me know. By the way, I don't like the configuration variables being words together without underscores or capital letters to separate them, but I decided to be consistent with the other ones.

With regard to the timeouts, they have been already updated in config.py.

review: Needs Resubmitting

[Max Brustkern (nuclearbob)]

I don't think there was any particular design decision making process in place to get the existing configuration variable names, so if we want to overhaul the rest of them to have underscores or something, that seems reasonable to me. I prefer configuration_option to ConfigurationOption myself, but I'm okay with either. As I'm thinking about it, I prefer functionName to function_name, so maybe my preferences aren't very consistent.

With regard to the option, I'm not crazy about the name and the specific values for the option. I don't think there's anything wrong with them, and they are solving a problem, so I don't want to slow down getting this merged, but I've been thinking more about it, and I think I'd almost favor generalizing it a little further. What I'm thinking is two lists, maybe something like install_packages and postinstall_packages, and the install_packages get installed by the latecommand and the postinstall_packages get installed by rc.local. It's definitely more work to implement, and I'm not sure there are important usecases for that flexibility, but when I think about the overall design, I like that idea better than what we have now.

That said, I don't want to derail this. It's a good solution to the problem we have right now, and if we do need more flexibility, we can always add it later. If we merge this, we can have quantal working, which is important, so I'm in favor of doing that.

review: Approve

[lp:~javier.collado/utah/bug1050976]

709. By Javier Collado on 2012-10-11

Replaced sshserverinstall with installpackages and postinstallpackages

710. By Javier Collado on 2012-10-11

Removed postinstallpackages from configuration

Since for now what we really want is to install a list of packages,
installpackages is enough. What the latecommand tries to do now is just install
the given packages both before and after reboot (it doesn't matter which method
succeeds).

[Javier Collado (javier.collado)]

@Max

You're right, the implementation was ugly and shouldn't be committed to dev.

Following your advice, I've removed that sshserverinstall configuration added another
one called installpackages were a list of package names is expected in the configuration
file.

When the late command string is generated the apt-get install commands are attempted
both before reboot in the chroot and after reboot in the rc.local script.

I've tested that in precise and quantal images and looking into /var/log/utah-install
in the VM I've seen as expected that in precise the package installation succeeds in
the chroot and in quantal it does in the rc.local script.

Note that even in precise, the apt-get install command runs in rc.local, but when the packages
are already installed the commands quickly exits because the package is already installed.

review: Needs Resubmitting

[Max Brustkern (nuclearbob)]

That's a great idea. It'll work as long as we have a working network connection ever, and there's minimal overhead.

review: Approve

=== modified file 'utah/config.py'
--- utah/config.py	2012-10-08 13:58:28 +0000
+++ utah/config.py	2012-10-11 11:03:23 +0000
@@ -27,14 +27,15 @@
 
 DEFAULTS = dict(
     arch='i386',
-    boottimeout=30,
-    checktimeout=180,
+    boottimeout=90,
+    checktimeout=15,
     consoleloglevel=logging.WARNING,
     debug=False,
     fileloglevel=logging.INFO,
     group='utah',
     image=None,
     initrd=None,
+    installpackages=['openssh-server', 'gdebi'],
     installtimeout=3600,
     installtype='desktop',
     kernel=None,

=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-10-09 14:01:08 +0000
+++ utah/provisioning/provisioning.py	2012-10-11 11:03:23 +0000
@@ -842,19 +842,23 @@
         question = preseed['preseed/late_command']
         if self.installtype == 'desktop':
             self.logger.info('Changing d-i latecommand '
-                    + 'to ubiquity success_command '
-                    + 'and prepending ubiquity lines')
+                    'to ubiquity success_command '
+                    'and prepending ubiquity lines')
             question.owner = 'ubiquity'
             question.qname = 'ubiquity/success_command'
-            question.value.prepend(
-                    'chroot /target sh -c \''        # chroot start
-                    'export LOG_FILE=/var/log/utah-install; '
-                    'apt-get install -y --force-yes openssh-server '
-                    '>>$LOG_FILE 2>&1 ; '
-                    'apt-get install -y --force-yes gdebi-core '
-                    '>>$LOG_FILE 2>&1 ; '
-                    '\'; '                           # chroot end
+            log_file = '/var/log/utah-install'
+
+            install_commands = (
+                    ' '.join(['apt-get install -y --force-yes {} '
+                              '>>{} 2>&1;'.format(pkg_name, log_file)
+                              for pkg_name in config.installpackages]))
+            postinstall_commands = (
+                    'sed -i -e "/^exit 0$/i{}" /etc/rc.local;'
+                    .format(install_commands)
                     )
+            chroot_command = ' '.join([install_commands, postinstall_commands])
+            question.value.prepend('chroot /target sh -c \'{}\'; '
+                                   .format(chroot_command))
             question.prepend("ubiquity ubiquity/summary note")
             question.prepend("ubiquity ubiquity/reboot boolean true")
         else:

=== modified file 'utah/provisioning/vm/libvirtvm.py'
--- utah/provisioning/vm/libvirtvm.py	2012-10-08 13:58:28 +0000
+++ utah/provisioning/vm/libvirtvm.py	2012-10-11 11:03:23 +0000
@@ -697,7 +697,7 @@
         self.vm.create()
         self.logger.info('Waiting ' + str(self.boottimeout) +
                          ' seconds to allow machine to boot')
-        self.sshpoll(self.boottimeout, 3)
+        self.sshpoll(self.boottimeout)
         self.active = True
 
     def destroy(self, *args, **kw):

=== modified file 'utah/run.py'
--- utah/run.py	2012-08-29 19:32:22 +0000
+++ utah/run.py	2012-10-11 11:03:23 +0000
@@ -41,11 +41,8 @@
         utah_command = 'utah' + extraopts + options
         try:
             returncode, stdout, stderr = machine.run(utah_command)
-            if returncode != 0:
-                machine.logger.error('utah failed with return code: {}\n'
-                                     .format(returncode))
-                exitstatus = 1
-                break
+            # TODO: Decide which returncode means utah client failure
+            # and which one means test case failure
         except UTAHException as error:
             machine.logger.error('Failed to run test: ' + str(error))
             exitstatus = 1