Allow botlist API call to respond to quarantined: and is_dead:

Allow botlist API call to respond to quarantined: and is_dead:

Supports "true", "false", and "none" for both cases

BUG=631047
Committed: https://github.com/luci/luci-py/commit/7144f145b3226faac134b5adffd91dce1844ebc0

[kjlubick, 2016-08-08 17:15:15]

kjlubick@google.com changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2016-08-08 19:29:51]

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

https://codereview.chromium.org/2220373003/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:630: if parts[0] == 'quarantined':
Please change BotsRequest with formal arguments instead.

[kjlubick, 2016-08-08 20:12:05]

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

https://codereview.chromium.org/2220373003/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:630: if parts[0] == 'quarantined':
On 2016/08/08 at 19:29:51, M-A Ruel wrote:
> Please change BotsRequest with formal arguments instead.

Done.  Should I update BotsCount (line 657ish) to respond to these new
arguments?

[M-A Ruel, 2016-08-08 20:28:00]

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

https://codereview.chromium.org/2220373003/diff/1/appengine/swarming/handlers...
appengine/swarming/handlers_endpoints.py:630: if parts[0] == 'quarantined':
On 2016/08/08 20:12:05, kjlubick wrote:
> On 2016/08/08 at 19:29:51, M-A Ruel wrote:
> > Please change BotsRequest with formal arguments instead.
> 
> Done.  Should I update BotsCount (line 657ish) to respond to these new
> arguments?

Yes it should match.

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

https://codereview.chromium.org/2220373003/diff/20001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:311: quarantined = messages.StringField(4,
default='none')
I was more thinking about tree-state-bool. Since there's none, maybe define one
with
https://cloud.google.com/appengine/docs/python/tools/protorpc/messages/enumfi...
?

class ThreeStateBool(messages.Enum):  
  FALSE = 1
  TRUE = 2
  NONE = 3

I think we may already be using BoolField implicitly this way but I prefer
EnumField to StringField here.

[kjlubick, 2016-08-09 13:27:55]

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

https://codereview.chromium.org/2220373003/diff/20001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:311: quarantined = messages.StringField(4,
default='none')
On 2016/08/08 at 20:28:00, M-A Ruel wrote:
> I was more thinking about tree-state-bool. Since there's none, maybe define
one with
https://cloud.google.com/appengine/docs/python/tools/protorpc/messages/enumfi...
?
> 
> class ThreeStateBool(messages.Enum):  
>   FALSE = 1
>   TRUE = 2
>   NONE = 3
> 
> I think we may already be using BoolField implicitly this way but I prefer
EnumField to StringField here.

Done.

