Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exceptions: change error message for CommandFailedError #1652

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions teuthology/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
from re import sub

from teuthology.orchestra.run_helper import Raw


class BranchNotFoundError(ValueError):
def __init__(self, branch, repo=None):
self.branch = branch
Expand Down Expand Up @@ -50,21 +55,30 @@ class CommandFailedError(Exception):
"""
Exception thrown on command failure
"""
def __init__(self, command, exitstatus, node=None, label=None):
self.command = command
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From one side this might seem useful for a user to copy and paste command,
but from another point of view, it is bad for debugging, it will be not possible to figure out what
exactly command was before it got to the Exception. Also, the next line joining command with spaces is absolutely not the same as it makes it quoted inside run methods, which makes it hard to reproduce issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From one side this might seem useful for a user to copy and paste command,

but from another point of view, it is bad for debugging, it will be not possible to figure out what
exactly command was before it got to the Exception.

I don't see how would figuring out become difficult since parameter would be printed as it is (at least individually). For example, ['ls', 'dir1'] would become ls dir1. I have grepped plenty times and either don't make much of a difference.

Also, the next line joining command with spaces is absolutely not the same as it makes it quoted inside run methods, which makes it hard to reproduce issue.

Same as what?

Umm... are you referring to this line when you say "quoted inside run methods"? I think there's no nice way around that but printing as string definitely takes away effort of deleting a dozen inverted commas every time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean the quotation thing.
Also, if you want to get the exact command, it is already printed in the logs, you just need to find the place where debug is printing the command out before the execution.

def __init__(self, cmd, exitstatus, node=None, label=None):
self.cmd = cmd
Comment on lines +58 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why renaming command to cmd?

  • This change is not necessary.
  • Breaks the convention of naming arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why renaming command to cmd?

Lesser effort to type, and easier to keep statements under 79 characters which in turn saves us from hard to read split-over-several-line statements .

self.exitstatus = exitstatus
self.node = node
self.label = label

def __str__(self):
if isinstance(self.cmd, (list, tuple)):
cmd = [str(Raw) if isinstance(x, Raw) else x
for x in self.cmd]
cmd = ' '.join(self.cmd)
elif isinstance(self.cmd, str):
cmd = sub(r'Raw\([\'"](.*)[\'"]\)', r'\1', self.cmd)
else:
raise RuntimeError('variable "self.cmd" is not str, list or tuple')

prefix = "Command failed"
if self.label:
prefix += " ({label})".format(label=self.label)
if self.node:
prefix += " on {node}".format(node=self.node)
return "{prefix} with status {status}: {cmd!r}".format(
status=self.exitstatus,
cmd=self.command,
cmd=cmd,
prefix=prefix,
)

Expand All @@ -74,7 +88,7 @@ def fingerprint(self):
Used by sentry instead of grouping by backtrace.
"""
return [
self.label or self.command,
self.label or self.cmd,
'exit status {}'.format(self.exitstatus),
'{{ type }}',
]
Expand Down
100 changes: 1 addition & 99 deletions teuthology/orchestra/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
from paramiko import ChannelFile

import gevent
import gevent.event
import socket
import pipes
import logging
import shutil

from teuthology.orchestra.run_helper import quote, KludgeFile, PIPE
from teuthology.contextutil import safe_while
from teuthology.exceptions import (CommandCrashedError, CommandFailedError,
ConnectionLostError)
Expand Down Expand Up @@ -222,43 +221,6 @@ def __repr__(self):
)


class Raw(object):

"""
Raw objects are passed to remote objects and are not processed locally.
"""
def __init__(self, value):
self.value = value

def __repr__(self):
return '{cls}({value!r})'.format(
cls=self.__class__.__name__,
value=self.value,
)

def __eq__(self, value):
return self.value == value


def quote(args):
"""
Internal quote wrapper.
"""
def _quote(args):
"""
Handle quoted string, testing for raw charaters.
"""
for a in args:
if isinstance(a, Raw):
yield a.value
else:
yield pipes.quote(a)
if isinstance(args, list):
return ' '.join(_quote(args))
else:
return args


def copy_to_log(f, logger, loglevel=logging.INFO, capture=None, quiet=False):
"""
Copy line by line from file in f to the log from logger
Expand Down Expand Up @@ -322,66 +284,6 @@ def copy_file_to(src, logger, stream=None, quiet=False):
"""
copy_to_log(src, logger, capture=stream, quiet=quiet)

def spawn_asyncresult(fn, *args, **kwargs):
"""
Spawn a Greenlet and pass it's results to an AsyncResult.

