Merge lp:~allenap/maas/dns-serials--bug-1571645--garbage into lp:~maas-committers/maas/trunk
- dns-serials--bug-1571645--garbage
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Gavin Panella | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 5050 | ||||
Proposed branch: | lp:~allenap/maas/dns-serials--bug-1571645--garbage | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Prerequisite: | lp:~allenap/maas/dns-serials--bug-1571645 | ||||
Diff against target: |
366 lines (+266/-5) 7 files modified
src/maasserver/api/tests/test_interfaces.py (+3/-3) src/maasserver/dns/publication.py (+72/-0) src/maasserver/dns/tests/test_publication.py (+126/-0) src/maasserver/eventloop.py (+10/-0) src/maasserver/models/dnspublication.py (+7/-2) src/maasserver/models/tests/test_dnspublication.py (+47/-0) src/maasserver/tests/test_plugin.py (+1/-0) |
||||
To merge this branch: | bzr merge lp:~allenap/maas/dns-serials--bug-1571645--garbage | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gavin Panella (community) | Abstain | ||
Blake Rouse (community) | Approve | ||
Mike Pontillo (community) | Abstain | ||
Review via email:
|
Commit message
Periodically remove old DNS publication records.
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Mike Pontillo (mpontillo) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Mike Pontillo (mpontillo) wrote : | # |
Ignore my previous comment. I totally missed that it was already a random time interval. Well done. =)
That said, I'm really on the fence about this branch.
I think many "enterprise" customers would want full audit logging, which is admittedly a long way away. The previous branch does a good job of keeping a record of every change made, and why.
I personally believe that 7 days is enough, should we need the information in this table to diagnose an issue with MAAS.
On the other hand, it feels wrong to add complexity to MAAS in order to delete auditing data, which many customers might want to keep around.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gavin Panella (allenap) wrote : | # |
You make good points. It may be fine to do without cleaning garbage; each row is not more than ~300B so it's going to take a long time before this really becomes a problem.
If we decide not to use this, how would you feel if I landed all the code in here (with tests, naturally) but without hooking it up as a service, so that it does not run by default?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Blake Rouse (blake-rouse) wrote : | # |
I like this and I think we should run this service. I would not consider this an audit log, and we have no way of exposing this information to an administrator. We do log in the maaslog every time the DNS is updated. The DNS service code easily load the reason and include that in the log message.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~allenap/maas/dns-serials--bug-1571645--garbage into lp:maas failed. Below is the output from the failed tests.
Get:1 http://
Hit:2 http://
Get:3 http://
Hit:4 http://
Fetched 185 kB in 0s (393 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubun
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
libpq-dev is already the newest version (9.5.2-1).
make is already the newest version (4.1-6).
postgresql is already the ...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~allenap/maas/dns-serials--bug-1571645--garbage into lp:maas failed. Below is the output from the failed tests.
Hit:1 http://
Get:2 http://
Get:3 http://
Hit:4 http://
Fetched 185 kB in 0s (453 kB/s)
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubun
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
libpq-dev is already the newest version (9.5.2-1).
make is already the newest version (4.1-6).
postgresql is already the ...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gavin Panella (allenap) wrote : | # |
Clear review slot.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~allenap/maas/dns-serials--bug-1571645--garbage into lp:maas failed. Below is the output from the failed tests.
Hit:1 http://
Hit:2 http://
Hit:3 http://
Hit:4 http://
Reading package lists...
sudo DEBIAN_
--no-
Reading package lists...
Building dependency tree...
Reading state information...
apache2 is already the newest version (2.4.18-2ubuntu3).
archdetect-deb is already the newest version (1.117ubuntu2).
authbind is already the newest version (2.1.1+nmu1).
bash is already the newest version (4.3-14ubuntu1).
build-essential is already the newest version (12.1ubuntu2).
bzr is already the newest version (2.7.0-2ubuntu1).
curl is already the newest version (7.47.0-1ubuntu2).
debhelper is already the newest version (9.20160115ubun
distro-info is already the newest version (0.14build1).
freeipmi-tools is already the newest version (1.4.11-1ubuntu1).
git is already the newest version (1:2.7.4-0ubuntu1).
isc-dhcp-common is already the newest version (4.3.3-5ubuntu12).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+
python-django is already...
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Gavin Panella (allenap) wrote : | # |
Fix unrelated, spuriously failing test.
Preview Diff
1 | === modified file 'src/maasserver/api/tests/test_interfaces.py' |
2 | --- src/maasserver/api/tests/test_interfaces.py 2016-05-24 22:05:45 +0000 |
3 | +++ src/maasserver/api/tests/test_interfaces.py 2016-05-26 13:31:29 +0000 |
4 | @@ -26,7 +26,7 @@ |
5 | ContainsDict, |
6 | Equals, |
7 | MatchesDict, |
8 | - MatchesListwise, |
9 | + MatchesSetwise, |
10 | ) |
11 | |
12 | |
13 | @@ -703,7 +703,7 @@ |
14 | nic.name |
15 | for nic in children |
16 | ), parsed_interface["children"]) |
17 | - self.assertThat(parsed_interface["links"], MatchesListwise(links)) |
18 | + self.assertThat(parsed_interface["links"], MatchesSetwise(*links)) |
19 | json_discovered = parsed_interface["discovered"][0] |
20 | self.assertEqual(dhcp_subnet.id, json_discovered["subnet"]["id"]) |
21 | self.assertEqual(discovered_ip, json_discovered["ip_address"]) |
22 | @@ -1393,7 +1393,7 @@ |
23 | "params": Equals(interface.params), |
24 | "effective_mtu": Equals(interface.get_effective_mtu()), |
25 | })) |
26 | - self.assertThat(parsed_interface["links"], MatchesListwise(links)) |
27 | + self.assertThat(parsed_interface["links"], MatchesSetwise(*links)) |
28 | json_discovered = parsed_interface["discovered"][0] |
29 | self.assertEqual(dhcp_subnet.id, json_discovered["subnet"]["id"]) |
30 | self.assertEqual(discovered_ip, json_discovered["ip_address"]) |
31 | |
32 | === added file 'src/maasserver/dns/publication.py' |
33 | --- src/maasserver/dns/publication.py 1970-01-01 00:00:00 +0000 |
34 | +++ src/maasserver/dns/publication.py 2016-05-26 13:31:29 +0000 |
35 | @@ -0,0 +1,72 @@ |
36 | +# Copyright 2016 Canonical Ltd. This software is licensed under the |
37 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
38 | + |
39 | +"""Services related to DNS publication.""" |
40 | + |
41 | +__all__ = [ |
42 | + "DNSPublicationGarbageService", |
43 | +] |
44 | + |
45 | +from datetime import ( |
46 | + datetime, |
47 | + timedelta, |
48 | +) |
49 | +import random |
50 | + |
51 | +from maasserver.models.dnspublication import DNSPublication |
52 | +from maasserver.utils.orm import transactional |
53 | +from maasserver.utils.threads import deferToDatabase |
54 | +from provisioningserver.utils.twisted import callOut |
55 | +from pytz import UTC |
56 | +from twisted.application.service import Service |
57 | +from twisted.internet import reactor |
58 | +from twisted.internet.task import LoopingCall |
59 | +from twisted.python import log |
60 | + |
61 | + |
62 | +class DNSPublicationGarbageService(Service): |
63 | + """Periodically delete DNS publications more than 7 days old.""" |
64 | + |
65 | + clock = None |
66 | + |
67 | + def startService(self): |
68 | + super().startService() |
69 | + self._loop = LoopingCall(self._tryCollectGarbage) |
70 | + self._loop.clock = reactor if self.clock is None else self.clock |
71 | + self._loopDone = self._loop.start(self._getInterval(), now=False) |
72 | + self._loopDone.addErrback(log.err, "Garbage loop failed.") |
73 | + |
74 | + def stopService(self): |
75 | + if self._loop.running: |
76 | + self._loop.stop() |
77 | + return self._loopDone.addBoth( |
78 | + callOut, super().stopService) |
79 | + |
80 | + def _getInterval(self): |
81 | + """Return a random interval between 3 and 6 hours. |
82 | + |
83 | + :return: The number of seconds. |
84 | + """ |
85 | + return random.randrange( |
86 | + timedelta(hours=3).total_seconds(), |
87 | + timedelta(hours=6).total_seconds()) |
88 | + |
89 | + def _updateInterval(self): |
90 | + """Update the loop's interval. |
91 | + |
92 | + Only call this when the loop is running, otherwise it will crash. |
93 | + Also, it only really makes sense to call it when the loop's function |
94 | + is executing otherwise it will have no effect until the next loop. |
95 | + """ |
96 | + self._loop.interval = self._getInterval() |
97 | + |
98 | + def _tryCollectGarbage(self): |
99 | + cutoff = datetime.utcnow().replace(tzinfo=UTC) - timedelta(days=7) |
100 | + d = deferToDatabase(self._collectGarbage, cutoff) # In a transaction. |
101 | + d.addBoth(callOut, self._updateInterval) # Always adjust the schedule. |
102 | + d.addErrback(log.err, "Failure when removing old DNS publications.") |
103 | + return d |
104 | + |
105 | + @transactional |
106 | + def _collectGarbage(self, cutoff): |
107 | + return DNSPublication.objects.collect_garbage(cutoff) |
108 | |
109 | === added file 'src/maasserver/dns/tests/test_publication.py' |
110 | --- src/maasserver/dns/tests/test_publication.py 1970-01-01 00:00:00 +0000 |
111 | +++ src/maasserver/dns/tests/test_publication.py 2016-05-26 13:31:29 +0000 |
112 | @@ -0,0 +1,126 @@ |
113 | +# Copyright 2016 Canonical Ltd. This software is licensed under the |
114 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
115 | + |
116 | +"""Tests for `maasserver.dns.publication`.""" |
117 | + |
118 | +__all__ = [] |
119 | + |
120 | +from datetime import ( |
121 | + datetime, |
122 | + timedelta, |
123 | +) |
124 | + |
125 | +from crochet import wait_for |
126 | +from maasserver.dns import publication |
127 | +from maasserver.models.dnspublication import DNSPublication |
128 | +from maasserver.testing.testcase import MAASTransactionServerTestCase |
129 | +from maastesting.factory import factory |
130 | +from maastesting.matchers import ( |
131 | + DocTestMatches, |
132 | + GreaterThanOrEqual, |
133 | + MockCalledOnceWith, |
134 | + MockNotCalled, |
135 | +) |
136 | +from maastesting.runtest import MAASCrochetRunTest |
137 | +from maastesting.testcase import MAASTestCase |
138 | +from maastesting.twisted import TwistedLoggerFixture |
139 | +from provisioningserver.utils.twisted import pause |
140 | +from pytz import UTC |
141 | +from testtools.matchers import ( |
142 | + LessThan, |
143 | + MatchesAll, |
144 | +) |
145 | +from twisted.internet.defer import ( |
146 | + fail, |
147 | + inlineCallbacks, |
148 | +) |
149 | +from twisted.internet.task import Clock |
150 | + |
151 | + |
152 | +IsExpectedInterval = MatchesAll( |
153 | + GreaterThanOrEqual(3 * 60 * 60), LessThan(6 * 60 * 60), |
154 | + first_only=True) |
155 | + |
156 | + |
157 | +def patch_utcnow(test): |
158 | + utcnow = test.patch(publication, "datetime").utcnow |
159 | + ref = utcnow.return_value = datetime.utcnow() |
160 | + return ref |
161 | + |
162 | + |
163 | +class TestDNSPublicationGarbageService(MAASTestCase): |
164 | + """Tests for `DNSPublicationGarbageService`.""" |
165 | + |
166 | + run_tests_with = MAASCrochetRunTest |
167 | + |
168 | + def test_starting_and_stopping(self): |
169 | + deferToDatabase = self.patch(publication, "deferToDatabase") |
170 | + |
171 | + utcnow = patch_utcnow(self) |
172 | + cutoff = utcnow.replace(tzinfo=UTC) - timedelta(days=7) |
173 | + |
174 | + dnsgc = publication.DNSPublicationGarbageService() |
175 | + dnsgc.clock = clock = Clock() |
176 | + |
177 | + dnsgc.startService() |
178 | + self.assertTrue(dnsgc.running) |
179 | + self.assertTrue(dnsgc._loop.running) |
180 | + self.assertThat(deferToDatabase, MockNotCalled()) |
181 | + self.assertThat(dnsgc._loop.interval, IsExpectedInterval) |
182 | + |
183 | + clock.advance(dnsgc._loop.interval) |
184 | + self.assertThat( |
185 | + deferToDatabase, MockCalledOnceWith( |
186 | + dnsgc._collectGarbage, cutoff)) |
187 | + self.assertThat(dnsgc._loop.interval, IsExpectedInterval) |
188 | + |
189 | + dnsgc.stopService() |
190 | + self.assertFalse(dnsgc.running) |
191 | + self.assertFalse(dnsgc._loop.running) |
192 | + |
193 | + def test_failures_are_logged(self): |
194 | + deferToDatabase = self.patch(publication, "deferToDatabase") |
195 | + deferToDatabase.return_value = fail(factory.make_exception()) |
196 | + |
197 | + dnsgc = publication.DNSPublicationGarbageService() |
198 | + dnsgc.clock = clock = Clock() |
199 | + |
200 | + with TwistedLoggerFixture() as logger: |
201 | + dnsgc.startService() |
202 | + clock.advance(dnsgc._loop.interval) |
203 | + dnsgc.stopService() |
204 | + |
205 | + self.assertThat(logger.output, DocTestMatches( |
206 | + """\ |
207 | + Failure when removing old DNS publications. |
208 | + Traceback (most recent call last):... |
209 | + Failure: maastesting.factory.TestException#... |
210 | + """)) |
211 | + |
212 | + self.assertFalse(dnsgc.running) |
213 | + |
214 | + |
215 | +class TestDNSPublicationGarbageServiceWithDatabase( |
216 | + MAASTransactionServerTestCase): |
217 | + """Tests for `DNSPublicationGarbageService` with the database.""" |
218 | + |
219 | + run_tests_with = MAASCrochetRunTest |
220 | + |
221 | + @wait_for(30.0) |
222 | + @inlineCallbacks |
223 | + def test_garbage_is_collected(self): |
224 | + dnsgc = publication.DNSPublicationGarbageService() |
225 | + |
226 | + utcnow = patch_utcnow(self) |
227 | + cutoff = utcnow.replace(tzinfo=UTC) - timedelta(days=7) |
228 | + |
229 | + self.patch(dnsgc, "_getInterval").side_effect = [0, 999] |
230 | + self.patch(DNSPublication.objects, "collect_garbage") |
231 | + |
232 | + yield dnsgc.startService() |
233 | + yield pause(0.0) # Let the reactor tick. |
234 | + yield dnsgc.stopService() |
235 | + |
236 | + self.assertThat( |
237 | + DNSPublication.objects.collect_garbage, |
238 | + MockCalledOnceWith(cutoff)) |
239 | |
240 | === modified file 'src/maasserver/eventloop.py' |
241 | --- src/maasserver/eventloop.py 2016-04-01 18:16:57 +0000 |
242 | +++ src/maasserver/eventloop.py 2016-05-26 13:31:29 +0000 |
243 | @@ -94,6 +94,11 @@ |
244 | return nonces_cleanup.NonceCleanupService() |
245 | |
246 | |
247 | +def make_DNSPublicationGarbageService(): |
248 | + from maasserver.dns import publication |
249 | + return publication.DNSPublicationGarbageService() |
250 | + |
251 | + |
252 | def make_StatusMonitorService(): |
253 | from maasserver import status_monitor |
254 | return status_monitor.StatusMonitorService() |
255 | @@ -198,6 +203,11 @@ |
256 | "factory": make_NonceCleanupService, |
257 | "requires": [], |
258 | }, |
259 | + "dns-publication-cleanup": { |
260 | + "only_on_master": True, |
261 | + "factory": make_DNSPublicationGarbageService, |
262 | + "requires": [], |
263 | + }, |
264 | "status-monitor": { |
265 | "only_on_master": True, |
266 | "factory": make_StatusMonitorService, |
267 | |
268 | === modified file 'src/maasserver/models/dnspublication.py' |
269 | --- src/maasserver/models/dnspublication.py 2016-04-29 10:17:54 +0000 |
270 | +++ src/maasserver/models/dnspublication.py 2016-05-26 13:31:29 +0000 |
271 | @@ -7,6 +7,8 @@ |
272 | "DNSPublication", |
273 | ] |
274 | |
275 | +from datetime import datetime |
276 | + |
277 | from django.core.validators import ( |
278 | MaxValueValidator, |
279 | MinValueValidator, |
280 | @@ -54,14 +56,17 @@ |
281 | # use migrations to provide an initial publication. |
282 | raise self.model.DoesNotExist() from None |
283 | |
284 | - def collect_garbage(self): |
285 | + def collect_garbage(self, cutoff: datetime=None): |
286 | """Delete all but the most recently inserted `DNSPublication`.""" |
287 | try: |
288 | publication = self.get_most_recent() |
289 | except self.model.DoesNotExist: |
290 | pass # Nothing to do. |
291 | else: |
292 | - self.filter(id__lt=publication.id).delete() |
293 | + candidates = self.filter(id__lt=publication.id) |
294 | + if cutoff is not None: |
295 | + candidates = candidates.filter(created__lt=cutoff) |
296 | + candidates.delete() |
297 | |
298 | |
299 | class DNSPublication(Model): |
300 | |
301 | === modified file 'src/maasserver/models/tests/test_dnspublication.py' |
302 | --- src/maasserver/models/tests/test_dnspublication.py 2016-04-28 17:37:11 +0000 |
303 | +++ src/maasserver/models/tests/test_dnspublication.py 2016-05-26 13:31:29 +0000 |
304 | @@ -118,3 +118,50 @@ |
305 | self.assertThat(DNSPublication.objects.all(), HasLength(0)) |
306 | DNSPublication.objects.collect_garbage() |
307 | self.assertThat(DNSPublication.objects.all(), HasLength(0)) |
308 | + |
309 | + def test_collect_garbage_leaves_records_older_than_specified(self): |
310 | + publications = { |
311 | + timedelta(days=1): DNSPublication(source="1 day ago"), |
312 | + timedelta(minutes=1): DNSPublication(source="1 minute ago"), |
313 | + timedelta(seconds=0): DNSPublication(source="now"), |
314 | + } |
315 | + |
316 | + with connection.cursor() as cursor: |
317 | + cursor.execute("SELECT now()") |
318 | + [now] = cursor.fetchone() |
319 | + |
320 | + # Work from oldest to youngest so that the youngest gets the highest |
321 | + # primary key; the primary key is used to determine the most recent. |
322 | + for delta in sorted(publications, reverse=True): |
323 | + publication = publications[delta] |
324 | + publication.save() |
325 | + # Use SQL to set `created`; Django's field validation prevents it. |
326 | + with connection.cursor() as cursor: |
327 | + cursor.execute( |
328 | + "UPDATE maasserver_dnspublication SET created = %s" |
329 | + " WHERE id = %s", [now - delta, publication.id]) |
330 | + |
331 | + def get_ages(): |
332 | + pubs = DNSPublication.objects.all() |
333 | + return {now - pub.created for pub in pubs} |
334 | + |
335 | + deltas = set(publications) |
336 | + self.assertThat(get_ages(), Equals(deltas)) |
337 | + |
338 | + one_second = timedelta(seconds=1) |
339 | + # Work from oldest to youngest again, collecting garbage each time. |
340 | + while len(deltas) > 1: |
341 | + delta = max(deltas) |
342 | + # Publications of exactly the specified age are not deleted. |
343 | + DNSPublication.objects.collect_garbage(now - delta) |
344 | + self.assertThat(get_ages(), Equals(deltas)) |
345 | + # Publications of just a second over are deleted. |
346 | + DNSPublication.objects.collect_garbage(now - delta + one_second) |
347 | + self.assertThat(get_ages(), Equals(deltas - {delta})) |
348 | + # We're done with this one. |
349 | + deltas.discard(delta) |
350 | + |
351 | + # The most recent publication will never be deleted. |
352 | + DNSPublication.objects.collect_garbage() |
353 | + self.assertThat(get_ages(), Equals(deltas)) |
354 | + self.assertThat(deltas, HasLength(1)) |
355 | |
356 | === modified file 'src/maasserver/tests/test_plugin.py' |
357 | --- src/maasserver/tests/test_plugin.py 2016-04-12 17:36:35 +0000 |
358 | +++ src/maasserver/tests/test_plugin.py 2016-05-26 13:31:29 +0000 |
359 | @@ -93,6 +93,7 @@ |
360 | self.assertIsInstance(service, MultiService) |
361 | expected_services = [ |
362 | "database-tasks", |
363 | + "dns-publication-cleanup", |
364 | "import-resources", |
365 | "import-resources-progress", |
366 | "nonce-cleanup", |
So that HA regions don't step on each other and all try to do this at the same time,, should we introduce a random component in the timing?