)]}'
{"nova/api/openstack/placement/objects/resource_provider.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":4039,"context_line":"@db_api.placement_context_manager.writer"},{"line_number":4040,"context_line":"def reshape(ctx, inventories, allocations):"},{"line_number":4041,"context_line":"    \"\"\"The \u0027replace the world\u0027 strategy that is executed when we want to"},{"line_number":4042,"context_line":"    completely replace a set of provider, inventory, allocation and consumer"},{"line_number":4043,"context_line":"    information in a single transaction."},{"line_number":4044,"context_line":""},{"line_number":4045,"context_line":"    :note: The reason this has to be done in a single monolithic function is so"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_df7e5c1a","line":4042,"range":{"start_line":4042,"start_character":40,"end_line":4042,"end_character":41},"updated":"2018-07-12 22:52:54.000000000","message":"Remove this comma. We\u0027re not replacing provider information per se.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1abf7dd7eb132a39a84a78e18862ab34bf949d5","unresolved":false,"context_lines":[{"line_number":4039,"context_line":"@db_api.placement_context_manager.writer"},{"line_number":4040,"context_line":"def reshape(ctx, inventories, allocations):"},{"line_number":4041,"context_line":"    \"\"\"The \u0027replace the world\u0027 strategy that is executed when we want to"},{"line_number":4042,"context_line":"    completely replace a set of provider, inventory, allocation and consumer"},{"line_number":4043,"context_line":"    information in a single transaction."},{"line_number":4044,"context_line":""},{"line_number":4045,"context_line":"    :note: The reason this has to be done in a single monolithic function is so"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_bc9040f5","line":4042,"range":{"start_line":4042,"start_character":40,"end_line":4042,"end_character":41},"in_reply_to":"5f7c97a3_df7e5c1a","updated":"2018-07-13 12:22:05.000000000","message":"Done","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":4057,"context_line":"    :param inventories: dict, keyed by resource provider UUID, of"},{"line_number":4058,"context_line":"                        `InventoryList` objects representing the replaced"},{"line_number":4059,"context_line":"                        inventory information for the provider."},{"line_number":4060,"context_line":"    :param allocations: optional `AllocationList` object containing all"},{"line_number":4061,"context_line":"                        allocations for all consumers being modified by the"},{"line_number":4062,"context_line":"                        reshape operatiom"},{"line_number":4063,"context_line":"    :raises: `exception.ConcurrentUpdateDetected` when any resource provider or"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_df2f3cf3","line":4060,"range":{"start_line":4060,"start_character":24,"end_line":4060,"end_character":32},"updated":"2018-07-12 22:52:54.000000000","message":"Nix \u0027optional\u0027. If there\u0027s no allocations, it\u0027ll be an AllocationList with no objects [1][2].\n\n[1] https://review.openstack.org/#/c/576927/5/nova/api/openstack/placement/handlers/reshaper.py@78\n[2] https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/allocation.py#L168-L193","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1abf7dd7eb132a39a84a78e18862ab34bf949d5","unresolved":false,"context_lines":[{"line_number":4057,"context_line":"    :param inventories: dict, keyed by resource provider UUID, of"},{"line_number":4058,"context_line":"                        `InventoryList` objects representing the replaced"},{"line_number":4059,"context_line":"                        inventory information for the provider."},{"line_number":4060,"context_line":"    :param allocations: optional `AllocationList` object containing all"},{"line_number":4061,"context_line":"                        allocations for all consumers being modified by the"},{"line_number":4062,"context_line":"                        reshape operatiom"},{"line_number":4063,"context_line":"    :raises: `exception.ConcurrentUpdateDetected` when any resource provider or"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_7c9aa813","line":4060,"range":{"start_line":4060,"start_character":24,"end_line":4060,"end_character":32},"in_reply_to":"5f7c97a3_df2f3cf3","updated":"2018-07-13 12:22:05.000000000","message":"Done","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":4071,"context_line":"    for rp_uuid, inv_list in inventories.items():"},{"line_number":4072,"context_line":"        LOG.debug(\"reshaping: attempting inventory replacement for \""},{"line_number":4073,"context_line":"                  \"provider %s\", rp_uuid)"},{"line_number":4074,"context_line":"        # Before attempting to reshape any inventory, we first need to clear"},{"line_number":4075,"context_line":"        # allocations on the resource providers in question. The reason for"},{"line_number":4076,"context_line":"        # this is that we will get InventoryInUse exceptions below when we try"},{"line_number":4077,"context_line":"        # to update the provider\u0027s inventory records."},{"line_number":4078,"context_line":"        rp \u003d inv_list[0].resource_provider"},{"line_number":4079,"context_line":"        try:"},{"line_number":4080,"context_line":"            rp_allocs \u003d AllocationList.get_all_by_resource_provider(ctx, rp)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_9f50e461","line":4077,"range":{"start_line":4074,"start_character":0,"end_line":4077,"end_character":53},"updated":"2018-07-12 22:52:54.000000000","message":"I don\u0027t think this is the right way to go about this.  If we try to update inventory records in an allocation-inappropriate way, it\u0027s because the caller f\u0027ed up by not sending an allocation move appropriate to their inventory shuffle. In which case we *want* to raise InventoryInUse.\n\nOf course this means we need to process allocation moves first (before the inventory moves). Which seems like the logical order anyway.\n\n[Later] Okay, I see; the logical order would actually be:\n\n- create new inventories\n- move allocations\n- delete old inventories\n\nWhich means you have to figure out which inventories are \"new\" and which are \"old\"; which is doable when inventories are truly moving, but harder when they\u0027re being \"resized\".  However, I suspect you could get away with splitting it into \"inventory deletions\" (which you do last) and \"other\" (which you do first).\n\nI think it\u0027s worth playing around with that approach. It might end up being no uglier than this; it would obviate the need for the consumer recreate hack; and it would avoid the bug where consumer gens aren\u0027t being checked (see below).","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"7ea006896c5c2d66aeebfba585f9c43a42de535c","unresolved":false,"context_lines":[{"line_number":4071,"context_line":"    for rp_uuid, inv_list in inventories.items():"},{"line_number":4072,"context_line":"        LOG.debug(\"reshaping: attempting inventory replacement for \""},{"line_number":4073,"context_line":"                  \"provider %s\", rp_uuid)"},{"line_number":4074,"context_line":"        # Before attempting to reshape any inventory, we first need to clear"},{"line_number":4075,"context_line":"        # allocations on the resource providers in question. The reason for"},{"line_number":4076,"context_line":"        # this is that we will get InventoryInUse exceptions below when we try"},{"line_number":4077,"context_line":"        # to update the provider\u0027s inventory records."},{"line_number":4078,"context_line":"        rp \u003d inv_list[0].resource_provider"},{"line_number":4079,"context_line":"        try:"},{"line_number":4080,"context_line":"            rp_allocs \u003d AllocationList.get_all_by_resource_provider(ctx, rp)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_3c20b0b1","line":4077,"range":{"start_line":4074,"start_character":0,"end_line":4077,"end_character":53},"in_reply_to":"5f7c97a3_3c5bd055","updated":"2018-07-13 12:23:45.000000000","message":"Also, note that I was the one that originally wanted to do the whole \"send the set of change operations to perform\" approach but I was the only one who wanted that -- everyone else wanted the \"replace the world\" approach.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c1abf7dd7eb132a39a84a78e18862ab34bf949d5","unresolved":false,"context_lines":[{"line_number":4071,"context_line":"    for rp_uuid, inv_list in inventories.items():"},{"line_number":4072,"context_line":"        LOG.debug(\"reshaping: attempting inventory replacement for \""},{"line_number":4073,"context_line":"                  \"provider %s\", rp_uuid)"},{"line_number":4074,"context_line":"        # Before attempting to reshape any inventory, we first need to clear"},{"line_number":4075,"context_line":"        # allocations on the resource providers in question. The reason for"},{"line_number":4076,"context_line":"        # this is that we will get InventoryInUse exceptions below when we try"},{"line_number":4077,"context_line":"        # to update the provider\u0027s inventory records."},{"line_number":4078,"context_line":"        rp \u003d inv_list[0].resource_provider"},{"line_number":4079,"context_line":"        try:"},{"line_number":4080,"context_line":"            rp_allocs \u003d AllocationList.get_all_by_resource_provider(ctx, rp)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_3c5bd055","line":4077,"range":{"start_line":4074,"start_character":0,"end_line":4077,"end_character":53},"in_reply_to":"5f7c97a3_9f50e461","updated":"2018-07-13 12:22:05.000000000","message":"\u003e I don\u0027t think this is the right way to go about this.  If we try to\n \u003e update inventory records in an allocation-inappropriate way, it\u0027s\n \u003e because the caller f\u0027ed up by not sending an allocation move\n \u003e appropriate to their inventory shuffle. In which case we *want* to\n \u003e raise InventoryInUse.\n \u003e \n \u003e Of course this means we need to process allocation moves first\n \u003e (before the inventory moves). Which seems like the logical order\n \u003e anyway.\n \u003e \n \u003e [Later] Okay, I see; the logical order would actually be:\n \u003e \n \u003e - create new inventories\n \u003e - move allocations\n \u003e - delete old inventories\n \u003e \n \u003e Which means you have to figure out which inventories are \"new\" and\n \u003e which are \"old\"; which is doable when inventories are truly moving,\n \u003e but harder when they\u0027re being \"resized\".  However, I suspect you\n \u003e could get away with splitting it into \"inventory deletions\" (which\n \u003e you do last) and \"other\" (which you do first).\n \u003e \n \u003e I think it\u0027s worth playing around with that approach. It might end\n \u003e up being no uglier than this; it would obviate the need for the\n \u003e consumer recreate hack; and it would avoid the bug where consumer\n \u003e gens aren\u0027t being checked (see below).\n\nNo, it wouldn\u0027t. The reason is because AllocationList.create_all() blows away all allocations for all consumers involved in the allocations before recreating them entirely.\n\nSo there isn\u0027t any way to \"move\" allocations. :(","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"86f1b6f37fd38a2ae3921c4a3396d7fcfc4a81fb","unresolved":false,"context_lines":[{"line_number":4071,"context_line":"    for rp_uuid, inv_list in inventories.items():"},{"line_number":4072,"context_line":"        LOG.debug(\"reshaping: attempting inventory replacement for \""},{"line_number":4073,"context_line":"                  \"provider %s\", rp_uuid)"},{"line_number":4074,"context_line":"        # Before attempting to reshape any inventory, we first need to clear"},{"line_number":4075,"context_line":"        # allocations on the resource providers in question. The reason for"},{"line_number":4076,"context_line":"        # this is that we will get InventoryInUse exceptions below when we try"},{"line_number":4077,"context_line":"        # to update the provider\u0027s inventory records."},{"line_number":4078,"context_line":"        rp \u003d inv_list[0].resource_provider"},{"line_number":4079,"context_line":"        try:"},{"line_number":4080,"context_line":"            rp_allocs \u003d AllocationList.get_all_by_resource_provider(ctx, rp)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_60fbe47a","line":4077,"range":{"start_line":4074,"start_character":0,"end_line":4077,"end_character":53},"in_reply_to":"5f7c97a3_9f50e461","updated":"2018-07-13 02:23:20.000000000","message":"\u003e no uglier than this\n\nAw hell dude, I totally didn\u0027t mean that the way it came out. I just meant... You know what I meant. /me quits digging","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":4100,"context_line":"    if allocations:"},{"line_number":4101,"context_line":"        # NOTE(jaypipes): The above inventory replacements will have"},{"line_number":4102,"context_line":"        # incremented the resource provider generations, so we need to look in"},{"line_number":4103,"context_line":"        # the AllocationList and update any resource provider generation for"},{"line_number":4104,"context_line":"        # that provider."},{"line_number":4105,"context_line":"        for alloc in allocations:"},{"line_number":4106,"context_line":"            rp_uuid \u003d alloc.resource_provider.uuid"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_1feb747b","line":4103,"range":{"start_line":4103,"start_character":33,"end_line":4103,"end_character":72},"updated":"2018-07-12 22:52:54.000000000","message":"This is not what you\u0027re doing (but it is *why* you\u0027re doing it).","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"07c5873018ec8f686823cf06f7a6b16919fd4d98","unresolved":false,"context_lines":[{"line_number":4100,"context_line":"    if allocations:"},{"line_number":4101,"context_line":"        # NOTE(jaypipes): The above inventory replacements will have"},{"line_number":4102,"context_line":"        # incremented the resource provider generations, so we need to look in"},{"line_number":4103,"context_line":"        # the AllocationList and update any resource provider generation for"},{"line_number":4104,"context_line":"        # that provider."},{"line_number":4105,"context_line":"        for alloc in allocations:"},{"line_number":4106,"context_line":"            rp_uuid \u003d alloc.resource_provider.uuid"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ff454a45","line":4103,"range":{"start_line":4103,"start_character":33,"end_line":4103,"end_character":72},"in_reply_to":"5f7c97a3_1feb747b","updated":"2018-07-13 12:32:51.000000000","message":"Done","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":4119,"context_line":"            if alloc.consumer.uuid not in consumers:"},{"line_number":4120,"context_line":"                consumers[alloc.consumer.uuid] \u003d alloc.consumer"},{"line_number":4121,"context_line":""},{"line_number":4122,"context_line":"        # Create all the original consumer records that were auto-deleted"},{"line_number":4123,"context_line":"        # earlier. Make sure they don\u0027t use the supplied consumer generation,"},{"line_number":4124,"context_line":"        # since we now have to start over with a new consumer record."},{"line_number":4125,"context_line":"        for consumer in consumers.values():"},{"line_number":4126,"context_line":"            new_consumer \u003d consumer_obj.Consumer("},{"line_number":4127,"context_line":"                ctx, uuid\u003dconsumer.uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_3fbe7064","line":4124,"range":{"start_line":4122,"start_character":0,"end_line":4124,"end_character":69},"updated":"2018-07-12 22:52:54.000000000","message":"I think this therefore skips the generation checking for the incoming consumer records entirely. (And means that from the API user\u0027s standpoint, a reshape operation will set all affected consumers\u0027 gens back to zero, which kinda sucks and might even open races.)\n\n[Later] Yup, see comment in the test suite.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"07c5873018ec8f686823cf06f7a6b16919fd4d98","unresolved":false,"context_lines":[{"line_number":4119,"context_line":"            if alloc.consumer.uuid not in consumers:"},{"line_number":4120,"context_line":"                consumers[alloc.consumer.uuid] \u003d alloc.consumer"},{"line_number":4121,"context_line":""},{"line_number":4122,"context_line":"        # Create all the original consumer records that were auto-deleted"},{"line_number":4123,"context_line":"        # earlier. Make sure they don\u0027t use the supplied consumer generation,"},{"line_number":4124,"context_line":"        # since we now have to start over with a new consumer record."},{"line_number":4125,"context_line":"        for consumer in consumers.values():"},{"line_number":4126,"context_line":"            new_consumer \u003d consumer_obj.Consumer("},{"line_number":4127,"context_line":"                ctx, uuid\u003dconsumer.uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ffe1ca2c","line":4124,"range":{"start_line":4122,"start_character":0,"end_line":4124,"end_character":69},"in_reply_to":"5f7c97a3_3fbe7064","updated":"2018-07-13 12:32:51.000000000","message":"The strategy that folks wanted was \"replace the world\". It wasn\u0027t \"determine the change operations that were needed and perform those operations\". I fail to see how a caller of a \"replace the world\" operation would care that consumer generations of consumers involved in the \"replace the world\" operation changed to something different than before they replaced the world.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"094d24e056dcd3728c47baf5a86290607d6bf62e","unresolved":false,"context_lines":[{"line_number":4050,"context_line":"           avoid calling two functions that are already decorated with a"},{"line_number":4051,"context_line":"           transaction context manager from a function that *isn\u0027t* decorated"},{"line_number":4052,"context_line":"           with the transaction context manager if we want all changes involved"},{"line_number":4053,"context_line":"           in the sub-functions to operate within a single DB transaction."},{"line_number":4054,"context_line":""},{"line_number":4055,"context_line":"    :param ctx: `nova.api.openstack.placement.context.RequestContext` object"},{"line_number":4056,"context_line":"                containing the DB transaction context."}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_3d25d8bf","line":4053,"updated":"2018-07-16 22:09:29.000000000","message":"Thanks for adding this note.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"f939d9836d5eb7e7ae6f3d5cbc8e05e79b3abaa1","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_6162ea17","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"updated":"2018-07-15 11:12:44.000000000","message":"Does this mean the appropriate exception is raised within this function?\nIf yes, I\u0027d like to see the tests for the cases where\n\n* Not enough resource is moved for the moved allocation\n* Too much or all resource is moved while allocations remain\n\nI\u0027m a bit concerned whether we are raising an exception for the latter case here.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"e7de5f5f96df56b0eefd7268ee35bd6bcfdc944b","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_8b9701eb","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"in_reply_to":"5f7c97a3_28127356","updated":"2018-07-17 01:08:35.000000000","message":"\u003e There\u0027s also: send a payload where we *reduce* an inventory to below whatever is already allocated to it\n\nYeah, I just wanted to know if this case raises an exception, but actually it doesn\u0027t any more than PUT /resouce_providers/{u}/inventories doesn\u0027t. I just wanted to make clear because the note says like it always go into an exception error state. I feel better if this sentence is something more accurate like \"we\u0027ll get an appropriate exception or get warnings.\"\n\n\u003e I was thinking we should put them into the gabbits in the next patch; would that be acceptable?\n\nSure, thanks!","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"607cb057e9e8c712985c0da1f7ce6f1f1d91f54d","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_95a8a078","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"in_reply_to":"5f7c97a3_6162ea17","updated":"2018-07-15 17:46:47.000000000","message":"\u003e * Not enough resource is moved for the moved allocation\n\nThis one should bounce 409 (InvalidAllocationCapacityExceeded), just like if you PUT /allocations/{c} when there\u0027s not enough inventory.\n\n \u003e * Too much or all resource is moved while allocations remain\n\n\"Too much\" (but not all) will be allowed. Just like you\u0027re allowed to PUT /resource_providers/{u}/inventories in such a way that your allocations end up \"overcommitted\"; and subsequent allocations will be disallowed until you\u0027re back out of debt.\n\nMoving *all* the resource (deleting all of a resource class) should bounce 409 (InventoryInUse) just like if you try to PUT/DELETE /resource_providers/{u}/inventories with allocations.\n\n \u003e I\u0027d like to see the tests for the cases\n\nI was thinking we should put them into the gabbits in the next patch; would that be acceptable?","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"21ddacf09864db86229643bd6699f22c0f864fca","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_cfdd1840","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"in_reply_to":"5f7c97a3_95a8a078","updated":"2018-07-16 11:20:48.000000000","message":"\u003e \u003e * Too much or all resource is moved while allocations remain\n \u003e \n \u003e \"Too much\" (but not all) will be allowed. Just like you\u0027re allowed\n \u003e to PUT /resource_providers/{u}/inventories in such a way that your\n \u003e allocations end up \"overcommitted\"; and subsequent allocations will\n \u003e be disallowed until you\u0027re back out of debt.\n\nSo there are overcommit cases that this function allows. Therefore the  comment Tetsuro is highlighted is misleading.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6c8bdd6fb44c10d7ce2fdaab2d6841d78b63ffe0","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_28127356","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"in_reply_to":"5f7c97a3_a57b0e78","updated":"2018-07-16 22:55:17.000000000","message":"\u003e If I understand correctly, the allocations.replace_all() call at\n \u003e L4096 will raise InvalidAllocationCapacityExceeded if the state is\n \u003e overcommitted when it\u0027s called, without having to wait for a\n \u003e subsequent PUT allocations request.\n\nThat\u0027s exactly what we want, and is why I wrote this algorithm to make sure all of the *new* inventories are in place before we make that call. If the API client sent new allocations that exceed what it sent for the new inventories, we *want* to raise InvalidAllocationCapacityExceeded, and percolate its corresponding 409 all the way back to the client. So...\n\n \u003e I\u0027m not sure whether that means\n \u003e the test case for this scenario cannot be accomplished with a\n \u003e gabbit.\n\n...the test case would be to send a payload with one or more (target) allocations that *exceed* one or more inventories (in the target and/or already existing elsewhere), and assert that we get the proper 409 back.\n\nThere\u0027s also: send a payload where we *reduce* an inventory to below whatever is already allocated to it, but don\u0027t change the allocations on that resource via the payload. That one should succeed, but wind us up in an overcommitted state.\n\nAnd: send a payload where we *eliminate* an inventory, but without moving (all of) its allocations off of it. That one should bounce 409 with the InventoryInUse error.\n\nEtc.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"094d24e056dcd3728c47baf5a86290607d6bf62e","unresolved":false,"context_lines":[{"line_number":4072,"context_line":"    # (in their original form)."},{"line_number":4073,"context_line":"    # Note that this can cause us to end up with an interim situation where we"},{"line_number":4074,"context_line":"    # have modified an inventory to have less capacity than is currently"},{"line_number":4075,"context_line":"    # allocated, but that\u0027s allowed by the code. If the final picture is"},{"line_number":4076,"context_line":"    # overcommitted, we\u0027ll get an appropriate exception when we replace the"},{"line_number":4077,"context_line":"    # allocations at the end."},{"line_number":4078,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"},{"line_number":4079,"context_line":"        LOG.debug(\"reshaping: *interim* inventory replacement for provider %s\","},{"line_number":4080,"context_line":"                  rp_uuid)"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_a57b0e78","line":4077,"range":{"start_line":4075,"start_character":49,"end_line":4077,"end_character":29},"in_reply_to":"5f7c97a3_cfdd1840","updated":"2018-07-16 22:09:29.000000000","message":"If I understand correctly, the allocations.replace_all() call at L4096 will raise InvalidAllocationCapacityExceeded if the state is overcommitted when it\u0027s called, without having to wait for a subsequent PUT allocations request. I\u0027m not sure whether that means the test case for this scenario cannot be accomplished with a gabbit.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"038613207b4788037f324f25a4de458fc8164ac4","unresolved":false,"context_lines":[{"line_number":4093,"context_line":""},{"line_number":4094,"context_line":"    # Now we can replace all the allocations"},{"line_number":4095,"context_line":"    LOG.debug(\"reshaping: attempting allocation replacement\")"},{"line_number":4096,"context_line":"    allocations.replace_all()"},{"line_number":4097,"context_line":""},{"line_number":4098,"context_line":"    # And finally, we can set the inventories to their actual desired state."},{"line_number":4099,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_d354c658","line":4096,"updated":"2018-07-17 12:28:29.000000000","message":"A generation conflict can happen here. Prior to reshaper being called the Allocation and Inventory objects in the provided data have resource provider objects with generations on them.\n\nIn rp.set_inventory(), above, the generation gets increased for each of the resource providers.\n\nNow the resource provider objects in `allocations` have stale generations and we conflict because the generation we provide no longer matches a row.\n\nPresumably, this problem isn\u0027t showing up in the tests because the same ResourceProviders are being used in both the Inventory objects and the Allocation objects, and the change for the Inventory is \"leaking\" into the Allocations.\n\nThe code in https://review.openstack.org/#/c/576927/ doesn\u0027t reuse the same objects (because it is reusing separate code from the inventory and allocation handlers).\n\nThe new handler code could probably be changed so that it too is using the same rp objects but that has what strikes me as at leas two problems:\n\n* We can no longer reuse the existing code as directly and easily as we currently do.\n* This reshape method leaks some expectations and creates a coupling between the inventories and allocations parameters which is not made obvious by the signature and creates awkwardness.\n\nA quick hack might be to inject a loop before the call to replace_all which \"fixes\" the resource provider objects in the allocations with any found in the inventories.\n\nThoughts?","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"1604ed94e57022789b87877e9121f1e78af75f2d","unresolved":false,"context_lines":[{"line_number":4093,"context_line":""},{"line_number":4094,"context_line":"    # Now we can replace all the allocations"},{"line_number":4095,"context_line":"    LOG.debug(\"reshaping: attempting allocation replacement\")"},{"line_number":4096,"context_line":"    allocations.replace_all()"},{"line_number":4097,"context_line":""},{"line_number":4098,"context_line":"    # And finally, we can set the inventories to their actual desired state."},{"line_number":4099,"context_line":"    for rp_uuid, new_inv_list in inventories.items():"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_fcc98faa","line":4096,"in_reply_to":"5f7c97a3_d354c658","updated":"2018-07-17 14:00:03.000000000","message":"#agreed in IRC [1] to restore the loop from PS1 L4100-4108 [2] so that the allocations side is sharing the rp objects with the inventory side.\n\n[1] http://eavesdrop.openstack.org/irclogs/%23openstack-placement/%23openstack-placement.2018-07-17.log.html#t2018-07-17T12:30:22\n[2] https://review.openstack.org/#/c/582383/1/nova/api/openstack/placement/objects/resource_provider.py@4100","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"8b6bb694d8c5ff0369022e8f22149b8e63178127","unresolved":false,"context_lines":[{"line_number":4098,"context_line":"        rp.set_inventory(InventoryList(objects\u003dlist(inv_by_rc.values())))"},{"line_number":4099,"context_line":"        affected_providers[rp_uuid] \u003d rp"},{"line_number":4100,"context_line":""},{"line_number":4101,"context_line":"    # NOTE(jaypipes): The above inventory replacements will have"},{"line_number":4102,"context_line":"    # incremented the resource provider generations, so we need to look in"},{"line_number":4103,"context_line":"    # the AllocationList and swap the resource provider object with the one we"},{"line_number":4104,"context_line":"    # saved above that has the updated provider generation in it."},{"line_number":4105,"context_line":"    for alloc in allocations:"},{"line_number":4106,"context_line":"        rp_uuid \u003d alloc.resource_provider.uuid"},{"line_number":4107,"context_line":"        if rp_uuid in affected_providers:"},{"line_number":4108,"context_line":"            alloc.resource_provider \u003d affected_providers[rp_uuid]"},{"line_number":4109,"context_line":""},{"line_number":4110,"context_line":"    # Now we can replace all the allocations"},{"line_number":4111,"context_line":"    LOG.debug(\"reshaping: attempting allocation replacement\")"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_07cf54b3","line":4108,"range":{"start_line":4101,"start_character":0,"end_line":4108,"end_character":65},"updated":"2018-07-17 14:32:34.000000000","message":"cool","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"3ea013fbc23ba2ca99ca374e79aab19361fa6401","unresolved":false,"context_lines":[{"line_number":4098,"context_line":"        rp.set_inventory(InventoryList(objects\u003dlist(inv_by_rc.values())))"},{"line_number":4099,"context_line":"        affected_providers[rp_uuid] \u003d rp"},{"line_number":4100,"context_line":""},{"line_number":4101,"context_line":"    # NOTE(jaypipes): The above inventory replacements will have"},{"line_number":4102,"context_line":"    # incremented the resource provider generations, so we need to look in"},{"line_number":4103,"context_line":"    # the AllocationList and swap the resource provider object with the one we"},{"line_number":4104,"context_line":"    # saved above that has the updated provider generation in it."},{"line_number":4105,"context_line":"    for alloc in allocations:"},{"line_number":4106,"context_line":"        rp_uuid \u003d alloc.resource_provider.uuid"},{"line_number":4107,"context_line":"        if rp_uuid in affected_providers:"},{"line_number":4108,"context_line":"            alloc.resource_provider \u003d affected_providers[rp_uuid]"},{"line_number":4109,"context_line":""},{"line_number":4110,"context_line":"    # Now we can replace all the allocations"},{"line_number":4111,"context_line":"    LOG.debug(\"reshaping: attempting allocation replacement\")"}],"source_content_type":"text/x-python","patch_set":4,"id":"5f7c97a3_d29349a3","line":4108,"range":{"start_line":4101,"start_character":0,"end_line":4108,"end_character":65},"updated":"2018-07-18 08:15:53.000000000","message":"We know that the above set_inventory() call bumped the RP generation. But we replace the whole provider here not just set the new generation. In theory the incoming inv.resource_provider and the alloc.resource_provider could differ in content. However looking at the handler code[1] I think that will lead to a conflict at set_inventories() so we are safe.\n\n[1]https://review.openstack.org/#/c/576927/10/nova/api/openstack/placement/handlers/reshaper.py@82","commit_id":"d7315a0f5a56ba627ec629869e4222c5792b7b54"}],"nova/tests/functional/api/openstack/placement/db/test_reshape.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"ca7ee268cd6ab346e78c950f046829c5d665ef09","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # consumers representing the two instances above will have had their"},{"line_number":143,"context_line":"        # generations incremented in the original call to PUT"},{"line_number":144,"context_line":"        # /allocations/{consumer_uuid}"},{"line_number":145,"context_line":"        i1_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i1_uuid)"},{"line_number":146,"context_line":"        i2_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i2_uuid)"},{"line_number":147,"context_line":"        after_allocs \u003d rp_obj.AllocationList(self.ctx, objects\u003d["},{"line_number":148,"context_line":"            # instance1 gets VCPU from NUMA0, MEMORY_MB from cn1 and DISK_GB"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_9ad79251","line":145,"range":{"start_line":145,"start_character":8,"end_line":145,"end_character":74},"updated":"2018-07-12 22:52:54.000000000","message":"So yeah, I added\n\n i1_consumer.generation \u003d 99\n\nafter this line, and the test still passes.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"5dd017fbd05e888053cd75760a9520fa5249b212","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # consumers representing the two instances above will have had their"},{"line_number":143,"context_line":"        # generations incremented in the original call to PUT"},{"line_number":144,"context_line":"        # /allocations/{consumer_uuid}"},{"line_number":145,"context_line":"        i1_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i1_uuid)"},{"line_number":146,"context_line":"        i2_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i2_uuid)"},{"line_number":147,"context_line":"        after_allocs \u003d rp_obj.AllocationList(self.ctx, objects\u003d["},{"line_number":148,"context_line":"            # instance1 gets VCPU from NUMA0, MEMORY_MB from cn1 and DISK_GB"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_cb11abfa","line":145,"range":{"start_line":145,"start_character":8,"end_line":145,"end_character":74},"in_reply_to":"5f7c97a3_3fc18282","updated":"2018-07-13 19:21:18.000000000","message":"That we\u0027re not validating the incoming consumer generations.","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"24c2a379661350d59b7875927608c598cef02977","unresolved":false,"context_lines":[{"line_number":142,"context_line":"        # consumers representing the two instances above will have had their"},{"line_number":143,"context_line":"        # generations incremented in the original call to PUT"},{"line_number":144,"context_line":"        # /allocations/{consumer_uuid}"},{"line_number":145,"context_line":"        i1_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i1_uuid)"},{"line_number":146,"context_line":"        i2_consumer \u003d consumer_obj.Consumer.get_by_uuid(self.ctx, i2_uuid)"},{"line_number":147,"context_line":"        after_allocs \u003d rp_obj.AllocationList(self.ctx, objects\u003d["},{"line_number":148,"context_line":"            # instance1 gets VCPU from NUMA0, MEMORY_MB from cn1 and DISK_GB"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_3fc18282","line":145,"range":{"start_line":145,"start_character":8,"end_line":145,"end_character":74},"in_reply_to":"5f7c97a3_9ad79251","updated":"2018-07-13 12:33:32.000000000","message":"I fail to see what that proves?","commit_id":"52bd22d27b3fcfb736a020603f0fdf635038fe5c"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"21ddacf09864db86229643bd6699f22c0f864fca","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        # *before* attempting any reshape activity."},{"line_number":97,"context_line":"        cn1_numa0 \u003d self._create_provider(\u0027cn1_numa0\u0027, parent\u003dcn1.uuid)"},{"line_number":98,"context_line":"        cn1_numa1 \u003d self._create_provider(\u0027cn1_numa1\u0027, parent\u003dcn1.uuid)"},{"line_number":99,"context_line":"        ss \u003d self._create_provider(\u0027ss\u0027)"},{"line_number":100,"context_line":"        tb.set_traits(ss, \"MISC_SHARES_VIA_AGGREGATE\")"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        # OK, now emulate the call to POST /reshaper that will be triggered by"},{"line_number":103,"context_line":"        # a virt driver wanting to replace the world and change its modeling"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_2fd92c27","line":100,"range":{"start_line":99,"start_character":1,"end_line":100,"end_character":54},"updated":"2018-07-16 11:20:48.000000000","message":"I think you missed to put cn1 and ss in the same aggregate.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"336dd35b1964b1a99ffad360391edde82aa66eb4","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        # *before* attempting any reshape activity."},{"line_number":97,"context_line":"        cn1_numa0 \u003d self._create_provider(\u0027cn1_numa0\u0027, parent\u003dcn1.uuid)"},{"line_number":98,"context_line":"        cn1_numa1 \u003d self._create_provider(\u0027cn1_numa1\u0027, parent\u003dcn1.uuid)"},{"line_number":99,"context_line":"        ss \u003d self._create_provider(\u0027ss\u0027)"},{"line_number":100,"context_line":"        tb.set_traits(ss, \"MISC_SHARES_VIA_AGGREGATE\")"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        # OK, now emulate the call to POST /reshaper that will be triggered by"},{"line_number":103,"context_line":"        # a virt driver wanting to replace the world and change its modeling"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_6b6833bb","line":100,"range":{"start_line":99,"start_character":1,"end_line":100,"end_character":54},"in_reply_to":"5f7c97a3_1265411c","updated":"2018-07-18 15:16:45.000000000","message":"remove the set_traits() call in the latest rev.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"d77baa99bc0cd846000e6a5ecfe61c7f1cecd135","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        # *before* attempting any reshape activity."},{"line_number":97,"context_line":"        cn1_numa0 \u003d self._create_provider(\u0027cn1_numa0\u0027, parent\u003dcn1.uuid)"},{"line_number":98,"context_line":"        cn1_numa1 \u003d self._create_provider(\u0027cn1_numa1\u0027, parent\u003dcn1.uuid)"},{"line_number":99,"context_line":"        ss \u003d self._create_provider(\u0027ss\u0027)"},{"line_number":100,"context_line":"        tb.set_traits(ss, \"MISC_SHARES_VIA_AGGREGATE\")"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        # OK, now emulate the call to POST /reshaper that will be triggered by"},{"line_number":103,"context_line":"        # a virt driver wanting to replace the world and change its modeling"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_78f92bc3","line":100,"range":{"start_line":99,"start_character":1,"end_line":100,"end_character":54},"in_reply_to":"5f7c97a3_2fd92c27","updated":"2018-07-16 14:02:20.000000000","message":"It actually doesn\u0027t matter at all. I could leave off the trait setting entirely; I just added it here for a bit of context.\n\nThe only thing that needs the traits and aggregate associations is the GET /allocation_candidates. But POST /reshaper doesn\u0027t do any attempt to determine if something is shared or not shared, associated or not with any aggregate. It just basically (re)writes inventory and allocation records. That\u0027s it.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"3ea013fbc23ba2ca99ca374e79aab19361fa6401","unresolved":false,"context_lines":[{"line_number":96,"context_line":"        # *before* attempting any reshape activity."},{"line_number":97,"context_line":"        cn1_numa0 \u003d self._create_provider(\u0027cn1_numa0\u0027, parent\u003dcn1.uuid)"},{"line_number":98,"context_line":"        cn1_numa1 \u003d self._create_provider(\u0027cn1_numa1\u0027, parent\u003dcn1.uuid)"},{"line_number":99,"context_line":"        ss \u003d self._create_provider(\u0027ss\u0027)"},{"line_number":100,"context_line":"        tb.set_traits(ss, \"MISC_SHARES_VIA_AGGREGATE\")"},{"line_number":101,"context_line":""},{"line_number":102,"context_line":"        # OK, now emulate the call to POST /reshaper that will be triggered by"},{"line_number":103,"context_line":"        # a virt driver wanting to replace the world and change its modeling"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_1265411c","line":100,"range":{"start_line":99,"start_character":1,"end_line":100,"end_character":54},"in_reply_to":"5f7c97a3_78f92bc3","updated":"2018-07-18 08:15:53.000000000","message":"You are right aggregates and traits are not changed during reshape.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d67c8fdd8ee09e031f79feb02572a8e414e4b67c","unresolved":false,"context_lines":[{"line_number":168,"context_line":"                self.ctx, resource_provider\u003dss, resource_class\u003d\u0027DISK_GB\u0027,"},{"line_number":169,"context_line":"                consumer\u003di2_consumer, used\u003d100),"},{"line_number":170,"context_line":"        ])"},{"line_number":171,"context_line":"        rp_obj.reshape(self.ctx, after_inventories, after_allocs)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"        # Verify we have the allocations we expect for the AFTER scenario"},{"line_number":174,"context_line":"        after_allocs_i1 \u003d rp_obj.AllocationList.get_all_by_consumer_id("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_409d7207","line":171,"updated":"2018-07-13 22:32:17.000000000","message":"@Jay: So what I did here was:\n\n- Bogusify ss.generation\n- Assert that reshape raises ConcurrentUpdate\n- Fix ss.generation\n\nand/or:\n\n- Bogusify i1_consumer.generation\n- Assert that reshape raises ConcurrentUpdate\n- Fix i1_consumer.generation\n\nAfter doing either/both of those, this line (the real reshape call) should succeed. But it doesn\u0027t; I get ConcurrentUpdate again, on whichever provider it happens to get to first.\n\nSo reshape isn\u0027t rolling back gen bumps on failure.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"f939d9836d5eb7e7ae6f3d5cbc8e05e79b3abaa1","unresolved":false,"context_lines":[{"line_number":168,"context_line":"                self.ctx, resource_provider\u003dss, resource_class\u003d\u0027DISK_GB\u0027,"},{"line_number":169,"context_line":"                consumer\u003di2_consumer, used\u003d100),"},{"line_number":170,"context_line":"        ])"},{"line_number":171,"context_line":"        rp_obj.reshape(self.ctx, after_inventories, after_allocs)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"        # Verify we have the allocations we expect for the AFTER scenario"},{"line_number":174,"context_line":"        after_allocs_i1 \u003d rp_obj.AllocationList.get_all_by_consumer_id("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_642d1844","line":171,"in_reply_to":"5f7c97a3_409d7207","updated":"2018-07-15 11:12:44.000000000","message":"I had the following experiment to see what eric says.... and it seems they are rolled back in the DB, so the question is whether we have cases we re-use those rp objects after raising an exception... not sure.\n\n    $ git diff\n    diff --git a/nova/tests/functional/api/openstack/placement/db/test_reshape.py b/nova/tests/functional/api/openstack/placement/db/test_reshape.py\n    index f7a39e0..4e49b13 100644\n    --- a/nova/tests/functional/api/openstack/placement/db/test_reshape.py\n    +++ b/nova/tests/functional/api/openstack/placement/db/test_reshape.py\n    @@ -15,6 +15,7 @@ from nova.api.openstack.placement.objects import consumer as consumer_obj\n     from nova.api.openstack.placement.objects import resource_provider as rp_obj\n     from nova.tests.functional.api.openstack.placement.db import test_base as tb\n     from nova.tests import uuidsentinel as uuids\n    +from nova.api.openstack.placement import exception\n\n\n     def alloc_for_rc(alloc_list, rc):\n    @@ -168,6 +169,37 @@ class ReshapeTestCase(tb.PlacementDbBaseTestCase):\n                     self.ctx, resource_provider\u003dss, resource_class\u003d\u0027DISK_GB\u0027,\n                     consumer\u003di2_consumer, used\u003d100),\n             ])\n    +\n    +        # Let\u0027s see the current generation for each rp\n    +        self.assertEqual(cn1.generation, 5)\n    +        self.assertEqual(cn1_numa0.generation, 0)\n    +        self.assertEqual(cn1_numa1.generation, 0)\n    +        self.assertEqual(ss.generation, 1)\n    +\n    +        # Let\u0027s validate that we get ConcurrencyUpdate error\n    +        # with inappropriate consumer generation\n    +        i1_consumer.generation \u003d 99\n    +        self.assertRaises(exception.ConcurrentUpdateDetected,\n    +                          rp_obj.reshape, self.ctx, after_inventories,\n    +                          after_allocs)\n    +\n    +        # Now we have bumped generation for all the rps via\n    +        # rp.set_inventory() before allocations.replace_all()\n    +        # which has actually raised ConcurrentUpdateDetected\n    +        self.assertEqual(cn1.generation, 7)\n    +        self.assertEqual(cn1_numa0.generation, 2)\n    +        self.assertEqual(cn1_numa1.generation, 2)\n    +        self.assertEqual(ss.generation, 3)\n    +\n    +        # But in the DB, they are properly rolled back,\n    +        # so we need to reset them here to pass the\n    +        # next reshape() execution.\n    +        cn1.generation \u003d 5\n    +        cn1_numa0.generation \u003d 0\n    +        cn1_numa1.generation \u003d 0\n    +        ss.generation \u003d 1\n    +\n    +        # Let\u0027s get the consumer generation fixed and try\n    +        # reshape() again. This now successes.\n    +        i1_consumer.generation \u003d 1\n             rp_obj.reshape(self.ctx, after_inventories, after_allocs)\n\n             # Verify we have the allocations we expect for the AFTER scenario","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"607cb057e9e8c712985c0da1f7ce6f1f1d91f54d","unresolved":false,"context_lines":[{"line_number":168,"context_line":"                self.ctx, resource_provider\u003dss, resource_class\u003d\u0027DISK_GB\u0027,"},{"line_number":169,"context_line":"                consumer\u003di2_consumer, used\u003d100),"},{"line_number":170,"context_line":"        ])"},{"line_number":171,"context_line":"        rp_obj.reshape(self.ctx, after_inventories, after_allocs)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"        # Verify we have the allocations we expect for the AFTER scenario"},{"line_number":174,"context_line":"        after_allocs_i1 \u003d rp_obj.AllocationList.get_all_by_consumer_id("}],"source_content_type":"text/x-python","patch_set":2,"id":"5f7c97a3_9573c04e","line":171,"in_reply_to":"5f7c97a3_642d1844","updated":"2018-07-15 17:46:47.000000000","message":"\u003e seems they are rolled back in the DB, so the question is whether we\n\nAhhh, thank you Tetsuro. That makes sense - the rollback only applies to the database, not to the objects themselves. So when e.g. any of the rp.set_inventory calls succeeds, that rp\u0027s generation is incremented *provisionally* in the database, but *permanently* on the object.\n\n \u003e have cases we re-use those rp objects after raising an exception...\n\nNo, I\u0027m totally not worried about this.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"21ddacf09864db86229643bd6699f22c0f864fca","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5f7c97a3_2f75ac00","line":205,"updated":"2018-07-16 11:20:48.000000000","message":"I would also assert the inventories are properly set after reshape. E.g. cn1 doesn\u0027t have VCPU and DISK_GB inventory.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8e68c9983eed3cc4db6b84e6919b74b10a9c9da5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5f7c97a3_195fbd75","line":205,"in_reply_to":"5f7c97a3_2f75ac00","updated":"2018-07-17 15:20:00.000000000","message":"Done","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"d77baa99bc0cd846000e6a5ecfe61c7f1cecd135","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"5f7c97a3_1886f717","line":205,"in_reply_to":"5f7c97a3_2f75ac00","updated":"2018-07-16 14:02:20.000000000","message":"ack, good suggestion.","commit_id":"136ba92dda59322f0c8b8b2eb29035a6f51c999e"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"331e9396b21463371aa2cd2cc6f4b6dd4a817aa6","unresolved":false,"context_lines":[{"line_number":134,"context_line":"                    max_unit\u003d1000, min_unit\u003d1, step_size\u003d1,"},{"line_number":135,"context_line":"                    allocation_ratio\u003d1.0),"},{"line_number":136,"context_line":"            ]),"},{"line_number":137,"context_line":"        }"},{"line_number":138,"context_line":"        # We do a fetch from the DB for each instance to get its latest"},{"line_number":139,"context_line":"        # generation. This would be done by the resource tracker or scheduler"},{"line_number":140,"context_line":"        # report client before issuing the call to reshape() because the"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_1e778d3d","line":137,"updated":"2018-07-18 07:23:59.000000000","message":"If we want to use different rp objects, why not change tests accordingly asserting here the following clones and use them in `after_allocs`? I tried this and found that this failed on PS2 and worked on PS4, which ensures that the approach on PS4 is good and necessary.\n\n        cn1_clone \u003d rp_obj.ResourceProvider.get_by_uuid(self.ctx, cn1.uuid)\n        cn1_numa0_clone \u003d rp_obj.ResourceProvider.get_by_uuid(self.ctx, cn1_numa0.uuid)\n        cn1_numa1_clone \u003d rp_obj.ResourceProvider.get_by_uuid(self.ctx, cn1_numa1.uuid)\n        ss_clone \u003d rp_obj.ResourceProvider.get_by_uuid(self.ctx, ss.uuid)","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"9f79aeeacffb0ef270162225873df70562e2426a","unresolved":false,"context_lines":[{"line_number":134,"context_line":"                    max_unit\u003d1000, min_unit\u003d1, step_size\u003d1,"},{"line_number":135,"context_line":"                    allocation_ratio\u003d1.0),"},{"line_number":136,"context_line":"            ]),"},{"line_number":137,"context_line":"        }"},{"line_number":138,"context_line":"        # We do a fetch from the DB for each instance to get its latest"},{"line_number":139,"context_line":"        # generation. This would be done by the resource tracker or scheduler"},{"line_number":140,"context_line":"        # report client before issuing the call to reshape() because the"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_eb42033c","line":137,"in_reply_to":"5f7c97a3_1e778d3d","updated":"2018-07-18 15:15:31.000000000","message":"I decided to test the generation concurrent update failures in a separate test (below). You will note on 342 that I\u0027m creating new resource provider objects to test the concurrent update detected issues.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"f3b05c2cc1237cc17229ca0f6d8af2bf32f47bb9","unresolved":false,"context_lines":[{"line_number":241,"context_line":"        involved in the reshape transaction. We should get a"},{"line_number":242,"context_line":"        ConcurrentUpdateDetected in this case."},{"line_number":243,"context_line":"        \"\"\""},{"line_number":244,"context_line":"        # First create our consumers"},{"line_number":245,"context_line":"        i1_uuid \u003d uuids.instance1"},{"line_number":246,"context_line":"        i1_consumer \u003d consumer_obj.Consumer("},{"line_number":247,"context_line":"            self.ctx, uuid\u003di1_uuid, user\u003dself.user_obj,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_8d694327","line":244,"updated":"2018-07-17 15:59:08.000000000","message":"These first 90LOC or so are common with the previous test case, should be factored out.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"331e9396b21463371aa2cd2cc6f4b6dd4a817aa6","unresolved":false,"context_lines":[{"line_number":313,"context_line":"                    max_unit\u003d1000, min_unit\u003d1, step_size\u003d1,"},{"line_number":314,"context_line":"                    allocation_ratio\u003d1.0),"},{"line_number":315,"context_line":"            ]),"},{"line_number":316,"context_line":"        }"},{"line_number":317,"context_line":"        # We do a fetch from the DB for each instance to get its latest"},{"line_number":318,"context_line":"        # generation. This would be done by the resource tracker or scheduler"},{"line_number":319,"context_line":"        # report client before issuing the call to reshape() because the"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_3edfe90b","line":316,"updated":"2018-07-18 07:23:59.000000000","message":"ditto (or we should factor it out as efried says)","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"9f79aeeacffb0ef270162225873df70562e2426a","unresolved":false,"context_lines":[{"line_number":313,"context_line":"                    max_unit\u003d1000, min_unit\u003d1, step_size\u003d1,"},{"line_number":314,"context_line":"                    allocation_ratio\u003d1.0),"},{"line_number":315,"context_line":"            ]),"},{"line_number":316,"context_line":"        }"},{"line_number":317,"context_line":"        # We do a fetch from the DB for each instance to get its latest"},{"line_number":318,"context_line":"        # generation. This would be done by the resource tracker or scheduler"},{"line_number":319,"context_line":"        # report client before issuing the call to reshape() because the"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_ab496b66","line":316,"in_reply_to":"5f7c97a3_3edfe90b","updated":"2018-07-18 15:15:31.000000000","message":"ack, I can refactor them out in a followup.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"8b6bb694d8c5ff0369022e8f22149b8e63178127","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # another thread updating one of the involved provider\u0027s generations"},{"line_number":357,"context_line":"        self.assertRaises("},{"line_number":358,"context_line":"            exception.ConcurrentUpdateDetected,"},{"line_number":359,"context_line":"            rp_obj.reshape, self.ctx, after_inventories, after_allocs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_e7f018ff","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":26},"updated":"2018-07-17 14:32:34.000000000","message":"Soooo... is this just effectively calling reshape with the wrong generations? Like, did we really have to do all that setup \"emulating\" another thread?","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4f4540c4d2131fa89b76455d6e96296a09f8bfd6","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # another thread updating one of the involved provider\u0027s generations"},{"line_number":357,"context_line":"        self.assertRaises("},{"line_number":358,"context_line":"            exception.ConcurrentUpdateDetected,"},{"line_number":359,"context_line":"            rp_obj.reshape, self.ctx, after_inventories, after_allocs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_ed0c77ec","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":26},"in_reply_to":"5f7c97a3_42a45a4c","updated":"2018-07-17 15:44:18.000000000","message":"oh, I see. yeah, I guess I wanted to be a bit more explicit and realistic, so instead of making one of the provider\u0027s generations bogus, I actually went ahead and created a new variable and updated that new provider variable\u0027s inventory","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"80a22f7825bfbf9395ad1be5b9754461a9faa1b8","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # another thread updating one of the involved provider\u0027s generations"},{"line_number":357,"context_line":"        self.assertRaises("},{"line_number":358,"context_line":"            exception.ConcurrentUpdateDetected,"},{"line_number":359,"context_line":"            rp_obj.reshape, self.ctx, after_inventories, after_allocs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_42a45a4c","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":26},"in_reply_to":"5f7c97a3_62e956a9","updated":"2018-07-17 15:26:21.000000000","message":"Sorry, my point was that the stuff on L338-353 could be replaced by setting ss.generation to something bogus. But this is fine too.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"8e68c9983eed3cc4db6b84e6919b74b10a9c9da5","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # another thread updating one of the involved provider\u0027s generations"},{"line_number":357,"context_line":"        self.assertRaises("},{"line_number":358,"context_line":"            exception.ConcurrentUpdateDetected,"},{"line_number":359,"context_line":"            rp_obj.reshape, self.ctx, after_inventories, after_allocs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_62e956a9","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":26},"in_reply_to":"5f7c97a3_e7f018ff","updated":"2018-07-17 15:20:00.000000000","message":"I like to keep failure scenarios in separate test cases in order to prevent side-effects from leaking into the test scenario.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"f3b05c2cc1237cc17229ca0f6d8af2bf32f47bb9","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        # another thread updating one of the involved provider\u0027s generations"},{"line_number":357,"context_line":"        self.assertRaises("},{"line_number":358,"context_line":"            exception.ConcurrentUpdateDetected,"},{"line_number":359,"context_line":"            rp_obj.reshape, self.ctx, after_inventories, after_allocs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_6d17079f","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":26},"in_reply_to":"5f7c97a3_ed0c77ec","updated":"2018-07-17 15:59:08.000000000","message":"Yup, I dig it.  And at first I had thought it was a lot more of the code doing that, but it\u0027s just a few lines.","commit_id":"0ffa445665c961fba076e77ce1bd21d1dc1ca846"}]}