Better catch the error when pip install by liyanhui1228 · Pull Request #233 · GoogleCloudPlatform/cloud-opensource-python

liyanhui1228 · 2019-01-28T21:34:54Z

No description provided.

brianquinlan · 2019-01-28T22:39:20Z

-    r'not install packages due to an EnvironmentError: (?P<error>.*)')
+# Pattern for pip installation error related to the package being
+# installed.
+PIP_INSTALL_ERROR_PATTERN = re.compile(


If I understand correctly, the idea is that the only time that pip will return a non-zero error code that is package-related is when the package doesn't exist. And you think that is cleaner than the previous approach. Could you document that and maybe point to some source that shows that is true?

Yeah I was thinking that if there is a docker timeout error during the pip install process, it will also be treated as an INSTALL_ERROR in the result, which actually isn't. And there might still be some other type of error happened during the installation, probably we could restrict that install error only means the package and version doesn't exist.

A docker timeout should lead to a ReadTimeout, which results in
raise PipCheckerError(...)

Are you sure that a non-zero pip install return can only be one of:

the package/version not existing

some sort of internal error

Oh I didn't mean that a non-zero pip install returncode can only be the two cases. I was just seeing in our BigQuery data there were some results showing INSTALL_ERROR but apparently they weren't real install errors but because of docker timeout. I changed to turn the raise_on_failure which can also fix this.

ylil93 · 2019-01-28T23:01:22Z

LGTM - please address Brian's comment

brianquinlan

I don't understand how a docker timeout can end up returning a non-500 result. Could you explain the scenario?

brianquinlan · 2019-01-29T00:34:24Z

            stdout=True,
            stderr=True,
-            raise_on_failure=False)
+            raise_on_failure=True)


I don't think that this will work (see the comment 7 lines up).

In here if the docker was already killed, the container.exec_run will return a code 137 with no error message. And if raise_on_failure is False, the _run_command func will return 137, b''. Then in here, it will be treated as a INSTALL_ERROR, which leads to...

Ahhh...interesting. 137 means that the process ended due to sigkill (http://tldp.org/LDP/abs/html/exitcodes.html). We could probably check for that error code specifically. If we really wanted to be paranoid, when could then check to confirm that the container has already exited, but that might be a bad idea.

Yep and probably we could just raise an error for that case, then it will return a 500 and will not be treated as INSTALL_ERROR.

And I have tested that if I set the docker timeout to a very short time like 1 second, I could reproduce this.

brianquinlan

Maybe this belongs in _run_command (since this could occur for any command). So something like:

def _run_command(
   ...

    if returncode > 128 and returncode < 137:
        ...
    elif returncode and raise_on_failure:
      raise PipError(...)

brianquinlan · 2019-01-29T19:20:37Z

                               command=command,
                               returncode=returncode)

+            # Checking for error caused by sigkill (128+9)


Suggested change

# Checking for error caused by sigkill (128+9)

# Checking for cases where the command was killed by a signal.

# If a process was killed by a signal, then it's exit code will be

# 128 + <signal number>.

# If a docker container exits with a running command then it will be

# killed with SIGKILL => 128 + 9 = 137

brianquinlan · 2019-01-29T19:20:50Z

                               returncode=returncode)

+            # Checking for error caused by sigkill (128+9)
+            if returncode >= 128 and returncode <= 137:


Suggested change

if returncode >= 128 and returncode <= 137:

if returncode > 128 and returncode <= 137:

brianquinlan · 2019-01-29T19:21:42Z

+            # Checking for error caused by sigkill (128+9)
+            if returncode >= 128 and returncode <= 137:
+                raise PipCheckerError(
+                    error_msg="The docker container timed out before executing"


Suggested change

error_msg="The docker container timed out before executing"

error_msg="The command {0} was killed by the signal {1}. "

brianquinlan · 2019-01-29T19:25:50Z

+            if returncode >= 128 and returncode <= 137:
+                raise PipCheckerError(
+                    error_msg="The docker container timed out before executing"
+                              " pip command. Error msg: {}".format(output))


Suggested change

" pip command. Error msg: {}".format(output))

"This likely means that the Docker container timed out. '

'Error msg: {}".format(command, returncode - 128, output))

brianquinlan · 2019-01-30T01:16:01Z

Looks good but shouldn't there be a test for this?

liyanhui1228 · 2019-01-30T02:17:23Z


        if duration > pip_checker.TIME_OUT:
-            raise docker.errors.APIError(message="time out",
-                                         explanation="Request time out.")


This wasn't mocking the docker container timeout behavior correctly before, which should actually return a 137 code instead of raising a docker.errors.APIError. Changing this to match the behavior and the test in line 123 is for testing the timeout error.

brianquinlan

test_pip_checker.py line #130:

with patch_timeout, self.assertRaisesRegex(pip_checker.PipCheckerError, 'killed by signal 9')

brianquinlan · 2019-01-30T18:08:23Z

+        # killed with SIGKILL => 128 + 9 = 137
+        if returncode > 128 and returncode <= 137:
+            raise PipCheckerError(
+                error_msg="The command {} was killed by the signal {}."


Suggested change

error_msg="The command {} was killed by the signal {}."

error_msg="The command {} was killed by the signal {}. "

brianquinlan · 2019-01-30T18:14:55Z

+        # killed with SIGKILL => 128 + 9 = 137
+        if returncode > 128 and returncode <= 137:
+            raise PipCheckerError(
+                error_msg="The command {} was killed by the signal {}."


Suggested change

error_msg="The command {} was killed by the signal {}."

error_msg="The command {} was killed by signal {}."

brianquinlan · 2019-01-30T18:44:46Z

+        if returncode > 128 and returncode <= 137:
+            raise PipCheckerError(
+                error_msg="The command {} was killed by signal {}. "
+                          "This likely means that the Docker container timed"


Suggested change

"This likely means that the Docker container timed"

"This likely means that the Docker container timed "

brianquinlan · 2019-01-30T19:04:01Z

                        'self': False,
                    }
                )
-                if status_type is 'self-success':


Nice catch!

Better catch the error when pip install

70c8404

googlebot added the cla: yes label Jan 28, 2019

liyanhui1228 requested a review from ylil93 January 28, 2019 21:35

fix test

41249c5

ylil93 suggested changes Jan 28, 2019

View reviewed changes

Comment thread compatibility_server/pip_checker.py Outdated

brianquinlan reviewed Jan 28, 2019

View reviewed changes

ylil93 approved these changes Jan 28, 2019

View reviewed changes

liyanhui1228 added 2 commits January 28, 2019 16:25

turn on raise_on_failure

9aaf9c6

fix

8ef271e

brianquinlan reviewed Jan 29, 2019

View reviewed changes

liyanhui1228 added 2 commits January 29, 2019 10:33

Check for returncode 137

47cb0bd

fix

598c25c

brianquinlan reviewed Jan 29, 2019

View reviewed changes

fix

e2f5442

brianquinlan mentioned this pull request Jan 30, 2019

Track docker errors in compatibility server #234

Merged

liyanhui1228 added 2 commits January 29, 2019 18:12

fix test

18d7570

fix lint

d71e513

liyanhui1228 commented Jan 30, 2019

View reviewed changes

brianquinlan suggested changes Jan 30, 2019

View reviewed changes

liyanhui1228 added 2 commits January 30, 2019 10:22

fix

a122504

fix lint

74df635

brianquinlan suggested changes Jan 30, 2019

View reviewed changes

fix

c59acc6

brianquinlan reviewed Jan 30, 2019

View reviewed changes

brianquinlan approved these changes Jan 30, 2019

View reviewed changes

liyanhui1228 merged commit cb817cc into GoogleCloudPlatform:master Jan 30, 2019

liyanhui1228 deleted the install_error branch January 30, 2019 19:15

-            # Checking for error caused by sigkill (128+9)
+            # Checking for cases where the command was killed by a signal.
+            # If a process was killed by a signal, then it's exit code will be
+            # 128 + <signal number>.
+            # If a docker container exits with a running command then it will be
+            # killed with SIGKILL => 128 + 9 = 137

	if returncode >= 128 and returncode <= 137:
	if returncode > 128 and returncode <= 137:

	error_msg="The docker container timed out before executing"
	error_msg="The command {0} was killed by the signal {1}. "

	" pip command. Error msg: {}".format(output))
	"This likely means that the Docker container timed out. '
	'Error msg: {}".format(command, returncode - 128, output))

	error_msg="The command {} was killed by the signal {}."
	error_msg="The command {} was killed by the signal {}. "

Conversation

liyanhui1228 commented Jan 28, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ylil93 commented Jan 28, 2019

Uh oh!

brianquinlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan commented Jan 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants