)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 15:01:58 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"df3967d1_14261ab9","line":7,"updated":"2017-07-28 17:51:10.000000000","message":"This summary is no longer accurate","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 15:01:58 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"df3967d1_f4d956f4","line":7,"in_reply_to":"df3967d1_14261ab9","updated":"2017-07-28 18:26:41.000000000","message":"No, it\u0027s correct. It is the rebalancing, which can move nodes among compute services, which introduces the bug. The fact that the code to call the migration isn\u0027t in the _refresh_hash_ring() method is irrelevant; it runs whenever new nodes are added to a compute service, which only happens on a hash ring rebalance.","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"f598e69fce6a09f1a3cfadf94a522944c16412c1","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 19:11:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df3967d1_850e44e3","line":7,"updated":"2017-07-29 11:56:25.000000000","message":"For the sake of conversation: I\u0027m still confused by this summary. You say hash ring here, and make the handling of it the subject, but in the paragraph below never mention the hash ring. So it\u0027s confusing. Isn\u0027t what your doing:\n\n    Move ironic flavor migration to cache refresh\n\nor something like that?\n\nIn any case the fact that I can\u0027t extract that info is some kind of sign?","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 19:11:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df3967d1_acb8cd29","line":7,"in_reply_to":"df3967d1_704c1cc7","updated":"2017-08-01 17:24:25.000000000","message":"OK, I was under the impression that adding nodes would cause a hash ring rebalance - guess not. And I really, really hate making cdent happy.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dea7618d5720a303be226f4cae34abfbfbf0643d","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 19:11:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df3967d1_704c1cc7","line":7,"in_reply_to":"df3967d1_850e44e3","updated":"2017-07-31 21:22:32.000000000","message":"+1. the code used to be in the hash ring rebalancing, but it turns out, it was really being done when the cache was refreshed w diff nodes. I\u0027m fine if the commit msg below clarifies it or if this is changed. We probably want to make cdent happy if it is easy to do :)","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ca5fbf8bdafbc3e1e6cfb7acef8609cf5e3e1c8c","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 19:11:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df3967d1_eca2559a","line":7,"in_reply_to":"df3967d1_acb8cd29","updated":"2017-08-01 17:29:12.000000000","message":"I\u0027m confused. Rebalancing the hash ring will happen when you add new nodes, as each compute will decide it owns a different set of nodes (like, my_nodes \u003d number_of_nodes % number_of_computes) kind of thing.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"72b389d2176b74cd048ec4637cb827e822e8dc65","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-07-28 19:11:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle hash ring rebalancing in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"df3967d1_2c005d67","line":7,"in_reply_to":"df3967d1_eca2559a","updated":"2017-08-01 17:31:59.000000000","message":"@Dan - Well, if it confused Ruby, I\u0027d tend to re-word it. My understanding was the same as yours.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e415092d795b6490359677cb645160cf33936151","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-08-01 21:31:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle addition of new nodes/instances in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df3967d1_ca764c07","line":7,"updated":"2017-08-01 21:41:17.000000000","message":"I am _so_ happy","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"72f49b7d680054f43819fdb8885af9ef3c564ef3","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-08-01 21:31:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle addition of new nodes/instances in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df3967d1_617767d6","line":7,"in_reply_to":"df3967d1_b214874f","updated":"2017-08-02 18:03:07.000000000","message":"...and I haz a sad.  :(","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e64166af7300a0c45932fe32ea5afd68b147691a","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     EdLeafe \u003ced@leafe.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-08-01 21:31:54 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Handle addition of new nodes/instances in ironic flavor migration"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The original idea for ironic flavor migration was to handle it in"},{"line_number":10,"context_line":"init_host(), so that the code wouldn\u0027t run often. But in a situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":6,"id":"df3967d1_b214874f","line":7,"in_reply_to":"df3967d1_ca764c07","updated":"2017-08-02 17:01:25.000000000","message":"I am happy that Chris is _so_ happy","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"c4b73d7d154f518dbb27309e42d6a944ac60396e","unresolved":false,"context_lines":[{"line_number":13,"context_line":"cache is refreshed, and new nodes for the compute service that have"},{"line_number":14,"context_line":"instances are detected. We also need to cover the cases where an"},{"line_number":15,"context_line":"existing node has a new instance created on it to ensure that that new"},{"line_number":16,"context_line":"instance\u0027s flavor is also migrated."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #1707021"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"bf3e73b7_cb4466ba","line":16,"updated":"2017-08-06 02:36:27.000000000","message":"I\u0027m a little confuse on the case we fix. We only migrate the node which already run an instance. But from the comment https://github.com/openstack/nova/blob/master/nova/virt/ironic/driver.py#L675, The node which already run an instance will always be managed by the same nova-compute?","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"dfee3dfc8eae1df79703ed058516955f937047b3","unresolved":false,"context_lines":[{"line_number":13,"context_line":"cache is refreshed, and new nodes for the compute service that have"},{"line_number":14,"context_line":"instances are detected. We also need to cover the cases where an"},{"line_number":15,"context_line":"existing node has a new instance created on it to ensure that that new"},{"line_number":16,"context_line":"instance\u0027s flavor is also migrated."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #1707021"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"bf3e73b7_8eff7a66","line":16,"in_reply_to":"bf3e73b7_cb4466ba","updated":"2017-08-07 11:56:13.000000000","message":"The prior version of the code would keep track of the nodes that a compute manages that had been looked at for migration. If a node did not have an instance on it, there would be nothing to do, but it would have been marked as migrated. If an instance is then built on that node, its flavor would not get migrated, since its node was already cached as being \"done\". Similarly, a node with an instance could have had its instance\u0027s flavor migrated, then that instance is destroyed and a new one built. We need to catch those cases too.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"68d737d81f741859d49d10908a14e26afc75cdf2","unresolved":false,"context_lines":[{"line_number":11,"context_line":"like an upgrade with multiple Nova computes handling ironic nodes, this"},{"line_number":12,"context_line":"may not cover all the nodes, so we are moving it to be run whenever the"},{"line_number":13,"context_line":"cache is refreshed, and new nodes for the compute service that have"},{"line_number":14,"context_line":"instances are detected. We also need to cover the cases where an"},{"line_number":15,"context_line":"existing node has a new instance created on it to ensure that that new"},{"line_number":16,"context_line":"instance\u0027s flavor is also migrated."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #1707021"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"9f436f4f_9e498e8c","line":16,"range":{"start_line":14,"start_character":23,"end_line":16,"end_character":35},"updated":"2017-08-11 13:09:01.000000000","message":"if it\u0027s a new instance, why would we need to migrate the instance\u0027s flavor? isn\u0027t that already done by the conductor when creating the build request?","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1a15fb6222840451bd5cc6231a8eca539bd52636","unresolved":false,"context_lines":[{"line_number":11,"context_line":"like an upgrade with multiple Nova computes handling ironic nodes, this"},{"line_number":12,"context_line":"may not cover all the nodes, so we are moving it to be run whenever the"},{"line_number":13,"context_line":"cache is refreshed, and new nodes for the compute service that have"},{"line_number":14,"context_line":"instances are detected. We also need to cover the cases where an"},{"line_number":15,"context_line":"existing node has a new instance created on it to ensure that that new"},{"line_number":16,"context_line":"instance\u0027s flavor is also migrated."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #1707021"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"9f436f4f_17ec61f1","line":16,"range":{"start_line":14,"start_character":23,"end_line":16,"end_character":35},"in_reply_to":"9f436f4f_54aae7d1","updated":"2017-08-11 14:31:24.000000000","message":"gotcha, thanks for the explanation.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"c13be2922f4d66bf329fcf735fbee70254f7daee","unresolved":false,"context_lines":[{"line_number":11,"context_line":"like an upgrade with multiple Nova computes handling ironic nodes, this"},{"line_number":12,"context_line":"may not cover all the nodes, so we are moving it to be run whenever the"},{"line_number":13,"context_line":"cache is refreshed, and new nodes for the compute service that have"},{"line_number":14,"context_line":"instances are detected. We also need to cover the cases where an"},{"line_number":15,"context_line":"existing node has a new instance created on it to ensure that that new"},{"line_number":16,"context_line":"instance\u0027s flavor is also migrated."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #1707021"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":13,"id":"9f436f4f_54aae7d1","line":16,"range":{"start_line":14,"start_character":23,"end_line":16,"end_character":35},"in_reply_to":"9f436f4f_9e498e8c","updated":"2017-08-11 13:48:15.000000000","message":"Because the ironic flavors will be the last things to be updated with custom RC, since we are supporting both custom RC-based and the old ram/cpu/disk-based scheduling. So an operator can set the node\u0027s RC, and a new instance can be created on it based on a flavor that doesn\u0027t have that custom RC in its extra_specs.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"}],"nova/tests/unit/virt/ironic/test_driver.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2435,"context_line":""},{"line_number":2436,"context_line":"    @mock.patch.object(fields.ResourceClass, \u0027normalize_name\u0027)"},{"line_number":2437,"context_line":"    @mock.patch.object(ironic_driver.IronicDriver, \u0027_node_from_cache\u0027)"},{"line_number":2438,"context_line":"    def test_pike_flavor_migration_empty_node(self, mock_node_from_cache,"},{"line_number":2439,"context_line":"            mock_normalize):"},{"line_number":2440,"context_line":"        mock_node_from_cache.return_value \u003d None"},{"line_number":2441,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_29cebcb3","line":2438,"updated":"2017-08-03 17:38:21.000000000","message":"so why is it that these two tests test the ._pike_flavor_migration() method, but the ones above this that start with test_pike_flavor_migration_* do not explicitly call it.\n\nWould it make sense to rename these two to test__pike_flavor_migration_...?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2435,"context_line":""},{"line_number":2436,"context_line":"    @mock.patch.object(fields.ResourceClass, \u0027normalize_name\u0027)"},{"line_number":2437,"context_line":"    @mock.patch.object(ironic_driver.IronicDriver, \u0027_node_from_cache\u0027)"},{"line_number":2438,"context_line":"    def test_pike_flavor_migration_empty_node(self, mock_node_from_cache,"},{"line_number":2439,"context_line":"            mock_normalize):"},{"line_number":2440,"context_line":"        mock_node_from_cache.return_value \u003d None"},{"line_number":2441,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_fcd07421","line":2438,"in_reply_to":"df3967d1_29cebcb3","updated":"2017-08-03 19:01:46.000000000","message":"The tests above are testing the conditions that control whether _pike_flavor_migration is called. These are testing how that method behaves internally under different conditions. The ones after that are testing how _clean_up_extra_specs() behaves internally.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2439,"context_line":"            mock_normalize):"},{"line_number":2440,"context_line":"        mock_node_from_cache.return_value \u003d None"},{"line_number":2441,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"},{"line_number":2442,"context_line":"                \"fake_resource_class\")])"},{"line_number":2443,"context_line":"        mock_normalize.assert_not_called()"},{"line_number":2444,"context_line":""},{"line_number":2445,"context_line":"    @mock.patch.object(fields.ResourceClass, \u0027normalize_name\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_e96794e8","line":2442,"updated":"2017-08-03 17:38:21.000000000","message":"should be list of node uuids?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2439,"context_line":"            mock_normalize):"},{"line_number":2440,"context_line":"        mock_node_from_cache.return_value \u003d None"},{"line_number":2441,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"},{"line_number":2442,"context_line":"                \"fake_resource_class\")])"},{"line_number":2443,"context_line":"        mock_normalize.assert_not_called()"},{"line_number":2444,"context_line":""},{"line_number":2445,"context_line":"    @mock.patch.object(fields.ResourceClass, \u0027normalize_name\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_dccdb085","line":2442,"in_reply_to":"df3967d1_e96794e8","updated":"2017-08-03 19:01:46.000000000","message":"Done","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2454,"context_line":"        mock_node_from_cache.return_value \u003d node1"},{"line_number":2455,"context_line":"        self.driver._migrated_instance_uuids \u003d set([(uuids.instance, \"first\")])"},{"line_number":2456,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"},{"line_number":2457,"context_line":"                \"fake_resource_class\")])"},{"line_number":2458,"context_line":"        mock_normalize.assert_not_called()"},{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"    def test_clean_up_extra_specs_start_empty(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_697cc482","line":2457,"updated":"2017-08-03 17:38:21.000000000","message":"this isn\u0027t correct any more?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2454,"context_line":"        mock_node_from_cache.return_value \u003d node1"},{"line_number":2455,"context_line":"        self.driver._migrated_instance_uuids \u003d set([(uuids.instance, \"first\")])"},{"line_number":2456,"context_line":"        self.driver._pike_flavor_migration([(uuids.node, uuids.instance,"},{"line_number":2457,"context_line":"                \"fake_resource_class\")])"},{"line_number":2458,"context_line":"        mock_normalize.assert_not_called()"},{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"    def test_clean_up_extra_specs_start_empty(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_7c4da498","line":2457,"in_reply_to":"df3967d1_697cc482","updated":"2017-08-03 19:01:46.000000000","message":"Done","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2469,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2470,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs, \"\","},{"line_number":2471,"context_line":"                \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2472,"context_line":"        self.assertTrue(result)"},{"line_number":2473,"context_line":""},{"line_number":2474,"context_line":"    def test_clean_up_extra_specs_start_wrong(self):"},{"line_number":2475,"context_line":"        node1_uuid \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_29591c34","line":2472,"updated":"2017-08-03 17:38:21.000000000","message":"assert specs \u003d {} ?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2469,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2470,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs, \"\","},{"line_number":2471,"context_line":"                \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2472,"context_line":"        self.assertTrue(result)"},{"line_number":2473,"context_line":""},{"line_number":2474,"context_line":"    def test_clean_up_extra_specs_start_wrong(self):"},{"line_number":2475,"context_line":"        node1_uuid \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_dc2250b9","line":2472,"in_reply_to":"df3967d1_29591c34","updated":"2017-08-03 19:01:46.000000000","message":"Done","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2484,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2485,"context_line":"                \"older_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2486,"context_line":"        self.assertTrue(result)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"    def test_clean_up_extra_specs_start_updated(self):"},{"line_number":2489,"context_line":"        node1_uuid \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_495460fe","line":2486,"updated":"2017-08-03 17:38:21.000000000","message":"maybe assert that specs \u003d {}","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2484,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2485,"context_line":"                \"older_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2486,"context_line":"        self.assertTrue(result)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"    def test_clean_up_extra_specs_start_updated(self):"},{"line_number":2489,"context_line":"        node1_uuid \u003d uuidutils.generate_uuid()"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_fc2514c3","line":2486,"in_reply_to":"df3967d1_495460fe","updated":"2017-08-03 19:01:46.000000000","message":"Added here and the method below.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2511,"context_line":"                host\u003dhostname,"},{"line_number":2512,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2513,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2514,"context_line":"                \"test_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2515,"context_line":"        self.assertTrue(result)"},{"line_number":2516,"context_line":""},{"line_number":2517,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_c62b9f92","line":2514,"updated":"2017-08-03 17:38:21.000000000","message":"shouldn\u0027t s/test_class/custom_test_class/ ? Does normalizing the resource class make it all UPPERCASE?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2511,"context_line":"                host\u003dhostname,"},{"line_number":2512,"context_line":"                flavor\u003dfake_flavor1)"},{"line_number":2513,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2514,"context_line":"                \"test_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2515,"context_line":"        self.assertTrue(result)"},{"line_number":2516,"context_line":""},{"line_number":2517,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_bc1e6cb8","line":2514,"in_reply_to":"df3967d1_c62b9f92","updated":"2017-08-03 19:01:46.000000000","message":"Normalizing converts to uppercase, adds the \u0027CUSTOM_\u0027 namespace prefix, and converts some punctuation to underscores. This was an accommodation to ironic operators so that they didn\u0027t have to know the internals of Placement, and could just give their nodes whatever names made sense to them.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":2513,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2514,"context_line":"                \"test_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2515,"context_line":"        self.assertTrue(result)"},{"line_number":2516,"context_line":""},{"line_number":2517,"context_line":""},{"line_number":2518,"context_line":"@mock.patch.object(instance_metadata, \u0027InstanceMetadata\u0027)"},{"line_number":2519,"context_line":"@mock.patch.object(configdrive, \u0027ConfigDriveBuilder\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_692e0493","line":2516,"updated":"2017-08-03 17:38:21.000000000","message":"I suppose you could assert that specs \u003d {\u0027resources:CUSTOM_TEST_CLASS\u0027: \"1\"}","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":2513,"context_line":"        result \u003d self.driver._clean_up_extra_specs(inst1, specs,"},{"line_number":2514,"context_line":"                \"test_class\", \"resources:CUSTOM_TEST_CLASS\")"},{"line_number":2515,"context_line":"        self.assertTrue(result)"},{"line_number":2516,"context_line":""},{"line_number":2517,"context_line":""},{"line_number":2518,"context_line":"@mock.patch.object(instance_metadata, \u0027InstanceMetadata\u0027)"},{"line_number":2519,"context_line":"@mock.patch.object(configdrive, \u0027ConfigDriveBuilder\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_dc7490d1","line":2516,"in_reply_to":"df3967d1_692e0493","updated":"2017-08-03 19:01:46.000000000","message":"Done","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7a2271314e47a143b1244853ab6f8c7e5d560f78","unresolved":false,"context_lines":[{"line_number":2133,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2134,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2135,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2136,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2137,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2138,"context_line":""},{"line_number":2139,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_63c54e5b","line":2136,"range":{"start_line":2136,"start_character":8,"end_line":2136,"end_character":35},"updated":"2017-08-10 20:14:03.000000000","message":"Why do we need to do this if it\u0027s done in setUp?","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"68d737d81f741859d49d10908a14e26afc75cdf2","unresolved":false,"context_lines":[{"line_number":2133,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2134,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2135,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2136,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2137,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2138,"context_line":""},{"line_number":2139,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_9ebbce70","line":2136,"range":{"start_line":2136,"start_character":8,"end_line":2136,"end_character":35},"in_reply_to":"9f436f4f_63442eb8","updated":"2017-08-11 13:09:01.000000000","message":"All tests start after setUp() executes, which means all tests now start with the cache empty...","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"64b8c7e4877cc88cba7ec0b496525ede36f3d6aa","unresolved":false,"context_lines":[{"line_number":2133,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2134,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2135,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2136,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2137,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2138,"context_line":""},{"line_number":2139,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_63442eb8","line":2136,"range":{"start_line":2136,"start_character":8,"end_line":2136,"end_character":35},"in_reply_to":"9f436f4f_63c54e5b","updated":"2017-08-10 20:22:56.000000000","message":"Just to be explicit, since some of these tests start with the cache populated, and some don\u0027t.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1a15fb6222840451bd5cc6231a8eca539bd52636","unresolved":false,"context_lines":[{"line_number":2133,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2134,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2135,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2136,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2137,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2138,"context_line":""},{"line_number":2139,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_b7b6b5f9","line":2136,"range":{"start_line":2136,"start_character":8,"end_line":2136,"end_character":35},"in_reply_to":"9f436f4f_94e04ffc","updated":"2017-08-11 14:31:24.000000000","message":"ok :)","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"c13be2922f4d66bf329fcf735fbee70254f7daee","unresolved":false,"context_lines":[{"line_number":2133,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2134,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2135,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2136,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2137,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2138,"context_line":""},{"line_number":2139,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_94e04ffc","line":2136,"range":{"start_line":2136,"start_character":8,"end_line":2136,"end_character":35},"in_reply_to":"9f436f4f_9ebbce70","updated":"2017-08-11 13:48:15.000000000","message":"Yes, I understand how unit tests work. :)\n\nWhen creating this series, it was useful to be able to *see* the initial states in the same block of code and how they differ from test to test. The code has to handle a variety of initial conditions, and spelling out those conditions made it easier to understand what was being tested.\n\nIt\u0027s hard to see the absence of a condition. So I can look at this test and compare it to the L2352, and know immediately what the difference in initial state is for each test.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7a2271314e47a143b1244853ab6f8c7e5d560f78","unresolved":false,"context_lines":[{"line_number":2185,"context_line":"                network_interface\u003d\"flat\")"},{"line_number":2186,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2187,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2188,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2189,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2190,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2191,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_639aee6e","line":2188,"range":{"start_line":2188,"start_character":8,"end_line":2188,"end_character":35},"updated":"2017-08-10 20:14:03.000000000","message":"Same","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"7a2271314e47a143b1244853ab6f8c7e5d560f78","unresolved":false,"context_lines":[{"line_number":2241,"context_line":"        inst_dict \u003d {inst1.uuid: inst1, inst2.uuid: inst2}"},{"line_number":2242,"context_line":"        mock_get_uuids_by_host.return_value \u003d [inst1.uuid, inst2.uuid]"},{"line_number":2243,"context_line":"        mock_svc_by_hv.return_value \u003d []"},{"line_number":2244,"context_line":"        self.driver.node_cache \u003d {}"},{"line_number":2245,"context_line":"        mock_get_node_list.return_value \u003d [node1, node2]"},{"line_number":2246,"context_line":""},{"line_number":2247,"context_line":"        def fake_inst_by_uuid(ctx, uuid, expected_attrs\u003dNone):"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_e387fe0e","line":2244,"range":{"start_line":2244,"start_character":8,"end_line":2244,"end_character":35},"updated":"2017-08-10 20:14:03.000000000","message":"Same","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"a58af75317d93d64c1ed37bfed373270ca5adbf9","unresolved":false,"context_lines":[{"line_number":2311,"context_line":"        self.assertEqual(2, mock_get_by_uuid.call_count)"},{"line_number":2312,"context_line":"        # Refresh the hash ring again. The mock for getting an instance by uuid"},{"line_number":2313,"context_line":"        # should not be called again."},{"line_number":2314,"context_line":"        self.driver._refresh_hash_ring(self.ctx)"},{"line_number":2315,"context_line":"        self.assertEqual(2, mock_get_by_uuid.call_count)"},{"line_number":2316,"context_line":""},{"line_number":2317,"context_line":"    @mock.patch.object(ironic_driver.IronicDriver, \u0027_get_node_list\u0027)"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_c66ac8b1","line":2314,"range":{"start_line":2314,"start_character":20,"end_line":2314,"end_character":38},"updated":"2017-08-10 20:39:51.000000000","message":"As edleafe mentioned on IRC, this is supposed to be _refresh_cache()","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"}],"nova/virt/ironic/driver.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f6df06ea83e6c35824f0180b2eca997baa47935a","unresolved":false,"context_lines":[{"line_number":538,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":539,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":540,"context_line":"                continue"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"            rc \u003d obj_fields.ResourceClass.normalize_name(node.resource_class)"},{"line_number":543,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":544,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_7b6fe1fb","line":541,"updated":"2017-07-27 17:23:14.000000000","message":"Random whitespace.","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"0ba726befd4dac250308390431dfbdabab3aa813","unresolved":false,"context_lines":[{"line_number":538,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":539,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":540,"context_line":"                continue"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"            rc \u003d obj_fields.ResourceClass.normalize_name(node.resource_class)"},{"line_number":543,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":544,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_2646dcc1","line":541,"in_reply_to":"ff346bd7_068cf87c","updated":"2017-07-27 17:37:44.000000000","message":"and or a brief brief moment I was","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"1cdf8f255837a9ad4174579ed1b8ab933656369a","unresolved":false,"context_lines":[{"line_number":538,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":539,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":540,"context_line":"                continue"},{"line_number":541,"context_line":""},{"line_number":542,"context_line":"            rc \u003d obj_fields.ResourceClass.normalize_name(node.resource_class)"},{"line_number":543,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":544,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_068cf87c","line":541,"in_reply_to":"ff346bd7_7b6fe1fb","updated":"2017-07-27 17:34:14.000000000","message":"I was trying to make cdent happy :/","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f6df06ea83e6c35824f0180b2eca997baa47935a","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                continue"},{"line_number":550,"context_line":"            specs[res_key] \u003d 1"},{"line_number":551,"context_line":"            instance.save()"},{"line_number":552,"context_line":"            self._migrated_flavor_instance_uuids.append(instance.uuid)"},{"line_number":553,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":554,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":555,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_fb7ab1b7","line":552,"updated":"2017-07-27 17:23:14.000000000","message":"You should do this above line 547 or you\u0027ll keep checking it each time, right?","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"1cdf8f255837a9ad4174579ed1b8ab933656369a","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                continue"},{"line_number":550,"context_line":"            specs[res_key] \u003d 1"},{"line_number":551,"context_line":"            instance.save()"},{"line_number":552,"context_line":"            self._migrated_flavor_instance_uuids.append(instance.uuid)"},{"line_number":553,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":554,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":555,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_46c44091","line":552,"in_reply_to":"ff346bd7_fb7ab1b7","updated":"2017-07-27 17:34:14.000000000","message":"Done","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f6df06ea83e6c35824f0180b2eca997baa47935a","unresolved":false,"context_lines":[{"line_number":668,"context_line":"            # Since this will run several times in the life of the driver,"},{"line_number":669,"context_line":"            # track the instances that have already been migrated. This is"},{"line_number":670,"context_line":"            # defined here to make it easier to clean up in Queens."},{"line_number":671,"context_line":"            self._migrated_flavor_instance_uuids \u003d []"},{"line_number":672,"context_line":"        utils.spawn_n(self._pike_flavor_migration, CONF.host)"},{"line_number":673,"context_line":""},{"line_number":674,"context_line":"    def _refresh_cache(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_db324d10","line":671,"updated":"2017-07-27 17:23:14.000000000","message":"I don\u0027t think that\u0027s a good reason to do this outside of __init__() or init_host(). Slap a big comment on it and it\u0027ll be easy to clean up later. If we remove the migration method and not the initialization, we\u0027re really not hurting much.","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"1cdf8f255837a9ad4174579ed1b8ab933656369a","unresolved":false,"context_lines":[{"line_number":668,"context_line":"            # Since this will run several times in the life of the driver,"},{"line_number":669,"context_line":"            # track the instances that have already been migrated. This is"},{"line_number":670,"context_line":"            # defined here to make it easier to clean up in Queens."},{"line_number":671,"context_line":"            self._migrated_flavor_instance_uuids \u003d []"},{"line_number":672,"context_line":"        utils.spawn_n(self._pike_flavor_migration, CONF.host)"},{"line_number":673,"context_line":""},{"line_number":674,"context_line":"    def _refresh_cache(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff346bd7_86b8e8e4","line":671,"in_reply_to":"ff346bd7_db324d10","updated":"2017-07-27 17:34:14.000000000","message":"Done","commit_id":"f5f62d290cb96765d8699cad62782b534b8de39b"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"252870a4c43bd49f5ab0028517dd44e61c7f51c5","unresolved":false,"context_lines":[{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                continue"},{"line_number":553,"context_line":"            specs[res_key] \u003d 1"},{"line_number":554,"context_line":"            instance.save()"},{"line_number":555,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":556,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":557,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_66050446","side":"PARENT","line":554,"updated":"2017-07-27 17:42:03.000000000","message":"If this save fails (is that a thing that could happen?) then the _migrated_flavor... dict is now incorrect","commit_id":"8b52ebd4c9c57c02f6efa208b485afec0e34d732"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"c47b56a54759970c9384a268fcc2d2eb2ab23511","unresolved":false,"context_lines":[{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                continue"},{"line_number":553,"context_line":"            specs[res_key] \u003d 1"},{"line_number":554,"context_line":"            instance.save()"},{"line_number":555,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":556,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":557,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_e17afeee","side":"PARENT","line":554,"in_reply_to":"ff346bd7_21c7e666","updated":"2017-07-27 18:03:14.000000000","message":"Hrm, yeah. Maybe then it should be as-is. Another dumb question, are ovo understanding the fact that one key in a dictionary has changed and the extra_specs has to be updated? In ironic we always do \n\n dii \u003d node.driver_internal_info\n dii[\u0027abc\u0027] \u003d \u0027foo\u0027\n node.driver_internal_info \u003d dii\n node.save()","commit_id":"8b52ebd4c9c57c02f6efa208b485afec0e34d732"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"5efa53d7915804eb497341a4a84205b5855cc697","unresolved":false,"context_lines":[{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                continue"},{"line_number":553,"context_line":"            specs[res_key] \u003d 1"},{"line_number":554,"context_line":"            instance.save()"},{"line_number":555,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":556,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":557,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_21c7e666","side":"PARENT","line":554,"in_reply_to":"ff346bd7_4149ca2e","updated":"2017-07-27 17:57:41.000000000","message":"You mean like it was in PS1?\n\nhttps://review.openstack.org/#/c/487954/1/nova/virt/ironic/driver.py@552","commit_id":"8b52ebd4c9c57c02f6efa208b485afec0e34d732"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"ffde8dc38938784a1782da7b70ebba65ea55e67d","unresolved":false,"context_lines":[{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                continue"},{"line_number":553,"context_line":"            specs[res_key] \u003d 1"},{"line_number":554,"context_line":"            instance.save()"},{"line_number":555,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":556,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":557,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_4149ca2e","side":"PARENT","line":554,"in_reply_to":"ff346bd7_66050446","updated":"2017-07-27 17:53:37.000000000","message":"hrm, yeah, seems unlikely, but maybe better to append instance_uuid only after the save?","commit_id":"8b52ebd4c9c57c02f6efa208b485afec0e34d732"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"eea4ea3a8a8d84d415aa431b17cd8133f2f28e96","unresolved":false,"context_lines":[{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                continue"},{"line_number":553,"context_line":"            specs[res_key] \u003d 1"},{"line_number":554,"context_line":"            instance.save()"},{"line_number":555,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":556,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":557,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_33f85d79","side":"PARENT","line":554,"in_reply_to":"ff346bd7_e17afeee","updated":"2017-07-27 18:29:08.000000000","message":"Ah, we use a separate field type for dictionaries https://github.com/openstack/ironic/blob/master/ironic/objects/fields.py#L85. this should be fine then.","commit_id":"8b52ebd4c9c57c02f6efa208b485afec0e34d732"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"8fc103d23e28bfa5af66cc90b4a235f3804357dd","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                # Flavor has already been updated"},{"line_number":554,"context_line":"                continue"},{"line_number":555,"context_line":"            specs[res_key] \u003d 1"},{"line_number":556,"context_line":"            instance.save()"},{"line_number":557,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":558,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":559,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_01baa208","line":556,"updated":"2017-07-27 18:01:49.000000000","message":"just a nit, but wouldn\u0027t it be safer to move L549 to after this? cuz what if something happened and it wasn\u0027t saved?","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"2dccb0637a81abbed84ea20f39245500b3a6edd7","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                # Flavor has already been updated"},{"line_number":554,"context_line":"                continue"},{"line_number":555,"context_line":"            specs[res_key] \u003d 1"},{"line_number":556,"context_line":"            instance.save()"},{"line_number":557,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":558,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":559,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_2105a614","line":556,"in_reply_to":"ff346bd7_01baa208","updated":"2017-07-27 18:13:29.000000000","message":"I didn\u0027t see the comments in the Base patch, until after I pushed up my comment. If we don\u0027t want to duplicate L549 twice (to be after L553 \u0026 after this line) then I suggest adding a comment above L549, so other great minds (but not as great as Dan who we shall not name) don\u0027t wonder about it too.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"92831c4e7393b62ac92c6d74773c7c2aebb1fda0","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                # Flavor has already been updated"},{"line_number":554,"context_line":"                continue"},{"line_number":555,"context_line":"            specs[res_key] \u003d 1"},{"line_number":556,"context_line":"            instance.save()"},{"line_number":557,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":558,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":559,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_33729d7c","line":556,"in_reply_to":"ff346bd7_2105a614","updated":"2017-07-27 18:22:28.000000000","message":"The concern is the call to Instance.get_by_uuid(), which hits the conductor/database. Putting the line where it is now assures that we will only ever call that once per instance for the driver\u0027s life. My preference would be to error on the side of ensuring that all instances are migrated, at the cost of potentially making a few unnecessary calls.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"9c453894411d19fd5ac18c260d890dd3bc8933f6","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                # Flavor has already been updated"},{"line_number":554,"context_line":"                continue"},{"line_number":555,"context_line":"            specs[res_key] \u003d 1"},{"line_number":556,"context_line":"            instance.save()"},{"line_number":557,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":558,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":559,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_1648a759","line":556,"in_reply_to":"ff346bd7_33729d7c","updated":"2017-07-27 20:04:51.000000000","message":"After an IRC conversation, it would be best to store the instance uuid in 2 places, and not just on L549. Pushing a new version.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"579ed06f2dc9dbb1fe7b1b5002feb15d7e681923","unresolved":false,"context_lines":[{"line_number":553,"context_line":"                # Flavor has already been updated"},{"line_number":554,"context_line":"                continue"},{"line_number":555,"context_line":"            specs[res_key] \u003d 1"},{"line_number":556,"context_line":"            instance.save()"},{"line_number":557,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":558,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":559,"context_line":"                      {\"inst\": instance.uuid, \"rc\": rc})"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_0c053614","line":556,"in_reply_to":"ff346bd7_33729d7c","updated":"2017-07-27 19:52:16.000000000","message":"Discussed with Ed on IRC. We both agree that the code to add the instance.uuid has to be done after L547; either once (eg where it is now), or twice -- where we know that the flavour has been migrated (before L554 and after L556).","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"8fc103d23e28bfa5af66cc90b4a235f3804357dd","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_8137b24d","line":667,"updated":"2017-07-27 18:01:49.000000000","message":"did anyone confirm -- in the original case, in init_host(), you have \n        if not self.node_cache:\n            self._refresh_cache()\n\nWith this change, will the node_cache be populated by the time we get to here? (I\u0027m not very familiar with this code).","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"579ed06f2dc9dbb1fe7b1b5002feb15d7e681923","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_c7ea670d","line":667,"in_reply_to":"ff346bd7_13d8b91b","updated":"2017-07-27 19:52:16.000000000","message":"Thx vdrok. I see now, the periodic task to update_available_resources() calls .get_available_resource() which in turn invokes ._refresh_cache() which in turn calls _refresh_hash_ring :)\n\nI think it makes more sense to call ._pike_flavor_migration() in ._refresh_cache() then; takes out one level of something :)","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"92831c4e7393b62ac92c6d74773c7c2aebb1fda0","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_73802547","line":667,"in_reply_to":"ff346bd7_61784ec3","updated":"2017-07-27 18:22:28.000000000","message":"And the only place this method is called (besides __init__()) is from _refresh_cache(), so that would make for a circular call.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"d77c92814745d15628a9775903d88d5f0d298465","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_b3efad1c","line":667,"in_reply_to":"ff346bd7_61784ec3","updated":"2017-07-27 18:20:22.000000000","message":"If I understand it, it isn\u0027t a periodic task; just a background task. Only called when this _refresh_hash_ring is called.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"6161a99b8ecc6541ba7fc17101affa09b44c95ab","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_13d8b91b","line":667,"in_reply_to":"ff346bd7_73802547","updated":"2017-07-27 18:28:50.000000000","message":"I\u0027m confused now. If _refresh_cache() calls _refresh_hash_ring() BEFORE the cache has been refreshed, does it make sense to invoke ._pike_flavor_migration here, as opposed to *after* it is refreshed? Maybe it doesn\u0027t matter, that at some point/multiple calls to ._pike_flavor_migration, they will all have been migrated...","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"8cc89ef397ff9f68a1c14945631348183088aa3c","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_61784ec3","line":667,"in_reply_to":"ff346bd7_8137b24d","updated":"2017-07-27 18:10:40.000000000","message":"I suppose it does not really matter, as this is called periodically.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"abd478c9134a7439eae8bc1f1bef84dac6c3afe5","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_136b1985","line":667,"in_reply_to":"ff346bd7_b3efad1c","updated":"2017-07-27 18:22:17.000000000","message":"refresh hash ring also does get called when this periodic is run iirc https://github.com/openstack/nova/blob/f575ccbda930531f32c5d648cb9283e015f0dbf1/nova/compute/manager.py#L6581","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"9c453894411d19fd5ac18c260d890dd3bc8933f6","unresolved":false,"context_lines":[{"line_number":664,"context_line":""},{"line_number":665,"context_line":"        self.hash_ring \u003d hash_ring.HashRing(services,"},{"line_number":666,"context_line":"                                            partitions\u003d_HASH_RING_PARTITIONS)"},{"line_number":667,"context_line":"        # For Pike, we need to ensure that all instances have their flavor"},{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_67d3bbd6","line":667,"in_reply_to":"ff346bd7_c7ea670d","updated":"2017-07-27 20:04:51.000000000","message":"Agree; I\u0027ll move the call to _refresh_cache()","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"8fc103d23e28bfa5af66cc90b4a235f3804357dd","unresolved":false,"context_lines":[{"line_number":668,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":669,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":670,"context_line":"        # as not to block this service."},{"line_number":671,"context_line":"        utils.spawn_n(self._pike_flavor_migration, CONF.host)"},{"line_number":672,"context_line":""},{"line_number":673,"context_line":"    def _refresh_cache(self):"},{"line_number":674,"context_line":"        # NOTE(lucasagomes): limit \u003d\u003d 0 is an indicator to continue"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_819b3264","line":671,"updated":"2017-07-27 18:01:49.000000000","message":"just wondering. what if _refresh_hash_ring() gets called more than once, when self._pike_flavor_migration is still happening from the first call. Will the second call invoke this again and will it work? I suppose it will, the instance update is locked so nothing will break; just duplicated work.","commit_id":"8d3a624e0c397ca5a8ccc14d93ef6efe63d1f89a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"571e8223f32e4342bfc97aaa8ba9241e88f070d9","unresolved":false,"context_lines":[{"line_number":518,"context_line":"        \"\"\""},{"line_number":519,"context_line":"        return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def _pike_flavor_migration(self, host, nodes):"},{"line_number":522,"context_line":"        \"\"\"This code is needed in Pike to prevent problems where an operator"},{"line_number":523,"context_line":"        has already adjusted their flavors to add the custom resource class to"},{"line_number":524,"context_line":"        extra_specs. Since existing ironic instances will not have this in"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_5434dcfb","line":521,"updated":"2017-07-28 03:09:32.000000000","message":"i guess we don\u0027t need host anymore?","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d0b3b6f3968cd17a3a9a9237ca72fe001818c485","unresolved":false,"context_lines":[{"line_number":518,"context_line":"        \"\"\""},{"line_number":519,"context_line":"        return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def _pike_flavor_migration(self, host, nodes):"},{"line_number":522,"context_line":"        \"\"\"This code is needed in Pike to prevent problems where an operator"},{"line_number":523,"context_line":"        has already adjusted their flavors to add the custom resource class to"},{"line_number":524,"context_line":"        extra_specs. Since existing ironic instances will not have this in"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_112e58fa","line":521,"in_reply_to":"ff346bd7_5434dcfb","updated":"2017-07-28 12:20:02.000000000","message":"Yes, we do - it\u0027s what actually does the migration. Why would it seem that we wouldn\u0027t?","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d0b3b6f3968cd17a3a9a9237ca72fe001818c485","unresolved":false,"context_lines":[{"line_number":547,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":548,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":549,"context_line":"            res_key \u003d \"resources:%s\" % rc"},{"line_number":550,"context_line":"            if res_key in specs:"},{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                self._migrated_flavor_instance_uuids.add(instance.uuid)"},{"line_number":553,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_d1cfb026","line":550,"range":{"start_line":550,"start_character":12,"end_line":550,"end_character":32},"updated":"2017-07-28 12:20:02.000000000","message":"I\u0027ve since learned that an operator can change the resource class of a node, which could lead to multiple resource entries in extra_specs. Imagine this case:\n\n - node is set with \u0027FOO\u0027 as its resource_class\n - instance.flavor is migrated, and has \u0027resources:CUSTOM_FOO\u0027 in its extra_specs\n - operator changes node\u0027s resource_class to \u0027BAR\u0027.\n - compute is restarted, clearing the _migrated_flavor_instance_uuids cache\n - instance.flavor is migrated again. Since it isn\u0027t in the cache and doesn\u0027t contain \u0027resources:CUSTOM_BAR\u0027 in its extra_specs, that key is added.\n - instance now has two custom resource classes in its extra_specs.\n\nI\u0027m going to change this to clear any \u0027resources:%s\u0027 entries before adding the new key.","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"7489a037e21f41539c783755faccabc30c89c17a","unresolved":false,"context_lines":[{"line_number":547,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":548,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":549,"context_line":"            res_key \u003d \"resources:%s\" % rc"},{"line_number":550,"context_line":"            if res_key in specs:"},{"line_number":551,"context_line":"                # Flavor has already been updated"},{"line_number":552,"context_line":"                self._migrated_flavor_instance_uuids.add(instance.uuid)"},{"line_number":553,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_7186c4bb","line":550,"in_reply_to":"ff346bd7_d1cfb026","updated":"2017-07-28 12:22:52.000000000","message":"What if the flavor has some other custom resource class that hasn\u0027t come from the ironic node\u0027s resource_class, such as:\n\n  resources:CUSTOM_FIREPROOFING\n\nor whatever?","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"571e8223f32e4342bfc97aaa8ba9241e88f070d9","unresolved":false,"context_lines":[{"line_number":700,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":701,"context_line":"        # as not to block this service."},{"line_number":702,"context_line":"        curr_nodes \u003d set(self.node_cache.values())"},{"line_number":703,"context_line":"        new_nodes \u003d curr_nodes - original_nodes"},{"line_number":704,"context_line":"        if new_nodes:"},{"line_number":705,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":706,"context_line":"            utils.spawn_n(self._pike_flavor_migration, CONF.host, new_nodes)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_6f2f0988","line":703,"updated":"2017-07-28 03:09:32.000000000","message":"I know I was thinking that it made sense to take the diff. But I realize I don\u0027t know the performance trade off between doing this set diff, vs ._pike_flavor_migration going through all the nodes in the cache. The set diff has to compare each node object, and the node object has quite a few fields. I\u0027m also wondering how much/if a node changes much, since this is doing a diff of the object, not just the node uuids. I think during deployment, the node doesn\u0027t change unless the operator explicitly changes it. (And I guess it is late for me and I\u0027m too tired to really think.) Someone smarter than me will know the answer :)","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"6416cb4d4a566bea9e1def5031212de1782817fa","unresolved":false,"context_lines":[{"line_number":700,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":701,"context_line":"        # as not to block this service."},{"line_number":702,"context_line":"        curr_nodes \u003d set(self.node_cache.values())"},{"line_number":703,"context_line":"        new_nodes \u003d curr_nodes - original_nodes"},{"line_number":704,"context_line":"        if new_nodes:"},{"line_number":705,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":706,"context_line":"            utils.spawn_n(self._pike_flavor_migration, CONF.host, new_nodes)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_da2a32d9","line":703,"in_reply_to":"ff346bd7_6f2f0988","updated":"2017-07-28 06:57:15.000000000","message":"I don\u0027t think doing set() has a big performance penalty. We don\u0027t seem to implement __hash__ for nodes, so hash(node) \u003d\u003d id(node). Which may be a problem here actually..","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d0b3b6f3968cd17a3a9a9237ca72fe001818c485","unresolved":false,"context_lines":[{"line_number":700,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":701,"context_line":"        # as not to block this service."},{"line_number":702,"context_line":"        curr_nodes \u003d set(self.node_cache.values())"},{"line_number":703,"context_line":"        new_nodes \u003d curr_nodes - original_nodes"},{"line_number":704,"context_line":"        if new_nodes:"},{"line_number":705,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":706,"context_line":"            utils.spawn_n(self._pike_flavor_migration, CONF.host, new_nodes)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_71cec45a","line":703,"in_reply_to":"ff346bd7_da2a32d9","updated":"2017-07-28 12:20:02.000000000","message":"I agree that if id(node) can change between cache refreshes, this isn\u0027t going to help. I\u0027ll change it to compare node UUIDs instead.","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"6416cb4d4a566bea9e1def5031212de1782817fa","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        new_nodes \u003d curr_nodes - original_nodes"},{"line_number":704,"context_line":"        if new_nodes:"},{"line_number":705,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":706,"context_line":"            utils.spawn_n(self._pike_flavor_migration, CONF.host, new_nodes)"},{"line_number":707,"context_line":""},{"line_number":708,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":709,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_7a1b46c9","line":706,"range":{"start_line":706,"start_character":12,"end_line":706,"end_character":76},"updated":"2017-07-28 06:57:15.000000000","message":"a usual concern with spawn_n: what if another _refresh_cache happens before previous spawn_n is finished?","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d0b3b6f3968cd17a3a9a9237ca72fe001818c485","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        new_nodes \u003d curr_nodes - original_nodes"},{"line_number":704,"context_line":"        if new_nodes:"},{"line_number":705,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":706,"context_line":"            utils.spawn_n(self._pike_flavor_migration, CONF.host, new_nodes)"},{"line_number":707,"context_line":""},{"line_number":708,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":709,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":3,"id":"ff346bd7_d1fe9069","line":706,"range":{"start_line":706,"start_character":12,"end_line":706,"end_character":76},"in_reply_to":"ff346bd7_7a1b46c9","updated":"2017-07-28 12:20:02.000000000","message":"Worst case is that an instance could be migrated twice. Otherwise, no penalty, and it prevents this method from blocking.","commit_id":"1a811c6b29ea137d8546da60c0f65cfa113666e6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":518,"context_line":"        \"\"\""},{"line_number":519,"context_line":"        return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def _pike_flavor_migration(self, host, node_uuids):"},{"line_number":522,"context_line":"        \"\"\"This code is needed in Pike to prevent problems where an operator"},{"line_number":523,"context_line":"        has already adjusted their flavors to add the custom resource class to"},{"line_number":524,"context_line":"        extra_specs. Since existing ironic instances will not have this in"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_f1d968f4","line":521,"updated":"2017-07-28 17:51:10.000000000","message":"host is not used in this method?","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":518,"context_line":"        \"\"\""},{"line_number":519,"context_line":"        return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def _pike_flavor_migration(self, host, node_uuids):"},{"line_number":522,"context_line":"        \"\"\"This code is needed in Pike to prevent problems where an operator"},{"line_number":523,"context_line":"        has already adjusted their flavors to add the custom resource class to"},{"line_number":524,"context_line":"        extra_specs. Since existing ironic instances will not have this in"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_94a7aa50","line":521,"in_reply_to":"df3967d1_f1d968f4","updated":"2017-07-28 18:26:41.000000000","message":"Ah, vestigial parameters. Gotta love \u0027em.","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":580,"context_line":"        This assumes that an Ironic instance would not have any other keys in"},{"line_number":581,"context_line":"        its extra_specs that begin with the string \u0027resources:\u0027. After some"},{"line_number":582,"context_line":"        discussion, we could not come up with a scenario where that would not"},{"line_number":583,"context_line":"        be true, so this should be safe."},{"line_number":584,"context_line":"        \"\"\""},{"line_number":585,"context_line":"        unchanged \u003d False"},{"line_number":586,"context_line":"        existing_resource_keys \u003d [k for k in specs"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_d1ff8432","line":583,"updated":"2017-07-28 17:51:10.000000000","message":"hate that \"should\" word","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":580,"context_line":"        This assumes that an Ironic instance would not have any other keys in"},{"line_number":581,"context_line":"        its extra_specs that begin with the string \u0027resources:\u0027. After some"},{"line_number":582,"context_line":"        discussion, we could not come up with a scenario where that would not"},{"line_number":583,"context_line":"        be true, so this should be safe."},{"line_number":584,"context_line":"        \"\"\""},{"line_number":585,"context_line":"        unchanged \u003d False"},{"line_number":586,"context_line":"        existing_resource_keys \u003d [k for k in specs"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_170dfc36","line":583,"in_reply_to":"df3967d1_d1ff8432","updated":"2017-07-28 18:26:41.000000000","message":"OK, I was being conservative. I\u0027ll change it to be definite.","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":594,"context_line":"                    # If this is the only existing resource key, we don\u0027t"},{"line_number":595,"context_line":"                    # need to re-save the instance. But as there could be"},{"line_number":596,"context_line":"                    # other keys, we need to continue this loop, so set the"},{"line_number":597,"context_line":"                    # \u0027unchanged\u0027 flag as False. If there aren\u0027t any \"bad\""},{"line_number":598,"context_line":"                    # keys, the save below will be skipped."},{"line_number":599,"context_line":"                    unchanged \u003d True"},{"line_number":600,"context_line":"                    continue"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_7119f886","line":597,"updated":"2017-07-28 17:51:10.000000000","message":"True?","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":594,"context_line":"                    # If this is the only existing resource key, we don\u0027t"},{"line_number":595,"context_line":"                    # need to re-save the instance. But as there could be"},{"line_number":596,"context_line":"                    # other keys, we need to continue this loop, so set the"},{"line_number":597,"context_line":"                    # \u0027unchanged\u0027 flag as False. If there aren\u0027t any \"bad\""},{"line_number":598,"context_line":"                    # keys, the save below will be skipped."},{"line_number":599,"context_line":"                    unchanged \u003d True"},{"line_number":600,"context_line":"                    continue"}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_d7aad4d0","line":597,"in_reply_to":"df3967d1_7119f886","updated":"2017-07-28 18:26:41.000000000","message":"Done","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":595,"context_line":"                    # need to re-save the instance. But as there could be"},{"line_number":596,"context_line":"                    # other keys, we need to continue this loop, so set the"},{"line_number":597,"context_line":"                    # \u0027unchanged\u0027 flag as False. If there aren\u0027t any \"bad\""},{"line_number":598,"context_line":"                    # keys, the save below will be skipped."},{"line_number":599,"context_line":"                    unchanged \u003d True"},{"line_number":600,"context_line":"                    continue"},{"line_number":601,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during \""}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_b1064020","line":598,"updated":"2017-07-28 17:51:10.000000000","message":"where? you mean _way_ below?","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":595,"context_line":"                    # need to re-save the instance. But as there could be"},{"line_number":596,"context_line":"                    # other keys, we need to continue this loop, so set the"},{"line_number":597,"context_line":"                    # \u0027unchanged\u0027 flag as False. If there aren\u0027t any \"bad\""},{"line_number":598,"context_line":"                    # keys, the save below will be skipped."},{"line_number":599,"context_line":"                    unchanged \u003d True"},{"line_number":600,"context_line":"                    continue"},{"line_number":601,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during \""}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_f7db987a","line":598,"in_reply_to":"df3967d1_b1064020","updated":"2017-07-28 18:26:41.000000000","message":"Actually, it\u0027s above now :)\n\nWhen I moved this code out, I didn\u0027t update the comments.","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"035a9a7c26b095a8d2741388b5cecb0c5d4b427c","unresolved":false,"context_lines":[{"line_number":603,"context_line":"                        {\"key\": key, \"inst\": instance.uuid})"},{"line_number":604,"context_line":"                specs.pop(key)"},{"line_number":605,"context_line":"                unchanged \u003d False"},{"line_number":606,"context_line":"        return unchanged"},{"line_number":607,"context_line":""},{"line_number":608,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":609,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_b12d20aa","line":606,"updated":"2017-07-28 17:51:10.000000000","message":"If you\u0027re going to return true or false, I would name the method something different so that the negative nature of the return is more clear? unchange is true when things are not changed is complicated, so some assistance from the method name could help.","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"15f7a81a8402fd8fdbd6df6eb2f70e6229ee1863","unresolved":false,"context_lines":[{"line_number":603,"context_line":"                        {\"key\": key, \"inst\": instance.uuid})"},{"line_number":604,"context_line":"                specs.pop(key)"},{"line_number":605,"context_line":"                unchanged \u003d False"},{"line_number":606,"context_line":"        return unchanged"},{"line_number":607,"context_line":""},{"line_number":608,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":609,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"df3967d1_571824d2","line":606,"in_reply_to":"df3967d1_b12d20aa","updated":"2017-07-28 18:26:41.000000000","message":"Done","commit_id":"6141e6d2aa5d9f2672e448b7f2ca40ac66cb1293"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"f598e69fce6a09f1a3cfadf94a522944c16412c1","unresolved":false,"context_lines":[{"line_number":550,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":551,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":552,"context_line":"            resource_key \u003d \"resources:%s\" % rc"},{"line_number":553,"context_line":"            if self._extra_specs_already_updated(instance, specs,"},{"line_number":554,"context_line":"                    resource_key):"},{"line_number":555,"context_line":"                continue"},{"line_number":556,"context_line":"            specs[resource_key] \u003d 1"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_85276465","line":553,"updated":"2017-07-29 11:56:25.000000000","message":"Is there any value in commenting here that specs might be modified when it goes through this method? I think that\u0027s the right thing to do in this instance but initially I wasn\u0027t expecting the side effect.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":550,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":551,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":552,"context_line":"            resource_key \u003d \"resources:%s\" % rc"},{"line_number":553,"context_line":"            if self._extra_specs_already_updated(instance, specs,"},{"line_number":554,"context_line":"                    resource_key):"},{"line_number":555,"context_line":"                continue"},{"line_number":556,"context_line":"            specs[resource_key] \u003d 1"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_ec73757f","line":553,"in_reply_to":"df3967d1_85276465","updated":"2017-08-01 17:24:25.000000000","message":"OK, as long as you don\u0027t complain about having to make the condtional \u0027if not\u0027, since having the action being done in the name would mean that no change \u003d\u003d False.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dea7618d5720a303be226f4cae34abfbfbf0643d","unresolved":false,"context_lines":[{"line_number":573,"context_line":""},{"line_number":574,"context_line":"        If extra_specs has the correct key, though, the instance has had its"},{"line_number":575,"context_line":"        flavor correctly migrated already, so nothing further needs to be done."},{"line_number":576,"context_line":"        This method will return True in that case, indicating that the instance"},{"line_number":577,"context_line":"        is already migrated. In all other cases it will return False."},{"line_number":578,"context_line":""},{"line_number":579,"context_line":"        This assumes that an Ironic instance would not have any other keys in"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_67b57f31","line":576,"updated":"2017-07-31 21:22:32.000000000","message":"I don\u0027t know if I am just parsing things incorrectly, but where is the case with extra_specs having the correct key AND having other resource_class keys? Is that covered above L573 or here? This one seems to include it but if so, I don\u0027t think the description is correct.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":573,"context_line":""},{"line_number":574,"context_line":"        If extra_specs has the correct key, though, the instance has had its"},{"line_number":575,"context_line":"        flavor correctly migrated already, so nothing further needs to be done."},{"line_number":576,"context_line":"        This method will return True in that case, indicating that the instance"},{"line_number":577,"context_line":"        is already migrated. In all other cases it will return False."},{"line_number":578,"context_line":""},{"line_number":579,"context_line":"        This assumes that an Ironic instance would not have any other keys in"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_8c1629f1","line":576,"in_reply_to":"df3967d1_67b57f31","updated":"2017-08-01 17:24:25.000000000","message":"Yeah, good catch. If there are more than one key, and the correct one is evaluated last, it will think everything\u0027s fine. I\u0027ll correct this.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"4d01701461c1e1ea87aae4acd80a09bea15e72c2","unresolved":false,"context_lines":[{"line_number":573,"context_line":""},{"line_number":574,"context_line":"        If extra_specs has the correct key, though, the instance has had its"},{"line_number":575,"context_line":"        flavor correctly migrated already, so nothing further needs to be done."},{"line_number":576,"context_line":"        This method will return True in that case, indicating that the instance"},{"line_number":577,"context_line":"        is already migrated. In all other cases it will return False."},{"line_number":578,"context_line":""},{"line_number":579,"context_line":"        This assumes that an Ironic instance would not have any other keys in"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_600f89e1","line":576,"in_reply_to":"df3967d1_8c1629f1","updated":"2017-08-01 21:32:48.000000000","message":"Well, actually, it\u0027s covered on L579. There was a discussion in -ironic where we decided that there would not be other such keys. But since I\u0027m mucking with the code, I\u0027ll remove that assumption.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dea7618d5720a303be226f4cae34abfbfbf0643d","unresolved":false,"context_lines":[{"line_number":713,"context_line":"    def _refresh_cache(self):"},{"line_number":714,"context_line":"        # This is only needed for calculating new nodes that need to have their"},{"line_number":715,"context_line":"        # instance\u0027s flavor migrated in Pike."},{"line_number":716,"context_line":"        original_nodes \u003d set((node.uuid for node in self.node_cache.values()))"},{"line_number":717,"context_line":""},{"line_number":718,"context_line":"        # NOTE(lucasagomes): limit \u003d\u003d 0 is an indicator to continue"},{"line_number":719,"context_line":"        # pagination until there\u0027re no more values to be returned."}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_e4872dd7","line":716,"updated":"2017-07-31 21:22:32.000000000","message":"if you only want the node.uuid\u0027s, why don\u0027t you just take node_cache.keys()? node_cache is a dictionary keyed by node.uuid, see L728 \u0026 739.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":713,"context_line":"    def _refresh_cache(self):"},{"line_number":714,"context_line":"        # This is only needed for calculating new nodes that need to have their"},{"line_number":715,"context_line":"        # instance\u0027s flavor migrated in Pike."},{"line_number":716,"context_line":"        original_nodes \u003d set((node.uuid for node in self.node_cache.values()))"},{"line_number":717,"context_line":""},{"line_number":718,"context_line":"        # NOTE(lucasagomes): limit \u003d\u003d 0 is an indicator to continue"},{"line_number":719,"context_line":"        # pagination until there\u0027re no more values to be returned."}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_2cddfd41","line":716,"in_reply_to":"df3967d1_e4872dd7","updated":"2017-08-01 17:24:25.000000000","message":"Done","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"29fdd87cb1116d041ef44e7f66091686027ce799","unresolved":false,"context_lines":[{"line_number":744,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":745,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":746,"context_line":"        # as not to block this service."},{"line_number":747,"context_line":"        curr_nodes \u003d set((node.uuid for node in self.node_cache.values()))"},{"line_number":748,"context_line":"        new_node_uuids \u003d curr_nodes - original_nodes"},{"line_number":749,"context_line":"        if new_node_uuids:"},{"line_number":750,"context_line":"            # No need to run if we\u0027ve already run all the nodes"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_8b005c69","line":747,"updated":"2017-08-01 10:58:31.000000000","message":"Ditto to rloo\u0027s comment above.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":744,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":745,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":746,"context_line":"        # as not to block this service."},{"line_number":747,"context_line":"        curr_nodes \u003d set((node.uuid for node in self.node_cache.values()))"},{"line_number":748,"context_line":"        new_node_uuids \u003d curr_nodes - original_nodes"},{"line_number":749,"context_line":"        if new_node_uuids:"},{"line_number":750,"context_line":"            # No need to run if we\u0027ve already run all the nodes"}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_ece6d516","line":747,"in_reply_to":"df3967d1_8b005c69","updated":"2017-08-01 17:24:25.000000000","message":"Done","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"dea7618d5720a303be226f4cae34abfbfbf0643d","unresolved":false,"context_lines":[{"line_number":748,"context_line":"        new_node_uuids \u003d curr_nodes - original_nodes"},{"line_number":749,"context_line":"        if new_node_uuids:"},{"line_number":750,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":751,"context_line":"            utils.spawn_n(self._pike_flavor_migration, new_node_uuids)"},{"line_number":752,"context_line":""},{"line_number":753,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":754,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_64e1fd9a","line":751,"updated":"2017-07-31 21:22:32.000000000","message":"Ed, would you confirm that you\u0027re only interested in ironic nodes that *already* have instances (being deployed) by the time this/nova code is being called? It is a one-time thing only?\n\nI am asking because (i could be totally wrong here) eg: node3 is in new_node_uuids, in node_cache, but node3.instance_uuid is None. The next time the cache is renewed, node3 will not be in new_node_uuids. But would it be possible that node3.instance_uuid is now valid and would that instance need its flavor updated?","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"d41786ee84a7a44c6be3282ea888d649dc410d99","unresolved":false,"context_lines":[{"line_number":748,"context_line":"        new_node_uuids \u003d curr_nodes - original_nodes"},{"line_number":749,"context_line":"        if new_node_uuids:"},{"line_number":750,"context_line":"            # No need to run if we\u0027ve already run all the nodes"},{"line_number":751,"context_line":"            utils.spawn_n(self._pike_flavor_migration, new_node_uuids)"},{"line_number":752,"context_line":""},{"line_number":753,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":754,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":5,"id":"df3967d1_4cbf21d0","line":751,"in_reply_to":"df3967d1_64e1fd9a","updated":"2017-08-01 17:24:25.000000000","message":"OK, that was a scenario that was not considered. Let me re-work the logic to handle that.","commit_id":"af3618f59d177bfb0354fed877f6a8b49ace5560"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e64166af7300a0c45932fe32ea5afd68b147691a","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        migrated, the _migrated_instance_uuids cache will have been"},{"line_number":565,"context_line":"        emptied, and the instance will be processed for migration again."},{"line_number":566,"context_line":"        However, since it is possible for an operator to change a node\u0027s"},{"line_number":567,"context_line":"        resource_class, the existing key may be for the old class. So if there"},{"line_number":568,"context_line":"        are any resource class keys in the extra_specs that *don\u0027t* match the"},{"line_number":569,"context_line":"        current resource_class, we need to remove them, and then add the"},{"line_number":570,"context_line":"        correct key."}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_50ad532f","line":567,"updated":"2017-08-02 17:01:25.000000000","message":"by \u0027key\u0027, you mean the instance\u0027s extra_spec key, right? Ah yeah, you clarify that in the next line.","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e64166af7300a0c45932fe32ea5afd68b147691a","unresolved":false,"context_lines":[{"line_number":581,"context_line":"        if existing_resource_keys:"},{"line_number":582,"context_line":"            needs_save \u003d ((len(existing_resource_keys) !\u003d 1) or"},{"line_number":583,"context_line":"                    (existing_resource_keys[0] !\u003d resource_key))"},{"line_number":584,"context_line":"        if needs_save:"},{"line_number":585,"context_line":"            for key in existing_resource_keys:"},{"line_number":586,"context_line":"                if key \u003d\u003d resource_key:"},{"line_number":587,"context_line":"                    # Flavor has already been updated, and the resource_class"}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_eb60d6a7","line":584,"updated":"2017-08-02 17:01:25.000000000","message":"i\u0027ve noticed recently that some people like to see\n\n   if not needs_save:\n       return false/needs_save\n\nSo they don\u0027t have to indent or whatever the following stuff. I\u0027m good either way (but thought I\u0027d mention it).","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"72f49b7d680054f43819fdb8885af9ef3c564ef3","unresolved":false,"context_lines":[{"line_number":581,"context_line":"        if existing_resource_keys:"},{"line_number":582,"context_line":"            needs_save \u003d ((len(existing_resource_keys) !\u003d 1) or"},{"line_number":583,"context_line":"                    (existing_resource_keys[0] !\u003d resource_key))"},{"line_number":584,"context_line":"        if needs_save:"},{"line_number":585,"context_line":"            for key in existing_resource_keys:"},{"line_number":586,"context_line":"                if key \u003d\u003d resource_key:"},{"line_number":587,"context_line":"                    # Flavor has already been updated, and the resource_class"}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_264bb559","line":584,"in_reply_to":"df3967d1_eb60d6a7","updated":"2017-08-02 18:03:07.000000000","message":"Yeah, but just as many people dislike negative tests. You can\u0027t win. :)","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e64166af7300a0c45932fe32ea5afd68b147691a","unresolved":false,"context_lines":[{"line_number":592,"context_line":"                            \"ironic flavor migration for instance %(inst)s\","},{"line_number":593,"context_line":"                            {\"key\": key, \"inst\": instance.uuid})"},{"line_number":594,"context_line":"                specs.pop(key)"},{"line_number":595,"context_line":"        return needs_save"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":598,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_cbd63228","line":595,"updated":"2017-08-02 17:01:25.000000000","message":"i think you\u0027re missing this condition:\n\nif not needs_save:\n    self._migrated_instance_uuids.add(instance.uuid)\n\nwhich most likely means you need to do that regardless of needs_save or not needs_save... ?","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"72f49b7d680054f43819fdb8885af9ef3c564ef3","unresolved":false,"context_lines":[{"line_number":592,"context_line":"                            \"ironic flavor migration for instance %(inst)s\","},{"line_number":593,"context_line":"                            {\"key\": key, \"inst\": instance.uuid})"},{"line_number":594,"context_line":"                specs.pop(key)"},{"line_number":595,"context_line":"        return needs_save"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":598,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_41ad832f","line":595,"in_reply_to":"df3967d1_cbd63228","updated":"2017-08-02 18:03:07.000000000","message":"Good catch","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"e415092d795b6490359677cb645160cf33936151","unresolved":false,"context_lines":[{"line_number":730,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":731,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":732,"context_line":"        # as not to block this service."},{"line_number":733,"context_line":"        new_node_inst \u003d set([(node.uuid, node.instance_uuid)"},{"line_number":734,"context_line":"                for node in self.node_cache.values()"},{"line_number":735,"context_line":"                if node.instance_uuid and"},{"line_number":736,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids])"}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_eaac7036","line":733,"updated":"2017-08-01 21:41:17.000000000","message":"I don\u0027t know what this variable name means. Sounds like it is an instance of a new node, but I know it is not that.","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"0ec433f7897c66c00545696dc70e4d2d5a8927ea","unresolved":false,"context_lines":[{"line_number":730,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":731,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":732,"context_line":"        # as not to block this service."},{"line_number":733,"context_line":"        new_node_inst \u003d set([(node.uuid, node.instance_uuid)"},{"line_number":734,"context_line":"                for node in self.node_cache.values()"},{"line_number":735,"context_line":"                if node.instance_uuid and"},{"line_number":736,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids])"}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_25b98be8","line":733,"in_reply_to":"df3967d1_eaac7036","updated":"2017-08-01 21:58:58.000000000","message":"It is (node, instance) tuples that represent new instances to migrate. I had several longer versions, but those just sounded comical.","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e64166af7300a0c45932fe32ea5afd68b147691a","unresolved":false,"context_lines":[{"line_number":736,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids])"},{"line_number":737,"context_line":"        if new_node_inst:"},{"line_number":738,"context_line":"            # No need to run unless something has changed"},{"line_number":739,"context_line":"            utils.spawn_n(self._pike_flavor_migration, new_node_inst)"},{"line_number":740,"context_line":""},{"line_number":741,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":742,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":6,"id":"df3967d1_ebb8d6c8","line":739,"updated":"2017-08-02 17:01:25.000000000","message":"I\u0027m not convinced that this takes care of the case where an operator might change a node\u0027s resource_class value *after* the instance\u0027s extraspecs has the old resource_class key.\n\nBecause the instance.uuid will be in self._migrated_instance_uuids.","commit_id":"e511fd580640c65afcefde6615b26f5b6a4dbd61"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"abe493b0d255c9661e9184efaee12f337972a626","unresolved":false,"context_lines":[{"line_number":536,"context_line":"        for node_uuid, instance_uuid, curr_rc in node_inst_rc_tuples:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":539,"context_line":"            if not node:"},{"line_number":540,"context_line":"                continue"},{"line_number":541,"context_line":"            if not node.resource_class:"},{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_e4e364d8","line":539,"updated":"2017-08-02 18:43:05.000000000","message":"not covered?","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"531ebd50ca93ad91211f48e01ef35a005a7c014f","unresolved":false,"context_lines":[{"line_number":536,"context_line":"        for node_uuid, instance_uuid, curr_rc in node_inst_rc_tuples:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":539,"context_line":"            if not node:"},{"line_number":540,"context_line":"                continue"},{"line_number":541,"context_line":"            if not node.resource_class:"},{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_b5cea8b1","line":539,"in_reply_to":"df3967d1_e4e364d8","updated":"2017-08-02 19:52:59.000000000","message":"Thought that you would have caught that L538 would blow up if node didn\u0027t exist. :)\n\nMoved L538 after this, and added a test.","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"abe493b0d255c9661e9184efaee12f337972a626","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_64cf7456","line":545,"updated":"2017-08-02 18:43:05.000000000","message":"not covered?","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"531ebd50ca93ad91211f48e01ef35a005a7c014f","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_75c4c091","line":545,"in_reply_to":"df3967d1_64cf7456","updated":"2017-08-02 19:52:59.000000000","message":"Done","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"abe493b0d255c9661e9184efaee12f337972a626","unresolved":false,"context_lines":[{"line_number":590,"context_line":"            if key \u003d\u003d resource_key:"},{"line_number":591,"context_line":"                # Flavor has already been updated, and the resource_class"},{"line_number":592,"context_line":"                # hasn\u0027t changed"},{"line_number":593,"context_line":"                self._migrated_instance_uuids.add((instance.uuid, node_rc))"},{"line_number":594,"context_line":"            else:"},{"line_number":595,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during ironic \""},{"line_number":596,"context_line":"                        \"flavor migration for instance %(inst)s\","}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_24d5ece5","line":593,"updated":"2017-08-02 18:43:05.000000000","message":"not covered","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"531ebd50ca93ad91211f48e01ef35a005a7c014f","unresolved":false,"context_lines":[{"line_number":590,"context_line":"            if key \u003d\u003d resource_key:"},{"line_number":591,"context_line":"                # Flavor has already been updated, and the resource_class"},{"line_number":592,"context_line":"                # hasn\u0027t changed"},{"line_number":593,"context_line":"                self._migrated_instance_uuids.add((instance.uuid, node_rc))"},{"line_number":594,"context_line":"            else:"},{"line_number":595,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during ironic \""},{"line_number":596,"context_line":"                        \"flavor migration for instance %(inst)s\","}],"source_content_type":"text/x-python","patch_set":7,"id":"df3967d1_55bd3c00","line":593,"in_reply_to":"df3967d1_24d5ece5","updated":"2017-08-02 19:52:59.000000000","message":"Done","commit_id":"4051e24e8d98ab9fe1a7a24e2e970beea56ef7f6"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"7c95fd09d7aa250ba9d97902dcc31cf34a9f8cbe","unresolved":false,"context_lines":[{"line_number":533,"context_line":"        \"\"\""},{"line_number":534,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid, instance_uuid, curr_rc in node_inst_rc_tuples:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":8,"id":"df3967d1_03c3f55e","line":536,"updated":"2017-08-03 00:21:43.000000000","message":"this might be a dumb question, but why include the resource class in the tuple? In fact, why are we passing instance_uuid too? Aren\u0027t they both available from node.resource_class and node.instance_uuid. Not sure how I missed that before. You just need a list of node uuids, you can get the rest of the node info from the cache.","commit_id":"5a320ffcf755e1702f8bdeaeb2cd94bf6ed38000"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"7c95fd09d7aa250ba9d97902dcc31cf34a9f8cbe","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":543,"context_line":"                continue"},{"line_number":544,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":545,"context_line":"            if (instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":8,"id":"df3967d1_e03ea789","line":545,"updated":"2017-08-03 00:21:43.000000000","message":"seems like you\u0027d want to save and compare against the normalized rc in self._migrated_instance_uuids?\n\nHmm, ok, I looked at the code. As long as you decide whether you are saving the normalized rc or the actual rc, and stick to that.","commit_id":"5a320ffcf755e1702f8bdeaeb2cd94bf6ed38000"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"7c95fd09d7aa250ba9d97902dcc31cf34a9f8cbe","unresolved":false,"context_lines":[{"line_number":584,"context_line":"            needs_save \u003d ((len(existing_resource_keys) !\u003d 1) or"},{"line_number":585,"context_line":"                    (existing_resource_keys[0] !\u003d resource_key))"},{"line_number":586,"context_line":"        if not needs_save:"},{"line_number":587,"context_line":"            self._migrated_instance_uuids.add((instance.uuid, node_rc))"},{"line_number":588,"context_line":"            return"},{"line_number":589,"context_line":"        for key in existing_resource_keys:"},{"line_number":590,"context_line":"            if key \u003d\u003d resource_key:"}],"source_content_type":"text/x-python","patch_set":8,"id":"df3967d1_43a7bda8","line":587,"updated":"2017-08-03 00:21:43.000000000","message":"it seems to me that once you enter this method, you want to do this. So I\u0027m not sure it makes sense to have it twice, here and L 593.\n\nOn the other hand -- it is also done at L558. I actually like it only done in _pike_flavor_migration() since that\u0027s where you also use it. Makes it easier to follow.","commit_id":"5a320ffcf755e1702f8bdeaeb2cd94bf6ed38000"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"6ddb7aa6056eeb374aa7d6b3c40ee5bff991254e","unresolved":false,"context_lines":[{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid in node_uuids:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            if not node.resource_class:"},{"line_number":541,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_f7ff74a8","line":538,"updated":"2017-08-03 16:29:59.000000000","message":"not sure I get how this can happen. If the node is not found in cache in _node_from_cache method, request to ironic happens. ironicclient should be raising NotFound if there is no such node anymore.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid in node_uuids:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            if not node.resource_class:"},{"line_number":541,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_bfb53e34","line":538,"in_reply_to":"df3967d1_e6c34388","updated":"2017-08-03 19:01:46.000000000","message":"This is simply extra-defensive code. I don\u0027t think that it is likely to be called, but it cannot hurt anything.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid in node_uuids:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            if not node.resource_class:"},{"line_number":541,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_e6c34388","line":538,"in_reply_to":"df3967d1_f7ff74a8","updated":"2017-08-03 17:38:21.000000000","message":"Oh. Yes, that\u0027s true. Well... it should be in the cache itself, so I guess it is ok to change L537 to node \u003d self.node_cache.get(node_uuid). If it disappeared from there in the microsecs since we looked, it shouldn\u0027t matter.","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"6ddb7aa6056eeb374aa7d6b3c40ee5bff991254e","unresolved":false,"context_lines":[{"line_number":541,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":542,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":543,"context_line":"                continue"},{"line_number":544,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_d7711046","line":544,"updated":"2017-08-03 16:29:59.000000000","message":"nit - move before L540?","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":541,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":542,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":543,"context_line":"                continue"},{"line_number":544,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_fff986c7","line":544,"in_reply_to":"df3967d1_d7711046","updated":"2017-08-03 19:01:46.000000000","message":"Done","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"4e7a9d1b5bc86e611ff885cee2fb48cb0f0386f9","unresolved":false,"context_lines":[{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def _clean_up_extra_specs(self, instance, specs, node_rc, resource_key):"},{"line_number":565,"context_line":"        \"\"\"If a compute node is restarted after an instance\u0027s flavor has been"},{"line_number":566,"context_line":"        migrated, the _migrated_instance_uuids cache will have been"},{"line_number":567,"context_line":"        emptied, and the instance will be processed for migration again."}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_666993f7","line":564,"updated":"2017-08-03 17:38:21.000000000","message":"Is node_rc used? I don\u0027t see it, but I thought if it wasn\u0027t, pep8 or something would complain.\n\nIf Ed hasn\u0027t yet, he\u0027s going to hate me...","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6ff464f80ac6bca606911b089068d7ca235e4083","unresolved":false,"context_lines":[{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def _clean_up_extra_specs(self, instance, specs, node_rc, resource_key):"},{"line_number":565,"context_line":"        \"\"\"If a compute node is restarted after an instance\u0027s flavor has been"},{"line_number":566,"context_line":"        migrated, the _migrated_instance_uuids cache will have been"},{"line_number":567,"context_line":"        emptied, and the instance will be processed for migration again."}],"source_content_type":"text/x-python","patch_set":9,"id":"df3967d1_7f06b6d0","line":564,"in_reply_to":"df3967d1_666993f7","updated":"2017-08-03 19:01:46.000000000","message":"No, that\u0027s left over from the prior version. And nothing but love here!  :)","commit_id":"7226c6d98ed5b1ef14f190248ea50dc9a56ff9a8"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"574ae789d7bb1dedfe2bd350cc0dce4ec5165dc7","unresolved":false,"context_lines":[{"line_number":529,"context_line":"        from thinking that that node is available. This code can be removed in"},{"line_number":530,"context_line":"        Queens, and will need to be updated to also alter extra_specs to"},{"line_number":531,"context_line":"        zero-out the old-style standard resource classes of VCPU, MEMORY_MB,"},{"line_number":532,"context_line":"        and DISK_GB."},{"line_number":533,"context_line":"        \"\"\""},{"line_number":534,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":535,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"df3967d1_7caa2d19","line":532,"updated":"2017-08-04 12:36:42.000000000","message":"isn\u0027t it what _clean_up_extra_specs does?","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"67690f820debedfd7171cd96c10c9b5c2a8d7ac5","unresolved":false,"context_lines":[{"line_number":529,"context_line":"        from thinking that that node is available. This code can be removed in"},{"line_number":530,"context_line":"        Queens, and will need to be updated to also alter extra_specs to"},{"line_number":531,"context_line":"        zero-out the old-style standard resource classes of VCPU, MEMORY_MB,"},{"line_number":532,"context_line":"        and DISK_GB."},{"line_number":533,"context_line":"        \"\"\""},{"line_number":534,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":535,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"df3967d1_9fc38389","line":532,"in_reply_to":"df3967d1_7caa2d19","updated":"2017-08-04 12:56:08.000000000","message":"No, that method handles the cases where there are entries in extra_specs other than the one for the node\u0027s resource_class. This can happen, say, when an operator changes the resource_class of a node whose instance has already had its flavor migrated with the old resource class.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"bec4a1c4acd2072c793403ffb2a4e2abefe89d40","unresolved":false,"context_lines":[{"line_number":534,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid in node_uuids:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            node_rc \u003d node.resource_class"}],"source_content_type":"text/x-python","patch_set":11,"id":"df3967d1_ba28ac96","line":537,"updated":"2017-08-03 20:32:12.000000000","message":"I think vdrok\u0027s point is that IF it isn\u0027t in the cache, it\u0027ll get retrieved by a request to ironic. And if that node isn\u0027t avail in ironic, an exception will be raised. So the extra-defensive code below is a noop. If you want to be defensive, you should check for an exception.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"c22021c90ccae10126636f4f7ea227832d032e20","unresolved":false,"context_lines":[{"line_number":534,"context_line":"        ctx \u003d nova_context.get_admin_context()"},{"line_number":535,"context_line":""},{"line_number":536,"context_line":"        for node_uuid in node_uuids:"},{"line_number":537,"context_line":"            node \u003d self._node_from_cache(node_uuid)"},{"line_number":538,"context_line":"            if not node:"},{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            node_rc \u003d node.resource_class"}],"source_content_type":"text/x-python","patch_set":11,"id":"df3967d1_b0a68daa","line":537,"in_reply_to":"df3967d1_ba28ac96","updated":"2017-08-03 21:16:21.000000000","message":"In order for this method to be called, the node would have to have been just retrieved from the cache. An earlier version did not do that, so the conditional was added. When later versions were made that work like the current version, I left it in.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"59d2e72b5720e093b2702b35420bc8c58a5ade99","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_c282e7c8","line":545,"updated":"2017-08-07 19:46:14.000000000","message":"Why are you checking for this tuple? Especially weird that you\u0027re checking for it in a set supposedly of just uuids.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"f8cfea5331e0f146dafa0c74d87304c128ec1871","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"}],"source_content_type":"text/x-python","patch_set":11,"id":"9f436f4f_47ebc9af","line":545,"in_reply_to":"bf3e73b7_c282e7c8","updated":"2017-08-08 11:45:26.000000000","message":"Because it was pointed out by Ironic people that a node can have its resource_class changed, even with an instance on it. I changed the cache to contain the combination of (instance_uuid, resource_class) to handle that case. I didn\u0027t think that it merited changing the name of the attribute to something like self._migrated_instance_uuid_and_resource_class_combinations","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"c4b73d7d154f518dbb27309e42d6a944ac60396e","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":550,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":551,"context_line":"            resource_key \u003d \"resources:%s\" % normalized_rc"},{"line_number":552,"context_line":"            self._migrated_instance_uuids.add((node.instance_uuid, node_rc))"},{"line_number":553,"context_line":"            if not self._clean_up_extra_specs(instance, specs, resource_key):"},{"line_number":554,"context_line":"                # No changes were made."},{"line_number":555,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_0b3b8e38","line":552,"range":{"start_line":552,"start_character":47,"end_line":552,"end_character":65},"updated":"2017-08-06 02:36:27.000000000","message":"We need to check there is value in the nova.instance_uuid?","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"dfee3dfc8eae1df79703ed058516955f937047b3","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":550,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":551,"context_line":"            resource_key \u003d \"resources:%s\" % normalized_rc"},{"line_number":552,"context_line":"            self._migrated_instance_uuids.add((node.instance_uuid, node_rc))"},{"line_number":553,"context_line":"            if not self._clean_up_extra_specs(instance, specs, resource_key):"},{"line_number":554,"context_line":"                # No changes were made."},{"line_number":555,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_4e6cf235","line":552,"range":{"start_line":552,"start_character":47,"end_line":552,"end_character":65},"in_reply_to":"bf3e73b7_0b3b8e38","updated":"2017-08-07 11:56:13.000000000","message":"No, because the code that calls this and passes in the list to migrate (L733-736) will only include nodes that have an instance_uuid.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"59d2e72b5720e093b2702b35420bc8c58a5ade99","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        \"\"\"If a compute node is restarted after an instance\u0027s flavor has been"},{"line_number":565,"context_line":"        migrated, the _migrated_instance_uuids cache will have been"},{"line_number":566,"context_line":"        emptied, and the instance will be processed for migration again."},{"line_number":567,"context_line":"        However, since it is possible for an operator to change a node\u0027s"},{"line_number":568,"context_line":"        resource_class, the existing key may be for the old class. So if there"},{"line_number":569,"context_line":"        are any resource class keys in the extra_specs that *don\u0027t* match the"},{"line_number":570,"context_line":"        current resource_class, we need to remove them, and then add the"},{"line_number":571,"context_line":"        correct key."}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_e2660b52","line":568,"range":{"start_line":567,"start_character":8,"end_line":568,"end_character":22},"updated":"2017-08-07 19:46:14.000000000","message":"Sure, it\u0027s possible, but that\u0027s going to screw up all accounting the instant they do it. Why are we even tolerating this?  And is your intent to keep this forever to handle such changes? I\u0027m -1 on that.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"f8cfea5331e0f146dafa0c74d87304c128ec1871","unresolved":false,"context_lines":[{"line_number":564,"context_line":"        \"\"\"If a compute node is restarted after an instance\u0027s flavor has been"},{"line_number":565,"context_line":"        migrated, the _migrated_instance_uuids cache will have been"},{"line_number":566,"context_line":"        emptied, and the instance will be processed for migration again."},{"line_number":567,"context_line":"        However, since it is possible for an operator to change a node\u0027s"},{"line_number":568,"context_line":"        resource_class, the existing key may be for the old class. So if there"},{"line_number":569,"context_line":"        are any resource class keys in the extra_specs that *don\u0027t* match the"},{"line_number":570,"context_line":"        current resource_class, we need to remove them, and then add the"},{"line_number":571,"context_line":"        correct key."}],"source_content_type":"text/x-python","patch_set":11,"id":"9f436f4f_e7207dfa","line":568,"range":{"start_line":567,"start_character":8,"end_line":568,"end_character":22},"in_reply_to":"bf3e73b7_e2660b52","updated":"2017-08-08 11:45:26.000000000","message":"This came out of discussion with several people from Ironic. While not something that should be common, it is a) possible to do and b) likely during the Pike transition with the population of node.resource_class values. And no, all of this code is clearly marked as being only for the Pike transition; it will all be removed in Queens. If the operators haven\u0027t finished configuring their nodes by then, that\u0027s their problem.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"59d2e72b5720e093b2702b35420bc8c58a5ade99","unresolved":false,"context_lines":[{"line_number":592,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during ironic \""},{"line_number":593,"context_line":"                        \"flavor migration for instance %(inst)s\","},{"line_number":594,"context_line":"                        {\"key\": key, \"inst\": instance.uuid})"},{"line_number":595,"context_line":"        return needs_save"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":598,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"bf3e73b7_82da9fe6","line":595,"updated":"2017-08-07 19:46:14.000000000","message":"This whole method breaks the operator\u0027s ability to do any other resource overrides in the flavor right? That\u0027s bad because they might want to use other CUSTOM_FOO accounting for any number of other reasons. If this is all just because the class could be changed on the node at will, I think we can/should remove it.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"f8cfea5331e0f146dafa0c74d87304c128ec1871","unresolved":false,"context_lines":[{"line_number":592,"context_line":"                LOG.warning(\"Removed %(key)s from extra_specs during ironic \""},{"line_number":593,"context_line":"                        \"flavor migration for instance %(inst)s\","},{"line_number":594,"context_line":"                        {\"key\": key, \"inst\": instance.uuid})"},{"line_number":595,"context_line":"        return needs_save"},{"line_number":596,"context_line":""},{"line_number":597,"context_line":"    def _get_hypervisor_type(self):"},{"line_number":598,"context_line":"        \"\"\"Get hypervisor type.\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"9f436f4f_07002134","line":595,"in_reply_to":"bf3e73b7_82da9fe6","updated":"2017-08-08 11:45:26.000000000","message":"I thought that we had this discussion earlier: that for Pike, the only thing we would have in extra_specs with the \"resources:\" prefix would be the custom resource_class. If an operator needs to track other things in extra_specs, they can do so, but they should not prefix those things with \"resources:\", which is only for placement. And if they do configure extra_specs to have \"resources:CUSTOM_FOO\" and \"resources:CUSTOM_BAR\", they will never be able to build an instance with that flavor, since Placement will AND those resource requirements together. Since an Ironic node can only have one custom resource class, it will never find any matches.\n\nIf, however, you\u0027d like to permit the zeroing-out of *standard* resource classes, that could be added back in.","commit_id":"446e5dd0f8d49555b2f256623e75dc628a95771a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cc4d66f6b02c0ab748a6f212de388e267a8038be","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_inst_uuid_rcs:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_9a6676ed","line":545,"range":{"start_line":545,"start_character":15,"end_line":545,"end_character":44},"updated":"2017-08-09 18:32:13.000000000","message":"We can do away with this now and just record instances that have been migrated, right?","commit_id":"792317dff7b7ddd42cb2d9fe780f6da6497fc122"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"153857e2f4bd1fdfb23e28df90c81b42aa48acdf","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if (node.instance_uuid, node_rc) in self._migrated_inst_uuid_rcs:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f436f4f_0d259bab","line":545,"range":{"start_line":545,"start_character":15,"end_line":545,"end_character":44},"in_reply_to":"9f436f4f_9a6676ed","updated":"2017-08-10 13:12:13.000000000","message":"Sure, but it doesn\u0027t seem that this would hurt anything, so I didn\u0027t want to muck with it at this point.","commit_id":"792317dff7b7ddd42cb2d9fe780f6da6497fc122"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dccb9f665b9213f8f90f7474d1caee5a4fdcc7e2","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":550,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":551,"context_line":"            resource_key \u003d \"resources:%s\" % normalized_rc"},{"line_number":552,"context_line":"            self._migrated_instance_uuids.add(node.instance_uuid)"},{"line_number":553,"context_line":"            if resource_key in specs:"},{"line_number":554,"context_line":"                # The compute must have been restarted, and the instance.flavor"},{"line_number":555,"context_line":"                # has already been migrated"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_277073dc","line":552,"updated":"2017-08-10 20:08:56.000000000","message":"At first I wondered if this should be after L558, after the migration actually happens, but I see because of the comment on L554, this is accounting for the case of a restart where we want to add an instance to the (now empty) migrated list if we know it either 1) will be migrated or 2) will be skipped on L556 as already migrated.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"64b8c7e4877cc88cba7ec0b496525ede36f3d6aa","unresolved":false,"context_lines":[{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"},{"line_number":550,"context_line":"            specs \u003d instance.flavor.extra_specs"},{"line_number":551,"context_line":"            resource_key \u003d \"resources:%s\" % normalized_rc"},{"line_number":552,"context_line":"            self._migrated_instance_uuids.add(node.instance_uuid)"},{"line_number":553,"context_line":"            if resource_key in specs:"},{"line_number":554,"context_line":"                # The compute must have been restarted, and the instance.flavor"},{"line_number":555,"context_line":"                # has already been migrated"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_43deca11","line":552,"in_reply_to":"9f436f4f_277073dc","updated":"2017-08-10 20:22:56.000000000","message":"zactly","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"815a3df16075b45f662b5db4587ad6b24f708815","unresolved":false,"context_lines":[{"line_number":554,"context_line":"                # The compute must have been restarted, and the instance.flavor"},{"line_number":555,"context_line":"                # has already been migrated"},{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d 1"},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_a284595f","line":557,"updated":"2017-08-10 15:21:43.000000000","message":"I didn\u0027t catch this before, but note that extra_specs are actually always/only strings, so this should be \u00271\u0027 technically. If you hadn\u0027t changed the name of this then it would be not part of your change, so I guess consider it a wash.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"64b8c7e4877cc88cba7ec0b496525ede36f3d6aa","unresolved":false,"context_lines":[{"line_number":554,"context_line":"                # The compute must have been restarted, and the instance.flavor"},{"line_number":555,"context_line":"                # has already been migrated"},{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d 1"},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_230aa689","line":557,"in_reply_to":"9f436f4f_a284595f","updated":"2017-08-10 20:22:56.000000000","message":"You can set it to an integer, and the object will store it as a string. But if it seems cleaner, and since I have to fix more style things, I\u0027ll change this too.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"815a3df16075b45f662b5db4587ad6b24f708815","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                if node.instance_uuid and"},{"line_number":707,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_22dd0970","line":706,"updated":"2017-08-10 15:21:43.000000000","message":"I would normally expect this to be indented to match the code in the opening square bracket...","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dccb9f665b9213f8f90f7474d1caee5a4fdcc7e2","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                if node.instance_uuid and"},{"line_number":707,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_a71c63c1","line":706,"in_reply_to":"9f436f4f_22dd0970","updated":"2017-08-10 20:08:56.000000000","message":"+1 this is wonky","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"004f5c8c12b600ae1e3dd1845bcac2f6c066a3d4","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                if node.instance_uuid and"},{"line_number":707,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_c3f99ac8","line":706,"in_reply_to":"9f436f4f_22dd0970","updated":"2017-08-10 20:07:49.000000000","message":"If you haven\u0027t noticed yet, Ed doesn\u0027t care about what everyone else considers good style when it comes to indenting and insists on 8-spacing everything.","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"6c4197d31bb4fce13de9634c7e33568cb75df757","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                if node.instance_uuid and"},{"line_number":707,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_864ba057","line":706,"in_reply_to":"9f436f4f_83c6f247","updated":"2017-08-10 20:47:54.000000000","message":"Or I could do it like this: \n\n  705         node_uuids \u003d [\n  706                 node.uuid for node in self.node_cache.values()\n  707                 if node.instance_uuid and\n  708                 node.instance_uuid not in self._migrated_instance_uuids]\n\nhehehe","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"64b8c7e4877cc88cba7ec0b496525ede36f3d6aa","unresolved":false,"context_lines":[{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                if node.instance_uuid and"},{"line_number":707,"context_line":"                node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"}],"source_content_type":"text/x-python","patch_set":13,"id":"9f436f4f_83c6f247","line":706,"in_reply_to":"9f436f4f_a71c63c1","updated":"2017-08-10 20:22:56.000000000","message":"OK, if you are all so in love with visual indentation...  :)","commit_id":"b689b584c318cb429946fca73c151c593dcc5e7d"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"40f9326c950496450621093a763098bbc1c603c8","unresolved":false,"context_lines":[{"line_number":156,"context_line":""},{"line_number":157,"context_line":"        self.ironicclient \u003d client_wrapper.IronicClientWrapper()"},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"        # This is needed for the instance flavor migration in Pike, and should"},{"line_number":160,"context_line":"        # be removed in Queens. Since this will run several times in the life"},{"line_number":161,"context_line":"        # of the driver, track the instances that have already been migrated."},{"line_number":162,"context_line":"        self._migrated_instance_uuids \u003d set()"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_29f4c55a","line":159,"range":{"start_line":159,"start_character":8,"end_line":159,"end_character":11},"updated":"2017-08-10 21:31:30.000000000","message":"nit: TODO missing","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"133d549dcee20d7f464a4a43f4caab4fb40afd45","unresolved":false,"context_lines":[{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":541,"context_line":"            if not node_rc:"},{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if node.instance_uuid in self._migrated_instance_uuids:"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_ac9323a3","line":542,"updated":"2017-08-10 21:38:51.000000000","message":"OK seeing this in the ironic job which doesn\u0027t yet set node.resource_class:\n\nhttp://logs.openstack.org/54/487954/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial-nv/041c03a/logs/screen-n-cpu.txt.gz#_Aug_09_19_31_21_252127\n\nAs expected.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e5ce1253acff2cd05d0425aa7dc5c2a05451bb8","unresolved":false,"context_lines":[{"line_number":539,"context_line":"                continue"},{"line_number":540,"context_line":"            node_rc \u003d node.resource_class"},{"line_number":541,"context_line":"            if not node_rc:"},{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if node.instance_uuid in self._migrated_instance_uuids:"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_6c177bef","line":542,"in_reply_to":"9f436f4f_ac9323a3","updated":"2017-08-10 21:41:55.000000000","message":"And I don\u0027t see that in the Ironic change that runs devstack with a resource_class set:\n\nhttp://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-cpu.txt.gz","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"133d549dcee20d7f464a4a43f4caab4fb40afd45","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if node.instance_uuid in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_2cedf324","line":546,"range":{"start_line":545,"start_character":12,"end_line":546,"end_character":24},"updated":"2017-08-10 21:38:51.000000000","message":"This should have been caught on L707, but we could have competing threads I guess so this is another safety check before we try to change anything in the extra spec, right?","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"ae4acb2097e3f4baff84891c7619b28a82678277","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if node.instance_uuid in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_d76a70b1","line":546,"range":{"start_line":545,"start_character":12,"end_line":546,"end_character":24},"in_reply_to":"9f436f4f_2cedf324","updated":"2017-08-10 22:06:33.000000000","message":"It was added when we were concerned with a node having its resource class changed after an instance\u0027s flavor had already been migrated. Once we removed that possibility, I left it there as an extra safety check in the case of multiple threads.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"dd7cae619d037df26dfce95318e2ad246069df43","unresolved":false,"context_lines":[{"line_number":542,"context_line":"                LOG.warning(\"Node %(node)s does not have its resource_class \""},{"line_number":543,"context_line":"                        \"set.\", {\"node\": node.uuid})"},{"line_number":544,"context_line":"                continue"},{"line_number":545,"context_line":"            if node.instance_uuid in self._migrated_instance_uuids:"},{"line_number":546,"context_line":"                continue"},{"line_number":547,"context_line":"            normalized_rc \u003d obj_fields.ResourceClass.normalize_name(node_rc)"},{"line_number":548,"context_line":"            instance \u003d objects.Instance.get_by_uuid(ctx, node.instance_uuid,"},{"line_number":549,"context_line":"                    expected_attrs\u003d[\"flavor\"])"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_f7a2144b","line":546,"range":{"start_line":545,"start_character":12,"end_line":546,"end_character":24},"in_reply_to":"9f436f4f_d76a70b1","updated":"2017-08-10 22:47:14.000000000","message":"yeah I think it\u0027s fine as we don\u0027t synchronize on the set","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8e5ce1253acff2cd05d0425aa7dc5c2a05451bb8","unresolved":false,"context_lines":[{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d \"1\""},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_ace20300","line":559,"updated":"2017-08-10 21:41:55.000000000","message":"I\u0027m not seeing this in the Ironic change that is supposed to set the resource_class in devstack to test this:\n\nhttp://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-cpu.txt.gz","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"62f08bc08c55eb970f44f90c6d58c313395baecf","unresolved":false,"context_lines":[{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d \"1\""},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_9bd08521","line":559,"in_reply_to":"9f436f4f_379f7cb5","updated":"2017-08-11 09:44:12.000000000","message":"The last run still doesn\u0027t have that log in n-cpu...\n\nhttp://logs.openstack.org/54/487954/14/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial-nv/a739db0/logs/screen-n-cpu.txt.gz","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"e9d3f3782b0abefcbc082a751d59c92c1e9a79d4","unresolved":false,"context_lines":[{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d \"1\""},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_e9a46bbb","line":559,"in_reply_to":"9f436f4f_9bd08521","updated":"2017-08-11 11:03:27.000000000","message":"Right, I don\u0027t think this would be shown in nova\u0027s ironic job. This happens when the instance is booted at the point when the old-style flavor is used, and then resource class is added to the node right? I suppose the actual run of this can be seen only on our grenade job, if we\u0027d be setting the resource class to the node during the upgrade.\n\nIn 476968 we create a node with a resource class right away I think.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0fb1118be668b4944a966ff2477d0e241d3f45ef","unresolved":false,"context_lines":[{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d \"1\""},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_cc7e6718","line":559,"in_reply_to":"9f436f4f_ace20300","updated":"2017-08-10 21:43:43.000000000","message":"Probably because things are blowing up during scheduling:\n\nhttp://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-sch.txt.gz#_Aug_09_20_35_41_533621","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"c86d5a31af6c1933cc81b943eb4d167752bb8cb5","unresolved":false,"context_lines":[{"line_number":556,"context_line":"                continue"},{"line_number":557,"context_line":"            specs[resource_key] \u003d \"1\""},{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_379f7cb5","line":559,"in_reply_to":"9f436f4f_cc7e6718","updated":"2017-08-10 22:07:13.000000000","message":"Looks weird.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"133d549dcee20d7f464a4a43f4caab4fb40afd45","unresolved":false,"context_lines":[{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def _get_hypervisor_type(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_ccc6679b","line":561,"range":{"start_line":561,"start_character":52,"end_line":561,"end_character":59},"updated":"2017-08-10 21:38:51.000000000","message":"This used to be the normalized_rc, but now it\u0027s the original, why did you change it?","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"ae4acb2097e3f4baff84891c7619b28a82678277","unresolved":false,"context_lines":[{"line_number":558,"context_line":"            instance.save()"},{"line_number":559,"context_line":"            LOG.debug(\"The flavor extra_specs for Ironic instance %(inst)s \""},{"line_number":560,"context_line":"                      \"have been updated for custom resource class \u0027%(rc)s\u0027.\","},{"line_number":561,"context_line":"                      {\"inst\": instance.uuid, \"rc\": node_rc})"},{"line_number":562,"context_line":"        return"},{"line_number":563,"context_line":""},{"line_number":564,"context_line":"    def _get_hypervisor_type(self):"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_971d0868","line":561,"range":{"start_line":561,"start_character":52,"end_line":561,"end_character":59},"in_reply_to":"9f436f4f_ccc6679b","updated":"2017-08-10 22:06:33.000000000","message":"IIRC, it was from a discussion with some of the ironic people - it would match the name of the custom resource that an operator had assigned to a node.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"40f9326c950496450621093a763098bbc1c603c8","unresolved":false,"context_lines":[{"line_number":702,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                      if node.instance_uuid and"},{"line_number":707,"context_line":"                      node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_a909d531","line":705,"range":{"start_line":705,"start_character":44,"end_line":705,"end_character":59},"updated":"2017-08-10 21:31:30.000000000","message":"could self.node_cache could be different between multiple spawns ?","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"fa4d3a6ff9b0e9d2c58c59cd495527d8e37a838a","unresolved":false,"context_lines":[{"line_number":702,"context_line":"        # migrated to include the resource_class. Since there could be many,"},{"line_number":703,"context_line":"        # many instances controlled by this host, spawn this asynchronously so"},{"line_number":704,"context_line":"        # as not to block this service."},{"line_number":705,"context_line":"        node_uuids \u003d [node.uuid for node in self.node_cache.values()"},{"line_number":706,"context_line":"                      if node.instance_uuid and"},{"line_number":707,"context_line":"                      node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_8cca9f6a","line":705,"range":{"start_line":705,"start_character":44,"end_line":705,"end_character":59},"in_reply_to":"9f436f4f_a909d531","updated":"2017-08-10 21:43:05.000000000","message":"Well, looks like not a problem for greenlets.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"40f9326c950496450621093a763098bbc1c603c8","unresolved":false,"context_lines":[{"line_number":707,"context_line":"                      node.instance_uuid not in self._migrated_instance_uuids]"},{"line_number":708,"context_line":"        if node_uuids:"},{"line_number":709,"context_line":"            # No need to run unless something has changed"},{"line_number":710,"context_line":"            utils.spawn_n(self._pike_flavor_migration, node_uuids)"},{"line_number":711,"context_line":""},{"line_number":712,"context_line":"    def get_available_nodes(self, refresh\u003dFalse):"},{"line_number":713,"context_line":"        \"\"\"Returns the UUIDs of Ironic nodes managed by this compute service."}],"source_content_type":"text/x-python","patch_set":14,"id":"9f436f4f_89817192","line":710,"range":{"start_line":710,"start_character":12,"end_line":710,"end_character":66},"updated":"2017-08-10 21:31:30.000000000","message":"there could be multiple concurrent runs in case the flavor migration could take more time than the refresh cache period, but I think the above verification about node_uuids should be enough.","commit_id":"bf07bd7f61a0f92149a13fb4a87b23636c260706"}]}