Make mirror_file fail if file object already exists (#7134) #7141
Conversation
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
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
from
2aa836e to
5ea6493
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7141 +/- ##
===========================================
- Coverage 85.24% 85.22% -0.02%
===========================================
Files 152 152
Lines 22060 22099 +39
===========================================
+ Hits 18804 18834 +30
- Misses 3256 3265 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dsotirho-ucsc
requested changes
May 20, 2025
There was a problem hiding this comment.
If I understand this correctly, an attempt to mirror a file that already exists in the destination will cause S3 to return a 413 (due to IfNoneMatch = *), however there is no R assertion as described in the ticket description. Was it decided the 413 was sufficient?
Also, the two parts of the reupload subtest both start by calling self._s3.delete_object(). Shouldn't this include a test where the file is not deleted and then reuploaded?
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
2 times, most recently
from
12ff57e to
e5cc410
Compare
There are two objects, the file object and the info object. If the info object is present, the mirror service will skip trying to upload the file object, so all subtests after the first will skip the parts of the code we need coverage for. The two ways to circumvent this are to delete the info object or patch the method that checks for it. For some reason I had concluded the former was preferable but I can't remember my reasoning so I switched back to the latter. |
hannes-ucsc
requested changes
May 21, 2025
src/azul/service/storage_service.py
Outdated
| @@ -181,12 +198,16 @@ def upload(self, | |||
|
|
|||
| def _object_creation_kwargs(self, *, | |||
| content_type: str | None = None, | |||
| tagging: Tagging | None = None): | |||
| tagging: Tagging | None = None, | |||
| exists_okay: bool = True | |||
There was a problem hiding this comment.
Suggested change
| exists_okay: bool = True | |
| overwrite: bool = True |
and everywhere else.
src/azul/service/storage_service.py
Outdated
| **kwargs) | ||
| except botocore.exceptions.ClientError as e: | ||
| error = e.response['Error'] | ||
| if error['Code'] == 'PreconditionFailed' and error['Condition'] == 'If-None-Match': |
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
from
e5cc410 to
fc3d6c1
Compare
hannes-ucsc
requested changes
May 21, 2025
src/azul/service/storage_service.py
Outdated
| parts = [ | ||
| { | ||
| 'PartNumber': index + 1, | ||
| 'ETag': etag | ||
| } | ||
| for index, etag in enumerate(etags) | ||
| ] | ||
| upload.complete(MultipartUpload={'Parts': parts}) | ||
| upload.complete(MultipartUpload={'Parts': parts}, |
There was a problem hiding this comment.
That same exception needs to be handled here as well.
Additionally, we don't want to create a big MP upload only to then realize at the end that the object already exists. The proper way to deal with this for large files is to check explicitly with HeadObject. I don't know if PutObject with If-None-Match fails early before processing the entire request body or after. Unless you can find documentation about this, please add the HeadObject for small files as well.
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
2 times, most recently
from
f36a492 to
d3318e2
Compare
hannes-ucsc
requested changes
May 23, 2025
There was a problem hiding this comment.
Subject: [PATCH] Clean-up mirroring fixture duplication
---
Index: test/indexer/test_mirror_controller.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/indexer/test_mirror_controller.py b/test/indexer/test_mirror_controller.py
--- a/test/indexer/test_mirror_controller.py (revision 1c0a00197edc274bb9804375a59b3ecf3a9dfe3a)
+++ b/test/indexer/test_mirror_controller.py (date 1748042321516)
@@ -78,12 +78,14 @@
with self.subTest('mirror_file', corrupted=False, exists=False):
self._test_mirror_file(file, file_message)
- # Force reupload attempts even if the info object is present
- with patch.object(MirrorService, 'info_exists', return_value=False):
- with self.subTest('mirror_file', corrupted=True):
- self._test_corrupted_download(file_message)
- with self.subTest('mirror_file', corrupted=False, exists=True):
- self._test_reuploaded_file(file_message)
+ self._s3.delete_object(Bucket=self.mirror_bucket,
+ Key=self.mirror_controller.service.info_object_key(file))
+
+ with self.subTest('mirror_file', corrupted=True):
+ self._test_corrupted_download(file_message)
+
+ with self.subTest('mirror_file', corrupted=False, exists=True):
+ self._test_reuploaded_file(file_message)
_file_contents = b'lorem ipsum dolor sit\n'
Index: src/azul/indexer/mirror_controller.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/indexer/mirror_controller.py b/src/azul/indexer/mirror_controller.py
--- a/src/azul/indexer/mirror_controller.py (revision 1c0a00197edc274bb9804375a59b3ecf3a9dfe3a)
+++ b/src/azul/indexer/mirror_controller.py (date 1748042570717)
@@ -152,12 +152,12 @@
deployment_is_stable = (config.deployment.is_stable
and not config.deployment.is_unit_test
and catalog not in config.integration_test_catalogs)
- if self.service.info_exists(catalog, file):
+ if file_is_large and not deployment_is_stable:
+ log.info('Not mirroring file to save cost: %r', file)
+ elif self.service.info_exists(catalog, file):
log.info('File is already mirrored, skipping upload: %r', file)
elif self.service.file_exists(catalog, file):
assert False, R('File object is already present', file)
- elif file_is_large and not deployment_is_stable:
- log.info('Not mirroring file to save cost: %r', file)
else:
# Ensure we test with multiple parts on lower deployments
part_size = FilePart.default_size if deployment_is_stable else FilePart.min_size
Index: src/azul/service/storage_service.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/azul/service/storage_service.py b/src/azul/service/storage_service.py
--- a/src/azul/service/storage_service.py (revision 1c0a00197edc274bb9804375a59b3ecf3a9dfe3a)
+++ b/src/azul/service/storage_service.py (date 1748043612861)
@@ -77,6 +77,10 @@
pass
+class StorageObjectExists(Exception):
+ pass
+
+
class StorageService:
def __init__(self, bucket_name: str | None = None):
@@ -94,7 +98,8 @@
Key=object_key)
except self._s3.exceptions.ClientError as e:
if int(e.response['Error']['Code']) == 404:
- raise StorageObjectNotFound
+ # REVIEW: separate commit
+ raise StorageObjectNotFound(object_key)
else:
raise e
@@ -103,7 +108,8 @@
response = self._s3.get_object(Bucket=self.bucket_name,
Key=object_key)
except self._s3.exceptions.NoSuchKey:
- raise StorageObjectNotFound
+ # REVIEW: same commit as above
+ raise StorageObjectNotFound(object_key)
else:
return response['Body'].read()
@@ -309,7 +315,7 @@
error = exception.response['Error']
code, condition = error['Code'], error['Condition']
if code == 'PreconditionFailed' and condition == 'If-None-Match':
- assert False, R('Object exists', object_key)
+ raise StorageObjectExists(object_key)
else:
raise exception
The last two commits should either be squashed or made into a split commit. Please change title StorageService supports IfNoneMatch param to Optionally prevent StorageService from overwriting objects
nadove-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
from
d3318e2 to
19410cd
Compare
hannes-ucsc
approved these changes
May 28, 2025
Security design review
|
dsotirho-ucsc
force-pushed
the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
from
19410cd to
f0d2f2b
Compare
dsotirho-ucsc
added
the
sandbox
dsotirho-ucsc
deleted the
issues/nadove-ucsc/7134-mirror-file-fail-object-exists
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Connected issues: #7134
Checklist
Author
developissues/<GitHub handle of author>/<issue#>-<slug>1 when the issue title describes a problem, the corresponding PR
title is
Fix:followed by the issue titleAuthor (partiality)
ptag to titles of partial commitspartialor completely resolves all connected issuespartiallabelAuthor (chains)
baseor this PR is not chained to another PRchainedor is not chained to another PRAuthor (reindex, API changes)
rtag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:devor the changes introduced by it will not require reindexing ofdevreindex:anvildevor the changes introduced by it will not require reindexing ofanvildevreindex:anvilprodor the changes introduced by it will not require reindexing ofanvilprodreindex:prodor the changes introduced by it will not require reindexing ofprodreindex:partialand its description documents the specific reindexing procedure fordev,anvildev,anvilprodandprodor requires a full reindex or carries none of the labelsreindex:dev,reindex:anvildev,reindex:anvilprodandreindex:prodAPIor this PR does not modify a REST APIa(A) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.pyor this PR does not modify a REST APIAuthor (upgrading deployments)
make docker_images.jsonand committed the resulting changes or this PR does not modifyazul_docker_images, or any other variables referenced in the definition of that variableutag to commit title or this PR does not require upgrading deploymentsupgradeor does not require upgrading deploymentsdeploy:sharedor does not modifydocker_images.json, and does not require deploying thesharedcomponent for any other reasondeploy:gitlabor does not require deploying thegitlabcomponentdeploy:runneror does not require deploying therunnerimageAuthor (hotfixes)
Ftag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprodandprod) have temporary hotfixes for any of the issues connected to this PRAuthor (before every review)
develop, squashed old fixupsmake requirements_updateor this PR does not modifyrequirements*.txt,common.mk,MakefileandDockerfileRtag to commit title or this PR does not modifyrequirements*.txtreqsor does not modifyrequirements*.txtmake integration_testpasses in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
System administrator (after approval)
demoorno demono demono sandboxN reviewslabel is accurateOperator (before pushing merge the commit)
reindex:…labels andrcommit title tagno demodevelop_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlab_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unusedor this PR is not labeleddeploy:shared_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab applyor this PR is not labeleddeploy:gitlabdeploy:gitlabdeploy:gitlabSystem administrator
dev.gitlabare complete or this PR is not labeleddeploy:gitlabanvildev.gitlabare complete or this PR is not labeleddeploy:gitlabOperator (before pushing merge the commit)
_select dev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runner_select anvildev.gitlab && make -C terraform/gitlab/runneror this PR is not labeleddeploy:runnersandboxlabel or PR is labeledno sandboxdevor PR is labeledno sandboxanvildevor PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxdeployment or PR is labeledno sandboxanvilboxdeployment or PR is labeledno sandboxsandboxor this PR does not remove catalogs or otherwise causes unreferenced indices indevanvilboxor this PR does not remove catalogs or otherwise causes unreferenced indices inanvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevsandboxor this PR is not labeledreindex:devanvilboxor this PR is not labeledreindex:anvildevpif the PR is also labeledpartialOperator (chain shortening)
developor this PR is not labeledbasechainedlabel from the blocked PR or this PR is not labeledbasebasebaselabel from this PR or this PR is not labeledbaseOperator (after pushing the merge commit)
devanvildevdevdevanvildevanvildev_select dev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shared_select anvildev.shared && make -C terraform/shared applyor this PR is not labeleddeploy:shareddevanvildevOperator (reindex)
devor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR is neither labeledreindex:partialnorreindex:devanvildevor this PR is neither labeledreindex:partialnorreindex:anvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdevor this PR does not require reindexingdevdevor this PR does not require reindexingdevdeploy_browserjob in the GitLab pipeline for this PR indevor this PR does not require reindexingdevanvildevor this PR does not require reindexinganvildevdeploy_browserjob in the GitLab pipeline for this PR inanvildevor this PR does not require reindexinganvildevOperator
deploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels to the next promotion PRs or this PR carries none of these labelsdeploy:shared,deploy:gitlab,deploy:runner,API,reindex:partial,reindex:anvilprodandreindex:prodlabels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
Lline is too longWline wrapping is wrongQbad quotesFother formatting problem