)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove check_attach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch is an experiment to remove the \u0027check_attach\u0027"},{"line_number":10,"context_line":"call from Nova completely and let Cinder to perform the required"},{"line_number":11,"context_line":"checks as part of the \u0027reserve_volume\u0027 call."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":17,"id":"5a8bc5a2_f14bc25d","line":9,"updated":"2016-09-28 02:02:50.000000000","message":"Is it still an experiment if it\u0027s fixing a bug? I think we might want to reword this.\n\nYou should probably explain a bit that the os-reserve API in Cinder is doing the same status checking that check_attach was doing, and the point of removing check_attach is to let Cinder handle the state checking since it\u0027s the \"source of truth\".\n\nYou\u0027ll also want to mention the need for the minimum nova-compute service version bump.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove check_attach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch is an experiment to remove the \u0027check_attach\u0027"},{"line_number":10,"context_line":"call from Nova completely and let Cinder to perform the required"},{"line_number":11,"context_line":"checks as part of the \u0027reserve_volume\u0027 call."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":17,"id":"1a95cdbc_d36878bd","line":9,"in_reply_to":"5a8bc5a2_f14bc25d","updated":"2016-10-05 10:28:14.000000000","message":"Done","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove check_attach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch finishes to remove the \u0027check_attach\u0027 call from Nova"},{"line_number":10,"context_line":"completely. As Cinder already performs the required checks as part"},{"line_number":11,"context_line":"of the \u0027reserve_volume\u0027 (os-reserve) call it is unnecessary to check the"},{"line_number":12,"context_line":"statemachine in Nova also and it can lead to race conditions."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"da36d5c6_0182b500","line":9,"updated":"2017-02-18 17:56:25.000000000","message":"s/finishes to remove/finishes removing/","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Remove check_attach"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"This patch finishes to remove the \u0027check_attach\u0027 call from Nova"},{"line_number":10,"context_line":"completely. As Cinder already performs the required checks as part"},{"line_number":11,"context_line":"of the \u0027reserve_volume\u0027 (os-reserve) call it is unnecessary to check the"},{"line_number":12,"context_line":"statemachine in Nova also and it can lead to race conditions."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"da36d5c6_17b037f4","line":9,"in_reply_to":"da36d5c6_0182b500","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":14,"context_line":"Due to the missing \u0027reserve_volume\u0027 call in the BFV flow we need to"},{"line_number":15,"context_line":"bump the minimum compute version. The change is required as the"},{"line_number":16,"context_line":"compute manager has to be updated in order to perform"},{"line_number":17,"context_line":"\u0027unreserve_volume\u0027 in case of a failure to avoid the volume gets"},{"line_number":18,"context_line":"stuck in \u0027attaching\u0027 state. The Nova API performs the \u0027reserve_volume\u0027"},{"line_number":19,"context_line":"call instead of the \u0027check_attach\u0027 functionality only in case of the"},{"line_number":20,"context_line":"computes are all updated to the high enough version."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"da36d5c6_2187b910","line":17,"updated":"2017-02-18 17:56:25.000000000","message":"s/volume gets/volume getting/","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":16,"context_line":"compute manager has to be updated in order to perform"},{"line_number":17,"context_line":"\u0027unreserve_volume\u0027 in case of a failure to avoid the volume gets"},{"line_number":18,"context_line":"stuck in \u0027attaching\u0027 state. The Nova API performs the \u0027reserve_volume\u0027"},{"line_number":19,"context_line":"call instead of the \u0027check_attach\u0027 functionality only in case of the"},{"line_number":20,"context_line":"computes are all updated to the high enough version."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1581230"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"da36d5c6_a173a94c","line":19,"updated":"2017-02-18 17:56:25.000000000","message":"s/only in case of the computes are all updated to the high enough version/only when all computes have been updated to understand the new microversion/","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":16,"context_line":"compute manager has to be updated in order to perform"},{"line_number":17,"context_line":"\u0027unreserve_volume\u0027 in case of a failure to avoid the volume gets"},{"line_number":18,"context_line":"stuck in \u0027attaching\u0027 state. The Nova API performs the \u0027reserve_volume\u0027"},{"line_number":19,"context_line":"call instead of the \u0027check_attach\u0027 functionality only in case of the"},{"line_number":20,"context_line":"computes are all updated to the high enough version."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-Bug: #1581230"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":38,"id":"da36d5c6_c15fbd73","line":19,"in_reply_to":"da36d5c6_a173a94c","updated":"2017-02-18 18:32:09.000000000","message":"*there isn\u0027t actually a microversion change here, just the nova-compute service version.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":11,"context_line":"of the \u0027reserve_volume\u0027 (os-reserve) call it is unnecessary to check the"},{"line_number":12,"context_line":"statemachine in Nova also and it can lead to race conditions."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The missing \u0027reserve_volume\u0027 call is added to the BFV flow. In case of"},{"line_number":15,"context_line":"the volume will be locked in \u0027attaching\u0027 state until the instance"},{"line_number":16,"context_line":"in ERROR state is cleaned up."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"We also check AZ for each volume attach operation which we haven\u0027t"},{"line_number":19,"context_line":"done for unshelve. A release note is added to enable \u0027cross_az_attach\u0027"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":46,"id":"da36d5c6_7a57bc57","line":16,"range":{"start_line":14,"start_character":60,"end_line":16,"end_character":29},"updated":"2017-02-26 16:26:52.000000000","message":"This seems to be missing something, maybe it should say \"In case of build failure, the volume will be locked...\"?","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":11,"context_line":"of the \u0027reserve_volume\u0027 (os-reserve) call it is unnecessary to check the"},{"line_number":12,"context_line":"statemachine in Nova also and it can lead to race conditions."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"The missing \u0027reserve_volume\u0027 call is added to the BFV flow. In case of"},{"line_number":15,"context_line":"the volume will be locked in \u0027attaching\u0027 state until the instance"},{"line_number":16,"context_line":"in ERROR state is cleaned up."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"We also check AZ for each volume attach operation which we haven\u0027t"},{"line_number":19,"context_line":"done for unshelve. A release note is added to enable \u0027cross_az_attach\u0027"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":46,"id":"da36d5c6_500bb923","line":16,"range":{"start_line":14,"start_character":60,"end_line":16,"end_character":29},"in_reply_to":"da36d5c6_7a57bc57","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"}],"nova/compute/api.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1e5ddf2d35812f48e8a7c26c0abb40896f1966cc","unresolved":false,"context_lines":[{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_6e9fb55f","line":1290,"updated":"2016-07-25 10:23:38.000000000","message":"I think you should instead call _check_attach_and_reserve_volume, so we share code with the other cases.\n\nThe only change is to return the volume from the _check_attach_and_reserve_volume method.","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a38183aa08fe0f4ec856593100284861b2aeb6d7","unresolved":false,"context_lines":[{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_afe7d4e2","line":1290,"in_reply_to":"dada55a8_6e9fb55f","updated":"2016-07-25 17:49:35.000000000","message":"My biggest question here is that why it wasn\u0027t called here originally? Do you know? As in case of attaching a volume at boot time \u0027reserve_volume\u0027 is not called at all in the current code base. I would guess someone missed to add it, when it was implemented and it never really caused a big issue, but I\u0027m not sure.\n\nSo if we think we can safely use that call here as well, I\u0027m happy to do it. I wanted to have feedback first as this case is different from the others and I hoped someone has an idea why.\n\nBesides my question above it\u0027s a good catch, it would\u0027ve been easier to just add that call here, but I changed this part iteratively and it finally didn\u0027t come to mind. :)","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1e5ddf2d35812f48e8a7c26c0abb40896f1966cc","unresolved":false,"context_lines":[{"line_number":2985,"context_line":"                device_type\u003ddevice_type)"},{"line_number":2986,"context_line":"        return volume_bdm"},{"line_number":2987,"context_line":""},{"line_number":2988,"context_line":"    def _check_attach_and_reserve_volume(self, context, volume_id, instance):"},{"line_number":2989,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":2990,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":2991,"context_line":"                                                instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_2e95bd3f","line":2988,"updated":"2016-07-25 10:23:38.000000000","message":"See above.","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1e5ddf2d35812f48e8a7c26c0abb40896f1966cc","unresolved":false,"context_lines":[{"line_number":3132,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":3133,"context_line":"                                                instance\u003dinstance)"},{"line_number":3134,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3135,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"},{"line_number":3136,"context_line":"        try:"},{"line_number":3137,"context_line":"            self.compute_rpcapi.swap_volume("},{"line_number":3138,"context_line":"                    context, instance\u003dinstance,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_ceb221e5","line":3135,"updated":"2016-07-25 10:23:38.000000000","message":"Why do we call reserve so late here?\n\nI think we should really try to call _check_attach_and_reserve_volume here if possible.\n\nHonestly, all this logic that is here:\nhttps://github.com/openstack/nova/blob/bc22a15e4e4ffc6521c1c860316aa1e65c42f15c/nova/api/openstack/compute/volumes.py#L356\nShould really get pulled into this compute api.py method.\n\nThat should make it easier to make this consistent with the other methods.","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a38183aa08fe0f4ec856593100284861b2aeb6d7","unresolved":false,"context_lines":[{"line_number":3132,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":3133,"context_line":"                                                instance\u003dinstance)"},{"line_number":3134,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3135,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"},{"line_number":3136,"context_line":"        try:"},{"line_number":3137,"context_line":"            self.compute_rpcapi.swap_volume("},{"line_number":3138,"context_line":"                    context, instance\u003dinstance,"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_453559d8","line":3135,"in_reply_to":"dada55a8_ceb221e5","updated":"2016-07-25 17:49:35.000000000","message":"This one is a bit of a mess. I think it triple checks almost everything here. For the multiattach code I tried to refactor check_attach to work well and remove a few checks here, but it turned out it raises exceptions in a different order than any other part of the code and I think that was the part I started to give it up...\n\nIf we think this should be changed to use the regularly \u0027_check_attach_and_reserve_volume\u0027 I might do it in a follow up patch as that\u0027s not really a part of removing \u0027check_attach\u0027 and then we might look into how to make this one more sane if possible. What do you think?","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4ac54bfe02cde98f98e677a63118e17606c8b82a","unresolved":false,"context_lines":[{"line_number":1284,"context_line":"                        \"size specified\"))"},{"line_number":1285,"context_line":"            elif volume_id is not None:"},{"line_number":1286,"context_line":"                try:"},{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_19ec02f2","line":1290,"range":{"start_line":1287,"start_character":20,"end_line":1290,"end_character":70},"updated":"2016-08-01 17:52:26.000000000","message":"Should just re-use _check_attach_and_reserve_volume probably.","commit_id":"61bd27c3547e7541a5f235bd63d731028efa09c1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"657c92a0970fd073e85ebd5d1dbe645c96dfcd63","unresolved":false,"context_lines":[{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_395586c5","line":1290,"updated":"2016-08-01 17:46:54.000000000","message":"So this changes boot from volume to reserve the volume in the API, which we weren\u0027t before. If we fail to attach the volume on the compute when building the instance do we unreserve the volume?","commit_id":"61bd27c3547e7541a5f235bd63d731028efa09c1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"37ffce355f6bf85b823e7f939f75b1afc06b4245","unresolved":false,"context_lines":[{"line_number":1284,"context_line":"                        \"size specified\"))"},{"line_number":1285,"context_line":"            elif volume_id is not None:"},{"line_number":1286,"context_line":"                try:"},{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_2f0378b9","line":1290,"range":{"start_line":1287,"start_character":20,"end_line":1290,"end_character":70},"in_reply_to":"bacf61ea_19ec02f2","updated":"2016-08-01 18:36:43.000000000","message":"Here is the other thing, if we start reserving the volume in this flow in the api, we need the compute to unreserve it on failure in the compute - however, with mixed level computes we could have a mitaka compute node that doesn\u0027t have the unreserve code (which doesn\u0027t exist yet). So we might not be able to reserve the volume in the API until the minimum compute version is new enough to unreserve on failure - which would be a compute service version bump and check.","commit_id":"61bd27c3547e7541a5f235bd63d731028efa09c1"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"b35827b22d37df6df836be68a69949510b16a115","unresolved":false,"context_lines":[{"line_number":1284,"context_line":"                        \"size specified\"))"},{"line_number":1285,"context_line":"            elif volume_id is not None:"},{"line_number":1286,"context_line":"                try:"},{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_e203b574","line":1290,"range":{"start_line":1287,"start_character":20,"end_line":1290,"end_character":70},"in_reply_to":"bacf61ea_2f0378b9","updated":"2016-08-03 20:37:53.000000000","message":"Would this be the place for unreserve_volume: https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L1597 ?\n\nAlso can you point me to an example of a version check you mentioned above? I know we talked about it earlier in connection with multi-attach, but I cannot find the link.","commit_id":"61bd27c3547e7541a5f235bd63d731028efa09c1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e200935a2fa089b98d442ca3d814a5aef69e3486","unresolved":false,"context_lines":[{"line_number":1284,"context_line":"                        \"size specified\"))"},{"line_number":1285,"context_line":"            elif volume_id is not None:"},{"line_number":1286,"context_line":"                try:"},{"line_number":1287,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1288,"context_line":"                    self.volume_api.check_availability_zone(context, volume,"},{"line_number":1289,"context_line":"                                                            instance\u003dinstance)"},{"line_number":1290,"context_line":"                    self.volume_api.reserve_volume(context, volume_id)"},{"line_number":1291,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1292,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1293,"context_line":"                        exception.InvalidVolume):"}],"source_content_type":"text/x-python","patch_set":9,"id":"bacf61ea_391e69bb","line":1290,"range":{"start_line":1287,"start_character":20,"end_line":1290,"end_character":70},"in_reply_to":"bacf61ea_e203b574","updated":"2016-08-04 18:55:34.000000000","message":"That looks like the right place to unreserve on failure.\n\nYou can get the min nova-compute service version like this:\n\nhttps://github.com/openstack/nova/blob/720e5af1e08cc829e98db10da4b93795771a927e/nova/api/openstack/compute/servers.py#L519\n\nIf you update the compute manager to unreserve you\u0027d need to bump the service version and then check that minimum in the api code before you could reserve.","commit_id":"61bd27c3547e7541a5f235bd63d731028efa09c1"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"            elif volume_id is not None:"},{"line_number":1304,"context_line":"                try:"},{"line_number":1305,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1306,"context_line":"                    self.volume_api.check_attach(context,"},{"line_number":1307,"context_line":"                                                 volume,"},{"line_number":1308,"context_line":"                                                 instance\u003dinstance)"},{"line_number":1309,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_3140ca07","side":"PARENT","line":1306,"range":{"start_line":1306,"start_character":36,"end_line":1306,"end_character":48},"updated":"2016-09-28 02:02:50.000000000","message":"If we\u0027re going to remove this completely, can we at least move this new big block of code into a private method in this class so we don\u0027t bloat _validate_bdm even further? That way it can also be unit tested in isolation.","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"            elif volume_id is not None:"},{"line_number":1304,"context_line":"                try:"},{"line_number":1305,"context_line":"                    volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1306,"context_line":"                    self.volume_api.check_attach(context,"},{"line_number":1307,"context_line":"                                                 volume,"},{"line_number":1308,"context_line":"                                                 instance\u003dinstance)"},{"line_number":1309,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_136d8098","side":"PARENT","line":1306,"range":{"start_line":1306,"start_character":36,"end_line":1306,"end_character":48},"in_reply_to":"5a8bc5a2_3140ca07","updated":"2016-10-05 10:28:14.000000000","message":"Sure, I can move out this check to a temporary helper method.","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"                        \"size specified\"))"},{"line_number":1304,"context_line":"            elif volume_id is not None:"},{"line_number":1305,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1306,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1307,"context_line":"                try:"},{"line_number":1308,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1309,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_d18c0682","line":1306,"range":{"start_line":1306,"start_character":20,"end_line":1306,"end_character":52},"updated":"2016-09-28 02:02:50.000000000","message":"I\u0027m not sure you need an admin context for this.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":1303,"context_line":"                        \"size specified\"))"},{"line_number":1304,"context_line":"            elif volume_id is not None:"},{"line_number":1305,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1306,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1307,"context_line":"                try:"},{"line_number":1308,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1309,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_d3c4f893","line":1306,"range":{"start_line":1306,"start_character":20,"end_line":1306,"end_character":52},"in_reply_to":"5a8bc5a2_d18c0682","updated":"2016-10-05 10:28:14.000000000","message":"I saw it elsewhere when I was searching for examples for this code. Does it cause any issue to use admin_context here?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":1305,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1306,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1307,"context_line":"                try:"},{"line_number":1308,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1309,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1310,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1311,"context_line":"                            context, volume_id, instance)"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_11302e9e","line":1308,"updated":"2016-09-28 02:02:50.000000000","message":"We should have a comment in here explaining why we\u0027re doing this check.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":1305,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1306,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1307,"context_line":"                try:"},{"line_number":1308,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1309,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1310,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1311,"context_line":"                            context, volume_id, instance)"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_b3e20c02","line":1308,"in_reply_to":"5a8bc5a2_11302e9e","updated":"2016-10-05 10:28:14.000000000","message":"Done","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":1312,"context_line":"                    else:"},{"line_number":1313,"context_line":"                        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1314,"context_line":"                        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1315,"context_line":"                            msg \u003d (\"Status of volume \u0027%s\u0027 must be \""},{"line_number":1316,"context_line":"                                   \"available\" % volume)"},{"line_number":1317,"context_line":"                            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":1318,"context_line":"                        if volume[\u0027attach_status\u0027] \u003d\u003d \u0027attached\u0027:"},{"line_number":1319,"context_line":"                            msg \u003d \"already attached\""}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_9147de1c","line":1316,"range":{"start_line":1315,"start_character":28,"end_line":1316,"end_character":56},"updated":"2016-09-28 02:02:50.000000000","message":"The message here and below should be marked for translation, and I\u0027d prefer to use the exact same messages that we already had in the check_attach method.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":1312,"context_line":"                    else:"},{"line_number":1313,"context_line":"                        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1314,"context_line":"                        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1315,"context_line":"                            msg \u003d (\"Status of volume \u0027%s\u0027 must be \""},{"line_number":1316,"context_line":"                                   \"available\" % volume)"},{"line_number":1317,"context_line":"                            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":1318,"context_line":"                        if volume[\u0027attach_status\u0027] \u003d\u003d \u0027attached\u0027:"},{"line_number":1319,"context_line":"                            msg \u003d \"already attached\""}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_d3e79810","line":1316,"range":{"start_line":1315,"start_character":28,"end_line":1316,"end_character":56},"in_reply_to":"5a8bc5a2_9147de1c","updated":"2016-10-05 10:28:14.000000000","message":"Done","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":3391,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3392,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3393,"context_line":"                                                instance\u003dinstance)"},{"line_number":3394,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3395,"context_line":""},{"line_number":3396,"context_line":"        return volume"},{"line_number":3397,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_71f7b28c","line":3394,"range":{"start_line":3394,"start_character":24,"end_line":3394,"end_character":38},"updated":"2016-09-28 02:02:50.000000000","message":"It seems odd that in every case that _check_attach_and_reserve_volume is called in here, we don\u0027t unreserve the volume on failure...am I missing something?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":3391,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3392,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3393,"context_line":"                                                instance\u003dinstance)"},{"line_number":3394,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3395,"context_line":""},{"line_number":3396,"context_line":"        return volume"},{"line_number":3397,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_73bfa4d0","line":3394,"range":{"start_line":3394,"start_character":24,"end_line":3394,"end_character":38},"in_reply_to":"3a98d1f4_88c28383","updated":"2016-10-05 10:28:14.000000000","message":"What about adding a note here about the attempt to cover all the failure cases regarding unreserve?\n\nThe generic usage of reserve_volume is not in the scope of this patch, but if we have a bug report/already ongoing activity to fix this, then it can be reflected here in a note.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6bae2dae3ec9111cdf468ab9cde20b863ba8494a","unresolved":false,"context_lines":[{"line_number":3391,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3392,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3393,"context_line":"                                                instance\u003dinstance)"},{"line_number":3394,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3395,"context_line":""},{"line_number":3396,"context_line":"        return volume"},{"line_number":3397,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"3a98d1f4_88c28383","line":3394,"range":{"start_line":3394,"start_character":24,"end_line":3394,"end_character":38},"in_reply_to":"5a8bc5a2_71f7b28c","updated":"2016-10-03 17:26:06.000000000","message":"I think https://review.openstack.org/#/c/290793/ attempts to address some of this. For example, the current flow where this is used for one place is attach_volume. The compute API (this) reserves the volume and then casts to the compute to attach the volume. If the attach fails on the compute then the compute manager unreserves the volume. This is a bit incomplete since if we didn\u0027t even get the RPC cast to the compute (rabbit is down or network short or something), then we\u0027d fail the cast and the volume would be stuck in \u0027attaching\u0027 status and you\u0027d have to reset the state on the volume (via the admin API in cinder) to re-use it.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":3533,"context_line":"            msg \u003d _(\"New volume must be the same size or larger.\")"},{"line_number":3534,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":3535,"context_line":"        self.volume_api.check_detach(context, old_volume)"},{"line_number":3536,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":3537,"context_line":"                                                instance\u003dinstance)"},{"line_number":3538,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3539,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_5113960d","line":3536,"updated":"2016-09-28 02:02:50.000000000","message":"Why don\u0027t we have the same minimum nova-compute service version check here?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":3533,"context_line":"            msg \u003d _(\"New volume must be the same size or larger.\")"},{"line_number":3534,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":3535,"context_line":"        self.volume_api.check_detach(context, old_volume)"},{"line_number":3536,"context_line":"        self.volume_api.check_availability_zone(context, new_volume,"},{"line_number":3537,"context_line":"                                                instance\u003dinstance)"},{"line_number":3538,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3539,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_ee790b31","line":3536,"in_reply_to":"5a8bc5a2_5113960d","updated":"2016-10-05 10:28:14.000000000","message":"The code calls reserve_volume on the new_volume in line 3539. So check_attach was basically unnecessary here already, this change just removes the volume status check here.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1311,"context_line":"                    # that case only, when the computes are a high enough"},{"line_number":1312,"context_line":"                    # version to contain the unreserve_volume call. If not, we"},{"line_number":1313,"context_line":"                    # check the volume state in Nova and act accordingly."},{"line_number":1314,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1315,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1316,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_e91493f6","line":1313,"updated":"2016-12-13 12:30:51.000000000","message":"OK, I am missing something there. Where was the unreserve_volume call added?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1311,"context_line":"                    # that case only, when the computes are a high enough"},{"line_number":1312,"context_line":"                    # version to contain the unreserve_volume call. If not, we"},{"line_number":1313,"context_line":"                    # check the volume state in Nova and act accordingly."},{"line_number":1314,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1315,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1316,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_89607f37","line":1313,"in_reply_to":"1a6eadb0_699de364","updated":"2016-12-13 17:21:41.000000000","message":"That\u0027s exactly the one, which had to be added for error handling.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"f1f92cc6dddcd2b164762b31bc908c79f5bfde6f","unresolved":false,"context_lines":[{"line_number":1310,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1311,"context_line":"                    # that case only, when the computes are a high enough"},{"line_number":1312,"context_line":"                    # version to contain the unreserve_volume call. If not, we"},{"line_number":1313,"context_line":"                    # check the volume state in Nova and act accordingly."},{"line_number":1314,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1315,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1316,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_699de364","line":1313,"in_reply_to":"1a6eadb0_e91493f6","updated":"2016-12-13 12:31:55.000000000","message":"Yup, I was: https://review.openstack.org/#/c/335358/20/nova/conductor/manager.py@517","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":1316,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1317,"context_line":"                            context, volume_id, instance)"},{"line_number":1318,"context_line":"                    else:"},{"line_number":1319,"context_line":"                        volume \u003d self._check_attach(context, instance,"},{"line_number":1320,"context_line":"                                                    volume_id)"},{"line_number":1321,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1322,"context_line":"                except (exception.CinderConnectionFailed,"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_c5097d33","line":1319,"updated":"2016-12-13 12:30:51.000000000","message":"Nit: I guess the idea is next release we can remove all this, and just do the reserve? If so we should add a TODO note telling us when we can delete this code and tidy things up.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":1316,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1317,"context_line":"                            context, volume_id, instance)"},{"line_number":1318,"context_line":"                    else:"},{"line_number":1319,"context_line":"                        volume \u003d self._check_attach(context, instance,"},{"line_number":1320,"context_line":"                                                    volume_id)"},{"line_number":1321,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1322,"context_line":"                except (exception.CinderConnectionFailed,"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_a9653b46","line":1319,"in_reply_to":"1a6eadb0_c5097d33","updated":"2016-12-13 17:21:41.000000000","message":"There\u0027s a note below in the function to remove it, I\u0027ll replace it with a TODO.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":1971,"context_line":"                except Exception as exc:"},{"line_number":1972,"context_line":"                    err_str \u003d _LW(\"Ignoring volume cleanup failure due to %s\")"},{"line_number":1973,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"},{"line_number":1974,"context_line":"            bdm.destroy()"},{"line_number":1975,"context_line":""},{"line_number":1976,"context_line":"    def _local_delete(self, context, instance, bdms, delete_type, cb):"},{"line_number":1977,"context_line":"        if instance.vm_state \u003d\u003d vm_states.SHELVED_OFFLOADED:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_4963c7dc","line":1974,"updated":"2016-12-13 12:30:51.000000000","message":"Yeah, I wonder if we need unreserve in here? Or is detach good enough for this case?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":1971,"context_line":"                except Exception as exc:"},{"line_number":1972,"context_line":"                    err_str \u003d _LW(\"Ignoring volume cleanup failure due to %s\")"},{"line_number":1973,"context_line":"                    LOG.warning(err_str, exc, instance\u003dinstance)"},{"line_number":1974,"context_line":"            bdm.destroy()"},{"line_number":1975,"context_line":""},{"line_number":1976,"context_line":"    def _local_delete(self, context, instance, bdms, delete_type, cb):"},{"line_number":1977,"context_line":"        if instance.vm_state \u003d\u003d vm_states.SHELVED_OFFLOADED:"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_ac2b8981","line":1974,"in_reply_to":"1a6eadb0_4963c7dc","updated":"2016-12-13 17:21:41.000000000","message":"I only saw unreserve calls in error handling cases, when the volume is not attached yet. I think terminate and detach do the job here.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":3437,"context_line":"        # NOTE(ildikov): It needs to be checked whether we attempt an"},{"line_number":3438,"context_line":"        # unreserve_volume call in every failure scenario. E.g. what happens if"},{"line_number":3439,"context_line":"        # the cast fails to get to the compute because of MQ failure? Related"},{"line_number":3440,"context_line":"        # bug reports: #1427060, #1629952"},{"line_number":3441,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3442,"context_line":""},{"line_number":3443,"context_line":"        return volume"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_e95c9327","line":3440,"updated":"2016-12-13 12:30:51.000000000","message":"As an asside, most of the time you end up with an instance in an ERROR state that gets deleted. So its worth starting a search on the delete paths (particularly the local delete path):\nhttps://github.com/openstack/nova/blob/master/nova/compute/api.py#L1986\n\nI just added a comment above about that. Anyways, probably best to deal with that in a separate patch, maybe...","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":3716,"context_line":"            msg \u003d _(\"New volume must be the same size or larger.\")"},{"line_number":3717,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":3718,"context_line":"        self.volume_api.check_detach(context, old_volume)"},{"line_number":3719,"context_line":"        self.volume_api.check_attach(context, new_volume, instance\u003dinstance)"},{"line_number":3720,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3721,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"},{"line_number":3722,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_21e98eb7","side":"PARENT","line":3719,"range":{"start_line":3719,"start_character":24,"end_line":3719,"end_character":36},"updated":"2017-02-16 03:18:27.000000000","message":"I guess this was mostly redundant because we already checked if the new_volume\u0027s attach_status was \u0027attached\u0027 above, the only difference is check_attach checked:\n\nvolume[\u0027status\u0027] !\u003d \"available\"\n\nWhich this doesn\u0027t, but if attach_status is \u0027attached\u0027 I think that implies that status is not \u0027available\u0027.","commit_id":"1129a9ff474529be49dbf5e55ee392cc5ca4cd84"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":3716,"context_line":"            msg \u003d _(\"New volume must be the same size or larger.\")"},{"line_number":3717,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":3718,"context_line":"        self.volume_api.check_detach(context, old_volume)"},{"line_number":3719,"context_line":"        self.volume_api.check_attach(context, new_volume, instance\u003dinstance)"},{"line_number":3720,"context_line":"        self.volume_api.begin_detaching(context, old_volume[\u0027id\u0027])"},{"line_number":3721,"context_line":"        self.volume_api.reserve_volume(context, new_volume[\u0027id\u0027])"},{"line_number":3722,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_97aa676f","side":"PARENT","line":3719,"range":{"start_line":3719,"start_character":24,"end_line":3719,"end_character":36},"in_reply_to":"fa31d9ce_21e98eb7","updated":"2017-02-19 02:39:11.000000000","message":"Yes, this was redundant. I had an attempt to remove this even with the simple multi-attach implementation before thinking about fully remove \u0027check_attach\u0027 itself.","commit_id":"1129a9ff474529be49dbf5e55ee392cc5ca4cd84"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":105,"context_line":"AGGREGATE_ACTION_UPDATE_META \u003d \u0027UpdateMeta\u0027"},{"line_number":106,"context_line":"AGGREGATE_ACTION_DELETE \u003d \u0027Delete\u0027"},{"line_number":107,"context_line":"AGGREGATE_ACTION_ADD \u003d \u0027Add\u0027"},{"line_number":108,"context_line":"BFV_UNRESERVE_MIN_COMPUTE_VERSION \u003d 17"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"# FIXME(danms): Keep a global cache of the cells we find the"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_265c447c","line":108,"updated":"2017-02-16 03:18:27.000000000","message":"A comment explaining what this is a marker for would be nice.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":105,"context_line":"AGGREGATE_ACTION_UPDATE_META \u003d \u0027UpdateMeta\u0027"},{"line_number":106,"context_line":"AGGREGATE_ACTION_DELETE \u003d \u0027Delete\u0027"},{"line_number":107,"context_line":"AGGREGATE_ACTION_ADD \u003d \u0027Add\u0027"},{"line_number":108,"context_line":"BFV_UNRESERVE_MIN_COMPUTE_VERSION \u003d 17"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"# FIXME(danms): Keep a global cache of the cells we find the"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_377c5b20","line":108,"in_reply_to":"fa31d9ce_265c447c","updated":"2017-02-19 02:39:11.000000000","message":"There\u0027s a comment where it is used. I wouldn\u0027t add the same thing to two places. Should it be here instead?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1381,"context_line":"                        \"size specified\"))"},{"line_number":1382,"context_line":"            elif volume_id is not None:"},{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_a67bd4bc","line":1384,"range":{"start_line":1384,"start_character":20,"end_line":1384,"end_character":52},"updated":"2017-02-16 03:18:27.000000000","message":"You can just use the context that\u0027s passed in.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":1381,"context_line":"                        \"size specified\"))"},{"line_number":1382,"context_line":"            elif volume_id is not None:"},{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_f781e3fc","line":1384,"range":{"start_line":1384,"start_character":20,"end_line":1384,"end_character":52},"in_reply_to":"da36d5c6_015ab583","updated":"2017-02-19 02:39:11.000000000","message":"I changed it to simple context, will switch to elevate, if needed.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":1381,"context_line":"                        \"size specified\"))"},{"line_number":1382,"context_line":"            elif volume_id is not None:"},{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_015ab583","line":1384,"range":{"start_line":1384,"start_character":20,"end_line":1384,"end_character":52},"in_reply_to":"da36d5c6_e193e14d","updated":"2017-02-18 18:32:09.000000000","message":"My point was I don\u0027t think we need to actually elevate the context, unless I\u0027m missing something.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":1381,"context_line":"                        \"size specified\"))"},{"line_number":1382,"context_line":"            elif volume_id is not None:"},{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_e193e14d","line":1384,"range":{"start_line":1384,"start_character":20,"end_line":1384,"end_character":52},"in_reply_to":"fa31d9ce_a67bd4bc","updated":"2017-02-18 17:56:25.000000000","message":"Yes, you can just do:\n\n context.elevated()\n\nabove.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"},{"line_number":1388,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1389,"context_line":"                    # that case only, when the computes are a high enough"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_2646047b","line":1386,"range":{"start_line":1386,"start_character":42,"end_line":1386,"end_character":45},"updated":"2017-02-16 03:18:27.000000000","message":"Let\u0027s actually say boot from volume here.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"},{"line_number":1388,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1389,"context_line":"                    # that case only, when the computes are a high enough"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_779fb35f","line":1386,"range":{"start_line":1386,"start_character":42,"end_line":1386,"end_character":45},"in_reply_to":"fa31d9ce_2646047b","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"},{"line_number":1388,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1389,"context_line":"                    # that case only, when the computes are a high enough"},{"line_number":1390,"context_line":"                    # version to contain the unreserve_volume call. If not, we"},{"line_number":1391,"context_line":"                    # check the volume state in Nova and act accordingly."},{"line_number":1392,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1393,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1394,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_a1173e43","line":1391,"range":{"start_line":1386,"start_character":37,"end_line":1391,"end_character":73},"updated":"2017-02-16 03:18:27.000000000","message":"This is a bit confusing. I think it might be simpler to just say something like:\n\n\"Older compute nodes will not unreserve the volume on failure when creating the server. We have to check that the minimum nova-compute version in the deployment is high enough that we know the compute will unreserve the volume on failure before we can reserve it here in the API.\"","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":1383,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1384,"context_line":"                    nova_context.get_admin_context(), \u0027nova-compute\u0027)"},{"line_number":1385,"context_line":"                try:"},{"line_number":1386,"context_line":"                    # NOTE(ildikov): This BFV flow didn\u0027t reserve the volume"},{"line_number":1387,"context_line":"                    # before use, therefore the clean up at failure scenarios"},{"line_number":1388,"context_line":"                    # in the compute is missing as well. We do the reserve in"},{"line_number":1389,"context_line":"                    # that case only, when the computes are a high enough"},{"line_number":1390,"context_line":"                    # version to contain the unreserve_volume call. If not, we"},{"line_number":1391,"context_line":"                    # check the volume state in Nova and act accordingly."},{"line_number":1392,"context_line":"                    if (min_compute_version \u003e\u003d"},{"line_number":1393,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1394,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_979c276b","line":1391,"range":{"start_line":1386,"start_character":37,"end_line":1391,"end_character":73},"in_reply_to":"fa31d9ce_a1173e43","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1393,"context_line":"                        BFV_UNRESERVE_MIN_COMPUTE_VERSION):"},{"line_number":1394,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1395,"context_line":"                            context, volume_id, instance)"},{"line_number":1396,"context_line":"                    else:"},{"line_number":1397,"context_line":"                        # NOTE(ildikov): This call is here only for backward"},{"line_number":1398,"context_line":"                        # compatibility."},{"line_number":1399,"context_line":"                        volume \u003d self._check_attach(context, instance,"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_73c7dd63","line":1396,"updated":"2017-02-16 03:18:27.000000000","message":"I don\u0027t believe you have any unit tests for the else case here where the nova-compute service version is \u003c 17. All I see in the unit tests is mocking get_minimum_version to return 17.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1394,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1395,"context_line":"                            context, volume_id, instance)"},{"line_number":1396,"context_line":"                    else:"},{"line_number":1397,"context_line":"                        # NOTE(ildikov): This call is here only for backward"},{"line_number":1398,"context_line":"                        # compatibility."},{"line_number":1399,"context_line":"                        volume \u003d self._check_attach(context, instance,"},{"line_number":1400,"context_line":"                                                    volume_id)"},{"line_number":1401,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_e12bf67f","line":1398,"range":{"start_line":1397,"start_character":24,"end_line":1398,"end_character":40},"updated":"2017-02-16 03:18:27.000000000","message":"I think we could add to this note that this can be removed after pike-eol.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":1394,"context_line":"                        volume \u003d self._check_attach_and_reserve_volume("},{"line_number":1395,"context_line":"                            context, volume_id, instance)"},{"line_number":1396,"context_line":"                    else:"},{"line_number":1397,"context_line":"                        # NOTE(ildikov): This call is here only for backward"},{"line_number":1398,"context_line":"                        # compatibility."},{"line_number":1399,"context_line":"                        volume \u003d self._check_attach(context, instance,"},{"line_number":1400,"context_line":"                                                    volume_id)"},{"line_number":1401,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_5779cf2e","line":1398,"range":{"start_line":1397,"start_character":24,"end_line":1398,"end_character":40},"in_reply_to":"fa31d9ce_e12bf67f","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1446,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1447,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1448,"context_line":"        # compatibility and should be removed after the deprecation period in"},{"line_number":1449,"context_line":"        # the Pike release."},{"line_number":1450,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1451,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1452,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_81b87a2b","line":1449,"range":{"start_line":1449,"start_character":14,"end_line":1449,"end_character":18},"updated":"2017-02-16 03:18:27.000000000","message":"I\u0027m assuming this is old right?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":1446,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1447,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1448,"context_line":"        # compatibility and should be removed after the deprecation period in"},{"line_number":1449,"context_line":"        # the Pike release."},{"line_number":1450,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1451,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1452,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_f798036d","line":1449,"range":{"start_line":1449,"start_character":14,"end_line":1449,"end_character":18},"in_reply_to":"fa31d9ce_81b87a2b","updated":"2017-02-19 02:39:11.000000000","message":"Probably.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":2056,"context_line":"                          {\u0027connector_host\u0027: connector.get(\u0027host\u0027),"},{"line_number":2057,"context_line":"                           \u0027instance_host\u0027: instance.host}, instance\u003dinstance)"},{"line_number":2058,"context_line":""},{"line_number":2059,"context_line":"    def _local_cleanup_bdm_volumes(self, bdms, instance, context):"},{"line_number":2060,"context_line":"        \"\"\"The method deletes the bdm records and, if a bdm is a volume, call"},{"line_number":2061,"context_line":"        the terminate connection and the detach volume via the Volume API."},{"line_number":2062,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_5309c32b","line":2059,"updated":"2017-02-20 18:49:13.000000000","message":"So I think this is where we need to probably handle unreserving a volume when doing a local delete on the instance. A \u0027local delete\u0027 is where the compute service that the instance is running on is down, so nova-api is cleaning up what it can since we can\u0027t get to the hypervisor.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":2077,"context_line":"                    # Attempt to detach the volume. If there was no connection"},{"line_number":2078,"context_line":"                    # made in the first place this is just cleaning up the"},{"line_number":2079,"context_line":"                    # volume state in the Cinder database."},{"line_number":2080,"context_line":"                    self.volume_api.detach(elevated, bdm.volume_id,"},{"line_number":2081,"context_line":"                                           instance.uuid)"},{"line_number":2082,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":2083,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_53570302","line":2080,"updated":"2017-02-20 18:49:13.000000000","message":"Will detach also unreserve the volume? What if we never got to the point of attaching the volume but it is reserved?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a7268c1ec429850fbea68201d19e0b4e4f95aa44","unresolved":false,"context_lines":[{"line_number":2077,"context_line":"                    # Attempt to detach the volume. If there was no connection"},{"line_number":2078,"context_line":"                    # made in the first place this is just cleaning up the"},{"line_number":2079,"context_line":"                    # volume state in the Cinder database."},{"line_number":2080,"context_line":"                    self.volume_api.detach(elevated, bdm.volume_id,"},{"line_number":2081,"context_line":"                                           instance.uuid)"},{"line_number":2082,"context_line":"                    if bdm.delete_on_termination:"},{"line_number":2083,"context_line":"                        self.volume_api.delete(context, bdm.volume_id)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_73ede78a","line":2080,"in_reply_to":"da36d5c6_53570302","updated":"2017-02-20 19:00:55.000000000","message":"Yeah Cinder detach will also unreserve if it\u0027s not attached:\n\nhttps://github.com/openstack/cinder/blob/38aae0602b124742095044e34c7c9c61df361fab/cinder/volume/manager.py#L1123","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":3609,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3610,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3611,"context_line":"                                                instance\u003dinstance)"},{"line_number":3612,"context_line":"        # NOTE(ildikov): It needs to be checked whether we attempt an"},{"line_number":3613,"context_line":"        # unreserve_volume call in every failure scenario. E.g. what happens if"},{"line_number":3614,"context_line":"        # the cast fails to get to the compute because of MQ failure? Related"},{"line_number":3615,"context_line":"        # bug reports: #1427060, #1629952"},{"line_number":3616,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_a15c9e19","line":3613,"range":{"start_line":3612,"start_character":25,"end_line":3613,"end_character":58},"updated":"2017-02-16 03:18:27.000000000","message":"I\u0027m not sure why this is here. What is \"It\" that needs to be checked?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":3609,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":3610,"context_line":"        self.volume_api.check_availability_zone(context, volume,"},{"line_number":3611,"context_line":"                                                instance\u003dinstance)"},{"line_number":3612,"context_line":"        # NOTE(ildikov): It needs to be checked whether we attempt an"},{"line_number":3613,"context_line":"        # unreserve_volume call in every failure scenario. E.g. what happens if"},{"line_number":3614,"context_line":"        # the cast fails to get to the compute because of MQ failure? Related"},{"line_number":3615,"context_line":"        # bug reports: #1427060, #1629952"},{"line_number":3616,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_57a0ef90","line":3613,"range":{"start_line":3612,"start_character":25,"end_line":3613,"end_character":58},"in_reply_to":"fa31d9ce_a15c9e19","updated":"2017-02-19 02:39:11.000000000","message":"This note is more like a reminder that there might be scenarios, like the example, where we don\u0027t unreserve the volume still.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":3611,"context_line":"                                                instance\u003dinstance)"},{"line_number":3612,"context_line":"        # NOTE(ildikov): It needs to be checked whether we attempt an"},{"line_number":3613,"context_line":"        # unreserve_volume call in every failure scenario. E.g. what happens if"},{"line_number":3614,"context_line":"        # the cast fails to get to the compute because of MQ failure? Related"},{"line_number":3615,"context_line":"        # bug reports: #1427060, #1629952"},{"line_number":3616,"context_line":"        self.volume_api.reserve_volume(context, volume_id)"},{"line_number":3617,"context_line":""},{"line_number":3618,"context_line":"        return volume"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_21a7ce0b","line":3615,"range":{"start_line":3614,"start_character":70,"end_line":3615,"end_character":41},"updated":"2017-02-16 03:18:27.000000000","message":"I typically don\u0027t like to see references to bug reports for things that aren\u0027t resolved. Explaining a case or issue is good enough IMO.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":105,"context_line":"AGGREGATE_ACTION_UPDATE_META \u003d \u0027UpdateMeta\u0027"},{"line_number":106,"context_line":"AGGREGATE_ACTION_DELETE \u003d \u0027Delete\u0027"},{"line_number":107,"context_line":"AGGREGATE_ACTION_ADD \u003d \u0027Add\u0027"},{"line_number":108,"context_line":"BFV_UNRESERVE_MIN_COMPUTE_VERSION \u003d 17"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"# FIXME(danms): Keep a global cache of the cells we find the"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_bae4d4ab","line":108,"range":{"start_line":108,"start_character":4,"end_line":108,"end_character":13},"updated":"2017-02-26 16:26:52.000000000","message":"I wonder if this should just be RESERVE now? Since you\u0027ve removed the unreserve calls for this flow.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":105,"context_line":"AGGREGATE_ACTION_UPDATE_META \u003d \u0027UpdateMeta\u0027"},{"line_number":106,"context_line":"AGGREGATE_ACTION_DELETE \u003d \u0027Delete\u0027"},{"line_number":107,"context_line":"AGGREGATE_ACTION_ADD \u003d \u0027Add\u0027"},{"line_number":108,"context_line":"BFV_UNRESERVE_MIN_COMPUTE_VERSION \u003d 17"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"# FIXME(danms): Keep a global cache of the cells we find the"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_f5065f84","line":108,"range":{"start_line":108,"start_character":4,"end_line":108,"end_character":13},"in_reply_to":"da36d5c6_bae4d4ab","updated":"2017-02-26 18:45:55.000000000","message":"Good point. Done.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":1359,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1360,"context_line":"                    context, \u0027nova-compute\u0027)"},{"line_number":1361,"context_line":"                try:"},{"line_number":1362,"context_line":"                    # NOTE(ildikov): The boot from volume operation did not"},{"line_number":1363,"context_line":"                    # reserve the volume and as the older computes are running"},{"line_number":1364,"context_line":"                    # \u0027check_attach\u0027 which will fail if the volume is in"},{"line_number":1365,"context_line":"                    # \u0027attaching\u0027 state if the compute service version is not"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_ba4db4c5","line":1362,"range":{"start_line":1362,"start_character":68,"end_line":1362,"end_character":75},"updated":"2017-02-26 16:26:52.000000000","message":"This is a little bit confusing. It might be more clear if you said something like, \"The boot from volume operation did not reserve the volume before Pike and as the older computes...\".","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":1359,"context_line":"                min_compute_version \u003d objects.Service.get_minimum_version("},{"line_number":1360,"context_line":"                    context, \u0027nova-compute\u0027)"},{"line_number":1361,"context_line":"                try:"},{"line_number":1362,"context_line":"                    # NOTE(ildikov): The boot from volume operation did not"},{"line_number":1363,"context_line":"                    # reserve the volume and as the older computes are running"},{"line_number":1364,"context_line":"                    # \u0027check_attach\u0027 which will fail if the volume is in"},{"line_number":1365,"context_line":"                    # \u0027attaching\u0027 state if the compute service version is not"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_150a138a","line":1362,"range":{"start_line":1362,"start_character":68,"end_line":1362,"end_character":75},"in_reply_to":"da36d5c6_ba4db4c5","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":1362,"context_line":"                    # NOTE(ildikov): The boot from volume operation did not"},{"line_number":1363,"context_line":"                    # reserve the volume and as the older computes are running"},{"line_number":1364,"context_line":"                    # \u0027check_attach\u0027 which will fail if the volume is in"},{"line_number":1365,"context_line":"                    # \u0027attaching\u0027 state if the compute service version is not"},{"line_number":1366,"context_line":"                    # high enough we will just perform the old check as opposed"},{"line_number":1367,"context_line":"                    # to reserving the volume here."},{"line_number":1368,"context_line":"                    if (min_compute_version \u003e\u003d"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_5ad3d8d4","line":1365,"range":{"start_line":1365,"start_character":34,"end_line":1365,"end_character":42},"updated":"2017-02-26 16:26:52.000000000","message":"I think you need a ; between \"state\" and \"if\" here to break up the thoughts. Previously we didn\u0027t reserve the volume so older computes call check_attach which will fail if the volume is in the \u0027attaching\u0027 state; if the minimum compute service is not high enough we will just perform the old check_attach as opposed to reserving the volume.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":1362,"context_line":"                    # NOTE(ildikov): The boot from volume operation did not"},{"line_number":1363,"context_line":"                    # reserve the volume and as the older computes are running"},{"line_number":1364,"context_line":"                    # \u0027check_attach\u0027 which will fail if the volume is in"},{"line_number":1365,"context_line":"                    # \u0027attaching\u0027 state if the compute service version is not"},{"line_number":1366,"context_line":"                    # high enough we will just perform the old check as opposed"},{"line_number":1367,"context_line":"                    # to reserving the volume here."},{"line_number":1368,"context_line":"                    if (min_compute_version \u003e\u003d"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_b51f87b5","line":1365,"range":{"start_line":1365,"start_character":34,"end_line":1365,"end_character":42},"in_reply_to":"da36d5c6_5ad3d8d4","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":1376,"context_line":"                                                    volume_id)"},{"line_number":1377,"context_line":"                    bdm.volume_size \u003d volume.get(\u0027size\u0027)"},{"line_number":1378,"context_line":"                except (exception.CinderConnectionFailed,"},{"line_number":1379,"context_line":"                        exception.InvalidVolume):"},{"line_number":1380,"context_line":"                    raise"},{"line_number":1381,"context_line":"                except Exception:"},{"line_number":1382,"context_line":"                    raise exception.InvalidBDMVolume(id\u003dvolume_id)"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_ba05b414","line":1379,"range":{"start_line":1379,"start_character":34,"end_line":1379,"end_character":47},"updated":"2017-02-26 16:26:52.000000000","message":"As noted later in the test_compute_api module, _check_attach_and_reserve_volume can raise up InvalidInput if the reserve_volume call fails. If you want to maintain consistency with check_attach, we should probably handle InvalidInput in here separately and translate it to InvalidVolume and raise that up.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"            if num_local \u003e max_local:"},{"line_number":1420,"context_line":"                raise exception.InvalidBDMLocalsLimit()"},{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_5aeaf874","line":1422,"range":{"start_line":1422,"start_character":22,"end_line":1422,"end_character":56},"updated":"2017-02-26 16:26:52.000000000","message":"nit: It might be nice to have the same order of arguments as _check_attach_and_reserve_volume so someone doesn\u0027t get that confused at some point.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":1419,"context_line":"            if num_local \u003e max_local:"},{"line_number":1420,"context_line":"                raise exception.InvalidBDMLocalsLimit()"},{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_555c8b7c","line":1422,"range":{"start_line":1422,"start_character":22,"end_line":1422,"end_character":56},"in_reply_to":"da36d5c6_5aeaf874","updated":"2017-02-26 18:45:55.000000000","message":"I saw that too after I uploaded the latest version, so was waiting for some comments to roll in before rebasing again. :)","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"55d14b910ce2592a8a36cd7835e8bbc2820390b1","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_a9bd7403","line":1424,"updated":"2017-02-26 04:12:46.000000000","message":"This would actually be Ocata EOL, right? We just need it for the upgrade/mix of Ocata and Pike, so once we are at a minimum of Pike it should be safe to remove.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"f0fc11cf1acc4f0ccbd692101a0a4ef7a6d6c8b2","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_d52ebb8a","line":1424,"in_reply_to":"da36d5c6_3a82a4c8","updated":"2017-02-26 16:22:11.000000000","message":"But that would mean Ocata EOL is what we care about. Ocata is using it, so we need to support Ocata talking to Pike. But once Ocata is dead, we don\u0027t care any more and this can just be removed.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"29887bf1a17815672436f223c22dfa745e6b34fb","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_3a82a4c8","line":1424,"in_reply_to":"da36d5c6_5a591834","updated":"2017-02-26 15:43:54.000000000","message":"The comment is correct. We still call check_attach in ocata, which is why the previous patch set of this change was failing grenade, because stable/ocata was calling check_attach and failing because the Pike API was reserving the volume.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_15669350","line":1424,"in_reply_to":"da36d5c6_75998fc3","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"8bc33996a1f2e0cfd005e52bfabe95d09be85b24","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_5a591834","line":1424,"in_reply_to":"da36d5c6_a9bd7403","updated":"2017-02-26 14:48:28.000000000","message":"It can be removed after Queens, so I think you\u0027re right with the after Ocata EOL if my calculations are right.\n\n@Matt: What is your view on this?","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8fc725e6a18fe5a71527c4262ddb2694ce3219d4","unresolved":false,"context_lines":[{"line_number":1421,"context_line":""},{"line_number":1422,"context_line":"    def _check_attach(self, context, instance, volume_id):"},{"line_number":1423,"context_line":"        # TODO(ildikov): This check_attach code is kept only for backward"},{"line_number":1424,"context_line":"        # compatibility and should be removed after Pike EOL."},{"line_number":1425,"context_line":"        volume \u003d self.volume_api.get(context, volume_id)"},{"line_number":1426,"context_line":"        if volume[\u0027status\u0027] !\u003d \u0027available\u0027:"},{"line_number":1427,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_75998fc3","line":1424,"in_reply_to":"da36d5c6_d52ebb8a","updated":"2017-02-26 16:29:35.000000000","message":"Yeah you\u0027re right, pike computes don\u0027t use check_attach, so once ocata is dead (ocata-eol) then we can remove this. Good point, I was messing up the timeline.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d00fe358bddd90ae6de45d00a6ebe8376fe54a4d","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                        exception.InvalidVolume):"},{"line_number":1379,"context_line":"                    raise"},{"line_number":1380,"context_line":"                except exception.InvalidInput as exc:"},{"line_number":1381,"context_line":"                    raise exception.InvalidVolume(reason\u003dexc)"},{"line_number":1382,"context_line":"                except Exception:"},{"line_number":1383,"context_line":"                    raise exception.InvalidBDMVolume(id\u003dvolume_id)"},{"line_number":1384,"context_line":"            elif snapshot_id is not None:"}],"source_content_type":"text/x-python","patch_set":47,"id":"da36d5c6_0ef2fd05","line":1381,"range":{"start_line":1381,"start_character":57,"end_line":1381,"end_character":60},"updated":"2017-02-27 01:30:57.000000000","message":"I\u0027m not sure that this will render properly. We should probably do this instead:\n\nreason\u003dexc.format_message()","commit_id":"bb35446251f1604c687507ade13d8533bcf2c5a4"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"98111c8e4d0575188408ce2200474a00fac0d30f","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                        exception.InvalidVolume):"},{"line_number":1379,"context_line":"                    raise"},{"line_number":1380,"context_line":"                except exception.InvalidInput as exc:"},{"line_number":1381,"context_line":"                    raise exception.InvalidVolume(reason\u003dexc)"},{"line_number":1382,"context_line":"                except Exception:"},{"line_number":1383,"context_line":"                    raise exception.InvalidBDMVolume(id\u003dvolume_id)"},{"line_number":1384,"context_line":"            elif snapshot_id is not None:"}],"source_content_type":"text/x-python","patch_set":47,"id":"da36d5c6_0de91ab7","line":1381,"range":{"start_line":1381,"start_character":57,"end_line":1381,"end_character":60},"in_reply_to":"da36d5c6_0ef2fd05","updated":"2017-02-27 10:52:27.000000000","message":"I hoped the translation takes care about this.","commit_id":"bb35446251f1604c687507ade13d8533bcf2c5a4"}],"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":1594,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"},{"line_number":1595,"context_line":"                          instance\u003dinstance)"},{"line_number":1596,"context_line":"            for bdm in bdms:"},{"line_number":1597,"context_line":"                self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1598,"context_line":"            raise exception.InvalidBDM()"},{"line_number":1599,"context_line":""},{"line_number":1600,"context_line":"    def _update_instance_after_spawn(self, context, instance):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_31b36a0c","line":1597,"range":{"start_line":1597,"start_character":58,"end_line":1597,"end_character":71},"updated":"2016-09-28 02:02:50.000000000","message":"I\u0027m not entirely sure that bdm.volume_id is always set at this point, like in the case we\u0027re booting from volume with a snapshot ID and nova creates a new volume from the snapshot, and then has to update the bdm.volume_id value in the database. I remember races with this code getting set in the past in the ebs test in Tempest. See the note on L2361.\n\nI might be overthinking this since we only reserve the volume in the compute API if it\u0027s a boot from existing volume scenario, but we aren\u0027t checking if this is that case before trying to unreserve it here, so it could be a boot from snapshot or boot from image case.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":1594,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"},{"line_number":1595,"context_line":"                          instance\u003dinstance)"},{"line_number":1596,"context_line":"            for bdm in bdms:"},{"line_number":1597,"context_line":"                self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1598,"context_line":"            raise exception.InvalidBDM()"},{"line_number":1599,"context_line":""},{"line_number":1600,"context_line":"    def _update_instance_after_spawn(self, context, instance):"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_7387a461","line":1597,"range":{"start_line":1597,"start_character":58,"end_line":1597,"end_character":71},"in_reply_to":"5a8bc5a2_31b36a0c","updated":"2016-10-05 10:28:14.000000000","message":"I can do the same check as below and run unreserve if the BDM is a volume and has volume_id. Does it sound ok?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":2358,"context_line":""},{"line_number":2359,"context_line":"        @utils.synchronized(instance.uuid)"},{"line_number":2360,"context_line":"        def do_terminate_instance(instance, bdms):"},{"line_number":2361,"context_line":"            # NOTE(mriedem): If we are deleting the instance while it was"},{"line_number":2362,"context_line":"            # booting from volume, we could be racing with a database update of"},{"line_number":2363,"context_line":"            # the BDM volume_id. Since the compute API passes the BDMs over RPC"},{"line_number":2364,"context_line":"            # to compute here, the BDMs may be stale at this point. So check"},{"line_number":2365,"context_line":"            # for any volume BDMs that don\u0027t have volume_id set and if we"},{"line_number":2366,"context_line":"            # detect that, we need to refresh the BDM list before proceeding."},{"line_number":2367,"context_line":"            # TODO(mriedem): Move this into _delete_instance and make the bdms"},{"line_number":2368,"context_line":"            # parameter optional."},{"line_number":2369,"context_line":"            for bdm in list(bdms):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_d19b2671","line":2366,"range":{"start_line":2361,"start_character":12,"end_line":2366,"end_character":77},"updated":"2016-09-28 02:02:50.000000000","message":"Yeah, see here for the info on that race.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"7fb7e1f96bb2f2f09de232c8f05944ed74751c1a","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"            msg \u003d _LW(\u0027Failed to create block device for instance due to \u0027"},{"line_number":1590,"context_line":"                      \u0027being over volume resource quota\u0027)"},{"line_number":1591,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1592,"context_line":"            raise exception.VolumeLimitExceeded()"},{"line_number":1593,"context_line":""},{"line_number":1594,"context_line":"        except Exception:"},{"line_number":1595,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_293dab2b","line":1592,"updated":"2016-12-13 12:42:34.000000000","message":"Do we not need unreserve here too? Seems to be the same case really.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":1589,"context_line":"            msg \u003d _LW(\u0027Failed to create block device for instance due to \u0027"},{"line_number":1590,"context_line":"                      \u0027being over volume resource quota\u0027)"},{"line_number":1591,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1592,"context_line":"            raise exception.VolumeLimitExceeded()"},{"line_number":1593,"context_line":""},{"line_number":1594,"context_line":"        except Exception:"},{"line_number":1595,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_8cba4de0","line":1592,"in_reply_to":"1a6eadb0_293dab2b","updated":"2016-12-13 17:21:41.000000000","message":"Good point, will fix. :)","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"7fb7e1f96bb2f2f09de232c8f05944ed74751c1a","unresolved":false,"context_lines":[{"line_number":1594,"context_line":"        except Exception:"},{"line_number":1595,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"},{"line_number":1596,"context_line":"                          instance\u003dinstance)"},{"line_number":1597,"context_line":"            for bdm in list(bdms):"},{"line_number":1598,"context_line":"                if bdm.is_volume and not bdm.volume_id:"},{"line_number":1599,"context_line":"                    LOG.debug(\u0027Ensure the BlockDeviceMappingList is up to date\u0027"},{"line_number":1600,"context_line":"                              \u0027 before attempting the unreserve_volume call.\u0027,"},{"line_number":1601,"context_line":"                              instance\u003dinstance)"},{"line_number":1602,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1603,"context_line":"                        context, instance.uuid)"},{"line_number":1604,"context_line":"                    break"},{"line_number":1605,"context_line":"            for bdm in list(bdms):"},{"line_number":1606,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1607,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_69c24335","line":1604,"range":{"start_line":1597,"start_character":0,"end_line":1604,"end_character":25},"updated":"2016-12-13 12:42:34.000000000","message":"Honestly, I would rather we just get the new bdms. I guess there was some previous comment saying thats inefficient.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":1594,"context_line":"        except Exception:"},{"line_number":1595,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"},{"line_number":1596,"context_line":"                          instance\u003dinstance)"},{"line_number":1597,"context_line":"            for bdm in list(bdms):"},{"line_number":1598,"context_line":"                if bdm.is_volume and not bdm.volume_id:"},{"line_number":1599,"context_line":"                    LOG.debug(\u0027Ensure the BlockDeviceMappingList is up to date\u0027"},{"line_number":1600,"context_line":"                              \u0027 before attempting the unreserve_volume call.\u0027,"},{"line_number":1601,"context_line":"                              instance\u003dinstance)"},{"line_number":1602,"context_line":"                    bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1603,"context_line":"                        context, instance.uuid)"},{"line_number":1604,"context_line":"                    break"},{"line_number":1605,"context_line":"            for bdm in list(bdms):"},{"line_number":1606,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1607,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_97b6c988","line":1604,"range":{"start_line":1597,"start_character":0,"end_line":1604,"end_character":25},"in_reply_to":"1a6eadb0_69c24335","updated":"2016-12-13 17:21:41.000000000","message":"I didn\u0027t find any, so I simplified it to get the new ones always. Can change back if someone complains.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1588,"context_line":"            msg \u003d _LW(\u0027Failed to create block device for instance due to \u0027"},{"line_number":1589,"context_line":"                      \u0027being over volume resource quota\u0027)"},{"line_number":1590,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1591,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1592,"context_line":"                    context, instance.uuid)"},{"line_number":1593,"context_line":"            for bdm in list(bdms):"},{"line_number":1594,"context_line":"                if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_8154fac9","line":1591,"updated":"2017-02-16 03:18:27.000000000","message":"Why are you getting the bdms again when we have them as args to this method?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":1588,"context_line":"            msg \u003d _LW(\u0027Failed to create block device for instance due to \u0027"},{"line_number":1589,"context_line":"                      \u0027being over volume resource quota\u0027)"},{"line_number":1590,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1591,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1592,"context_line":"                    context, instance.uuid)"},{"line_number":1593,"context_line":"            for bdm in list(bdms):"},{"line_number":1594,"context_line":"                if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_4154ad53","line":1591,"in_reply_to":"da36d5c6_81c4e540","updated":"2017-02-18 18:32:09.000000000","message":"Yeah that\u0027s possible, I\u0027ve seen (and had to fix) that race before in terminate_instance:\n\nhttps://github.com/openstack/nova/blob/6d64b7274410ae45b95bd7ac7f702c16daaa0fcd/nova/compute/manager.py#L2412-L2427\n\nHowever, as noted below, I\u0027m not sure we really need this because we only hit a quota issue if nova is creating the volume here, and when we create the volume we don\u0027t reserve it, so there is nothing to unreserve.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":1588,"context_line":"            msg \u003d _LW(\u0027Failed to create block device for instance due to \u0027"},{"line_number":1589,"context_line":"                      \u0027being over volume resource quota\u0027)"},{"line_number":1590,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1591,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1592,"context_line":"                    context, instance.uuid)"},{"line_number":1593,"context_line":"            for bdm in list(bdms):"},{"line_number":1594,"context_line":"                if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_81c4e540","line":1591,"in_reply_to":"fa31d9ce_8154fac9","updated":"2017-02-18 17:56:25.000000000","message":"@mriedem: My guess is that the bdms parameter isn\u0027t updated to have the bdm.volume_id attribute set and this BDM.get_by_instance_uuid() grabs those freshly-set attributes for volumes that were created on the Nova side but failed to reserve or attach on the Cinder side.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1590,"context_line":"            LOG.warning(msg, instance\u003dinstance)"},{"line_number":1591,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1592,"context_line":"                    context, instance.uuid)"},{"line_number":1593,"context_line":"            for bdm in list(bdms):"},{"line_number":1594,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1595,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1596,"context_line":"            raise exception.VolumeLimitExceeded()"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_215a6ef2","line":1593,"range":{"start_line":1593,"start_character":23,"end_line":1593,"end_character":33},"updated":"2017-02-16 03:18:27.000000000","message":"BlockDeviceMappingList is already a list, no need to cast it.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1592,"context_line":"                    context, instance.uuid)"},{"line_number":1593,"context_line":"            for bdm in list(bdms):"},{"line_number":1594,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1595,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1596,"context_line":"            raise exception.VolumeLimitExceeded()"},{"line_number":1597,"context_line":""},{"line_number":1598,"context_line":"        except Exception:"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_611e260d","line":1595,"updated":"2017-02-16 03:18:27.000000000","message":"If we hit an OverQuota error, wouldn\u0027t that be the case that nova attempted to create the volume and then attach the instance to it? In that case, nova created the volume and it won\u0027t be reserved, so there is nothing to unreserve, right?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":1598,"context_line":"        except Exception:"},{"line_number":1599,"context_line":"            LOG.exception(_LE(\u0027Instance failed block device setup\u0027),"},{"line_number":1600,"context_line":"                          instance\u003dinstance)"},{"line_number":1601,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":1602,"context_line":"                    context, instance.uuid)"},{"line_number":1603,"context_line":"            for bdm in list(bdms):"},{"line_number":1604,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1605,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1606,"context_line":"            raise exception.InvalidBDM()"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_e13136a3","line":1603,"range":{"start_line":1601,"start_character":12,"end_line":1603,"end_character":34},"updated":"2017-02-16 03:18:27.000000000","message":"same as above","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":1602,"context_line":"                    context, instance.uuid)"},{"line_number":1603,"context_line":"            for bdm in list(bdms):"},{"line_number":1604,"context_line":"                if bdm.is_volume and bdm.volume_id:"},{"line_number":1605,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":1606,"context_line":"            raise exception.InvalidBDM()"},{"line_number":1607,"context_line":""},{"line_number":1608,"context_line":"    def _update_instance_after_spawn(self, context, instance):"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_a1c1e930","line":1605,"updated":"2017-02-18 17:56:25.000000000","message":"Ildiko, could you DRY up the code above and make a little helper function that you can call from both exception-handling blocks?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":2102,"context_line":"            instance.task_state \u003d task_states.BLOCK_DEVICE_MAPPING"},{"line_number":2103,"context_line":"            instance.save()"},{"line_number":2104,"context_line":""},{"line_number":2105,"context_line":"            block_device_info \u003d self._prep_block_device(context, instance,"},{"line_number":2106,"context_line":"                    block_device_mapping)"},{"line_number":2107,"context_line":"            resources[\u0027block_device_info\u0027] \u003d block_device_info"},{"line_number":2108,"context_line":"        except (exception.InstanceNotFound,"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_01b3aa0a","line":2105,"updated":"2017-02-16 03:18:27.000000000","message":"So not doing check_attach here is OK since we would have already reserved in the API if all of the computes are new enough, and even if they aren\u0027t we still check attach in the API before casting to the compute. I guess there is a window there were things can go wrong, but I suppose when we actually try to attach the volume cinder will complain and tell us it\u0027s already in-use and _prep_block_device will fail.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":2102,"context_line":"            instance.task_state \u003d task_states.BLOCK_DEVICE_MAPPING"},{"line_number":2103,"context_line":"            instance.save()"},{"line_number":2104,"context_line":""},{"line_number":2105,"context_line":"            block_device_info \u003d self._prep_block_device(context, instance,"},{"line_number":2106,"context_line":"                    block_device_mapping)"},{"line_number":2107,"context_line":"            resources[\u0027block_device_info\u0027] \u003d block_device_info"},{"line_number":2108,"context_line":"        except (exception.InstanceNotFound,"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_389a6c15","line":2105,"in_reply_to":"da36d5c6_37ca9b50","updated":"2017-02-20 18:49:13.000000000","message":"Having re-read this comment I\u0027m not sure, if we reserve the volume in the API then there shouldn\u0027t be a window for a race failure, except two competing BFV requests on the same volume in the API but we don\u0027t care about that here in the compute manager.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":2102,"context_line":"            instance.task_state \u003d task_states.BLOCK_DEVICE_MAPPING"},{"line_number":2103,"context_line":"            instance.save()"},{"line_number":2104,"context_line":""},{"line_number":2105,"context_line":"            block_device_info \u003d self._prep_block_device(context, instance,"},{"line_number":2106,"context_line":"                    block_device_mapping)"},{"line_number":2107,"context_line":"            resources[\u0027block_device_info\u0027] \u003d block_device_info"},{"line_number":2108,"context_line":"        except (exception.InstanceNotFound,"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_37ca9b50","line":2105,"in_reply_to":"fa31d9ce_01b3aa0a","updated":"2017-02-19 02:39:11.000000000","message":"What window do you actually have in mind?\n\nOn the other hand the attach on Cinder side will complain, see the code here: https://github.com/openstack/cinder/blob/master/cinder/volume/manager.py#L1001-#L1010","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d1f9324e0be29c72fb6260cfdc48caad6f9b8ffc","unresolved":false,"context_lines":[{"line_number":2273,"context_line":"                # NOTE(vish): actual driver detach done in driver.destroy, so"},{"line_number":2274,"context_line":"                #             just tell cinder that we are done with it."},{"line_number":2275,"context_line":"                connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2276,"context_line":"                self.volume_api.terminate_connection(context,"},{"line_number":2277,"context_line":"                                                     bdm.volume_id,"},{"line_number":2278,"context_line":"                                                     connector)"},{"line_number":2279,"context_line":"                self.volume_api.detach(context, bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_92e2d92d","line":2276,"updated":"2017-02-23 18:09:23.000000000","message":"\u003e The desire is to leave the volume reserved in case of error as the instance goes to ERROR state and when it gets deleted the detach call to Cinder will unreserve the volume. --\u003e Remove the compute service bump and the unreserve calls from this patch.\n\nI was wondering what happens if we reserve the volume in the API (for BFV) and _prep_block_device fails and we don\u0027t unreserve, when we go to delete the instance and get to _shutdown_instance here but we never initialized a connection, will calling terminate fail?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"38035a9f51402ea0b2ba8e644daf315a0e530675","unresolved":false,"context_lines":[{"line_number":2273,"context_line":"                # NOTE(vish): actual driver detach done in driver.destroy, so"},{"line_number":2274,"context_line":"                #             just tell cinder that we are done with it."},{"line_number":2275,"context_line":"                connector \u003d self.driver.get_volume_connector(instance)"},{"line_number":2276,"context_line":"                self.volume_api.terminate_connection(context,"},{"line_number":2277,"context_line":"                                                     bdm.volume_id,"},{"line_number":2278,"context_line":"                                                     connector)"},{"line_number":2279,"context_line":"                self.volume_api.detach(context, bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_b22d1d48","line":2276,"in_reply_to":"da36d5c6_92e2d92d","updated":"2017-02-23 18:13:07.000000000","message":"It looks like the volume API in cinder does an RPC call to the volume backend to terminate the connection and then it unreserves the volume. But the question I have is what happens if the terminate_connection on the volume backend fails because we never actually initialized it? That will skip the unreserve and then we\u0027re left with a volume that\u0027s stuck in \u0027attaching\u0027 status and you have to force the detach on the cinder side.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":2276,"context_line":"                self.volume_api.terminate_connection(context,"},{"line_number":2277,"context_line":"                                                     bdm.volume_id,"},{"line_number":2278,"context_line":"                                                     connector)"},{"line_number":2279,"context_line":"                self.volume_api.detach(context, bdm.volume_id, instance.uuid)"},{"line_number":2280,"context_line":"            except exception.DiskNotFound as exc:"},{"line_number":2281,"context_line":"                LOG.debug(\u0027Ignoring DiskNotFound: %s\u0027, exc,"},{"line_number":2282,"context_line":"                          instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_33a0cff4","line":2279,"updated":"2017-02-20 18:49:13.000000000","message":"What will happen here if the volume was reserved but never attached, does this unreserve it? Or do we get a 409 or 400 if it\u0027s not actually attached?","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a7268c1ec429850fbea68201d19e0b4e4f95aa44","unresolved":false,"context_lines":[{"line_number":2276,"context_line":"                self.volume_api.terminate_connection(context,"},{"line_number":2277,"context_line":"                                                     bdm.volume_id,"},{"line_number":2278,"context_line":"                                                     connector)"},{"line_number":2279,"context_line":"                self.volume_api.detach(context, bdm.volume_id, instance.uuid)"},{"line_number":2280,"context_line":"            except exception.DiskNotFound as exc:"},{"line_number":2281,"context_line":"                LOG.debug(\u0027Ignoring DiskNotFound: %s\u0027, exc,"},{"line_number":2282,"context_line":"                          instance\u003dinstance)"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_b3ff5f33","line":2279,"in_reply_to":"da36d5c6_33a0cff4","updated":"2017-02-20 19:00:55.000000000","message":"os-detach will unreserve:\n\nhttps://github.com/openstack/cinder/blob/38aae0602b124742095044e34c7c9c61df361fab/cinder/volume/manager.py#L1123","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1c0363197f50a50ba83b286972071843ba2664b9","unresolved":false,"context_lines":[{"line_number":4800,"context_line":"                              {\u0027volume_id\u0027: bdm.volume_id,"},{"line_number":4801,"context_line":"                               \u0027mountpoint\u0027: bdm[\u0027mount_device\u0027]},"},{"line_number":4802,"context_line":"                              instance\u003dinstance)"},{"line_number":4803,"context_line":"                self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":4804,"context_line":""},{"line_number":4805,"context_line":"        info \u003d {\u0027volume_id\u0027: bdm.volume_id}"},{"line_number":4806,"context_line":"        self._notify_about_instance_usage("}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_f2add584","line":4803,"updated":"2017-02-23 18:17:53.000000000","message":"In the normal volume attach flow, we unreserve the volume if something fails during the attach, like if os-initialize_connection or connect_volume (in os-brick) fails. So when we get to deleting the instance in _shutdown_instance, that volume is no longer attached.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"}],"nova/conductor/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":524,"context_line":"            # instance specific information"},{"line_number":525,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":526,"context_line":"                    context, instance.uuid)"},{"line_number":527,"context_line":""},{"line_number":528,"context_line":"            inst_mapping \u003d self._populate_instance_mapping(context, instance,"},{"line_number":529,"context_line":"                                                           host)"},{"line_number":530,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_f1c5820a","side":"PARENT","line":527,"updated":"2016-09-28 02:02:50.000000000","message":"unrelated whitespace change","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":524,"context_line":"            # instance specific information"},{"line_number":525,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":526,"context_line":"                    context, instance.uuid)"},{"line_number":527,"context_line":""},{"line_number":528,"context_line":"            inst_mapping \u003d self._populate_instance_mapping(context, instance,"},{"line_number":529,"context_line":"                                                           host)"},{"line_number":530,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_131e6063","side":"PARENT","line":527,"in_reply_to":"5a8bc5a2_f1c5820a","updated":"2016-10-05 10:28:14.000000000","message":"Done","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":514,"context_line":"                        context, instance.uuid)"},{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":517,"context_line":"            return"},{"line_number":518,"context_line":""},{"line_number":519,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_11166ea7","line":516,"updated":"2016-09-28 02:02:50.000000000","message":"What happens if the minimum compute is \u003c16 and we didn\u0027t reserve the volume in the compute API? Is unreserve going to give us a 400 because it\u0027s not in an \u0027attaching\u0027 state?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"61b0f5fedc700817a002d26d535e9a18ca19d453","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":514,"context_line":"                        context, instance.uuid)"},{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":517,"context_line":"            return"},{"line_number":518,"context_line":""},{"line_number":519,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":17,"id":"3a98d1f4_dc70c942","line":516,"in_reply_to":"3a98d1f4_c2ae292d","updated":"2016-10-03 19:22:11.000000000","message":"Actually we sorted this out today in the cinder channel, unreserve won\u0027t fail, it just won\u0027t do anything, which is actually OK for us.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":514,"context_line":"                        context, instance.uuid)"},{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":517,"context_line":"            return"},{"line_number":518,"context_line":""},{"line_number":519,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a95cdbc_d313185b","line":516,"in_reply_to":"3a98d1f4_dc70c942","updated":"2016-10-05 10:28:14.000000000","message":"So just to ensure my understanding is correct, the changes here are fine and the only side effect that can happen is that unreserve_volume will not have an effect, right?","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f694616f86c470e8ab406aa17927c05b32fae69c","unresolved":false,"context_lines":[{"line_number":513,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":514,"context_line":"                        context, instance.uuid)"},{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    self.volume_api.unreserve_volume(context, bdm.volume_id)"},{"line_number":517,"context_line":"            return"},{"line_number":518,"context_line":""},{"line_number":519,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":17,"id":"3a98d1f4_c2ae292d","line":516,"in_reply_to":"5a8bc5a2_11166ea7","updated":"2016-10-03 17:18:49.000000000","message":"Looking at:\n\nhttps://github.com/openstack/cinder/blob/e7de3c64bc0644e7604683e54bb9b10a024104e0/cinder/volume/api.py#L633\n\nunreserve will fail if we didn\u0027t reserve the volume first.","commit_id":"59115c00e4f9c6c39735a2889b0320342f26e9eb"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"7fb7e1f96bb2f2f09de232c8f05944ed74751c1a","unresolved":false,"context_lines":[{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    if bdm.volume_id:"},{"line_number":517,"context_line":"                        self.volume_api.unreserve_volume(context,"},{"line_number":518,"context_line":"                                                         bdm.volume_id)"},{"line_number":519,"context_line":"            return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_09038f43","line":518,"updated":"2016-12-13 12:42:34.000000000","message":"Hmm, I guess. I still wonder if this should be done on instance delete really. But that would be inconsistent when the networks I guess. I should ask around about this bit really. We seem very inconsistent.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":515,"context_line":"                for bdm in bdms:"},{"line_number":516,"context_line":"                    if bdm.volume_id:"},{"line_number":517,"context_line":"                        self.volume_api.unreserve_volume(context,"},{"line_number":518,"context_line":"                                                         bdm.volume_id)"},{"line_number":519,"context_line":"            return"},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"        for (instance, host) in six.moves.zip(instances, hosts):"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_1bf4a3f0","line":518,"in_reply_to":"1a6eadb0_09038f43","updated":"2016-12-13 17:21:41.000000000","message":"You mean to move this to instance delete?\n\nThe idea here is to clean up if build fails here and unreserve the volume we now reserve in the API.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":496,"context_line":"                inst_mapping.save()"},{"line_number":497,"context_line":"        return inst_mapping"},{"line_number":498,"context_line":""},{"line_number":499,"context_line":"    def build_instances(self, context, instances, image, filter_properties,"},{"line_number":500,"context_line":"            admin_password, injected_files, requested_networks,"},{"line_number":501,"context_line":"            security_groups, block_device_mapping\u003dNone, legacy_bdm\u003dTrue):"},{"line_number":502,"context_line":"        # TODO(ndipanov): Remove block_device_mapping and legacy_bdm in version"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_73619dd2","line":499,"range":{"start_line":499,"start_character":8,"end_line":499,"end_character":23},"updated":"2017-02-16 03:18:27.000000000","message":"This is called from the nova-api in the case of cells v1 and it\u0027s called from nova-compute in the case of a reschedule. The point of the change in this method is to simply unreserve any volumes that were reserved in nova-api (since nova-compute doesn\u0027t reserve a volume).","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":540,"context_line":"                    pass"},{"line_number":541,"context_line":"                self._cleanup_allocated_networks("},{"line_number":542,"context_line":"                    context, instance, requested_networks)"},{"line_number":543,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":544,"context_line":"                        context, instance.uuid)"},{"line_number":545,"context_line":"                for bdm in bdms:"},{"line_number":546,"context_line":"                    if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_a171fe0d","line":543,"updated":"2017-02-16 03:18:27.000000000","message":"I was going to say we could just use block_device_mapping but above and below this it says not to use it, so I guess we shouldn\u0027t (I need to figure out why that is at some point).","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":540,"context_line":"                    pass"},{"line_number":541,"context_line":"                self._cleanup_allocated_networks("},{"line_number":542,"context_line":"                    context, instance, requested_networks)"},{"line_number":543,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":544,"context_line":"                        context, instance.uuid)"},{"line_number":545,"context_line":"                for bdm in bdms:"},{"line_number":546,"context_line":"                    if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_586d6003","line":543,"in_reply_to":"da36d5c6_61d8d1f0","updated":"2017-02-20 18:49:13.000000000","message":"I was talking with John and Dan about this and they pointed out that we probably don\u0027t need to worry about unreserving the volume here because we\u0027ll unreserve the volume when we delete the instance. I assume you added this because we cleanup allocated networks here, but we also do that when we delete an instance so I\u0027m not really sure why either is needed as long as we do the cleanup when we delete the instance. I probably need them to look at this to confirm if it\u0027s what they were thinking.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":540,"context_line":"                    pass"},{"line_number":541,"context_line":"                self._cleanup_allocated_networks("},{"line_number":542,"context_line":"                    context, instance, requested_networks)"},{"line_number":543,"context_line":"                bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":544,"context_line":"                        context, instance.uuid)"},{"line_number":545,"context_line":"                for bdm in bdms:"},{"line_number":546,"context_line":"                    if bdm.is_volume and bdm.volume_id:"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_61d8d1f0","line":543,"in_reply_to":"fa31d9ce_a171fe0d","updated":"2017-02-18 18:32:09.000000000","message":"Oh yeah the bdms are pulled again from the db on L564 below.\n\nFWIW if we\u0027re hitting this from the nova-api call, I think the BDMs will already have the volume_id set if they are boot from volume with a volume ID passed to nova-api. In the case of nova-compute failing, you\u0027re already unreserving these in _prep_block_device. So I question the need to pulling these from the database again - you could omit that I think and leave a comment explaining why we don\u0027t get them from the DB again.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":561,"context_line":"            # The block_device_mapping passed from the api doesn\u0027t contain"},{"line_number":562,"context_line":"            # instance specific information"},{"line_number":563,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":564,"context_line":"                    context, instance.uuid)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"            # This is populated in scheduler_utils.populate_retry"},{"line_number":567,"context_line":"            num_attempts \u003d local_filter_props.get(\u0027retry\u0027,"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_e1e121d2","line":564,"updated":"2017-02-18 17:56:25.000000000","message":"Seems a little wasteful to be pulling BDM information for the instances yet again here, but I guess the new lines added on line 543-548 are only for exceptions that occur in creating the build request, so should be OK.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":561,"context_line":"            # The block_device_mapping passed from the api doesn\u0027t contain"},{"line_number":562,"context_line":"            # instance specific information"},{"line_number":563,"context_line":"            bdms \u003d objects.BlockDeviceMappingList.get_by_instance_uuid("},{"line_number":564,"context_line":"                    context, instance.uuid)"},{"line_number":565,"context_line":""},{"line_number":566,"context_line":"            # This is populated in scheduler_utils.populate_retry"},{"line_number":567,"context_line":"            num_attempts \u003d local_filter_props.get(\u0027retry\u0027,"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_e1f2016f","line":564,"in_reply_to":"da36d5c6_e1e121d2","updated":"2017-02-18 18:32:09.000000000","message":"Yeah I don\u0027t remember why this is here, I need to dig, but I don\u0027t know what instance specific information the comment above is referring to. And these changes don\u0027t give many clues:\n\nhttps://github.com/openstack/nova/commit/8c302e57401ca5b4f000076e7d6f2fe371e47aa0\n\nhttps://github.com/openstack/nova/commit/70196c4854e6cc1cb566dc9fc5b1a3397a4b69ab\n\nMaybe dansmith remembers that.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":805,"context_line":"            bdm.update_or_create()"},{"line_number":806,"context_line":"        return instance_block_device_mapping"},{"line_number":807,"context_line":""},{"line_number":808,"context_line":"    def _bury_in_cell0(self, context, request_spec, exc,"},{"line_number":809,"context_line":"                       build_requests\u003dNone, instances\u003dNone):"},{"line_number":810,"context_line":"        \"\"\"Ensure all provided build_requests and instances end up in cell0."},{"line_number":811,"context_line":""}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_33756563","line":808,"range":{"start_line":808,"start_character":8,"end_line":808,"end_character":22},"updated":"2017-02-16 03:18:27.000000000","message":"I\u0027m pretty sure you need to handle unreserving volumes in here if the API reserved the volume for a given instance but then we couldn\u0027t schedule the instance to a cell.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":805,"context_line":"            bdm.update_or_create()"},{"line_number":806,"context_line":"        return instance_block_device_mapping"},{"line_number":807,"context_line":""},{"line_number":808,"context_line":"    def _bury_in_cell0(self, context, request_spec, exc,"},{"line_number":809,"context_line":"                       build_requests\u003dNone, instances\u003dNone):"},{"line_number":810,"context_line":"        \"\"\"Ensure all provided build_requests and instances end up in cell0."},{"line_number":811,"context_line":""}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_f87e14ad","line":808,"range":{"start_line":808,"start_character":8,"end_line":808,"end_character":22},"in_reply_to":"fa31d9ce_33756563","updated":"2017-02-20 18:49:13.000000000","message":"Again, after talking with John and Dan we think we could leave this alone since we can still list/show instances out of the API even if they are in cell0, and then we wouldn\u0027t need to worry about unreserving the volume until we delete the instance.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":864,"context_line":"                with obj_target_cell(inst, cell0):"},{"line_number":865,"context_line":"                    inst.destroy()"},{"line_number":866,"context_line":""},{"line_number":867,"context_line":"    def schedule_and_build_instances(self, context, build_requests,"},{"line_number":868,"context_line":"                                     request_specs, image,"},{"line_number":869,"context_line":"                                     admin_password, injected_files,"},{"line_number":870,"context_line":"                                     requested_networks, block_device_mapping):"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_93b3714c","line":867,"range":{"start_line":867,"start_character":8,"end_line":867,"end_character":36},"updated":"2017-02-16 03:18:27.000000000","message":"This is new as of cells v2 in Ocata and is what\u0027s called from nova-api in the non-cellsv1 case (so the normal case). I think you\u0027re missing some unreserve_volume calls in here.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":952,"context_line":"                for bdm in instance_bdms:"},{"line_number":953,"context_line":"                    with obj_target_cell(bdm, cell):"},{"line_number":954,"context_line":"                        try:"},{"line_number":955,"context_line":"                            bdm.destroy()"},{"line_number":956,"context_line":"                        except exception.ObjectActionError:"},{"line_number":957,"context_line":"                            pass"},{"line_number":958,"context_line":"                return"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_f3f10d02","line":955,"updated":"2017-02-16 03:18:27.000000000","message":"For example, at this point I\u0027m thinking we need to unreserve the volume if it\u0027s a volume BDM.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":952,"context_line":"                for bdm in instance_bdms:"},{"line_number":953,"context_line":"                    with obj_target_cell(bdm, cell):"},{"line_number":954,"context_line":"                        try:"},{"line_number":955,"context_line":"                            bdm.destroy()"},{"line_number":956,"context_line":"                        except exception.ObjectActionError:"},{"line_number":957,"context_line":"                            pass"},{"line_number":958,"context_line":"                return"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_1831280c","line":955,"in_reply_to":"fa31d9ce_f3f10d02","updated":"2017-02-20 18:49:13.000000000","message":"So if we properly unreserve the volume when the instance is deleted, which this flow is trying to account for, then we shouldn\u0027t need to worry about unreserve here.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"}],"nova/objects/service.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Introducing reserve_volume for the already existing volumes"},{"line_number":99,"context_line":"    # in the BFV flow indicates the need of a version bump in order to be able"},{"line_number":100,"context_line":"    # to identify when we can use it on the API as the older computes does not"},{"line_number":101,"context_line":"    # unreserve on failure."},{"line_number":102,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_66d9dcf2","line":99,"range":{"start_line":99,"start_character":13,"end_line":99,"end_character":16},"updated":"2017-02-16 03:18:27.000000000","message":"nit: I\u0027d actually say \"boot from volume\" here just for people that are new to nova and aren\u0027t familiar with the acronym.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Introducing reserve_volume for the already existing volumes"},{"line_number":99,"context_line":"    # in the BFV flow indicates the need of a version bump in order to be able"},{"line_number":100,"context_line":"    # to identify when we can use it on the API as the older computes does not"},{"line_number":101,"context_line":"    # unreserve on failure."},{"line_number":102,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_97d8a71f","line":99,"range":{"start_line":99,"start_character":13,"end_line":99,"end_character":16},"in_reply_to":"fa31d9ce_66d9dcf2","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":95,"context_line":"    # Version 16: Indicate that nova-compute will refuse to start if it doesn\u0027t"},{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Introducing reserve_volume for the already existing volumes"},{"line_number":99,"context_line":"    # in the BFV flow indicates the need of a version bump in order to be able"},{"line_number":100,"context_line":"    # to identify when we can use it on the API as the older computes does not"},{"line_number":101,"context_line":"    # unreserve on failure."},{"line_number":102,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":103,"context_line":")"},{"line_number":104,"context_line":""}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_e69a2c13","line":101,"range":{"start_line":98,"start_character":18,"end_line":101,"end_character":27},"updated":"2017-02-16 03:18:27.000000000","message":"This description is confusing. I think we could say something simpler like:\n\n\"Indicates that nova-compute will unreserve a volume when boot from volume fails. The API will use this to tell if it can reserve a volume before casting to the compute to build the instance.\"","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":95,"context_line":"    # Version 16: Indicate that nova-compute will refuse to start if it doesn\u0027t"},{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Introducing reserve_volume for the already existing volumes"},{"line_number":99,"context_line":"    # in the BFV flow indicates the need of a version bump in order to be able"},{"line_number":100,"context_line":"    # to identify when we can use it on the API as the older computes does not"},{"line_number":101,"context_line":"    # unreserve on failure."},{"line_number":102,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":103,"context_line":")"},{"line_number":104,"context_line":""}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_77db331c","line":101,"range":{"start_line":98,"start_character":18,"end_line":101,"end_character":27},"in_reply_to":"fa31d9ce_e69a2c13","updated":"2017-02-19 02:39:11.000000000","message":"Done","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":95,"context_line":"    # Version 16: Indicate that nova-compute will refuse to start if it doesn\u0027t"},{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Add \u0027reserve_volume\u0027 to the boot fomr volume flow and"},{"line_number":99,"context_line":"    # remove \u0027check_attach\u0027. The service version bump is needed to fall back to"},{"line_number":100,"context_line":"    # the old check in the API as the old computes fail if the volume is moved"},{"line_number":101,"context_line":"    # to \u0027attaching\u0027 state by reserve."}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_daf6e85b","line":98,"range":{"start_line":98,"start_character":51,"end_line":98,"end_character":55},"updated":"2017-02-26 16:26:52.000000000","message":"from","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":95,"context_line":"    # Version 16: Indicate that nova-compute will refuse to start if it doesn\u0027t"},{"line_number":96,"context_line":"    # have a placement section configured."},{"line_number":97,"context_line":"    {\u0027compute_rpc\u0027: \u00274.13\u0027},"},{"line_number":98,"context_line":"    # Version 17: Add \u0027reserve_volume\u0027 to the boot fomr volume flow and"},{"line_number":99,"context_line":"    # remove \u0027check_attach\u0027. The service version bump is needed to fall back to"},{"line_number":100,"context_line":"    # the old check in the API as the old computes fail if the volume is moved"},{"line_number":101,"context_line":"    # to \u0027attaching\u0027 state by reserve."}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_b54dc7c5","line":98,"range":{"start_line":98,"start_character":51,"end_line":98,"end_character":55},"in_reply_to":"da36d5c6_daf6e85b","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":4679,"context_line":"        def fake_volume_check_availability_zone(self, context,"},{"line_number":4680,"context_line":"                                                volume_id, instance):"},{"line_number":4681,"context_line":"            pass"},{"line_number":4682,"context_line":"        self.stubs.Set(cinder.API, \"check_availability_zone\","},{"line_number":4683,"context_line":"                       fake_volume_check_availability_zone)"},{"line_number":4684,"context_line":""},{"line_number":4685,"context_line":"        def fake_get_volume_encryption_metadata(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_931451e9","line":4682,"range":{"start_line":4682,"start_character":8,"end_line":4682,"end_character":22},"updated":"2017-02-16 03:18:27.000000000","message":"Please use stub_out or mock instead as self.stubs uses mox and we\u0027re trying to get rid of that since it doesn\u0027t support py35.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":4679,"context_line":"        def fake_volume_check_availability_zone(self, context,"},{"line_number":4680,"context_line":"                                                volume_id, instance):"},{"line_number":4681,"context_line":"            pass"},{"line_number":4682,"context_line":"        self.stubs.Set(cinder.API, \"check_availability_zone\","},{"line_number":4683,"context_line":"                       fake_volume_check_availability_zone)"},{"line_number":4684,"context_line":""},{"line_number":4685,"context_line":"        def fake_get_volume_encryption_metadata(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_d7085f94","line":4682,"range":{"start_line":4682,"start_character":8,"end_line":4682,"end_character":22},"in_reply_to":"fa31d9ce_931451e9","updated":"2017-02-19 02:39:11.000000000","message":"Good catch, the test case must\u0027ve been updated since I added this line here.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":4714,"context_line":"        def fake_volume_check_availability_zone(self, context,"},{"line_number":4715,"context_line":"                                                volume_id, instance):"},{"line_number":4716,"context_line":"            pass"},{"line_number":4717,"context_line":"        self.stubs.Set(cinder.API, \"check_availability_zone\","},{"line_number":4718,"context_line":"                       fake_volume_check_availability_zone)"},{"line_number":4719,"context_line":""},{"line_number":4720,"context_line":"        def fake_get_volume_encryption_metadata(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_9a8710b4","line":4717,"range":{"start_line":4717,"start_character":8,"end_line":4717,"end_character":22},"updated":"2017-02-26 16:26:52.000000000","message":"Just mock this method out as a method decorator since the stub is just passing so we don\u0027t care about the result. The point is we don\u0027t want to use self.stubs.Set as that uses mox which we\u0027re trying to remove since it\u0027s not safe with py35.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":4714,"context_line":"        def fake_volume_check_availability_zone(self, context,"},{"line_number":4715,"context_line":"                                                volume_id, instance):"},{"line_number":4716,"context_line":"            pass"},{"line_number":4717,"context_line":"        self.stubs.Set(cinder.API, \"check_availability_zone\","},{"line_number":4718,"context_line":"                       fake_volume_check_availability_zone)"},{"line_number":4719,"context_line":""},{"line_number":4720,"context_line":"        def fake_get_volume_encryption_metadata(self, context, volume_id):"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_f5c9ff26","line":4717,"range":{"start_line":4717,"start_character":8,"end_line":4717,"end_character":22},"in_reply_to":"da36d5c6_9a8710b4","updated":"2017-02-26 18:45:55.000000000","message":"It\u0027s a rebase issue that I fixed earlier, but as I had to go back to the service version bump version again it came back. :(\n\nDone.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"}],"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":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":3315,"context_line":"                       side_effect\u003dexception.InvalidVolume(reason\u003d\u0027error\u0027))"},{"line_number":3316,"context_line":"    def test_validate_bdm_with_error_volume(self, mock_reserve_volume,"},{"line_number":3317,"context_line":"                                            mock_get, mock_get_min_ver):"},{"line_number":3318,"context_line":"        # Tests that an InvalidVolume exception raised from"},{"line_number":3319,"context_line":"        # volume_api.reserve_volume due to the volume status not being"},{"line_number":3320,"context_line":"        # \u0027available\u0027 results in _validate_bdm re-raising InvalidVolume."},{"line_number":3321,"context_line":"        instance \u003d self._create_instance_obj()"},{"line_number":3322,"context_line":"        instance_type \u003d self._create_flavor()"},{"line_number":3323,"context_line":"        volume_id \u003d \u0027e856840e-9f5b-4894-8bde-58c6e29ac1e8\u0027"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_5a6f58fb","line":3320,"range":{"start_line":3318,"start_character":8,"end_line":3320,"end_character":72},"updated":"2017-02-26 16:26:52.000000000","message":"Actually what error code does Cinder return? I\u0027m assuming 400 because of this:\n\nhttps://github.com/openstack/cinder/blob/467b9a4d3fba5210f4f5839f7f5e7c5926c9bedd/cinder/volume/api.py#L641\n\nI don\u0027t know where that is translated to a 400 error in the cinder REST API.\n\nThis is what\u0027s going to handle the error:\n\nhttps://review.openstack.org/#/c/335358/46/nova/volume/cinder.py@185\n\nIf we do get a 400 back from Cinder, then this is going to raise InvalidInput rather than InvalidVolume:\n\nhttps://review.openstack.org/#/c/335358/46/nova/volume/cinder.py@176\n\nSo the tests in here which are mocking out cinder.API.reserve_volume to raise InvalidVolume are actually wrong since we wouldn\u0027t get that.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":3315,"context_line":"                       side_effect\u003dexception.InvalidVolume(reason\u003d\u0027error\u0027))"},{"line_number":3316,"context_line":"    def test_validate_bdm_with_error_volume(self, mock_reserve_volume,"},{"line_number":3317,"context_line":"                                            mock_get, mock_get_min_ver):"},{"line_number":3318,"context_line":"        # Tests that an InvalidVolume exception raised from"},{"line_number":3319,"context_line":"        # volume_api.reserve_volume due to the volume status not being"},{"line_number":3320,"context_line":"        # \u0027available\u0027 results in _validate_bdm re-raising InvalidVolume."},{"line_number":3321,"context_line":"        instance \u003d self._create_instance_obj()"},{"line_number":3322,"context_line":"        instance_type \u003d self._create_flavor()"},{"line_number":3323,"context_line":"        volume_id \u003d \u0027e856840e-9f5b-4894-8bde-58c6e29ac1e8\u0027"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_f5be5f63","line":3320,"range":{"start_line":3318,"start_character":8,"end_line":3320,"end_character":72},"in_reply_to":"da36d5c6_5a6f58fb","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":3466,"context_line":"    @mock.patch.object(cinder.API, \u0027check_availability_zone\u0027)"},{"line_number":3467,"context_line":"    @mock.patch.object(cinder.API, \u0027reserve_volume\u0027,"},{"line_number":3468,"context_line":"                       side_effect\u003dexception.InvalidVolume(reason\u003d\u0027error\u0027))"},{"line_number":3469,"context_line":"    def test_provision_instances_with_error_volume(self,"},{"line_number":3470,"context_line":"                                                   mock_cinder_check_av_zone,"},{"line_number":3471,"context_line":"                                                   mock_reserve_volume,"},{"line_number":3472,"context_line":"                                                   mock_get,"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_9a7550cc","line":3469,"range":{"start_line":3469,"start_character":8,"end_line":3469,"end_character":50},"updated":"2017-02-26 16:26:52.000000000","message":"This is redundant with test_validate_bdm_with_error_volume isn\u0027t it? It\u0027s testing the same error condition, it\u0027s just this method starts from higher up in the stack.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0db0d2ec3243c1ddcf781f5b04547433fa1f7067","unresolved":false,"context_lines":[{"line_number":3466,"context_line":"    @mock.patch.object(cinder.API, \u0027check_availability_zone\u0027)"},{"line_number":3467,"context_line":"    @mock.patch.object(cinder.API, \u0027reserve_volume\u0027,"},{"line_number":3468,"context_line":"                       side_effect\u003dexception.InvalidVolume(reason\u003d\u0027error\u0027))"},{"line_number":3469,"context_line":"    def test_provision_instances_with_error_volume(self,"},{"line_number":3470,"context_line":"                                                   mock_cinder_check_av_zone,"},{"line_number":3471,"context_line":"                                                   mock_reserve_volume,"},{"line_number":3472,"context_line":"                                                   mock_get,"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_eb975ce9","line":3469,"range":{"start_line":3469,"start_character":8,"end_line":3469,"end_character":50},"in_reply_to":"da36d5c6_55464b7b","updated":"2017-02-26 23:19:29.000000000","message":"It\u0027s fine to leave it I think. I don\u0027t think _provision_instances handles this failure any differently, it\u0027s just passed through back up to the REST API controller code, but it\u0027s fine to leave it.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":3466,"context_line":"    @mock.patch.object(cinder.API, \u0027check_availability_zone\u0027)"},{"line_number":3467,"context_line":"    @mock.patch.object(cinder.API, \u0027reserve_volume\u0027,"},{"line_number":3468,"context_line":"                       side_effect\u003dexception.InvalidVolume(reason\u003d\u0027error\u0027))"},{"line_number":3469,"context_line":"    def test_provision_instances_with_error_volume(self,"},{"line_number":3470,"context_line":"                                                   mock_cinder_check_av_zone,"},{"line_number":3471,"context_line":"                                                   mock_reserve_volume,"},{"line_number":3472,"context_line":"                                                   mock_get,"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_55464b7b","line":3469,"range":{"start_line":3469,"start_character":8,"end_line":3469,"end_character":50},"in_reply_to":"da36d5c6_9a7550cc","updated":"2017-02-26 18:45:55.000000000","message":"It\u0027s testing _provision_instances as opposed to _validate_bdm so it\u0027s not completely redundant in that sense. Does not add much more to the testing either, but as this test is not added by this patch I would rather not remove it with this change. Or should I?","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":3674,"context_line":"                       return_value\u003d17)"},{"line_number":3675,"context_line":"    @mock.patch.object(cinder.API, \u0027get\u0027)"},{"line_number":3676,"context_line":"    @mock.patch.object(cinder.API, \u0027check_availability_zone\u0027,)"},{"line_number":3677,"context_line":"    @mock.patch.object(cinder.API, \u0027reserve_volume\u0027,"},{"line_number":3678,"context_line":"                   side_effect\u003d(None, exception.InvalidVolume(reason\u003d\u0027error\u0027)))"},{"line_number":3679,"context_line":"    def test_provision_instances_cleans_up_when_volume_invalid(self,"},{"line_number":3680,"context_line":"            _mock_cinder_reserve_volume,"},{"line_number":3681,"context_line":"            _mock_cinder_check_availability_zone, _mock_cinder_get,"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_ba80548c","line":3678,"range":{"start_line":3677,"start_character":23,"end_line":3678,"end_character":79},"updated":"2017-02-26 16:26:52.000000000","message":"Another case that I\u0027m thinking doesn\u0027t actually happen.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":3674,"context_line":"                       return_value\u003d17)"},{"line_number":3675,"context_line":"    @mock.patch.object(cinder.API, \u0027get\u0027)"},{"line_number":3676,"context_line":"    @mock.patch.object(cinder.API, \u0027check_availability_zone\u0027,)"},{"line_number":3677,"context_line":"    @mock.patch.object(cinder.API, \u0027reserve_volume\u0027,"},{"line_number":3678,"context_line":"                   side_effect\u003d(None, exception.InvalidVolume(reason\u003d\u0027error\u0027)))"},{"line_number":3679,"context_line":"    def test_provision_instances_cleans_up_when_volume_invalid(self,"},{"line_number":3680,"context_line":"            _mock_cinder_reserve_volume,"},{"line_number":3681,"context_line":"            _mock_cinder_check_availability_zone, _mock_cinder_get,"}],"source_content_type":"text/x-python","patch_set":46,"id":"da36d5c6_d5523b34","line":3678,"range":{"start_line":3677,"start_character":23,"end_line":3678,"end_character":79},"in_reply_to":"da36d5c6_ba80548c","updated":"2017-02-26 18:45:55.000000000","message":"Well, it\u0027s lost in the translation that we have an InvalidVolume error...","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d00fe358bddd90ae6de45d00a6ebe8376fe54a4d","unresolved":false,"context_lines":[{"line_number":3315,"context_line":"                       side_effect\u003dexception.InvalidInput(reason\u003d\u0027error\u0027))"},{"line_number":3316,"context_line":"    def test_validate_bdm_with_error_volume(self, mock_reserve_volume,"},{"line_number":3317,"context_line":"                                            mock_get, mock_get_min_ver):"},{"line_number":3318,"context_line":"        # Tests that an InvalidVolume exception raised from"},{"line_number":3319,"context_line":"        # volume_api.reserve_volume due to the volume status not being"},{"line_number":3320,"context_line":"        # \u0027available\u0027 results in _validate_bdm re-raising InvalidVolume."},{"line_number":3321,"context_line":"        instance \u003d self._create_instance_obj()"}],"source_content_type":"text/x-python","patch_set":47,"id":"da36d5c6_aee1f12b","line":3318,"range":{"start_line":3318,"start_character":24,"end_line":3318,"end_character":37},"updated":"2017-02-27 01:30:57.000000000","message":"InvalidInput","commit_id":"bb35446251f1604c687507ade13d8533bcf2c5a4"}],"nova/tests/unit/virt/test_block_device.py":[{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"064ae7a4152621bc177d9dc2b91e2929bd191d99","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        self.assertEqual(expected_conn_info, test_bdm[\u0027connection_info\u0027])"},{"line_number":520,"context_line":"        self.assertEqual(42, test_bdm.volume_size)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"    def test_volume_attach_check_aV_zone_fails(self):"},{"line_number":523,"context_line":"        test_bdm \u003d self.driver_classes[\u0027volume\u0027]("},{"line_number":524,"context_line":"            self.volume_bdm)"},{"line_number":525,"context_line":"        volume \u003d {\u0027id\u0027: \u0027fake-volume-id-1\u0027}"}],"source_content_type":"text/x-python","patch_set":29,"id":"5a3905b3_b4f24104","line":522,"range":{"start_line":522,"start_character":34,"end_line":522,"end_character":35},"updated":"2017-01-23 19:55:06.000000000","message":"s/V/v/","commit_id":"fbc765600c60c48f50d41e0c4c3931c0affdb3c1"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"4e7b64f5021e7be828fc3c3f808be17db83cd205","unresolved":false,"context_lines":[{"line_number":519,"context_line":"        self.assertEqual(expected_conn_info, test_bdm[\u0027connection_info\u0027])"},{"line_number":520,"context_line":"        self.assertEqual(42, test_bdm.volume_size)"},{"line_number":521,"context_line":""},{"line_number":522,"context_line":"    def test_volume_attach_check_aV_zone_fails(self):"},{"line_number":523,"context_line":"        test_bdm \u003d self.driver_classes[\u0027volume\u0027]("},{"line_number":524,"context_line":"            self.volume_bdm)"},{"line_number":525,"context_line":"        volume \u003d {\u0027id\u0027: \u0027fake-volume-id-1\u0027}"}],"source_content_type":"text/x-python","patch_set":29,"id":"5a3905b3_f705ab45","line":522,"range":{"start_line":522,"start_character":34,"end_line":522,"end_character":35},"in_reply_to":"5a3905b3_b4f24104","updated":"2017-01-23 20:37:58.000000000","message":"Bah, good catch! :)","commit_id":"fbc765600c60c48f50d41e0c4c3931c0affdb3c1"}],"nova/virt/block_device.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1e5ddf2d35812f48e8a7c26c0abb40896f1966cc","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":250,"context_line":"        if do_check_attach:"},{"line_number":251,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":252,"context_line":"                                               instance\u003dinstance)"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"},{"line_number":255,"context_line":"        context \u003d context.elevated()"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_2e201d49","line":252,"updated":"2016-07-25 10:23:38.000000000","message":"It feels like this is where we should be calling reserve, so its matches whats called in the previous attach case.","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"a38183aa08fe0f4ec856593100284861b2aeb6d7","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":250,"context_line":"        if do_check_attach:"},{"line_number":251,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":252,"context_line":"                                               instance\u003dinstance)"},{"line_number":253,"context_line":""},{"line_number":254,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"},{"line_number":255,"context_line":"        context \u003d context.elevated()"}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_0f30a0e7","line":252,"in_reply_to":"dada55a8_2e201d49","updated":"2016-07-25 17:49:35.000000000","message":"\u0027reserve_volume\u0027 has already been called on the API for instance in case of attach, see this diagram: https://drive.google.com/file/d/0B1Kp6K43HLHyRkFMZHVHWDJ3amM/view?pref\u003d2\u0026pli\u003d1\n\n\nIn general it is too late to call it here. And to be honest Nova most probably calls this here in certain cases to make everyone relaxed that nothing goes wrong or maybe it helps with race conditions that I don\u0027t know.\n\n\u0027check_attach\u0027 here is called only in a few cases not all the time. We need to ensure we have \u0027reserve_volume\u0027 called in every flow preferably in the API, where it is needed as opposed to this down in the flow.","commit_id":"83672e3274efc4284d42e0bf5ba8bef6fa9fef32"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"797c5013e9c5c23ebda189f78f00bb676cce8568","unresolved":false,"context_lines":[{"line_number":248,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":249,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":250,"context_line":"        if do_check_attach:"},{"line_number":251,"context_line":"            volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"},{"line_number":254,"context_line":"        context \u003d context.elevated()"}],"source_content_type":"text/x-python","patch_set":17,"id":"5a8bc5a2_51ec768f","side":"PARENT","line":251,"range":{"start_line":251,"start_character":23,"end_line":251,"end_character":35},"updated":"2016-09-28 02:02:50.000000000","message":"I guess this is OK because nova/virt/block_device is really at the same service version as the compute manager in this case so we should be alright.","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"708348d332368e94677d27c637cd4af1f506140a","unresolved":false,"context_lines":[{"line_number":248,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":249,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":250,"context_line":"        if do_check_attach:"},{"line_number":251,"context_line":"            volume_api.check_attach(context, volume, instance\u003dinstance)"},{"line_number":252,"context_line":""},{"line_number":253,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"},{"line_number":254,"context_line":"        context \u003d context.elevated()"}],"source_content_type":"text/x-python","patch_set":17,"id":"3a98d1f4_22298d8a","side":"PARENT","line":251,"range":{"start_line":251,"start_character":23,"end_line":251,"end_character":35},"in_reply_to":"5a8bc5a2_51ec768f","updated":"2016-10-05 10:28:14.000000000","message":"This change removes the volume status check here and leaves the availability zone check in place. I don\u0027t see what issue it can possibly cause as we don\u0027t do any additional steps like reserve here. Also the API will get updated first, so this should be fine.","commit_id":"c7bf2baa266a5f69c3b8249b50bf1f43d38a3cea"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"69d00689386e01e22050356c99ab723b330ed73f","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        if do_check_attach:"},{"line_number":250,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":251,"context_line":"                                               instance\u003dinstance)"},{"line_number":252,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"9a629dbe_b49574e5","line":249,"updated":"2016-11-08 17:44:31.000000000","message":"probably should make some comment here about not actually checking attach.  Or we should refactor the name do_check_attach to something else, since we aren\u0027t doing it anymore.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"48b1fa09b714b944b35197074876e11c91caf004","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        if do_check_attach:"},{"line_number":250,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":251,"context_line":"                                               instance\u003dinstance)"},{"line_number":252,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_fbbd27d8","line":249,"in_reply_to":"1a6eadb0_77e20dd7","updated":"2016-12-13 17:21:41.000000000","message":"Yes, that\u0027s exactly the case and I had the exact same thought. My expectation is that check_availability_zone will never fail here, but I\u0027m not 100% confident with this statement so it seems safer to leave it here.\n\nWould be happy to remove it though if we get to that conclusion.","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":5997,"name":"Walt","display_name":"Hemna","email":"waboring@hemna.com","username":"walter-boring","status":"SAP"},"change_message_id":"3b1be1717200bc31ee538299239455bf1c88c4c1","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        if do_check_attach:"},{"line_number":250,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":251,"context_line":"                                               instance\u003dinstance)"},{"line_number":252,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_77e20dd7","line":249,"in_reply_to":"1a6eadb0_e593d9f8","updated":"2016-12-13 17:07:22.000000000","message":"I think the AZ check was happening every time check_attach was called, but is there ever a case where checking the AZ would be a problem?  If not, then just nuke the do_check_attach completely?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"bc230bf4707f22f5b67b70e7e848492ef4fb614d","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        if do_check_attach:"},{"line_number":250,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":251,"context_line":"                                               instance\u003dinstance)"},{"line_number":252,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_e593d9f8","line":249,"in_reply_to":"9a629dbe_9a91729c","updated":"2016-12-13 12:14:26.000000000","message":"Walter, good catch.\n\nYeah, check_availability_zone is a cheap metadata operation, so I think its safe to just call it all the time:\nhttps://github.com/openstack/nova/blob/master/nova/volume/cinder.py#L290","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"864b63ca7188e68264411f740a1d893fb0edd82e","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_check_attach\u003dTrue, do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        if do_check_attach:"},{"line_number":250,"context_line":"            volume_api.check_availability_zone(context, volume,"},{"line_number":251,"context_line":"                                               instance\u003dinstance)"},{"line_number":252,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"9a629dbe_9a91729c","line":249,"in_reply_to":"9a629dbe_b49574e5","updated":"2016-11-08 20:06:09.000000000","message":"To be honest I wonder whether we need this at all in the sense of the real deal here was checking or not checking the volume state as oppose to the availability zone I think.\n\nAs an alternative, I can rename it to do_check_av_zone also if that makes sense. What do you think?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":280,"context_line":"                                   \u0027mountpoint\u0027: self[\u0027mount_device\u0027]},"},{"line_number":281,"context_line":"                                  instance\u003dinstance)"},{"line_number":282,"context_line":"                    volume_api.terminate_connection(context, volume_id,"},{"line_number":283,"context_line":"                                                    connector)"},{"line_number":284,"context_line":"        self[\u0027connection_info\u0027] \u003d connection_info"},{"line_number":285,"context_line":"        if self.volume_size is None:"},{"line_number":286,"context_line":"            self.volume_size \u003d volume.get(\u0027size\u0027)"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_a93c5b94","line":283,"updated":"2016-12-13 12:30:51.000000000","message":"Do we need unreserve in these sorts of cases?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"c8c6b3e294ac1e652f9e6d31ef2543b4931acd41","unresolved":false,"context_lines":[{"line_number":317,"context_line":"                    # Cinder-volume might have completed volume attach. So"},{"line_number":318,"context_line":"                    # we should detach the volume. If the attach did not"},{"line_number":319,"context_line":"                    # happen, the detach request will be ignored."},{"line_number":320,"context_line":"                    volume_api.detach(context, volume_id)"},{"line_number":321,"context_line":""},{"line_number":322,"context_line":"    @update_db"},{"line_number":323,"context_line":"    def refresh_connection_info(self, context, instance,"}],"source_content_type":"text/x-python","patch_set":20,"id":"1a6eadb0_695323e2","line":320,"updated":"2016-12-13 12:30:51.000000000","message":"I guess that does unreserve for us?","commit_id":"11e764271f316499685c0f175c824164a3a9cb1a"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f6a9baa01d06d3d377ff5d5fee99e24acb81b3f9","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"fa31d9ce_1384e108","line":249,"updated":"2017-02-16 03:18:27.000000000","message":"Is this right? There were cases where do_check_attach was passed in as False from the compute manager before this change, which meant those operations didn\u0027t care about checking the volume attach status or the AZ, but here it\u0027s an unconditional check of the AZ, which is a change in behavior.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f67290cbc2f42fe40170df8a68bb11b066b84110","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_5358e3fa","line":249,"in_reply_to":"da36d5c6_0431c70b","updated":"2017-02-20 18:49:13.000000000","message":"@Sylain, in the case of someone that doesn\u0027t care about AZ filtering, they should have CONF.cinder.cross_az_attach\u003dTrue (which is the default).","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"652588b14918ab73bae73d068a29ee8b0b1e2209","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_0431c70b","line":249,"in_reply_to":"da36d5c6_24d5eb42","updated":"2017-02-20 16:22:46.000000000","message":"My above scenario can even be simplified to the specific usecase where operators don\u0027t enable the AZFilter. In that case, Nova was blindly accepting to have compute AZs that weren\u0027t matching the instance AZ (which is an user flag, meaning that the end-user has no assumptions at all about whether AZs are enforced in a specific cloud).\n\nWith that change, it would break an unshelve action and would result in an ERROR state for the instance if by chance the instance is scheduled onto a compute that is not having the same AZ. In that case, the call to az.instance_get_availability_zone() that we make in volume_api.check_availability_zone() would return an AZ that would be different from the volume AZ and that could be a problem if cross_az_attach is set to False.\n\nI\u0027m just saying we add a check that would return a different behaviour in some specific case. At least worth a big upgrade note to me.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"906c8b47cac7d3d5fcf630c85144b5555f48a6e6","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_e1676123","line":249,"in_reply_to":"da36d5c6_413f4d25","updated":"2017-02-18 18:32:09.000000000","message":"Jay, for attach_volume in the API, the AZ is checked between the volume and instance here:\n\nhttps://github.com/openstack/nova/blob/6d64b7274410ae45b95bd7ac7f702c16daaa0fcd/nova/compute/api.py#L3574\n\nIn the case of unshelve I think you\u0027re right. That seems like a separate bug because we could have had a volume attached to an instance in AZ X, then shelved it (and shelved offloaded it), then when we unshelve, the scheduler could put the instance in AZ Y which is not compatible with the AZ for the volume, and then the unshelve is going to fail here.\n\nI might post to the ML about this to see if it\u0027s an oversight or intentional.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"00f091f7f0560d88f1902653e67215a338047c62","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_73d6c7c6","line":249,"in_reply_to":"da36d5c6_5358e3fa","updated":"2017-02-20 18:59:58.000000000","message":"Sure, I just think we should add a note for this, that\u0027s it.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1046d255f057ed824ed49dc37143df31c9cab356","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_b30f5ee0","line":249,"in_reply_to":"da36d5c6_73d6c7c6","updated":"2017-02-20 19:24:38.000000000","message":"Yeah I\u0027m totally fine adding a release note saying that when unshelving an instance we\u0027ll be checking the AZ on the instance against the volume, so if you don\u0027t care about AZs (e.g. don\u0027t use the AZ scheduler filter), then ensure you\u0027ve got nova-compute configured with [cinder]/cross_az_attach\u003dTrue.\n\nFWIW, I think we want a release note for this anyway because the boot from volume flow will now reserve the volume in the API before creating the instance on the compute.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"cfb8d4bd9eb5fbed072c0e6c112c3a4edf104f8f","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_24d5eb42","line":249,"in_reply_to":"da36d5c6_9746e7bd","updated":"2017-02-20 16:07:54.000000000","message":"Just to make it clear, nothing was preventing pre-Mitaka to have the instance being unshelved on a different compute that wouldn\u0027t be in the same AZ than the original compute.\n\nIn Mitaka, we stored the original query and we looked up at it when unshelving, so now the scheduler knows the AZ query.\n\nThat said, all of those AZ checks are only done if we enable the AZFilter. Some operators that don\u0027t care at all with AZs disable that filter (which is defaulted to run), so here I\u0027m wondering how what could be the impact for them if we would force them to stick with some AZ.\n\nJust to be clear, say the following scenario : \n- operator started to use AZs\n- by some time, the operator no longer wants to use AZs, so their stop enabling the AZFilter\n- instances are being scheduled on hosts that don\u0027t match the AZ constraints, but that\u0027s okay because the operator explicitly wanted to not verify those.\n- now, with that change, an unshelve would break them because it\u0027s forcing to verify the AZ.\n\nI assume it\u0027s a tricky and corner case, and by some extent, we should say to operators that they need to clean up their instances, but I\u0027m still a bit worried by that.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"002fcd072d1608ea2ff5553e112995302575d09a","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_9746e7bd","line":249,"in_reply_to":"da36d5c6_a17e49ac","updated":"2017-02-19 02:39:11.000000000","message":"It was needed for the attach case too as during the flow \u0027check_attach\u0027 would\u0027ve been called twice otherwise. Between the two calls \u0027reserve_volume\u0027 changes the volume state to \u0027attaching\u0027 which would\u0027ve made the second call fail that\u0027s why the second attempt got the \u0027False\u0027 flag.\n\nSee the diagram here: https://drive.google.com/file/d/0B1Kp6K43HLHyRkFMZHVHWDJ3amM/view?pref\u003d2\u0026pli\u003d1\n\nAs for \u0027unshelve\u0027 and other cases I lived with the assumption that the AZ should be already checked/work.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"3e8ea1bba8f7f341a32d64a651ee8f555d930b43","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_a17e49ac","line":249,"in_reply_to":"da36d5c6_e1676123","updated":"2017-02-18 18:39:32.000000000","message":"It looks like our friend do_check_attach was added specifically for the unshelve case:\n\n8831f3d0e2e2a81a6b406cc9c8bf89bc15989065\n\nBecause when we shelve the instance we don\u0027t detach the volume and when we unshelve it, it would fail because check_attach would be called and check the attach status on the already attached volume. So 8831f3d0e2e2a81a6b406cc9c8bf89bc15989065 bypassed that call to check_attach so we could unshelve the instance that already has a volume attached to it.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"ed3b188c2cf818483cf721d2369ed4f77f1303af","unresolved":false,"context_lines":[{"line_number":246,"context_line":"    def attach(self, context, instance, volume_api, virt_driver,"},{"line_number":247,"context_line":"               do_driver_attach\u003dFalse, **kwargs):"},{"line_number":248,"context_line":"        volume \u003d volume_api.get(context, self.volume_id)"},{"line_number":249,"context_line":"        volume_api.check_availability_zone(context, volume,"},{"line_number":250,"context_line":"                                           instance\u003dinstance)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        volume_id \u003d volume[\u0027id\u0027]"}],"source_content_type":"text/x-python","patch_set":38,"id":"da36d5c6_413f4d25","line":249,"in_reply_to":"fa31d9ce_1384e108","updated":"2017-02-18 17:56:25.000000000","message":"The two cases where do_check_attach\u003dFalse are passed to the compute manager\u0027s _prep_block_device() are:\n\n * unshelve_instance()\n * attach_volume()\n\nIn both of those cases, there is an assumption that the availability zone of the volume matches the compute node upon which the instance is being operated against.\n\nIn the case of attach_volume(), I see no nothing in the Nova API server that is checking that the volume\u0027s availability zone matches the instance\u0027s compute node AZ, and therefore while this is indeed a change in behaviour, I think it\u0027s a sane one even though it likely needs a release note describing that this is filling a gap in verification that exists today.\n\nIn the case of unshelve_instance(), there is nothing in the Nova API code that checks that volumes associated with an instance being unshelved are in the same AZ as the compute host being unshelved to. The assumption must be that the check for AZ must have happened on initial boot and therefore it\u0027s not necessary to check again on unshelve. I don\u0027t think checking is harmful, just wasteful in this case.","commit_id":"762fb5dc7d79fc0d8da308d87d04af0426c4f602"}],"nova/volume/cinder.py":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"1e5ddf2d35812f48e8a7c26c0abb40896f1966cc","unresolved":false,"context_lines":[{"line_number":274,"context_line":"                                              \"status\": volume[\u0027status\u0027]}"},{"line_number":275,"context_line":"            raise exception.InvalidVolume(reason\u003dmsg)"},{"line_number":276,"context_line":""},{"line_number":277,"context_line":"    def check_attach(self, context, volume, instance\u003dNone):"},{"line_number":278,"context_line":"        # TODO(vish): abstract status checking?"},{"line_number":279,"context_line":"        if volume[\u0027status\u0027] !\u003d \"available\":"},{"line_number":280,"context_line":"            msg \u003d _(\"volume \u0027%(vol)s\u0027 status must be \u0027available\u0027. Currently \""}],"source_content_type":"text/x-python","patch_set":8,"id":"dada55a8_2ee77d13","side":"PARENT","line":277,"updated":"2016-07-25 10:23:38.000000000","message":"Whoop!","commit_id":"5415d621c000630514c0bdb1008dcf9c97740599"}],"releasenotes/notes/remove-check-attach-68b9ec781ad184ff.yaml":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixes `bug 1581230`_ by removing the ``check_attach`` call from the Nova"},{"line_number":5,"context_line":"    code, which made Nova decoupled from Cinder\u0027s possible updates in volume"},{"line_number":6,"context_line":"    state and was causing possible race conditions. ``reserve_volume`` is called"},{"line_number":7,"context_line":"    in every volume attach scenario to provide the necessary checks and volume"},{"line_number":8,"context_line":"    state change on the Cinder side."}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_753d0fc3","line":5,"range":{"start_line":5,"start_character":16,"end_line":5,"end_character":35},"updated":"2017-02-26 16:26:52.000000000","message":"\"which decouples Nova from possible volume state validation in Cinder\"\n\nSomething like that. The \"Fixes\" and \"made\" messes up the present and past tense.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixes `bug 1581230`_ by removing the ``check_attach`` call from the Nova"},{"line_number":5,"context_line":"    code, which made Nova decoupled from Cinder\u0027s possible updates in volume"},{"line_number":6,"context_line":"    state and was causing possible race conditions. ``reserve_volume`` is called"},{"line_number":7,"context_line":"    in every volume attach scenario to provide the necessary checks and volume"},{"line_number":8,"context_line":"    state change on the Cinder side."}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_153773ff","line":5,"range":{"start_line":5,"start_character":16,"end_line":5,"end_character":35},"in_reply_to":"da36d5c6_753d0fc3","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":8,"context_line":"    state change on the Cinder side."},{"line_number":9,"context_line":"other:"},{"line_number":10,"context_line":"  - |"},{"line_number":11,"context_line":"    By removing the ``check_attach`` internal call from Nova small behavioral"},{"line_number":12,"context_line":"    changes were introduced."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    ``reserve_volume`` call was added to the boot from volume scenario. In case"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_b53307f5","line":11,"range":{"start_line":11,"start_character":56,"end_line":11,"end_character":60},"updated":"2017-02-26 16:26:52.000000000","message":"Nova,","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":8,"context_line":"    state change on the Cinder side."},{"line_number":9,"context_line":"other:"},{"line_number":10,"context_line":"  - |"},{"line_number":11,"context_line":"    By removing the ``check_attach`` internal call from Nova small behavioral"},{"line_number":12,"context_line":"    changes were introduced."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    ``reserve_volume`` call was added to the boot from volume scenario. In case"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_353ab7e7","line":11,"range":{"start_line":11,"start_character":56,"end_line":11,"end_character":60},"in_reply_to":"da36d5c6_b53307f5","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":11,"context_line":"    By removing the ``check_attach`` internal call from Nova small behavioral"},{"line_number":12,"context_line":"    changes were introduced."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    ``reserve_volume`` call was added to the boot from volume scenario. In case"},{"line_number":15,"context_line":"    failure in the operation the instance goes into ERROR state while the volume"},{"line_number":16,"context_line":"    stays in ``attaching`` state. The volume state will be set back to"},{"line_number":17,"context_line":"    ``available`` when the instance gets deleted."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_35563706","line":15,"range":{"start_line":14,"start_character":72,"end_line":15,"end_character":28},"updated":"2017-02-26 16:26:52.000000000","message":"\"In case a failure occurs while building the instance, the instance goes into ERROR state...\"","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":11,"context_line":"    By removing the ``check_attach`` internal call from Nova small behavioral"},{"line_number":12,"context_line":"    changes were introduced."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"    ``reserve_volume`` call was added to the boot from volume scenario. In case"},{"line_number":15,"context_line":"    failure in the operation the instance goes into ERROR state while the volume"},{"line_number":16,"context_line":"    stays in ``attaching`` state. The volume state will be set back to"},{"line_number":17,"context_line":"    ``available`` when the instance gets deleted."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_7528af9a","line":15,"range":{"start_line":14,"start_character":72,"end_line":15,"end_character":28},"in_reply_to":"da36d5c6_35563706","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"    Additional availability zone check is added to the volume attach flow, which"},{"line_number":20,"context_line":"    results in an availability zone check when an instance gets unshelved. In"},{"line_number":21,"context_line":"    case the deployment is not sensitive to availability zones and not using the"},{"line_number":22,"context_line":"    scheduler filter the following configuration parameter needs to be enabled in"},{"line_number":23,"context_line":"    ``nova.conf``::"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"      [cinder]"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_75442f49","line":22,"range":{"start_line":21,"start_character":67,"end_line":22,"end_character":20},"updated":"2017-02-26 16:26:52.000000000","message":"\"not using the AvailabilityZoneFilter scheduler filter\"","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":18,"context_line":""},{"line_number":19,"context_line":"    Additional availability zone check is added to the volume attach flow, which"},{"line_number":20,"context_line":"    results in an availability zone check when an instance gets unshelved. In"},{"line_number":21,"context_line":"    case the deployment is not sensitive to availability zones and not using the"},{"line_number":22,"context_line":"    scheduler filter the following configuration parameter needs to be enabled in"},{"line_number":23,"context_line":"    ``nova.conf``::"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"      [cinder]"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_f51adf7b","line":22,"range":{"start_line":21,"start_character":67,"end_line":22,"end_character":20},"in_reply_to":"da36d5c6_75442f49","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c48a103256736c53a38f83a1ae9ef3a3315267b4","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    ``nova.conf``::"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"      [cinder]"},{"line_number":26,"context_line":"      cross_az_attach\u003dTrue"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    .. _`bug 1581230`: https://bugs.launchpad.net/nova/+bug/1581230"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_b54a2753","line":26,"updated":"2017-02-26 16:26:52.000000000","message":"This is actually the default already if not set explicitly. I\u0027d like to avoid scaring operators into thinking they have to set that explicitly which they wouldn\u0027t have to if they aren\u0027t enabling the AvailabilityZoneFilter filter.","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":9562,"name":"Ildiko Vancsa","email":"ildiko.vancsa@gmail.com","username":"ildikov"},"change_message_id":"57d5497082b7c5365a5304e4a9603166153c39dd","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    ``nova.conf``::"},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"      [cinder]"},{"line_number":26,"context_line":"      cross_az_attach\u003dTrue"},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"    .. _`bug 1581230`: https://bugs.launchpad.net/nova/+bug/1581230"}],"source_content_type":"text/x-yaml","patch_set":46,"id":"da36d5c6_950a8324","line":26,"in_reply_to":"da36d5c6_b54a2753","updated":"2017-02-26 18:45:55.000000000","message":"Done","commit_id":"0ba7c6fe323ebf5a4e90353614a4917101cc7cfd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d00fe358bddd90ae6de45d00a6ebe8376fe54a4d","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    Fixes `bug 1581230`_ by removing the internal ``check_attach`` call from"},{"line_number":5,"context_line":"    the Nova code as it can cause race conditions and the checks are handled by"},{"line_number":6,"context_line":"    ``reserve_volume`` in Cinder. ``reserve_volume`` is called in every volume"},{"line_number":7,"context_line":"    attach scenario to provide the necessary checks and volume state change on"},{"line_number":8,"context_line":"    the Cinder side."},{"line_number":9,"context_line":"other:"},{"line_number":10,"context_line":"  - |"}],"source_content_type":"text/x-yaml","patch_set":47,"id":"da36d5c6_4ec79595","line":7,"range":{"start_line":7,"start_character":69,"end_line":7,"end_character":75},"updated":"2017-02-27 01:30:57.000000000","message":"validation","commit_id":"bb35446251f1604c687507ade13d8533bcf2c5a4"}]}