Merge lp:~julian-edwards/launchpad/die-buildqueue-die--bug-492632 into lp:launchpad

Proposed by Julian Edwards
Status: Merged
Merged at revision: not available
Proposed branch: lp:~julian-edwards/launchpad/die-buildqueue-die--bug-492632
Merge into: lp:launchpad
Diff against target: 80 lines (+45/-0)
2 files modified
lib/lp/soyuz/model/buildqueue.py (+9/-0)
lib/lp/soyuz/tests/test_build.py (+36/-0)
To merge this branch: bzr merge lp:~julian-edwards/launchpad/die-buildqueue-die--bug-492632
Reviewer Review Type Date Requested Status
Björn Tillenius (community) release-critical Approve
Muharem Hrnjadovic (community) Approve
Review via email: mp+15927@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

= Summary =
Make sure buildqueue rows are deleted properly

== Proposed fix ==
Last cycle the buildqueue record was refactored a bit and now it also has an
associated Job and BuildPackageJob.

This fix makes sure that the other two are deleted when
buildqueue.destroySelf() is called.

This is a tentative fix for https://bugs.edge.launchpad.net/soyuz/+bug/492632
since we don't really have a better idea right now.

== Pre-implementation notes ==
Does 4 days of investigative hacking count?

== Implementation details ==
I added a destroySelf() method to the content class which calls the SQLObject
method and deletes the other table rows at the same time.

== Tests ==
bin/test -cvvt testBuildqueueRemoval

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/soyuz/tests/test_build.py
  lib/lp/soyuz/model/buildqueue.py

Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

Looks good. Even if it does not solve the original bug, this code is required for the correct life cycle management of BuildQueue related records going forward.

review: Approve
Revision history for this message
Björn Tillenius (bjornt) wrote :

On Thu, Dec 10, 2009 at 10:50:36AM -0000, Julian Edwards wrote:
> === modified file 'lib/lp/soyuz/tests/test_build.py'
> --- lib/lp/soyuz/tests/test_build.py 2009-07-17 00:26:05 +0000
> +++ lib/lp/soyuz/tests/test_build.py 2009-12-10 10:50:36 +0000

> @@ -43,6 +47,69 @@
>
> return depwait_build
>
> + def assertNoBuildQueue(self, build):
> + """Test there's no buildqueue, buildpackagejob or job for `build`"""
> + store = Store.of(build)
> + result = store.find(
> + BuildPackageJob,
> + BuildPackageJob.build == build.id)
> + bpj_count = result.count()
> + self.assertEqual(
> + bpj_count, 0,
> + "Expected no buildpackagejob rows, got %s" % bpj_count)
> +
> + bpj = result.one()
> +
> + result = store.find(
> + Job,
> + bpj.job == Job.id)
> + job_count = result.count()
> + self.assertEqual(
> + job_count, 0,
> + "Expected no job rows, got %s" % job_count)
> +
> + job = result.one()
> +
> + result = store.find(
> + BuildQueue,
> + BuildQueue.job == job.id)
> + buildqueue_count = result.count()
> + self.assertEqual(
> + buildqueue_count, 0,
> + "Expected no buildqueue rows, got %s" % buildqueue_count)
> +
> + def testBuildqueueRemoval(self):
> + """Test removing buildqueue items.
> +
> + Removing a Buildqueue row should also remove its associated
> + BuildPackageJob and Job rows.
> + """
> + # Create a build in depwait.
> + depwait_build = self._setupSimpleDepwaitContext()
> +
> + # Grab the relevant db records for later comparison.
> + store = Store.of(depwait_build)
> + build_package_job = store.find(
> + BuildPackageJob, depwait_build.id == BuildPackageJob.build).one()
> + job = store.find(Job, Job.id == build_package_job.id).one()
> + build_queue = store.find(
> + BuildQueue, BuildQueue.job == job.id).one()
> +
> + depwait_build.buildqueue_record.destroySelf()
> +
> + # Test that the records above no longer exist in the db.
> + self.assertEqual(
> + store.find(
> + BuildPackageJob,
> + depwait_build.id == BuildPackageJob.build).count(),
> + 0)
> + self.assertEqual(
> + store.find(Job, Job.id == build_package_job.id).count(),
> + 0)
> + self.assertEqual(
> + store.find(BuildQueue, BuildQueue.job == job.id).count(),
> + 0)

