932396 - implement "disable" action in slaveapi

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

I'm thinking of this as a one stop solution for "get this slave out of the pool". My initial thought is something like:
POST /slaves/:slave/actions/disable

Which would accept the following args:
* force - whether or not to use graceful shutdown (defaults to true)
* bug_comment - if passed, slave's problem tracking bug will be re-opened with this comment.

The implementation should do at least the following:
* Disable the slave in slavealloc.
* Shut down the buildbot process (using graceful shutdown or not depending on the 'force' flag). If the process can't be confirmed to be dead we should get more forceful and reboot the slave.
* Comment/reopen in the bug if a comment was provided.

Comments/suggestions welcome.

Comment 1

[Ed Morley [:emorley]]

This sounds great! :-)

Comment 2

[Justin Wood (:Callek)]

Attachment: slavealloc

I discussed this in IRC a few days ago and this was roughly what we thought should be done.

This does not fully implement this bug though. Does this approach or is c#0's wording a better idea?

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 8405977 [details] [diff] [review]
slavealloc

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

I think you forgot to add a file in this patch - I don't see the action.

::: slaveapi/clients/slavealloc.py
@@ +27,5 @@
> +
> +def update_slavealloc(api, id_=None, name=None, data={}):
> +    if id_ and name:
> +        raise ValueError("Can't update slave by id and name at the same time.")
> +    

AFAIK you can use URLs like http://slavealloc.build.mozilla.org/api/slaves/foo?byname=1 to update - you don't need to bother with the extra round trip to get the id. Besides, slaveapi is slave name-centric, unless something big changes, there's no situation where you'd have the id but not the name.

Comment 4

[[e-mail] (:bhearsum)]

Oh, and having an explicit "disable" URL might be nice just from a UX standpoint - it makes it pretty clear what's happening. You could still have a single function to support generic updating and disabling, you'd just pass different args.

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8405977 [details] [diff] [review]
slavealloc

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

Waiting for a new patch

Comment 6

[Justin Wood (:Callek)]

Attachment: [slaveapi] v1.1

same patch as attachment 8405977 [details] [diff] [review] just with added file

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 8407790 [details] [diff] [review]
[slaveapi] v1.1

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

The code looks ok, just a couple of structural things to address:

::: slaveapi/actions/update.py
@@ +12,5 @@
> +        return 0
> +    if val == "disabled" or val == False:
> +        return 0
> +    if val == 0:
> +        return 0

This is probably a bit shorter and more readable by doing:
if val in ["n", "no", "disabled", False, 0]:

@@ +22,5 @@
> +        return 1
> +    raise ValueError("Unsupported value (%s) for truthiness" % val)
> +
> +def update(name):
> +    args = request.args

You're mixing layers by touching the request here. This should be done in the View and passed explicitly.

@@ +30,5 @@
> +        if key == "enabled":
> +            # Normalize value
> +            slavealloc_values['enabled'] = normalize_truthiness(value)
> +        else:
> +            raise ValueError("Unsupported update arg %s" % key)

This normalization is probably better done in the View. You can also be nicer to the client and return a descriptive 400 error if a bad key is passed, or if normalize_truthiness raises a ValueError.

Comment 8

[Jordan Lund (:jlund)]

==== status update

I'm gonna give a crack at this.

Comment 9

[Jordan Lund (:jlund)]

Attachment: 140514_932396_disable_action-slaveapi.patch

this is a draft.

I have been able to verify it disables in slavealloc but I am having issues with my local setup. see next comment.

Comment 10

[Jordan Lund (:jlund)]

I can not call the action shutdown_buildslave on my local setup.

this fails either directly like:
curl -d 'foo=bar' localhost:8080/slaves/talos-r4-snow-103/actions/shutdown_buildslave

or through the disable action

stacktrace tells me it is while trying to ping(), gevent_subprocess looks like it monkeypatches subprocess and has an issue with how I am calling get_output(). I verified that slave.fqdn == a valid fqdn eg:'talos-r4-snow-103.build.scl1.mozilla.com' so it is not the arg being passed.

