Merge lp:~bjornt/landscape-client/process-state-key-error into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Handle new process states in when getting process information.

The bug report was about "t (tracing stop)", but a bunch of other states
can occur as well that we don't handle. I've change the code to use the
one-character representation of the process state that the kernel uses,
instead of using our own mapping. This ensure that future state
additions will be handled as well. As a result, we now represent the
"tracing stop" state as "t" instead of "I", but as far as I can see, the
only state we're explicitly checking for in our code is Z, so that
change shouldn't cause a problem.

I put in a special-case for "T (tracing stop)", since in Lucid you can
have both "T (stopped)" and "T (tracing stop)". I made it so that the
latter is represented by "t", since that's the way it is in releases
newer than Lucid.

[Alberto Donato (ack)]

Nice fix, +1!

review: Approve

[Alberto Donato (ack)]

#1:
I have a test failure in landscape.monitor.tests.test_activeprocessinfo.ActiveProcessInfoTest.test_read_sample_data, related to the "I" to "t" change.

[lp:~bjornt/landscape-client/process-state-key-error]

591. By Björn Tillenius on 2012-10-11

Fix test failures.

[Björn Tillenius (bjornt)]

On Wed, Oct 10, 2012 at 03:20:07PM -0000, Alberto Donato wrote:
> #1:
> I have a test failure in
> landscape.monitor.tests.test_activeprocessinfo.ActiveProcessInfoTest.test_read_sample_data,
> related to the "I" to "t" change.

Indeed. There were two other similar test failures as well. I've fixed
the tests to expect "t" now.

--
Björn Tillenius | https://launchpad.net/~bjornt

[Jerry Seutter (jseutter)]

+1, looks good!

review: Approve

=== modified file 'landscape/lib/process.py'
--- landscape/lib/process.py	2011-07-28 12:54:34 +0000
+++ landscape/lib/process.py	2012-10-11 08:30:30 +0000
@@ -10,15 +10,6 @@
 from landscape.monitor.computeruptime import BootTimes, get_uptime
 
 
-STATES = {"R (running)": "R",
-          "D (disk sleep)": "D",
-          "S (sleeping)": "S",
-          "T (stopped)": "T",
-          "T (tracing stop)": "I",
-          "X (dead)": "X",
-          "Z (zombie)": "Z"}
-
-
 class ProcessInformation(object):
     """
     @param proc_dir: The directory to use for process information.
@@ -66,8 +57,8 @@
             file = open(os.path.join(process_dir, "cmdline"), "r")
             try:
                 # cmdline is a \0 separated list of strings
-                # We take the first, and then strip off the path, leaving us with
-                # the basename.
+                # We take the first, and then strip off the path, leaving
+                # us with the basename.
                 cmd_line = file.readline()
                 cmd_line_name = os.path.basename(cmd_line.split("\0")[0])
             finally:
@@ -82,7 +73,12 @@
                                                 parts[1].strip())
                     elif parts[0] == "State":
                         state = parts[1].strip()
-                        process_info["state"] = STATES[state]
+                        # In Lucid, capital T is used for both tracing stop
+                        # and stopped. Starting with Natty, lowercase t is
+                        # used for tracing stop.
+                        if state == "T (tracing stop)":
+                            state = state.lower()
+                        process_info["state"] = state[0]
                     elif parts[0] == "Uid":
                         value_parts = parts[1].split()
                         process_info["uid"] = int(value_parts[0])
@@ -99,14 +95,14 @@
             file = open(os.path.join(process_dir, "stat"), "r")
             try:
                 # These variable names are lifted directly from proc(5)
-                # utime: The number of jiffies that this process has been scheduled in
-                # user mode.
-                # stime: The number of jiffies that this process has been scheduled in
-                # kernel mode.
-                # cutime: The number of jiffies that this process's waited-for children
-                # have been scheduled in user mode.
-                # cstime: The number of jiffies that this process's waited-for children
-                # have been scheduled in kernel mode.
+                # utime: The number of jiffies that this process has been
+                #        scheduled in user mode.
+                # stime: The number of jiffies that this process has been
+                #        scheduled in kernel mode.
+                # cutime: The number of jiffies that this process's waited-for
+                #         children have been scheduled in user mode.
+                # cstime: The number of jiffies that this process's waited-for
+                #         children have been scheduled in kernel mode.
                 parts = file.read().split()
                 start_time = int(parts[21])
                 utime = int(parts[13])
@@ -117,9 +113,11 @@
                 process_info["percent-cpu"] = pcpu
                 delta = timedelta(0, start_time // self._jiffies_per_sec)
                 if self._boot_time is None:
-                   logging.warning("Skipping process (PID %s) without boot time.")
-                   return None
-                process_info["start-time"] = to_timestamp(self._boot_time  + delta)
+                    logging.warning(
+                        "Skipping process (PID %s) without boot time.")
+                    return None
+                process_info["start-time"] = to_timestamp(
+                    self._boot_time + delta)
             finally:
                 file.close()
 

=== modified file 'landscape/lib/tests/test_process.py'
--- landscape/lib/tests/test_process.py	2011-07-05 05:09:11 +0000
+++ landscape/lib/tests/test_process.py	2012-10-11 08:30:30 +0000
@@ -1,12 +1,44 @@
 import unittest
+import os
 
 from landscape.tests.helpers import LandscapeTest
 
 from landscape.lib.process import calculate_pcpu, ProcessInformation
+from landscape.lib.fs import create_file
 
 
 class ProcessInfoTest(LandscapeTest):
 
+    def setUp(self):
+        super(ProcessInfoTest, self).setUp()
+        self.proc_dir = self.makeDir()
+
+    def _add_process_info(self, process_id, state="R (running)"):
+        """Add information about a process.
+
+        The cmdline, status and stat files will be created in the
+        process directory, so that get_process_info can get the required
+        information.
+        """
+        process_dir = os.path.join(self.proc_dir, str(process_id))
+        os.mkdir(process_dir)
+
+        cmd_line = "/usr/bin/foo"
+        create_file(os.path.join(process_dir, "cmdline"), cmd_line)
+
+        status = "\n".join([
+            "Name: foo",
+            "State: %s" % state,
+            "Uid: 1000",
+            "Gid: 2000",
+            "VmSize: 3000",
+            "Ignored: value"])
+        create_file(os.path.join(process_dir, "status"), status)
+
+        stat_array = [str(index) for index in range(44)]
+        stat = " ".join(stat_array)
+        create_file(os.path.join(process_dir, "stat"), stat)
+
     def test_missing_process_race(self):
         """
         We use os.listdir("/proc") to get the list of active processes, if a
