Merge pull request #1774 from coreos-inc/label-delete

Fix deletion of labels and add tests

data/model/repository.py

@@ -57,16 +57,17 @@ def _purge_all_repository_tags(namespace_name, repository_name):
     return
 
   # Find all labels to delete.
-  labels = list(TagManifestLabel
+  manifest_labels_query = (TagManifestLabel
-                .select(TagManifestLabel.label)
+                           .select()
                            .where(TagManifestLabel.repository == repo))
 
+  label_ids = [manifest_label.label_id for manifest_label in manifest_labels_query]
+  if label_ids:
     # Delete all the mapping entries.
     TagManifestLabel.delete().where(TagManifestLabel.repository == repo).execute()
 
     # Delete all the matching labels.
-  if labels:
+    Label.delete().where(Label.id << label_ids).execute()
-    Label.delete().where(Label.id << [label.id for label in labels]).execute()
 
   # Delete all the manifests.
   TagManifest.delete().where(TagManifest.tag << repo_tags).execute()

data/model/tag.py

@@ -152,19 +152,22 @@ def garbage_collect_tags(repo):
       num_deleted_manifests = 0
       if len(manifests_to_delete) > 0:
         # Find the set of IDs for all the labels to delete.
-        labels = list(TagManifestLabel
+        manifest_labels_query = (TagManifestLabel
-                      .select(TagManifestLabel.id)
+                                 .select()
-                      .where(TagManifestLabel.annotated << manifests_to_delete))
+                                 .where(TagManifestLabel.repository == repo,
+                                        TagManifestLabel.annotated << manifests_to_delete))
 
-        if len(labels) > 0:
+        label_ids = [manifest_label.label_id for manifest_label in manifest_labels_query]
-          # Delete the mapping entries.
+        if label_ids:
+          # Delete all the mapping entries.
           (TagManifestLabel
            .delete()
-           .where(TagManifestLabel.annotated << manifests_to_delete)
+           .where(TagManifestLabel.repository == repo,
+                  TagManifestLabel.annotated << manifests_to_delete)
            .execute())
 
-          # Delete the labels themselves.
+          # Delete all the matching labels.
-          Label.delete().where(Label.id << [label.id for label in labels]).execute()
+          Label.delete().where(Label.id << label_ids).execute()
 
         # Delete the tag manifests themselves.
         num_deleted_manifests = (TagManifest

test/test_api_usage.py

@@ -1768,6 +1768,8 @@ class TestDeleteRepository(ApiTestCase):
                                  date=datetime.datetime.now() - datetime.timedelta(days=5), count=6)
 
     # Create some labels.
+    pre_delete_label_count = database.Label.select().count()
+
     tag_manifest = model.tag.load_tag_manifest(ADMIN_ACCESS_USER, 'complex', 'prod')
     model.label.create_manifest_label(tag_manifest, 'foo', 'bar', 'manifest')
     model.label.create_manifest_label(tag_manifest, 'foo', 'baz', 'manifest')
@@ -1785,6 +1787,10 @@ class TestDeleteRepository(ApiTestCase):
                      params=dict(repository=self.COMPLEX_REPO),
                      expected_code=404)
 
+    # Verify the labels are gone.
+    post_delete_label_count = database.Label.select().count()
+    self.assertEquals(post_delete_label_count, pre_delete_label_count)
+
 
 class TestGetRepository(ApiTestCase):
   PUBLIC_REPO = PUBLIC_USER + '/publicrepo'

test/test_gc.py

@@ -6,7 +6,7 @@ from playhouse.test_utils import assert_query_count
 from app import app, storage
 from initdb import setup_database_for_testing, finished_database_for_testing
 from data import model, database
-from data.database import Image, ImageStorage, DerivedStorageForImage
+from data.database import Image, ImageStorage, DerivedStorageForImage, Label, TagManifestLabel
 from endpoints.v2.manifest import _generate_and_store_manifest
 
 
@@ -16,6 +16,26 @@ PUBLIC_USER = 'public'
 REPO = 'somerepo'
 
 
