Merge ~bjornt/maas:baseline-test into maas:machine-list-spike

Proposed by Björn Tillenius
Status: Merged
Approved by: Björn Tillenius
Approved revision: 19c56f161b0fc719b031f4980027435529fafec1
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~bjornt/maas:baseline-test
Merge into: maas:machine-list-spike
Diff against target: 199 lines (+165/-0)
5 files modified
setup.cfg (+1/-0)
src/maasperf/tests/maasspike/__init__.py (+0/-0)
src/maasperf/tests/maasspike/test_listing.py (+123/-0)
src/maasspike/__init__.py (+7/-0)
src/maasspike/baseline.py (+34/-0)
Reviewer Review Type Date Requested Status
MAAS Lander Needs Fixing
Alberto Donato (community) Approve
Review via email: mp+433941@code.launchpad.net

Commit message

Add the performance test for the original machine list implementation.

It's meant to be used as a baseline, to which we can compare the other
implementations.

This commit also adds the basic infrastructure for writing the tests.

To post a comment you must log in.
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b baseline-test lp:~bjornt/maas/+git/maas into -b machine-list-spike lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/1536/consoleText
COMMIT: cddede0fb9129ce413bb01a25d2b8a6ee9c33a48

review: Needs Fixing
Revision history for this message
Alberto Donato (ack) wrote :

nice, +1

one suggestion inline

review: Approve
~bjornt/maas:baseline-test updated
19c56f1... by Björn Tillenius

Compare ids instead of the length.