I checked the site-packages on slaveapi-dev1 and both gevent and gevent_subprocess versions match my local setup.

Maybe it's not cross platform and my mac has issues with stdin/err/pipe stuff.

here is the full stacktrace:
2014-05-14 00:20:36,561 - INFO - talos-r4-snow-103 - Getting devices.json info
2014-05-14 00:20:36,842 - ERROR - Something went wrong while processing!
Traceback (most recent call last):
  File "/Users/jlund/devel/mozilla/dirtyRepos/slaveapi_jlund/slaveapi/processor.py", line 60, in _worker
    res, msg = action(slave, *args, **kwargs)
  File "/Users/jlund/devel/mozilla/dirtyRepos/slaveapi_jlund/slaveapi/actions/disable.py", line 65, in disable
    stop_buildslave_result, stop_buildslave_msg = shutdown_buildslave(name)
  File "/Users/jlund/devel/mozilla/dirtyRepos/slaveapi_jlund/slaveapi/actions/shutdown_buildslave.py", line 31, in shutdown_buildslave
    if not ping(slave.fqdn):
  File "/Users/jlund/devel/mozilla/dirtyRepos/slaveapi_jlund/slaveapi/clients/ping.py", line 14, in ping
    output = check_output(cmd, stderr=STDOUT)
  File "/Users/jlund/venv/slaveapi/lib/python2.7/site-packages/gevent_subprocess-0.1.1-py2.7.egg/gevent_subprocess/gevent_subprocess.py", line 364, in check_output
    process = Popen(stdout=PIPE, *popenargs, **kwargs)
  File "/Users/jlund/venv/slaveapi/lib/python2.7/site-packages/gevent_subprocess-0.1.1-py2.7.egg/gevent_subprocess/gevent_subprocess.py", line 237, in __init__
    universal_newlines, startupinfo, creationflags)
  File "/Users/jlund/venv/slaveapi/lib/python2.7/site-packages/gevent_subprocess-0.1.1-py2.7.egg/gevent_subprocess/gevent_subprocess.py", line 196, in __init__
    errread, errwrite) = self._get_handles(stdin, stdout, stderr)

Question: I will need to continue testing https://bugzilla.mozilla.org/attachment.cgi?id=8422259 but I suppose it will have to be on the dev slaveapi machine. looks like slaveapi-dev1 doesn't have git or hg on it so what is the procedure here? Should I get this through review, land on dev, and then continue testing?

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 8422259 [details] [diff] [review]
140514_932396_disable_action-slaveapi.patch

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

fly-by for now

::: slaveapi/actions/reboot.py
@@ +114,3 @@
>              status_text += "Filed IT bug for reboot (bug %s)" % slave.reboot_bug.id_
> +            if update_bug:
> +                slave.reboot_bug = file_reboot_bug(slave)

status_text += should be moved into this block too

Comment 12

[[e-mail] (:bhearsum)]