Can you be sure that the id attributes are the same, after the objects
have been removed from the db? I'd be a bit wary of using objects that
have been removed from the db. It could be that the attributes get set
to None, for example, no?

--
Björn Tillenius | https://launchpad.net/~bjornt

Revision history for this message
Björn Tillenius (bjornt) :
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/soyuz/model/buildqueue.py'
--- lib/lp/soyuz/model/buildqueue.py 2009-12-01 08:43:43 +0000
+++ lib/lp/soyuz/model/buildqueue.py 2009-12-10 10:57:17 +0000
@@ -68,6 +68,15 @@
68 """See `IBuildQueue`."""68 """See `IBuildQueue`."""
69 return self.job.date_started69 return self.job.date_started
7070
71 def destroySelf(self):
72 """Remove this record and associated job/specific_job."""
73 store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
74 job = self.job
75 specific_job = self.specific_job
76 SQLBase.destroySelf(self)
77 store.remove(specific_job)
78 job.destroySelf()
79
71 def manualScore(self, value):80 def manualScore(self, value):
72 """See `IBuildQueue`."""81 """See `IBuildQueue`."""
73 self.lastscore = value82 self.lastscore = value
7483
=== modified file 'lib/lp/soyuz/tests/test_build.py'
--- lib/lp/soyuz/tests/test_build.py 2009-07-17 00:26:05 +0000
+++ lib/lp/soyuz/tests/test_build.py 2009-12-10 10:57:17 +0000
@@ -5,13 +5,17 @@
55
6import unittest6import unittest
77
8from storm.store import Store
8from zope.component import getUtility9from zope.component import getUtility
910
10from canonical.testing import LaunchpadZopelessLayer11from canonical.testing import LaunchpadZopelessLayer
12from lp.services.job.model.job import Job
11from lp.soyuz.interfaces.builder import IBuilderSet13from lp.soyuz.interfaces.builder import IBuilderSet
12from lp.soyuz.interfaces.component import IComponentSet14from lp.soyuz.interfaces.component import IComponentSet
13from lp.soyuz.interfaces.build import BuildStatus, IBuildSet15from lp.soyuz.interfaces.build import BuildStatus, IBuildSet
14from lp.soyuz.interfaces.publishing import PackagePublishingStatus16from lp.soyuz.interfaces.publishing import PackagePublishingStatus
17from lp.soyuz.model.buildqueue import BuildQueue
18from lp.soyuz.model.buildpackagejob import BuildPackageJob
15from lp.soyuz.model.processor import ProcessorFamilySet19from lp.soyuz.model.processor import ProcessorFamilySet
16from lp.soyuz.tests.test_publishing import SoyuzTestPublisher20from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
17from lp.testing import TestCaseWithFactory21from lp.testing import TestCaseWithFactory
@@ -43,6 +47,38 @@
4347
44 return depwait_build48 return depwait_build
4549
50 def testBuildqueueRemoval(self):
51 """Test removing buildqueue items.
52
53 Removing a Buildqueue row should also remove its associated
54 BuildPackageJob and Job rows.
55 """
56 # Create a build in depwait.
57 depwait_build = self._setupSimpleDepwaitContext()
58
59 # Grab the relevant db records for later comparison.
60 store = Store.of(depwait_build)
61 build_package_job = store.find(
62 BuildPackageJob, depwait_build.id == BuildPackageJob.build).one()
63 job = store.find(Job, Job.id == build_package_job.id).one()
64 build_queue = store.find(
65 BuildQueue, BuildQueue.job == job.id).one()
66
67 depwait_build.buildqueue_record.destroySelf()
68
69 # Test that the records above no longer exist in the db.
70 self.assertEqual(
71 store.find(
72 BuildPackageJob,
73 depwait_build.id == BuildPackageJob.build).count(),
74 0)
75 self.assertEqual(
76 store.find(Job, Job.id == build_package_job.id).count(),
77 0)
78 self.assertEqual(
79 store.find(BuildQueue, BuildQueue.job == job.id).count(),
80 0)
81
46 def testUpdateDependenciesWorks(self):82 def testUpdateDependenciesWorks(self):
47 # Calling `IBuild.updateDependencies` makes the build83 # Calling `IBuild.updateDependencies` makes the build
48 # record ready for dispatch.84 # record ready for dispatch.