launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #29460
[Merge] ~cjwatson/launchpad:immutable-pg-json into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:immutable-pg-json into launchpad:master.
Commit message:
Use an immutable property for builder resources/constraints
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/434104
Adding builder resources and constraints using `storm.databases.postgres.JSON` caused some performance issues, e.g. timeouts on `/builders`, because Storm has to do a lot of work on every flush to check whether these variables have changed. We've encountered similar problems in the past and worked around them in scripts (see commit 74ffded6f6), but in this case we're encountering problems in the web UI where that's much less practical.
Instead, introduce a new `ImmutablePgJSON` property, which is essentially a variant of `storm.databases.postgres.JSON` but without the mutable-value tracking. Users of models using this property must ensure that they assign entirely new values when they want to change them rather than mutating existing values in place. To make this less error-prone, `ImmutablePgJSON` turns lists into tuples and wraps dicts in `types.MappingProxyType` to make them effectively immutable.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:immutable-pg-json into launchpad:master.
diff --git a/lib/lp/buildmaster/browser/builder.py b/lib/lp/buildmaster/browser/builder.py
index 784eed4..1c24da5 100644
--- a/lib/lp/buildmaster/browser/builder.py
+++ b/lib/lp/buildmaster/browser/builder.py
@@ -195,8 +195,8 @@ class BuilderSetView(CleanInfoMixin, LaunchpadView):
def getBuilderSortKey(builder):
return (
not builder.virtualized,
- tuple(builder.restricted_resources or ()),
- tuple(builder.open_resources or ()),
+ builder.restricted_resources or (),
+ builder.open_resources or (),
tuple(p.name for p in builder.processors),
builder.name,
)
diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index 00f90b1..174411c 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -391,16 +391,8 @@ def extract_vitals_from_db(builder, build_queue=_BQ_UNSPECIFIED):
builder.virtualized,
builder.vm_host,
builder.vm_reset_protocol,
- (
- None
- if builder.open_resources is None
- else tuple(builder.open_resources)
- ),
- (
- None
- if builder.restricted_resources is None
- else tuple(builder.restricted_resources)
- ),
+ builder.open_resources,
+ builder.restricted_resources,
builder.builderok,
builder.manual,
build_queue,
diff --git a/lib/lp/buildmaster/interfaces/builder.py b/lib/lp/buildmaster/interfaces/builder.py
index 5a5d5f2..9225325 100644
--- a/lib/lp/buildmaster/interfaces/builder.py
+++ b/lib/lp/buildmaster/interfaces/builder.py
@@ -34,7 +34,16 @@ from lazr.restful.declarations import (
from lazr.restful.fields import Reference, ReferenceChoice
from lazr.restful.interface import copy_field
from zope.interface import Attribute, Interface
-from zope.schema import Bool, Choice, Datetime, Int, List, Text, TextLine
+from zope.schema import (
+ Bool,
+ Choice,
+ Datetime,
+ Int,
+ List,
+ Text,
+ TextLine,
+ Tuple,
+)
from lp import _
from lp.app.validators.name import name_validator
@@ -217,7 +226,7 @@ class IBuilderView(IHasBuildRecords, IHasOwner):
)
open_resources = exported(
- List(
+ Tuple(
title=_("Open resources"),
description=_(
"Resource tags offered by this builder, that can be required "
@@ -229,7 +238,7 @@ class IBuilderView(IHasBuildRecords, IHasOwner):
)
restricted_resources = exported(
- List(
+ Tuple(
title=_("Restricted resources"),
description=_(
"Resource tags offered by this builder, indicating that the "
diff --git a/lib/lp/buildmaster/interfaces/buildfarmjob.py b/lib/lp/buildmaster/interfaces/buildfarmjob.py
index f05596f..0f516e5 100644
--- a/lib/lp/buildmaster/interfaces/buildfarmjob.py
+++ b/lib/lp/buildmaster/interfaces/buildfarmjob.py
@@ -29,7 +29,7 @@ from lazr.restful.declarations import (
)
from lazr.restful.fields import Reference
from zope.interface import Attribute, Interface
-from zope.schema import Bool, Choice, Datetime, Int, List, TextLine, Timedelta
+from zope.schema import Bool, Choice, Datetime, Int, TextLine, Timedelta, Tuple
from lp import _
from lp.buildmaster.enums import BuildFarmJobType, BuildStatus
@@ -109,7 +109,7 @@ class IBuildFarmJobView(Interface):
),
)
- builder_constraints = List(
+ builder_constraints = Tuple(
title=_("Builder constraints"),
required=False,
readonly=True,
diff --git a/lib/lp/buildmaster/interfaces/buildqueue.py b/lib/lp/buildmaster/interfaces/buildqueue.py
index be1e4cd..824791f 100644
--- a/lib/lp/buildmaster/interfaces/buildqueue.py
+++ b/lib/lp/buildmaster/interfaces/buildqueue.py
@@ -15,10 +15,10 @@ from zope.schema import (
Choice,
Datetime,
Int,
- List,
Text,
TextLine,
Timedelta,
+ Tuple,
)
from lp import _
@@ -66,7 +66,7 @@ class IBuildQueue(Interface):
"The virtualization setting required by this build farm job."
),
)
- builder_constraints = List(
+ builder_constraints = Tuple(
required=False,
value_type=TextLine(),
description=_(
diff --git a/lib/lp/buildmaster/model/builder.py b/lib/lp/buildmaster/model/builder.py
index d52551f..ea1a4ff 100644
--- a/lib/lp/buildmaster/model/builder.py
+++ b/lib/lp/buildmaster/model/builder.py
@@ -8,7 +8,6 @@ __all__ = [
]
import pytz
-from storm.databases.postgres import JSON
from storm.expr import Coalesce, Count, Sum
from storm.properties import Bool, DateTime, Int, Unicode
from storm.references import Reference
@@ -34,6 +33,7 @@ from lp.services.database.decoratedresultset import DecoratedResultSet
from lp.services.database.enumcol import DBEnum
from lp.services.database.interfaces import IStandbyStore, IStore
from lp.services.database.stormbase import StormBase
+from lp.services.database.stormexpr import ImmutablePgJSON
from lp.services.propertycache import cachedproperty, get_property_cache
# XXX Michael Nelson 2010-01-13 bug=491330
@@ -61,8 +61,10 @@ class Builder(StormBase):
virtualized = Bool(name="virtualized", default=True, allow_none=False)
manual = Bool(name="manual", default=False)
vm_host = Unicode(name="vm_host")
- open_resources = JSON(name="open_resources", allow_none=True)
- restricted_resources = JSON(name="restricted_resources", allow_none=True)
+ open_resources = ImmutablePgJSON(name="open_resources", allow_none=True)
+ restricted_resources = ImmutablePgJSON(
+ name="restricted_resources", allow_none=True
+ )
active = Bool(name="active", allow_none=False, default=True)
failure_count = Int(name="failure_count", default=0, allow_none=False)
version = Unicode(name="version")
diff --git a/lib/lp/buildmaster/model/buildqueue.py b/lib/lp/buildmaster/model/buildqueue.py
index c69cc65..1bbcd03 100644
--- a/lib/lp/buildmaster/model/buildqueue.py
+++ b/lib/lp/buildmaster/model/buildqueue.py
@@ -13,7 +13,6 @@ from itertools import groupby
from operator import attrgetter
import pytz
-from storm.databases.postgres import JSON
from storm.expr import SQL, Cast, Coalesce, Desc, Exists, Or
from storm.properties import Bool, DateTime, Int, TimeDelta, Unicode
from storm.references import Reference
@@ -35,7 +34,7 @@ from lp.services.database.constants import DEFAULT, UTC_NOW
from lp.services.database.enumcol import DBEnum
from lp.services.database.interfaces import IStore
from lp.services.database.stormbase import StormBase
-from lp.services.database.stormexpr import JSONContains
+from lp.services.database.stormexpr import ImmutablePgJSON, JSONContains
from lp.services.features import getFeatureFlag
from lp.services.propertycache import cachedproperty, get_property_cache
@@ -101,7 +100,9 @@ class BuildQueue(StormBase):
processor_id = Int(name="processor")
processor = Reference(processor_id, "Processor.id")
virtualized = Bool(name="virtualized")
- builder_constraints = JSON(name="builder_constraints", allow_none=True)
+ builder_constraints = ImmutablePgJSON(
+ name="builder_constraints", allow_none=True
+ )
@property
def specific_source(self):
@@ -355,8 +356,7 @@ class BuildQueueSet:
JSONContains(
Cast(
json.dumps(
- tuple(open_resources or ())
- + tuple(restricted_resources or ())
+ (open_resources or ()) + (restricted_resources or ())
),
"jsonb",
),
diff --git a/lib/lp/buildmaster/tests/mock_workers.py b/lib/lp/buildmaster/tests/mock_workers.py
index 01c9732..55ac82d 100644
--- a/lib/lp/buildmaster/tests/mock_workers.py
+++ b/lib/lp/buildmaster/tests/mock_workers.py
@@ -22,7 +22,6 @@ import shlex
import sys
import xmlrpc.client
from collections import OrderedDict
-from copy import copy
from textwrap import dedent
try:
@@ -81,8 +80,8 @@ class MockBuilder:
self.virtualized = virtualized
self.vm_host = vm_host
self.vm_reset_protocol = vm_reset_protocol
- self.open_resources = copy(open_resources)
- self.restricted_resources = copy(restricted_resources)
+ self.open_resources = open_resources
+ self.restricted_resources = restricted_resources
self.failnotes = None
self.version = version
self.clean_status = clean_status
diff --git a/lib/lp/buildmaster/tests/test_builder.py b/lib/lp/buildmaster/tests/test_builder.py
index e5af5cb..39da0c8 100644
--- a/lib/lp/buildmaster/tests/test_builder.py
+++ b/lib/lp/buildmaster/tests/test_builder.py
@@ -240,7 +240,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
processor=das.processor,
virtualized=True,
limit=5,
- open_resources=["large"],
+ open_resources=("large",),
),
)
self.assertContentEqual(
@@ -249,7 +249,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
processor=das.processor,
virtualized=True,
limit=5,
- open_resources=["large", "gpu"],
+ open_resources=("large", "gpu"),
),
)
self.assertEqual(
@@ -258,7 +258,7 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
processor=das.processor,
virtualized=True,
limit=5,
- restricted_resources=["gpu"],
+ restricted_resources=("gpu",),
),
)
self.assertEqual(
@@ -267,8 +267,8 @@ class TestFindBuildCandidatesGeneralCases(TestFindBuildCandidatesBase):
processor=das.processor,
virtualized=True,
limit=5,
- open_resources=["large"],
- restricted_resources=["gpu"],
+ open_resources=("large",),
+ restricted_resources=("gpu",),
),
)
diff --git a/lib/lp/code/browser/tests/test_gitrepository.py b/lib/lp/code/browser/tests/test_gitrepository.py
index 1c5120c..51bf64b 100644
--- a/lib/lp/code/browser/tests/test_gitrepository.py
+++ b/lib/lp/code/browser/tests/test_gitrepository.py
@@ -1119,7 +1119,7 @@ class TestGitRepositoryEditBuilderConstraintsView(BrowserTestCase):
browser.getControl("Change Git Repository").click()
login_person(owner)
- self.assertEqual(["gpu", "large"], repository.builder_constraints)
+ self.assertEqual(("gpu", "large"), repository.builder_constraints)
class TestGitRepositoryEditView(TestCaseWithFactory):
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index a955c0c..2bbbb6b 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -43,7 +43,16 @@ from lazr.restful.fields import CollectionField, Reference
from lazr.restful.interface import copy_field
from zope.component import getUtility
from zope.interface import Attribute, Interface
-from zope.schema import Bool, Choice, Datetime, Int, List, Text, TextLine
+from zope.schema import (
+ Bool,
+ Choice,
+ Datetime,
+ Int,
+ List,
+ Text,
+ TextLine,
+ Tuple,
+)
from lp import _
from lp.app.enums import InformationType
@@ -1247,7 +1256,7 @@ class IGitRepositoryAdminAttributes(Interface):
"""
builder_constraints = exported(
- List(
+ Tuple(
title=_("Builder constraints"),
required=False,
readonly=False,
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index bd09864..b1a8c28 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -24,7 +24,7 @@ from breezy import urlutils
from lazr.enum import DBItem
from lazr.lifecycle.event import ObjectModifiedEvent
from lazr.lifecycle.snapshot import Snapshot
-from storm.databases.postgres import JSON, Returning
+from storm.databases.postgres import Returning
from storm.expr import SQL, And, Coalesce, Desc, Insert, Join, Not, Or, Select
from storm.info import ClassAlias, get_cls_info
from storm.locals import Bool, DateTime, Int, Reference, Unicode
@@ -143,6 +143,7 @@ from lp.services.database.stormexpr import (
ArrayAgg,
ArrayIntersects,
BulkUpdate,
+ ImmutablePgJSON,
Values,
)
from lp.services.features import getFeatureFlag
@@ -323,7 +324,9 @@ class GitRepository(
name="date_last_scanned", tzinfo=pytz.UTC, allow_none=True
)
- builder_constraints = JSON(name="builder_constraints", allow_none=True)
+ builder_constraints = ImmutablePgJSON(
+ name="builder_constraints", allow_none=True
+ )
def __init__(
self,
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index 84ca9e1..114b606 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -168,7 +168,7 @@ class TestCIBuild(TestCaseWithFactory):
)
bq = build.queueBuild()
self.assertProvides(bq, IBuildQueue)
- self.assertEqual(["gpu"], bq.builder_constraints)
+ self.assertEqual(("gpu",), bq.builder_constraints)
def test_is_private(self):
# A CIBuild is private iff its repository is.
@@ -777,7 +777,7 @@ class TestCIBuildSet(TestCaseWithFactory):
build = getUtility(ICIBuildSet).requestBuild(
repository, commit_sha1, das, [[("test", 0)]]
)
- self.assertEqual(["gpu"], build.builder_constraints)
+ self.assertEqual(("gpu",), build.builder_constraints)
def test_requestBuildsForRefs_triggers_builds(self):
ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
diff --git a/lib/lp/code/model/tests/test_gitnamespace.py b/lib/lp/code/model/tests/test_gitnamespace.py
index c9c92b0..97bd2a8 100644
--- a/lib/lp/code/model/tests/test_gitnamespace.py
+++ b/lib/lp/code/model/tests/test_gitnamespace.py
@@ -158,7 +158,7 @@ class NamespaceMixin:
repository_name,
builder_constraints=["gpu"],
)
- self.assertEqual(["gpu"], repository.builder_constraints)
+ self.assertEqual(("gpu",), repository.builder_constraints)
def test_getRepositories_no_repositories(self):
# getRepositories on an IGitNamespace returns a result set of
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 2239439..8fd9c9f 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -6621,7 +6621,7 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
)
self.assertEqual(209, response.status)
with person_logged_in(ANONYMOUS):
- self.assertEqual(["gpu"], repository_db.builder_constraints)
+ self.assertEqual(("gpu",), repository_db.builder_constraints)
def test_builder_constraints_owner(self):
# The owner of a repository cannot change its builder constraints
diff --git a/lib/lp/services/database/stormexpr.py b/lib/lp/services/database/stormexpr.py
index d9b25d6..f9e808f 100644
--- a/lib/lp/services/database/stormexpr.py
+++ b/lib/lp/services/database/stormexpr.py
@@ -14,6 +14,7 @@ __all__ = [
"fti_search",
"Greatest",
"get_where_for_reference",
+ "ImmutablePgJSON",
"IsDistinctFrom",
"IsFalse",
"IsTrue",
@@ -30,6 +31,9 @@ __all__ = [
"WithMaterialized",
]
+import json
+from types import MappingProxyType
+
from storm import Undef
from storm.exceptions import ClassInfoError
from storm.expr import (
@@ -49,6 +53,8 @@ from storm.expr import (
compile,
)
from storm.info import get_cls_info, get_obj_info
+from storm.properties import SimpleProperty
+from storm.variables import Variable
class BulkUpdate(Expr):
@@ -324,6 +330,85 @@ def compile_with_materialized(compile, with_expr, state):
return "".join(tokens)
+class ImmutablePgJSONVariable(Variable):
+ """An immutable version of `storm.databases.postgres.JSONVariable`.
+
+ Storm can't easily detect when variables that contain references to
+ mutable content have been changed, and has to work around this by
+ checking the variable's state when the store is flushing current
+ objects. Unfortunately, this means that if there's a large number of
+ live objects containing mutable-value properties, then flushes become
+ very slow as Storm has to dump many objects to check whether they've
+ changed.
+
+ This class works around this performance problem by disabling the
+ mutable-value tracking, at the expense of users no longer being able to
+ mutate the value in place. Any mutations must be implemented by
+ assigning to the property, rather than by mutating its value.
+ """
+
+ __slots__ = ()
+
+ def get_state(self):
+ return self._lazy_value, self._dumps(self._value)
+
+ def set_state(self, state):
+ self._lazy_value = state[0]
+ self._value = self._loads(state[1])
+
+ def _make_immutable(self, value):
+ """Return an immutable version of `value`.
+
+ This only supports those types that `json.loads` can return as part
+ of a decoded object.
+ """
+ if isinstance(value, list):
+ return tuple(self._make_immutable(item) for item in value)
+ elif isinstance(value, dict):
+ return MappingProxyType(
+ {k: self._make_immutable(v) for k, v in value.items()}
+ )
+ else:
+ return value
+
+ def parse_set(self, value, from_db):
+ if from_db:
+ value = self._loads(value)
+ return self._make_immutable(value)
+
+ def parse_get(self, value, to_db):
+ if to_db:
+ return self._dumps(value)
+ else:
+ return value
+
+ def _loads(self, value):
+ # psycopg2 >= 2.5 automatically decodes JSON columns for us.
+ return value
+
+ def _dumps(self, value):
+ # See https://github.com/python/cpython/issues/79039.
+ def default(value):
+ if isinstance(value, MappingProxyType):
+ return value.copy()
+ else:
+ raise TypeError(
+ "Object of type %s is not JSON serializable"
+ % value.__class__.__name__
+ )
+
+ return json.dumps(value, ensure_ascii=False, default=default)
+
+
+class ImmutablePgJSON(SimpleProperty):
+ """An immutable version of `storm.databases.postgres.JSON`.
+
+ See `ImmutablePgJSONVariable`.
+ """
+
+ variable_class = ImmutablePgJSONVariable
+
+
def get_where_for_reference(reference, other):
"""Generate a column comparison expression for a reference property.
diff --git a/lib/lp/services/database/tests/test_stormexpr.py b/lib/lp/services/database/tests/test_stormexpr.py
index 61eb4a3..c8a803d 100644
--- a/lib/lp/services/database/tests/test_stormexpr.py
+++ b/lib/lp/services/database/tests/test_stormexpr.py
@@ -3,9 +3,11 @@
"""Test custom Storm expressions."""
-from types import SimpleNamespace
+from types import MappingProxyType, SimpleNamespace
-from storm.expr import Select
+from storm.exceptions import NoneError
+from storm.expr import Column, Select
+from storm.info import get_obj_info
from zope.component import getUtility
from lp.services.database.interfaces import (
@@ -14,7 +16,11 @@ from lp.services.database.interfaces import (
IStoreSelector,
)
from lp.services.database.sqlbase import convert_storm_clause_to_string
-from lp.services.database.stormexpr import WithMaterialized
+from lp.services.database.stormexpr import (
+ ImmutablePgJSON,
+ ImmutablePgJSONVariable,
+ WithMaterialized,
+)
from lp.testing import TestCase
from lp.testing.layers import BaseLayer, ZopelessDatabaseLayer
@@ -64,3 +70,67 @@ class TestWithMaterializedRealDatabase(TestCase):
"test AS MATERIALIZED (SELECT 1)",
],
)
+
+
+class TestImmutablePgJSON(TestCase):
+
+ layer = BaseLayer
+
+ def setUpProperty(self, *args, **kwargs):
+ class Class:
+ __storm_table__ = "mytable"
+ prop1 = ImmutablePgJSON("column1", *args, primary=True, **kwargs)
+
+ class Subclass(Class):
+ pass
+
+ self.Class = Class
+ self.Subclass = Subclass
+ self.obj = Subclass()
+ self.obj_info = get_obj_info(self.obj)
+ self.column1 = self.Subclass.prop1
+ self.variable1 = self.obj_info.variables[self.column1]
+
+ def test_basic(self):
+ self.setUpProperty(default_factory=dict, allow_none=False)
+ self.assertIsInstance(self.column1, Column)
+ self.assertEqual("column1", self.column1.name)
+ self.assertEqual(self.Subclass, self.column1.table)
+ self.assertIsInstance(self.variable1, ImmutablePgJSONVariable)
+
+ def test_immutable_default(self):
+ self.setUpProperty(default_factory=dict, allow_none=False)
+ self.assertIsInstance(self.obj.prop1, MappingProxyType)
+ self.assertEqual({}, self.obj.prop1)
+ self.assertEqual("{}", self.variable1.get(to_db=True))
+ self.assertRaises(NoneError, setattr, self.obj, "prop1", None)
+ self.assertRaises(
+ AttributeError, getattr, self.obj.prop1, "__setitem__"
+ )
+
+ def test_immutable_dict(self):
+ self.setUpProperty()
+ self.variable1.set({"a": {"b": []}}, from_db=True)
+ self.assertIsInstance(self.obj.prop1, MappingProxyType)
+ self.assertIsInstance(self.obj.prop1["a"], MappingProxyType)
+ self.assertIsInstance(self.obj.prop1["a"]["b"], tuple)
+ self.assertEqual({"a": {"b": ()}}, self.obj.prop1)
+ self.assertEqual('{"a": {"b": []}}', self.variable1.get(to_db=True))
+ self.assertRaises(
+ AttributeError, getattr, self.obj.prop1, "__setitem__"
+ )
+ self.obj.prop1 = {"a": 1}
+ self.assertIsInstance(self.obj.prop1, MappingProxyType)
+ self.assertEqual({"a": 1}, self.obj.prop1)
+ self.assertEqual('{"a": 1}', self.variable1.get(to_db=True))
+
+ def test_immutable_list(self):
+ self.setUpProperty()
+ self.variable1.set([], from_db=True)
+ self.assertIsInstance(self.obj.prop1, tuple)
+ self.assertEqual((), self.obj.prop1)
+ self.assertEqual("[]", self.variable1.get(to_db=True))
+ self.obj.prop1 = ["a"]
+ self.assertIsInstance(self.obj.prop1, tuple)
+ self.assertEqual(("a",), self.obj.prop1)
+ self.assertEqual('["a"]', self.variable1.get(to_db=True))
