Have service_manager start a cloudtail attached to each service it starts.

Have service_manager start a cloudtail attached to each service it starts.

This is enabled by a flag and defaults to disabled

BUG=492404
Committed: https://chromium.googlesource.com/infra/infra/+/a3824c3eb58713bf4320bc458c2fde231010a498

[dsansome, 2015-09-23 04:45:21]

dsansome@chromium.org changed reviewers:
+ vadimsh@chromium.org

[Vadim Sh., 2015-09-23 17:40:22]

looks reasonable

https://codereview.chromium.org/1365583002/diff/40001/infra/libs/service_util...
File infra/libs/service_utils/_daemon_nix.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/libs/service_util...
infra/libs/service_utils/_daemon_nix.py:110: for fd in reversed(range(2048)):
(I don't remember this code)

Why 2048? There's no guarantee it's all of them...

Reading subprocess.py I found following interesting snippets:

try:
    MAXFD = os.sysconf("SC_OPEN_MAX")
except:
    MAXFD = 256

and

if hasattr(os, 'closerange'):  # Introduced in 2.6
    @staticmethod
    def _closerange(fd_low, fd_high):
        os.closerange(fd_low, fd_high)
else:
    @staticmethod
    def _closerange(fd_low, fd_high):
        for fd in xrange(fd_low, fd_high):
            while True:
                try:
                    os.close(fd)
                except (OSError, IOError), e:
                    if e.errno == errno.EINTR:
                        continue
                    break

I didn't know close_fds implementation is so... unsophisticated.

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
File infra/services/service_manager/__main__.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
infra/services/service_manager/__main__.py:28: default_cloudtail_path =
'C:\\infra-tools\\cloudtail.exe'
I propose to bundle cloudtail executable with infra_python package. That way it
will be easier to deploy it everywhere where infra_python is used.

Simplest way to do so is to add one more line to
https://chromium.googlesource.com/infra/infra/+/master/build/packages/infra_p...:

- file: go/bin/cloudtail${exe_suffix}

It works because build.py script compiles everything before doing anything else.

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
File infra/services/service_manager/service.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
infra/services/service_manager/service.py:210: '--log-id', self.name,
they have limit on number of these, as far as I understand. Since we have <2048
services, we should be fine :)

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments.py
File infra_libs/experiments.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:25: for label, value in labels.iteritems():
sorted(labels.iteritems())

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:31: print hash(digest) % 100
remove :)

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:38: 'host': socket.getfqdn(),
This thing can return nonsense (e.g. 1.1.1.1.1.1.arp.something-something) or
throw exceptions if it is called before network is fully up. Just saying :) I
think our upstart config sets up a dependency on eth0.

[Vadim Sh., 2015-09-24 01:37:31]

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
File infra/services/service_manager/__main__.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
infra/services/service_manager/__main__.py:28: default_cloudtail_path =
'C:\\infra-tools\\cloudtail.exe'
On 2015/09/23 17:40:21, Vadim Sh. wrote:
> I propose to bundle cloudtail executable with infra_python package. That way
it
> will be easier to deploy it everywhere where infra_python is used.
> 
> Simplest way to do so is to add one more line to
>
https://chromium.googlesource.com/infra/infra/+/master/build/packages/infra_p...:
> 
> - file: go/bin/cloudtail${exe_suffix}
> 
> It works because build.py script compiles everything before doing anything
else.

There's a chance infra.git will have infra/bin/<binary-stuff> directory soon
with pinned versions of binaries that infra.git depend on. We can put cloudtail
there too. That way one can roll infra_python (python part) without deploying
new cloudtail part. And developers will have access to cloudtail too without
building it.

+Dan who works on this. Basically infra.git DEPS hook (post checkout hook
essentially) will bootstrap cipd client and fetch a known set of pinned packages
with development-related goodies (like "protoc" etc).

[dsansome, 2015-09-24 04:34:13]

