)]}'
{"nova/api/openstack/compute/servers.py":[{"author":{"_account_id":10234,"name":"Andrey Pavlov","email":"andrey.mp@gmail.com","username":"andrey-mp"},"change_message_id":"a2b905123ecc1fdbd8830c56b1be90c16c351080","unresolved":false,"context_lines":[{"line_number":776,"context_line":"        ctxt \u003d req.environ[\u0027nova.context\u0027]"},{"line_number":777,"context_line":"        update_dict \u003d {}"},{"line_number":778,"context_line":"        ctxt.can(server_policies.SERVERS % \u0027update\u0027,"},{"line_number":779,"context_line":"                 target\u003d{\u0027user_id\u0027: instance.user_id,"},{"line_number":780,"context_line":"                         \u0027project_id\u0027: instance.project_id})"},{"line_number":781,"context_line":""},{"line_number":782,"context_line":"        server \u003d body[\u0027server\u0027]"}],"source_content_type":"text/x-python","patch_set":1,"id":"9a89bdaa_20420811","line":779,"range":{"start_line":779,"start_character":36,"end_line":779,"end_character":44},"updated":"2016-09-05 17:58:52.000000000","message":"UnboundLocalError: local variable \u0027instance\u0027 referenced before assignment","commit_id":"c0594b0be728e8b542f2046eabaf2594b53659b2"}],"nova/compute/api.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"81060ad799cedee883756f676673d130963ede71","unresolved":false,"context_lines":[{"line_number":2430,"context_line":"            # NOTE(mikal): this try block needs to stay because save() still"},{"line_number":2431,"context_line":"            # might throw an exception."},{"line_number":2432,"context_line":"            instance.update(updates)"},{"line_number":2433,"context_line":"            instance.save()"},{"line_number":2434,"context_line":"        inst_map \u003d self._get_instance_map_or_none(context, instance_id)"},{"line_number":2435,"context_line":"        if inst_map and (inst_map.cell_mapping is not None):"},{"line_number":2436,"context_line":"            with nova_context.target_cell(context, inst_map.cell_mapping):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a89bdaa_d24ca944","line":2433,"updated":"2016-09-06 14:24:31.000000000","message":"Almost everything above this can be simplified with a check that instance.id is set. If it is the instance is from a db record and instance.save will work.","commit_id":"18ceef9504b1aef8076a876a8dad0407557c5d51"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"81060ad799cedee883756f676673d130963ede71","unresolved":false,"context_lines":[{"line_number":2479,"context_line":"            instance \u003d objects.Instance.get_by_uuid("},{"line_number":2480,"context_line":"                context, instance_id, expected_attrs\u003dexpected_attrs)"},{"line_number":2481,"context_line":"            instance.update(updates)"},{"line_number":2482,"context_line":"            instance.save()"},{"line_number":2483,"context_line":""},{"line_number":2484,"context_line":"        return instance"},{"line_number":2485,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9a89bdaa_321dd5bd","line":2482,"updated":"2016-09-06 14:24:31.000000000","message":"I would change the ordering of the above code. You look for an instance first and then the buildreq. Instead, if instance.id is not set, lookup the buildreq first and update it if found then look for an instance if buildreq is not found.\n\nThere\u0027s a small race window where you could update the buildreq after it has been looked up for the boot process so this update won\u0027t carry over to the instance being built from it. Without distributed locking that\u0027s difficult to address but you could look for an instance after updating the buildreq and push the update there if found. But I would do that work as a separate patch from this. Keep this as a simple fix for the reported bug and then worry about that small race.","commit_id":"18ceef9504b1aef8076a876a8dad0407557c5d51"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2408,"context_line":"        # service versions."},{"line_number":2409,"context_line":"        service_version \u003d objects.Service.get_minimum_version("},{"line_number":2410,"context_line":"            context, \u0027nova-osapi_compute\u0027)"},{"line_number":2411,"context_line":"        if service_version \u003c 15:"},{"line_number":2412,"context_line":"            instance.update(updates)"},{"line_number":2413,"context_line":"            instance.save()"},{"line_number":2414,"context_line":"            return instance"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_c43bf936","line":2411,"updated":"2016-09-06 19:03:14.000000000","message":"This check is unnecessary. If service_version \u003c 15 then you will always have an instance object and the check on 2415 will catch it. The API did not return BuildRequest.instance objects until version 15.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2412,"context_line":"            instance.update(updates)"},{"line_number":2413,"context_line":"            instance.save()"},{"line_number":2414,"context_line":"            return instance"},{"line_number":2415,"context_line":"        if instance.obj_attr_is_set(\u0027id\u0027) and instance.id is not None:"},{"line_number":2416,"context_line":"            # Instance has been scheduled and the BuildRequest has been deleted"},{"line_number":2417,"context_line":"            # we can directly write the update down to the right cell."},{"line_number":2418,"context_line":"            inst_map \u003d self._get_instance_map_or_none(context, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_2461053a","line":2415,"updated":"2016-09-06 19:03:14.000000000","message":"If instance.id is set it can\u0027t be None, the second half of this clause is unnecessary.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2418,"context_line":"            inst_map \u003d self._get_instance_map_or_none(context, instance.uuid)"},{"line_number":2419,"context_line":"            if inst_map and (inst_map.cell_mapping is not None):"},{"line_number":2420,"context_line":"                with nova_context.target_cell(context, inst_map.cell_mapping):"},{"line_number":2421,"context_line":"                    instance.update(updates)"},{"line_number":2422,"context_line":"                    instance.save()"},{"line_number":2423,"context_line":"            else:"},{"line_number":2424,"context_line":"                # If BuildRequest is not found but inst_map.cell_mapping"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_44cb691e","line":2421,"updated":"2016-09-06 19:03:14.000000000","message":"nit: you could move the instance.update() out of this if/else so it\u0027s not duplicated on line 2429.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2423,"context_line":"            else:"},{"line_number":2424,"context_line":"                # If BuildRequest is not found but inst_map.cell_mapping"},{"line_number":2425,"context_line":"                # does not point at a cell then cell migration has not"},{"line_number":2426,"context_line":"                # happened yet. This will be a failure case later."},{"line_number":2427,"context_line":"                # TODO(alaski): Make this a failure case after we put in"},{"line_number":2428,"context_line":"                # a block that requires migrating to cellsv2."},{"line_number":2429,"context_line":"                instance.update(updates)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_e430fd03","line":2426,"updated":"2016-09-06 19:03:14.000000000","message":"The first three lines here are inaccurate as there\u0027s no BuildRequest lookup here. It\u0027s close though, the bit about inst_map.cell_mapping is correct.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2429,"context_line":"                instance.update(updates)"},{"line_number":2430,"context_line":"                instance.save()"},{"line_number":2431,"context_line":"        else:"},{"line_number":2432,"context_line":"            # Instance is not yet scheduled, so we need to update BuildRequest"},{"line_number":2433,"context_line":"            # instead"},{"line_number":2434,"context_line":"            try:"},{"line_number":2435,"context_line":"                build_req \u003d objects.BuildRequest.get_by_instance_uuid("}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_24190589","line":2432,"range":{"start_line":2432,"start_character":34,"end_line":2432,"end_character":43},"updated":"2016-09-06 19:03:14.000000000","message":"It\u0027s more accurate to say that the instance is not yet mapped.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2439,"context_line":"                build_req.save()"},{"line_number":2440,"context_line":"            except exception.BuildRequestNotFound:"},{"line_number":2441,"context_line":"                # Instance was mapped and the BuildRequest was deleted"},{"line_number":2442,"context_line":"                # while fetching. Try again."},{"line_number":2443,"context_line":"                inst_map \u003d self._get_instance_map_or_none(context,"},{"line_number":2444,"context_line":"                                                                instance.uuid)"},{"line_number":2445,"context_line":"                if inst_map and (inst_map.cell_mapping is not None):"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_670a2f23","line":2442,"updated":"2016-09-06 19:03:14.000000000","message":"Technically the instance could have been deleted as well, so this could raise InstanceNotFound. However I think it\u0027s correct to not handle that here, but maybe the comment could reflect that.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2446,"context_line":"                    with nova_context.target_cell(context,"},{"line_number":2447,"context_line":"                                                  inst_map.cell_mapping):"},{"line_number":2448,"context_line":"                        instance.update(updates)"},{"line_number":2449,"context_line":"                        instance.save()"},{"line_number":2450,"context_line":"                else:"},{"line_number":2451,"context_line":"                    # If BuildRequest is not found but inst_map.cell_mapping"},{"line_number":2452,"context_line":"                    # does not point at a cell then cell migration has not"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_e7639fda","line":2449,"updated":"2016-09-06 19:03:14.000000000","message":"This instance.save will not work because the else clause on line 2431 means instance.id is not set. You need to do an instance lookup before it can be updated.","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"514cbd870823bf8b40ccd1c6ba061984ce8d0ba1","unresolved":false,"context_lines":[{"line_number":2454,"context_line":"                    # TODO(alaski): Make this a failure case after we put in"},{"line_number":2455,"context_line":"                    # a block that requires migrating to cellsv2."},{"line_number":2456,"context_line":"                    instance.update(updates)"},{"line_number":2457,"context_line":"                    instance.save()"},{"line_number":2458,"context_line":"        return instance"},{"line_number":2459,"context_line":""},{"line_number":2460,"context_line":"    # NOTE(melwitt): We don\u0027t check instance lock for backup because lock is"}],"source_content_type":"text/x-python","patch_set":3,"id":"9a89bdaa_075fa30e","line":2457,"updated":"2016-09-06 19:03:14.000000000","message":"Same comment here as #2449","commit_id":"9ee274286e3c366ab13ead5b54603a87185d2dc6"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"8e25cdc65928fa6b3f4e012362c34cd65affa359","unresolved":false,"context_lines":[{"line_number":2415,"context_line":"        if instance.obj_attr_is_set(\u0027id\u0027) and instance.id is not None:"},{"line_number":2416,"context_line":"            # Instance has been scheduled and the BuildRequest has been deleted"},{"line_number":2417,"context_line":"            # we can directly write the update down to the right cell."},{"line_number":2418,"context_line":"            # TODO(sbauza): Use the cellsv2 target_cell contextmanager for"},{"line_number":2419,"context_line":"            # updating the right cell DB related to the instance."},{"line_number":2420,"context_line":"            instance.update(updates)"},{"line_number":2421,"context_line":"            instance.save()"}],"source_content_type":"text/x-python","patch_set":4,"id":"9a89bdaa_47d44b46","line":2418,"updated":"2016-09-06 19:06:00.000000000","message":"I would like to have the contextmanager code in N so I think it would be better to couple it in with this bug fix.","commit_id":"296689a7427227990b8d3d562656aa05e0b51141"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"0db1bec9f07a232faf41a2c86a9a3d4f8f37ca4c","unresolved":false,"context_lines":[{"line_number":2443,"context_line":"                # instance while the scheduler still looks at the destination,"},{"line_number":2444,"context_line":"                # it means I think it\u0027s okay to just return a 404 and ask"},{"line_number":2445,"context_line":"                # the user to call it again."},{"line_number":2446,"context_line":"                raise exception.InstanceNotFound(uuid\u003dinstance.uuid)"},{"line_number":2447,"context_line":"        return instance"},{"line_number":2448,"context_line":""},{"line_number":2449,"context_line":"    # NOTE(melwitt): We don\u0027t check instance lock for backup because lock is"}],"source_content_type":"text/x-python","patch_set":5,"id":"9a89bdaa_d2a53c47","line":2446,"updated":"2016-09-07 17:14:42.000000000","message":"Returning a 404 here would be bad practice. You shouldn\u0027t be able to show the instance but then get a 404 for an update.\n\nI think what needs to happen here is that the instance is looked up and updated. If it\u0027s not found it was deleted and InstanceNotFound can be raised.","commit_id":"f90269d52d0a5c31a2795d83d4a946eff6a90fa8"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"1d5190ad2072a613a0b549baf12e75d7951f2726","unresolved":false,"context_lines":[{"line_number":2450,"context_line":"                # NOTE(sbauza): Given we don\u0027t know which attributes were asked"},{"line_number":2451,"context_line":"                # for getting the instance, we need to provide all the set ones"},{"line_number":2452,"context_line":"                expected_attrs \u003d [attr for attr in instance.obj_fields"},{"line_number":2453,"context_line":"                                  if instance.obj_attr_is_set(attr)]"},{"line_number":2454,"context_line":"                inst_map \u003d self._get_instance_map_or_none(context,"},{"line_number":2455,"context_line":"                                                          instance.uuid)"},{"line_number":2456,"context_line":"                if inst_map and (inst_map.cell_mapping is not None):"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a89bdaa_0d262f16","line":2453,"updated":"2016-09-09 14:46:43.000000000","message":"If you\u0027re going to go this route you should also check that the field is in objects.instance._INSTANCE_EXTRA_FIELDS as those are the only ones that matter.\n\nHowever, you can guess pretty accurately what the expected_attrs are going to be. [\u0027flavor\u0027, \u0027pci_devices\u0027, \u0027numa_topology\u0027, \u0027tags\u0027, \u0027metadata\u0027, \u0027system_metadata\u0027,\u0027security_groups\u0027, \u0027info_cache\u0027] since that\u0027s the standard set. And this is a corner case that shouldn\u0027t be hit that often so if it\u0027s not completely optimal that\u0027s okay.","commit_id":"88f4c84ecc7f45a6d8a4fa1d007b11974952c136"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"2b51009c5e71a6397f9cd4a90d2d60647a67128a","unresolved":false,"context_lines":[{"line_number":2450,"context_line":"                # NOTE(sbauza): Given we don\u0027t know which attributes were asked"},{"line_number":2451,"context_line":"                # for getting the instance, we need to provide all the set ones"},{"line_number":2452,"context_line":"                expected_attrs \u003d [attr for attr in instance.obj_fields"},{"line_number":2453,"context_line":"                                  if instance.obj_attr_is_set(attr)]"},{"line_number":2454,"context_line":"                inst_map \u003d self._get_instance_map_or_none(context,"},{"line_number":2455,"context_line":"                                                          instance.uuid)"},{"line_number":2456,"context_line":"                if inst_map and (inst_map.cell_mapping is not None):"}],"source_content_type":"text/x-python","patch_set":6,"id":"9a89bdaa_72775d9f","line":2453,"in_reply_to":"9a89bdaa_0d262f16","updated":"2016-09-12 08:58:32.000000000","message":"Okay, I don\u0027t want to have a lot of questions about that... I\u0027ll provide the list directly and leave a TODO then.","commit_id":"88f4c84ecc7f45a6d8a4fa1d007b11974952c136"}],"nova/objects/build_request.py":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"355fb6aa8c9e6caa6fac89407a29804204e1d752","unresolved":false,"context_lines":[{"line_number":202,"context_line":"        db_req \u003d context.session.query("},{"line_number":203,"context_line":"            api_models.BuildRequest).filter_by(id\u003dreq_id).first()"},{"line_number":204,"context_line":"        if not db_req:"},{"line_number":205,"context_line":"            raise exception.BuildRequestNotFound(uuid\u003dreq_id)"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"        db_req.update(updates)"},{"line_number":208,"context_line":"        context.session.add(db_req)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_916f5ffb","line":205,"range":{"start_line":205,"start_character":49,"end_line":205,"end_character":60},"updated":"2016-09-12 17:18:39.000000000","message":"From the wording in nova/exception.py this is supposed to be the instance uuid. I know we don\u0027t have it here but just noting an error message could be potentially confusing in this case.","commit_id":"7a12417ad749da26d30ea958ace009e3971806c5"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"1742fbeb122b671e393c7a3e3ccd9775d8937e11","unresolved":false,"context_lines":[{"line_number":202,"context_line":"        db_req \u003d context.session.query("},{"line_number":203,"context_line":"            api_models.BuildRequest).filter_by(id\u003dreq_id).first()"},{"line_number":204,"context_line":"        if not db_req:"},{"line_number":205,"context_line":"            raise exception.BuildRequestNotFound(uuid\u003dreq_id)"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"        db_req.update(updates)"},{"line_number":208,"context_line":"        context.session.add(db_req)"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_07406c33","line":205,"range":{"start_line":205,"start_character":49,"end_line":205,"end_character":60},"in_reply_to":"9a89bdaa_916f5ffb","updated":"2016-09-12 19:38:49.000000000","message":"Good point.","commit_id":"7a12417ad749da26d30ea958ace009e3971806c5"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"355fb6aa8c9e6caa6fac89407a29804204e1d752","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        updates \u003d self._get_update_primitives()"},{"line_number":214,"context_line":"        db_req \u003d self._save_in_db(self._context, self.id, updates)"},{"line_number":215,"context_line":"        self._from_db_object(self._context, self, db_req)"},{"line_number":216,"context_line":"        self.obj_reset_changes(recursive\u003dTrue)"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_ec11b253","line":216,"range":{"start_line":216,"start_character":31,"end_line":216,"end_character":45},"updated":"2016-09-12 17:18:39.000000000","message":"Why do you want this to be recursive? I don\u0027t think save will save each nested object, so this would hide object changes in nested objects that are valid and could be saved later.","commit_id":"7a12417ad749da26d30ea958ace009e3971806c5"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"1742fbeb122b671e393c7a3e3ccd9775d8937e11","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        updates \u003d self._get_update_primitives()"},{"line_number":214,"context_line":"        db_req \u003d self._save_in_db(self._context, self.id, updates)"},{"line_number":215,"context_line":"        self._from_db_object(self._context, self, db_req)"},{"line_number":216,"context_line":"        self.obj_reset_changes(recursive\u003dTrue)"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_e742a83d","line":216,"range":{"start_line":216,"start_character":31,"end_line":216,"end_character":45},"in_reply_to":"9a89bdaa_8c32e65c","updated":"2016-09-12 19:38:49.000000000","message":"Ack, will remove that line indeed because it\u0027s duplicating the clean-up call.\n\nMelanie, since save(\u003d persists all changes including the nested fields (primitived by L213), we still need to recursively clean up those.","commit_id":"7a12417ad749da26d30ea958ace009e3971806c5"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"d1b7a0841f7e701ea85a39907aea00107f2751b7","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        updates \u003d self._get_update_primitives()"},{"line_number":214,"context_line":"        db_req \u003d self._save_in_db(self._context, self.id, updates)"},{"line_number":215,"context_line":"        self._from_db_object(self._context, self, db_req)"},{"line_number":216,"context_line":"        self.obj_reset_changes(recursive\u003dTrue)"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":""},{"line_number":219,"context_line":"@base.NovaObjectRegistry.register"}],"source_content_type":"text/x-python","patch_set":8,"id":"9a89bdaa_8c32e65c","line":216,"range":{"start_line":216,"start_character":31,"end_line":216,"end_character":45},"in_reply_to":"9a89bdaa_ec11b253","updated":"2016-09-12 17:33:12.000000000","message":"This is also unnecessary because _from_db_object leaves the object in a \"clean\" state.","commit_id":"7a12417ad749da26d30ea958ace009e3971806c5"}]}