[M-A Ruel, 2016-08-09 13:57:27]

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints_test.py (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints_test.py:1380: self.maxDiff = None
remove before committing, note that calling the test with -v yeilds to th same
outcome.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/inde...
File appengine/swarming/index.yaml (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/inde...
appengine/swarming/index.yaml:30: - kind: BotInfo
I'm a bit concerned by the explosion of indexes here.

A way to make it work is to add to BotInfo:

meta = ndb.ComputerProperty(_calc)


def _calc(obj):
  return calculate(obj.quarantined, obj.busy)


def calculate(quarantined, busy):
  return int(quarantined) << 2 || int(busy) << 1 | 1


this will remove one axis from the explosion of combinations.

wdyt?

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

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:54: NONE = 3
def to_bool(tree_state):
  if tree_state == ThreeStateBool.FALSE:
    return False
  if tree_state == ThreeStateBool.TRUE:
    return True


then use this function in handlers_endpoints. This way, you could do:
val = to_bool(request.quarantined)
if val is not None:
  q = q.filter(... == val)

[M-A Ruel, 2016-08-09 17:10:35]

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:612: def _build_query(self, request,
now):
"""Returns a ndb.Query for BotInfo based on the request."""

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:615: if not ':' in d:
This check is not necessary, the len(parts) != 2 already checks for this
condition. So remove lines 615-616.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:627: dt = datetime.timedelta(
according to line 617, this fits one line.

[kjlubick, 2016-08-09 17:41:32]

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:612: def _build_query(self, request,
now):
On 2016/08/09 at 17:10:34, M-A Ruel wrote:
> """Returns a ndb.Query for BotInfo based on the request."""

Done.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:615: if not ':' in d:
On 2016/08/09 at 17:10:34, M-A Ruel wrote:
> This check is not necessary, the len(parts) != 2 already checks for this
condition. So remove lines 615-616.

Done.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:627: dt = datetime.timedelta(
On 2016/08/09 at 17:10:34, M-A Ruel wrote:
> according to line 617, this fits one line.

Done.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints_test.py (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints_test.py:1380: self.maxDiff = None
On 2016/08/09 at 13:57:27, M-A Ruel wrote:
> remove before committing, note that calling the test with -v yeilds to th same
outcome.

Done.

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/inde...
File appengine/swarming/index.yaml (right):

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/inde...
appengine/swarming/index.yaml:30: - kind: BotInfo
On 2016/08/09 at 13:57:27, M-A Ruel wrote:
> I'm a bit concerned by the explosion of indexes here.
> 
> A way to make it work is to add to BotInfo:
> 
> meta = ndb.ComputerProperty(_calc)
> 
> 
> def _calc(obj):
>   return calculate(obj.quarantined, obj.busy)
> 
> 
> def calculate(quarantined, busy):
>   return int(quarantined) << 2 || int(busy) << 1 | 1
> 
> 
> this will remove one axis from the explosion of combinations.
> 
> wdyt?

Abandoned, as per our discussion.

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

https://codereview.chromium.org/2220373003/diff/40001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:54: NONE = 3
On 2016/08/09 at 13:57:27, M-A Ruel wrote:
> def to_bool(tree_state):
>   if tree_state == ThreeStateBool.FALSE:
>     return False
>   if tree_state == ThreeStateBool.TRUE:
>     return True
> 
> 
> then use this function in handlers_endpoints. This way, you could do:
> val = to_bool(request.quarantined)
> if val is not None:
>   q = q.filter(... == val)

Done.

[M-A Ruel, 2016-08-09 18:05:27]

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:612: def _filter_dimensions(self, q,
request):
maybe worth passing request.dimensions as an argument instead of request, so
it's cleaner here.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:621: def _filter_availability(self, q,
request, now):
same here
def _filter_availability(self, q, is_busy, quarantined, is_dead, now):

Actually, these functions should them be moved to server/bot_management.py. This
way, if we need to refactor the way the entity BotInfo is stored to optimize the
indexes, most of the changes are self-contained in bot_management.py.

From bot_management.py, you should raise ValueError(). You can use a
try/except+raise to convert to endpoints.BadRequestException().

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:667: q = bot_management.BotInfo.query()
you can probably inline lines 667 and 668, and same for 647 and 648.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:677: f_dead = q.filter(
you can do:
f_dead = self._filter_availability(q, True, False).count_async()

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:679: f_quarantined = q.filter(
same here

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

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:55: 
two empty lines between file level symbols.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:56: def to_bool(three_state):
you could add:

if three_state in (None, True, False):
  return three_state

this simplify the call sites.

[kjlubick, 2016-08-09 19:25:04]

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:612: def _filter_dimensions(self, q,
request):
On 2016/08/09 at 18:05:27, M-A Ruel wrote:
> maybe worth passing request.dimensions as an argument instead of request, so
it's cleaner here.

Done.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:621: def _filter_availability(self, q,
request, now):
On 2016/08/09 at 18:05:27, M-A Ruel wrote:
> same here
> def _filter_availability(self, q, is_busy, quarantined, is_dead, now):
> 
> Actually, these functions should them be moved to server/bot_management.py.
This way, if we need to refactor the way the entity BotInfo is stored to
optimize the indexes, most of the changes are self-contained in
bot_management.py.
> 
> From bot_management.py, you should raise ValueError(). You can use a
try/except+raise to convert to endpoints.BadRequestException().

Done.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:667: q = bot_management.BotInfo.query()
On 2016/08/09 at 18:05:26, M-A Ruel wrote:
> you can probably inline lines 667 and 668, and same for 647 and 648.

Cannot, it is too long

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:677: f_dead = q.filter(
On 2016/08/09 at 18:05:26, M-A Ruel wrote:
> you can do:
> f_dead = self._filter_availability(q, True, False).count_async()

Done.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:679: f_quarantined = q.filter(
On 2016/08/09 at 18:05:27, M-A Ruel wrote:
> same here

Done.

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

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:55: 
On 2016/08/09 at 18:05:27, M-A Ruel wrote:
> two empty lines between file level symbols.

Done.

https://codereview.chromium.org/2220373003/diff/60001/appengine/swarming/swar...
appengine/swarming/swarming_rpcs.py:56: def to_bool(three_state):
On 2016/08/09 at 18:05:27, M-A Ruel wrote:
> you could add:
> 
> if three_state in (None, True, False):
>   return three_state
> 
> this simplify the call sites.

Done.

[M-A Ruel, 2016-08-09 20:23:14]

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:627: q =
bot_management.filter_availability(q, request.quarantined,
I don't like this kind of alignment, indent all arguments at +4 or none.

q = bot_management.filter_availability(
    q, request.quarantined, request.is_dead, now)

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:630: raise
endpoints.BadRequestException('%s' % e)
str(e)

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:641: swarming_rpcs.BotsRequest,
swarming_rpcs.BotsCount,
BotsRequest shouldn't be used here. Only dimensions is needed, nothing else. So
create another BotsCountRequest message. This is not a breaking change _because_
it is a GET method.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_management.py:42: 
remove one line

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_management.py:285: val =
swarming_rpcs.to_bool(quarantined)
actually that's a layering violation :/  Here only python types should be used.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_rpcs.py:64: 
2 lines here

[kjlubick, 2016-08-09 20:44:44]

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:627: q =
bot_management.filter_availability(q, request.quarantined,
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> I don't like this kind of alignment, indent all arguments at +4 or none.
> 
> q = bot_management.filter_availability(
>     q, request.quarantined, request.is_dead, now)

Done.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:630: raise
endpoints.BadRequestException('%s' % e)
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> str(e)

Done.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:641: swarming_rpcs.BotsRequest,
swarming_rpcs.BotsCount,
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> BotsRequest shouldn't be used here. Only dimensions is needed, nothing else.
So create another BotsCountRequest message. This is not a breaking change
_because_ it is a GET method.

What if I want to count all bots that aren't quarantined or dead, i.e.
is_dead=FALSE&quarantined=FALSE

I think we can still use the BotsRequest here.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
File appengine/swarming/server/bot_management.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_management.py:42: 
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> remove one line

Done.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/ser...
appengine/swarming/server/bot_management.py:285: val =
swarming_rpcs.to_bool(quarantined)
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> actually that's a layering violation :/  Here only python types should be
used.

Done.

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/swa...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/swa...
appengine/swarming/swarming_rpcs.py:64: 
On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> 2 lines here

Done.

[M-A Ruel, 2016-08-09 21:15:32]

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:641: swarming_rpcs.BotsRequest,
swarming_rpcs.BotsCount,
On 2016/08/09 20:44:43, kjlubick wrote:
> On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> > BotsRequest shouldn't be used here. Only dimensions is needed, nothing else.
> So create another BotsCountRequest message. This is not a breaking change
> _because_ it is a GET method.
> 
> What if I want to count all bots that aren't quarantined or dead, i.e.
> is_dead=FALSE&quarantined=FALSE
> 
> I think we can still use the BotsRequest here.

It's already part of BoutsCount.

[kjlubick, 2016-08-10 12:22:20]

On 2016/08/09 at 21:15:32, maruel wrote:
>
https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
> File appengine/swarming/handlers_endpoints.py (right):
> 
>
https://codereview.chromium.org/2220373003/diff/120001/appengine/swarming/han...
> appengine/swarming/handlers_endpoints.py:641: swarming_rpcs.BotsRequest,
swarming_rpcs.BotsCount,
> On 2016/08/09 20:44:43, kjlubick wrote:
> > On 2016/08/09 at 20:23:13, M-A Ruel wrote:
> > > BotsRequest shouldn't be used here. Only dimensions is needed, nothing
else.
> > So create another BotsCountRequest message. This is not a breaking change
> > _because_ it is a GET method.
> > 
> > What if I want to count all bots that aren't quarantined or dead, i.e.
> > is_dead=FALSE&quarantined=FALSE
> > 
> > I think we can still use the BotsRequest here.
> 
> It's already part of BoutsCount.

Done.

[M-A Ruel, 2016-08-10 12:38:33]

lgtm

https://codereview.chromium.org/2220373003/diff/180001/appengine/swarming/han...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2220373003/diff/180001/appengine/swarming/han...
appengine/swarming/handlers_endpoints.py:668: 
remove

https://codereview.chromium.org/2220373003/diff/180001/appengine/swarming/swa...
File appengine/swarming/swarming_rpcs.py (right):

https://codereview.chromium.org/2220373003/diff/180001/appengine/swarming/swa...
appengine/swarming/swarming_rpcs.py:335: 
add one line

[kjlubick, 2016-08-10 13:18:15]

The CQ bit was checked by kjlubick@google.com

[kjlubick, 2016-08-10 13:18:15]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/2220373003/#ps200001
(title: "Address nit")

[commit-bot: I haz the power, 2016-08-10 13:18:17]

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

[commit-bot: I haz the power, 2016-08-10 13:21:34]

Description was changed from

==========
Allow botlist API call to respond to quarantined: and is_dead:

Supports "true", "false", and "none" for both cases

BUG=631047
==========

to

==========
Allow botlist API call to respond to quarantined: and is_dead:

Supports "true", "false", and "none" for both cases

BUG=631047

Committed:
https://github.com/luci/luci-py/commit/7144f145b3226faac134b5adffd91dce1844ebc0
==========

[commit-bot: I haz the power, 2016-08-10 13:21:35]

Committed patchset #11 (id:200001) as
https://github.com/luci/luci-py/commit/7144f145b3226faac134b5adffd91dce1844ebc0