https://codereview.chromium.org/1365583002/diff/40001/infra/libs/service_util...
File infra/libs/service_utils/_daemon_nix.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/libs/service_util...
infra/libs/service_utils/_daemon_nix.py:110: for fd in reversed(range(2048)):
On 2015/09/23 17:40:21, Vadim Sh. wrote:
> (I don't remember this code)
> 
> Why 2048? There's no guarantee it's all of them...

I just picked a high number :(

> Reading subprocess.py I found following interesting snippets:
> 
> try:
>     MAXFD = os.sysconf("SC_OPEN_MAX")
> except:
>     MAXFD = 256
> 
> and
> 
> if hasattr(os, 'closerange'):  # Introduced in 2.6
>     @staticmethod
>     def _closerange(fd_low, fd_high):
>         os.closerange(fd_low, fd_high)
> else:
>     @staticmethod
>     def _closerange(fd_low, fd_high):
>         for fd in xrange(fd_low, fd_high):
>             while True:
>                 try:
>                     os.close(fd)
>                 except (OSError, IOError), e:
>                     if e.errno == errno.EINTR:
>                         continue
>                     break
> 
> I didn't know close_fds implementation is so... unsophisticated.

It looks like Python 3 is a bit smarter and looks in /proc/self/fd for a list of
things to close:
https://github.com/python/cpython/blob/829b49cbd2e4b1d573470da79ca844b730120f...

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
File infra/services/service_manager/__main__.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
infra/services/service_manager/__main__.py:28: default_cloudtail_path =
'C:\\infra-tools\\cloudtail.exe'
On 2015/09/24 01:37:31, Vadim Sh. wrote:
> On 2015/09/23 17:40:21, Vadim Sh. wrote:
> > I propose to bundle cloudtail executable with infra_python package. That way
> it
> > will be easier to deploy it everywhere where infra_python is used.
> > 
> > Simplest way to do so is to add one more line to
> >
>
https://chromium.googlesource.com/infra/infra/+/master/build/packages/infra_p...:
> > 
> > - file: go/bin/cloudtail${exe_suffix}
> > 
> > It works because build.py script compiles everything before doing anything
> else.
> 
> There's a chance infra.git will have infra/bin/<binary-stuff> directory soon
> with pinned versions of binaries that infra.git depend on. We can put
cloudtail
> there too. That way one can roll infra_python (python part) without deploying
> new cloudtail part. And developers will have access to cloudtail too without
> building it.
> 
> +Dan who works on this. Basically infra.git DEPS hook (post checkout hook
> essentially) will bootstrap cipd client and fetch a known set of pinned
packages
> with development-related goodies (like "protoc" etc).

Sounds good!  For now I'll bundle it in the infra_python package as you
suggested, then later we can use the one in infra/bin.

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
File infra/services/service_manager/service.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra/services/service_...
infra/services/service_manager/service.py:210: '--log-id', self.name,
On 2015/09/23 17:40:21, Vadim Sh. wrote:
> they have limit on number of these, as far as I understand. Since we have
<2048
> services, we should be fine :)

Yep yep

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments.py
File infra_libs/experiments.py (right):

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:25: for label, value in labels.iteritems():
On 2015/09/23 17:40:22, Vadim Sh. wrote:
> sorted(labels.iteritems())

Done.

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:31: print hash(digest) % 100
On 2015/09/23 17:40:22, Vadim Sh. wrote:
> remove :)

Oops!

https://codereview.chromium.org/1365583002/diff/40001/infra_libs/experiments....
infra_libs/experiments.py:38: 'host': socket.getfqdn(),
On 2015/09/23 17:40:21, Vadim Sh. wrote:
> This thing can return nonsense (e.g. 1.1.1.1.1.1.arp.something-something) or
> throw exceptions if it is called before network is fully up. Just saying :) I
> think our upstart config sets up a dependency on eth0.

Nonsense is fine - it's just being hashed anyway.
ts_mon.config also does a socket.getfqdn before this, so if that one worked this
one will too :)

[Vadim Sh., 2015-09-24 18:16:44]

lgtm

[dsansome, 2015-09-25 00:51:25]

The CQ bit was checked by dsansome@chromium.org

[commit-bot: I haz the power, 2015-09-25 00:51:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365583002/60001

[commit-bot: I haz the power, 2015-09-25 00:55:57]

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

[commit-bot: I haz the power, 2015-09-25 00:55:58]

Try jobs failed on following builders:
  Infra Win Tester on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

[dsansome, 2015-09-25 01:07:17]

On 2015/09/25 00:55:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   Infra Win Tester on tryserver.infra (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)

Makes sense: the md5 hash is the same, but hash() returns different values on
32-bit than 64-bit.  I'll hack the test...

[Vadim Sh., 2015-09-25 01:11:21]

On 2015/09/25 01:07:17, dsansome wrote:
> On 2015/09/25 00:55:58, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   Infra Win Tester on tryserver.infra (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)
> 
> Makes sense: the md5 hash is the same, but hash() returns different values on
> 32-bit than 64-bit.  I'll hack the test...

just return md5[0] + (md5[1] << 8) + (md5[2] << 16) +  (md5[3] << 24)

first 4 bytes are random enough already

[dsansome, 2015-09-25 01:19:18]

On 2015/09/25 01:11:21, Vadim Sh. wrote:
> On 2015/09/25 01:07:17, dsansome wrote:
> > On 2015/09/25 00:55:58, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   Infra Win Tester on tryserver.infra (JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Win%20Tester/bu...)
> > 
> > Makes sense: the md5 hash is the same, but hash() returns different values
on
> > 32-bit than 64-bit.  I'll hack the test...
> 
> just return md5[0] + (md5[1] << 8) + (md5[2] << 16) +  (md5[3] << 24)
> 
> first 4 bytes are random enough already

Oh yeah good idea :)  Done with struct.unpack instead.

[dsansome, 2015-09-25 01:19:37]

The CQ bit was checked by dsansome@chromium.org

[dsansome, 2015-09-25 01:19:37]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1365583002/#ps80001
(title: "Don't use hash()")

[commit-bot: I haz the power, 2015-09-25 01:19:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1365583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1365583002/80001

[commit-bot: I haz the power, 2015-09-25 01:23:35]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/a3824c3eb58713bf4320bc458c2fd...