qa/tasks/ceph_manager: use StringIO for capturing COT output

there are couple factors we should consider when choosing between
BytesIO and StringIO:

- if the producer is producing binary
- if we are expecting binary
- if the layers in between them are doing the decoding/encoding
  automatically.

in our case, the producer is either the ChannelFile instances returned
by paramiko.SSHClient or subprocess.CompletedProcess insances returned
by subprocess.run(). the former are file-like objects opened in "r" mode,
but their contents are decoded with utf-8 when reading if
ChannelFile.FLAG_BINARY is not specified. that's why we always try to
add this flag in orchestra/run.py when collecting the stdout and stderr
from paramiko.SSHClient after executing a command.

back in python2, this works just fine. as we don't differentiate bytes
from str by then.

but in python3, we have to make a decision. in the case of
ceph-objectstore-tool (COT for short), it does not produce binary and
we don't check its output with binary, so, if neither Remote.run() nor
LocalRemote.run() decodes/encodes for us, it's fine.

so it boils down to `copy_to_log()`:

i think we we should respect the consumer's expectation, and only decode
the output if a StringIO is passed in as stdout or stderr.

as we always log the output with logging we could either set
`ChannelFile.FLAG_BINARY` depending on the type of `capture` or not.
if it's not set, paramiko will return str (bytes) on python2, and str on
python3. if it's not set paramiko will return str (bytes) on python2,
and bytes on python3.

if there is non-ASCII in the output, logging will bail fail with
`UnicodeDecodeError` exception. and paramiko throws the same exception
when trying to decode for us if `ChannelFile.FLAG_BINARY` is not
specified.

so to ensure that we always have logging messages no matter if the
producer follows the rule of "use StringIO if you only emit text" or
not, we have to use `ChannelFile.FLAG_BINARY`, and force paramiko
to send us the bytes. but we still have the luxury to use StringIO
and do the decode when the caller asks for str explicitly. that'd save
the pain of using `str.decode()` or `six.ensure_str()` everywhere
even if we can assure that the program does not write binary.

Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/qa/tasks/ceph_manager.py b/qa/tasks/ceph_manager.py
index 68f1c84d8775f13d9fc5b1908be7b346e2af773c..21ce1703426647f1de274fcf400e6775d29304e3 100644 (file)
--- a/qa/tasks/ceph_manager.py
+++ b/qa/tasks/ceph_manager.py
@@ -13,6 +13,7 @@ import logging
 import threading
 import traceback
 import os
+import six
 
 from io import BytesIO
 from teuthology import misc as teuthology
@@ -24,7 +25,7 @@ from teuthology.orchestra.remote import Remote
 from teuthology.orchestra import run
 from teuthology.exceptions import CommandFailedError
 from tasks.thrasher import Thrasher
-import six
+from six import StringIO
 
 try:
     from subprocess import DEVNULL # py3k
@@ -237,13 +238,14 @@ class OSDThrasher(Thrasher):
                 args=['ceph-objectstore-tool'] + cmd,
                 name=osd,
                 wait=True, check_status=False,
-                stdout=BytesIO(),
-                stderr=BytesIO())
+                stdout=StringIO(),
+                stderr=StringIO())
         else:
             return remote.run(
                 args=['sudo', 'adjust-ulimits', 'ceph-objectstore-tool'] + cmd,
-                wait=True, check_status=False, stdout=BytesIO(),
-                stderr=BytesIO())
+                wait=True, check_status=False,
+                stdout=StringIO(),
+                stderr=StringIO())
 
     def kill_osd(self, osd=None, mark_down=False, mark_out=False):
         """
@@ -307,7 +309,7 @@ class OSDThrasher(Thrasher):
                     if proc.exitstatus == 0:
                         break
                     elif (proc.exitstatus == 1 and
-                          six.ensure_str(proc.stderr.getvalue()) == "OSD has the store locked"):
+                          proc.stderr.getvalue() == "OSD has the store locked"):
                         continue
                     else:
                         raise Exception("ceph-objectstore-tool: "
@@ -419,7 +421,7 @@ class OSDThrasher(Thrasher):
                 ])
             if proc.exitstatus == 1:
                 bogosity = "The OSD you are using is older than the exported PG"
-                if bogosity in six.ensure_str(proc.stderr.getvalue()):
+                if bogosity in proc.stderr.getvalue():
                     self.log("OSD older than exported PG"
                              "...ignored")
             elif proc.exitstatus == 10:
@@ -456,8 +458,8 @@ class OSDThrasher(Thrasher):
                            + " --op apply-layout-settings --pool " + pool).format(id=osd)
                     proc = imp_remote.run(args=cmd,
                                           wait=True, check_status=False,
-                                          stderr=BytesIO)
-                    if 'Couldn\'t find pool' in six.ensure_str(proc.stderr.getvalue()):
+                                          stderr=StringIO())
+                    if 'Couldn\'t find pool' in proc.stderr.getvalue():
                         continue
                     if proc.exitstatus:
                         raise Exception("ceph-objectstore-tool apply-layout-settings"