Merge lp:~lamont/maas/bug-1553176-1.9 into lp:maas/1.9
- bug-1553176-1.9
- Merge into 1.9
Status: | Merged |
---|---|
Approved by: | Mike Pontillo |
Approved revision: | no longer in the source branch. |
Merged at revision: | 4598 |
Proposed branch: | lp:~lamont/maas/bug-1553176-1.9 |
Merge into: | lp:maas/1.9 |
Diff against target: |
210 lines (+32/-116) 2 files modified
src/provisioningserver/utils/fs.py (+16/-62) src/provisioningserver/utils/tests/test_fs.py (+16/-54) |
To merge this branch: | bzr merge lp:~lamont/maas/bug-1553176-1.9 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mike Pontillo (community) | Approve | ||
Review via email: mp+316384@code.launchpad.net |
Commit message
Backport parts of r4750 from trunk: BIND compares zone file mtime to last load time and skips the load when the file is older, so our approach to slowly incrementing mtime breaks.
Description of the change
Stop trying to be smarter than BIND about zonefile modification times, since it uses last-load-time and file mtime to determine whether or not it's already loaded.
LaMont Jones (lamont) wrote : | # |
To follow up on the discussion in IRC, bind9 introduced a change (that landed in trusty) wherein it compares the mtime of the zone file to the system time when the zone was last loaded, and declares the zone to be current if it isn't newer. By setting the mtime into the past and then slowly creeping forward 1 second per update, unless there is a flurry of activity that never stops (and we maintain roughly 1 update per second after that flurry), the DNS will generally be wrong. As such, it makes sense to land this even if bind9 hasn't arrived in -updates yet.
Mike Pontillo (mpontillo) wrote : | # |
Sounds good. You can go ahead and land it.
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1553176-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Fetched 2,982 kB in 1s (1,638 kB/s)
Reading package lists...
sudo DEBIAN_
--no-...
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1553176-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Fetched 2,987 kB in 1s (1,662 kB/s)
Reading package lists...
sudo DEBIAN_
--no-...
MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1553176-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Fetched 2,995 kB in 1s (1,555 kB/s)
Reading package lists...
sudo DEBIAN_
--no-...
Preview Diff
1 | === modified file 'src/provisioningserver/utils/fs.py' |
2 | --- src/provisioningserver/utils/fs.py 2015-10-14 16:27:21 +0000 |
3 | +++ src/provisioningserver/utils/fs.py 2017-02-08 18:01:00 +0000 |
4 | @@ -52,10 +52,7 @@ |
5 | import sys |
6 | import tempfile |
7 | import threading |
8 | -from time import ( |
9 | - sleep, |
10 | - time, |
11 | -) |
12 | +from time import sleep |
13 | |
14 | from provisioningserver.utils import sudo |
15 | from provisioningserver.utils.shell import ExternalProcessError |
16 | @@ -207,69 +204,26 @@ |
17 | raise |
18 | |
19 | |
20 | -def pick_new_mtime(old_mtime=None, starting_age=1000): |
21 | - """Choose a new modification time for a file that needs it updated. |
22 | - |
23 | - This function is used to manage the modification time of files |
24 | - for which we need to see an increment in the modification time |
25 | - each time the file is modified. This is the case for DNS zone |
26 | - files which only get properly reloaded if BIND sees that the |
27 | - modification time is > to the time it has in its database. |
28 | - |
29 | - Modification time can have a resolution as low as one second in |
30 | - some relevant environments (we have observed this with ext3). |
31 | - To produce mtime changes regardless, we set a file's modification |
32 | - time in the past when it is first written, and |
33 | - increment it by 1 second on each subsequent write. |
34 | - |
35 | - However we also want to be careful not to set the modification time |
36 | - in the future, mostly because BIND does not deal with that very |
37 | - well. |
38 | - |
39 | - :param old_mtime: File's previous modification time, as a number |
40 | - with a unity of one second, or None if it did not previously |
41 | - exist. |
42 | - :param starting_age: If the file did not exist previously, set its |
43 | - modification time this many seconds in the past. |
44 | - """ |
45 | - now = time() |
46 | - if old_mtime is None: |
47 | - # File is new. Set modification time in the past to have room for |
48 | - # sub-second modifications. |
49 | - return now - starting_age |
50 | - elif old_mtime + 1 <= now: |
51 | - # There is room to increment the file's mtime by one second |
52 | - # without ending up in the future. |
53 | - return old_mtime + 1 |
54 | - else: |
55 | - # We can't increase the file's modification time. Give up and |
56 | - # return the previous modification time. |
57 | - return old_mtime |
58 | - |
59 | - |
60 | def incremental_write(content, filename, mode=0o600): |
61 | - """Write the given `content` into the file `filename` and |
62 | - increment the modification time by 1 sec. |
63 | + """Write the given `content` into the file `filename`. In the past, this |
64 | + would potentially change modification time to arbitrary values. |
65 | |
66 | :param mode: Access permissions for the file. |
67 | """ |
68 | - old_mtime = get_mtime(filename) |
69 | + # We used to change modification time on the files, in an attempt to out |
70 | + # smart BIND into loading zones on file systems where time was not granular |
71 | + # enough. BIND got smarter about how it loads zones and remembers when it |
72 | + # last loaded the zone: either the then-current time, or the mtime of the |
73 | + # file, whichever is later. When BIND then gets a reload request, it |
74 | + # compares the current time to the loadtime for the domain, and skips it if |
75 | + # the file is not new. If we set mtime in the past, then zones don't load. |
76 | + # If we set it in the future, then we WILL sometimes hit the race condition |
77 | + # where BIND looks at the time after we atomic_write, but before we manage |
78 | + # to set the time into the future. N.B.: /etc on filesystems with 1-second |
79 | + # granularity are no longer supported by MAAS. The good news is that since |
80 | + # 2.6, linux has supported nanosecond-granular time. As of bind9 |
81 | + # 1:9.10.3.dfsg.P2-5, BIND even uses it. |
82 | atomic_write(content, filename, mode=mode) |
83 | - new_mtime = pick_new_mtime(old_mtime) |
84 | - os.utime(filename, (new_mtime, new_mtime)) |
85 | - |
86 | - |
87 | -def get_mtime(filename): |
88 | - """Return a file's modification time, or None if it does not exist.""" |
89 | - try: |
90 | - return os.stat(filename).st_mtime |
91 | - except OSError as e: |
92 | - if e.errno == errno.ENOENT: |
93 | - # File does not exist. Be helpful, return None. |
94 | - return None |
95 | - else: |
96 | - # Other failure. The caller will want to know. |
97 | - raise |
98 | |
99 | |
100 | def sudo_write_file(filename, contents, encoding='utf-8', mode=0o644): |
101 | |
102 | === modified file 'src/provisioningserver/utils/tests/test_fs.py' |
103 | --- src/provisioningserver/utils/tests/test_fs.py 2015-10-14 18:00:50 +0000 |
104 | +++ src/provisioningserver/utils/tests/test_fs.py 2017-02-08 18:01:00 +0000 |
105 | @@ -17,7 +17,6 @@ |
106 | from base64 import urlsafe_b64encode |
107 | import os |
108 | import os.path |
109 | -from random import randint |
110 | import re |
111 | from shutil import rmtree |
112 | import stat |
113 | @@ -31,7 +30,6 @@ |
114 | |
115 | from maastesting import root |
116 | from maastesting.factory import factory |
117 | -from maastesting.fakemethod import FakeMethod |
118 | from maastesting.matchers import ( |
119 | MockCalledOnceWith, |
120 | MockCallsMatch, |
121 | @@ -52,9 +50,7 @@ |
122 | ensure_dir, |
123 | FileLock, |
124 | get_maas_provision_command, |
125 | - get_mtime, |
126 | incremental_write, |
127 | - pick_new_mtime, |
128 | read_text_file, |
129 | RunLock, |
130 | sudo_delete_file, |
131 | @@ -264,8 +260,22 @@ |
132 | old_mtime = os.stat(filename).st_mtime - 10 |
133 | os.utime(filename, (old_mtime, old_mtime)) |
134 | incremental_write(content, filename) |
135 | - self.assertAlmostEqual( |
136 | - os.stat(filename).st_mtime, old_mtime + 1, delta=0.01) |
137 | + # should be much closer to new_time than to old_mtime. |
138 | + new_time = time.time() |
139 | + self.assertAlmostEqual( |
140 | + os.stat(filename).st_mtime, new_time, delta=2.0) |
141 | + |
142 | + def test_incremental_write_does_not_set_future_time(self): |
143 | + content = factory.make_bytes() |
144 | + filename = self.make_file(contents=factory.make_string()) |
145 | + # Pretend that this file is older than it is. So that |
146 | + # incrementing its mtime won't put it in the future. |
147 | + old_mtime = os.stat(filename).st_mtime + 10 |
148 | + os.utime(filename, (old_mtime, old_mtime)) |
149 | + incremental_write(content, filename) |
150 | + new_time = time.time() |
151 | + self.assertAlmostEqual( |
152 | + os.stat(filename).st_mtime, new_time, delta=2.0) |
153 | |
154 | def test_incremental_write_sets_permissions(self): |
155 | atomic_file = self.make_file() |
156 | @@ -274,54 +284,6 @@ |
157 | self.assertEqual(mode, stat.S_IMODE(os.stat(atomic_file).st_mode)) |
158 | |
159 | |
160 | -class TestGetMTime(MAASTestCase): |
161 | - """Test `get_mtime`.""" |
162 | - |
163 | - def test_get_mtime_returns_None_for_nonexistent_file(self): |
164 | - nonexistent_file = os.path.join( |
165 | - self.make_dir(), factory.make_name('nonexistent-file')) |
166 | - self.assertIsNone(get_mtime(nonexistent_file)) |
167 | - |
168 | - def test_get_mtime_returns_mtime(self): |
169 | - existing_file = self.make_file() |
170 | - mtime = os.stat(existing_file).st_mtime - randint(0, 100) |
171 | - os.utime(existing_file, (mtime, mtime)) |
172 | - # Some small rounding/representation errors can happen here. |
173 | - # That's just the way of floating-point numbers. According to |
174 | - # Gavin there's a conversion to fixed-point along the way, which |
175 | - # would raise representability issues. |
176 | - self.assertAlmostEqual(mtime, get_mtime(existing_file), delta=0.00001) |
177 | - |
178 | - def test_get_mtime_passes_on_other_error(self): |
179 | - forbidden_file = self.make_file() |
180 | - self.patch(os, 'stat', FakeMethod(failure=OSError("Forbidden file"))) |
181 | - self.assertRaises(OSError, get_mtime, forbidden_file) |
182 | - |
183 | - |
184 | -class TestPickNewMTime(MAASTestCase): |
185 | - """Test `pick_new_mtime`.""" |
186 | - |
187 | - def test_pick_new_mtime_applies_starting_age_to_new_file(self): |
188 | - before = time.time() |
189 | - starting_age = randint(0, 5) |
190 | - recommended_age = pick_new_mtime(None, starting_age=starting_age) |
191 | - now = time.time() |
192 | - self.assertAlmostEqual( |
193 | - now - starting_age, |
194 | - recommended_age, |
195 | - delta=(now - before)) |
196 | - |
197 | - def test_pick_new_mtime_increments_mtime_if_possible(self): |
198 | - past = time.time() - 2 |
199 | - self.assertEqual(past + 1, pick_new_mtime(past)) |
200 | - |
201 | - def test_pick_new_mtime_refuses_to_move_mtime_into_the_future(self): |
202 | - # Race condition: this will fail if the test gets held up for |
203 | - # a second between readings of the clock. |
204 | - now = time.time() |
205 | - self.assertEqual(now, pick_new_mtime(now)) |
206 | - |
207 | - |
208 | class TestGetMAASProvisionCommand(MAASTestCase): |
209 | |
210 | def test__returns_just_command_for_production(self): |
Looks good; let's wait to land this until the new version of bind makes it from -proposed to -updates.