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)
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