+class assert_no_new_dangling_labels(object):
+  """ Specialized assertion for ensuring that GC cleans up all labels.
+  """
+  def __init__(self):
+    self.existing_count = 0
+
+  def _get_dangling_count(self):
+    label_ids = set([current.id for current in Label.select()])
+    referenced_by_manifest = set([mlabel.label_id for mlabel in TagManifestLabel.select()])
+    return len(label_ids - referenced_by_manifest)
+
+  def __enter__(self):
+    self.existing_count = self._get_dangling_count()
+    return self
+
+  def __exit__(self, exc_type, exc_val, exc_tb):
+    updated_count = self._get_dangling_count()
+    assert updated_count == self.existing_count
+
+
 class assert_no_new_dangling_storages(object):
   """ Specialized assertion for ensuring that GC cleans up all dangling storages.
   """
@@ -114,7 +134,12 @@ class TestGarbageCollection(unittest.TestCase):
                                                 parent=parent)
 
       # Set the tag for the image.
-      model.tag.create_or_update_tag(namespace, name, tag_name, image_ids[-1])
+      tag_manifest, _ = model.tag.store_tag_manifest(namespace, name, tag_name, image_ids[-1],
+                                                     'sha:someshahere', '{}')
+
+      # Add some labels to the tag.
+      model.label.create_manifest_label(tag_manifest, 'foo', 'bar', 'manifest')
+      model.label.create_manifest_label(tag_manifest, 'meh', 'grah', 'manifest')
 
     return repo
 
@@ -201,6 +226,7 @@ class TestGarbageCollection(unittest.TestCase):
   def test_one_tag(self):
     """ Create a repository with a single tag, then remove that tag and verify that the repository
         is now empty. """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'])
         self.deleteTag(repository, 'latest')
@@ -209,6 +235,7 @@ class TestGarbageCollection(unittest.TestCase):
 
   def test_two_tags_unshared_images(self):
     """ Repository has two tags with no shared images between them. """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['f1', 'f2'])
         self.deleteTag(repository, 'latest')
@@ -220,6 +247,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Repository has two tags with shared images. Deleting the tag should only remove the
         unshared images.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1'])
         self.deleteTag(repository, 'latest')
@@ -231,6 +259,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Two repositories with different images. Removing the tag from one leaves the other's
         images intact.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository1 = self.createRepository(latest=['i1', 'i2', 'i3'], name='repo1')
         repository2 = self.createRepository(latest=['j1', 'j2', 'j3'], name='repo2')
@@ -245,6 +274,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Two repositories with shared images. Removing the tag from one leaves the other's
         images intact.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository1 = self.createRepository(latest=['i1', 'i2', 'i3'], name='repo1')
         repository2 = self.createRepository(latest=['i1', 'i2', 'j1'], name='repo2')
@@ -259,6 +289,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Two repositories under different namespaces should result in the images being deleted
         but not completely removed from the database.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository1 = self.createRepository(namespace=ADMIN_ACCESS_USER, latest=['i1', 'i2', 'i3'])
         repository2 = self.createRepository(namespace=PUBLIC_USER, latest=['i1', 'i2', 'i3'])
@@ -272,6 +303,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Repository has multiple tags with shared images. Selectively deleting the tags, and
         verifying at each step.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1', 'f2'],
                                            third=['t1', 't2', 't3'], fourth=['i1', 'f1'])
@@ -310,6 +342,7 @@ class TestGarbageCollection(unittest.TestCase):
 
 
   def test_empty_gc(self):
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1', 'f2'],
                                            third=['t1', 't2', 't3'], fourth=['i1', 'f1'])
@@ -321,6 +354,7 @@ class TestGarbageCollection(unittest.TestCase):
   def test_time_machine_no_gc(self):
     """ Repository has two tags with shared images. Deleting the tag should not remove any images
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1'])
         self._set_tag_expiration_policy(repository.namespace_user.username, 60*60*24)
@@ -334,6 +368,7 @@ class TestGarbageCollection(unittest.TestCase):
     """ Repository has two tags with shared images. Deleting the second tag should cause the images
         for the first deleted tag to gc.
     """
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1'])
 
@@ -351,6 +386,7 @@ class TestGarbageCollection(unittest.TestCase):
 
 
   def test_manifest_gc(self):
+    with assert_no_new_dangling_labels():
       with assert_no_new_dangling_storages():
         repository = self.createRepository(latest=['i1', 'i2', 'i3'], other=['i1', 'f1'])
         _generate_and_store_manifest(ADMIN_ACCESS_USER, REPO, 'latest')