Merge pull request #3340 from quay/manifest-lookup-optimization

Fix bug where we weren't properly caching enum values and we were always looking up the media type when constructing manifests

data/database.py

@@ -398,6 +398,16 @@ class QuayUserField(ForeignKeyField):
     super(QuayUserField, self).__init__(*args, **kwargs)
 
 
+@lru_cache(maxsize=16)
+def _get_enum_field_values(enum_field):
+  values = []
+  for row in enum_field.rel_model.select():
+    key = getattr(row, enum_field.enum_key_field)
+    value = getattr(row, 'id')
+    values.append((key, value))
+  return Enum(enum_field.rel_model.__name__, values)
+
+
 class EnumField(ForeignKeyField):
   """ Create a cached python Enum from an EnumTable """
   def __init__(self, model, enum_key_field='name', *args, **kwargs):
@@ -409,15 +419,9 @@ class EnumField(ForeignKeyField):
     super(EnumField, self).__init__(model, *args, **kwargs)
 
   @property
-  @lru_cache(maxsize=1)
   def enum(self):
     """ Returns a python enun.Enum generated from the associated EnumTable """
-    values = []
-    for row in self.rel_model.select():
-      key = getattr(row, self.enum_key_field)
-      value = getattr(row, 'id')
-      values.append((key, value))
-    return Enum(self.rel_model.__name__, values)
+    return _get_enum_field_values(self)
 
   def get_id(self, name):
     """ Returns the ForeignKeyId from the name field

data/registry_model/datatypes.py

@@ -6,6 +6,7 @@ from enum import Enum, unique
 from cachetools import lru_cache
 
 from data import model
+from data.database import Manifest as ManifestTable
 from data.registry_model.datatype import datatype, requiresinput, optionalinput
 from image.docker import ManifestException
 from image.docker.schemas import parse_manifest_from_bytes
@@ -197,7 +198,7 @@ class Manifest(datatype('Manifest', ['digest', 'media_type', 'internal_manifest_
     return Manifest(db_id=manifest.id,
                     digest=manifest.digest,
                     internal_manifest_bytes=Bytes.for_string_or_unicode(manifest.manifest_bytes),
-                    media_type=manifest.media_type.name,
+                    media_type=ManifestTable.media_type.get_name(manifest.media_type_id),
                     inputs=dict(legacy_image=legacy_image, tag_manifest=False))
 
   @property

data/registry_model/test/test_interface.py

@@ -256,17 +256,31 @@ def test_repository_tags(repo_namespace, repo_name, registry_model):
     assert tags_map[tag.name] == found_image.docker_image_id
 
 
-def test_repository_tag_history(registry_model):
-  repository_ref = registry_model.lookup_repository('devtable', 'history')
+@pytest.mark.parametrize('namespace, name, expected_tag_count, has_expired', [
+  ('devtable', 'simple', 2, False),
+  ('devtable', 'history', 2, True),
+  ('devtable', 'gargantuan', 8, False),
+  ('public', 'publicrepo', 1, False),
+])
+def test_repository_tag_history(namespace, name, expected_tag_count, has_expired, registry_model):
+  # Pre-cache media type loads to ensure consistent query count.
+  Manifest.media_type.get_name(1)
+
+  repository_ref = registry_model.lookup_repository(namespace, name)
 
   with assert_query_count(4 if isinstance(registry_model, SplitModel) else 2):
     history, has_more = registry_model.list_repository_tag_history(repository_ref)
     assert not has_more
-    assert len(history) == 2
+    assert len(history) == expected_tag_count
 
-  # Ensure the latest tag is marked expired, since there is an expired one.
-  with assert_query_count(1):
-    assert registry_model.has_expired_tag(repository_ref, 'latest')
+    for tag in history:
+      # Retrieve the manifest to ensure it doesn't issue extra queries.
+      tag.manifest
+
+  if has_expired:
+    # Ensure the latest tag is marked expired, since there is an expired one.
+    with assert_query_count(1):
+      assert registry_model.has_expired_tag(repository_ref, 'latest')
 
 
 @pytest.mark.parametrize('repo_namespace, repo_name', [

endpoints/api/tag.py

@@ -37,6 +37,7 @@ def _tag_dict(tag):
   # TODO(jschorr): Remove this check once fully on OCI data model.
   if tag.manifest_digest:
     tag_info['manifest_digest'] = tag.manifest_digest
+
     if tag.manifest:
       try:
         tag_info['manifest'] = json.loads(tag.manifest.internal_manifest_bytes.as_unicode())

endpoints/api/test/test_tag.py

@@ -1,6 +1,9 @@
 import pytest
 
+from playhouse.test_utils import assert_query_count
+
 from data.registry_model import registry_model
+from data.database import Manifest
 
 from endpoints.api.test.shared import conduct_api_call
 from endpoints.test.shared import client_with_identity
@@ -75,16 +78,23 @@ def test_move_tag(image_exists, test_tag, expected_status, client, app):
       conduct_api_call(cl, RepositoryTag, 'put', params, request_body, expected_status)
 
 
-@pytest.mark.parametrize('repo_namespace, repo_name', [
-  ('devtable', 'simple'),
-  ('devtable', 'history'),
-  ('devtable', 'complex'),
-  ('buynlarge', 'orgrepo'),
+@pytest.mark.parametrize('repo_namespace, repo_name, query_count', [
+  ('devtable', 'simple', 7),
+  ('devtable', 'history', 7),
+  ('devtable', 'complex', 7),
+  ('devtable', 'gargantuan', 7),
+  ('buynlarge', 'orgrepo', 9), # +2 for permissions checks.
+  ('buynlarge', 'anotherorgrepo', 9), # +2 for permissions checks.
 ])
-def test_list_repo_tags(repo_namespace, repo_name, client, app):
+def test_list_repo_tags(repo_namespace, repo_name, client, query_count, app):
+  # Pre-cache media type loads to ensure consistent query count.
+  Manifest.media_type.get_name(1)
+
   params = {'repository': repo_namespace + '/' + repo_name}
   with client_with_identity('devtable', client) as cl:
-    tags = conduct_api_call(cl, ListRepositoryTags, 'get', params).json['tags']
+    with assert_query_count(query_count):
+      tags = conduct_api_call(cl, ListRepositoryTags, 'get', params).json['tags']
+
     repo_ref = registry_model.lookup_repository(repo_namespace, repo_name)
     history, _ = registry_model.list_repository_tag_history(repo_ref)
     assert len(tags) == len(history)