@@ -52,6 +84,41 @@
         self.assertTrue(fakefile1.closed)
         self.assertTrue(fakefile2.closed)
 
+    def test_get_process_info_state(self):
+        """
+        C{get_process_info} reads the process state from the status file
+        and uses the first character to represent the process state.
+        """
+        self._add_process_info(12, state="A (some state)")
+        process_info = ProcessInformation(self.proc_dir)
+        info = process_info.get_process_info(12)
+        self.assertEqual("A", info["state"])
+
+    def test_get_process_info_state_preserves_case(self):
+        """
+        C{get_process_info} retains the case of the process state, since
+        for example both x and X can be different states.
+        """
+        self._add_process_info(12, state="a (some state)")
+        process_info = ProcessInformation(self.proc_dir)
+        info = process_info.get_process_info(12)
+        self.assertEqual("a", info["state"])
+
+    def test_get_process_info_state_tracing_stop_lucid(self):
+        """
+        In Lucid, capital T was used for both stopped and tracing stop.
+        From Natty and onwards lowercase t is used for tracing stop, so
+        we special-case that state and always return lowercase t for
+        tracing stop.
+        """
+        self._add_process_info(12, state="T (tracing stop)")
+        self._add_process_info(13, state="t (tracing stop)")
+        process_info = ProcessInformation(self.proc_dir)
+        info1 = process_info.get_process_info(12)
+        info2 = process_info.get_process_info(12)
+        self.assertEqual("t", info1["state"])
+        self.assertEqual("t", info2["state"])
+
 
 class CalculatePCPUTest(unittest.TestCase):
 

=== modified file 'landscape/monitor/tests/test_activeprocessinfo.py'
--- landscape/monitor/tests/test_activeprocessinfo.py	2011-07-05 05:09:11 +0000
+++ landscape/monitor/tests/test_activeprocessinfo.py	2012-10-11 08:30:30 +0000
@@ -122,7 +122,7 @@
         expected_process_1 = {"state": "T", "gid": 1000, "pid": 671,
                               "vm-size": 11676, "name": "blargh", "uid": 1000,
                               "start-time": 111, "percent-cpu": 0.0}
-        expected_process_2 = {"state": "I", "gid": 1000, "pid": 672,
+        expected_process_2 = {"state": "t", "gid": 1000, "pid": 672,
                               "vm-size": 11676, "name": "blarpy", "uid": 1000,
                               "start-time": 112, "percent-cpu": 0.0}
         processes = message["add-processes"]
@@ -213,7 +213,7 @@
                               "vm-size": 11676, "name": "blargh",
                               "uid": 1000, "start-time": 102,
                               "percent-cpu": 0.0}
-        expected_process_2 = {"state": "I", "gid": 1000, "pid": 672,
+        expected_process_2 = {"state": "t", "gid": 1000, "pid": 672,
                               "vm-size": 11676, "name": "blarpy",
                               "uid": 1000, "start-time": 104,
                               "percent-cpu": 0.0}
@@ -376,7 +376,7 @@
                                 "name": u"blarpy",
                                 "pid": 672,
                                 "start-time": 112,
-                                "state": "I",
+                                "state": "t",
                                 "uid": 1000,
                                 "vm-size": 11676,
                                 "percent-cpu": 0.0},