Merge lp:~nuclearbob/utah/bug1087973 into lp:utah

[Max Brustkern]

This branch fixes a couple of bugs preventing kernel config options from being passed in when provisioning physical hardware. It also moves the setup of the boottimeout variable out of CustomInstallMixin and standardizes use of that variable in the Machine class. I have tested the changes in magners-orchestra and verified that the kernel options are correctly passed to cobbler. I am waiting for the install to finish to verify nothing has gone wrong.

[Joe Talbott (joetalbott)]

Looks good to me.

review: Approve

[Javier Collado (javier.collado)]

I don't like much the loops to set keyword arguments. I know that's not
something changed in the merge request, but let me add it as a comment since
I'm seeing them in the diff.

I think that `vargs` and `dict.update` as in this example:

for arg in ['boot', 'image', 'preseed', 'rewrite']:
    value = vars(args)[arg]
    if value is not None:
        kw.update([(arg, value)])

Should be replaced with `getattr` and dict assignment as follows:

for arg in ['boot', 'image', 'preseed', 'rewrite']:
    value = getattr(args, arg)
    if value is not None:
        kw[arg] = value

[Javier Collado (javier.collado)]

It looks like `Machine` objects aren't getting a customized `timeout`
value at initialization time.

Is there any advantage in using this:
self.boottimeout = config.boottimeout

instead of `config.bootimeout` directly?

review: Needs Information

[Javier Collado (javier.collado)]

Aside from the comments above, the code looks nice, so I'm approving as we've
been doing for Critical bugs and we can fix any cosmetic issue later.

review: Approve

[Max Brustkern (nuclearbob)]

When libvirtvm is using qemu, it raises all timeouts because qemu is super duper slow. At this point it may make more sense to either set a different config for the qemu jobs or just raise the timeout high enough for them to all work. Then, we could just use the config file version. Let me know what you think.

[lp:~nuclearbob/utah/bug1087973]

781. By Max Brustkern on 2012-12-10

Changed argument loops per Javier's comment

[Max Brustkern (nuclearbob)]

Javier's comment on the argument loops should be addressed now. I'd like to determine whether to also change the boottimeout setup to just use the config and avoid the internal multiplication done for qemu. I'd like to get all these merged to make a new stable version, so it would be good to take care of anything that's bothering us first.

review: Needs Resubmitting

[Javier Collado (javier.collado)]

Ok, I've seen the multiplication factor for QEMU:
self.boottimeout *= 4

This isn't bothering me because I'm not using it. However, I'm not in favor of
changing configuration values this way. I think a good alternative would be to
write warning messages if a configuration value isn't good enough to let the
user know he should consider to change it.

Anyway, I'm going to merge these changes to have them committed for the boot
args bug and we can think about the boot timeout in a different merge request.

=== modified file 'examples/run_test_bamboo_feeder.py'
--- examples/run_test_bamboo_feeder.py	2012-12-08 02:10:12 +0000
+++ examples/run_test_bamboo_feeder.py	2012-12-10 19:20:24 +0000
@@ -57,6 +57,8 @@
                         help='Image/ISO file to use for installation')
     parser.add_argument('-p', '--preseed', type=url_argument,
                         help='Preseed file to use for installation')
+    parser.add_argument('-b', '--boot',
+                        help='Boot arguments for initial installation')
     parser.add_argument('-n', '--no-destroy', action='store_true',
                         help='Preserve VM after tests have run')
     parser.add_argument('-d', '--debug', action='store_true',
@@ -85,10 +87,10 @@
             db=os.path.join('~', '.utah-bamboofeeder-inventory'),
             lockfile=os.path.join('~', '.utah-bamboofeeder-lock'))
         kw = {}
-        for arg in ['image', 'preseed']:
-            value = vars(args)[arg]
+        for arg in ['boot', 'image', 'preseed']:
+            value = getattr(args, arg)
             if value is not None:
-                kw.update([(arg, value)])
+                kw[arg] = value
         machine = inventory.request(machinetype=BambooFeederMachine,
                                     clean=(not args.no_destroy),
                                     debug=args.debug, new=True,

=== modified file 'examples/run_test_cobbler.py'
--- examples/run_test_cobbler.py	2012-12-08 02:10:12 +0000
+++ examples/run_test_cobbler.py	2012-12-10 19:20:24 +0000
@@ -44,6 +44,8 @@
                         help='Image/ISO file to use for installation')
     parser.add_argument('-p', '--preseed', type=url_argument,
                         help='Preseed file to use for installation')
