981039 - slaveapi should have a seperate knob for concurrency on buildapi calls alone

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Justin Wood (:Callek)]

Attachment: concurrent_buildapi

Buildapi load is one of the largest reasons we've kept slave rebooter and slaveapi's max concurrency low, with this patch we should be able to expand that.

Comment 1

[[e-mail] (:bhearsum)]

Comment on attachment 8387767 [details] [diff] [review]
concurrent_buildapi

Review of attachment 8387767 [details] [diff] [review]:
-----------------------------------------------------------------

This might be a bit cleaner if you subclass Semaphore to store the old max, but I'm fine with this too - totally up to you. If you take this approach, there's a couple of things to address:

::: slaveapi-server.py
@@ +115,5 @@
> +        else:
> +            max_buildapi = config["concurrency"]
> +        if semaphores["buildapi"] and \
> +                max_buildapi != cached_sem_max["buildapi"]:
> +            semaphores["buildapi"].counter += max_buildapi - cached_sem_max["buildapi"]

Have you tested what happens if this becomes negative? Eg:
max_buildapi is 5
cached_sem_max["buildapi"] is 10
counter is 2

Counter will get set to -3 -- does the Semaphore handle this correctly?

@@ +119,5 @@
> +            semaphores["buildapi"].counter += max_buildapi - cached_sem_max["buildapi"]
> +        elif semaphores["buildapi"]:
> +            pass # Nothing to do, counter didn't change
> +        else:
> +            semaphores["buildapi"] = Semaphore(max_buildapi)

I think these 3 blocks get simpler if you check for the semaphore upfront:
if "buildapi" not in semaphores:
  ...
else:
  if max_buildapi != cached_sem_max["buildapi"]:
    ...

::: slaveapi.ini.sample
@@ +21,4 @@
>  
>  [buildapi]
>  api_url = https://buildapi.pvt.build.mozilla.org/buildapi/
> +; max_concurrent is optional, defaults to [server].concurrency

This doesn't seem like a very sensible default if you want to turn up the overall server concurrency by rate limiting buildapi to something smaller. Seems like this should just be required.

::: slaveapi/global_state.py
@@ +15,5 @@
>  
>  from .messenger import Messenger
>  messenger = Messenger()
> +
> +semaphores = defaultdict(lambda: None)

I don't think there's any reason for this to be a defaultdict. You'll never have a case where semaphores["buildapi"] is undefined if you reformat the block from above.

Comment 2

[Justin Wood (:Callek)]

Attachment: [slaveapi] v2

Addresses previous comments and additionally corrects a gevent 1.0 v 0.13 compat issue (caused by looking at docs for 1.0 instead of .13

Comment 3

[Justin Wood (:Callek)]

Attachment: [puppet] v1

Here is the puppet patch to adjust the buildapi concurrency knob now, rather than try and remember this when we deploy.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 8392634 [details] [diff] [review]
[puppet] v1

https://hg.mozilla.org/build/puppet/rev/c35dccaa97e6

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8392632 [details] [diff] [review]
[slaveapi] v2

$ git push
Counting objects: 42, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (29/29), done.
Writing objects: 100% (29/29), 4.22 KiB | 0 bytes/s, done.
Total 29 (delta 22), reused 0 (delta 0)
To ssh://git.mozilla.org/build/slaveapi.git
   1173310..ccce02a  master -> master

$ git log 1173310..ccce02a
commit ccce02ac8aa2bd7a000733a8d1211881d84fd927
Author: Justin Wood <[e-mail]>
Date:   Tue Mar 25 22:46:10 2014 -0400

    Bump slaveapi ver for Bug 921067

commit ce5eb8a162100f825b910a92c43cc7f6cb10d8e5
Author: Justin Wood <[e-mail]>
Date:   Tue Mar 25 22:44:17 2014 -0400

    Bug 921067 - Slaveapi should support panda devices for reboots. r=bhearsum

commit 8d13f0e58ce69436973a25ee2209cc09c08cf24a
Author: Justin Wood <[e-mail]>
Date:   Tue Mar 25 22:41:45 2014 -0400

    Bug 984721 - Slaveapi should abstract out Machine from Slave. r=bhearsum

commit 7b3966fd3b2904a9b51edac38239bba432ac16cc
Author: Justin Wood <[e-mail]>
Date:   Tue Mar 25 22:35:30 2014 -0400

    Bug 981039 - slaveapi should have a seperate knob for concurrency on buildapi calls alone. r=bhearsum

Comment 6

[Justin Wood (:Callek)]

This actually failed in production :( -- I had to push a bustage fix:

To ssh://[e-mail]/build/slaveapi.git
   c57290f..f51fe93  master -> master

http://hg.mozilla.org/build/slaveapi/rev/7cab0d7af0da

Comment 7

[Justin Wood (:Callek)]

deployed for real as part of current Slaveapi 1.0.18.1