Merge lp:~doanac/utah/bug1158743 into lp:utah/trunk

[Andy Doan]

This should add the logging Javier was suggesting to understand what processes are actually using a machine.

Changed to proper branch

[Javier Collado (javier.collado)]

I see that you fixed the bug in a branch based on trunk and created the merge
for dev (which actually reveals an entry in debian/changelog that isn't in
dev). Is this the expected way to work with bugs?

[Javier Collado (javier.collado)]

The use of a static private method suggests that `_in_use` is a utility method,
so I believe it should be moved to the `process` module which is expected to
have that kind of methods.

[Andy Doan (doanac)]

On 03/25/2013 04:16 AM, Javier Collado wrote:
> I see that you fixed the bug in a branch based on trunk and created the merge
> for dev (which actually reveals an entry in debian/changelog that isn't in
> dev). Is this the expected way to work with bugs?
>
Nope - that was a mechanical error. I'll correct

[lp:~doanac/utah/bug1158743]

270. By Andy Doan on 2013-03-25

move pid _in_use to process.py

As per Javier's review comment this moves the function to become a
generic helper. I also added some unit-testing logic for it

[Andy Doan (doanac)]

updated as per Javier's suggestion

[Javier Collado (javier.collado)]

Thanks for the update (specially for the test cases).

I've got a couple of commments:

- Exceptions

When getting the process object, TypeError and ValueError exceptions are
caught:
except (psutil.error.NoSuchProcess, TypeError, ValueError):

However, I think they shouldn't be caught since I'd like to get an exception
when I don't pass the right parameter type as in:
pid_in_use('string')

Otherwise, I misuse is not noticed by the end user.

- Documentation

I've seen you've made the effor to document everything. However the
documentation strings aren't pep257 (http://www.python.org/dev/peps/pep-0257/)
compliant and they don't follow the sphinx markup. If you could fix those
problems as well (just on the new code), that would be great.

To check documentation strings, I use the following tool and works quite well:
https://pypi.python.org/pypi/pep257/

With regard sphinx markup, please have a look at:
http://sphinx-doc.org/domains.html#info-field-lists

In particular, the fields that we've been using so far are:
:param <param>: to describe a parameter
:type <type>: to annotate the parameter type
:raises <exception>: to add exceptions that might be raised
:returns: to describe what is being returned
:rtype: to annotate the type of the return value

[Andy Doan (doanac)]

On 03/26/2013 12:35 PM, Javier Collado wrote:
> - Exceptions
>
> When getting the process object, TypeError and ValueError exceptions are
> caught:
> except (psutil.error.NoSuchProcess, TypeError, ValueError):
>
> However, I think they shouldn't be caught since I'd like to get an exception
> when I don't pass the right parameter type as in:
> pid_in_use('string')
>
> Otherwise, I misuse is not noticed by the end user.

Is this going to cause a possible regression that we need to test for?

@max - I think you suggested this, but i don't recall why

[Max Brustkern (nuclearbob)]

I think originally we caught TypeError and ValueError to allow is flexibility in the transition from a database without a pid column to a database with a pid column. As long as we do sufficient testing of an actual Inventory and verify things work, leaving those out should be fine.

[Max Brustkern (nuclearbob)]

I've tested this with an actual physical machine inventory on magners-orchestra, and I'm satisfied that it works correctly. Unfortunately, we're still not printing info to console by default, so the message won't be seen by some jobs. That's a different fix, however, so I'm in favor of merging this.

review: Approve

[Max Brustkern (nuclearbob)]

Can you add a debian changelog entry for this so it's sourced properly? I use
dch "Made a totally sweet change"
for that.

[lp:~doanac/utah/bug1158743]

271. By Andy Doan on 2013-04-10

fix pep257 issues

272. By Andy Doan on 2013-04-10

add changelog entry

[Max Brustkern (nuclearbob)]

I removed TypeError and ValueError from the list of exceptions caught, and things still test out fine. I think those were probably mainly useful during the transition period when the database might or might not have a pid column.

[Max Brustkern (nuclearbob)]

This looks good to me.

review: Approve

[Javier Collado (javier.collado)]

My comment has been addressed in:
https://code.launchpad.net/~doanac/utah/bug11588743-part2/+merge/160498

Hence, I think these changes are fine.

review: Approve

[Max Brustkern (nuclearbob)]

This appears to already be in dev, but the target of this proposal is trunk. Is this something we want to release as a bugfix, or can we just wait until the next release?

[Andy Doan (doanac)]

hmm - i think i originally was going to target this as a fix, but then we started using dev for everything. So i think we are good here.

[Javier Collado (javier.collado)]

Rejecting the merge request as per the previous comment.

Unmerged revisions

272. By Andy Doan on 2013-04-10

