launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-stormbase-eq-hash into launchpad:master.

Commit message:
Fix handling of removed objects in StormBase

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1916522 in Launchpad itself: "buildd-manager regression: 'NoneType' object has no attribute 'flush'"
  https://bugs.launchpad.net/launchpad/+bug/1916522

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/398498

Objects that have been removed don't have `primary_vars`, and `Store.of` returns None for them.  This caused `StormBase.__eq__` (and, less importantly, `StormBase.__hash__`) to crash if given such an object.  In most cases we don't keep removed objects around, but there is at least one exception: the cache of the current `BuildQueue` associated with the builder managed by a `SlaveScanner`, which is checked for equality with the `BuildQueue` found from the builder's vitals.

To avoid this problem, use `IStore(obj.__class__).flush()` instead, and in the `__eq__` case explicitly return False if this still doesn't cause `primary_vars` to appear.  (`__hash__` will still crash if used on a removed object, but that seems to be OK.)  This is a better match for the details of the behaviour of `SQLBase`.

Fixes a regression introduced by https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/398087.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-stormbase-eq-hash into launchpad:master.

diff --git a/lib/lp/buildmaster/tests/test_manager.py b/lib/lp/buildmaster/tests/test_manager.py
index 99b9e52..6b8c8d5 100644
--- a/lib/lp/buildmaster/tests/test_manager.py
+++ b/lib/lp/buildmaster/tests/test_manager.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the renovated slave scanner aka BuilddManager."""
@@ -680,12 +680,17 @@ class TestSlaveScannerWithLibrarian(TestCaseWithFactory):
     @defer.inlineCallbacks
     def test_end_to_end(self):
         # Test that SlaveScanner.scan() successfully finds, dispatches,
-        # collects and cleans a build.
+        # collects and cleans a build, and then makes a reasonable start on
+        # a second build.
         build = self.factory.makeBinaryPackageBuild()
         build.distro_arch_series.addOrUpdateChroot(
             self.factory.makeLibraryFileAlias(db_only=True))
         bq = build.queueBuild()
         bq.manualScore(9000)
+        build2 = self.factory.makeBinaryPackageBuild(
+            distroarchseries=build.distro_arch_series)
+        bq2 = build2.queueBuild()
+        bq2.manualScore(8900)
 
         builder = self.factory.makeBuilder(
             processors=[bq.processor], manual=False, vm_host='VMHOST')
@@ -765,6 +770,24 @@ class TestSlaveScannerWithLibrarian(TestCaseWithFactory):
         self.assertIs(None, builder.currentjob)
         self.assertEqual(BuilderCleanStatus.CLEAN, builder.clean_status)
 
+        # Now we can go round the loop again with a second build.  (We only
+        # go far enough to ensure that the lost-job check works on the
+        # second iteration.)
+        get_slave.result = OkSlave()
+        yield scanner.scan()
+        self.assertEqual(
+            ['status', 'ensurepresent', 'build'],
+            get_slave.result.method_log)
+        self.assertEqual(bq2, builder.currentjob)
+        self.assertEqual(BuildQueueStatus.RUNNING, bq2.status)
+        self.assertEqual(BuildStatus.BUILDING, build2.status)
+        self.assertEqual(BuilderCleanStatus.DIRTY, builder.clean_status)
+
+        get_slave.result = BuildingSlave(build2.build_cookie)
+        yield scanner.scan()
+        yield scanner.manager.flushLogTails()
+        self.assertEqual("This is a build log: 0", bq2.logtail)
+
 
 class TestPrefetchedBuilderFactory(TestCaseWithFactory):
 
diff --git a/lib/lp/services/database/stormbase.py b/lib/lp/services/database/stormbase.py
index 4d99064..29888ed 100644
--- a/lib/lp/services/database/stormbase.py
+++ b/lib/lp/services/database/stormbase.py
@@ -7,12 +7,10 @@ __all__ = [
     ]
 
 from storm.info import get_obj_info
-from storm.locals import (
-    Store,
-    Storm,
-    )
+from storm.locals import Storm
 from zope.security.proxy import removeSecurityProxy
 
+from lp.services.database.interfaces import IStore
 from lp.services.propertycache import clear_property_cache
 
 
@@ -25,12 +23,16 @@ class StormBase(Storm):
     def __eq__(self, other):
         """Equality operator.
 
