Merge ~niedbalski/ubuntu/+source/cinder:stable/newton into ~ubuntu-server-dev/ubuntu/+source/cinder:stable/newton
- Git
- lp:~niedbalski/ubuntu/+source/cinder
- stable/newton
- Merge into stable/newton
Proposed by
Jorge Niedbalski
Status: | Merged |
---|---|
Merged at revision: | 42e7a27b2c1068d6b8f0c103f7c0fb29a8bdff7f |
Proposed branch: | ~niedbalski/ubuntu/+source/cinder:stable/newton |
Merge into: | ~ubuntu-server-dev/ubuntu/+source/cinder:stable/newton |
Diff against target: |
335 lines (+314/-0) 3 files modified
debian/changelog (+7/-0) debian/patches/log-message-cleanup-for-volume-usage-audit.patch (+306/-0) debian/patches/series (+1/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ubuntu Server Developers | Pending | ||
Review via email: mp+319623@code.launchpad.net |
Commit message
Description of the change
* d/p/log-
Fixes LP: #1631561.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/debian/changelog b/debian/changelog | |||
2 | index 570a073..cdcd84a 100644 | |||
3 | --- a/debian/changelog | |||
4 | +++ b/debian/changelog | |||
5 | @@ -1,3 +1,10 @@ | |||
6 | 1 | cinder (2:9.1.2-0ubuntu2) yakkety; urgency=medium | ||
7 | 2 | |||
8 | 3 | * d/p/log-message-cleanup-for-volume-usage-audit.patch: | ||
9 | 4 | Fixes LP: #1631561. | ||
10 | 5 | |||
11 | 6 | -- Jorge Niedbalski <jorge.niedbalski@canonical.com> Fri, 10 Mar 2017 18:20:41 -0300 | ||
12 | 7 | |||
13 | 1 | cinder (2:9.1.2-0ubuntu1) yakkety; urgency=medium | 8 | cinder (2:9.1.2-0ubuntu1) yakkety; urgency=medium |
14 | 2 | 9 | ||
15 | 3 | [ Corey Bryant ] | 10 | [ Corey Bryant ] |
16 | diff --git a/debian/patches/log-message-cleanup-for-volume-usage-audit.patch b/debian/patches/log-message-cleanup-for-volume-usage-audit.patch | |||
17 | 4 | new file mode 100644 | 11 | new file mode 100644 |
18 | index 0000000..9421d04 | |||
19 | --- /dev/null | |||
20 | +++ b/debian/patches/log-message-cleanup-for-volume-usage-audit.patch | |||
21 | @@ -0,0 +1,306 @@ | |||
22 | 1 | From 82792be330e3b88aa1f625537ae1d2649e14ba4f Mon Sep 17 00:00:00 2001 | ||
23 | 2 | From: Zhiteng Huang <zhithuang@ebaysf.com> | ||
24 | 3 | Date: Sat, 8 Oct 2016 13:03:37 -0700 | ||
25 | 4 | Subject: [PATCH] Log message cleanup for volume-usage-audit | ||
26 | 5 | |||
27 | 6 | volume-usage-audit utlis logs unecessary message at for deleted | ||
28 | 7 | volumes or exception: | ||
29 | 8 | |||
30 | 9 | try: | ||
31 | 10 | LOG.debug('Send notification for vol X') | ||
32 | 11 | _notify_func | ||
33 | 12 | except Exception: | ||
34 | 13 | LOG.exception(...) | ||
35 | 14 | |||
36 | 15 | In the case when _notify_func raises exception, it logs | ||
37 | 16 | 'Send notification' and the exception. | ||
38 | 17 | |||
39 | 18 | This change reduce logs by only logging 'Sent notification' when | ||
40 | 19 | there is no exception by: | ||
41 | 20 | |||
42 | 21 | try: | ||
43 | 22 | _notify_func | ||
44 | 23 | LOG.debug('Sent notification for vol X') | ||
45 | 24 | except Exception: | ||
46 | 25 | LOG.exception(...) | ||
47 | 26 | |||
48 | 27 | Also, this change fixes notify_snapshot_usage() for not handling | ||
49 | 28 | the case when source volume for the snapshot is deleted. | ||
50 | 29 | |||
51 | 30 | Closes-bug: #1631561 | ||
52 | 31 | |||
53 | 32 | Change-Id: Iff5448e888ee39bce1ff99c7bf48581d655fb687 | ||
54 | 33 | --- | ||
55 | 34 | cinder/cmd/volume_usage_audit.py | 80 +++++++++++++++++----------------- | ||
56 | 35 | cinder/tests/unit/test_volume_utils.py | 46 +++++++++++++++++++ | ||
57 | 36 | cinder/volume/utils.py | 14 +++++- | ||
58 | 37 | 3 files changed, 99 insertions(+), 41 deletions(-) | ||
59 | 38 | |||
60 | 39 | diff --git a/cinder/cmd/volume_usage_audit.py b/cinder/cmd/volume_usage_audit.py | ||
61 | 40 | index ba8d38f..678d4e0 100644 | ||
62 | 41 | --- a/cinder/cmd/volume_usage_audit.py | ||
63 | 42 | +++ b/cinder/cmd/volume_usage_audit.py | ||
64 | 43 | @@ -48,7 +48,7 @@ from cinder import i18n | ||
65 | 44 | i18n.enable_lazy() | ||
66 | 45 | from cinder import context | ||
67 | 46 | from cinder import db | ||
68 | 47 | -from cinder.i18n import _, _LE | ||
69 | 48 | +from cinder.i18n import _, _LE, _LI | ||
70 | 49 | from cinder import objects | ||
71 | 50 | from cinder import rpc | ||
72 | 51 | from cinder import utils | ||
73 | 52 | @@ -95,9 +95,9 @@ def main(): | ||
74 | 53 | 'end': end} | ||
75 | 54 | LOG.error(msg) | ||
76 | 55 | sys.exit(-1) | ||
77 | 56 | - LOG.debug("Starting volume usage audit") | ||
78 | 57 | - msg = _("Creating usages for %(begin_period)s until %(end_period)s") | ||
79 | 58 | - LOG.debug(msg, {"begin_period": str(begin), "end_period": str(end)}) | ||
80 | 59 | + LOG.info(_LI("Starting volume usage audit")) | ||
81 | 60 | + msg = _LI("Creating usages for %(begin_period)s until %(end_period)s") | ||
82 | 61 | + LOG.info(msg, {"begin_period": str(begin), "end_period": str(end)}) | ||
83 | 62 | |||
84 | 63 | extra_info = { | ||
85 | 64 | 'audit_period_beginning': str(begin), | ||
86 | 65 | @@ -107,19 +107,19 @@ def main(): | ||
87 | 66 | volumes = db.volume_get_active_by_window(admin_context, | ||
88 | 67 | begin, | ||
89 | 68 | end) | ||
90 | 69 | - LOG.debug("Found %d volumes", len(volumes)) | ||
91 | 70 | + LOG.info(_LI("Found %d volumes"), len(volumes)) | ||
92 | 71 | for volume_ref in volumes: | ||
93 | 72 | try: | ||
94 | 73 | - LOG.debug("Send exists notification for <volume_id: " | ||
95 | 74 | + cinder.volume.utils.notify_about_volume_usage( | ||
96 | 75 | + admin_context, | ||
97 | 76 | + volume_ref, | ||
98 | 77 | + 'exists', extra_usage_info=extra_info) | ||
99 | 78 | + LOG.debug("Sent exists notification for <volume_id: " | ||
100 | 79 | "%(volume_id)s> <project_id %(project_id)s> " | ||
101 | 80 | "<%(extra_info)s>", | ||
102 | 81 | {'volume_id': volume_ref.id, | ||
103 | 82 | 'project_id': volume_ref.project_id, | ||
104 | 83 | 'extra_info': extra_info}) | ||
105 | 84 | - cinder.volume.utils.notify_about_volume_usage( | ||
106 | 85 | - admin_context, | ||
107 | 86 | - volume_ref, | ||
108 | 87 | - 'exists', extra_usage_info=extra_info) | ||
109 | 88 | except Exception as exc_msg: | ||
110 | 89 | LOG.exception(_LE("Exists volume notification failed: %s"), | ||
111 | 90 | exc_msg, resource=volume_ref) | ||
112 | 91 | @@ -132,12 +132,6 @@ def main(): | ||
113 | 92 | 'audit_period_beginning': str(volume_ref.created_at), | ||
114 | 93 | 'audit_period_ending': str(volume_ref.created_at), | ||
115 | 94 | } | ||
116 | 95 | - LOG.debug("Send create notification for " | ||
117 | 96 | - "<volume_id: %(volume_id)s> " | ||
118 | 97 | - "<project_id %(project_id)s> <%(extra_info)s>", | ||
119 | 98 | - {'volume_id': volume_ref.id, | ||
120 | 99 | - 'project_id': volume_ref.project_id, | ||
121 | 100 | - 'extra_info': local_extra_info}) | ||
122 | 101 | cinder.volume.utils.notify_about_volume_usage( | ||
123 | 102 | admin_context, | ||
124 | 103 | volume_ref, | ||
125 | 104 | @@ -146,6 +140,12 @@ def main(): | ||
126 | 105 | admin_context, | ||
127 | 106 | volume_ref, | ||
128 | 107 | 'create.end', extra_usage_info=local_extra_info) | ||
129 | 108 | + LOG.debug("Sent create notification for " | ||
130 | 109 | + "<volume_id: %(volume_id)s> " | ||
131 | 110 | + "<project_id %(project_id)s> <%(extra_info)s>", | ||
132 | 111 | + {'volume_id': volume_ref.id, | ||
133 | 112 | + 'project_id': volume_ref.project_id, | ||
134 | 113 | + 'extra_info': local_extra_info}) | ||
135 | 114 | except Exception as exc_msg: | ||
136 | 115 | LOG.exception(_LE("Create volume notification failed: %s"), | ||
137 | 116 | exc_msg, resource=volume_ref) | ||
138 | 117 | @@ -158,12 +158,6 @@ def main(): | ||
139 | 118 | 'audit_period_beginning': str(volume_ref.deleted_at), | ||
140 | 119 | 'audit_period_ending': str(volume_ref.deleted_at), | ||
141 | 120 | } | ||
142 | 121 | - LOG.debug("Send delete notification for " | ||
143 | 122 | - "<volume_id: %(volume_id)s> " | ||
144 | 123 | - "<project_id %(project_id)s> <%(extra_info)s>", | ||
145 | 124 | - {'volume_id': volume_ref.id, | ||
146 | 125 | - 'project_id': volume_ref.project_id, | ||
147 | 126 | - 'extra_info': local_extra_info}) | ||
148 | 127 | cinder.volume.utils.notify_about_volume_usage( | ||
149 | 128 | admin_context, | ||
150 | 129 | volume_ref, | ||
151 | 130 | @@ -172,24 +166,30 @@ def main(): | ||
152 | 131 | admin_context, | ||
153 | 132 | volume_ref, | ||
154 | 133 | 'delete.end', extra_usage_info=local_extra_info) | ||
155 | 134 | + LOG.debug("Sent delete notification for " | ||
156 | 135 | + "<volume_id: %(volume_id)s> " | ||
157 | 136 | + "<project_id %(project_id)s> <%(extra_info)s>", | ||
158 | 137 | + {'volume_id': volume_ref.id, | ||
159 | 138 | + 'project_id': volume_ref.project_id, | ||
160 | 139 | + 'extra_info': local_extra_info}) | ||
161 | 140 | except Exception as exc_msg: | ||
162 | 141 | LOG.exception(_LE("Delete volume notification failed: %s"), | ||
163 | 142 | exc_msg, resource=volume_ref) | ||
164 | 143 | |||
165 | 144 | snapshots = objects.SnapshotList.get_active_by_window(admin_context, | ||
166 | 145 | begin, end) | ||
167 | 146 | - LOG.debug("Found %d snapshots", len(snapshots)) | ||
168 | 147 | + LOG.info(_LI("Found %d snapshots"), len(snapshots)) | ||
169 | 148 | for snapshot_ref in snapshots: | ||
170 | 149 | try: | ||
171 | 150 | - LOG.debug("Send notification for <snapshot_id: %(snapshot_id)s> " | ||
172 | 151 | - "<project_id %(project_id)s> <%(extra_info)s>", | ||
173 | 152 | - {'snapshot_id': snapshot_ref.id, | ||
174 | 153 | - 'project_id': snapshot_ref.project_id, | ||
175 | 154 | - 'extra_info': extra_info}) | ||
176 | 155 | cinder.volume.utils.notify_about_snapshot_usage(admin_context, | ||
177 | 156 | snapshot_ref, | ||
178 | 157 | 'exists', | ||
179 | 158 | extra_info) | ||
180 | 159 | + LOG.debug("Sent notification for <snapshot_id: %(snapshot_id)s> " | ||
181 | 160 | + "<project_id %(project_id)s> <%(extra_info)s>", | ||
182 | 161 | + {'snapshot_id': snapshot_ref.id, | ||
183 | 162 | + 'project_id': snapshot_ref.project_id, | ||
184 | 163 | + 'extra_info': extra_info}) | ||
185 | 164 | except Exception as exc_msg: | ||
186 | 165 | LOG.exception(_LE("Exists snapshot notification failed: %s"), | ||
187 | 166 | exc_msg, resource=snapshot_ref) | ||
188 | 167 | @@ -202,12 +202,6 @@ def main(): | ||
189 | 168 | 'audit_period_beginning': str(snapshot_ref.created_at), | ||
190 | 169 | 'audit_period_ending': str(snapshot_ref.created_at), | ||
191 | 170 | } | ||
192 | 171 | - LOG.debug("Send create notification for " | ||
193 | 172 | - "<snapshot_id: %(snapshot_id)s> " | ||
194 | 173 | - "<project_id %(project_id)s> <%(extra_info)s>", | ||
195 | 174 | - {'snapshot_id': snapshot_ref.id, | ||
196 | 175 | - 'project_id': snapshot_ref.project_id, | ||
197 | 176 | - 'extra_info': local_extra_info}) | ||
198 | 177 | cinder.volume.utils.notify_about_snapshot_usage( | ||
199 | 178 | admin_context, | ||
200 | 179 | snapshot_ref, | ||
201 | 180 | @@ -216,6 +210,12 @@ def main(): | ||
202 | 181 | admin_context, | ||
203 | 182 | snapshot_ref, | ||
204 | 183 | 'create.end', extra_usage_info=local_extra_info) | ||
205 | 184 | + LOG.debug("Sent create notification for " | ||
206 | 185 | + "<snapshot_id: %(snapshot_id)s> " | ||
207 | 186 | + "<project_id %(project_id)s> <%(extra_info)s>", | ||
208 | 187 | + {'snapshot_id': snapshot_ref.id, | ||
209 | 188 | + 'project_id': snapshot_ref.project_id, | ||
210 | 189 | + 'extra_info': local_extra_info}) | ||
211 | 190 | except Exception as exc_msg: | ||
212 | 191 | LOG.exception(_LE("Create snapshot notification failed: %s"), | ||
213 | 192 | exc_msg, resource=snapshot_ref) | ||
214 | 193 | @@ -228,12 +228,6 @@ def main(): | ||
215 | 194 | 'audit_period_beginning': str(snapshot_ref.deleted_at), | ||
216 | 195 | 'audit_period_ending': str(snapshot_ref.deleted_at), | ||
217 | 196 | } | ||
218 | 197 | - LOG.debug("Send delete notification for " | ||
219 | 198 | - "<snapshot_id: %(snapshot_id)s> " | ||
220 | 199 | - "<project_id %(project_id)s> <%(extra_info)s>", | ||
221 | 200 | - {'snapshot_id': snapshot_ref.id, | ||
222 | 201 | - 'project_id': snapshot_ref.project_id, | ||
223 | 202 | - 'extra_info': local_extra_info}) | ||
224 | 203 | cinder.volume.utils.notify_about_snapshot_usage( | ||
225 | 204 | admin_context, | ||
226 | 205 | snapshot_ref, | ||
227 | 206 | @@ -242,6 +236,12 @@ def main(): | ||
228 | 207 | admin_context, | ||
229 | 208 | snapshot_ref, | ||
230 | 209 | 'delete.end', extra_usage_info=local_extra_info) | ||
231 | 210 | + LOG.debug("Sent delete notification for " | ||
232 | 211 | + "<snapshot_id: %(snapshot_id)s> " | ||
233 | 212 | + "<project_id %(project_id)s> <%(extra_info)s>", | ||
234 | 213 | + {'snapshot_id': snapshot_ref.id, | ||
235 | 214 | + 'project_id': snapshot_ref.project_id, | ||
236 | 215 | + 'extra_info': local_extra_info}) | ||
237 | 216 | except Exception as exc_msg: | ||
238 | 217 | LOG.exception(_LE("Delete snapshot notification failed: %s"), | ||
239 | 218 | exc_msg, resource=snapshot_ref) | ||
240 | 219 | diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py | ||
241 | 220 | index c49b113..bca8c71 100644 | ||
242 | 221 | --- a/cinder/tests/unit/test_volume_utils.py | ||
243 | 222 | +++ b/cinder/tests/unit/test_volume_utils.py | ||
244 | 223 | @@ -166,6 +166,52 @@ class NotifyUsageTestCase(test.TestCase): | ||
245 | 224 | } | ||
246 | 225 | self.assertDictMatch(expected_snapshot, usage_info) | ||
247 | 226 | |||
248 | 227 | + @mock.patch('cinder.objects.Volume.get_by_id') | ||
249 | 228 | + def test_usage_from_deleted_snapshot(self, volume_get_by_id): | ||
250 | 229 | + raw_volume = { | ||
251 | 230 | + 'id': fake.VOLUME_ID, | ||
252 | 231 | + 'availability_zone': 'nova', | ||
253 | 232 | + 'deleted': 1 | ||
254 | 233 | + } | ||
255 | 234 | + ctxt = context.get_admin_context() | ||
256 | 235 | + volume_obj = fake_volume.fake_volume_obj(ctxt, **raw_volume) | ||
257 | 236 | + volume_get_by_id.side_effect = exception.VolumeNotFound( | ||
258 | 237 | + volume_id=fake.VOLUME_ID) | ||
259 | 238 | + | ||
260 | 239 | + raw_snapshot = { | ||
261 | 240 | + 'project_id': fake.PROJECT_ID, | ||
262 | 241 | + 'user_id': fake.USER_ID, | ||
263 | 242 | + 'volume': volume_obj, | ||
264 | 243 | + 'volume_id': fake.VOLUME_ID, | ||
265 | 244 | + 'volume_size': 1, | ||
266 | 245 | + 'id': fake.SNAPSHOT_ID, | ||
267 | 246 | + 'display_name': '11', | ||
268 | 247 | + 'created_at': '2014-12-11T10:10:00', | ||
269 | 248 | + 'status': fields.SnapshotStatus.ERROR, | ||
270 | 249 | + 'deleted': '', | ||
271 | 250 | + 'snapshot_metadata': [{'key': 'fake_snap_meta_key', | ||
272 | 251 | + 'value': 'fake_snap_meta_value'}], | ||
273 | 252 | + 'expected_attrs': ['metadata'], | ||
274 | 253 | + } | ||
275 | 254 | + | ||
276 | 255 | + snapshot_obj = fake_snapshot.fake_snapshot_obj(ctxt, **raw_snapshot) | ||
277 | 256 | + usage_info = volume_utils._usage_from_snapshot(snapshot_obj) | ||
278 | 257 | + expected_snapshot = { | ||
279 | 258 | + 'tenant_id': fake.PROJECT_ID, | ||
280 | 259 | + 'user_id': fake.USER_ID, | ||
281 | 260 | + 'availability_zone': '', | ||
282 | 261 | + 'volume_id': fake.VOLUME_ID, | ||
283 | 262 | + 'volume_size': 1, | ||
284 | 263 | + 'snapshot_id': fake.SNAPSHOT_ID, | ||
285 | 264 | + 'display_name': '11', | ||
286 | 265 | + 'created_at': 'DONTCARE', | ||
287 | 266 | + 'status': fields.SnapshotStatus.ERROR, | ||
288 | 267 | + 'deleted': '', | ||
289 | 268 | + 'metadata': six.text_type({'fake_snap_meta_key': | ||
290 | 269 | + u'fake_snap_meta_value'}), | ||
291 | 270 | + } | ||
292 | 271 | + self.assertDictMatch(expected_snapshot, usage_info) | ||
293 | 272 | + | ||
294 | 273 | @mock.patch('cinder.db.volume_glance_metadata_get') | ||
295 | 274 | @mock.patch('cinder.db.volume_attachment_get_all_by_volume_id') | ||
296 | 275 | def test_usage_from_volume(self, mock_attachment, mock_image_metadata): | ||
297 | 276 | diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py | ||
298 | 277 | index 0aec08a..c811e2d 100644 | ||
299 | 278 | --- a/cinder/volume/utils.py | ||
300 | 279 | +++ b/cinder/volume/utils.py | ||
301 | 280 | @@ -149,10 +149,22 @@ def notify_about_backup_usage(context, backup, event_suffix, | ||
302 | 281 | |||
303 | 282 | |||
304 | 283 | def _usage_from_snapshot(snapshot, **extra_usage_info): | ||
305 | 284 | + try: | ||
306 | 285 | + az = snapshot.volume['availability_zone'] | ||
307 | 286 | + except exception.VolumeNotFound: | ||
308 | 287 | + # (zhiteng) Snapshot's source volume could have been deleted | ||
309 | 288 | + # (which means snapshot has been deleted as well), | ||
310 | 289 | + # lazy-loading volume would raise VolumeNotFound exception. | ||
311 | 290 | + # In that case, not going any further by abusing low level | ||
312 | 291 | + # DB API to fetch deleted volume but simply return empty | ||
313 | 292 | + # string for snapshot's AZ info. | ||
314 | 293 | + az = '' | ||
315 | 294 | + LOG.debug("Source volume %s deleted", snapshot.volume_id) | ||
316 | 295 | + | ||
317 | 296 | usage_info = { | ||
318 | 297 | 'tenant_id': snapshot.project_id, | ||
319 | 298 | 'user_id': snapshot.user_id, | ||
320 | 299 | - 'availability_zone': snapshot.volume['availability_zone'], | ||
321 | 300 | + 'availability_zone': az, | ||
322 | 301 | 'volume_id': snapshot.volume_id, | ||
323 | 302 | 'volume_size': snapshot.volume_size, | ||
324 | 303 | 'snapshot_id': snapshot.id, | ||
325 | 304 | -- | ||
326 | 305 | 2.7.4 | ||
327 | 306 | |||
328 | diff --git a/debian/patches/series b/debian/patches/series | |||
329 | index 7abc008..238c9c6 100644 | |||
330 | --- a/debian/patches/series | |||
331 | +++ b/debian/patches/series | |||
332 | @@ -1,2 +1,3 @@ | |||
333 | 1 | google-back-requirements.patch | 1 | google-back-requirements.patch |
334 | 2 | fix-long-casting.patch | 2 | fix-long-casting.patch |
335 | 3 | log-message-cleanup-for-volume-usage-audit.patch |