Merge ~bjornt/maas:bug-1835316-add-event-index into maas:master

Proposed by Björn Tillenius
Status: Merged
Approved by: Björn Tillenius
Approved revision: 86622db0d513f34527b61ffcd434a0bcbe83567b
Merge reported by: MAAS Lander
Merged at revision: not available
Proposed branch: ~bjornt/maas:bug-1835316-add-event-index
Merge into: maas:master
Diff against target: 174 lines (+56/-10)
5 files modified
src/maasserver/migrations/maasserver/0194_machine_listing_event_index.py (+24/-0)
src/maasserver/migrations/maasserver/0195_merge_20190902_1357.py (+16/-0)
src/maasserver/models/event.py (+6/-0)
src/maasserver/models/tests/test_node.py (+9/-9)
src/maasserver/rpc/tests/test_boot.py (+1/-1)
Reviewer Review Type Date Requested Status
Blake Rouse (community) Approve
MAAS Lander Needs Fixing
Alberto Donato Approve
Review via email: mp+371979@code.launchpad.net

Commit message

Add index to the event table to speed up machine listing.

The index speeds up getting the latest event for each node that is
displayed on the machine listing.

The added patch depends on 0187, which is the latest patch on 2.6, since this
patch is to be backported there.

To post a comment you must log in.
Revision history for this message
Alberto Donato (ack) wrote :

LGTM

review: Approve
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b bug-1835316-add-event-index lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6265/console
COMMIT: 637213a7fb9aac5cc07b13c0415bffdabf80bc71

review: Needs Fixing
Revision history for this message
MAAS Lander (maas-lander) wrote :

UNIT TESTS
-b bug-1835316-add-event-index lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/6266/console
COMMIT: c3182226de62752bbb9fde4b281ab4b06fb8c3df

review: Needs Fixing
Revision history for this message
Blake Rouse (blake-rouse) wrote :

I read your email, still a little confused on the merging of migrations. Want to understand that completely first. Overall the change of adding the index looks good.

review: Needs Information
Revision history for this message
Björn Tillenius (bjornt) :
Revision history for this message
Blake Rouse (blake-rouse) wrote :

That means the backport will not be a strait cherry-pick, is it possible to split this over 2 branches or will it not land in master?

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

> That means the backport will not be a strait cherry-pick, is it possible to
> split this over 2 branches or will it not land in master?

No, it's not possible to split it over two commits. Without that merge patch,
Django will complain that there are more than one leaf patch.

On the other hand, backporting of DB patches have never been straight
cherry-picks, since they did re-ordering of db patches that didn't exist in
the backported version. So from that point of view, it's no worse than before.

Revision history for this message
Blake Rouse (blake-rouse) wrote :

