From 4ad5a458c2927be557f876a5e66928a02c67f87d Mon Sep 17 00:00:00 2001 From: Kenny Lee Sin Cheong <2530351+kleesc@users.noreply.github.com> Date: Wed, 21 Apr 2021 13:53:28 -0400 Subject: [PATCH] chore: remove uploading filtering from imagestorage queries (PROJQUAY-1914) (#764) The "uploading" column is an artifact from depending on writing to the Image table (see BlobUpload table instead). As of 3.4, Quay no longer writes to that table, and is only needed until quayio moves away from Clair v2, after which work to remove "glue" code and fully deprecate the Image table (amongst other tables) can start. This is done as a separate commit from the actual migration so that it can be cherrypicked. --- data/model/blob.py | 27 ++++++++----------- data/model/image.py | 8 +++--- data/model/oci/blob.py | 2 -- data/model/oci/manifest.py | 3 +-- data/model/storage.py | 3 +-- data/model/test/test_gc.py | 6 ++--- data/model/test/test_image_sharing.py | 10 ------- data/registry_model/manifestbuilder.py | 1 - data/registry_model/test/test_blobuploader.py | 1 - data/registry_model/test/test_interface.py | 2 -- tools/orphans.py | 1 - util/verifyplacements.py | 2 +- 12 files changed, 20 insertions(+), 46 deletions(-) diff --git a/data/model/blob.py b/data/model/blob.py index 70ee097935..c92e9c2bc2 100644 --- a/data/model/blob.py +++ b/data/model/blob.py @@ -80,7 +80,6 @@ def store_blob_record_and_temp_link_in_repo( except ImageStorage.DoesNotExist: storage = ImageStorage.create( content_checksum=blob_digest, - uploading=False, image_size=byte_count, uncompressed_size=uncompressed_byte_count, ) @@ -217,7 +216,7 @@ def get_shared_blob(digest): """ assert digest try: - return ImageStorage.get(content_checksum=digest, uploading=False) + return ImageStorage.get(content_checksum=digest) except ImageStorage.DoesNotExist: return None @@ -235,22 +234,18 @@ def get_or_create_shared_blob(digest, byte_data, storage): assert storage try: - return ImageStorage.get(content_checksum=digest, uploading=False) + return ImageStorage.get(content_checksum=digest) except ImageStorage.DoesNotExist: - record = ImageStorage.create( - image_size=len(byte_data), content_checksum=digest, cas_path=True, uploading=True - ) preferred = storage.preferred_locations[0] location_obj = ImageStorageLocation.get(name=preferred) - try: - storage.put_content([preferred], storage_model.get_layer_path(record), byte_data) - ImageStoragePlacement.create(storage=record, location=location_obj) - record.uploading = False - record.save() - except: - logger.exception("Exception when trying to write special layer %s", digest) - record.delete_instance() - raise + with db_transaction(): + record = ImageStorage.create(image_size=len(byte_data), content_checksum=digest) + try: + storage.put_content([preferred], storage_model.get_layer_path(record), byte_data) + ImageStoragePlacement.create(storage=record, location=location_obj) + except: + logger.exception("Exception when trying to write special layer %s", digest) + raise - return record + return record diff --git a/data/model/image.py b/data/model/image.py index 40c5931bb6..2c19e95327 100644 --- a/data/model/image.py +++ b/data/model/image.py @@ -316,7 +316,7 @@ def find_create_or_link_image( .join(RepositoryPermission, JOIN.LEFT_OUTER) .switch(Repository) .join(Namespace, on=(Repository.namespace_user == Namespace.id)) - .where(ImageStorage.uploading == False, Image.docker_image_id == docker_image_id) + .where(Image.docker_image_id == docker_image_id) ) existing_image_query = _basequery.filter_to_repos_for_user( @@ -524,10 +524,8 @@ def get_images_eligible_for_scan(clair_version): """ Returns a query that gives all images eligible for a clair scan. """ - return ( - get_image_with_storage_and_parent_base() - .where(Image.security_indexed_engine < clair_version) - .where(ImageStorage.uploading == False) + return get_image_with_storage_and_parent_base().where( + Image.security_indexed_engine < clair_version ) diff --git a/data/model/oci/blob.py b/data/model/oci/blob.py index 5ed9796aa5..dfdb77e741 100644 --- a/data/model/oci/blob.py +++ b/data/model/oci/blob.py @@ -25,7 +25,6 @@ def _lookup_blob_uploaded(repository, blob_digest): .where( UploadedBlob.repository == repository, ImageStorage.content_checksum == blob_digest, - ImageStorage.uploading == False, ) .get() ) @@ -41,7 +40,6 @@ def _lookup_blob_in_repository(repository, blob_digest): .where( ManifestBlob.repository == repository, ImageStorage.content_checksum == blob_digest, - ImageStorage.uploading == False, ) .get() ) diff --git a/data/model/oci/manifest.py b/data/model/oci/manifest.py index da260de286..d631a2b6da 100644 --- a/data/model/oci/manifest.py +++ b/data/model/oci/manifest.py @@ -26,7 +26,7 @@ from data.model.oci.tag import filter_to_alive_tags, create_temporary_tag_if_necessary from data.model.oci.label import create_manifest_label from data.model.oci.retriever import RepositoryContentRetriever -from data.model.storage import lookup_repo_storages_by_content_checksum, create_v1_storage +from data.model.storage import lookup_repo_storages_by_content_checksum from data.model.image import lookup_repository_images, get_image, synthesize_v1_image from image.docker.schema2 import EMPTY_LAYER_BLOB_DIGEST, EMPTY_LAYER_BYTES from image.docker.schema1 import ManifestException @@ -443,7 +443,6 @@ def _build_blob_map( shared_blob = get_or_create_shared_blob( EMPTY_LAYER_BLOB_DIGEST, EMPTY_LAYER_BYTES, storage ) - assert not shared_blob.uploading assert shared_blob.content_checksum == EMPTY_LAYER_BLOB_DIGEST blob_map[EMPTY_LAYER_BLOB_DIGEST] = shared_blob diff --git a/data/model/storage.py b/data/model/storage.py index 2d43b22316..c3f7cffd37 100644 --- a/data/model/storage.py +++ b/data/model/storage.py @@ -255,7 +255,7 @@ def placements_to_filtered_paths_set(placements_list): def create_v1_storage(location_name): - storage = ImageStorage.create(cas_path=False, uploading=True) + storage = ImageStorage.create(cas_path=False) location = get_image_location_for_name(location_name) ImageStoragePlacement.create(location=location.id, storage=storage) storage.locations = {location_name} @@ -382,7 +382,6 @@ def _lookup_repo_storages_by_content_checksum(repo, checksums, model_class): ImageStorage.uuid, ImageStorage.cas_path, ImageStorage.uncompressed_size, - ImageStorage.uploading, ) .join(model_class) .where(model_class.repository == repo, ImageStorage.content_checksum == checksum) diff --git a/data/model/test/test_gc.py b/data/model/test/test_gc.py index fd3ad4603e..23537085a9 100644 --- a/data/model/test/test_gc.py +++ b/data/model/test/test_gc.py @@ -615,7 +615,7 @@ def test_image_with_cas(default_tag_policy, initialized_db): preferred = storage.preferred_locations[0] storage.put_content({preferred}, storage.blob_path(digest), content) - image_storage = database.ImageStorage.create(content_checksum=digest, uploading=False) + image_storage = database.ImageStorage.create(content_checksum=digest) location = database.ImageStorageLocation.get(name=preferred) database.ImageStoragePlacement.create(location=location, storage=image_storage) @@ -676,8 +676,8 @@ def test_images_shared_cas(default_tag_policy, initialized_db): preferred = storage.preferred_locations[0] storage.put_content({preferred}, storage.blob_path(digest), content) - is1 = database.ImageStorage.create(content_checksum=digest, uploading=False) - is2 = database.ImageStorage.create(content_checksum=digest, uploading=False) + is1 = database.ImageStorage.create(content_checksum=digest) + is2 = database.ImageStorage.create(content_checksum=digest) location = database.ImageStorageLocation.get(name=preferred) diff --git a/data/model/test/test_image_sharing.py b/data/model/test/test_image_sharing.py index 4e506c1865..74ff6806b7 100644 --- a/data/model/test/test_image_sharing.py +++ b/data/model/test/test_image_sharing.py @@ -43,8 +43,6 @@ def createStorage(storage, docker_image_id, repository=REPO, username=ADMIN_ACCE image = model.image.find_create_or_link_image( docker_image_id, repository_obj, username, {}, preferred ) - image.storage.uploading = False - image.storage.save() return image.storage @@ -282,11 +280,3 @@ def test_org_not_team_member_with_no_access(storage, initialized_db): assertDifferentStorage( storage, "the-image", first_storage, username=OUTSIDE_ORG_USER, repository=OUTSIDE_ORG_REPO ) - - -def test_no_link_to_uploading(storage, initialized_db): - still_uploading = createStorage(storage, "an-image", repository=PUBLIC_REPO) - still_uploading.uploading = True - still_uploading.save() - - assertDifferentStorage(storage, "an-image", still_uploading) diff --git a/data/registry_model/manifestbuilder.py b/data/registry_model/manifestbuilder.py index 750d9d1bc6..fffc9193d4 100644 --- a/data/registry_model/manifestbuilder.py +++ b/data/registry_model/manifestbuilder.py @@ -150,7 +150,6 @@ def assign_layer_blob(self, layer, blob, computed_checksums): Assigns a blob to a layer. """ assert blob - assert not blob.uploading image_metadata = self._builder_state.image_metadata.get(layer.layer_id) if image_metadata is None: diff --git a/data/registry_model/test/test_blobuploader.py b/data/registry_model/test/test_blobuploader.py index 51ef722c64..ac5450baa2 100644 --- a/data/registry_model/test/test_blobuploader.py +++ b/data/registry_model/test/test_blobuploader.py @@ -68,7 +68,6 @@ def test_basic_upload_blob(chunk_count, subchunk, registry_model): # Check the blob. assert blob.compressed_size == len(data) - assert not blob.uploading assert blob.digest == "sha256:" + hashlib.sha256(data).hexdigest() # Ensure the blob exists in storage and has the expected data. diff --git a/data/registry_model/test/test_interface.py b/data/registry_model/test/test_interface.py index ce235d1c6c..71751f1846 100644 --- a/data/registry_model/test/test_interface.py +++ b/data/registry_model/test/test_interface.py @@ -645,7 +645,6 @@ def test_get_cached_repo_blob(registry_model): assert found.uuid == blob.uuid assert found.compressed_size == blob.compressed_size assert found.uncompressed_size == blob.uncompressed_size - assert found.uploading == blob.uploading assert found.placements == blob.placements # Disconnect from the database by overwriting the connection. @@ -662,7 +661,6 @@ def fail(x, y): assert cached.uuid == blob.uuid assert cached.compressed_size == blob.compressed_size assert cached.uncompressed_size == blob.uncompressed_size - assert cached.uploading == blob.uploading assert cached.placements == blob.placements # Try another blob, which should fail since the DB is not connected and the cache diff --git a/tools/orphans.py b/tools/orphans.py index 7fc41bfdd5..e64810475b 100644 --- a/tools/orphans.py +++ b/tools/orphans.py @@ -4,7 +4,6 @@ orphaned = ( ImageStorage.select() - .where(ImageStorage.uploading == False) .join(Image, JOIN.LEFT_OUTER) .group_by(ImageStorage) .having(fn.Count(Image.id) == 0) diff --git a/util/verifyplacements.py b/util/verifyplacements.py index b7fceecabd..f698ea8d48 100644 --- a/util/verifyplacements.py +++ b/util/verifyplacements.py @@ -33,7 +33,7 @@ def verify_placements(): encountered = set() iterator = yield_random_entries( - lambda: ImageStorage.select().where(ImageStorage.uploading == False), + lambda: ImageStorage.select(), ImageStorage.id, 1000, ImageStorage.select(fn.Max(ImageStorage.id)).scalar(),