)]}'
{"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f3528433b0eb0b8716e06dc41ad3c1957e0e072e","unresolved":false,"context_lines":[{"line_number":3865,"context_line":"                      instance\u003dinstance)"},{"line_number":3866,"context_line":""},{"line_number":3867,"context_line":"        # We only have a claim against one provider, it is the source node"},{"line_number":3868,"context_line":"        cn_uuid \u003d list(orig_alloc.keys())[0]"},{"line_number":3869,"context_line":""},{"line_number":3870,"context_line":"        # Get just the resources part of the one allocation we need below"},{"line_number":3871,"context_line":"        orig_alloc \u003d orig_alloc[cn_uuid].get(\u0027resources\u0027, {})"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f4e5783_fec400f7","line":3868,"updated":"2017-10-13 17:38:14.000000000","message":"hmm, unrelated?","commit_id":"268bad3a55dd408f3aba6510f016555788929be8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"fc9b12f6cd464400866f63106a61360ec5c08d9d","unresolved":false,"context_lines":[{"line_number":3865,"context_line":"                      instance\u003dinstance)"},{"line_number":3866,"context_line":""},{"line_number":3867,"context_line":"        # We only have a claim against one provider, it is the source node"},{"line_number":3868,"context_line":"        cn_uuid \u003d list(orig_alloc.keys())[0]"},{"line_number":3869,"context_line":""},{"line_number":3870,"context_line":"        # Get just the resources part of the one allocation we need below"},{"line_number":3871,"context_line":"        orig_alloc \u003d orig_alloc[cn_uuid].get(\u0027resources\u0027, {})"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f4e5783_f4342195","line":3868,"in_reply_to":"5f4e5783_b4cea9b1","updated":"2017-10-13 18:34:00.000000000","message":"But shouldn\u0027t this have been caught by the py35 unit tests in the change that introduced this code? I\u0027m not saying it\u0027s wrong, just that it\u0027s weird that it\u0027s showing up now in an unrelated change.","commit_id":"268bad3a55dd408f3aba6510f016555788929be8"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"823db137c4cb3a981b7cd0b7f892eca5730a3b27","unresolved":false,"context_lines":[{"line_number":3865,"context_line":"                      instance\u003dinstance)"},{"line_number":3866,"context_line":""},{"line_number":3867,"context_line":"        # We only have a claim against one provider, it is the source node"},{"line_number":3868,"context_line":"        cn_uuid \u003d list(orig_alloc.keys())[0]"},{"line_number":3869,"context_line":""},{"line_number":3870,"context_line":"        # Get just the resources part of the one allocation we need below"},{"line_number":3871,"context_line":"        orig_alloc \u003d orig_alloc[cn_uuid].get(\u0027resources\u0027, {})"}],"source_content_type":"text/x-python","patch_set":20,"id":"5f4e5783_b4cea9b1","line":3868,"in_reply_to":"5f4e5783_fec400f7","updated":"2017-10-13 18:30:39.000000000","message":"No, on python3 keys() is a generator so you can\u0027t [0] it.","commit_id":"268bad3a55dd408f3aba6510f016555788929be8"}],"nova/compute/resource_tracker.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"84bb633f8c1c90e6cb488cbee0fbb9a9c60b936c","unresolved":false,"context_lines":[{"line_number":1144,"context_line":"                continue"},{"line_number":1145,"context_line":""},{"line_number":1146,"context_line":"            # We know these are instances now, so proceed"},{"line_number":1147,"context_line":"            instance_uuid \u003d consumer_uuid"},{"line_number":1148,"context_line":"            try:"},{"line_number":1149,"context_line":"                instance \u003d objects.Instance.get_by_uuid(context, instance_uuid,"},{"line_number":1150,"context_line":"                                                        expected_attrs\u003d[])"}],"source_content_type":"text/x-python","patch_set":5,"id":"7f515b1d_66365ce9","line":1147,"updated":"2017-09-26 16:21:22.000000000","message":"tidy","commit_id":"234e26356bca78666e7020e882f61999d6117242"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"b7e98877e478db10cfc240c31b81c4910ac03ee5","unresolved":false,"context_lines":[{"line_number":1143,"context_line":"                          consumer_uuid, alloc)"},{"line_number":1144,"context_line":"                continue"},{"line_number":1145,"context_line":""},{"line_number":1146,"context_line":"            # We know these are instances now, so proceed"},{"line_number":1147,"context_line":"            instance_uuid \u003d consumer_uuid"},{"line_number":1148,"context_line":"            try:"},{"line_number":1149,"context_line":"                instance \u003d objects.Instance.get_by_uuid(context, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":11,"id":"7f515b1d_84e977b8","line":1146,"range":{"start_line":1146,"start_character":14,"end_line":1146,"end_character":45},"updated":"2017-10-05 16:52:15.000000000","message":"Except if in some edge case the migration is moved out from in-progress state without deleting the allocation attached to it. I don\u0027t know how realistic this case is but Jay\u0027s comment on at L1126 makes me think that it is possible.\n\nActually the code at L1160 will clean up such an allocation only the debug message will be misleading by using the migration uuid as an instance uuid.","commit_id":"d89a08f2c01a6f70d1ec5cddcf6ac5177f2d6b1b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4246f41e099d718b7ab883cd38a5baef31b264d7","unresolved":false,"context_lines":[{"line_number":671,"context_line":"                            \u0027flavor\u0027, \u0027migration_context\u0027])"},{"line_number":672,"context_line":""},{"line_number":673,"context_line":"        # Now calculate usage based on instance utilization:"},{"line_number":674,"context_line":"        self._update_usage_from_instances(context, instances, nodename)"},{"line_number":675,"context_line":""},{"line_number":676,"context_line":"        # Grab all in-progress migrations:"},{"line_number":677,"context_line":"        migrations \u003d objects.MigrationList.get_in_progress_by_host_and_node("}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_59d12efb","line":674,"range":{"start_line":674,"start_character":13,"end_line":674,"end_character":41},"updated":"2017-10-11 20:54:35.000000000","message":"OK so this is what used to call _remove_deleted_instances_allocations once it looped over the instances. You\u0027ve moved the call out of that method, but still after it, so we should be OK.","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4246f41e099d718b7ab883cd38a5baef31b264d7","unresolved":false,"context_lines":[{"line_number":674,"context_line":"        self._update_usage_from_instances(context, instances, nodename)"},{"line_number":675,"context_line":""},{"line_number":676,"context_line":"        # Grab all in-progress migrations:"},{"line_number":677,"context_line":"        migrations \u003d objects.MigrationList.get_in_progress_by_host_and_node("},{"line_number":678,"context_line":"                context, self.host, nodename)"},{"line_number":679,"context_line":""},{"line_number":680,"context_line":"        self._pair_instances_to_migrations(migrations, instances)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_39f432cf","line":677,"range":{"start_line":677,"start_character":43,"end_line":677,"end_character":75},"updated":"2017-10-11 20:54:35.000000000","message":"This looks like it might get dicey:\n\nhttps://github.com/openstack/nova/blob/64635ad4a5f60a79e1ec2d5369a8f84bfeccb7e4/nova/db/sqlalchemy/api.py#L4863-L4866\n\nSo that\u0027s looking for migrations which are NOT in one of these states:\n\n[\u0027accepted\u0027, \u0027confirmed\u0027, \u0027reverted\u0027, \u0027error\u0027, \u0027failed\u0027,\n \u0027completed\u0027, \u0027cancelled\u0027]\n\nBut looking at this change of Takashi\u0027s, there is also a \u0027done\u0027 status:\n\nhttps://review.openstack.org/#/c/430608/48/nova/objects/fields.py@1092\n\nSo seems we probably have a bug in our DB API filtering code, yay!\n\nThat \u0027done\u0027 migration status is set when an evacuation is complete:\n\nhttps://github.com/openstack/nova/blob/64635ad4a5f60a79e1ec2d5369a8f84bfeccb7e4/nova/compute/manager.py#L2848\n\nI guess that\u0027s never been a problem here because for an evacuation to set that migration status to \u0027done\u0027 it would happen on the destination node and the RT on the source is presumably not running. However, couldn\u0027t that also apply to rebuild on the same host? Although it looks like we don\u0027t pass a migration object during rebuild because the API only creates the migration record during evacuate:\n\nhttps://github.com/openstack/nova/blob/64635ad4a5f60a79e1ec2d5369a8f84bfeccb7e4/nova/compute/api.py#L3939\n\nnot rebuild:\n\nhttps://github.com/openstack/nova/blob/64635ad4a5f60a79e1ec2d5369a8f84bfeccb7e4/nova/compute/api.py#L2862","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6fa99cb56b44cde733a92923c8b74eed1abd8c59","unresolved":false,"context_lines":[{"line_number":674,"context_line":"        self._update_usage_from_instances(context, instances, nodename)"},{"line_number":675,"context_line":""},{"line_number":676,"context_line":"        # Grab all in-progress migrations:"},{"line_number":677,"context_line":"        migrations \u003d objects.MigrationList.get_in_progress_by_host_and_node("},{"line_number":678,"context_line":"                context, self.host, nodename)"},{"line_number":679,"context_line":""},{"line_number":680,"context_line":"        self._pair_instances_to_migrations(migrations, instances)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_99ea0673","line":677,"range":{"start_line":677,"start_character":43,"end_line":677,"end_character":75},"in_reply_to":"5f4e5783_39f432cf","updated":"2017-10-11 21:00:53.000000000","message":"\u003e I guess that\u0027s never been a problem here because for an evacuation to set that migration status to \u0027done\u0027 it would happen on the destination node and the RT on the source is presumably not running.\n\nFurthermore, if we are on the destination host during an evacuate and we query that migration record out and it\u0027s status is \u0027done\u0027, it shouldn\u0027t have any allocations in Placement anyway *for this node* since the migration allocations would be on the source node.\n\nNote that it is kind of scary that the DB query will return migration records where the host/node passed in are either the source OR dest nodes:\n\nhttps://github.com/openstack/nova/blob/64635ad4a5f60a79e1ec2d5369a8f84bfeccb7e4/nova/db/sqlalchemy/api.py#L4863-L4866\n\nThat seems like it could be problematic for us, no?","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d7aefd65777fe818ce169673183d32f649654e93","unresolved":false,"context_lines":[{"line_number":674,"context_line":"        self._update_usage_from_instances(context, instances, nodename)"},{"line_number":675,"context_line":""},{"line_number":676,"context_line":"        # Grab all in-progress migrations:"},{"line_number":677,"context_line":"        migrations \u003d objects.MigrationList.get_in_progress_by_host_and_node("},{"line_number":678,"context_line":"                context, self.host, nodename)"},{"line_number":679,"context_line":""},{"line_number":680,"context_line":"        self._pair_instances_to_migrations(migrations, instances)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_acbfbece","line":677,"range":{"start_line":677,"start_character":43,"end_line":677,"end_character":75},"in_reply_to":"5f4e5783_99ea0673","updated":"2017-10-11 21:20:18.000000000","message":"I don\u0027t think it is, because we\u0027re processing allocations that we have against us, filtering out those that are for instances we know about. I don\u0027t see any reason to do the migration-holds-the-old-allocation-on-source behavior for evacuations, since you\u0027re never going back there once you start to leave (other than if you go through the scheduler again).\n\nSo I don\u0027t think for evac we\u0027ll ever have allocations against us held by migrations we might accidentally skip. Right?","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9797f070f7c47989b0d699bbdda4005c906e3371","unresolved":false,"context_lines":[{"line_number":674,"context_line":"        self._update_usage_from_instances(context, instances, nodename)"},{"line_number":675,"context_line":""},{"line_number":676,"context_line":"        # Grab all in-progress migrations:"},{"line_number":677,"context_line":"        migrations \u003d objects.MigrationList.get_in_progress_by_host_and_node("},{"line_number":678,"context_line":"                context, self.host, nodename)"},{"line_number":679,"context_line":""},{"line_number":680,"context_line":"        self._pair_instances_to_migrations(migrations, instances)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_0f92449f","line":677,"range":{"start_line":677,"start_character":43,"end_line":677,"end_character":75},"in_reply_to":"5f4e5783_acbfbece","updated":"2017-10-11 21:56:40.000000000","message":"\u003e I don\u0027t see any reason to do the migration-holds-the-old-allocation-on-source behavior for evacuations, since you\u0027re never going back there once you start to leave (other than if you go through the scheduler again).\n\nTo recap some of the discussion in IRC, the only time the allocations for a source node are cleaned up for an evacuation are if the source comes back up and the evacuation is either in-progress (status\u003daccepted) or successfully moved the instance (status\u003ddone):\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L643\n\nhttps://github.com/openstack/nova/blob/master/nova/compute/manager.py#L680\n\nBut if the evacuation fails in the conductor because we didn\u0027t find a destination host, the migration status is set to \u0027error\u0027, or if we failed to spawn/claim on the dest node the migration status is also set to \u0027failed\u0027, so the source node wouldn\u0027t cleanup.\n\nAnd we can\u0027t just always remove from the source node when it comes up and rely on rebuilding to the source node because rebuild bypasses the scheduler so the allocations would be gone in that case, which is bad.\n\nI\u0027m trying to figure out if that\u0027s OK and nothing bad happens, and I\u0027m having a hard time sorting that out right now. It probably won\u0027t make a difference, but it\u0027s also kind of weird that we\u0027ll be doing the new style migration allocation stuff for only cold/live migrations but not evacuations - which means we\u0027ll have to always remember that when thinking about where allocations are tracked during move operations, which gets complicated.","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9da37bd51ce0100fb92c024db1bdcf5c1fd14532","unresolved":false,"context_lines":[{"line_number":1132,"context_line":"        allocations \u003d self.reportclient.get_allocations_for_resource_provider("},{"line_number":1133,"context_line":"                cn.uuid) or {}"},{"line_number":1134,"context_line":"        for consumer_uuid, alloc in allocations.items():"},{"line_number":1135,"context_line":"            if consumer_uuid in known_instances:"},{"line_number":1136,"context_line":"                LOG.debug(\"Instance %s actively managed on this compute host \""},{"line_number":1137,"context_line":"                          \"and has allocations in placement: %s.\","},{"line_number":1138,"context_line":"                          consumer_uuid, alloc)"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_6fead8ac","line":1135,"updated":"2017-10-11 22:22:37.000000000","message":"If we hit this code during an evacuation after we\u0027ve set instance.host to the local host, then the instance is being tracked on this host and we hit this condition and will skip removing any allocations.","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9da37bd51ce0100fb92c024db1bdcf5c1fd14532","unresolved":false,"context_lines":[{"line_number":1178,"context_line":"                            \"heal of allocation because we do not know \""},{"line_number":1179,"context_line":"                            \"what to do.\", instance_uuid, alloc)"},{"line_number":1180,"context_line":"                continue"},{"line_number":1181,"context_line":"            if instance.host !\u003d cn.host:"},{"line_number":1182,"context_line":"                # The instance has been moved to another host either via a"},{"line_number":1183,"context_line":"                # migration, evacuation or unshelve in between the time when we"},{"line_number":1184,"context_line":"                # ran InstanceList.get_by_host_and_node(), added those"}],"source_content_type":"text/x-python","patch_set":19,"id":"5f4e5783_0fc6c449","line":1181,"updated":"2017-10-11 22:22:37.000000000","message":"OK so if we hit this code during an evacuate before the instance.host is changed to our local host, the instance wouldn\u0027t be in known_instances, so we\u0027d hit condition and not remove any allocations. If the source node comes up after the instance was evacuated, it will remove the allocations for the source node. If the source node never comes up, we don\u0027t care about the allocations anyway.","commit_id":"61ff2691a5563c828b4232c51055124c0b58a76c"}]}