This function is useful to shuffle data from a Greenlet to
AsyncResult, which then again is useful because any Greenlets that
raise exceptions will cause tracebacks to be shown on stderr by
gevent, even when ``.link_exception`` has been called. Using an
AsyncResult avoids this.
"""
r = gevent.event.AsyncResult()

def wrapper():
"""
Internal wrapper.
"""
try:
value = fn(*args, **kwargs)
except Exception as e:
r.set_exception(e)
else:
r.set(value)
gevent.spawn(wrapper)

return r


class Sentinel(object):

"""
Sentinel -- used to define PIPE file-like object.
"""
def __init__(self, name):
self.name = name

def __str__(self):
return self.name

PIPE = Sentinel('PIPE')


class KludgeFile(object):

"""
Wrap Paramiko's ChannelFile in a way that lets ``f.close()``
actually cause an EOF for the remote command.
"""
def __init__(self, wrapped):
self._wrapped = wrapped

def __getattr__(self, name):
return getattr(self._wrapped, name)

def close(self):
"""
Close and shutdown.
"""
self._wrapped.close()
self._wrapped.channel.shutdown_write()


def run(
client, args,
Expand Down
82 changes: 82 additions & 0 deletions teuthology/orchestra/run_helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
"""
Contains classes that "help" RemoteProcess and rest of the code in run.py

This module was started as it was needed that Raw and quote be separated from
teuthology.orchestra.run so that these can be imported in
teuthology.exceptions without causing circular dependency.
"""

from pipes import quote as pipes_quote


class Raw(object):
"""
Raw objects are passed to remote objects and are not processed locally.
"""
def __init__(self, value):
self.value = value

def __repr__(self):
return '{cls}({value!r})'.format(
cls=self.__class__.__name__,
value=self.value,
)

def __eq__(self, value):
return self.value == value

def __str__(self, value):
return str(value)


def quote(args):
"""
Internal quote wrapper.
"""
def _quote(args):
"""
Handle quoted string, testing for raw charaters.
"""
for a in args:
if isinstance(a, Raw):
yield a.value
else:
yield pipes_quote(a)

if isinstance(args, list):
return ' '.join(_quote(args))
else:
return args


class Sentinel(object):
"""
Sentinel -- used to define PIPE file-like object.
"""
def __init__(self, name):
self.name = name

def __str__(self):
return self.name


PIPE = Sentinel('PIPE')


class KludgeFile(object):
"""
Wrap Paramiko's ChannelFile in a way that lets ``f.close()``
actually cause an EOF for the remote command.
"""
def __init__(self, wrapped):
self._wrapped = wrapped

def __getattr__(self, name):
return getattr(self._wrapped, name)

def close(self):
"""
Close and shutdown.
"""
self._wrapped.close()
self._wrapped.channel.shutdown_write()
2 changes: 1 addition & 1 deletion teuthology/packaging.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
NoRemoteError)
from teuthology.misc import sudo_write_file
from teuthology.orchestra.opsys import OS, DEFAULT_OS_VERSION
from teuthology.orchestra.run import Raw
from teuthology.orchestra.run_helper import Raw

log = logging.getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion teuthology/test/task/test_pcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from teuthology.config import config, FakeNamespace
from teuthology.orchestra.cluster import Cluster
from teuthology.orchestra.remote import Remote
from teuthology.orchestra.run import Raw
from teuthology.orchestra.run_helper import Raw
from teuthology.task.pcp import (PCPDataSource, PCPArchive, PCPGrapher,
GrafanaGrapher, GraphiteGrapher, PCP)

Expand Down