)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dbe190dd00ff436fa8440f3b7bb49b2f6895408c","unresolved":false,"context_lines":[{"line_number":4449,"context_line":"        new host machine."},{"line_number":4450,"context_line":""},{"line_number":4451,"context_line":"        \"\"\""},{"line_number":4452,"context_line":"        try:"},{"line_number":4453,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_10f15b01","line":4452,"updated":"2018-02-14 14:09:37.000000000","message":"This would look cleaner to me if you did the try/except in a new private _finish_resize method similar to what you did with resize_instance.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"6a5d906477110211b1cad21d63eac9ed4e975648","unresolved":false,"context_lines":[{"line_number":4449,"context_line":"        new host machine."},{"line_number":4450,"context_line":""},{"line_number":4451,"context_line":"        \"\"\""},{"line_number":4452,"context_line":"        try:"},{"line_number":4453,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_0e584254","line":4452,"in_reply_to":"3fa0c359_10f15b01","updated":"2018-02-15 14:46:16.000000000","message":"I agree, but there\u0027s already a _finish_resize method, and I didn\u0027t know what to call the \"new\" method, so it wouldn\u0027t get confusing.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"ff87c135767e451916030be91932e805ead484e2","unresolved":false,"context_lines":[{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""},{"line_number":4456,"context_line":"            with self._error_out_instance_on_exception(context, instance), \\"},{"line_number":4457,"context_line":"                 errors_out_migration_ctxt(migration):"},{"line_number":4458,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"},{"line_number":4459,"context_line":"                network_info \u003d self._finish_resize("},{"line_number":4460,"context_line":"                    context, instance, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_64d5891b","line":4457,"range":{"start_line":4457,"start_character":17,"end_line":4457,"end_character":42},"updated":"2018-02-14 11:35:36.000000000","message":"Would it make sense to move the allocation handling into this context manager? It think it would help handling every (including future) cases where migration is set to error without checking if there is any allocation due to that migration.\n\nBTW this context manager is used not just in resize_instance and finish_resize handled in this patch but also in finish_revert_resize and revert_resize as well as a decorator. Do we need to handle allocation in those cases too?","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"2a396b8a5f775c07f57edfe23e55c0098e8d1032","unresolved":false,"context_lines":[{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""},{"line_number":4456,"context_line":"            with self._error_out_instance_on_exception(context, instance), \\"},{"line_number":4457,"context_line":"                 errors_out_migration_ctxt(migration):"},{"line_number":4458,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"},{"line_number":4459,"context_line":"                network_info \u003d self._finish_resize("},{"line_number":4460,"context_line":"                    context, instance, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_a6ffee32","line":4457,"range":{"start_line":4457,"start_character":17,"end_line":4457,"end_character":42},"in_reply_to":"3fa0c359_2b298c35","updated":"2018-02-15 15:38:24.000000000","message":"Hm, as far as I can tell from the code, and from functional tests, this is what happens on revert_resize:\n\nAfter a resize finished (and before confirm / revert), there\u0027s only 1 set allocations.\n\nDestination node: The instance is destroyed, after which the allocation is deleted, which is Ok. Even if there is something  breaking between destroying the instance and deleting the allocations: L3877 and L3900 [1], there\u0027s still one set of allocations, and finish_revert_resize is not called, which would have created another set of allocations.\n\nSource node: The allocations are reverted (L3940), after which the instance is recreated (L3973). Even if the recreation failed, there\u0027s still only one set of allocations.\n\n[1] https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L3877","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"a33bf38b1da904a1dada40dbe53878b6234a28f8","unresolved":false,"context_lines":[{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""},{"line_number":4456,"context_line":"            with self._error_out_instance_on_exception(context, instance), \\"},{"line_number":4457,"context_line":"                 errors_out_migration_ctxt(migration):"},{"line_number":4458,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"},{"line_number":4459,"context_line":"                network_info \u003d self._finish_resize("},{"line_number":4460,"context_line":"                    context, instance, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_2b298c35","line":4457,"range":{"start_line":4457,"start_character":17,"end_line":4457,"end_character":42},"in_reply_to":"3fa0c359_30cd77b6","updated":"2018-02-14 14:57:33.000000000","message":"OK, let\u0027s not overcomplicate this bug fix.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dbe190dd00ff436fa8440f3b7bb49b2f6895408c","unresolved":false,"context_lines":[{"line_number":4454,"context_line":"                context, instance.uuid)"},{"line_number":4455,"context_line":""},{"line_number":4456,"context_line":"            with self._error_out_instance_on_exception(context, instance), \\"},{"line_number":4457,"context_line":"                 errors_out_migration_ctxt(migration):"},{"line_number":4458,"context_line":"                image_meta \u003d objects.ImageMeta.from_dict(image)"},{"line_number":4459,"context_line":"                network_info \u003d self._finish_resize("},{"line_number":4460,"context_line":"                    context, instance, migration,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_30cd77b6","line":4457,"range":{"start_line":4457,"start_character":17,"end_line":4457,"end_character":42},"in_reply_to":"3fa0c359_64d5891b","updated":"2018-02-14 14:09:37.000000000","message":"I was thinking the same yesterday, that errors_out_migration_ctxt is used in several places and might be a nice way to handle reverting the allocation; however, I would have to audit all of those places to make sure that reverting the allocation is really what we\u0027d want to do, like I don\u0027t think it would be appropriate for confirm_resize probably where the instance is running on the dest host.\n\nSo I\u0027m a bit torn between the whack-a-mole issue with the various places that resize could fail, and just wanting to fix this specific bug that we know is an issue. We could handle other places separately later, presumably when we have functional tests showing they are a problem.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e2f6fdd91ceb973382b33b3422ce0b320a44017","unresolved":false,"context_lines":[{"line_number":4456,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4457,"context_line":"                self._revert_allocation(context, instance, migration)"},{"line_number":4458,"context_line":""},{"line_number":4459,"context_line":"    def _finish_resize_helper(self, context, disk_info, image, instance,"},{"line_number":4460,"context_line":"                              migration):"},{"line_number":4461,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4462,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa0c359_02707507","line":4459,"updated":"2018-02-22 17:02:52.000000000","message":"nit: a short docstring explaining what this method is for would be nice (what is it helping us with?).","commit_id":"e70992adbdee222ea4ab9b69c37fba5e0e325b2d"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"dfaf54c6886eef7fec2a0e46a7636b8f17ea8293","unresolved":false,"context_lines":[{"line_number":4456,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4457,"context_line":"                self._revert_allocation(context, instance, migration)"},{"line_number":4458,"context_line":""},{"line_number":4459,"context_line":"    def _finish_resize_helper(self, context, disk_info, image, instance,"},{"line_number":4460,"context_line":"                              migration):"},{"line_number":4461,"context_line":"        bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":4462,"context_line":"            context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3fa0c359_1df9b6c6","line":4459,"in_reply_to":"3fa0c359_02707507","updated":"2018-02-22 17:21:13.000000000","message":"Done","commit_id":"e70992adbdee222ea4ab9b69c37fba5e0e325b2d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"72419e54a629b608c7007ad5014067b2b8af1c14","unresolved":false,"context_lines":[{"line_number":4452,"context_line":"        try:"},{"line_number":4453,"context_line":"            self._finish_resize_helper(context, disk_info, image, instance,"},{"line_number":4454,"context_line":"                                       migration)"},{"line_number":4455,"context_line":"        except Exception:"},{"line_number":4456,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":4457,"context_line":"                self._revert_allocation(context, instance, migration)"},{"line_number":4458,"context_line":""},{"line_number":4459,"context_line":"    def _finish_resize_helper(self, context, disk_info, image, instance,"},{"line_number":4460,"context_line":"                              migration):"}],"source_content_type":"text/x-python","patch_set":6,"id":"5fc1f717_97b5b934","line":4457,"range":{"start_line":4455,"start_character":8,"end_line":4457,"end_character":69},"updated":"2019-03-25 14:05:46.000000000","message":"I\u0027m not sure this is actually something we should have done. Assuming the resize_instance method on the source host completes successfully, it has changed the instance.host/node values to the dest host/node. Then if something fails in finish_resize (on the dest), we\u0027ll revert the allocations (remove the allocations for the dest host) but not reset the instance host/node values to the source, so we\u0027ve effectively said the instance is on the dest (via the nova db) but removed the allocations for the dest host, so the allocations are only pointing at the source host, which would be wrong if the user hard reboots or rebuilds the instance on the dest host to get out of the ERROR status on the instance.","commit_id":"caf167862dd82e98f0189c9598856de57dfa7d35"}],"nova/tests/functional/test_servers.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"61104885c86ddad8494fd74d53fc49f5674926e3","unresolved":false,"context_lines":[{"line_number":2029,"context_line":"        self.assertEqual(1, len(allocations))"},{"line_number":2030,"context_line":"        allocation \u003d allocations[source_rp_uuid][\u0027resources\u0027]"},{"line_number":2031,"context_line":"        self.assertFlavorMatchesAllocation(self.flavor1, allocation)"},{"line_number":2032,"context_line":""},{"line_number":2033,"context_line":"        self._delete_and_check_allocations(server)"},{"line_number":2034,"context_line":""},{"line_number":2035,"context_line":"    def test_evacuate(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_503923a1","side":"PARENT","line":2032,"updated":"2018-02-14 14:11:15.000000000","message":"nit: unrelated","commit_id":"fba4161f71e47e4155e7f9a1f0d3a41b8107cef5"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"3b626c2d824476e54546dac7234eda4f9d6fa5dc","unresolved":false,"context_lines":[{"line_number":2029,"context_line":"        self.assertEqual(1, len(allocations))"},{"line_number":2030,"context_line":"        allocation \u003d allocations[source_rp_uuid][\u0027resources\u0027]"},{"line_number":2031,"context_line":"        self.assertFlavorMatchesAllocation(self.flavor1, allocation)"},{"line_number":2032,"context_line":""},{"line_number":2033,"context_line":"        self._delete_and_check_allocations(server)"},{"line_number":2034,"context_line":""},{"line_number":2035,"context_line":"    def test_evacuate(self):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_203506f8","side":"PARENT","line":2032,"in_reply_to":"3fa0c359_503923a1","updated":"2018-02-15 14:51:11.000000000","message":"Done","commit_id":"fba4161f71e47e4155e7f9a1f0d3a41b8107cef5"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"a33bf38b1da904a1dada40dbe53878b6234a28f8","unresolved":false,"context_lines":[{"line_number":2776,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2777,"context_line":"            \u0027_prep_resize\u0027, \u0027compute_prep_resize\u0027)"},{"line_number":2778,"context_line":""},{"line_number":2779,"context_line":"    def test_resize_instance_fails_allocation_cleanup(self):"},{"line_number":2780,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2781,"context_line":"            \u0027_resize_instance\u0027, \u0027compute_resize_instance\u0027)"},{"line_number":2782,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_cb96e84c","line":2779,"updated":"2018-02-14 14:57:33.000000000","message":"Something does not adds up here to me, this test does not fail due to duplicated allocation when I remove the bugfix locally but it fails as the _wait_for_action_fail_completion() call times out.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"3b626c2d824476e54546dac7234eda4f9d6fa5dc","unresolved":false,"context_lines":[{"line_number":2776,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2777,"context_line":"            \u0027_prep_resize\u0027, \u0027compute_prep_resize\u0027)"},{"line_number":2778,"context_line":""},{"line_number":2779,"context_line":"    def test_resize_instance_fails_allocation_cleanup(self):"},{"line_number":2780,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2781,"context_line":"            \u0027_resize_instance\u0027, \u0027compute_resize_instance\u0027)"},{"line_number":2782,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_60030e11","line":2779,"in_reply_to":"3fa0c359_cb96e84c","updated":"2018-02-15 14:51:11.000000000","message":"are you sure? if I comment the cleanup bit in resize_instance (and adding pass, ofc), this test then fails:\n\n        Traceback (most recent call last):\n      File \"nova/tests/functional/test_servers.py\", line 2781, in test_resize_instance_fails_allocation_cleanup\n        \u0027_resize_instance\u0027, \u0027compute_resize_instance\u0027)\n      File \"nova/tests/functional/test_servers.py\", line 2773, in _test_resize_to_same_host_instance_fails\n        self.assertFlavorMatchesAllocation(self.flavor1, source_usages)\n      File \"nova/tests/functional/test_servers.py\", line 1462, in assertFlavorMatchesAllocation\n        self.assertEqual(flavor[\u0027vcpus\u0027], allocation[\u0027VCPU\u0027])\n      File \"/usr/local/lib/python2.7/dist-packages/testtools/testcase.py\", line 411, in assertEqual\n        self.assertThat(observed, matcher, message)\n      File \"/usr/local/lib/python2.7/dist-packages/testtools/testcase.py\", line 498, in assertThat\n        raise mismatch_error\n    testtools.matchers._impl.MismatchError: 1 !\u003d 2","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"a33bf38b1da904a1dada40dbe53878b6234a28f8","unresolved":false,"context_lines":[{"line_number":2780,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2781,"context_line":"            \u0027_resize_instance\u0027, \u0027compute_resize_instance\u0027)"},{"line_number":2782,"context_line":""},{"line_number":2783,"context_line":"    def test_finish_resize_fails_allocation_cleanup(self):"},{"line_number":2784,"context_line":"        self._test_resize_to_same_host_instance_fails("},{"line_number":2785,"context_line":"            \u0027_finish_resize\u0027, \u0027compute_finish_resize\u0027)"},{"line_number":2786,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3fa0c359_4b8c1835","line":2783,"updated":"2018-02-14 14:57:33.000000000","message":"OK, this fails due to duplicated allocation without the bugfix.","commit_id":"02b62f310c2c3d7ae966a27f177528013497ba70"}]}