[Notes] [Git][BuildStream/buildstream][507-some-log-lines-appear-to-be-d

Tristan Maat pushed to branch 507-some-log-lines-appear-to-be-duplicates at BuildStream / buildstream

Commits:

9ebd7fe1

by Josh Smith at 2018-07-27T04:41:26Z

_exceptions.py: Modify BstError API to allow optional retry

job.py: Changes to the logic surrounding retry attempts and child process return codes
element.py, source.py: ElementError and SourceError also implement this change.

These exceptions now have an optional parameter of temporary which defaults to false. This will potentially break
backwards compatibility where exceptions were previously raised and a retry was intended.

To trigger a retry, one must now raise their SourceError or ElementError with temporary=True.

This aims to fix #397.

ca224425

by Josh Smith at 2018-07-27T04:41:26Z

_downloadablefilesource.py: Update network errors to be temporary

This follows the change in 67ecd97a05279a3b7570ad59f05bf0a5973ef04c.

0a7cfcdf

by Josh Smith at 2018-07-27T04:41:26Z

tests/sources: deb, tar and zip all test for retry behaviour

This is following 5d6418a06a3a0262df5f6b9e6e1a36578515fd4a where
DownloadableFileSource was modified to raise 'temporary'
errors when relating to file downloading.

9b2bc78f

by Josh Smith at 2018-07-27T04:41:26Z

cascache.py: Update cache push/pull errors to be temporary

Further work needs to be done for the current grpc exceptions which are reraised.

4986527f

by Josh Smith at 2018-07-27T04:41:26Z

plugin.py: Extend Plugin.call() API

Plugin.call() now takes fail_temporarily as an optional parameter,
when supplied it will cause subsequent failures to trigger temporary
errors as opposed to permanent errors.

This also extends Plugin.check_output() which makes use of Plugin.call()

a9a1048e

by Josh Smith at 2018-07-27T04:41:26Z

git.py: Update git plugin to fail temporarily on clone failures

The git plugin will now make use of the fail_temporarily parameter
to Plugin.call(), allowing failures to trigger a retry.

aa071cb6

by Tristan Van Berkom at 2018-07-27T06:13:53Z

Merge branch 'tristan/temporary-failures' into 'master'

Temorary failures

Closes #397

See merge request BuildStream/buildstream!574

4d86e7c5

by Tristan Van Berkom at 2018-07-27T06:43:48Z

element.py, source.py, plugin.py: Adjusting API version since annotations.

Since we backported the temporary/permanent failures patch for #397
into the `bst-1.2` branch, we need to adjust the since versions in master
down to 1.2.

fb222ba1

by Josh Smith at 2018-07-27T08:50:45Z

cascache.py: Raise ArtifactError on grpc error

This aims to fix issue #502.

48916b8a

by Tristan Maat at 2018-07-27T10:22:41Z

Merge branch 'Qinusty/502' into 'master'

cascache.py: Raise ArtifactError on grpc error

Closes #502

See merge request BuildStream/buildstream!576

48184cf2

by Tristan Maat at 2018-07-27T10:33:22Z

buildelement.py: Only display one activity message (#507)

12 changed files:

buildstream/_artifactcache/cascache.py
buildstream/_exceptions.py
buildstream/_scheduler/jobs/job.py
buildstream/buildelement.py
buildstream/element.py
buildstream/plugin.py
buildstream/plugins/sources/_downloadablefilesource.py
buildstream/plugins/sources/git.py
buildstream/source.py
tests/sources/deb.py
tests/sources/tar.py
tests/sources/zip.py

Changes:

buildstream/_artifactcache/cascache.py

@@ -240,7 +240,8 @@ class CASCache(ArtifactCache):
              except grpc.RpcError as e:
                  if e.code() != grpc.StatusCode.NOT_FOUND:
 -                    raise
 +                    raise ArtifactError("Failed to pull artifact {}: {}".format(
 +                        element._get_brief_display_key(), e)) from e
          return False
@@ -285,6 +286,7 @@ class CASCache(ArtifactCache):
                      except grpc.RpcError as e:
                          if e.code() != grpc.StatusCode.NOT_FOUND:
 +                            # Intentionally re-raise RpcError for outer except block.
                              raise
                      missing_blobs = {}
@@ -340,7 +342,7 @@ class CASCache(ArtifactCache):
              except grpc.RpcError as e:
                  if e.code() != grpc.StatusCode.RESOURCE_EXHAUSTED:
 -                    raise ArtifactError("Failed to push artifact {}: {}".format(refs, e)) from e
 +                    raise ArtifactError("Failed to push artifact {}: {}".format(refs, e), temporary=True) from e
          return pushed

buildstream/_exceptions.py

@@ -99,7 +99,7 @@ class ErrorDomain(Enum):
+ #
  class BstError(Exception):
 -    def __init__(self, message, *, detail=None, domain=None, reason=None):
 +    def __init__(self, message, *, detail=None, domain=None, reason=None, temporary=False):
          global _last_exception
          super().__init__(message)
@@ -114,6 +114,11 @@ class BstError(Exception):
+         #
          self.sandbox = None
 +        # When this exception occurred during the handling of a job, indicate
 +        # whether or not there is any point retrying the job.
 +        #
 +        self.temporary = temporary
++
          # Error domain and reason
+         #
          self.domain = domain
@@ -131,8 +136,8 @@ class BstError(Exception):
  # or by the base :class:`.Plugin` element itself.
+ #
  class PluginError(BstError):
 -    def __init__(self, message, reason=None):
 -        super().__init__(message, domain=ErrorDomain.PLUGIN, reason=reason)
 +    def __init__(self, message, reason=None, temporary=False):
 +        super().__init__(message, domain=ErrorDomain.PLUGIN, reason=reason, temporary=False)
  # LoadErrorReason
@@ -249,8 +254,8 @@ class SandboxError(BstError):
  # Raised when errors are encountered in the artifact caches
+ #
  class ArtifactError(BstError):
 -    def __init__(self, message, *, detail=None, reason=None):
 -        super().__init__(message, detail=detail, domain=ErrorDomain.ARTIFACT, reason=reason)
 +    def __init__(self, message, *, detail=None, reason=None, temporary=False):
 +        super().__init__(message, detail=detail, domain=ErrorDomain.ARTIFACT, reason=reason, temporary=True)
  # PipelineError

buildstream/_scheduler/jobs/job.py

@@ -35,6 +35,12 @@ from ..._exceptions import ImplError, BstError, set_last_task_error
  from ..._message import Message, MessageType, unconditional_messages
  from ... import _signals, utils
 +# Return code values shutdown of job handling child processes
 +#
 +RC_OK = 0
 +RC_FAIL = 1
 +RC_PERM_FAIL = 2
++
  # Used to distinguish between status messages and return values
  class Envelope():
@@ -111,6 +117,10 @@ class Job():
          self._max_retries = max_retries        # Maximum number of automatic retries
          self._result = None                    # Return value of child action in the parent
          self._tries = 0                        # Try count, for retryable jobs
++
 +        # If False, a retry will not be attempted regardless of whether _tries is less than _max_retries.
 +        #
 +        self._retry_flag = True
          self._logfile = logfile
          self._task_id = None
@@ -388,8 +398,9 @@ class Job():
                  result = self.child_process()
              except BstError as e:
                  elapsed = datetime.datetime.now() - starttime
 +                self._retry_flag = e.temporary
 -                if self._tries <= self._max_retries:
 +                if self._retry_flag and (self._tries <= self._max_retries):
                      self.message(MessageType.FAIL,
                                   "Try #{} failed, retrying".format(self._tries),
                                   elapsed=elapsed)
@@ -402,7 +413,10 @@ class Job():
                  # Report the exception to the parent (for internal testing purposes)
                  self._child_send_error(e)
 -                self._child_shutdown(1)
++
 +                # Set return code based on whether or not the error was temporary.
 +                #
 +                self._child_shutdown(RC_FAIL if self._retry_flag else RC_PERM_FAIL)
              except Exception as e:                        # pylint: disable=broad-except
@@ -416,7 +430,7 @@ class Job():
                  self.message(MessageType.BUG, self.action_name,
                               elapsed=elapsed, detail=detail,
                               logfile=filename)
 -                self._child_shutdown(1)
 +                self._child_shutdown(RC_FAIL)
              else:
                  # No exception occurred in the action
@@ -430,7 +444,7 @@ class Job():
                  # Shutdown needs to stay outside of the above context manager,
                  # make sure we dont try to handle SIGTERM while the process
                  # is already busy in sys.exit()
 -                self._child_shutdown(0)
 +                self._child_shutdown(RC_OK)
      # _child_send_error()
+     #
@@ -495,7 +509,8 @@ class Job():
          message.action_name = self.action_name
          message.task_id = self._task_id
 -        if message.message_type == MessageType.FAIL and self._tries <= self._max_retries:
 +        if (message.message_type == MessageType.FAIL and
 +                self._tries <= self._max_retries and self._retry_flag):
              # Job will be retried, display failures as warnings in the frontend
              message.message_type = MessageType.WARN
@@ -529,12 +544,17 @@ class Job():
      def _parent_child_completed(self, pid, returncode):
          self._parent_shutdown()
 -        if returncode != 0 and self._tries <= self._max_retries:
 +        # We don't want to retry if we got OK or a permanent fail.
 +        # This is set in _child_action but must also be set for the parent.
 +        #
 +        self._retry_flag = returncode not in (RC_OK, RC_PERM_FAIL)
++
 +        if self._retry_flag and (self._tries <= self._max_retries):
              self.spawn()
              return
 -        self.parent_complete(returncode == 0, self._result)
 -        self._scheduler.job_completed(self, returncode == 0)
 +        self.parent_complete(returncode == RC_OK, self._result)
 +        self._scheduler.job_completed(self, returncode == RC_OK)
      # _parent_process_envelope()
+     #

buildstream/buildelement.py

+ #
  #  Copyright (C) 2016 Codethink Limited
 +#  Copyright (C) 2018 Bloomberg Finance LP
+ #
  #  This program is free software; you can redistribute it and/or
  #  modify it under the terms of the GNU Lesser General Public
@@ -204,8 +205,9 @@ class BuildElement(Element):
      def prepare(self, sandbox):
          commands = self.__commands['configure-commands']
          if commands:
 -            for cmd in commands:
 -                self.__run_command(sandbox, cmd, 'configure-commands')
 +            with self.timed_activity("Running configure-commands"):
 +                for cmd in commands:
 +                    self.__run_command(sandbox, cmd, 'configure-commands')
      def generate_script(self):
          script = ""
@@ -231,13 +233,12 @@ class BuildElement(Element):
          return commands
      def __run_command(self, sandbox, cmd, cmd_name):
 -        with self.timed_activity("Running {}".format(cmd_name)):
 -            self.status("Running {}".format(cmd_name), detail=cmd)
+-
 -            # Note the -e switch to 'sh' means to exit with an error
 -            # if any untested command fails.
 -            #
 -            exitcode = sandbox.run(['sh', '-c', '-e', cmd + '\n'],
 -                                   SandboxFlags.ROOT_READ_ONLY)
 -            if exitcode != 0:
 -                raise ElementError("Command '{}' failed with exitcode {}".format(cmd, exitcode))
 +        self.status("Running {}".format(cmd_name), detail=cmd)
++
 +        # Note the -e switch to 'sh' means to exit with an error
 +        # if any untested command fails.
 +        #
 +        exitcode = sandbox.run(['sh', '-c', '-e', cmd + '\n'],
 +                               SandboxFlags.ROOT_READ_ONLY)
 +        if exitcode != 0:
 +            raise ElementError("Command '{}' failed with exitcode {}".format(cmd, exitcode))

buildstream/element.py

@@ -140,9 +140,10 @@ class ElementError(BstError):
         message (str): The error message to report to the user
         detail (str): A possibly multiline, more detailed error message
         reason (str): An optional machine readable reason string, used for test cases
 +       temporary (bool): An indicator to whether the error may occur if the operation was run again. (*Since: 1.2*)
      """
 -    def __init__(self, message, *, detail=None, reason=None):
 -        super().__init__(message, detail=detail, domain=ErrorDomain.ELEMENT, reason=reason)
 +    def __init__(self, message, *, detail=None, reason=None, temporary=False):
 +        super().__init__(message, detail=detail, domain=ErrorDomain.ELEMENT, reason=reason, temporary=temporary)
  class Element(Plugin):

buildstream/plugin.py

@@ -478,13 +478,15 @@ class Plugin():
                                             silent_nested=silent_nested):
              yield
 -    def call(self, *popenargs, fail=None, **kwargs):
 +    def call(self, *popenargs, fail=None, fail_temporarily=False, **kwargs):
          """A wrapper for subprocess.call()
          Args:
             popenargs (list): Popen() arguments
             fail (str): A message to display if the process returns
                         a non zero exit code
 +           fail_temporarily (bool): Whether any exceptions should
 +                                    be raised as temporary. (*Since: 1.2*)
             rest_of_args (kwargs): Remaining arguments to subprocess.call()
          Returns:
@@ -507,16 +509,18 @@ class Plugin():
                "Failed to download ponies from {}".format(
                    self.mirror_directory))
          """
 -        exit_code, _ = self.__call(*popenargs, fail=fail, **kwargs)
 +        exit_code, _ = self.__call(*popenargs, fail=fail, fail_temporarily=fail_temporarily, **kwargs)
          return exit_code
 -    def check_output(self, *popenargs, fail=None, **kwargs):
 +    def check_output(self, *popenargs, fail=None, fail_temporarily=False, **kwargs):
          """A wrapper for subprocess.check_output()
          Args:
             popenargs (list): Popen() arguments
             fail (str): A message to display if the process returns
                         a non zero exit code
 +           fail_temporarily (bool): Whether any exceptions should
 +                                    be raised as temporary. (*Since: 1.2*)
             rest_of_args (kwargs): Remaining arguments to subprocess.call()
          Returns:
@@ -555,7 +559,7 @@ class Plugin():
                raise SourceError(
                    fmt.format(plugin=self, track=tracking)) from e
          """
 -        return self.__call(*popenargs, collect_stdout=True, fail=fail, **kwargs)
 +        return self.__call(*popenargs, collect_stdout=True, fail=fail, fail_temporarily=fail_temporarily, **kwargs)
      #############################################################
      #            Private Methods used in BuildStream            #
@@ -619,7 +623,7 @@ class Plugin():
      # Internal subprocess implementation for the call() and check_output() APIs
+     #
 -    def __call(self, *popenargs, collect_stdout=False, fail=None, **kwargs):
 +    def __call(self, *popenargs, collect_stdout=False, fail=None, fail_temporarily=False, **kwargs):
          with self._output_file() as output_file:
              if 'stdout' not in kwargs:
@@ -634,7 +638,8 @@ class Plugin():
              exit_code, output = utils._call(*popenargs, **kwargs)
              if fail and exit_code:
 -                raise PluginError("{plugin}: {message}".format(plugin=self, message=fail))
 +                raise PluginError("{plugin}: {message}".format(plugin=self, message=fail),
 +                                  temporary=fail_temporarily)
          return (exit_code, output)

buildstream/plugins/sources/_downloadablefilesource.py

@@ -150,11 +150,11 @@ class DownloadableFileSource(Source):
                  # we would have downloaded.
                  return self.ref
              raise SourceError("{}: Error mirroring {}: {}"
 -                              .format(self, self.url, e)) from e
 +                              .format(self, self.url, e), temporary=True) from e
          except (urllib.error.URLError, urllib.error.ContentTooShortError, OSError) as e:
              raise SourceError("{}: Error mirroring {}: {}"
 -                              .format(self, self.url, e)) from e
 +                              .format(self, self.url, e), temporary=True) from e
      def _get_mirror_dir(self):
          return os.path.join(self.get_mirror_directory(),

buildstream/plugins/sources/git.py

@@ -113,7 +113,8 @@ class GitMirror():
+             #
              with self.source.tempdir() as tmpdir:
                  self.source.call([self.source.host_git, 'clone', '--mirror', '-n', self.url, tmpdir],
 -                                 fail="Failed to clone git repository {}".format(self.url))
 +                                 fail="Failed to clone git repository {}".format(self.url),
 +                                 fail_temporarily=True)
                  try:
                      shutil.move(tmpdir, self.mirror)
@@ -124,6 +125,7 @@ class GitMirror():
      def fetch(self):
          self.source.call([self.source.host_git, 'fetch', 'origin', '--prune'],
                           fail="Failed to fetch from remote git repository: {}".format(self.url),
 +                         fail_temporarily=True,
                           cwd=self.mirror)
      def has_ref(self):
@@ -157,7 +159,8 @@ class GitMirror():
          # case we're just checking out a specific commit and then removing the .git/
          # directory.
          self.source.call([self.source.host_git, 'clone', '--no-checkout', '--shared', self.mirror, fullpath],
 -                         fail="Failed to create git mirror {} in directory: {}".format(self.mirror, fullpath))
 +                         fail="Failed to create git mirror {} in directory: {}".format(self.mirror, fullpath),
 +                         fail_temporarily=True)
          self.source.call([self.source.host_git, 'checkout', '--force', self.ref],
                           fail="Failed to checkout git ref {}".format(self.ref),
@@ -170,7 +173,8 @@ class GitMirror():
          fullpath = os.path.join(directory, self.path)
          self.source.call([self.source.host_git, 'clone', '--no-checkout', self.mirror, fullpath],
 -                         fail="Failed to clone git mirror {} in directory: {}".format(self.mirror, fullpath))
 +                         fail="Failed to clone git mirror {} in directory: {}".format(self.mirror, fullpath),
 +                         fail_temporarily=True)
          self.source.call([self.source.host_git, 'remote', 'set-url', 'origin', self.url],
                           fail='Failed to add remote origin "{}"'.format(self.url),

buildstream/source.py

@@ -108,9 +108,10 @@ class SourceError(BstError):
         message (str): The breif error description to report to the user
         detail (str): A possibly multiline, more detailed error message
         reason (str): An optional machine readable reason string, used for test cases
 +       temporary (bool): An indicator to whether the error may occur if the operation was run again. (*Since: 1.2*)
      """
 -    def __init__(self, message, *, detail=None, reason=None):
 -        super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason)
 +    def __init__(self, message, *, detail=None, reason=None, temporary=False):
 +        super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, reason=reason, temporary=temporary)
  class Source(Plugin):

tests/sources/deb.py

@@ -45,7 +45,7 @@ def test_no_ref(cli, tmpdir, datafiles):
      assert cli.get_element_state(project, 'target.bst') == 'no reference'
 -# Test that when I fetch a nonexistent URL, errors are handled gracefully.
 +# Test that when I fetch a nonexistent URL, errors are handled gracefully and a retry is performed.
  @pytest.mark.skipif(HAVE_ARPY is False, reason="arpy is not available")
  @pytest.mark.datafiles(os.path.join(DATA_DIR, 'fetch'))
  def test_fetch_bad_url(cli, tmpdir, datafiles):
@@ -56,6 +56,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 +    assert "Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

tests/sources/tar.py

@@ -56,7 +56,7 @@ def test_no_ref(cli, tmpdir, datafiles):
      assert cli.get_element_state(project, 'target.bst') == 'no reference'
 -# Test that when I fetch a nonexistent URL, errors are handled gracefully.
 +# Test that when I fetch a nonexistent URL, errors are handled gracefully and a retry is performed.
  @pytest.mark.datafiles(os.path.join(DATA_DIR, 'fetch'))
  def test_fetch_bad_url(cli, tmpdir, datafiles):
      project = os.path.join(datafiles.dirname, datafiles.basename)
@@ -66,6 +66,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 +    assert "Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

