swarming: add transaction_id to tasks.new request

swarming: add transaction_id to tasks.new request

tasks.new RPCs are not safe to repeat on transient errors beacuse tasks.new is
not idempotent. Also it is possible that a task creation request was served, but
the client did not receieve the response due to network. In this case it is not
possible to ensure the task does not exist.

Add transaction_id to NewTaskRequest. It serves two purposes:

- it makes tasks.new API "more" idempotent: if a second request with the same
  transaction id is received and a task already exists, the handler reuses
  existing task. This allow a client to retry tasks.new requests on transient
  failures.
- serves as a second, temporary, identity of the task: a user can cancel a task
  by transaction id.

Internally task_id is stored in the memcache entry with a key derived from
transaction id.

Also this CL fixes some tests.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=713307

[nodir, 2017-05-01 21:33:02]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:33:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nodir, 2017-05-01 21:34:33]

PTAL

[commit-bot: I haz the power, 2017-05-01 21:39:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 21:39:58]

Dry run: Try jobs failed on following builders:
  Luci-py Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/35dec32025d07410)

[M-A Ruel, 2017-05-03 01:33:21]

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:84: task_id =
memcache.get(transaction_id_to_memcache_key(transaction_id))
use a namespace, it's simpler. Then no need for
transaction_id_to_memcache_key().

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:86: return None, None
This makes the contract a bit scary though. :/ I mean, memcache can be flushed
anytime. I guess it's fine for the general case.

For example, using a duplicate transaction id once per day may work when
memcache is small, but fail when used at a slightly higher frequency.

Technically speaking the "right" thing is to do an DB get but it's much more
expensive, since on steady state it is a cold (uncached) miss.

Overall, I'm fine, it just needs to be documented with a lot of caveats.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:355: 'only one of task_id or
transaction_id must be specified')
That can't work because path is '{task_id}/cancel'. The test works because it
bypasses the URI router.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints_test.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1336: self.mock(utils,
'enqueue_task', enqueue_task_mock)
enqueue_task is already mocked above, this is not needed. Instead modify line 75
to enqueue the pubsub messages.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1415: self.mock(utils,
'enqueue_task', enqueue_task_mock)
same

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1470: self.mock(utils,
'enqueue_task', enqueue_task_mock)
same

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/server/t...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/server/t...
appengine/swarming/server/task_request.py:665: exclude = exclude or []
generally prefer to take a copy of list passed as an input, so if it's a global
variable it is not modified. e.g.

exclude = (exclude or [])[:]

this is not efficient but this is python. :)

[nodir, 2017-05-05 18:11:28]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 18:11:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nodir, 2017-05-05 18:11:38]

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:84: task_id =
memcache.get(transaction_id_to_memcache_key(transaction_id))
On 2017/05/03 01:33:20, M-A Ruel wrote:
> use a namespace, it's simpler. Then no need for
> transaction_id_to_memcache_key().

Done.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:86: return None, None
On 2017/05/03 01:33:20, M-A Ruel wrote:
> This makes the contract a bit scary though. :/ I mean, memcache can be flushed
> anytime. I guess it's fine for the general case.
> 
> For example, using a duplicate transaction id once per day may work when
> memcache is small, but fail when used at a slightly higher frequency.
> 
> Technically speaking the "right" thing is to do an DB get but it's much more
> expensive, since on steady state it is a cold (uncached) miss.
> 
> Overall, I'm fine, it just needs to be documented with a lot of caveats.

maybe set timeout to 1min?

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:355: 'only one of task_id or
transaction_id must be specified')
On 2017/05/03 01:33:20, M-A Ruel wrote:
> That can't work because path is '{task_id}/cancel'. The test works because it
> bypasses the URI router.

ugh, should I define a new API then? cancel_by_transaction_id or something?
the problem we are trying to solve here is not knowing the task id and passing
dummy task id is not great

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints_test.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1336: self.mock(utils,
'enqueue_task', enqueue_task_mock)
On 2017/05/03 01:33:20, M-A Ruel wrote:
> enqueue_task is already mocked above, this is not needed. Instead modify line
75
> to enqueue the pubsub messages.

Done

this code was copied from test_cancel_forbidden. Also, there were 4 mockings of
enqueue_task before this CL
https://cs.chromium.org/chromium/infra/luci/appengine/swarming/handlers_endpo...
so there is still code that mocks enqueue_task second time

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1415: self.mock(utils,
'enqueue_task', enqueue_task_mock)
On 2017/05/03 01:33:20, M-A Ruel wrote:
> same

Done.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints_test.py:1470: self.mock(utils,
'enqueue_task', enqueue_task_mock)
On 2017/05/03 01:33:20, M-A Ruel wrote:
> same

Done.

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/server/t...
File appengine/swarming/server/task_request.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/server/t...
appengine/swarming/server/task_request.py:665: exclude = exclude or []
On 2017/05/03 01:33:20, M-A Ruel wrote:
> generally prefer to take a copy of list passed as an input, so if it's a
global
> variable it is not modified. e.g.
> 
> exclude = (exclude or [])[:]
> 
> this is not efficient but this is python. :)

Done, dumb mistake

[M-A Ruel, 2017-05-05 18:15:33]

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:355: 'only one of task_id or
transaction_id must be specified')
On 2017/05/05 18:11:38, nodir wrote:
> On 2017/05/03 01:33:20, M-A Ruel wrote:
> > That can't work because path is '{task_id}/cancel'. The test works because
it
> > bypasses the URI router.
> 
> ugh, should I define a new API then? cancel_by_transaction_id or something?
> the problem we are trying to solve here is not knowing the task id and passing
> dummy task id is not great

Yes I prefer a new API. If we get rid of the feature then it's simpler to delete
a single method. :)

[commit-bot: I haz the power, 2017-05-05 18:16:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 18:16:22]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-05-05 18:39:43]

The CQ bit was checked by nodir@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 18:39:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nodir, 2017-05-05 18:41:37]

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2856733002/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:355: 'only one of task_id or
transaction_id must be specified')
On 2017/05/05 18:15:33, M-A Ruel wrote:
> On 2017/05/05 18:11:38, nodir wrote:
> > On 2017/05/03 01:33:20, M-A Ruel wrote:
> > > That can't work because path is '{task_id}/cancel'. The test works because
> it
> > > bypasses the URI router.
> > 
> > ugh, should I define a new API then? cancel_by_transaction_id or something?
> > the problem we are trying to solve here is not knowing the task id and
passing
> > dummy task id is not great
> 
> Yes I prefer a new API. If we get rid of the feature then it's simpler to
delete
> a single method. :)

Done.

[commit-bot: I haz the power, 2017-05-05 18:43:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 18:43:41]

Dry run: This issue passed the CQ dry run.

[nodir, 2017-05-09 15:05:42]

ping

[M-A Ruel, 2017-05-10 13:41:25]

https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:242: # If two NewTaskRequests use the same
transaction ID, requests must have all
Hey, thinking about it, what about if all attributes are the same, why not use
the hash of the request minus created_ts and using (expiration_ts-created_ts) as
a relative value? Then no need for an arbitrary ID.

Right now we use TaskRequest.properties_hash for task result deduping but what
I'm thinking about is TaskRequest.request_hash for task request deduping. This
would be much more generic and would be transparent to the user.

Give me a day to think about it. I think it'd work quite fine, as all the task
retry logic we have already use a different task name.

[nodir, 2017-05-10 14:01:04]

On 2017/05/10 13:41:25, M-A Ruel wrote:
>
https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
> File appengine/swarming/swarming_rpcs.py (right):
> 
>
https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
> appengine/swarming/swarming_rpcs.py:242: # If two NewTaskRequests use the same
> transaction ID, requests must have all
> Hey, thinking about it, what about if all attributes are the same, why not use
> the hash of the request minus created_ts and using (expiration_ts-created_ts)
as
> a relative value? Then no need for an arbitrary ID.
> 
> Right now we use TaskRequest.properties_hash for task result deduping but what
> I'm thinking about is TaskRequest.request_hash for task request deduping. This
> would be much more generic and would be transparent to the user.
> 
> Give me a day to think about it. I think it'd work quite fine, as all the task
> retry logic we have already use a different task name.

It would work for LUCI because tasks have different buildbucket ids,  but I
worry that for others swarming may start deduping non-idempotent tasks that look
the same

[M-A Ruel, 2017-05-10 15:48:50]

On 2017/05/10 14:01:04, nodir wrote:
> On 2017/05/10 13:41:25, M-A Ruel wrote:
> >
>
https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
> > File appengine/swarming/swarming_rpcs.py (right):
> > 
> >
>
https://codereview.chromium.org/2856733002/diff/80001/appengine/swarming/swar...
> > appengine/swarming/swarming_rpcs.py:242: # If two NewTaskRequests use the
same
> > transaction ID, requests must have all
> > Hey, thinking about it, what about if all attributes are the same, why not
use
> > the hash of the request minus created_ts and using
(expiration_ts-created_ts)
> as
> > a relative value? Then no need for an arbitrary ID.
> > 
> > Right now we use TaskRequest.properties_hash for task result deduping but
what
> > I'm thinking about is TaskRequest.request_hash for task request deduping.
This
> > would be much more generic and would be transparent to the user.
> > 
> > Give me a day to think about it. I think it'd work quite fine, as all the
task
> > retry logic we have already use a different task name.
> 
> It would work for LUCI because tasks have different buildbucket ids,  but I
> worry that for others swarming may start deduping non-idempotent tasks that
look
> the same

We can filter this on a short window, 1~5 minutes.