)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45d2f90b50d00efdfc6e031ada107a6a683f2cfe","unresolved":false,"context_lines":[{"line_number":38,"context_line":"Changes have been made to fix these problems:"},{"line_number":39,"context_line":"1) Collect metadata for a snapshot inside snapshot_volume_backed instead"},{"line_number":40,"context_line":"of outside it"},{"line_number":41,"context_line":"2) Collect metadata from an instance system metadata instead instead of"},{"line_number":42,"context_line":"from image/volume"},{"line_number":43,"context_line":"3) Clean useless properties to not store them in instance system"},{"line_number":44,"context_line":"metadata"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"baa041b7_f94d0973","line":41,"updated":"2015-04-03 13:25:25.000000000","message":"s/instead//","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"d89dbb8009fa89d58728caf1ef11daf37f5a1871","unresolved":false,"context_lines":[{"line_number":38,"context_line":"Changes have been made to fix these problems:"},{"line_number":39,"context_line":"1) Collect metadata for a snapshot inside snapshot_volume_backed instead"},{"line_number":40,"context_line":"of outside it"},{"line_number":41,"context_line":"2) Collect metadata from an instance system metadata instead instead of"},{"line_number":42,"context_line":"from image/volume"},{"line_number":43,"context_line":"3) Clean useless properties to not store them in instance system"},{"line_number":44,"context_line":"metadata"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"baa041b7_452751ca","line":41,"in_reply_to":"baa041b7_f94d0973","updated":"2015-04-05 00:53:09.000000000","message":"Done","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"fd0f1dcee72c330109d26ddbf327b3717eeb2923","unresolved":false,"context_lines":[{"line_number":50,"context_line":"Closes-Bug: #1369854"},{"line_number":51,"context_line":"Closes-Bug: #1384392"},{"line_number":52,"context_line":"Closes-Bug: #1439819"},{"line_number":53,"context_line":"Closes-Bug: #1439820"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"Change-Id: I067f66356a5ebd738add1591a0069d8049f35c24"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"7a016987_7d3cc292","line":53,"updated":"2015-06-01 14:30:34.000000000","message":"The fact that you\u0027ve details bullet points for 5 separate changes you\u0027ve made, and also linked to 4 separate bugs really strongly says this commit should have been split into a set of discrete commits, changing/fixing one thing/issue at a time. See also https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fb6c806530db17009229add6815114be6b4c3fac","unresolved":false,"context_lines":[{"line_number":50,"context_line":"Closes-Bug: #1369854"},{"line_number":51,"context_line":"Closes-Bug: #1384392"},{"line_number":52,"context_line":"Closes-Bug: #1439819"},{"line_number":53,"context_line":"Closes-Bug: #1439820"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"Change-Id: I067f66356a5ebd738add1591a0069d8049f35c24"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"fa32b979_efd438be","line":53,"in_reply_to":"7a016987_7d3cc292","updated":"2015-06-16 08:28:34.000000000","message":"1) resolves LP #1439820. 2) resolves all other bugs. If i split 1) and 2), i will at first move some block of code from one place to another and adopt tests for that. But then i will remove this block and readopt tests. So i think that it\u0027s inefficient to split 1) and 2).\n\nIf i split 2) and 5), i will get troubles to adopt tests because mapping property is truncated in instance system metadata to 255 chars. Sure i can fix the tests, but the result code will do not work properly with long mapping property. So i think that it\u0027s wrong to split 2) and 5).\n\nProbably i can move 3) and 4) to separate reviews.\n\nAs the result we will have 3 reviews. One will fix all 4 bugs. Other two will contain unforced changes (improvements not based on bugs or specs). Such reviews can be open a lot of time with no reaction from core reviewrs (e.g. see https://review.openstack.org/#/c/131026/).\n\nSo what can you recommend me?\n\nps. This review is a reincarnation of old https://review.openstack.org/#/c/95522/ which was ignored at all. Though it fixed both old bugs which was actual at that moment and these four bugs which was filed later, and also contained an attempt of refactoring you asked in this review.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"330c28c6c29e24e2f6797cb7cbdb4b2636cc104a","unresolved":false,"context_lines":[{"line_number":50,"context_line":"Closes-Bug: #1369854"},{"line_number":51,"context_line":"Closes-Bug: #1384392"},{"line_number":52,"context_line":"Closes-Bug: #1439819"},{"line_number":53,"context_line":"Closes-Bug: #1439820"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"Change-Id: I067f66356a5ebd738add1591a0069d8049f35c24"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_2e0f9089","line":53,"in_reply_to":"ba3cc151_d043ec40","updated":"2015-07-07 05:21:48.000000000","message":"Well, i\u0027ve splitted this to 3 reviews. But i didn\u0027t include cleaning of volume and instance metadata, since as i see you started some refactoring of instance metadata. So i think i should use the new objects to clean metadata, but i don\u0027t understand how to do this just now, because your work doesn\u0027t look finished. Will think about it later.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"6b08fc78c49f465fb879ff8a394eacf00655cea1","unresolved":false,"context_lines":[{"line_number":50,"context_line":"Closes-Bug: #1369854"},{"line_number":51,"context_line":"Closes-Bug: #1384392"},{"line_number":52,"context_line":"Closes-Bug: #1439819"},{"line_number":53,"context_line":"Closes-Bug: #1439820"},{"line_number":54,"context_line":""},{"line_number":55,"context_line":"Change-Id: I067f66356a5ebd738add1591a0069d8049f35c24"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"ba3cc151_d043ec40","line":53,"in_reply_to":"fa32b979_efd438be","updated":"2015-07-01 10:39:45.000000000","message":"Despite the fact that it might increase review time, I\u0027d still like to see this split - there has already been one piece of refactoring done as a separate patch. Splitting up the patch makes it much easier to review for correctness - as it is written now, it is hard to follow what each change is fixing.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"}],"nova/api/openstack/compute/servers.py":[{"author":{"_account_id":2276,"name":"Jorge Niedbalski","email":"jorge.niedbalski@canonical.com","username":"niedbalski"},"change_message_id":"b3075d31eae449af0d1cd690078e1821771122f1","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"                    image_meta \u003d {\u0027properties\u0027: properties}"},{"line_number":1080,"context_line":"                else:"},{"line_number":1081,"context_line":"                    image_meta \u003d self.compute_api.image_api.get(context, img)"},{"line_number":1082,"context_line":""},{"line_number":1083,"context_line":"                image \u003d self.compute_api.snapshot_volume_backed("},{"line_number":1084,"context_line":"                                                       context,"},{"line_number":1085,"context_line":"                                                       instance,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3acd31a7_a43292f0","side":"PARENT","line":1082,"updated":"2015-05-04 19:57:52.000000000","message":"Can you explain how this change will deal with https://bugs.launchpad.net/nova/+bug/1191069?","commit_id":"277ea04467c2fb0f24716b8e7e7799b43fc50af0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"f0ea8ebb26ec7a679feec5d845b89e4ca8c4c4b7","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"                    image_meta \u003d {\u0027properties\u0027: properties}"},{"line_number":1080,"context_line":"                else:"},{"line_number":1081,"context_line":"                    image_meta \u003d self.compute_api.image_api.get(context, img)"},{"line_number":1082,"context_line":""},{"line_number":1083,"context_line":"                image \u003d self.compute_api.snapshot_volume_backed("},{"line_number":1084,"context_line":"                                                       context,"},{"line_number":1085,"context_line":"                                                       instance,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3acd31a7_f5a26798","side":"PARENT","line":1082,"in_reply_to":"3acd31a7_a43292f0","updated":"2015-05-05 07:58:04.000000000","message":"Since this patchset uses instance system metadata to restore \u0027image\u0027 metadata, this change removes the loading of metadata from its original source (image or volume). Now snapshot_volume_backed collects metadata by itself, so we don\u0027t need to provide metadata to it.\n\nWhen an instance is booted from a volume, compute.api\u0027s _get_bdm_image_metadata gets image metadata from the volume and then this metadata is stored in instance system metadata with \u0027image_\u0027 prefix. snapshot_volume_backes uses this fact and calls utils.get_image_from_system_metadata to restore image metadata from instance system metadata.\n\nDoes this answer your question?","commit_id":"277ea04467c2fb0f24716b8e7e7799b43fc50af0"}],"nova/compute/api.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45d2f90b50d00efdfc6e031ada107a6a683f2cfe","unresolved":false,"context_lines":[{"line_number":2216,"context_line":"        \"\"\"Snapshot the given volume-backed instance."},{"line_number":2217,"context_line":""},{"line_number":2218,"context_line":"        :param instance: nova.objects.instance.Instance object"},{"line_number":2219,"context_line":"        :param image_meta: metadata for the new image"},{"line_number":2220,"context_line":"        :param name: name of the backup or snapshot"},{"line_number":2221,"context_line":"        :param extra_properties: dict of extra image properties to include"},{"line_number":2222,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_7ef9df7f","side":"PARENT","line":2219,"updated":"2015-04-03 13:25:25.000000000","message":"This line can now be removed","commit_id":"e9d8d933859d2cbbab0f07194ca1f114e10bf38c"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"d89dbb8009fa89d58728caf1ef11daf37f5a1871","unresolved":false,"context_lines":[{"line_number":2216,"context_line":"        \"\"\"Snapshot the given volume-backed instance."},{"line_number":2217,"context_line":""},{"line_number":2218,"context_line":"        :param instance: nova.objects.instance.Instance object"},{"line_number":2219,"context_line":"        :param image_meta: metadata for the new image"},{"line_number":2220,"context_line":"        :param name: name of the backup or snapshot"},{"line_number":2221,"context_line":"        :param extra_properties: dict of extra image properties to include"},{"line_number":2222,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_e5a66552","side":"PARENT","line":2219,"in_reply_to":"baa041b7_7ef9df7f","updated":"2015-04-05 00:53:09.000000000","message":"Done","commit_id":"e9d8d933859d2cbbab0f07194ca1f114e10bf38c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45d2f90b50d00efdfc6e031ada107a6a683f2cfe","unresolved":false,"context_lines":[{"line_number":2271,"context_line":""},{"line_number":2272,"context_line":"        if quiesced:"},{"line_number":2273,"context_line":"            self.compute_rpcapi.unquiesce_instance(context, instance, mapping)"},{"line_number":2274,"context_line":""},{"line_number":2275,"context_line":"        # NOTE (ndipanov): Remove swap/ephemerals from mappings as they will be"},{"line_number":2276,"context_line":"        # in the block_device_mapping for the new image."},{"line_number":2277,"context_line":"        image_mappings \u003d properties.get(\u0027mappings\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_7e555f4e","side":"PARENT","line":2274,"updated":"2015-04-03 13:25:25.000000000","message":"Nice clean","commit_id":"e9d8d933859d2cbbab0f07194ca1f114e10bf38c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45d2f90b50d00efdfc6e031ada107a6a683f2cfe","unresolved":false,"context_lines":[{"line_number":2238,"context_line":"                      \u0027properties\u0027: instance_image.get(\u0027properties\u0027, {})}"},{"line_number":2239,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2240,"context_line":"        # NOTE(ft): clean properties not actual for the snapshot or legacy"},{"line_number":2241,"context_line":"        for prop in (\u0027image_id\u0027, \u0027image_name\u0027, \u0027size\u0027, \u0027checksum\u0027,"},{"line_number":2242,"context_line":"                     \u0027base_image_ref\u0027, \u0027block_device_mapping\u0027, \u0027bdm_v2\u0027,"},{"line_number":2243,"context_line":"                     \u0027root_device_name\u0027, \u0027mappings\u0027):"},{"line_number":2244,"context_line":"            properties.pop(prop, None)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_fea16faa","line":2241,"updated":"2015-04-03 13:25:25.000000000","message":"Can we refer those properties somewhere in a \"constant\" perhaps nova/utils.py? SM_NON_INHERITABLE_KEYS_PROPERTY or whatever","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"d89dbb8009fa89d58728caf1ef11daf37f5a1871","unresolved":false,"context_lines":[{"line_number":2238,"context_line":"                      \u0027properties\u0027: instance_image.get(\u0027properties\u0027, {})}"},{"line_number":2239,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2240,"context_line":"        # NOTE(ft): clean properties not actual for the snapshot or legacy"},{"line_number":2241,"context_line":"        for prop in (\u0027image_id\u0027, \u0027image_name\u0027, \u0027size\u0027, \u0027checksum\u0027,"},{"line_number":2242,"context_line":"                     \u0027base_image_ref\u0027, \u0027block_device_mapping\u0027, \u0027bdm_v2\u0027,"},{"line_number":2243,"context_line":"                     \u0027root_device_name\u0027, \u0027mappings\u0027):"},{"line_number":2244,"context_line":"            properties.pop(prop, None)"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_85455971","line":2241,"in_reply_to":"baa041b7_fea16faa","updated":"2015-04-05 00:53:09.000000000","message":"I\u0027ve splitt them into two set. one is placed in utils, other is leaved here because it\u0027s strongly related to the function.","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"de7344823ed2d279bc881faac5a0da9af30316d8","unresolved":false,"context_lines":[{"line_number":2210,"context_line":"                      \u0027min_disk\u0027: instance_image.get(\u0027min_disk\u0027, 0),"},{"line_number":2211,"context_line":"                      \u0027properties\u0027: instance_image.get(\u0027properties\u0027, {})}"},{"line_number":2212,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2213,"context_line":"        # NOTE(ft): clean properties before filling"},{"line_number":2214,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2215,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2216,"context_line":"            properties.pop(prop, None)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_4c4c5042","line":2213,"updated":"2015-04-07 16:07:22.000000000","message":"Again not sure you need to use the term NOTE here, it\u0027s a general comment, nothing special","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06c19741d534d72974215def95669d48db7200f8","unresolved":false,"context_lines":[{"line_number":2210,"context_line":"                      \u0027min_disk\u0027: instance_image.get(\u0027min_disk\u0027, 0),"},{"line_number":2211,"context_line":"                      \u0027properties\u0027: instance_image.get(\u0027properties\u0027, {})}"},{"line_number":2212,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2213,"context_line":"        # NOTE(ft): clean properties before filling"},{"line_number":2214,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2215,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2216,"context_line":"            properties.pop(prop, None)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aaa499b_dfd00d56","line":2213,"in_reply_to":"9aa53dc9_4c4c5042","updated":"2015-04-13 18:34:15.000000000","message":"Done","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"fd0f1dcee72c330109d26ddbf327b3717eeb2923","unresolved":false,"context_lines":[{"line_number":1024,"context_line":"                # and depends on followed volume_api.check_attach() to"},{"line_number":1025,"context_line":"                # verify it. This hack should be harmless with that check."},{"line_number":1026,"context_line":"                image_meta[\u0027status\u0027] \u003d \u0027active\u0027"},{"line_number":1027,"context_line":"                return image_meta"},{"line_number":1028,"context_line":"        return {}"},{"line_number":1029,"context_line":""},{"line_number":1030,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a016987_7d528214","line":1027,"updated":"2015-06-01 14:30:34.000000000","message":"I feel like this block of code should all be isolated into a helper method like nova.utils.get_image_from_volume_metadata, to mirror the existing helper method get_image_from_system_metadata.  Of course any such re-factor should be done before adding the fix applied here.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"7ab2b8e000417af2da9cae3a560680fa817930e2","unresolved":false,"context_lines":[{"line_number":1024,"context_line":"                # and depends on followed volume_api.check_attach() to"},{"line_number":1025,"context_line":"                # verify it. This hack should be harmless with that check."},{"line_number":1026,"context_line":"                image_meta[\u0027status\u0027] \u003d \u0027active\u0027"},{"line_number":1027,"context_line":"                return image_meta"},{"line_number":1028,"context_line":"        return {}"},{"line_number":1029,"context_line":""},{"line_number":1030,"context_line":"    @staticmethod"}],"source_content_type":"text/x-python","patch_set":8,"id":"5afe65bd_8be5b6a9","line":1027,"in_reply_to":"7a016987_7d528214","updated":"2015-06-02 08:30:41.000000000","message":"https://review.openstack.org/#/c/187461/","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"fd0f1dcee72c330109d26ddbf327b3717eeb2923","unresolved":false,"context_lines":[{"line_number":2208,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2209,"context_line":"        # clean properties before filling"},{"line_number":2210,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2211,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2212,"context_line":"            properties.pop(prop, None)"},{"line_number":2213,"context_line":"        if instance.root_device_name:"},{"line_number":2214,"context_line":"            properties[\u0027root_device_name\u0027] \u003d instance.root_device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a016987_bdb23a47","line":2211,"updated":"2015-06-01 14:30:34.000000000","message":"You\u0027ve defined constants for various other lists of property names in this patch, but then hardcoded this list inline, which seems inconsistent.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fb6c806530db17009229add6815114be6b4c3fac","unresolved":false,"context_lines":[{"line_number":2208,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2209,"context_line":"        # clean properties before filling"},{"line_number":2210,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2211,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2212,"context_line":"            properties.pop(prop, None)"},{"line_number":2213,"context_line":"        if instance.root_device_name:"},{"line_number":2214,"context_line":"            properties[\u0027root_device_name\u0027] \u003d instance.root_device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_d2ca3ba1","line":2211,"in_reply_to":"7a016987_bdb23a47","updated":"2015-06-16 08:28:34.000000000","message":"In contrast to the constants which are used to clean unused properties, this list contains properties which are filled by this method. So this is a kind of initialization of \u0027variable\u0027 before to fill it.\n\nI think there is no reason to make a constant for this list. And to make a separate helper function as well. But if you don\u0027t like such \u0027initialization\u0027 way, i can do it near filling. See [1], [2], [3] below.\n\nOn the other hand [1] and [3] look redudant, because a volume backed instance must have root_device_name and at least the root volume in bdm. So i can probably remove \u0027if\u0027 conditions and leave only [2].\n\nWhat do you think?","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"2e3aa89822948a761d8235e55fb14869486b335d","unresolved":false,"context_lines":[{"line_number":2208,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2209,"context_line":"        # clean properties before filling"},{"line_number":2210,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2211,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2212,"context_line":"            properties.pop(prop, None)"},{"line_number":2213,"context_line":"        if instance.root_device_name:"},{"line_number":2214,"context_line":"            properties[\u0027root_device_name\u0027] \u003d instance.root_device_name"}],"source_content_type":"text/x-python","patch_set":8,"id":"ba3cc151_b7542121","line":2211,"in_reply_to":"fa32b979_d2ca3ba1","updated":"2015-07-01 10:19:34.000000000","message":"I don\u0027t really mind either way - i was just wanted to see an explanation for why you did it this way vs using constants elsewhere.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"fd0f1dcee72c330109d26ddbf327b3717eeb2923","unresolved":false,"context_lines":[{"line_number":2209,"context_line":"        # clean properties before filling"},{"line_number":2210,"context_line":"        for prop in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027,"},{"line_number":2211,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2212,"context_line":"            properties.pop(prop, None)"},{"line_number":2213,"context_line":"        if instance.root_device_name:"},{"line_number":2214,"context_line":"            properties[\u0027root_device_name\u0027] \u003d instance.root_device_name"},{"line_number":2215,"context_line":"        properties.update(extra_properties or {})"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a016987_bd5b5a21","line":2212,"updated":"2015-06-01 14:30:34.000000000","message":"It feels like there could be another helper method added to construct this too, eg get_image_from_volume_snapshot() or something.","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fb6c806530db17009229add6815114be6b4c3fac","unresolved":false,"context_lines":[{"line_number":2211,"context_line":"                     \u0027mappings\u0027):"},{"line_number":2212,"context_line":"            properties.pop(prop, None)"},{"line_number":2213,"context_line":"        if instance.root_device_name:"},{"line_number":2214,"context_line":"            properties[\u0027root_device_name\u0027] \u003d instance.root_device_name"},{"line_number":2215,"context_line":"        properties.update(extra_properties or {})"},{"line_number":2216,"context_line":""},{"line_number":2217,"context_line":"        quiesced \u003d False"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_12618339","line":2214,"updated":"2015-06-16 08:28:34.000000000","message":"[1]\n else:\n     properties.pop(\u0027root_device_name\u0027, None)","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fb6c806530db17009229add6815114be6b4c3fac","unresolved":false,"context_lines":[{"line_number":2256,"context_line":""},{"line_number":2257,"context_line":"        if quiesced:"},{"line_number":2258,"context_line":"            self.compute_rpcapi.unquiesce_instance(context, instance, mapping)"},{"line_number":2259,"context_line":""},{"line_number":2260,"context_line":"        if mapping:"},{"line_number":2261,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2262,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_32992720","line":2259,"updated":"2015-06-16 08:28:34.000000000","message":"[2]\n properties.pop(\u0027mapping\u0027, None)","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fb6c806530db17009229add6815114be6b4c3fac","unresolved":false,"context_lines":[{"line_number":2260,"context_line":"        if mapping:"},{"line_number":2261,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2262,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"},{"line_number":2263,"context_line":""},{"line_number":2264,"context_line":"        return self.image_api.create(context, image_meta)"},{"line_number":2265,"context_line":""},{"line_number":2266,"context_line":"    @wrap_check_policy"}],"source_content_type":"text/x-python","patch_set":8,"id":"fa32b979_92dcb3e5","line":2263,"updated":"2015-06-16 08:28:34.000000000","message":"[3]\n else:\n     properties.pop(\u0027block_device_mapping\u0027, None)\n     properties.pop(\u0027bdm_v2\u0027, None)","commit_id":"9a63a85a33ae4eff06b395a1f32e95ecd61d7f81"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"d616a2057b48ee14a383f950b4cad93c2765b51b","unresolved":false,"context_lines":[{"line_number":2360,"context_line":"        if image_mappings:"},{"line_number":2361,"context_line":"            properties[\u0027mappings\u0027] \u003d [m for m in image_mappings"},{"line_number":2362,"context_line":"                                      if not block_device.is_swap_or_ephemeral("},{"line_number":2363,"context_line":"                                          m[\u0027virtual\u0027])]"},{"line_number":2364,"context_line":"        if mapping:"},{"line_number":2365,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_f878da90","side":"PARENT","line":2363,"updated":"2015-08-13 14:31:46.000000000","message":"Why do we remove this? No bug refernces this as far as I can tell and it seems that we have to keep this because it can cause issues especially now that swap is in block device mapping.\n\nEphemeral too for that matter as it will add another ephemeral in case it was present here, which means we might be over the limit and the resulting image won\u0027t boot","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"ffc80659572e21bfc9297ad81986a486dc0c8638","unresolved":false,"context_lines":[{"line_number":2360,"context_line":"        if image_mappings:"},{"line_number":2361,"context_line":"            properties[\u0027mappings\u0027] \u003d [m for m in image_mappings"},{"line_number":2362,"context_line":"                                      if not block_device.is_swap_or_ephemeral("},{"line_number":2363,"context_line":"                                          m[\u0027virtual\u0027])]"},{"line_number":2364,"context_line":"        if mapping:"},{"line_number":2365,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_27f5bf4f","side":"PARENT","line":2363,"in_reply_to":"1a4dcd0f_1ba8201f","updated":"2015-08-13 16:01:44.000000000","message":"Sorry, my link for p.1 is bad. The right one is https://github.com/openstack/nova/blob/master/nova/utils.py#L1161\n\nAlso see L2045 in test_compute_api There is a check that if mapping property is contained in instance system metadata, then it is not put into image_meta anyway.","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"775f045acdb8002b570b68551968bd69d0096953","unresolved":false,"context_lines":[{"line_number":2360,"context_line":"        if image_mappings:"},{"line_number":2361,"context_line":"            properties[\u0027mappings\u0027] \u003d [m for m in image_mappings"},{"line_number":2362,"context_line":"                                      if not block_device.is_swap_or_ephemeral("},{"line_number":2363,"context_line":"                                          m[\u0027virtual\u0027])]"},{"line_number":2364,"context_line":"        if mapping:"},{"line_number":2365,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa1b9901_c7b3214c","side":"PARENT","line":2363,"in_reply_to":"1a4dcd0f_27f5bf4f","updated":"2015-08-25 16:19:44.000000000","message":"OK this seems fine - we will have the proper snapshot information to re-create the insatnce we were snapshotting even though we are loosing this data.","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"62eeaec0a5154bbed677e193d3d699b07ebd8099","unresolved":false,"context_lines":[{"line_number":2360,"context_line":"        if image_mappings:"},{"line_number":2361,"context_line":"            properties[\u0027mappings\u0027] \u003d [m for m in image_mappings"},{"line_number":2362,"context_line":"                                      if not block_device.is_swap_or_ephemeral("},{"line_number":2363,"context_line":"                                          m[\u0027virtual\u0027])]"},{"line_number":2364,"context_line":"        if mapping:"},{"line_number":2365,"context_line":"            properties[\u0027block_device_mapping\u0027] \u003d mapping"},{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_1ba8201f","side":"PARENT","line":2363,"in_reply_to":"1a4dcd0f_f878da90","updated":"2015-08-13 15:04:24.000000000","message":"1 utils.get_image_from_system_metadata doesn\u0027t return \u0027mapping\u0027 property at all (https://review.openstack.org/#/c/170243/12/nova/tests/unit/compute/test_compute_api.py)\n\n(This was done by DanPB because this property is stored in a string as serialized json, but Nova truncates long strings up to 255, so it can be not deserializable.)\n\nThus this patch doesn\u0027t store extra ephemerals image_meta.\n\n2 This can store root device name only, see L2046 of master code in test_compute_api (https://review.openstack.org/#/c/170243/12/nova/tests/unit/compute/test_compute_api.py). But since the volume-backed instance has root_device_name, the last one is stored in image_meta and is used later to boot an instance. So i think there is no reason to store it.","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"d616a2057b48ee14a383f950b4cad93c2765b51b","unresolved":false,"context_lines":[{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"},{"line_number":2367,"context_line":""},{"line_number":2368,"context_line":"        for attr in (\u0027status\u0027, \u0027location\u0027, \u0027id\u0027, \u0027owner\u0027):"},{"line_number":2369,"context_line":"            image_meta.pop(attr, None)"},{"line_number":2370,"context_line":""},{"line_number":2371,"context_line":"        # the new image is simply a bucket of properties (particularly the"},{"line_number":2372,"context_line":"        # block device mapping, kernel and ramdisk IDs) with no image data,"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_f8be9ad2","side":"PARENT","line":2369,"updated":"2015-08-13 14:31:46.000000000","message":"Also not sure why we want to ceep these now, even though it may be the right thing to do - might belong to another  pathc to keep this one as targetted as possible.","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"775f045acdb8002b570b68551968bd69d0096953","unresolved":false,"context_lines":[{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"},{"line_number":2367,"context_line":""},{"line_number":2368,"context_line":"        for attr in (\u0027status\u0027, \u0027location\u0027, \u0027id\u0027, \u0027owner\u0027):"},{"line_number":2369,"context_line":"            image_meta.pop(attr, None)"},{"line_number":2370,"context_line":""},{"line_number":2371,"context_line":"        # the new image is simply a bucket of properties (particularly the"},{"line_number":2372,"context_line":"        # block device mapping, kernel and ramdisk IDs) with no image data,"}],"source_content_type":"text/x-python","patch_set":12,"id":"fa1b9901_477e316b","side":"PARENT","line":2369,"in_reply_to":"1a4dcd0f_76da9b79","updated":"2015-08-25 16:19:44.000000000","message":"Ack","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"62eeaec0a5154bbed677e193d3d699b07ebd8099","unresolved":false,"context_lines":[{"line_number":2366,"context_line":"            properties[\u0027bdm_v2\u0027] \u003d True"},{"line_number":2367,"context_line":""},{"line_number":2368,"context_line":"        for attr in (\u0027status\u0027, \u0027location\u0027, \u0027id\u0027, \u0027owner\u0027):"},{"line_number":2369,"context_line":"            image_meta.pop(attr, None)"},{"line_number":2370,"context_line":""},{"line_number":2371,"context_line":"        # the new image is simply a bucket of properties (particularly the"},{"line_number":2372,"context_line":"        # block device mapping, kernel and ramdisk IDs) with no image data,"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_76da9b79","side":"PARENT","line":2369,"in_reply_to":"1a4dcd0f_f8be9ad2","updated":"2015-08-13 15:04:24.000000000","message":"All these attributes are not contained in image meta gotten from instance system metadata. It has only min_disk, min_size, container_format, disk_format attributes at the top level of dict. So.. no reason to drop something absent.","commit_id":"8c93400b5ba218a15757efd852226413923837a0"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"d616a2057b48ee14a383f950b4cad93c2765b51b","unresolved":false,"context_lines":[{"line_number":2301,"context_line":"                           \u0027name\u0027: name,"},{"line_number":2302,"context_line":"                           \u0027is_public\u0027: False})"},{"line_number":2303,"context_line":"        for attr in (\u0027container_format\u0027, \u0027disk_format\u0027):"},{"line_number":2304,"context_line":"            image_meta.pop(attr, None)"},{"line_number":2305,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2306,"context_line":"        # clean properties before filling"},{"line_number":2307,"context_line":"        for key in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_78104a4e","line":2304,"updated":"2015-08-13 14:31:46.000000000","message":"I am not a big fan of bundling fixes like this. might be worth it in this case though.","commit_id":"1bdf913eef48a2411bfb5727889f7899a58fd9ee"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"62eeaec0a5154bbed677e193d3d699b07ebd8099","unresolved":false,"context_lines":[{"line_number":2301,"context_line":"                           \u0027name\u0027: name,"},{"line_number":2302,"context_line":"                           \u0027is_public\u0027: False})"},{"line_number":2303,"context_line":"        for attr in (\u0027container_format\u0027, \u0027disk_format\u0027):"},{"line_number":2304,"context_line":"            image_meta.pop(attr, None)"},{"line_number":2305,"context_line":"        properties \u003d image_meta[\u0027properties\u0027]"},{"line_number":2306,"context_line":"        # clean properties before filling"},{"line_number":2307,"context_line":"        for key in (\u0027block_device_mapping\u0027, \u0027bdm_v2\u0027, \u0027root_device_name\u0027):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a4dcd0f_78adaa2f","line":2304,"in_reply_to":"1a4dcd0f_78104a4e","updated":"2015-08-13 15:04:24.000000000","message":"DanPB asked me on PS8 to split the review (https://review.openstack.org/#/c/170243/8//COMMIT_MSG)","commit_id":"1bdf913eef48a2411bfb5727889f7899a58fd9ee"}],"nova/utils.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45d2f90b50d00efdfc6e031ada107a6a683f2cfe","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"        # just the ones we need. Leaving it for now to keep the old behaviour."},{"line_number":1069,"context_line":"        if key.startswith(SM_IMAGE_PROP_PREFIX):"},{"line_number":1070,"context_line":"            key \u003d key[len(SM_IMAGE_PROP_PREFIX):]"},{"line_number":1071,"context_line":"        elif strict_image:"},{"line_number":1072,"context_line":"            continue"},{"line_number":1073,"context_line":""},{"line_number":1074,"context_line":"        if key in SM_INHERITABLE_KEYS:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_19c42d5d","line":1071,"updated":"2015-04-03 13:25:25.000000000","message":"So you avoid the way there are some keys introduced in properties and not desired, right? what can be those properties? - at least can we have a test for this new parameter.","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"d89dbb8009fa89d58728caf1ef11daf37f5a1871","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"        # just the ones we need. Leaving it for now to keep the old behaviour."},{"line_number":1069,"context_line":"        if key.startswith(SM_IMAGE_PROP_PREFIX):"},{"line_number":1070,"context_line":"            key \u003d key[len(SM_IMAGE_PROP_PREFIX):]"},{"line_number":1071,"context_line":"        elif strict_image:"},{"line_number":1072,"context_line":"            continue"},{"line_number":1073,"context_line":""},{"line_number":1074,"context_line":"        if key in SM_INHERITABLE_KEYS:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_45607101","line":1071,"in_reply_to":"baa041b7_19c42d5d","updated":"2015-04-05 00:53:09.000000000","message":"Done","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"bf4564174d0873c43926311e8ca9e5847056b51f","unresolved":false,"context_lines":[{"line_number":1068,"context_line":"        # just the ones we need. Leaving it for now to keep the old behaviour."},{"line_number":1069,"context_line":"        if key.startswith(SM_IMAGE_PROP_PREFIX):"},{"line_number":1070,"context_line":"            key \u003d key[len(SM_IMAGE_PROP_PREFIX):]"},{"line_number":1071,"context_line":"        elif strict_image:"},{"line_number":1072,"context_line":"            continue"},{"line_number":1073,"context_line":""},{"line_number":1074,"context_line":"        if key in SM_INHERITABLE_KEYS:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baa041b7_9d5b7b87","line":1071,"in_reply_to":"baa041b7_19c42d5d","updated":"2015-04-03 20:41:28.000000000","message":"System metadata includes not only image properties but also instance ones. For example network_allocated property, which is dependent of an instance state, and isn\u0027t image related.\n\nAll image properties starts with \u0027image_\u0027 prefix. This function removes it, but returns all properties regardless to the prefix. A note above mentions this fact.\n\nHere is a typical metadata of an instance booted from an imge:\n\n | image_container_format | ami                                  |\n | image_disk_format      | ami                                  |\n | image_min_disk         | 0                                    |\n | image_min_ram          | 0                                    |\n | image_kernel_id        | 06ecb03c-bb79-4d5d-a898-0fd1609cf764 |\n | image_ramdisk_id       | 0135796b-6d23-47be-bb80-9992e5535235 |\n | image_base_image_ref   | 109796e1-11d6-4e80-9825-6366d7a06d6b |\n | network_allocated      | True                                 |\n\nAnd here is the same for a volume backed instance:\n\n | image_container_format | ami                                  |\n | image_disk_format      | ami                                  |\n | image_min_disk         | 0                                    |\n | image_min_ram          | 0                                    |\n | image_kernel_id        | 06ecb03c-bb79-4d5d-a898-0fd1609cf764 |\n | image_ramdisk_id       | 0135796b-6d23-47be-bb80-9992e5535235 |\n | image_image_id         | 109796e1-11d6-4e80-9825-6366d7a06d6b |\n | image_image_name       | cirros-0.3.2-x86_64-uec              |\n | image_size             | 25165824                             |\n | image_checksum         | 4eada48c2843d2a262c814ddc92ecf2c     |\n | image_base_image_ref   |                                      |\n | network_allocated      | True                                 |\n\nI agree the test is needed. I will do.","commit_id":"5f7fe3d801eee7ef5344654ca9d39e0f9f575d97"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"de7344823ed2d279bc881faac5a0da9af30316d8","unresolved":false,"context_lines":[{"line_number":138,"context_line":"    \u0027min_ram\u0027, \u0027min_disk\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":139,"context_line":")"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"# NOTE(ft): These properties are stored in instance system metadata like"},{"line_number":142,"context_line":"# regular image properties (with \u0027image\u0027 prefix), but they are not properties"},{"line_number":143,"context_line":"# of a corresponding image."},{"line_number":144,"context_line":"SM_NOT_IMAGE_PROPERTIES \u003d ("}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_0c63e8dd","line":141,"updated":"2015-04-07 16:07:22.000000000","message":"Not necessary to use term \"NOTE\" here, just make a comment to explain what is that \"constant\".","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06c19741d534d72974215def95669d48db7200f8","unresolved":false,"context_lines":[{"line_number":138,"context_line":"    \u0027min_ram\u0027, \u0027min_disk\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":139,"context_line":")"},{"line_number":140,"context_line":""},{"line_number":141,"context_line":"# NOTE(ft): These properties are stored in instance system metadata like"},{"line_number":142,"context_line":"# regular image properties (with \u0027image\u0027 prefix), but they are not properties"},{"line_number":143,"context_line":"# of a corresponding image."},{"line_number":144,"context_line":"SM_NOT_IMAGE_PROPERTIES \u003d ("}],"source_content_type":"text/x-python","patch_set":3,"id":"7aaa499b_3f701172","line":141,"in_reply_to":"9aa53dc9_0c63e8dd","updated":"2015-04-13 18:34:15.000000000","message":"Done","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"de7344823ed2d279bc881faac5a0da9af30316d8","unresolved":false,"context_lines":[{"line_number":148,"context_line":"    \u0027base_image_ref\u0027,"},{"line_number":149,"context_line":")"},{"line_number":150,"context_line":""},{"line_number":151,"context_line":"# NOTE(ft): Cinder stores these properties as volume image metadata for"},{"line_number":152,"context_line":"# a bootable volume, but they are not properties of a corresponding image."},{"line_number":153,"context_line":"VIM_NOT_IMAGE_PROPERTIES \u003d ("},{"line_number":154,"context_line":"    \u0027image_id\u0027, \u0027image_name\u0027, \u0027size\u0027, \u0027checksum\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_aca07cf5","line":151,"updated":"2015-04-07 16:07:22.000000000","message":"Same thing here","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06c19741d534d72974215def95669d48db7200f8","unresolved":false,"context_lines":[{"line_number":148,"context_line":"    \u0027base_image_ref\u0027,"},{"line_number":149,"context_line":")"},{"line_number":150,"context_line":""},{"line_number":151,"context_line":"# NOTE(ft): Cinder stores these properties as volume image metadata for"},{"line_number":152,"context_line":"# a bootable volume, but they are not properties of a corresponding image."},{"line_number":153,"context_line":"VIM_NOT_IMAGE_PROPERTIES \u003d ("},{"line_number":154,"context_line":"    \u0027image_id\u0027, \u0027image_name\u0027, \u0027size\u0027, \u0027checksum\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aaa499b_1f6d5598","line":151,"in_reply_to":"9aa53dc9_aca07cf5","updated":"2015-04-13 18:34:15.000000000","message":"Done","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"de7344823ed2d279bc881faac5a0da9af30316d8","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_2fc8de54","line":1073,"updated":"2015-04-07 16:07:22.000000000","message":"What we can do here to make easier future developers to use it is that to make this one as private and create two new methods. Something like:\n\n  def _get_image_from_system_metadata(system_meta, strict_image\u003dFalse):\n    ...\n\n  def get_image_from_system_metadata(system_meta):\n    \"\"\"Returns dict of image metadata from the instance system meta...\n    return _get_image_from_system_metadata(system_meta, strict_image\u003dFalse)\n\n  def get_clean_image_from_system_metadata(system_meta):\n    \"\"\"Returns a clean dict of image metadata that can be used with new volumes...\n    return _get_image_from_system_metadata(system_meta, strict_image\u003dTrue)","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"06c19741d534d72974215def95669d48db7200f8","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"7aaa499b_aa0cdd94","line":1073,"in_reply_to":"7aaa499b_cc6cf49e","updated":"2015-04-13 18:34:15.000000000","message":"Done\n\nSince Nova had been switched to L-release, this is the best choice.","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"4858de33bf6406ba68fd24e05895c10205c60c3a","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_9af4c187","line":1073,"in_reply_to":"9aa53dc9_2fc8de54","updated":"2015-04-07 18:04:24.000000000","message":"As for me now get_image_from_system_metadata doesn\u0027t correspond to its name. It returns something funny: not pure image, not system metadata, but system metadata in image format. Since not long ago system metadata contained a copy of instance flavor, a purpose of such behavior is absolutely unclear for me.\n\nI guess the valuable destination of this function (if a caller does not need image format) is to truncate \u0027image_\u0027 prefix to get an easier access to image properties with no dependence of properties storage format. There is a lot of such usage in libvirt folder.\n\nI suppose that no one needs to access to a system metadata property by image format dict. As i understand xqueralt had thought the same when he leaved a comment in L1084.\n\nIn other words i mean that get_image_from_system_metadata should always return image properties only. But it\u0027s dangerous for backward compatibility to do it now. And anyway its should be done by a separate commit.\n\nHowever if you like two separate function i can create something like get_system_metadata_in_image_format and use it everywhere. Probably in a separate review though. But i\u0027m not sure this will make the code clearer.\n\nWhat is your opinion?","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"67b5cfc15cae70703146fb9d0a3a0f77812eb4e3","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"7aaa499b_cc6cf49e","line":1073,"in_reply_to":"9aa53dc9_3f5d4315","updated":"2015-04-13 12:42:59.000000000","message":"Well actually i believe you can just fix that method and remove the argument strict_image. I do not think there are cases for the behavior of the old method.","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"9dc3e3e10f35c74a4c3901898656e29cfd6fa7fe","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_b2e8b7e3","line":1073,"in_reply_to":"9aa53dc9_9af4c187","updated":"2015-04-08 08:27:53.000000000","message":"As you said this method is not good at all and we should probably not use it at all.\n\nMy first concern is about the argument you have added, \"strict_image\" it makes the method more complicated to understand that why I wanted you to create an other method with a good docstring.\n\n  def get_(clean|strict_image_from_system_metadata(system_meta):\n    \"\"\"Returns a clean dict of image metadata that can be used with new volumes...\n    return get_image_from_system_metadata(system_meta, strict_image\u003dTrue)\n  \nThen we can probably set a comment in the old one to make it deprecated. An other patchset can be addressed to clean our code path and finally remove that weird method.\n\nWhat do you think?","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"f596b05c46023a2917a9ee1671ce8b974e8b45c7","unresolved":false,"context_lines":[{"line_number":1070,"context_line":"    return system_meta"},{"line_number":1071,"context_line":""},{"line_number":1072,"context_line":""},{"line_number":1073,"context_line":"def get_image_from_system_metadata(system_meta, strict_image\u003dFalse):"},{"line_number":1074,"context_line":"    image_meta \u003d {}"},{"line_number":1075,"context_line":"    properties \u003d {}"},{"line_number":1076,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"9aa53dc9_3f5d4315","line":1073,"in_reply_to":"9aa53dc9_b2e8b7e3","updated":"2015-04-09 22:09:33.000000000","message":"I understand your concern about complication.\n\nBut you suggest to create the good function with a bad name, while the bad function still occupies the good name (i believe get_image_from_system_metadata is preffered name for the function after final refactoring). It\u0027s not ok for me.\n\nProbably we can select a clearer name for the argument? Such as get_image_properties_only, collect_image_properties_only, include_instance_properties, include_not_image_properties, or something else...","commit_id":"3265fa605a3ab438d2fb80c075754f0cb12eeb78"}]}