+    parser.add_argument('-b', '--boot',
+                        help='Boot arguments for initial installation')
     parser.add_argument('-s', '--series', metavar='SERIES',
                         choices=config.serieschoices,
                         help='Series to use for VM creation (%(choices)s)')
@@ -97,10 +99,10 @@
     try:
         inventory = ManualCobblerSQLiteInventory()
         kw = {}
-        for arg in ['image', 'preseed', 'rewrite']:
-            value = vars(args)[arg]
+        for arg in ['boot', 'image', 'preseed', 'rewrite']:
+            value = getattr(args, arg)
             if value is not None:
-                kw.update([(arg, value)])
+                kw[arg] = value
         machine = inventory.request(clean=(not args.no_destroy),
                                     debug=args.debug, dlpercentincrement=10,
                                     name=args.name, new=True, **kw)

=== modified file 'examples/run_test_vm.py'
--- examples/run_test_vm.py	2012-12-08 02:10:12 +0000
+++ examples/run_test_vm.py	2012-12-10 19:20:24 +0000
@@ -82,9 +82,9 @@
         inventory = TinySQLiteInventory()
         kw = {}
         for arg in ['arch', 'series']:
-            value = vars(args)[arg]
+            value = getattr(args, arg)
             if value is not None:
-                kw.update([(arg, value)])
+                kw[arg] = value
         if args.type is not None:
             kw.update([('installtype', args.type)])
         machine = inventory.request(clean=(not args.no_destroy),

=== modified file 'examples/run_utah_tests.py'
--- examples/run_utah_tests.py	2012-12-08 02:10:12 +0000
+++ examples/run_utah_tests.py	2012-12-10 19:20:24 +0000
@@ -22,7 +22,6 @@
 from utah.url import url_argument
 from utah.group import check_user_group, print_group_error_message
 from utah.timeout import timeout, UTAHTimeout
-from utah import config
 from run_install_test import run_install_test
 
 

=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2012-12-08 02:10:12 +0000
+++ utah/provisioning/baremetal/cobbler.py	2012-12-10 19:20:24 +0000
@@ -189,7 +189,7 @@
         self.restart()
 
         self.logger.info('Waiting for installation to begin')
-        time.sleep(config.boottimeout)
+        time.sleep(self.boottimeout)
         self._cobble(['system', 'edit', '--name=' + self.name,
                       '--netboot-enabled=N'])
 

=== modified file 'utah/provisioning/provisioning.py'
--- utah/provisioning/provisioning.py	2012-12-06 16:30:34 +0000
+++ utah/provisioning/provisioning.py	2012-12-10 19:20:24 +0000
@@ -121,6 +121,8 @@
         self.prefix = prefix
         self.template = template
 
+        self.boottimeout = config.boottimeout
+
         if clean is None:
             self.clean = True
         else:
@@ -330,10 +332,12 @@
             raise UTAHProvisioningException(err, retry=True)
 
     def pingpoll(self,
-                 timeout=config.boottimeout,
+                 timeout=None,
                  checktimeout=config.checktimeout,
                  logmethod=None):
         """Run pingcheck over and over until timeout expires."""
+        if timeout is None:
+            timeout = self.boottimeout
         if logmethod is None:
             logmethod = self.logger.debug
         utah.timeout.timeout(timeout, retry, self.pingcheck, checktimeout,
@@ -875,11 +879,13 @@
         except socket.error as err:
             raise UTAHProvisioningException(str(err), retry=True)
 
-    def sshpoll(self, timeout=config.boottimeout,
+    def sshpoll(self, timeout=None,
                 checktimeout=config.checktimeout, logmethod=None):
         """
         Run sshcheck over and over until timeout expires.
         """
+        if timeout is None:
+            timeout = self.boottimeout
         if logmethod is None:
             logmethod = self.logger.debug
         utah.timeout.timeout(timeout, retry, self.sshcheck, checktimeout,
@@ -1207,13 +1213,13 @@
         """
         Add the needed options to the command line for an automatic install.
         """
-        if boot is None:
-            self.cmdline = self.boot
-        if boot is None:
-            self.cmdline = ''
-        else:
-            self.cmdline = boot
-        self.boottimeout = config.boottimeout
+        # TODO: update libvirtvm to work like the hardware provisioners
+        # or vice versa
+        if boot is None:
+            boot = self.boot
+        if boot is None:
+            boot = ''
+        self.cmdline = boot
         # TODO: Refactor this into lists like BambooFeederMachine
         if self.rewrite == 'all':
             self.logger.info('Adding needed command line options')