-        Objects compare equal if they have the same class and id, and the id
-        is not None.
+        Objects compare equal if they have the same class and primary key,
+        and the primary key is not None.
 
         This rule allows objects retrieved from different stores to compare
-        equal.  Newly-created objects may not yet have an id; in such cases
-        we flush the store so that we can find out their id.
+        equal.  Newly-created objects may not yet have an primary key; in
+        such cases we flush the store so that we can find out their primary
+        key.  Objects that have been removed from the store will have no
+        primary key even after flushing the store, and compare unequal to
+        anything (although keeping objects around after they have been
+        removed is not normally a good idea).
         """
         other = removeSecurityProxy(other)
         if self.__class__ != other.__class__:
@@ -38,9 +40,13 @@ class StormBase(Storm):
         self_obj_info = get_obj_info(self)
         other_obj_info = get_obj_info(other)
         if "primary_vars" not in self_obj_info:
-            Store.of(self).flush()
+            IStore(self.__class__).flush()
+            if "primary_vars" not in self_obj_info:
+                return False
         if "primary_vars" not in other_obj_info:
-            Store.of(other).flush()
+            IStore(other.__class__).flush()
+            if "primary_vars" not in other_obj_info:
+                return False
         self_primary = [var.get() for var in self_obj_info["primary_vars"]]
         other_primary = [var.get() for var in other_obj_info["primary_vars"]]
         return self_primary == other_primary
@@ -60,7 +66,7 @@ class StormBase(Storm):
         """
         obj_info = get_obj_info(self)
         if "primary_vars" not in obj_info:
-            Store.of(self).flush()
+            IStore(self.__class__).flush()
         primary = [var.get() for var in obj_info["primary_vars"]]
         return hash((self.__class__,) + tuple(primary))
 
diff --git a/lib/lp/services/database/tests/test_stormbase.py b/lib/lp/services/database/tests/test_stormbase.py
new file mode 100644
index 0000000..6abfa1f
--- /dev/null
+++ b/lib/lp/services/database/tests/test_stormbase.py
@@ -0,0 +1,64 @@
+# Copyright 2021 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""StormBase tests."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+from storm.locals import Int
+import transaction
+from zope.component import getUtility
+
+from lp.services.database.interfaces import (
+    DEFAULT_FLAVOR,
+    IStore,
+    IStoreSelector,
+    MAIN_STORE,
+    )
+from lp.services.database.stormbase import StormBase
+from lp.testing import TestCase
+from lp.testing.layers import ZopelessDatabaseLayer
+
+
+class StormExample(StormBase):
+
+    __storm_table__ = "StormExample"
+
+    id = Int(primary=True)
+
+    @classmethod
+    def new(cls):
+        example = cls()
+        IStore(cls).add(example)
+        return example
+
+
+class TestStormBase(TestCase):
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        super(TestStormBase, self).setUp()
+        self.store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
+        self.store.execute("CREATE TABLE StormExample (id serial PRIMARY KEY)")
+
+    def test_eq_ne(self):
+        examples = [StormExample.new() for _ in range(3)]
+        transaction.commit()
+        for i in range(len(examples)):
+            for j in range(len(examples)):
+                if i == j:
+                    self.assertEqual(examples[i], examples[j])
+                else:
+                    self.assertNotEqual(examples[i], examples[j])
+
+    def test_ne_removed(self):
+        # A removed object is not equal to a newly-created object, even
+        # though we no longer know the removed object's primary key.
+        example = StormExample.new()
+        self.store.remove(example)
+        self.store.flush()
+        new_example = StormExample.new()
+        self.assertNotEqual(example, new_example)