)]}'
{"nova/compute/api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ef83cc48ba9fd1fa1e983f13a70871e116f83845","unresolved":false,"context_lines":[{"line_number":931,"context_line":""},{"line_number":932,"context_line":"    @staticmethod"},{"line_number":933,"context_line":"    @db_api.api_context_manager.writer"},{"line_number":934,"context_line":"    def _create_reqspec_buildreq_instmapping_in_db(ctxt, rs, br, im):"},{"line_number":935,"context_line":"        \"\"\"Create the request spec, build request, and instance mapping records"},{"line_number":936,"context_line":"        in a single database transaction."},{"line_number":937,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_5ed6cac5","line":934,"updated":"2019-04-24 16:15:49.000000000","message":"I could use something like this in the cross-cell resize series when I\u0027m creating essentially copies of the instance and it\u0027s many related records in the target cell DB:\n\nhttps://review.opendev.org/#/c/627892/27/nova/conductor/tasks/cross_cell_migrate.py@86\n\nWhich are all done with separate transactions. The one big difference there is if anything fails after creating the instance in the target cell, the rollback will hard delete the instance record and any related records created along with it, so it\u0027s a bit lower priority than this single transaction issue you\u0027re trying to fix.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ee716c33b1c3ffc59a9807e6719965c837f12357","unresolved":false,"context_lines":[{"line_number":941,"context_line":"        orphaned records from being left behind if any one of the database"},{"line_number":942,"context_line":"        INSERTs fail for various DB*Error exceptions."},{"line_number":943,"context_line":""},{"line_number":944,"context_line":"        The api_context_manager.writer decorators are not nestable. That is,"},{"line_number":945,"context_line":"        nesting them will not result in a single transaction, but multiple"},{"line_number":946,"context_line":"        transactions."},{"line_number":947,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_c68d0cd4","line":944,"range":{"start_line":944,"start_character":8,"end_line":944,"end_character":66},"updated":"2019-04-24 19:05:47.000000000","message":"as mentioned on IRC, this isn\u0027t the *intent* of oslo.db\u0027s transaction context manager :)\n\nif the enginefacade\u0027s context manager isn\u0027t properly enlisting multiple nested functions into a single transaction, that\u0027s a bug in oslo.db.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"eeae6a41ac77da9f6867d423837670b9fea1b5e7","unresolved":false,"context_lines":[{"line_number":941,"context_line":"        orphaned records from being left behind if any one of the database"},{"line_number":942,"context_line":"        INSERTs fail for various DB*Error exceptions."},{"line_number":943,"context_line":""},{"line_number":944,"context_line":"        The api_context_manager.writer decorators are not nestable. That is,"},{"line_number":945,"context_line":"        nesting them will not result in a single transaction, but multiple"},{"line_number":946,"context_line":"        transactions."},{"line_number":947,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_a9922bf5","line":944,"range":{"start_line":944,"start_character":8,"end_line":944,"end_character":66},"in_reply_to":"ffb9cba7_c68d0cd4","updated":"2019-04-24 19:09:27.000000000","message":"Thanks for raising this. You are right and I tried everything all over again with nesting and my func test worked. I must have messed something up the first time. Being able to nest this will make this patch 1000x better so thank you for pointing it out.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cb4bf9ccbf48e4aa35b057ffd5010390a5003b46","unresolved":false,"context_lines":[{"line_number":952,"context_line":"        db_rs.update(updates)"},{"line_number":953,"context_line":"        db_rs.save(ctxt.session)"},{"line_number":954,"context_line":""},{"line_number":955,"context_line":"        br._validate_for_create()"},{"line_number":956,"context_line":"        updates \u003d br._get_update_primitives()"},{"line_number":957,"context_line":"        db_br \u003d db_api_models.BuildRequest()"},{"line_number":958,"context_line":"        db_br.update(updates)"},{"line_number":959,"context_line":"        db_br.save(ctxt.session)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_dee07acf","line":956,"range":{"start_line":955,"start_character":8,"end_line":956,"end_character":45},"updated":"2019-04-24 16:08:19.000000000","message":"Following the _validate_for_create pattern like the other objects you\u0027d return updates and use them the same way.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"95b32d9a12985f7f37819ff6afb669013cfb0e50","unresolved":false,"context_lines":[{"line_number":952,"context_line":"        db_rs.update(updates)"},{"line_number":953,"context_line":"        db_rs.save(ctxt.session)"},{"line_number":954,"context_line":""},{"line_number":955,"context_line":"        br._validate_for_create()"},{"line_number":956,"context_line":"        updates \u003d br._get_update_primitives()"},{"line_number":957,"context_line":"        db_br \u003d db_api_models.BuildRequest()"},{"line_number":958,"context_line":"        db_br.update(updates)"},{"line_number":959,"context_line":"        db_br.save(ctxt.session)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_1e1db2bc","line":956,"range":{"start_line":955,"start_character":8,"end_line":956,"end_character":45},"in_reply_to":"ffb9cba7_dee07acf","updated":"2019-04-24 16:15:10.000000000","message":"Hm, true. Will change it. I don\u0027t know why I did this one differently.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cb4bf9ccbf48e4aa35b057ffd5010390a5003b46","unresolved":false,"context_lines":[{"line_number":962,"context_line":"        db_im \u003d db_api_models.InstanceMapping()"},{"line_number":963,"context_line":"        db_im.update(changes)"},{"line_number":964,"context_line":"        db_im.save(ctxt.session)"},{"line_number":965,"context_line":"        # NOTE: This is done because a later access will trigger a lazy load"},{"line_number":966,"context_line":"        # outside of the db session so it will fail. We don\u0027t lazy load"},{"line_number":967,"context_line":"        # cell_mapping on the object later because we never need an"},{"line_number":968,"context_line":"        # InstanceMapping without the CellMapping."}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_beb826c8","line":965,"updated":"2019-04-24 16:08:19.000000000","message":"This note is confusing, I\u0027m reading it as \"we do this now to avoid a failure later, but we never need it later\", so then why do we care now?","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"95b32d9a12985f7f37819ff6afb669013cfb0e50","unresolved":false,"context_lines":[{"line_number":962,"context_line":"        db_im \u003d db_api_models.InstanceMapping()"},{"line_number":963,"context_line":"        db_im.update(changes)"},{"line_number":964,"context_line":"        db_im.save(ctxt.session)"},{"line_number":965,"context_line":"        # NOTE: This is done because a later access will trigger a lazy load"},{"line_number":966,"context_line":"        # outside of the db session so it will fail. We don\u0027t lazy load"},{"line_number":967,"context_line":"        # cell_mapping on the object later because we never need an"},{"line_number":968,"context_line":"        # InstanceMapping without the CellMapping."}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_3e8996c9","line":965,"in_reply_to":"ffb9cba7_beb826c8","updated":"2019-04-24 16:15:10.000000000","message":"I lazily copied this from the instance_mapping.py file. I think this comment is saying that a lazy load will fail later on, so load it up front in case someone accesses it. The second sentence.... I\u0027m not so sure what it means. I\u0027ll check the review where the comment came from to see if that sheds any light, and improve the sentences here.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ee716c33b1c3ffc59a9807e6719965c837f12357","unresolved":false,"context_lines":[{"line_number":966,"context_line":"        # outside of the db session so it will fail. We don\u0027t lazy load"},{"line_number":967,"context_line":"        # cell_mapping on the object later because we never need an"},{"line_number":968,"context_line":"        # InstanceMapping without the CellMapping."},{"line_number":969,"context_line":"        db_im.cell_mapping"},{"line_number":970,"context_line":""},{"line_number":971,"context_line":"        return db_rs, db_br, db_im"},{"line_number":972,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_697993fa","line":969,"updated":"2019-04-24 19:05:47.000000000","message":"this is obviously from the original InstanceMapping.create() implementation but it definitely seems suspicious since the comment is talking about doing things outside of the DB session :)","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"045a90900dd8d15ce0e784157ea2a2f04431ddf4","unresolved":false,"context_lines":[{"line_number":984,"context_line":"        # This needs to be done separately because if any of the writes fail,"},{"line_number":985,"context_line":"        # _from_db_object() will raise NotFound."},{"line_number":986,"context_line":"        API._fill_in_reqspec_buildreq_instmapping_objects("},{"line_number":987,"context_line":"            ctxt, rs, db_rs, br, db_br, im, db_im)"},{"line_number":988,"context_line":""},{"line_number":989,"context_line":"    def _provision_instances(self, context, instance_type, min_count,"},{"line_number":990,"context_line":"            max_count, base_options, boot_meta, security_groups,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3fce034c_2e490e8a","line":987,"updated":"2019-04-18 23:57:05.000000000","message":"Question: move all these methods into compute.utils or somewhere else?","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ef83cc48ba9fd1fa1e983f13a70871e116f83845","unresolved":false,"context_lines":[{"line_number":1119,"context_line":"                    instance_group.members.extend(members)"},{"line_number":1120,"context_line":""},{"line_number":1121,"context_line":"        # In the case of any exceptions, attempt DB cleanup"},{"line_number":1122,"context_line":"        except Exception:"},{"line_number":1123,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":1124,"context_line":"                self._cleanup_build_artifacts(None, instances_to_build)"},{"line_number":1125,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_bebee662","line":1122,"updated":"2019-04-24 16:15:49.000000000","message":"Wouldn\u0027t a simple alternative be to just delete anything that was partially inserted for a given instance here? Or throw the reqspec/buildreq/im into a smaller scoped try/except block (like where you call _create_reqspec_buildreq_instmapping - or even in that method) and delete things in order if one of them fails to insert? Granted that\u0027s not as fancy as a single transaction, but it\u0027s also not as confusing.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"495c9abb8c96b2f95e7c3dcd7c722df99d51fd68","unresolved":false,"context_lines":[{"line_number":1119,"context_line":"                    instance_group.members.extend(members)"},{"line_number":1120,"context_line":""},{"line_number":1121,"context_line":"        # In the case of any exceptions, attempt DB cleanup"},{"line_number":1122,"context_line":"        except Exception:"},{"line_number":1123,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":1124,"context_line":"                self._cleanup_build_artifacts(None, instances_to_build)"},{"line_number":1125,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_60ff6801","line":1122,"in_reply_to":"ffb9cba7_3e7756d7","updated":"2019-04-24 16:23:04.000000000","message":"Sure, but it also means there is this weird side path for creating these objects from how everything else is done, which could be confusing to debug/maintain long-term. I\u0027m not necessarily -1 on it, but something to think about.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d9e82f34b30c7722dba5dc45d5ae5328ad151dfd","unresolved":false,"context_lines":[{"line_number":1119,"context_line":"                    instance_group.members.extend(members)"},{"line_number":1120,"context_line":""},{"line_number":1121,"context_line":"        # In the case of any exceptions, attempt DB cleanup"},{"line_number":1122,"context_line":"        except Exception:"},{"line_number":1123,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":1124,"context_line":"                self._cleanup_build_artifacts(None, instances_to_build)"},{"line_number":1125,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_43d4aebd","line":1122,"in_reply_to":"ffb9cba7_60ff6801","updated":"2019-04-24 17:42:56.000000000","message":"Understood. I liked this automatic handling of it via database transaction, it felt like a clean approach to me, but I can see your perspective that it has a bit more complexity, in a different way. That is, single database transaction has the simplicity of not needing separate cleanup, but having try-except cleanup has the simplicity of not having to use the object create() pieces in here.\n\nIf there\u0027s more discussion and people would rather do the try-except cleanup, I\u0027ll adapt this patch to the consensus.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0f33e0d8fc5b2846b31377c8b80b5adc91b4bd6e","unresolved":false,"context_lines":[{"line_number":1119,"context_line":"                    instance_group.members.extend(members)"},{"line_number":1120,"context_line":""},{"line_number":1121,"context_line":"        # In the case of any exceptions, attempt DB cleanup"},{"line_number":1122,"context_line":"        except Exception:"},{"line_number":1123,"context_line":"            with excutils.save_and_reraise_exception():"},{"line_number":1124,"context_line":"                self._cleanup_build_artifacts(None, instances_to_build)"},{"line_number":1125,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_3e7756d7","line":1122,"in_reply_to":"ffb9cba7_bebee662","updated":"2019-04-24 16:19:09.000000000","message":"Yeah.... I think that would do it too. But I thought this was really cool that we can do a single transaction with far less change than I had expected.","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d2f28f8facc0fda56fe80cd38734faf26328f5e8","unresolved":false,"context_lines":[{"line_number":936,"context_line":""},{"line_number":937,"context_line":"        The RequestContext must be passed in to this method so that the"},{"line_number":938,"context_line":"        database transaction context manager decorator will nest properly and"},{"line_number":939,"context_line":"        include each create() into the same transaction context."},{"line_number":940,"context_line":"        \"\"\""},{"line_number":941,"context_line":"        rs.create()"},{"line_number":942,"context_line":"        br.create()"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffb9cba7_74bf2ad0","line":939,"updated":"2019-04-24 19:27:46.000000000","message":"Each object\u0027s self._context \u003d\u003d context, so they are all using the same RequestContext.","commit_id":"85f8d033d27b31a6398529e0a25da74eae523b08"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"223748aef97fb110867fefb67d4e4f5095c92863","unresolved":false,"context_lines":[{"line_number":936,"context_line":""},{"line_number":937,"context_line":"        The RequestContext must be passed in to this method so that the"},{"line_number":938,"context_line":"        database transaction context manager decorator will nest properly and"},{"line_number":939,"context_line":"        include each create() into the same transaction context."},{"line_number":940,"context_line":"        \"\"\""},{"line_number":941,"context_line":"        rs.create()"},{"line_number":942,"context_line":"        br.create()"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffb9cba7_7492ca4d","line":939,"in_reply_to":"ffb9cba7_74bf2ad0","updated":"2019-04-24 19:29:26.000000000","message":"NOTE: not passing in the RequestContext is what I did wrong when I originally tried this change trying to use an outer database transaction context decorator. Without the RequestContext, it cannot start the outer transaction and tie the create()s together.","commit_id":"85f8d033d27b31a6398529e0a25da74eae523b08"}],"nova/objects/build_request.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cb4bf9ccbf48e4aa35b057ffd5010390a5003b46","unresolved":false,"context_lines":[{"line_number":192,"context_line":"                updates[key] \u003d jsonutils.dumps(value.obj_to_primitive())"},{"line_number":193,"context_line":"        return updates"},{"line_number":194,"context_line":""},{"line_number":195,"context_line":"    def _validate_for_create(self):"},{"line_number":196,"context_line":"        if self.obj_attr_is_set(\u0027id\u0027):"},{"line_number":197,"context_line":"            raise exception.ObjectActionError(action\u003d\u0027create\u0027,"},{"line_number":198,"context_line":"                                              reason\u003d\u0027already created\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"ffb9cba7_be8a6618","line":195,"range":{"start_line":195,"start_character":8,"end_line":195,"end_character":28},"updated":"2019-04-24 16:08:19.000000000","message":"Your pattern in the other versions of this is to return the changes that get persisted, so why not call and return self._get_update_primitives() from this?","commit_id":"caf94d63c5b42020658fa6c7b39e81f5aa309a8d"}],"nova/tests/unit/compute/test_compute_api.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c170cb334aa2eca1bc429894ddc4f73a78f600cf","unresolved":false,"context_lines":[{"line_number":4714,"context_line":"                                              mock_br, mock_rs):"},{"line_number":4715,"context_line":"        fake_keypair \u003d objects.KeyPair(name\u003d\u0027test\u0027)"},{"line_number":4716,"context_line":""},{"line_number":4717,"context_line":"        @mock.patch.object(self.compute_api,"},{"line_number":4718,"context_line":"                           \u0027_create_reqspec_buildreq_instmapping\u0027,"},{"line_number":4719,"context_line":"                           new\u003dmock.MagicMock())"},{"line_number":4720,"context_line":"        @mock.patch(\u0027nova.compute.utils.check_num_instances_quota\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffb9cba7_9aabe71b","line":4717,"updated":"2019-04-24 21:25:18.000000000","message":"Do you really even need to change anything in these unit tests anymore? Since you\u0027re back to just calling create() on each object now, you shouldn\u0027t need to change the unit tests and the functional test is covering the single transaction nature of the change.","commit_id":"85f8d033d27b31a6398529e0a25da74eae523b08"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"8e1a765e7fefd58fcf4515cb232528af1f7f30a7","unresolved":false,"context_lines":[{"line_number":4714,"context_line":"                                              mock_br, mock_rs):"},{"line_number":4715,"context_line":"        fake_keypair \u003d objects.KeyPair(name\u003d\u0027test\u0027)"},{"line_number":4716,"context_line":""},{"line_number":4717,"context_line":"        @mock.patch.object(self.compute_api,"},{"line_number":4718,"context_line":"                           \u0027_create_reqspec_buildreq_instmapping\u0027,"},{"line_number":4719,"context_line":"                           new\u003dmock.MagicMock())"},{"line_number":4720,"context_line":"        @mock.patch(\u0027nova.compute.utils.check_num_instances_quota\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffb9cba7_552b00ea","line":4717,"in_reply_to":"ffb9cba7_1599086d","updated":"2019-04-24 22:25:25.000000000","message":"Just tried it locally, no dice. The @db_api.api_context_manager.writer decorator on the helper method is what triggers the DatabasePoisonFixture to fail these tests if I don\u0027t apply these changes. If you know a different way around, lmk.","commit_id":"85f8d033d27b31a6398529e0a25da74eae523b08"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4b9b24e3a2f6fb6944b799ff64211339e5aa377e","unresolved":false,"context_lines":[{"line_number":4714,"context_line":"                                              mock_br, mock_rs):"},{"line_number":4715,"context_line":"        fake_keypair \u003d objects.KeyPair(name\u003d\u0027test\u0027)"},{"line_number":4716,"context_line":""},{"line_number":4717,"context_line":"        @mock.patch.object(self.compute_api,"},{"line_number":4718,"context_line":"                           \u0027_create_reqspec_buildreq_instmapping\u0027,"},{"line_number":4719,"context_line":"                           new\u003dmock.MagicMock())"},{"line_number":4720,"context_line":"        @mock.patch(\u0027nova.compute.utils.check_num_instances_quota\u0027)"}],"source_content_type":"text/x-python","patch_set":4,"id":"ffb9cba7_1599086d","line":4717,"in_reply_to":"ffb9cba7_9aabe71b","updated":"2019-04-24 22:17:20.000000000","message":"Oooh, good point. Probably not. If not, that\u0027ll RAWK","commit_id":"85f8d033d27b31a6398529e0a25da74eae523b08"}]}