Merge ~ajorgens/cloud-init:pipe-cat into cloud-init:master

[Andrew Jorgensen]

util: Disconnect subp from a tty

Some processes are less chatty when they don't find a terminal. Yum is a prime example. Without this it can display progress bars and fill up your console logs with garbage, making debugging harder.

util: Disconnect subp from a tty

Some processes are less chatty when they don't find a terminal. Yum is a prime example. Without this it can display progress bars and fill up your console logs with garbage, making debugging harder.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:c97434e6c94690a0df292a70fd5ed6f5f0a389fa
https://jenkins.ubuntu.com/server/job/cloud-init-ci/517/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/517
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/517
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/517
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/517
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/517

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/517/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

It seems like there is probably a better (or at very least less expensive) way to do this than to incur the cost of a subprocess.

I suspect
a.) change 'yum' to be invoked with '--quiet' or the like
b.) close and re-open stdout or something like that. yum is probably using 'isatty' to change its behavior.

[Andrew Jorgensen (ajorgens)]

a.) Unfortunately, asking Yum to be quiet means it won't give you any of the debugging information you need when it goes wrong.

b.) Possibly there's a better way to disconnect it from the terminal, but what isatty does is ask the system if stdout is associated with a terminal, so unless you make stdout something other than the one associated with the terminal it will answer that it's a tty. In other words, it has to be piped through something. Piping it through cat seemed the lesser of two evils compared to spinning off a thread (or greenlet or whatever) to consume stdout and spool it to the console.

I'm definitely open to advice on how to do this a better way, if you want to ask around.

[Andrew Jorgensen (ajorgens)]

Hmm. I think I've found a better way, pending more testing.

10676d5... by Andrew Jorgensen on 2017-06-14

read/write stdout instead of piping through cat

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:625846d4afb6c97eecb7c58682ad266cbf4118e5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/525/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/525/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/525/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/525/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/525/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/525/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/525/rebuild

review: Needs Fixing (continuous-integration)

[Andrew Jorgensen (ajorgens)]

New revision avoiding spawning cat, but emulating subprocess.communicate() instead.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:10676d5e7eef7c489c4d0c2c75ea9b37d828e1b2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/526/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/526/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/526/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/526/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/526/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/526/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/526/rebuild

review: Needs Fixing (continuous-integration)

4d09af6... by Andrew Jorgensen on 2017-06-14

Use a no_tty option instead of always disconnecting from tty

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:4d09af6deaf85811cc95fb753d0f41115f294ec6
https://jenkins.ubuntu.com/server/job/cloud-init-ci/527/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/527
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/527
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/527
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/527
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/527

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/527/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Andrew,
thanks. I think the code looks good
But a few things

a.) maybe 'detach_tty' as the name? 'no_tty=False' seems to imply that there would be a tty, when we are not actually creating one.
b.) need some sort of test, at least to push it through here. it might be tricky to do, but i'd like to run a subp 'detach_tty=False' and get some subprocess to believe it is a tty.
   subp(['bash', '-c', '[ "-t" 1 ] && echo attached to tty || echo not attached to tty'])
   the difficulty is that in any c-i or automated environment, we're probably not attached to a tty. but we do need some test of this code path.
c.) i think best to take the detach_tty path only if we *have* a tty.
   if os.isatty(sys.stdout):
      ... go the detach route
   else:
      ... no need to detach

[Scott Moser (smoser)]

we need *some* test of this though, even if we have to mock os.isatty to get reliable tests.

review: Needs Fixing

[Andrew Jorgensen (ajorgens)]

Agreed we need a unit test of this code path. I'll see what I can do.

[Scott Moser (smoser)]

I'll move this to 'work in progress' for now set back to Needs Review when you're ready.
thanks.

[Andrew Jorgensen (ajorgens)]

So... I spent a lot of time yesterday trying to use pty.openpty() to make a unit test for this, and I give up, at least for now. I'm unsure if the code is bad or if the unit test is impossible.

In Amazon Linux we're using the redirect_output thing to pipe everything through tee anyway, so this is likely a non-issue for us. If I feel ambitious or have a great idea how else to solve this I might do that some day, but for now I'm dropping it.

7941e58... by Andrew Jorgensen on 2017-06-16

work-in-progress - broken

Unmerged commits

7941e58... by Andrew Jorgensen on 2017-06-16

work-in-progress - broken

4d09af6... by Andrew Jorgensen on 2017-06-14

Use a no_tty option instead of always disconnecting from tty

10676d5... by Andrew Jorgensen on 2017-06-14

read/write stdout instead of piping through cat

