Code review comment for lp://staging/~gmdduf/lava-dispatcher/gauss-support

Hello Bernard,

Thanks for this submission. I have just reviewed it and found a few
problems, which have to be addressed before we merge this code.

 review needs-fixing

> === modified file 'lava_dispatcher/client/base.py'
> --- lava_dispatcher/client/base.py 2013-05-15 23:15:31 +0000
> +++ lava_dispatcher/client/base.py 2013-05-16 07:03:30 +0000
> @@ -426,7 +426,7 @@
> attempts = 0
> in_linaro_image = False
> while (attempts < boot_attempts) and (not in_linaro_image):
> - logging.info("Booting the test image. Attempt: %d" % attempts + 1)
> + logging.info("Booting the test image. Attempt: %d" % int(attempts + 1))
> try:
> self._boot_linaro_image()
> except (OperationFailed, pexpect.TIMEOUT) as e:

This is not needed, as attempts is already an integer.

> @@ -435,8 +435,8 @@
> attempts += 1
> continue
>
> - timeout = self.config.boot_linaro_timeout
> - TESTER_PS1_PATTERN = self.target_device.deployment_data['TESTER_PS1_PATTERN']
> + TESTER_PS1_PATTERN = self.target_device.deployment_data['TESTER_PS1_PATTERN']
> + timeout = self.config.boot_linaro_timeout
> try:
> wait_for_prompt(self.proc, TESTER_PS1_PATTERN, timeout=timeout)
> except (pexpect.TIMEOUT) as e:

This change does not apply anymore, please undo it.

> === modified file 'lava_dispatcher/config.py'
> --- lava_dispatcher/config.py 2013-05-16 01:53:21 +0000
> +++ lava_dispatcher/config.py 2013-05-16 07:03:30 +0000
> @@ -192,7 +192,7 @@
> if os.path.exists(path):
> config_files.append(path)
> if not config_files:
> - raise Exception("no config files named %r found" % (name + ".conf"))
> + raise Exception("no config files named %r found on %s" % (name + ".conf", config_dir))
> config_files.reverse()
> logging.debug("About to read %s", str(config_files))
> for path in config_files:

The dispatcher will look for config file in several directories, not
only in `config_dir`. So saying that the file was not found in
`config_dir` is not correct.

> === added file 'lava_dispatcher/device/gauss.py'

gauss.py is an exact copy of highbank.py

This is good because it means you could make it work without any change
there.

But it is bad from a maintaince point of view because we don't want to
maintain two copies of the same code.

So, I created a new merge proposal generalizing highbank.py to
ipmi_pxe.py, which basically only changes a couple of names:
https://code.launchpad.net/~terceiro/lava-dispatcher/generalize-ipmi-pxe/+merge/164781

This code will be merged in trunk in a few days, after one of the other
LAVA team members reviews it, and you should be able to use it then.

> === modified file 'lava_dispatcher/ipmi.py'
> --- lava_dispatcher/ipmi.py 2013-04-12 10:01:48 +0000
> +++ lava_dispatcher/ipmi.py 2013-05-16 07:03:30 +0000
> @@ -53,7 +53,10 @@
> self.__ipmi("chassis power on")
>
> def reset(self):
> - self.__ipmi("chassis power reset")
> + if self.host.find('gauss') == -1:
> + self.__ipmi("chassis power reset")
> + else :
> + self.__ipmi("chassis power cycle")

this block has a broken indentation.

More important, you cannot assume that the device host name is
associated with a given device type.

Is there a reason why `power cycle` does not work for you?

OTOH reading the ipmitool(1) manpage¹, maybe we actually want `reset` in
there for all cases?

¹ "cycle

    Provides a power off interval of at least 1 second. No action
    should occur if chassis power is in S4/S5 state, but it is
    recommended to check power state first and only issue a power cycle
    command if the system power is on or in lower sleep state than
    S4/S5."

--
Antonio Terceiro
Software Engineer - Linaro
http://www.linaro.org