)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"e77cf37f4301b1fe0d4342922b90477d96299410","unresolved":false,"context_lines":[{"line_number":22,"context_line":"(does not take into accont disk.ephN names), and no longer used."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Closes-bug: 1596957"},{"line_number":25,"context_line":"Closes-bug: 1587802"},{"line_number":26,"context_line":"Change-Id: Ic38c28edaf90247ea9638e8aa742255f6edc5325"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1aa78d24_382bae83","line":25,"updated":"2016-07-13 08:06:04.000000000","message":"by coincidence this fix is addressing even bug 1598661, please add it to the commit message","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"79b261807e9d42644e75b14d6a85ad4d3dac0e4b","unresolved":false,"context_lines":[{"line_number":22,"context_line":"(does not take into accont disk.ephN names), and no longer used."},{"line_number":23,"context_line":""},{"line_number":24,"context_line":"Closes-bug: 1596957"},{"line_number":25,"context_line":"Closes-bug: 1587802"},{"line_number":26,"context_line":"Change-Id: Ic38c28edaf90247ea9638e8aa742255f6edc5325"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"dada55a8_ed709d42","line":25,"in_reply_to":"1aa78d24_382bae83","updated":"2016-07-26 05:14:48.000000000","message":"It does not in fact. It fixes the check, added in I7989128d5bfa027c8f566c9d66956b1c4d328db5, but does not remove it. The last patch in this series (Ic2ca8aa42ce67a95bcdae946289179f98e6103e8) removes the duplicate check.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"23b958dae2f2e11575348e513d364f1f1836081a","unresolved":false,"context_lines":[{"line_number":14336,"context_line":"                                          \u0027connection_info\u0027: None},"},{"line_number":14337,"context_line":"                                         {\u0027boot_index\u0027: 0,"},{"line_number":14338,"context_line":"                                          \u0027mount_device\u0027: \u0027/dev/vda\u0027,"},{"line_number":14339,"context_line":"                                          \u0027connection_info\u0027: None}]}"},{"line_number":14340,"context_line":"        flavor \u003d {\u0027root_gb\u0027: 1, \u0027ephemeral_gb\u0027: 0}"},{"line_number":14341,"context_line":"        flavor_obj \u003d objects.Flavor(**flavor)"},{"line_number":14342,"context_line":"        self._test_migrate_disk_and_power_off("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_36c9e65b","line":14339,"updated":"2016-08-12 10:01:03.000000000","message":"If your goal is to test that disconnect_volume() was correctly called below, these 2 connection_infos should probably be mock sentinels. i.e. mock.sentinel.conn_info_vda and mock.sentinel.conn_info_vdd. That was you can assert mock.sentinel.conn_info_vda below. Asserting None below is a weak test.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"ee9cb8f95e0cb9d6d2758cbd492bca46343b19f1","unresolved":false,"context_lines":[{"line_number":14336,"context_line":"                                          \u0027connection_info\u0027: None},"},{"line_number":14337,"context_line":"                                         {\u0027boot_index\u0027: 0,"},{"line_number":14338,"context_line":"                                          \u0027mount_device\u0027: \u0027/dev/vda\u0027,"},{"line_number":14339,"context_line":"                                          \u0027connection_info\u0027: None}]}"},{"line_number":14340,"context_line":"        flavor \u003d {\u0027root_gb\u0027: 1, \u0027ephemeral_gb\u0027: 0}"},{"line_number":14341,"context_line":"        flavor_obj \u003d objects.Flavor(**flavor)"},{"line_number":14342,"context_line":"        self._test_migrate_disk_and_power_off("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_69cec5b5","line":14339,"in_reply_to":"9ad45d7e_36c9e65b","updated":"2016-08-12 17:46:01.000000000","message":"Done","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"7894c1ae67002a504ffb5b0de8459c0eff82ca9b","unresolved":false,"context_lines":[{"line_number":15346,"context_line":"        self._test_migrate_disk_and_power_off("},{"line_number":15347,"context_line":"            flavor_obj, block_device_info\u003dinfo,"},{"line_number":15348,"context_line":"            params_for_instance\u003d{"},{"line_number":15349,"context_line":"                \u0027image_ref\u0027:  uuids.fake_volume_backed_image_ref,"},{"line_number":15350,"context_line":"                \u0027root_gb\u0027: 10,"},{"line_number":15351,"context_line":"                \u0027ephemeral_gb\u0027: 0,"},{"line_number":15352,"context_line":"                \u0027flavor\u0027: {\u0027root_gb\u0027: 10,"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_59c6414a","line":15349,"updated":"2016-08-12 13:15:10.000000000","message":"You noticed the pep8 failure here, right?","commit_id":"93ce48ba4afe14d0435fe51f81e0c08f142ca610"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a8cab2824ed36f82a06370c7a8d3ff5f0f01ce4e","unresolved":false,"context_lines":[{"line_number":312,"context_line":""},{"line_number":313,"context_line":"    # root disk"},{"line_number":314,"context_line":"    if (instance.image_ref is not None and"},{"line_number":315,"context_line":"            instance.image_ref !\u003d uuids.fake_volume_backed_image_ref):"},{"line_number":316,"context_line":"        cache_name \u003d imagecache.get_cache_fname(instance.image_ref)"},{"line_number":317,"context_line":"        disk_info[\u0027disk\u0027] \u003d {"},{"line_number":318,"context_line":"            \u0027type\u0027: type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a89bdaa_087ee718","line":315,"updated":"2016-09-06 13:58:36.000000000","message":"Would personally have used a boot_from_volume argument to the function, I think, but this is fine.","commit_id":"b36f90572e606b3ee2ebe34d934842b7596e5dad"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"3e698f90d05a5d594009606c8f4353386b3d9b65","unresolved":false,"context_lines":[{"line_number":314,"context_line":"    disk_info \u003d OrderedDict()"},{"line_number":315,"context_line":""},{"line_number":316,"context_line":"    # root disk"},{"line_number":317,"context_line":"    if (instance.image_ref is not None and"},{"line_number":318,"context_line":"            instance.image_ref !\u003d uuids.fake_volume_backed_image_ref):"},{"line_number":319,"context_line":"        cache_name \u003d imagecache.get_cache_fname(instance.image_ref)"},{"line_number":320,"context_line":"        disk_info[\u0027disk\u0027] \u003d {"},{"line_number":321,"context_line":"            \u0027type\u0027: type,"}],"source_content_type":"text/x-python","patch_set":7,"id":"1a95cdbc_7f0a3e27","line":318,"range":{"start_line":317,"start_character":0,"end_line":318,"end_character":70},"updated":"2016-10-10 14:54:45.000000000","message":"nit: Since we\u0027re not checking against non-null values now, you could simplify to:\n\n    if instance.image_ref and instance.image_ref !\u003d ...","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"c79bd637af832c71d3f5211717f197e431eb9285","unresolved":false,"context_lines":[{"line_number":5337,"context_line":""},{"line_number":5338,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":5339,"context_line":"            block_device_info)"},{"line_number":5340,"context_line":"        booted_from_volume \u003d bool("},{"line_number":5341,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":5342,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5343,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5344,"context_line":"        disk_infos \u003d jsonutils.loads(disk_info_text)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_3f98211b","line":5341,"range":{"start_line":5340,"start_character":0,"end_line":5341,"end_character":60},"updated":"2016-07-11 14:33:24.000000000","message":"why do not include this logic in the existing _is_booted_from_volume method?","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"79b261807e9d42644e75b14d6a85ad4d3dac0e4b","unresolved":false,"context_lines":[{"line_number":5337,"context_line":""},{"line_number":5338,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":5339,"context_line":"            block_device_info)"},{"line_number":5340,"context_line":"        booted_from_volume \u003d bool("},{"line_number":5341,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":5342,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5343,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5344,"context_line":"        disk_infos \u003d jsonutils.loads(disk_info_text)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_adcfc554","line":5341,"range":{"start_line":5340,"start_character":0,"end_line":5341,"end_character":60},"in_reply_to":"1aa78d24_3f98211b","updated":"2016-07-26 05:14:48.000000000","message":"Because this block_device_mapping is not a disk_mapping, required by _is_booted_from_volume. block_device_mapping is a list, but disk_mapping must be a dictionary, in which keys are disk names (disk, disk.eph0, etc). Neither block_device_info, nor original block_device_mapping have such info.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"8e55235da394dec9db5865300ee0b2bd46c2f2cd","unresolved":false,"context_lines":[{"line_number":5337,"context_line":""},{"line_number":5338,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":5339,"context_line":"            block_device_info)"},{"line_number":5340,"context_line":"        booted_from_volume \u003d bool("},{"line_number":5341,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":5342,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5343,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5344,"context_line":"        disk_infos \u003d jsonutils.loads(disk_info_text)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_c47e0218","line":5341,"range":{"start_line":5340,"start_character":0,"end_line":5341,"end_character":60},"in_reply_to":"dada55a8_adcfc554","updated":"2016-07-27 13:06:50.000000000","message":"ok thanks for the explanation, that means to me that the method we have at the moment is not completely correct as it misses a case. I still think it would be good to have the logic in a single place and not having a different logic somewhere in the code base.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"c79bd637af832c71d3f5211717f197e431eb9285","unresolved":false,"context_lines":[{"line_number":5342,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5343,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5344,"context_line":"        disk_infos \u003d jsonutils.loads(disk_info_text)"},{"line_number":5345,"context_line":"        # This is wrong for rbd, since get_instance_dis_info returns"},{"line_number":5346,"context_line":"        # no rbd devices. But it does not matter, because has_local_disk"},{"line_number":5347,"context_line":"        # is checked after is_shared_block_storage flag."},{"line_number":5348,"context_line":"        has_local_disk \u003d len(disk_infos) \u003e int(not booted_from_volume)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_c2778639","line":5345,"range":{"start_line":5345,"start_character":7,"end_line":5345,"end_character":31},"updated":"2016-07-11 14:33:24.000000000","message":"sorry what is wrong? Can you make the comment more clear or can we avoid to the wrong thing if it\u0027s rbd?\n\nminor: typo in the comment s/get_instance_dis_info/get_instance_disk_info","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"79b261807e9d42644e75b14d6a85ad4d3dac0e4b","unresolved":false,"context_lines":[{"line_number":5342,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5343,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5344,"context_line":"        disk_infos \u003d jsonutils.loads(disk_info_text)"},{"line_number":5345,"context_line":"        # This is wrong for rbd, since get_instance_dis_info returns"},{"line_number":5346,"context_line":"        # no rbd devices. But it does not matter, because has_local_disk"},{"line_number":5347,"context_line":"        # is checked after is_shared_block_storage flag."},{"line_number":5348,"context_line":"        has_local_disk \u003d len(disk_infos) \u003e int(not booted_from_volume)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_cd55d950","line":5345,"range":{"start_line":5345,"start_character":7,"end_line":5345,"end_character":31},"in_reply_to":"1aa78d24_c2778639","updated":"2016-07-26 05:14:48.000000000","message":"We try to understand if the instance has local disks (managed by current ephemeral driver). To understand this we analyze list of devices, returned by get_instance_disk_info. This method returns block and file devices only. But rbd devices are network devices, so this method returns an empty list always.\n\nIn the last patch of the series (Ic2ca8aa42ce67a95bcdae946289179f98e6103e8) i move this comment and change it.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"c79bd637af832c71d3f5211717f197e431eb9285","unresolved":false,"context_lines":[{"line_number":5345,"context_line":"        # This is wrong for rbd, since get_instance_dis_info returns"},{"line_number":5346,"context_line":"        # no rbd devices. But it does not matter, because has_local_disk"},{"line_number":5347,"context_line":"        # is checked after is_shared_block_storage flag."},{"line_number":5348,"context_line":"        has_local_disk \u003d len(disk_infos) \u003e int(not booted_from_volume)"},{"line_number":5349,"context_line":""},{"line_number":5350,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"},{"line_number":5351,"context_line":"            dest_check_data.block_migration \u003d ("}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_82ea9eaa","line":5348,"range":{"start_line":5348,"start_character":8,"end_line":5348,"end_character":70},"updated":"2016-07-11 14:33:24.000000000","message":"Just a personal thing: I struggle to read that code, can you make it more readable?","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"79b261807e9d42644e75b14d6a85ad4d3dac0e4b","unresolved":false,"context_lines":[{"line_number":5345,"context_line":"        # This is wrong for rbd, since get_instance_dis_info returns"},{"line_number":5346,"context_line":"        # no rbd devices. But it does not matter, because has_local_disk"},{"line_number":5347,"context_line":"        # is checked after is_shared_block_storage flag."},{"line_number":5348,"context_line":"        has_local_disk \u003d len(disk_infos) \u003e int(not booted_from_volume)"},{"line_number":5349,"context_line":""},{"line_number":5350,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"},{"line_number":5351,"context_line":"            dest_check_data.block_migration \u003d ("}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_2dcd9531","line":5348,"range":{"start_line":5348,"start_character":8,"end_line":5348,"end_character":70},"in_reply_to":"1aa78d24_82ea9eaa","updated":"2016-07-26 05:14:48.000000000","message":"I\u0027ll think how to do this. Anyway this code is deleted in the last patch of the series (Ic2ca8aa42ce67a95bcdae946289179f98e6103e8)","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"23b958dae2f2e11575348e513d364f1f1836081a","unresolved":false,"context_lines":[{"line_number":5345,"context_line":"        # This is wrong for rbd, since get_instance_dis_info returns"},{"line_number":5346,"context_line":"        # no rbd devices. But it does not matter, because has_local_disk"},{"line_number":5347,"context_line":"        # is checked after is_shared_block_storage flag."},{"line_number":5348,"context_line":"        has_local_disk \u003d len(disk_infos) \u003e int(not booted_from_volume)"},{"line_number":5349,"context_line":""},{"line_number":5350,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"},{"line_number":5351,"context_line":"            dest_check_data.block_migration \u003d ("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_6d4d51c4","line":5348,"range":{"start_line":5348,"start_character":8,"end_line":5348,"end_character":70},"in_reply_to":"dada55a8_2dcd9531","updated":"2016-08-12 10:01:03.000000000","message":"So, we\u0027re saying:\n\nIf the number of disks with local paths, as taken from the libvirt XML, is greater than 0 if booted from volume, or 1 otherwise, then we have local disks?\n\nI\u0027m prepared to believe that might be correct, but my feeble brain can\u0027t parse it. Could you please write this completely differently?\n\nAlso, as you\u0027re touching it anyway, could you please move it down into the else path where it\u0027s used? That\u0027ll also make the context you allude to in your comment a bit more evident.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":1865,"name":"Andrea Rosa","email":"andrea.rosa@gmail.com","username":"andrea-rosa-m"},"change_message_id":"c79bd637af832c71d3f5211717f197e431eb9285","unresolved":false,"context_lines":[{"line_number":6952,"context_line":"            disk_dev \u003d vol[\u0027mount_device\u0027].rpartition(\"/\")[2]"},{"line_number":6953,"context_line":"            self._disconnect_volume(connection_info, disk_dev)"},{"line_number":6954,"context_line":""},{"line_number":6955,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":6956,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":6957,"context_line":"        disk_info \u003d jsonutils.loads(disk_info_text)"},{"line_number":6958,"context_line":""},{"line_number":6959,"context_line":"        try:"},{"line_number":6960,"context_line":"            utils.execute(\u0027mv\u0027, inst_base, inst_base_resize)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1aa78d24_c284a6b2","line":6957,"range":{"start_line":6955,"start_character":0,"end_line":6957,"end_character":51},"updated":"2016-07-11 14:33:24.000000000","message":"minor: this is the second time we have the same code, to me that\u0027s enough to have a separate small private method.","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"79b261807e9d42644e75b14d6a85ad4d3dac0e4b","unresolved":false,"context_lines":[{"line_number":6952,"context_line":"            disk_dev \u003d vol[\u0027mount_device\u0027].rpartition(\"/\")[2]"},{"line_number":6953,"context_line":"            self._disconnect_volume(connection_info, disk_dev)"},{"line_number":6954,"context_line":""},{"line_number":6955,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":6956,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":6957,"context_line":"        disk_info \u003d jsonutils.loads(disk_info_text)"},{"line_number":6958,"context_line":""},{"line_number":6959,"context_line":"        try:"},{"line_number":6960,"context_line":"            utils.execute(\u0027mv\u0027, inst_base, inst_base_resize)"}],"source_content_type":"text/x-python","patch_set":2,"id":"dada55a8_add985f5","line":6957,"range":{"start_line":6955,"start_character":0,"end_line":6957,"end_character":51},"in_reply_to":"1aa78d24_c284a6b2","updated":"2016-07-26 05:14:48.000000000","message":"See the next patch in the series (I9c0c6954ba5442414e615225a74df7e42f3d0c98)","commit_id":"ef9f396e7dc1c985e8422ee179c809dfc6478a30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a8cab2824ed36f82a06370c7a8d3ff5f0f01ce4e","unresolved":false,"context_lines":[{"line_number":7040,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":7041,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":7042,"context_line":"        booted_from_volume \u003d self._is_booted_from_volume(instance,"},{"line_number":7043,"context_line":"                                                         disk_info_text)"},{"line_number":7044,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"},{"line_number":7045,"context_line":"            reason \u003d _(\"Unable to resize disk down.\")"},{"line_number":7046,"context_line":"            raise exception.InstanceFaultRollback("}],"source_content_type":"text/x-python","patch_set":5,"id":"9a89bdaa_fcf42488","side":"PARENT","line":7043,"updated":"2016-09-06 13:58:36.000000000","message":"This is kinda interesting. _is_booted_from_volume does:\n\n        return ((not bool(instance.get(\u0027image_ref\u0027)))\n                or \u0027disk\u0027 not in disk_mapping)\n\nThat check for image_ref should be redundant, but I\u0027ll bet it\u0027s there because somebody didn\u0027t understand this usage. \u0027disk_info_text\u0027 is basically always going to have \u0027disk\u0027 as a substring, so the second part of that will never be true, meaning the only differentiation is the first part: does instance have an image_ref. As you note, this isn\u0027t always correct.","commit_id":"ac9959b3cdffce6bd8df791286f7c6accd03d062"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"a8cab2824ed36f82a06370c7a8d3ff5f0f01ce4e","unresolved":false,"context_lines":[{"line_number":7040,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":7041,"context_line":"            block_device_info)"},{"line_number":7042,"context_line":"        booted_from_volume \u003d bool("},{"line_number":7043,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":7044,"context_line":""},{"line_number":7045,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"},{"line_number":7046,"context_line":"            reason \u003d _(\"Unable to resize disk down.\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a89bdaa_8d9689ee","line":7043,"updated":"2016-09-06 13:58:36.000000000","message":"So we\u0027re saying that we\u0027re booted from volume if there\u0027s a root disk in the list of volumes. Yeah, that works.\n\n_is_booted_from_volume requires disk_mapping, which we don\u0027t construct in this code path and isn\u0027t worth it just for this, and the one in compute_utils goes back to the db, which isn\u0027t necessary here.\n\nIt will be nice when we consolidate these, though!","commit_id":"b36f90572e606b3ee2ebe34d934842b7596e5dad"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"48da859ff76a78b5c5b93157bca0fb8ad5daf93f","unresolved":false,"context_lines":[{"line_number":5434,"context_line":""},{"line_number":5435,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5436,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5437,"context_line":"        booted_from_volume \u003d self._is_booted_from_volume(instance,"},{"line_number":5438,"context_line":"                                                         disk_info_text)"},{"line_number":5439,"context_line":"        has_local_disk \u003d self._has_local_disk(instance, disk_info_text)"},{"line_number":5440,"context_line":""},{"line_number":5441,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a98d1f4_7a4f7592","line":5438,"range":{"start_line":5437,"start_character":8,"end_line":5438,"end_character":72},"updated":"2016-09-29 00:59:54.000000000","message":"This is another instance of the disk_info_text being passed to _is_booted_from_volume. But in this case, block_device_info\u003dNone because nothing is passed from nova/compute/manager.py. I don\u0027t think we can rely on the block_device_info parameter, especially since it defaults to None in the function signatures. And if we\u0027re going to need to query for block device mappings anyway because we can\u0027t rely on block_device_info, then the other approach [1] seems less error prone to me.\n\n[1] https://review.openstack.org/#/c/325708","commit_id":"282dc029f9ca0fbeab74b562b5453b5fe460697b"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"ae4710018e659d1ca3fa1b42e4051e47d7587780","unresolved":false,"context_lines":[{"line_number":5434,"context_line":""},{"line_number":5435,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5436,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5437,"context_line":"        booted_from_volume \u003d self._is_booted_from_volume(instance,"},{"line_number":5438,"context_line":"                                                         disk_info_text)"},{"line_number":5439,"context_line":"        has_local_disk \u003d self._has_local_disk(instance, disk_info_text)"},{"line_number":5440,"context_line":""},{"line_number":5441,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a98d1f4_95ffa800","line":5438,"range":{"start_line":5437,"start_character":8,"end_line":5438,"end_character":72},"in_reply_to":"3a98d1f4_7a4f7592","updated":"2016-09-29 07:01:29.000000000","message":"@Melanie, this code is fixed by the next patch of the series. Previously it was fixed here, but some reviewers did not like the result. See PS2.","commit_id":"282dc029f9ca0fbeab74b562b5453b5fe460697b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"17bd6d4407858435de7fcf1b7062bf897158d08d","unresolved":false,"context_lines":[{"line_number":5434,"context_line":""},{"line_number":5435,"context_line":"        disk_info_text \u003d self.get_instance_disk_info("},{"line_number":5436,"context_line":"            instance, block_device_info\u003dblock_device_info)"},{"line_number":5437,"context_line":"        booted_from_volume \u003d self._is_booted_from_volume(instance,"},{"line_number":5438,"context_line":"                                                         disk_info_text)"},{"line_number":5439,"context_line":"        has_local_disk \u003d self._has_local_disk(instance, disk_info_text)"},{"line_number":5440,"context_line":""},{"line_number":5441,"context_line":"        if \u0027block_migration\u0027 not in dest_check_data:"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a98d1f4_c3c79310","line":5438,"range":{"start_line":5437,"start_character":8,"end_line":5438,"end_character":72},"in_reply_to":"3a98d1f4_95ffa800","updated":"2016-09-29 18:25:28.000000000","message":"Thanks. That wasn\u0027t clear to me from the previous comments in this review, but after looking at the next patch, I see your point.\n\nOkay, so nova/compute/manager.py calls compute_utils.is_volume_backed_instance and passes that information down to this method via dest_check_data.","commit_id":"282dc029f9ca0fbeab74b562b5453b5fe460697b"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"45f06999a91524877671c56c12ee3e84d1e60810","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_dcedef8f","line":6991,"updated":"2016-10-11 08:26:32.000000000","message":"I might I missed why you did not do that on the review comments but I\u0027m not really pro of this change because we have a method (broken) responsible to return whether the instance is booted from volume we should just to fix it.\n\nI would suggest to update \u0027_is_booted_from_volume\u0027 to make accept only as an argument: \u0027block_device_info\u0027 so we can use your code:\n\n  def _is_booted_from_volume(self, block_device_info): \n    bdm \u003d driver.block_device_info_get_mapping(block_device_info)\n    return bool(bdm...)\n\nAnd update the 3 or 4 places on the driver where that method is used.","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"fc3968add4d7e6f5baa3f8574c7891bc5bc53d04","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_8d4f7cbc","line":6991,"in_reply_to":"fa6399be_1c1c9576","updated":"2016-10-12 14:28:40.000000000","message":"@sahid, ok, i\u0027ll reorder this series, when https://review.openstack.org/#/c/346263/ is merged. Otherwise it does not make sense.\n\nAs for a win - i have never won in this \u0027Nova\u0027 casino :/","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"d94979eeff6104f9f0542b0400687fc18d556891","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_6fe26005","line":6991,"in_reply_to":"fa6399be_37b524cd","updated":"2016-10-11 09:21:43.000000000","message":"Yes I just noticed that. There is not rush of fixing that issue correctly so I would recommend to move the change here.","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"938061f45235cbc941022c68c16591c58e2453dd","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_b2d65425","line":6991,"in_reply_to":"fa6399be_5952adde","updated":"2016-10-19 13:39:35.000000000","message":"@sahid, do you mean that https://review.openstack.org/#/c/346263/ makes no sense alone? Why? It is a separate patch, which fixes a custom bug, which one is not related to bugs touched in this series.\n\nAnd your backport concert is not clear for me. Does someone ask to backport all of this? If even yes, why whould not a backported just backport all these pathes in a proper order?\n\nWhy would not just push https://review.openstack.org/#/c/346263/ and then let me work on this series in confindence that it is needed work and it is going in the right direction?","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":10224,"name":"Feodor Tersin","email":"ftersin@hotmail.com","username":"ftersin"},"change_message_id":"034a3ca30fab5ef340d424bf3df1cd147ac73bff","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_79ddca6f","line":6991,"in_reply_to":"fa6399be_6fe26005","updated":"2016-10-11 11:36:09.000000000","message":"@sahid, currently the method works well, it\u0027s not broken, there are just wrong invokes of it in some places.\n\nThere are some obstacles to change the method interface before other changes:\n\nit prevents https://review.openstack.org/#/c/337056/ to look like revertion, because this method is called from the code reverted there (i could place that revertion at the first place of the series, though)\n\nit requires additional change (https://review.openstack.org/#/c/346263/, i mentioned it in the root level comment on https://review.openstack.org/#/c/382024/), which intersects with Matthew Booth\u0027s massive work (partial refactoring of image backend usage). As you could see there, Matthew does not agree to have that change at the moment, he prefers to merge his work instead, but the last one is yet not ready as i understand, and i have no idea when it would be merged (you should know, he has started at Jan\u002716)\n\nSince i\u0027d like to see this bug fixed independedly of other useful but slow going changes, i choosed this order of patches. Is it really critical for you to improve _is_booted_from_volume at the first place?","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"3512f63c33af38a217eb7e2d985c95f85829096b","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_1c1c9576","line":6991,"in_reply_to":"fa6399be_79ddca6f","updated":"2016-10-12 07:40:55.000000000","message":"Well I would say it\u0027s little bit more than an improvement, to fix that issue you have to pass on using that method which has been implemented especially for that purpose. So that looks like a hack.\n\nI don\u0027t see any problem with https://review.openstack.org/#/c/337056/ and I guess Matt can understand that we are not going to stop working on the driver until all of its changes are merged.\n\nI don\u0027t think you are going to win this time ;)","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"230d2677493a137992b12fd0fbfb1ff38e326cc0","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_5952adde","line":6991,"in_reply_to":"fa6399be_8d4f7cbc","updated":"2016-10-18 15:50:57.000000000","message":"Feodor I was on a point where I wanted to ask you to add a comment and remove my -1 to help on this merged and backported.\n\nBut really and as you mentioned, it makes a lot of sense to see your patch and [1] backported as a serie in a same time.\n\n[1] https://review.openstack.org/#/c/346263/","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"648d539f659be41824f7935adbd846d8c478e783","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_59e19899","line":6991,"in_reply_to":"fa6399be_b2d65425","updated":"2016-10-19 16:05:46.000000000","message":"\u003e  do you mean that https://review.openstack.org/#/c/346263/ makes no sense alone? Why? It is a separate patch, which fixes a custom bug, which one is not related to bugs touched in this series.\n\nI understand that.\n\n\u003e And your backport concert is not clear for me. Does someone ask to backport all of this? If even yes\n\nBoth patches are fixing issues, they are relatively small I think they can be backported yes. \n\n\u003e  why whould not a backported just backport all these pathes in a proper order?\n\nNo, we don\u0027t want to backport a large serie which is mostly re-factoring.\n\n\u003e Why would not just push https://review.openstack.org/#/c/346263/ and then let me work on this series in confindence that it is needed work and it is going in the right direction?\n\nNot sure to understand, I think you can continue to work on your serie. You just have to replace that patch with [1] and rebase your work on top of [2]\n\n  [1] https://review.openstack.org/#/c/382024\n  [2] https://review.openstack.org/#/c/346263","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"e958ea4d1881a235f1d3d758f63c657180f87e4b","unresolved":false,"context_lines":[{"line_number":6988,"context_line":"        ephemeral_down \u003d flavor.ephemeral_gb \u003c eph_size"},{"line_number":6989,"context_line":"        block_device_mapping \u003d driver.block_device_info_get_mapping("},{"line_number":6990,"context_line":"            block_device_info)"},{"line_number":6991,"context_line":"        booted_from_volume \u003d bool("},{"line_number":6992,"context_line":"            block_device.get_root_bdm(block_device_mapping))"},{"line_number":6993,"context_line":""},{"line_number":6994,"context_line":"        if (root_down and not booted_from_volume) or ephemeral_down:"}],"source_content_type":"text/x-python","patch_set":7,"id":"fa6399be_37b524cd","line":6991,"in_reply_to":"fa6399be_dcedef8f","updated":"2016-10-11 08:36:02.000000000","message":"So this is updated later in this chain. This fixes a bug in a small and simple change, please take a look on a change that refactors (improves) _is_booted_from_volume https://review.openstack.org/#/c/382024 exactly in the way you proposed here","commit_id":"9a0c31480f1e26feb6c5825dbaa43ca5a2fa6305"}]}