c97434e... by Andrew Jorgensen on 2017-06-12

rhel: Use the pipe_cat option for Yum commands

5675c65... by Andrew Jorgensen on 2013-08-29

util: Add an option for subp to pipe the process through cat

Some processes are less chatty when piped through cat, because they won't
detect a terminal (yum being a prime example).

Reviewed-by: Matt Nierzwicki <email address hidden>
Reviewed-by: Ethan Faust <email address hidden>
[<email address hidden>: rebase onto 0.7.9]

diff --git a/cloudinit/distros/rhel.py b/cloudinit/distros/rhel.py
index 1fecb61..03f02de 100644
--- a/cloudinit/distros/rhel.py
+++ b/cloudinit/distros/rhel.py
@@ -212,7 +212,7 @@ class Distro(distros.Distro):
         cmd.extend(pkglist)
 
         # Allow the output of this to flow outwards (ie not be captured)
-        util.subp(cmd, capture=False)
+        util.subp(cmd, capture=False, detach_tty=True)
 
     def update_package_sources(self):
         self._runner.run("update-sources", self.package_command,
diff --git a/cloudinit/util.py b/cloudinit/util.py
index c93b6d7..9f5715d 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -1,10 +1,12 @@
 # Copyright (C) 2012 Canonical Ltd.
 # Copyright (C) 2012, 2013 Hewlett-Packard Development Company, L.P.
 # Copyright (C) 2012 Yahoo! Inc.
+# Copyright (C) 2017 Amazon.com, Inc. or its affiliates.
 #
 # Author: Scott Moser <[e-mail]>
 # Author: Juerg Haefliger <[e-mail]>
 # Author: Joshua Harlow <[e-mail]>
+# Author: Andrew Jorgensen <[e-mail]>
 #
 # This file is part of cloud-init. See LICENSE file for license information.
 
@@ -1771,13 +1773,18 @@ def delete_dir_contents(dirname):
 
 
 def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
-         logstring=False, decode="replace", target=None, update_env=None):
+         logstring=False, decode="replace", target=None, update_env=None,
+         detach_tty=False):
 
     # not supported in cloud-init (yet), for now kept in the call signature
     # to ease maintaining code shared between cloud-init and curtin
     if target is not None:
         raise ValueError("target arg not supported by cloud-init")
 
+    if detach_tty and not sys.stdout.isatty():
+        LOG.debug("No TTY detected, ignoring detach_tty")
+        detach_tty = False
+
     if rcs is None:
         rcs = [0]
 
@@ -1803,8 +1810,9 @@ def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
         stdin = None
         stdout = None
         stderr = None
-        if capture:
+        if detach_tty or capture:
             stdout = subprocess.PIPE
+        if capture:
             stderr = subprocess.PIPE
         if data is None:
             # using devnull assures any reads get null, rather
@@ -1819,7 +1827,25 @@ def subp(args, data=None, rcs=None, env=None, capture=True, shell=False,
         sp = subprocess.Popen(args, stdout=stdout,
                               stderr=stderr, stdin=stdin,
                               env=env, shell=shell)
-        (out, err) = sp.communicate(data)
+
+        # Some processes are less chatty when disconnected from a tty.
+        # (capture=True already doesn't have a tty)
+        if detach_tty and not capture:
+            (out, err) = (None, None)
+            if data is not None:
+                try:
+                    sp.stdin.write(data)
+                # Borrowed from cpython 2.7 subprocess.py
+                except IOError as e:
+                    if e.errno != errno.EPIPE and e.errno != errno.EINVAL:
+                        raise
+                sp.stdin.close()
+            while sp.poll() is None:
+                sys.stdout.write(sp.stdout.readline())
+                sys.stdout.flush()
+            sp.stdout.close()
+        else:
+            (out, err) = sp.communicate(data)
 
         # Just ensure blank instead of none.
         if not out and capture:
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
index a73fd26..37354b4 100644
--- a/tests/unittests/test_util.py
+++ b/tests/unittests/test_util.py
@@ -4,6 +4,7 @@ from __future__ import print_function
 
 import logging
 import os
+import pty
 import shutil
 import stat
 import tempfile
@@ -627,6 +628,10 @@ class TestSubp(helpers.TestCase):
         self.assertEqual(
             ['FOO=BAR', 'HOME=/myhome', 'K1=V1', 'K2=V2'], out.splitlines())
 
+    def test_subp_detach_tty():
+        with pty.openpty() as (master, slave):
+            pass
+
     def test_returns_none_if_no_capture(self):
         (out, err) = util.subp(self.stdin2out, data=b'', capture=False)
         self.assertIsNone(err)