552681 - Sisyphus - Crash Automation - lock job queue

Status: RESOLVED FIXED

(Testing Graveyard :: Sisyphus, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

The current approach of not using locks to prevent concurrent access to the job queue does not scale. By locking the job queue we can simplify the logic, reduce optimistic updating of the signature and worker documents, reduce the need for database compaction and reduce idle time for workers.

This patch uses heuristic values for the number of attempts to obtain a lock as well as the wait time between attempts to obtain a lock. This may need to be adjusted depending on the number of workers, however these values appear to work well with the colo machines and Tomcat's test farm.

Comment 1

[Bob Clary [:bc] (inactive)]

This patch is still current and is performing well in testing and can be reviewed.

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 432826 [details] [diff] [review]
patch v1

The skip value used in getSignatureForWorker has a problem in that once a worker skips over an already processed job, it does not know when the better worker will process and delete that job.
 
The skip value does not take into account when the worker transitions from working on one of the queues returned by getPendingJobs to the next. For example, it might be calculating skip based on the os_name, cpu_name, os_version then after processing all of those jobs begins processing os_name, cpu_name but is still using the same skip value. The skip should be reset when a different key into the pending jobs view is used.

In addition, the worker should be smart enough to know that it has been getting no jobs from the first N job queue keysets and be smart about not querying them everytime. This is especially important for 0 priorty queues since they will result in 4 queries with no results unless there are 0 priority jobs.

I'm going to rethink this a bit.

Comment 3

[cmtalbert]

What if skip is a mapping of signature to a value.

Workers set skip in two places:
1. They have already processed the signature
2. A better worker is available for the signature and we want them to take it

In both these cases, you have a document and a query result available. So what if the skip value is a list of objects and each object is:
{signature:<signature>, worker:<bestworker>, skipnum:<order in query>}

In case 1 (worker processed signature already):
* set the signature = to signature
* set worker = self.worker ID (this worker's id)
* set skipnum = the ordinal value of this signature in the result you got back or perhaps to the number of rows to skip (which is the value skip currently is)

In case 2:
* set the signature to signature
* set worker to ID of best worker for the task
* set skipnum = the ordinal value of this signature in the result you got back or perhaps to the number of rows to skip (which is the value skip currently is)

Then if you determine that the best worker has already done the signature and the best worker was not "us" then the worker could attempt the signature.  

The worker would never attempt to re-do signatures it already processed.

The skip number could be incremented so that you immediately jump over this result in future result sets so that you don't query it again.

I dunno if that is over-engineering it, but I was thinking about it earlier and it sorta made sense to me.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: patch in addition to v1

This is the approach I've been thinking of. It is still in testing though and I don't have the comments added yet to explain it though. If we go this route, I'll fold it into the v1 patch.

Basically it uses a skip per possible key range into the pending jobs view. If a view doesn't return any matching rows it sets the skip to -1 so that the view is not tried again. Since the other workers will be deleting the processed signatures we won't be able to keep the skips completely up to date. In addition, new signatures or changes in the priority would mean the skip values need to be reset. Currently I'm doing that every hour.

I'll have to think about your approach more. It seems complicated though.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch v2 in addition to v1

fix some minor fubars.

Comment 6

[cmtalbert]

Comment on attachment 434856 [details] [diff] [review]
patch v2 in addition to v1

I think what you've got here is the best idea yet.  You are solving the problem that I was seeing - the skip number should be tied to the query (view) which is what I kind of understood but wasn't able to articulate a good solution for in my previous comment.  

I that tying the skip to the views is the right way to go, and this code looks like it handles this properly.  

I think I was confused on how the views come back from the database in my previous comment.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: patch v3

Comment 8

[cmtalbert]

Comment on attachment 438167 [details] [diff] [review]
patch v3

>From: Bob Clary <[e-mail]>
>Bug 552681 -  Sisyphus - Crash Automation - lock job queue.
>
>diff --git a/bin/sisyphus/couchdb.py b/bin/sisyphus/couchdb.py
>+    def deleteLock(self, document, reconnect = True, owned = False):
>+        """
>+        Delete an existing lock document.
>+
>+        deleteLock will raise Exception('InvalidLockDocument') if called with a non lock
>+        document.
>+
>+        deleteLock will raise Exception("deleteDocument: aborting after %d attempts"
>+                                        % (self.max_db_attempts[-1] + 1))
>+        if it fails to delete the lock after the maximum number of attempts.
>+        This may result in broken lock which must be resolved by user intervention.
>+
>+        deleteLock will pass through any exceptions not related to CouchDB.
>+        This may result in broken lock which must be resolved by user intervention.
>+        """
As we build out the administration interface, we'd want to detect this situation and/or capture an alert when a broken lock is formed.  So, would you mind filing a follow-on bug for this case so that we can modify this code later to add in the alert behavior for this type of error?  I think it's out of scope of this patch to add it here.

Really nice comments in this one, it was really easy to see what's going on.

Comment 9

[Bob Clary [:bc] (inactive)]

Currently, when the user starts up it will clear its own stale locks but I'll file something.

Comment 10

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/qa/sisyphus/rev/e63468d24710