Yeah I thought that was the case. Looks good!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/maasserver/migrations/maasserver/0194_machine_listing_event_index.py b/src/maasserver/migrations/maasserver/0194_machine_listing_event_index.py
2new file mode 100644
3index 0000000..b82a8ea
4--- /dev/null
5+++ b/src/maasserver/migrations/maasserver/0194_machine_listing_event_index.py
6@@ -0,0 +1,24 @@
7+# -*- coding: utf-8 -*-
8+# Generated by Django 1.11.11 on 2019-08-28 15:07
9+from __future__ import unicode_literals
10+
11+from django.db import (
12+ migrations,
13+ models,
14+)
15+
16+
17+class Migration(migrations.Migration):
18+
19+ # This depends on the latest (at time of writing) patch in 2.6,
20+ # since this patch is to be backported there.
21+ dependencies = [
22+ ('maasserver', '0192_event_node_no_set_null'),
23+ ]
24+
25+ operations = [
26+ migrations.AddIndex(
27+ model_name='event',
28+ index=models.Index(fields=['node', '-created', '-id'], name='maasserver__node_id_e4a8dd_idx'),
29+ ),
30+ ]
31diff --git a/src/maasserver/migrations/maasserver/0195_merge_20190902_1357.py b/src/maasserver/migrations/maasserver/0195_merge_20190902_1357.py
32new file mode 100644
33index 0000000..97af45a
34--- /dev/null
35+++ b/src/maasserver/migrations/maasserver/0195_merge_20190902_1357.py
36@@ -0,0 +1,16 @@
37+# -*- coding: utf-8 -*-
38+# Generated by Django 1.11.11 on 2019-09-02 13:57
39+from __future__ import unicode_literals
40+
41+from django.db import migrations
42+
43+
44+class Migration(migrations.Migration):
45+
46+ dependencies = [
47+ ('maasserver', '0193_merge_maasserver_0191_1092'),
48+ ('maasserver', '0194_machine_listing_event_index'),
49+ ]
50+
51+ operations = [
52+ ]
53diff --git a/src/maasserver/models/event.py b/src/maasserver/models/event.py
54index 3d0b169..fe58ff8 100644
55--- a/src/maasserver/models/event.py
56+++ b/src/maasserver/models/event.py
57@@ -13,6 +13,7 @@ from django.db.models import (
58 CharField,
59 DO_NOTHING,
60 ForeignKey,
61+ Index,
62 IntegerField,
63 Manager,
64 PROTECT,
65@@ -146,6 +147,11 @@ class Event(CleanSave, TimestampedModel):
66 index_together = (
67 ("node", "id"),
68 )
69+ indexes = [
70+ # Needed to get the latest event for each node on the
71+ # machine listing page.
72+ Index(fields=['node', '-created', '-id']),
73+ ]
74
75 @property
76 def endpoint_name(self):
77diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
78index 191b963..de1b513 100644
79--- a/src/maasserver/models/tests/test_node.py
80+++ b/src/maasserver/models/tests/test_node.py
81@@ -1982,7 +1982,7 @@ class TestNode(MAASServerTestCase):
82 self.patch(Node, "_set_status")
83 with post_commit_hooks:
84 node.abort_disk_erasing(owner)
85- events = Event.objects.filter(node=node)
86+ events = Event.objects.filter(node=node).order_by('id')
87 self.assertEqual(
88 events[0].type.name, EVENT_TYPES.REQUEST_NODE_ABORT_ERASE_DISK)
89 self.assertEqual(
90@@ -2113,7 +2113,7 @@ class TestNode(MAASServerTestCase):
91 self.patch(Node, "_stop").return_value = None
92 with post_commit_hooks:
93 node.abort_deploying(admin)
94- events = Event.objects.filter(node=node)
95+ events = Event.objects.filter(node=node).order_by('id')
96 self.assertEqual(
97 events[0].type.name, EVENT_TYPES.REQUEST_NODE_ABORT_DEPLOYMENT)
98 self.assertEqual(
99@@ -2170,7 +2170,7 @@ class TestNode(MAASServerTestCase):
100 self.patch(Node, "_stop").return_value = None
101 with post_commit_hooks:
102 node.abort_testing(admin)
103- events = Event.objects.filter(node=node)
104+ events = Event.objects.filter(node=node).order_by('id')
105 self.assertEqual(
106 events[0].type.name, EVENT_TYPES.REQUEST_NODE_ABORT_TESTING)
107 self.assertEqual(
108@@ -2339,7 +2339,7 @@ class TestNode(MAASServerTestCase):
109 node.power_state = POWER_STATE.OFF
110 with post_commit_hooks:
111 node.release()
112- events = Event.objects.filter(node=node)
113+ events = Event.objects.filter(node=node).order_by('id')
114 self.expectThat(events[1].type.name, Equals(EVENT_TYPES.RELEASING))
115 self.expectThat(events[2].type.name, Equals(EVENT_TYPES.RELEASED))
116 self.expectThat(node._stop, MockNotCalled())
117@@ -2542,7 +2542,7 @@ class TestNode(MAASServerTestCase):
118 self.patch(node, '_set_status')
119 with post_commit_hooks:
120 node.release(owner)
121- events = Event.objects.filter(node=node)
122+ events = Event.objects.filter(node=node).order_by('id')
123 self.assertEqual(events[0].type.name, EVENT_TYPES.REQUEST_NODE_RELEASE)
124 self.assertEqual(events[1].type.name, EVENT_TYPES.RELEASING)
125
126@@ -3079,7 +3079,7 @@ class TestNode(MAASServerTestCase):
127 node.start_commissioning(admin)
128 post_commit_hooks.reset() # Ignore these for now.
129 node = reload_object(node)
130- events = Event.objects.filter(node=node)
131+ events = Event.objects.filter(node=node).order_by('id')
132 self.assertEqual(
133 events[0].type.name, EVENT_TYPES.REQUEST_NODE_START_COMMISSIONING)
134 self.assertEqual(events[1].type.name, EVENT_TYPES.COMMISSIONING)
135@@ -3155,7 +3155,7 @@ class TestNode(MAASServerTestCase):
136 self.patch(Node, "_stop").return_value = None
137 with post_commit_hooks:
138 node.abort_commissioning(admin)
139- events = Event.objects.filter(node=node)
140+ events = Event.objects.filter(node=node).order_by('id')
141 self.assertEqual(
142 events[0].type.name, EVENT_TYPES.REQUEST_NODE_ABORT_COMMISSIONING)
143 self.assertEqual(
144@@ -3289,7 +3289,7 @@ class TestNode(MAASServerTestCase):
145 self.patch(node, '_start').return_value = None
146 admin = factory.make_admin()
147 node.start_testing(admin, testing_scripts=[script.name])
148- events = Event.objects.filter(node=node)
149+ events = Event.objects.filter(node=node).order_by('id')
150 post_commit_hooks.reset() # Ignore these for now.
151 node = reload_object(node)
152 self.assertEqual(
153@@ -4640,7 +4640,7 @@ class TestNode(MAASServerTestCase):
154 node.start_rescue_mode(admin)
155 post_commit_hooks.reset() # Ignore these for now.
156 node = reload_object(node)
157- events = Event.objects.filter(node=node)
158+ events = Event.objects.filter(node=node).order_by('id')
159 self.assertEqual(
160 events[0].type.name, EVENT_TYPES.REQUEST_NODE_START_RESCUE_MODE)
161 self.assertEqual(events[1].type.name, EVENT_TYPES.ENTERING_RESCUE_MODE)
162diff --git a/src/maasserver/rpc/tests/test_boot.py b/src/maasserver/rpc/tests/test_boot.py
163index a0bfd50..859b752 100644
164--- a/src/maasserver/rpc/tests/test_boot.py
165+++ b/src/maasserver/rpc/tests/test_boot.py
166@@ -769,7 +769,7 @@ class TestGetConfig(MAASServerTestCase):
167 for purpose, description in purposes:
168 node = self.make_node()
169 event_log_pxe_request(node, purpose)
170- events = Event.objects.filter(node=node)
171+ events = Event.objects.filter(node=node).order_by('id')
172 self.assertEqual(description, events[0].description)
173 self.assertEqual(events[1].type.description, EVENT_DETAILS[
174 EVENT_TYPES.PERFORMING_PXE_BOOT].description)

Subscribers

People subscribed via source and target branches