exceptions: change error message for CommandFailedError #1652

rishabh-d-dave · 2021-06-09T08:41:54Z

For the convenience of user, print the command that led to
CommandFailedError in error message as a str than as a list or a tuple.
This makes it more readable and enables the user to copy and use the
command without any hassles.

kshtsk · 2021-06-09T10:44:12Z

teuthology/exceptions.py

@@ -51,7 +51,12 @@ class CommandFailedError(Exception):
    Exception thrown on command failure
    """
    def __init__(self, command, exitstatus, node=None, label=None):
-        self.command = command


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.

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.

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.

batrick · 2021-06-11T22:02:05Z

teuthology/exceptions.py

@@ -51,7 +51,12 @@ class CommandFailedError(Exception):
    Exception thrown on command failure
    """
    def __init__(self, command, exitstatus, node=None, label=None):
-        self.command = command
+        if isinstance(command, (list, tuple)):
+            self.command = ' '.join(command)


I think the right place for this transformation would be in the __str__ method. It would improve readability, yes.

True, that'll be much better. Updated the patch now. Thanks!

kshtsk · 2021-06-14T15:35:50Z

teuthology/exceptions.py

+    def __init__(self, cmd, exitstatus, node=None, label=None):
+        self.cmd = cmd


Why renaming command to cmd?

This change is not necessary.

Breaks the convention of naming arguments.

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 .

kshtsk · 2021-06-14T15:37:38Z

teuthology/exceptions.py

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


Do not override the original command, just return the str representation computed in local variable.

Okay. That would be better, I guess. Thanks!

kshtsk · 2021-06-14T15:39:27Z

teuthology/orchestra/run.py

 import logging
 import shutil

 from teuthology.contextutil import safe_while
 from teuthology.exceptions import (CommandCrashedError, CommandFailedError,
                                   ConnectionLostError)
+from teuthology.orchestra.run_helper import Raw, quote


Instead of run_helper I suggest more appropriate name for the module, util or tool or common or for example

I find run_helper appropriate since it contains classes that help run.py.

BTW I moving rest of misc. classes from run.py to run_helper.py too.

that looks weird, if you really want to have run based classes it would have more appropriate to have this layout:

teuthology/orchestra/run/__init__.py teuthology/orchestra/run/helper.py

instead of underscore separated.

I personally feel this a bit of overkill but I don't really mind.

@batrick Any opinion?

kshtsk · 2021-06-14T15:44:22Z

teuthology/exceptions.py

+                            for x in self.cmd]
+            self.cmd = ' '.join(self.cmd)
+        elif isinstance(self.cmd, str):
+            self.cmd = sub(r'Raw\([\'"](.*)[\'"]\)', r'\1', self.cmd)


Why this branch is needed?

Transforms str 'param1 param2 Raw('&&") param3' to str 'param1 param2 && param3'.

why this is needed to transform I do not understand

or in which situation this is happening...

Same reason for which i am converting list to str.

well... I don't understand why we do not use quote method here from the helper?

How would that do the job?

$ python Python 3.8.9 (default, Apr 6 2021, 00:00:00) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from teuthology.orchestra.run_helper import Raw, quote >>> a = quote('a b Raw("&&") c') >>> a 'a b Raw("&&") c' >>>

Yep, but how is it possible to have the Raw in the string? From my point of view if you have Raw in the str it means that you've made a typo and this command is not going to be run successfully.

rishabh-d-dave · 2021-06-15T11:59:06Z

@kshtsk @batrick
teuthology.orchestra.run.spawn_asyncresult() is a dead function, there are no calls to it in teuthology codebase. Better to get rid of it?

kshtsk · 2021-06-15T13:03:52Z

@kshtsk @batrick
teuthology.orchestra.run.spawn_asyncresult() is a dead function, there are no calls to it in teuthology codebase. Better to get rid of it?

I don't know, Isn't it used in any named branch of ceph qa/tasks?

rishabh-d-dave · 2021-06-15T13:30:40Z

@kshtsk

@kshtsk @batrick
teuthology.orchestra.run.spawn_asyncresult() is a dead function, there are no calls to it in teuthology codebase. Better to get rid of it?

I don't know, Isn't it used in any named branch of ceph qa/tasks?

No call there too.

$ git pull --no-rebase && grep -rnI spawn_asyncresult qa/
Already up to date.

I will get rid of it.

Class Raw has been moved from teuthology.orchestra.run to teuthology.orchestra.run_helper. Update Ceph QA code accordingly. Related PR: ceph/teuthology#1652. Signed-off-by: Rishabh Dave <[email protected]>