add changelog entry

271. By Andy Doan on 2013-04-10

fix pep257 issues

270. By Andy Doan on 2013-03-25

move pid _in_use to process.py

As per Javier's review comment this moves the function to become a
generic helper. I also added some unit-testing logic for it

269. By Andy Doan on 2013-03-22

add logging for unavailable machines

this should help us understand who is using a machine in the event
one isn't available.

=== modified file 'debian/changelog'
--- debian/changelog	2013-03-27 15:33:54 +0000
+++ debian/changelog	2013-04-10 15:39:35 +0000
@@ -28,6 +28,9 @@
   * Overhauled exception handling based on code review
   * Made all shell scripts POSIX-compliant
 
+  [ Andy Doan ]
+  * Log process that has machine reserved. (LP: #1158743)
+
  -- Max Brustkern <[e-mail]>  Fri, 15 Mar 2013 13:55:24 -0400
 
 utah (0.8ubuntu1) quantal; urgency=low
 
=== added file 'tests/test_process.py'
--- tests/test_process.py	1970-01-01 00:00:00 +0000
+++ tests/test_process.py	2013-04-10 15:39:35 +0000
@@ -0,0 +1,44 @@
+# Ubuntu Testing Automation Harness
+# Copyright 2013 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Unit tests for the utah.process module."""
+
+import os
+import unittest
+
+from utah.process import pid_in_use
+
+
+class TestProcess(unittest.TestCase):
+
+    """Tests the functions of the utah.process module."""
+
+    def test_is_running(self):
+        """Ensure simple test of pid_in_use works for a pid running."""
+        pid = os.getpid()
+        self.assertIsNotNone(pid_in_use(pid))
+        self.assertIsNotNone(pid_in_use(pid, ['test_']))
+        # make sure we don't return when the command line doesn't match
+        self.assertIsNone(pid_in_use(pid, ['blah']))
+
+    def test_is_not_running(self):
+        """Ensure simple test of pid_in_use works for a pid not running."""
+        pid = os.fork()
+        if pid == 0:
+            os.execv('/bin/true', ['/bin/true'])
+        os.waitpid(pid, 0)
+        # the child should be dead so this will return None
+        self.assertIsNone(pid_in_use(pid))
+        self.assertIsNone(pid_in_use(pid, ['true']))
 
=== modified file 'utah/process.py'
--- utah/process.py	2012-12-08 02:10:12 +0000
+++ utah/process.py	2013-04-10 15:39:35 +0000
@@ -19,6 +19,31 @@
 import psutil
 
 
+def pid_in_use(pid, arg_patterns=None):
+    """Test if the given pid is running.
+
+    If arg_patterns is provided then the pid's command line will be compared
+    to make sure it matches an expected value.
+
+    :returns: proc object if the pid is active and optionally matches one of
+    the arg_patterns, otherwise None
+    :rtype: psutil.Process
+
+    """
+    try:
+        proc = psutil.Process(pid)
+    except (psutil.error.NoSuchProcess, TypeError, ValueError):
+        return None
+
+    cmdline = ' '.join(proc.cmdline)
+    if arg_patterns:
+        for p in arg_patterns:
+            if p in cmdline:
+                return proc
+        return None
+    return proc
+
+
 class ProcessChecker(object):
     """
     Check all running process looking for a given pattern
 
=== modified file 'utah/provisioning/baremetal/inventory.py'
--- utah/provisioning/baremetal/inventory.py	2013-01-22 14:13:17 +0000
+++ utah/provisioning/baremetal/inventory.py	2013-04-10 15:39:35 +0000
@@ -17,8 +17,10 @@
 Provide inventory functions specific to bare metal deployments.
 """
 
+import logging
 import os
-import psutil
+
+from utah.process import pid_in_use
 from utah.provisioning.inventory.exceptions import \
     UTAHProvisioningInventoryException
 from utah.provisioning.inventory.sqlite import SQLiteInventory
@@ -65,15 +67,11 @@
             for minfo in result:
                 machineinfo = dict(minfo)
                 pid = machineinfo['pid']
-                try:
-                    if not (psutil.pid_exists(pid) and ('utah' in
-                            ' '.join(psutil.Process(pid).cmdline)
-                            or 'run_test' in
-                            ' '.join(psutil.Process(pid).cmdline))):
-                        return self._take(machineinfo, machinetype,
-                                          *args, **kw)
-                except ValueError:
-                    continue
+                proc = pid_in_use(pid, ['utah', 'run_test'])
+                if not proc:
+                    return self._take(machineinfo, machinetype, *args, **kw)
+                else:
+                    logging.info('machine in use by: %s', proc.cmdline)
 
         raise UTAHProvisioningInventoryException(
             'All machines meeting criteria are currently unavailable')