Revision history for this message
Björn Tillenius (bjornt) :
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b baseline-test lp:~bjornt/maas/+git/maas into -b machine-list-spike lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas-tester/1543/consoleText
COMMIT: 19c56f161b0fc719b031f4980027435529fafec1

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/setup.cfg b/setup.cfg
2index cb3e5db..b61150e 100644
3--- a/setup.cfg
4+++ b/setup.cfg
5@@ -88,6 +88,7 @@ lint_files =
6 src/maascli
7 src/maasperf
8 src/maasserver
9+ src/maasspike
10 src/maastesting
11 src/metadataserver
12 src/provisioningserver
13diff --git a/src/maasperf/tests/maasspike/__init__.py b/src/maasperf/tests/maasspike/__init__.py
14new file mode 100644
15index 0000000..e69de29
16--- /dev/null
17+++ b/src/maasperf/tests/maasspike/__init__.py
18diff --git a/src/maasperf/tests/maasspike/test_listing.py b/src/maasperf/tests/maasspike/test_listing.py
19new file mode 100644
20index 0000000..36c9fbd
21--- /dev/null
22+++ b/src/maasperf/tests/maasspike/test_listing.py
23@@ -0,0 +1,123 @@
24+"""Performance tests for the machine listing spike.
25+
26+All tests that test the performance of the different implementations for the
27+machine listing spike should go in this file.
28+
29+For each implemenation, we should have a test that lists all machines, and
30+a test that list 50 machines.
31+
32+Each test should measure how long it takes to produce the list, and then
33+assert that the listing is the same as the original websocket handler.
34+"""
35+
36+import pytest
37+
38+from maasserver.models import Machine
39+from maasserver.websockets.handlers.machine import MachineHandler
40+from maasspike import baseline
41+
42+
43+class ExpectedMachines:
44+ def __init__(self, expected_list):
45+ self._expected_list = expected_list
46+
47+ def assert_list(self, machine_list, limit=None):
48+ """Assert that the passed in machine list is correct.
49+
50+ Compare the number of machines in each list, as well as making
51+ sure that the first and last machine of each list are equal.
52+
53+ Comparing the full list takes too long if you have a list of
54+ 1000 machines.
55+ """
56+ assert isinstance(machine_list, list)
57+ expected_list = (
58+ self._expected_list[:limit] if limit else self._expected_list
59+ )
60+ assert [machine["id"] for machine in machine_list] == [
61+ machine["id"] for machine in expected_list
62+ ]
63+ assert machine_list[0] == expected_list[0]
64+ assert machine_list[-1] == expected_list[-1]
65+
66+
67+def get_expected_machines(admin):
68+ """Get the list of machines that the normal websocket handler returns."""
69+ machine_count = Machine.objects.all().count()
70+ ws_handler = MachineHandler(admin, {}, None)
71+ params = {
72+ "filter": {},
73+ "page_number": 1,
74+ "page_size": machine_count + 1,
75+ "sort_direction": "ascending",
76+ "sort_key": "hostname",
77+ }
78+ result = ws_handler.list(params)
79+ return ExpectedMachines(result["groups"][0]["items"])
80+
81+
82+@pytest.fixture(scope="session")
83+def _expected():
84+ """Helper fixture to store the expected machine list for the session.
85+
86+ A session fixture doesn't have access to the DB, so we make use of a
87+ function fixture, expected_machines, to get the machine listing
88+ for the first test and store it here.
89+
90+ The fixture shouldn't be used by any test.
91+ """
92+ return {}
93+
94+
95+@pytest.fixture
96+def expected_machines(admin, _expected):
97+ if "machines" not in _expected:
98+ _expected["machines"] = get_expected_machines(admin)
99+ return _expected["machines"]
100+
101+
102+def test_populate_expected_machines(expected_machines):
103+ """A blank test to populate the expected_machines fixture.
104+
105+ This should be the first test to be run, so that other test
106+ don't get any advantage by having the query to the machine
107+ listing cached.
108+ """
109+
110+
111+@pytest.mark.parametrize("limit", [None, 50])
112+class TestListing:
113+ """Collection of tests for spike machine listing implementations.
114+
115+ A class is used to group the tests together to ensure they measure
116+ and assert the same thing.
117+
118+ Each implementation should have a test that does the required setup
119+ and then call self.run_listing_test.
120+
121+ Each test is run once using the full listing, and once using the first
122+ 50 machines.
123+ """
124+
125+ @pytest.fixture(autouse=True)
126+ def set_up(self, perf, admin, expected_machines):
127+ self._expected_machines = expected_machines
128+ self._admin = admin
129+ self._perf = perf
130+ yield
131+ self._expected_machines = None
132+ self._admin = None
133+ self._perf = None
134+
135+ def run_listing_test(self, name, func, limit):
136+ record_name = name
137+ if limit:
138+ record_name += f"_{limit}"
139+ else:
140+ record_name += "_all"
141+ with self._perf.record(record_name):
142+ machines = func(self._admin, limit)
143+ self._expected_machines.assert_list(machines, limit)
144+
145+ def test_baseline(self, limit):
146+ self.run_listing_test("baseline", baseline.list_machines, limit)
147diff --git a/src/maasspike/__init__.py b/src/maasspike/__init__.py
148new file mode 100644
149index 0000000..5476dfd
150--- /dev/null
151+++ b/src/maasspike/__init__.py
152@@ -0,0 +1,7 @@
153+"""This package contains experimental code for the websocket machine listing.
154+
155+We're doing a spike on how to improve the rendering time of the machine
156+listing in the UI. We keep the code in a separate package to make it
157+clear what's part of the spike, and any improvements we make that
158+can be merged back to master.
159+"""
160diff --git a/src/maasspike/baseline.py b/src/maasspike/baseline.py
161new file mode 100644
162index 0000000..f3a6d84
163--- /dev/null
164+++ b/src/maasspike/baseline.py
165@@ -0,0 +1,34 @@
166+from maasserver.websockets.handlers.machine import MachineHandler
167+
168+
169+class NoPaginationMachineHandler(MachineHandler):
170+ """A machine handler that returns only the machines.
171+
172+ It doesn't do any grouping, searching, or other things
173+ (like caching the ids) which might slow it down.
174+
175+ The purpose of this is to use our current implementation as a baseline
176+ for other implementations to beat. We reuse the current websocket
177+ handler as much as possible, so that any improvements to it will
178+ be shown here as well.
179+ """
180+
181+ def list(self, params):
182+ qs = self.get_queryset(for_list=True)
183+ qs = self._sort(qs, "list", params)
184+ limit = params.get("limit")
185+ if limit:
186+ qs = qs[:limit]
187+ objs = list(qs)
188+ # This is needed to calculate the script result summaries.
189+ # It's quite expensive.
190+ self._cache_script_results(objs)
191+ return [self.full_dehydrate(obj, for_list=True) for obj in objs]
192+
193+
194+def list_machines(admin, limit=None):
195+ ws_handler = NoPaginationMachineHandler(admin, {}, None)
196+ params = {}
197+ if limit:
198+ params["limit"] = limit
199+ return ws_handler.list(params)

Subscribers

People subscribed via source and target branches