Class Raw is needed in teuthology.exceptions and importing it from teuthology.orchestra.run is not possible since it would lead to a circular dependecy. Move these classes to a new module, run_helper.py, to avoid this and import through run_helper.py instead. Signed-off-by: Rishabh Dave <[email protected]>

For the convenience of user, print the command that led to CommandFailedError in error message as a str than as a list or a tuple. This makes it more readable and enables the user to copy and use the command without any hassles. This commit also uses the opportunity to rename the variable "command" to "cmd" and "self.command" to "self.cmd" in CommandFailedError for convenience and so that it's easy to keep statements under 80 characters. Signed-off-by: Rishabh Dave <[email protected]>

Since the function is dead code, there are no calls to it in teuthology or ceph repository, remove it. Signed-off-by: Rishabh Dave <[email protected]>

Class Raw has been moved from teuthology.orchestra.run to teuthology.orchestra.run_helper. Update Ceph QA code accordingly. Related PR: ceph/teuthology#1652. Signed-off-by: Rishabh Dave <[email protected]>

rishabh-d-dave requested review from kshtsk and batrick June 9, 2021 08:42

rishabh-d-dave force-pushed the cmdfailederr branch from b6f00af to 0d667e8 Compare June 9, 2021 09:03

kshtsk reviewed Jun 9, 2021

View reviewed changes

rishabh-d-dave force-pushed the cmdfailederr branch from 0d667e8 to 831dc66 Compare June 9, 2021 13:18

batrick reviewed Jun 11, 2021

View reviewed changes

rishabh-d-dave force-pushed the cmdfailederr branch from 831dc66 to 9013aa6 Compare June 14, 2021 13:52

rishabh-d-dave requested a review from batrick June 14, 2021 13:53

rishabh-d-dave force-pushed the cmdfailederr branch from 9013aa6 to b35aa79 Compare June 14, 2021 15:15

kshtsk reviewed Jun 14, 2021

View reviewed changes

rishabh-d-dave force-pushed the cmdfailederr branch from b35aa79 to 1cc8aa2 Compare June 15, 2021 13:46

rishabh-d-dave added 3 commits June 16, 2021 09:26

orchestra/run.py: remove spawn_asyncresult()

c013aa4

Since the function is dead code, there are no calls to it in teuthology or ceph repository, remove it. Signed-off-by: Rishabh Dave <[email protected]>

rishabh-d-dave force-pushed the cmdfailederr branch from 1cc8aa2 to c013aa4 Compare June 16, 2021 04:00

rishabh-d-dave mentioned this pull request Jun 16, 2021

qa: fix import statements ceph/ceph#41868

Closed

3 tasks

djgalloway changed the base branch from master to main June 1, 2022 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

exceptions: change error message for CommandFailedError #1652

exceptions: change error message for CommandFailedError #1652

rishabh-d-dave commented Jun 9, 2021

kshtsk Jun 9, 2021

rishabh-d-dave Jun 9, 2021

kshtsk Jun 9, 2021

batrick Jun 11, 2021

rishabh-d-dave Jun 14, 2021 •

edited

Loading

kshtsk Jun 14, 2021

rishabh-d-dave Jun 15, 2021

kshtsk Jun 14, 2021

rishabh-d-dave Jun 15, 2021 •

edited

Loading

kshtsk Jun 14, 2021

rishabh-d-dave Jun 15, 2021

kshtsk Jun 15, 2021 •

edited

Loading

rishabh-d-dave Jun 15, 2021

kshtsk Jun 14, 2021

rishabh-d-dave Jun 15, 2021

kshtsk Jun 15, 2021

kshtsk Jun 15, 2021

rishabh-d-dave Jun 15, 2021

kshtsk Jun 15, 2021

rishabh-d-dave Jun 16, 2021

kshtsk Jun 16, 2021

rishabh-d-dave commented Jun 15, 2021

kshtsk commented Jun 15, 2021

rishabh-d-dave commented Jun 15, 2021 •

edited

Loading

		def __init__(self, cmd, exitstatus, node=None, label=None):
		self.cmd = cmd

exceptions: change error message for CommandFailedError #1652

Are you sure you want to change the base?

exceptions: change error message for CommandFailedError #1652

Conversation

rishabh-d-dave commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh-d-dave Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh-d-dave Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh-d-dave commented Jun 15, 2021

kshtsk commented Jun 15, 2021

rishabh-d-dave commented Jun 15, 2021 • edited Loading

rishabh-d-dave Jun 14, 2021 •

edited

Loading

rishabh-d-dave Jun 15, 2021 •

edited

Loading

kshtsk Jun 15, 2021 •

edited

Loading

rishabh-d-dave commented Jun 15, 2021 •

edited

Loading