(In reply to Jordan Lund (:jlund) from comment #10)
> I can not call the action shutdown_buildslave on my local setup.
> 
> this fails either directly like:
> curl -d 'foo=bar'
> localhost:8080/slaves/talos-r4-snow-103/actions/shutdown_buildslave
> 
> or through the disable action
> 
> stacktrace tells me it is while trying to ping(), gevent_subprocess looks
> like it monkeypatches subprocess and has an issue with how I am calling
> get_output(). I verified that slave.fqdn == a valid fqdn
> eg:'talos-r4-snow-103.build.scl1.mozilla.com' so it is not the arg being
> passed.
> 
> I checked the site-packages on slaveapi-dev1 and both gevent and
> gevent_subprocess versions match my local setup.

You look like you're testing on Mac. If that's the case, I'd suggest trying on Linux - I've heard of issues running SlaveAPI on Mac. You can run an instance on cruncher on slaveapi-dev1 - they both have the required netflows.

Comment 13

[Jordan Lund (:jlund)]

Attachment: 160514_932396_disable_action-slaveapi-2.patch

This can be seen tested with this bug: https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720

I only tested against one platform but it utilizes two actions that are already implemented (shutdown_buildslave and reboot). The only other one is update_slavealloc() which should be platform agnostic. 

if (slave is disabled in slavealloc) and (shutdown_buildslave fails) and (use_force is false):
    https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720#c5

if (slave is disabled in slavealloc) and (shutdown_buildslave fails) and (use_force is true):
    https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720#c6

if (slave is disabled in slavealloc) and (shutdown_buildslave passes) and (buildslave isn't currently connected to a master):
    https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720#c7

if (slave is disabled in slavealloc) and (shutdown_buildslave passes) and (buildslave was connected to a master):
    https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720#c8

if (slave is already disabled in slavealloc):
   bug is not updated but this is returned: '"text": "Disabling Slave: talos-r4-snow-103 by...\nSlave is already disabled. Nothing to do."

I am sure there are many edge cases and this stuff will need tests but I think we are at a state where we can hammer this on default slaveapi-dev1

if it passes reviews, I'll update the docs component.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 8424162 [details] [diff] [review]
160514_932396_disable_action-slaveapi-2.patch

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

steps 2 and 3 in disable.py I suspect we might want to leave up to the caller to do as needed. I can easily see good value in always updating the problem tracking bug when disabling via the api though!

f? to coop for his thoughts on that specific bit

Comment 15

[Chris Cooper [:coop] (he/him)]

Comment on attachment 8424162 [details] [diff] [review]
160514_932396_disable_action-slaveapi-2.patch

(In reply to Justin Wood (:Callek) from comment #14)
> steps 2 and 3 in disable.py I suspect we might want to leave up to the
> caller to do as needed. I can easily see good value in always updating the
> problem tracking bug when disabling via the api though!
> 
> f? to coop for his thoughts on that specific bit

I see Callek's point, but I don't think there's ever a case where we *don't* want to make sure disabling a slave is acted on immediately, i.e. we never disable things speculatively because reboots (after jobs or slaveapi) could happen at any time.

I'm fine with the code the way it is, but would also support the addition of a shallower call that just toggled the database alongside the current code (or maybe a flag to just do the db action).

Comment 16

[Jordan Lund (:jlund)]

Comment on attachment 8424162 [details] [diff] [review]
160514_932396_disable_action-slaveapi-2.patch

kicking to ben as callek has been on buildduty and now projectduty.

Ben, this patch reflects the previous efforts of callek and the feedback you subsequently gave him. One thing you'll notice is that I wasn't able to impl as you suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=932396#c3

The reason is that I was not able to do a PUT on slavealloc unless I provided the ID. I think this is because of:
http://mxr.mozilla.org/build/source/tools/lib/python/slavealloc/daemon/http/api.py#105
http://mxr.mozilla.org/build/source/tools/lib/python/slavealloc/daemon/http/api.py#75

I am happy to add an option that accomplishes https://bugzilla.mozilla.org/show_bug.cgi?id=932396#c14

my reason for not including it was, at the time of implementing this, I thought it be useful to know when the slave was *actually* disabled and no longer running a job. This is so we can safely do things like shutdown, re-image, loan, etc.

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 8424162 [details] [diff] [review]
160514_932396_disable_action-slaveapi-2.patch

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

One overall comment: a lot of the variables are REALLY verbosely named. "use_force" could be "force", "reason_comment" could be "reason", "values_to_update" could be "data". I'm not going to r- for any of this, but for me, it hurts readability, not helps it.

::: slaveapi/actions/disable.py
@@ +32,5 @@
> +        name (str) -- hostname of slave
> +        reason_comment (str) -- reason we wish to disable slave
> +        use_force (bool) -- if true and buildslave proc can't be killed
> +            gracefully, reboot the slave
> +    """

Have you tried building the docs to see if this renders args properly? I don't think it will. http://git.mozilla.org/?p=build/slaveapi.git;a=blob;f=slaveapi/web/action_base.py;h=bcb73557dd49156687757046734d07d16016747e;hb=HEAD has an example of a properly constructed docstring.

@@ +33,5 @@
> +        reason_comment (str) -- reason we wish to disable slave
> +        use_force (bool) -- if true and buildslave proc can't be killed
> +            gracefully, reboot the slave
> +    """
> +    action_succeeded_so_far = True

I don't think this needs to exist. If you update result_status in each step you can use it instead.

@@ +53,5 @@
> +    update_alloc_result, update_alloc_msg = slavealloc.update_slave(
> +        api=config["slavealloc_api_url"], name=name,
> +        values_to_update=slavealloc_values,
> +    )
> +    if update_alloc_result is FAILURE:

This works, but only because everybody is using these defined names. "is" compares identity, not value, so if anyone ever assigns an int to update_alloc_result, this fails. You should use == here instead.

@@ +61,5 @@
> +
> +    #### 2. Stop Buildbot Process
> +    if action_succeeded_so_far:
> +        stop_buildslave_result, stop_buildslave_msg = shutdown_buildslave(name)
> +        status_msgs.append(str(stop_buildslave_msg))

There's no reason to do a shutdown_buildslave if we're forcing. I think this block is better written as:
if result == SUCCESS:
  if force:
    # do the reboot
  else:
    # shutdown the buildslave.

::: slaveapi/actions/shutdown_buildslave.py
@@ +24,5 @@
>      slave.load_slavealloc_info()
>      slave.load_devices_info()
>  
>      if not slave.master_url:
> +        status_text += "Success\nNo master set, nothing to do!"

Not including the slave name in these messages makes them useless in slaveapi.log because they can no longer be associated with a specific action or slave. I'm not sure why "Success" or "Failure" needs to be in here either. If you're looking at the log, the result is on the same line. If you're looking at the web, it's beside the message.

You probably have a good reason that I'm overlooking...can you explain?

::: slaveapi/clients/slavealloc.py
@@ +38,5 @@
> +        values_to_update (dict) -- the slave's values we wish to change
> +
> +    returns:
> +        a tuple that consists of the return_code and return_msg
> +    """

Same thing here re: docstring

::: slaveapi/web/slave.py
@@ +80,5 @@
> +        else:
> +            raise ValueError(
> +                "Unsupported value (%s) for truthiness. Accepted values: "
> +                "truthy - %s, falsy - %s" % (value, true_values, false_values)
> +            )

Seems like this could be factored out - it's certainly not specific to disabling a slave.

@@ +83,5 @@
> +                "truthy - %s, falsy - %s" % (value, true_values, false_values)
> +            )
> +
> +    def post(self, slave, *action_args, **action_kwargs):
> +        action_kwargs['reason_comment'] = request.form.get('reason_comment')

Modifying kwargs is icky, and sometimes leads to confusing states, because dicts are passed by reference, not by value. You don't need to do this, you can call ActionBase.post as follows instead:
ActionBase.post(self, slave, *args, reason_comment=reason_comment, **kwargs). Here's an example of that from Balrog: https://github.com/mozilla/balrog/blob/master/auslib/admin/views/base.py#L18

(It would also be good to just use "args" and "kwargs" as names -- that's the norm for this sort of thing.)

Comment 18

[Jordan Lund (:jlund)]

---------------------------------------------------------
> 
> One overall comment: a lot of the variables are REALLY verbosely named.
> "use_force" could be "force", "reason_comment" could be "reason",
> "values_to_update" could be "data". I'm not going to r- for any of this, but
> for me, it hurts readability, not helps it.

k


> Have you tried building the docs to see if this renders args properly? I
> don't think it will.
> http://git.mozilla.org/?p=build/slaveapi.git;a=blob;f=slaveapi/web/
> action_base.py;h=bcb73557dd49156687757046734d07d16016747e;hb=HEAD has an
> example of a properly constructed docstring.

haven't tested docs. I found the 'arg -- description' pattern here: http://legacy.python.org/dev/peps/pep-0257/#id17

I'll change it to match docstrings in slaveapi.


> I don't think this needs to exist. If you update result_status in each step
> you can use it instead.

will use that instead.


> This works, but only because everybody is using these defined names. "is"
> compares identity, not value, so if anyone ever assigns an int to
> update_alloc_result, this fails. You should use == here instead.

will fix

> @@ +61,5 @@
> > +
> > +    #### 2. Stop Buildbot Process
> > +    if action_succeeded_so_far:
> > +        stop_buildslave_result, stop_buildslave_msg = shutdown_buildslave(name)
> > +        status_msgs.append(str(stop_buildslave_msg))
> 
> There's no reason to do a shutdown_buildslave if we're forcing. I think this
> block is better written as:
> if result == SUCCESS:
>   if force:
>     # do the reboot
>   else:
>     # shutdown the buildslave.

ok.

> 
> ::: slaveapi/actions/shutdown_buildslave.py
> @@ +24,5 @@
> >      slave.load_slavealloc_info()
> >      slave.load_devices_info()
> >  
> >      if not slave.master_url:
> > +        status_text += "Success\nNo master set, nothing to do!"
> 
> Not including the slave name in these messages makes them useless in
> slaveapi.log because they can no longer be associated with a specific action
> or slave. I'm not sure why "Success" or "Failure" needs to be in here
> either. If you're looking at the log, the result is on the same line. If
> you're looking at the web, it's beside the message.
> 
> You probably have a good reason that I'm overlooking...can you explain?

I don't think this previously stated the slave either:
-        return SUCCESS, "%s - No master set, nothing to do!"

I was following the convention I saw in http://mxr.mozilla.org/build/source/slaveapi/slaveapi/actions/reboot.py#105 and IIUC, we do not include the 'slave name' in that return msg upon success either.

Speaking of which, the way things are returned at the end of the action confuse me. As in the convention:
return RETURN_CODE, RETURN_MSG

I don't know where the RETURN_MSG is saved to the log. you mention in slaveapi.log but on the dev instance of slaveapi, I don't see any lines of: mentioning this string: http://mxr.mozilla.org/build/source/slaveapi/slaveapi/actions/reboot.py#40

I can only see lines that use log.info/warning/exception showing up in slaveapi.log. and I thought RETURN_MSG was only used as the msg in json that is returned when you to a GET to an action you previously ran.

if you look at https://bugzilla-dev.allizom.org/show_bug.cgi?id=944720#c5, this was what I was my motivation. I wanted status_text to show (a) the slave it was disabling and then (b) each action it was attempting with a statement of whether it was a 'Success' or 'Failure'. status_text is one long string that combines (update_slavealloc -> shutdown_buildslave -> reboot) all into disable().


> ::: slaveapi/web/slave.py
> @@ +80,5 @@
> > +        else:
> > +            raise ValueError(
> > +                "Unsupported value (%s) for truthiness. Accepted values: "
> > +                "truthy - %s, falsy - %s" % (value, true_values, false_values)
> > +            )
> 
> Seems like this could be factored out - it's certainly not specific to
> disabling a slave.

k, I'll find a new home for normalize_truthiness()


> Modifying kwargs is icky, and sometimes leads to confusing states, because
> dicts are passed by reference, not by value. You don't need to do this, you
> can call ActionBase.post as follows instead:
> ActionBase.post(self, slave, *args, reason_comment=reason_comment,
> **kwargs). Here's an example of that from Balrog:
> https://github.com/mozilla/balrog/blob/master/auslib/admin/views/base.py#L18

k I'll use that idiom.

> 
> (It would also be good to just use "args" and "kwargs" as names -- that's
> the norm for this sort of thing.)

alright. I was using the same arg names as the super method I found here: http://mxr.mozilla.org/build/source/slaveapi/slaveapi/web/action_base.py#55

Comment 19

[[e-mail] (:bhearsum)]

(In reply to Jordan Lund (:jlund) from comment #18)
> > Have you tried building the docs to see if this renders args properly? I
> > don't think it will.
> > http://git.mozilla.org/?p=build/slaveapi.git;a=blob;f=slaveapi/web/
> > action_base.py;h=bcb73557dd49156687757046734d07d16016747e;hb=HEAD has an
> > example of a properly constructed docstring.
> 
> haven't tested docs. I found the 'arg -- description' pattern here:
> http://legacy.python.org/dev/peps/pep-0257/#id17
> 
> I'll change it to match docstrings in slaveapi.

Yeah, I'm not sure we're following that convention. Our docs are sphinx based, so we need to abide by theirs.

> > ::: slaveapi/actions/shutdown_buildslave.py
> > @@ +24,5 @@
> > >      slave.load_slavealloc_info()
> > >      slave.load_devices_info()
> > >  
> > >      if not slave.master_url:
> > > +        status_text += "Success\nNo master set, nothing to do!"
> > 
> > Not including the slave name in these messages makes them useless in
> > slaveapi.log because they can no longer be associated with a specific action
> > or slave. I'm not sure why "Success" or "Failure" needs to be in here
> > either. If you're looking at the log, the result is on the same line. If
> > you're looking at the web, it's beside the message.
> > 
> > You probably have a good reason that I'm overlooking...can you explain?
> 
> I don't think this previously stated the slave either:
> -        return SUCCESS, "%s - No master set, nothing to do!"
> 
> I was following the convention I saw in
> http://mxr.mozilla.org/build/source/slaveapi/slaveapi/actions/reboot.py#105
> and IIUC, we do not include the 'slave name' in that return msg upon success
> either.

My bad, you're right - this is fine.

> Speaking of which, the way things are returned at the end of the action
> confuse me. As in the convention:
> return RETURN_CODE, RETURN_MSG
> 
> I don't know where the RETURN_MSG is saved to the log. you mention in
> slaveapi.log but on the dev instance of slaveapi, I don't see any lines of:
> mentioning this string:
> http://mxr.mozilla.org/build/source/slaveapi/slaveapi/actions/reboot.py#40
> 
> I can only see lines that use log.info/warning/exception showing up in
> slaveapi.log. and I thought RETURN_MSG was only used as the msg in json that
> is returned when you to a GET to an action you previously ran.

You're right about this, too. So, ignore my comments here - sorry for the confusion!

> > 
> > (It would also be good to just use "args" and "kwargs" as names -- that's
> > the norm for this sort of thing.)
> 
> alright. I was using the same arg names as the super method I found here:
> http://mxr.mozilla.org/build/source/slaveapi/slaveapi/web/action_base.py#55

Ah, I see. So the reason they're named different there is because they're not just getting forwarded along to a parent class - they're getting passed to something else.

Comment 20

[Jordan Lund (:jlund)]

Attachment: 300514_932396_disable_action-slaveapi-interdiff.patch

interdiff from the last patch just to reflect r- changes.

Comment 21

[Jordan Lund (:jlund)]

Attachment: 300514_932396_disable_action-slaveapi.patch

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 8432253 [details] [diff] [review]
300514_932396_disable_action-slaveapi.patch

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

::: slaveapi/actions/disable.py
@@ +1,4 @@
> +from slaveapi.clients import slavealloc
> +from slaveapi.slave import Slave
> +from slaveapi.actions.reboot import reboot
> +from slaveapi.actions.shutdown_buildslave import shutdown_buildslave

Small nit here and elsewhere: use relative imports to stay consistent with the existing style. Feel free to fix that upon landing.

Comment 23

[Jordan Lund (:jlund)]

I can not upload a slaveapi version tarball due to: [14:45:49]IdustinIdividehex: rail: fyi /data is RO on the distinguished puppetmaster, pointing at an LVM snapshot. I'm syncing from relabs to releng on the live filesystem. Then I'll swap them back.

I did push the above patch to: http://git.mozilla.org/?p=build/slaveapi.git;a=commit;h=5e3b3842ecd2d7dc1afd52edbf9294cedf4e5d5b

note that I used relative imports like suggested in comment 22 and I bumped the version to 1.1.0 as semver.org says 'Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API'

I will finish landing this on dev instance once /data/python/packages is writable on releng-puppet2

Comment 24

[Jordan Lund (:jlund)]

Attachment: disable_slaveapi-puppet.patch

/data was writable today on puppet so I uploaded python dist version 1.1.0 here: http://puppetagain.pub.build.mozilla.org/data/python/packages/slaveapi-1.1.0.tar.gz

I guess after puppet is merged and deployed, the plan is to land on slaveapi-dev first, test a few api calls, and then land on prod?

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 8433859 [details] [diff] [review]
disable_slaveapi-puppet.patch

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

(In reply to Jordan Lund (:jlund) from comment #24)
> Created attachment 8433859 [details] [diff] [review]
> disable_slaveapi-puppet.patch
> 
> /data was writable today on puppet so I uploaded python dist version 1.1.0
> here:
> http://puppetagain.pub.build.mozilla.org/data/python/packages/slaveapi-1.1.0.
> tar.gz
> 
> I guess after puppet is merged and deployed, the plan is to land on
> slaveapi-dev first, test a few api calls, and then land on prod?

Makes sense to me. Note that Puppet will take of installing the new version though - you only need to restart the daemon: https://wiki.mozilla.org/ReleaseEngineering/Applications/SlaveAPI#Deployment

Comment 26

[Jordan Lund (:jlund)]

Attachment: disable_slaveapi-bustage-fix-util-package.patch

r=callek

this adds util package to setup.py so sdist will pick it up.

Comment 27

[Jordan Lund (:jlund)]

slaveapi bustage fix: http://git.mozilla.org/?p=build/slaveapi.git;a=commitdiff;h=b5aa9c168371a999bf2cc4ac55e24f31c2d62488

puppet version bump: https://hg.mozilla.org/build/puppet/rev/df54b8f4b218

Comment 28

[Jordan Lund (:jlund)]

slaveapi dev instance has been updated via puppet and restarted.

Comment 29

[Jordan Lund (:jlund)]

a few iterations were tested on dev instance. seems to work as expected. merged to prod. marking this resolved for now.

prod instance == version 1.1.1

\o/

Comment 30

[Jordan Lund (:jlund)]

Attachment: 140611_disable_bug_fix_slave_reboot_bug.patch

bug found:

slave: https://secure.pub.build.mozilla.org/builddata/reports/slave_health/slave.html?class=test&type=t-snow-r4&name=t-snow-r4-0021

log:
"""
File "/builds/slaveapi/prod/lib/python2.7/site-packages/slaveapi/actions/reboot.py", line 115, in reboot
AttributeError: 'NoneType' object has no attribute 'id_'
"""

Comment 31

[Jordan Lund (:jlund)]

Attachment: 140611_bug_932396_slaveapi_disable-puppet-version_bump_1.1.2.patch

a preemptive patch.

wont land/merge->prod till slaveapi patch is r+ and I can see the dist on: http://puppetagain.pub.build.mozilla.org/data/python/packages/

Comment 32

[Jordan Lund (:jlund)]

Comment on attachment 8438789 [details] [diff] [review]
140611_disable_bug_fix_slave_reboot_bug.patch

http://git.mozilla.org/?p=build/slaveapi.git;a=commit;h=2d701b64d569bc44e3a5a54fbc57ac2e11396706

Comment 33

[Jordan Lund (:jlund)]

Comment on attachment 8438793 [details] [diff] [review]
140611_bug_932396_slaveapi_disable-puppet-version_bump_1.1.2.patch

http://hg.mozilla.org/build/puppet/rev/b2ada68d6dc5

waiting for puppet to sync up with slaveapi nodes before restarting instances.

Comment 34

[Jordan Lund (:jlund)]

dev and prod slaveapi == 1.1.2