tests/sources/zip.py

@@ -43,7 +43,7 @@ def test_no_ref(cli, tmpdir, datafiles):
      assert cli.get_element_state(project, 'target.bst') == 'no reference'
 -# Test that when I fetch a nonexistent URL, errors are handled gracefully.
 +# Test that when I fetch a nonexistent URL, errors are handled gracefully and a retry is performed.
  @pytest.mark.datafiles(os.path.join(DATA_DIR, 'fetch'))
  def test_fetch_bad_url(cli, tmpdir, datafiles):
      project = os.path.join(datafiles.dirname, datafiles.basename)
@@ -53,6 +53,7 @@ def test_fetch_bad_url(cli, tmpdir, datafiles):
      result = cli.run(project=project, args=[
          'fetch', 'target.bst'
      ])
 +    assert "Try #" in result.stderr
      result.assert_main_error(ErrorDomain.STREAM, None)
      result.assert_task_error(ErrorDomain.SOURCE, None)

[Notes] [Git][BuildStream/buildstream][507-some-log-lines-appear-to-be-duplicates] 11 commits: _exceptions.py: Modify BstError API to allow optional retry

Tristan Maat pushed to branch 507-some-log-lines-appear-to-be-duplicates at BuildStream / buildstream

Commits:

12 changed files:

Changes: