fix(s3compatsigv4): ストレージ側クォータ超過時の異常系ハンドリング - #98
Draft
tishin-endou wants to merge 5 commits into
Draft
Conversation
_abort_chunked_upload() は abort 成功時に True を返すが、_chunked_upload() の判定が `if aborted:` となっており、abort に成功した時に「一時パーツの クリーンアップに失敗した。手動で削除してほしい」という誤った警告を付加し、 逆に abort が失敗した時には警告を出していなかった。 利用者に残存パーツの有無を正反対に伝えてしまうため、判定を `if not aborted:` に修正する。既存テスト test_chunked_upload_aborted_success は誤った挙動を 期待していたため、期待値も併せて修正した。
クラウドストレージ側のクォータを超過した際、アップロードが失敗したことも その原因も利用者に伝わらない問題を修正する。 - _parse_s3_error_body(): S3 の XML エラーから Code / Message を安全に抽出 (非 XML・パース不能時は (None, None) を返す) - _translate_upload_error(): QUOTA_EXCEEDED_ERROR_CODES に該当するコードを HTTP 507 + 明示メッセージへ変換。その他の S3 エラーはステータスコードを 保持しつつ生 XML ではなく可読メッセージにする。解釈不能な場合は元の エラーをそのまま返す - _chunked_upload(): 従来は実エラーを捨てて常に 500 を返していたため原因が 失われていた。UploadError は変換して原因とステータスを保全する - _contiguous_upload(): 同様に変換し、アップロード中の接続断 (aiohttp.ClientError) は 502 + 明示メッセージとして扱う - settings.py: QUOTA_EXCEEDED_ERROR_CODES を追加(ベンダー差異に対応するため config で拡張可能)
- test_chunked_upload_abort_failure_appends_warning: abort 失敗時のみ警告付加 - test_chunked_upload_storage_quota_exceeded: 507 + 明示メッセージ + abort 実行 - test_contiguous_upload_storage_quota_exceeded: contiguous 経路でも同様 - test_contiguous_upload_other_storage_error: 非クォータエラーはコード保持 - test_contiguous_upload_connection_interrupted: 接続断は 502 - test_parse_s3_error_body_non_xml: 非 XML ボディの入力バリデーション
_create_upload_session() の呼び出しが try ブロックの外にあり、セッション作成 時点でストレージがクォータ拒否した場合に変換処理を通らず、生の XML エラーが そのまま利用者に返っていた。 - _chunked_upload(): セッション作成呼び出しを try で囲み、UploadError は _translate_upload_error() で変換、aiohttp.ClientError は 502 に変換する。 セッションは未作成なので abort は行わない - _create_upload_session(): 応答 XML から UploadId を取り出す処理が無防備で、 不正応答時に素の ExpatError / KeyError が 500 になっていた。 (ExpatError, KeyError, TypeError) を捕捉して 502 + 明示メッセージに変換する。 この時点では UploadId 不明の孤児セッションがストレージ側に残る可能性がある ため、手動確認用に key とボディ先頭 512 バイトをログ出力する
- test_chunked_upload_create_session_quota_exceeded: セッション作成時の クォータ拒否が 507 になり、セッション未作成のため abort が呼ばれないこと - test_create_upload_session_invalid_response: 200 応答だがボディが不正 XML の 場合に素の 500 ではなく 502 + 明示メッセージになること
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket
Purpose
S3CompatSigV4 の機関ストレージで、クラウドストレージ側のクォータを超過した際に
アップロードが失敗したことも、その原因も利用者に伝わらない問題を修正します。
利用者からは「処理が終わったのか失敗したのか分からない」という申告でした。
原因は3点(+監査で1点追加)です。
_chunked_upload()がストレージの実エラー(例: 403 + XML
QuotaExceeded)を捨てて、常にAn unexpected error has occurred during the multi-part upload.(HTTP 500)に置き換えていた。利用者には原因不明の 500 しか届かない
_abort_chunked_upload()は成功時にTrueを返すが判定が
if aborted:だったため、abort に成功した時に「一時パーツのクリーンアップに失敗した。手動で削除してほしい」と誤警告し、失敗時には警告を出して
いなかった。残存パーツの有無を正反対に伝えていた
エラーメッセージになり判読不能。また超過時に接続を切るストレージ実装では
aiohttp.ClientErrorが捕捉されず原因不明の 500 になっていた_create_upload_session()の呼び出しが try 外にあり、セッション作成時点でのクォータ拒否が変換処理を通らなかった。さらに応答 XML のパースが無防備で、
不正応答時に素の ExpatError / KeyError が 500 になっていた
Changes
コミットは論理単位で5分割しています。
fix: abort 成否判定の反転を修正(if not aborted:)fix: ストレージ側エラーを利用者向けエラーに変換_parse_s3_error_body(): S3 XML エラーから Code / Message を安全に抽出(非 XML・パース不能時は
(None, None))_translate_upload_error(): クォータ系コードを HTTP 507 (InsufficientStorage) + 明示メッセージへ変換。その他の S3 エラーはステータスコードを
保持しつつ生 XML ではなく可読メッセージにする。解釈不能なら元エラーを
そのまま返す(フェイルセーフ)
_chunked_upload()/_contiguous_upload(): 上記を適用し、接続断(
aiohttp.ClientError)は 502 + 明示メッセージに変換settings.py:QUOTA_EXCEEDED_ERROR_CODESを追加(
QuotaExceeded/XMinioAdminBucketQuotaExceeded/InsufficientStorage。ベンダー差異に対応するため config で拡張可能)
fix: セッション作成経路の未ハンドリングを修正(未作成のため abort は行わない。UploadId 不明の孤児セッションが残り得るため手動確認用のログを出力)
test: 異常系テスト計8件を追加、既存1件を修正(誤った挙動を期待していたため)Side effects
接続断は 500 → 502。500 前提のクライアント側処理があれば影響します
s3compatsigv4配下に限定しており、他プロバイダには影響しませんs3/provider.py:276とs3compat/provider.py:377(継承先の
s3compatinstitutionsにも波及)に存在しますが、本 PR のスコープ外とし別チケットを推奨します
boto3
delete_objectsの例外未捕捉(3箇所)、_check_for_200_errorのExpatError 素通し・S3 エラーコード喪失・
resp.release()スキップによる接続リークQA Notes
済: ユニットテスト(GitHub Actions / pinned Docker 環境)
python:3.6-slim-buster+ pinneddev-requirements.txtで全件実行しています。f9fcd7cd(run 30882125906)collected / passed が基準からちょうど +8 で、追加した異常系テスト8件が全件実行され
PASS しています(9件目は既存テストの期待値修正)。skipped は 10 件のまま増加せず、
failed はゼロのため 回帰なしです。
invoke testは先頭でflake8 .を実行するためlint も通過しています。
未実施: 実ストレージ E2E(このため Draft のままにしています)
mc quota set)を設定し、**contiguous(小容量)**と**chunked(
CONTIGUOUS_UPLOAD_SIZE_LIMIT超)**の両方で超過させるListPartsし、パーツが残留していないことまた、GRDM のフロントエンドが WB の 507 メッセージを利用者に表示するかは未確認です。
表示されない場合はフロントエンド側の対応を別チケットとして起票し、本 PR には含めません。
Deployment Notes
特別な設定は不要です。ストレージベンダーが独自のクォータ超過エラーコードを返す場合のみ、
provider config で
QUOTA_EXCEEDED_ERROR_CODESを拡張してください。