Merge lp:~nuclearbob/utah/cobbler-pidlock into lp:utah

[Max Brustkern]

The original cobbler inventory used 'available' and 'provisioned' as states. If a job timed out and left the machine in the 'provisioned' state, there's no automatic process to recover that. This changes the inventory to use the pid of the process requesting the machine as the state, and if a machine is requested and that pid is no longer active on the system, the machine will be claimed anyway. If the listed pid is still active, the machine will not be used. I've tested it in magners-orchestra with the dx team's jenkins job. It could cause problems if multiple provisioning hosts were trying to use the same sqlite database to manage physical machines, but I think that scenario would introduce other problems as well. If we're dealing with multiple hosts trying to provision the same pool of machines, a more robust solution than sqlite is probably warranted.

[Joe Talbott (joetalbott)]

Keep in mind that on a highly loaded machine PIDs get recycled so you might have a whole different process getting an available PID that is in the inventory database.

[Javier Collado (javier.collado)]

To take Joe's comment into account, you might want to check the process command line:

if psutil.pid_exists(pid) and 'run_utah_test.py' in '.join(psutil.Process(pid).cmdline):
    <machine is being used>

utah already depends on psutil, so this won't add an extra dependency to the package.

[Javier Collado (javier.collado)]

One more comment is that if you plan to use the pid to track machine reservation,
then the column name in the table should be renamed to pid.

[lp:~nuclearbob/utah/cobbler-pidlock]

708. By Max Brustkern on 2012-10-11

Adding command line check

709. By Max Brustkern on 2012-10-11

Added check for "run_cobbler_test.py"

710. By Max Brustkern on 2012-10-11

Changed run_cobbler_test to run_test_cobbler

[Max Brustkern (nuclearbob)]

I updated the check to look for "utah" or "run_test_cobbler.py" on the command line. I'm testing now. The thing about changing the name of the field is that it still could be "available" which isn't a pid. What's a good name that indicates it can either be a state or a pid? Or can we change available to some number that will never be a pid, like -1?

[Joe Talbott (joetalbott)]

On Thu, Oct 11, 2012 at 01:24:20PM -0000, Max Brustkern wrote:
> I updated the check to look for "utah" or "run_test_cobbler.py" on the command line. I'm testing now. The thing about changing the name of the field is that it still could be "available" which isn't a pid. What's a good name that indicates it can either be a state or a pid? Or can we change available to some number that will never be a pid, like -1?

What about changing the value to 'available - <pid>'? Then you can string
compare on 'available' and easily parse out the pid for doing the check.
> --
> https://code.launchpad.net/~nuclearbob/utah/cobbler-pidlock/+merge/129046
> Your team UTAH Dev is requested to review the proposed merge of lp:~nuclearbob/utah/cobbler-pidlock into lp:utah.

[Javier Collado (javier.collado)]

@Max

I think this looks like two different columns are needed. One for the state
and one for the pid and that the pid is used to check if the state can be trusted.
Encoding both things in one column could lead to confusion.

Another improvement could be to have a new table named states with all the possible
states as strings and replace the state column in the machines with state_id to be
used as a foreign key.

Finally, I'm not sure if this will grow in complexity in the future, but have you
thought about using an ORM instead of write sql commands directly?

[Max Brustkern (nuclearbob)]

I'll rework this and resubmit later.

review: Disapprove

Unmerged revisions

=== modified file 'utah/provisioning/inventory/sqlite.py'
--- utah/provisioning/inventory/sqlite.py	2012-08-22 15:20:25 +0000
+++ utah/provisioning/inventory/sqlite.py	2012-10-11 13:23:20 +0000
@@ -2,6 +2,7 @@
 
 import sqlite3
 import os
+import psutil
 from utah.provisioning.inventory.exceptions import \
         UTAHProvisioningInventoryException
 from utah.provisioning.inventory.inventory import Inventory
@@ -112,35 +113,52 @@
         else:
             for machineinfo in result:
                 cargs = dict(machineinfo)
-                machineid = cargs.pop('machineid')
-                name = cargs.pop('name')
-                state = cargs.pop('state')
-                if state == 'available':
-                    update = self.connection.execute(
-                            "UPDATE machines SET state='provisioned' "
-                            "WHERE machineid=? AND state='available'",
-                            [machineid]).rowcount
-                    if update == 1:
-                        machine = CobblerMachine(*args, cargs=cargs,
-                                inventory=self, name=name, **kw)
-                        self.machines.append(machine)
-                        return machine
-                    elif update == 0:
-                        raise UTAHProvisioningInventoryException(
-                                'Machine was requested by another process '
-                                'before we could request it')
-                    elif update > 1:
-                        raise UTAHProvisioningInventoryException(
-                                'Multiple machines exist '
-                                'matching those criteria; '
-                                'database ' + self.db + ' may be corrupt')
-                    else:
-                        raise UTAHProvisioningInventoryException(
-                                'Negative rowcount returned '
-                                'when attempting to request machine')
+                if cargs['state'] == 'available':
+                    return self._take(cargs, *args, **kw)
+            for machineinfo in result:
+                cargs = dict(machineinfo)
+                name = cargs['name']
+                state = cargs['state']
+                try:
+                    pid = int(state)
+                    try:
+                        if not (psutil.pid_exists(pid) and ('utah' in 
+                                ' '.join(psutil.Process(pid).cmdline)
+                                or 'run_test_cobbler.py' in 
+                                ' '.join(psutil.Process(pid).cmdline))):
+                        return self._take(cargs, *args, **kw)
+                except ValueError:
+                    continue
+
         raise UTAHProvisioningInventoryException(
                 'All machines meeting criteria are currently unavailable')
 
+    def _take(self, cargs, *args, **kw):
+        machineid = cargs.pop('machineid')
+        name = cargs.pop('name')
+        state = cargs.pop('state')
+        update = self.connection.execute(
+                "UPDATE machines SET state=? WHERE machineid=? AND state=?",
+                [os.getpid(), machineid, state]).rowcount
+        if update == 1:
+            machine = CobblerMachine(*args, cargs=cargs,
+                    inventory=self, name=name, **kw)
+            self.machines.append(machine)
+            return machine
+        elif update == 0:
+            raise UTAHProvisioningInventoryException(
+                    'Machine was requested by another process '
+                    'before we could request it')
+        elif update > 1:
+            raise UTAHProvisioningInventoryException(
+                    'Multiple machines exist '
+                    'matching those criteria; '
+                    'database ' + self.db + ' may be corrupt')
+        else:
+            raise UTAHProvisioningInventoryException(
+                    'Negative rowcount returned '
+                    'when attempting to request machine')
+
     def release(self, machine=None, name=None):
         if machine is not None:
             name = machine.name