From d8d44ed1566b19eec055e07da2a0fed88fed4152 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Sun, 8 Mar 2020 14:00:53 +0800 Subject: [PATCH] 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 --- qa/tasks/ceph_manager.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/qa/tasks/ceph_manager.py b/qa/tasks/ceph_manager.py index 68f1c84d8775f..21ce170342664 100644 --- 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" -- 2.39.5