Add 503 to list of retriable HTTP response codes

[andrewpollock]

Environment details

• OS type and version:
• Python version: Python 3.11.4
• pip version: pip 23.1.2 from /usr/local/lib/python3.11/site-packages/pip (python 3.11)
• google-cloud-storage version: 2.14.0

Steps to reproduce

1. Usage of Blob.upload_from_filename() seems to sometimes encounter a 503 response
2. This seems to result in an immediate failure, despite indications in

   python-storage/google/cloud/storage/retry.py

   Lines 28 to 38 in ae9a53b

   	_RETRYABLE_TYPES = (
   	api_exceptions.TooManyRequests, # 429
   	api_exceptions.InternalServerError, # 500
   	api_exceptions.BadGateway, # 502
   	api_exceptions.ServiceUnavailable, # 503
   	api_exceptions.GatewayTimeout, # 504
   	ConnectionError,
   	requests.ConnectionError,
   	requests_exceptions.ChunkedEncodingError,
   	requests_exceptions.Timeout,
   	)

   that api_exceptions.ServiceUnavailable is a 503 and should be retried.

Code example

https://github.com/google/osv.dev/blob/b705d0d0b7450ce94137624118a2b54a7f719147/docker/exporter/exporter.py#L57-L64

  def upload_single(self, bucket, source_path, target_path):
    """Upload a single file to a bucket."""
    logging.info('Uploading %s', target_path)
    try:
      blob = bucket.blob(target_path)
      blob.upload_from_filename(source_path)
    except Exception as e:
      logging.error('Failed to export: %s', e)

Stack trace

Failed to export: 503 POST https://storage.googleapis.com/upload/storage/v1/b/osv-test-vulnerabilities/o?uploadType=multipart: {
  "error": {
    "code": 503,
    "message": "We encountered an internal error. Please try again.",
    "errors": [
      {
        "message": "We encountered an internal error. Please try again.",
        "domain": "global",
        "reason": "backendError"
      }
    ]
  }
}
: ('Request failed with status code', 503, 'Expected one of', <HTTPStatus.OK: 200>)

Perhaps 503 needs to be added to _ADDITIONAL_RETRYABLE_STATUS_CODES also?

[cojenco]

As you pointed out, 503 and connection errors are considered retryable in the python client. However, retries are only enabled by default for uploads if the upload is confirmed to be idempotent - that is by including request preconditions if_generation_match on the upload method call.

The retry strategy is outlined in https://cloud.google.com/storage/docs/retry-strategy#python. This docs go into detail about which operations are conditionally idempotent, retryable exceptions and client retry config options. Here is a sample code that demonstrates adding the generation match precondition to an upload.

In your code sample, assuming the upload is for a new file, set if_generation_match=0to enable retries; same goes for #1231

def upload_single(self, bucket, source_path, target_path):
"""Upload a single file to a bucket."""
logging.info('Uploading %s', target_path)
try:
    blob = bucket.blob(target_path)
    blob.upload_from_filename(source_path, if_generation_match=0)
except Exception as e:
    logging.error('Failed to export: %s', e)

[andrewpollock]

Hi @cojenco thank you for your fast response.

My reading of

python-storage/google/cloud/storage/blob.py

Lines 2827 to 2841 in ae9a53b

	def upload_from_filename(
	self,
	filename,
	content_type=None,
	num_retries=None,
	client=None,
	predefined_acl=None,
	if_generation_match=None,
	if_generation_not_match=None,
	if_metageneration_match=None,
	if_metageneration_not_match=None,
	timeout=_DEFAULT_TIMEOUT,
	checksum=None,
	retry=DEFAULT_RETRY_IF_GENERATION_SPECIFIED,
	):

and the definition of

python-storage/google/cloud/storage/retry.py

Lines 141 to 143 in ae9a53b

	DEFAULT_RETRY_IF_GENERATION_SPECIFIED = ConditionalRetryPolicy(
	DEFAULT_RETRY, is_generation_specified, ["query_params"]
	)

was that:

1. the default retry behaviour for upload_from_filename() is DEFAULT_RETRY_IF_GENERATION_SPECIFIED
2. the definition of DEFAULT_RETRY_IF_GENERATION_SPECIFIED is the superset of DEFAULT_RETRY and a generation precondition being met

Therefore, I am referring to the DEFAULT_RETRY behaviour being deficient for the handling of 503s and connection errors.

Have I misunderstood something?

In your code sample, assuming the upload is for a new file

No, our use case is to successfully upload new or overwrite existing files, so I don't believe that having a precondition on the generation being zero is going to comprehensively assist in this case, if I understand correctly?

[cojenco]

Hi @andrewpollock thanks for the follow-up.

1. the default retry behaviour for upload_from_filename() is DEFAULT_RETRY_IF_GENERATION_SPECIFIED
2. the definition of DEFAULT_RETRY_IF_GENERATION_SPECIFIED is the superset of DEFAULT_RETRY and a generation precondition being met

Your understanding in (1) and (2) are accurate.

However, to clarify your following point:

Therefore, I am referring to the DEFAULT_RETRY behaviour being deficient for the handling of 503s and connection errors.

DEFAULT_RETRY does handle 503s and connection errors as long as the error is in the retryable excpetions list. As shown below, DEFAULT_RETRY = retry.Retry(predicate=_should_retry) _should_retry predicate will always retry errors that are considered retryable.

python-storage/google/cloud/storage/retry.py

Lines 45 to 61 in ae9a53b

	def _should_retry(exc):
	"""Predicate for determining when to retry."""
	if isinstance(exc, _RETRYABLE_TYPES):
	return True
	elif isinstance(exc, api_exceptions.GoogleAPICallError):
	return exc.code in _ADDITIONAL_RETRYABLE_STATUS_CODES
	elif isinstance(exc, auth_exceptions.TransportError):
	return _should_retry(exc.args[0])
	else:
	return False
	DEFAULT_RETRY = retry.Retry(predicate=_should_retry)
	"""The default retry object.
	This retry setting will retry all _RETRYABLE_TYPES and any status codes from
	_ADDITIONAL_RETRYABLE_STATUS_CODES.

Given your use case has a mix of new and existing objects, a few options worth considering:
(a) add if_generation_match precondition using default retry strategy DEFAULT_RETRY_IF_GENERATION_SPECIFIED

blob = bucket.get_blob(target_path)
generation_match = 0

# For existing objects, get the object generation
if blob is not None:
   generation_match = blob.generation

blob.upload_from_filename(source_path, if_generation_match=generation_match)

(b) adjust the retry strategy to always retry retryable errors

blob.upload_from_filename(source_path, retry=DEFAULT_RETRY)

Setting retry=DEFAULT_RETRY on the upload method call will trigger an exponential retry for 503s and connection errors; though there is the risk of potential race conditions.

Please let me know if this doesn't answer your question.

[andrewpollock]

Your understanding in (1) and (2) are accurate.

Thank you. Today, I'm wondering if this is an "and" versus "or" misunderstanding on my behalf, with respect to (2)?

DEFAULT_RETRY does handle 503s and connection errors as long as the error is in the retryable excpetions list. As shown below, DEFAULT_RETRY = retry.Retry(predicate=_should_retry) _should_retry predicate will always retry errors that are considered retryable.

This was my point. Based on the behaviour I'm reporting here and in #1231 it doesn't appear to be the case? But maybe it's because of my misunderstanding as per above?