)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":7,"context_line":"WIP compute: Recreate attachments during a reschedule"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"WIP as I\u0027m not sure that this is the best place for this logic, maybe"},{"line_number":10,"context_line":"this needs to be over in the conductor or higher in the compute API?"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I739c06bd02336bf720cddacb21f48e7857378487"},{"line_number":13,"context_line":"Closes-bug: #1784353"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"5f7c97a3_d5751c36","line":10,"range":{"start_line":10,"start_character":39,"end_line":10,"end_character":67},"updated":"2018-08-02 14:32:17.000000000","message":"We won\u0027t hit the compute API during a reschedule. Once we\u0027re in a cell we should assume we can\u0027t get out.\n\nWe could potentially do some cleanup in the cell conductor in the build_instances method which is where we get during a reschedule:\n\nhttps://github.com/openstack/nova/blob/7125dcb9cb821faf3c68526ac34365a28141e480/nova/conductor/manager.py#L519\n\nBut it gets confusing having cleanup code strewn throughout both the compute manager and conductor, even though we do network cleanup in conductor if we\u0027ve exhausted our retries:\n\nhttps://github.com/openstack/nova/blob/7125dcb9cb821faf3c68526ac34365a28141e480/nova/conductor/manager.py#L613\n\nAlthough that network cleanup code in conductor, which was added in 2015:\n\nIc670dd4dc192603c2faecf18e14ef59ebca9e420\n\nIs probably dead code now since change Ib2abf73166598ff14fce4e935efe15eeea0d4f7d in compute makes us always deallocate networking before rescheduling.\n\nAnyway, point is, I think we want to keep cleanup code local to compute *before* we reschedule if possible since that\u0027s where we know what is going on.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6be999988a5145d688aab6a9ccc38b6973e16539","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When an instance with attached volumes fails to spawn cleanup code"},{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder . As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when spawning an instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I739c06bd02336bf720cddacb21f48e7857378487"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_fa1da10f","line":11,"range":{"start_line":11,"start_character":21,"end_line":11,"end_character":22},"updated":"2018-08-03 18:53:37.000000000","message":"nix","commit_id":"375c18ff0491c83cc540c7d7e10bcd41a979347f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6be999988a5145d688aab6a9ccc38b6973e16539","unresolved":false,"context_lines":[{"line_number":9,"context_line":"When an instance with attached volumes fails to spawn cleanup code"},{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder . As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when spawning an instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I739c06bd02336bf720cddacb21f48e7857378487"},{"line_number":15,"context_line":"Closes-bug: #1784353"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3f79a3b5_5ad29536","line":12,"range":{"start_line":12,"start_character":39,"end_line":12,"end_character":47},"updated":"2018-08-03 18:53:37.000000000","message":"rescheduling","commit_id":"375c18ff0491c83cc540c7d7e10bcd41a979347f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"conductor: Recreate volume attachments during a reschedule"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When an instance with attached volumes fails to spawn cleanup code"},{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder. As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when rescheduling an instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"3f79a3b5_2a8a8412","line":9,"range":{"start_line":9,"start_character":62,"end_line":9,"end_character":66},"updated":"2018-10-22 19:00:11.000000000","message":"Would be good to mention specifically that it\u0027s the last except block in the _build_resources method that calls _shutdown_instance. While looking over the bug report comments again, I remember it taking me awhile to connect the dots here.","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"conductor: Recreate volume attachments during a reschedule"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When an instance with attached volumes fails to spawn cleanup code"},{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder. As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when rescheduling an instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"3f79a3b5_0a822800","line":9,"range":{"start_line":9,"start_character":48,"end_line":9,"end_character":53},"updated":"2018-10-22 19:00:11.000000000","message":"nit: spawn, cleanup code...","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"conductor: Recreate volume attachments during a reschedule"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When an instance with attached volumes fails to spawn cleanup code"},{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder. As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when rescheduling an instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"3f79a3b5_4026bfbb","line":9,"range":{"start_line":9,"start_character":62,"end_line":9,"end_character":66},"in_reply_to":"3f79a3b5_2a8a8412","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder. As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when rescheduling an instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I739c06bd02336bf720cddacb21f48e7857378487"},{"line_number":15,"context_line":"Closes-bug: #1784353"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"3f79a3b5_eae1ecd0","line":13,"updated":"2018-10-22 19:00:11.000000000","message":"It\u0027s been awhile, but I know we discussed a few alternative fixes for this, and the pros/cons with each. It would be nice to have mentioned those in the commit message to know we put thought into it. There are a few arguments for doing it in compute though, either by not cleaning up the attachments in _shutdown_instance:\n\nhttps://review.openstack.org/#/c/587071/3/nova/compute/manager.py@1631\n\nOr after we\u0027ve rescheduled:\n\nhttps://review.openstack.org/#/c/587071/4/nova/compute/manager.py@1667\n\nThe problem with *not* cleaning up the attachments is if we don\u0027t reschedule, then we\u0027ve left orphaned \"reserved\" volumes in Cinder (or we have to add special logic to tell compute when to cleanup attachments).\n\nThe problem with checking the existence of the attachment on every new host we build on is that we\u0027d be needlessly checking that for initial creates even if we don\u0027t ever need to reschedule, unless again we have special logic against that (like checking to see if we\u0027ve rescheduled at all).\n\nSo ultimately it seems that the best way to handle this is (1) only deal with this on reschedules and (2) let the cell conductor orchestrate it since it\u0027s already dealing with the reschedule. Then the compute logic doesn\u0027t need to change.","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":10,"context_line":"within the compute manager will delete the volume attachments referenced"},{"line_number":11,"context_line":"by the bdms in Cinder. As a result we should check and if necessary"},{"line_number":12,"context_line":"recreate these volume attachments when rescheduling an instance."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: I739c06bd02336bf720cddacb21f48e7857378487"},{"line_number":15,"context_line":"Closes-bug: #1784353"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":14,"id":"3f79a3b5_202383ac","line":13,"in_reply_to":"3f79a3b5_eae1ecd0","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"}],"nova/compute/manager.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"525e9f6eb7c14beb287c1fec80d938d237cb0942","unresolved":false,"context_lines":[{"line_number":1608,"context_line":"            bdm.update(values)"},{"line_number":1609,"context_line":"            bdm.save()"},{"line_number":1610,"context_line":""},{"line_number":1611,"context_line":"    def _validate_existing_attachment_ids(self, context, instance, bdms):"},{"line_number":1612,"context_line":"        \"\"\"Ensure any attachment ids referenced by the bdms exist\"\"\""},{"line_number":1613,"context_line":"        for bdm in bdms:"},{"line_number":1614,"context_line":"            if bdm.is_volume and bdm.attachment_id:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_9b6593b3","line":1611,"updated":"2018-08-02 14:49:37.000000000","message":"I guess we could do this in build_instances in conductor if we know we\u0027re doing a reschedule...then we\u0027d avoid the lookup during a normal server build (or rebuild), and then it\u0027s more control-plane(ish)? Maybe see what dansmith thinks.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4d78f0265328d9bb91c4e4cc877465790e1b9ef6","unresolved":false,"context_lines":[{"line_number":1609,"context_line":"            bdm.save()"},{"line_number":1610,"context_line":""},{"line_number":1611,"context_line":"    def _validate_existing_attachment_ids(self, context, instance, bdms):"},{"line_number":1612,"context_line":"        \"\"\"Ensure any attachment ids referenced by the bdms exist\"\"\""},{"line_number":1613,"context_line":"        for bdm in bdms:"},{"line_number":1614,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":1615,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_f87dadc3","line":1612,"range":{"start_line":1612,"start_character":8,"end_line":1612,"end_character":68},"updated":"2018-08-02 14:34:20.000000000","message":"You could probably expand on this to explain the reschedule wrinkle and the fact that if the bdm.attachment_id doesn\u0027t exist in cinder, we create a new attachment and update the bdm.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":1613,"context_line":"        for bdm in bdms:"},{"line_number":1614,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":1615,"context_line":"                try:"},{"line_number":1616,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":1617,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":1618,"context_line":"                    # VolumeAttachmentNotFound suggests that the attachment was"},{"line_number":1619,"context_line":"                    # deleted, possibily as the result of a failure to spawn"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_3596d03f","line":1616,"updated":"2018-08-02 14:32:17.000000000","message":"OK so if we get here, we know the bdm.attachment_id was set to something by the API, which does the necessary checks for cinder being new enough to handle the new style attachment flow, so that\u0027s probably OK (and is probably why you have the TODO on L1621 which I think you can remove).","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":1616,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":1617,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":1618,"context_line":"                    # VolumeAttachmentNotFound suggests that the attachment was"},{"line_number":1619,"context_line":"                    # deleted, possibily as the result of a failure to spawn"},{"line_number":1620,"context_line":"                    # the instance on another compute host."},{"line_number":1621,"context_line":"                    # TODO(lyarwood): Should we check the compute version?"},{"line_number":1622,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_b83f35b1","line":1619,"range":{"start_line":1619,"start_character":31,"end_line":1619,"end_character":40},"updated":"2018-08-02 14:32:17.000000000","message":"possibly","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":1628,"context_line":"        \"\"\"Set up the block device for an instance with error logging.\"\"\""},{"line_number":1629,"context_line":"        try:"},{"line_number":1630,"context_line":"            self._add_missing_dev_names(bdms, instance)"},{"line_number":1631,"context_line":"            self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":1632,"context_line":"            block_device_info \u003d driver.get_block_device_info(instance, bdms)"},{"line_number":1633,"context_line":"            mapping \u003d driver.block_device_info_get_mapping(block_device_info)"},{"line_number":1634,"context_line":"            driver_block_device.attach_block_devices("}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_b58740b4","line":1631,"updated":"2018-08-02 14:32:17.000000000","message":"Rather than do this, can\u0027t we avoid deleting the attachment in the first place *before* we reschedule? That\u0027s also kind of awkward though because before the new style attachment_id stuff, during a reschedule _shutdown_instance will disconnect the volume (via os-brick) through driver.destroy() and then call terminate_connection and detach APIs in Cinder.\n\nWith the new flow, we still disconnect the volume from the host and are currently deleting the attachment record.\n\nOn a reschedule with the old flow, during attach we\u0027d just initialize a new connection, and that\u0027s basically what we should be doing with attachment_create, and jgriffith always says \"attachments are cheap and plentiful and meant to be created and deleted often\".\n\nSo this is probably OK. One issue we might have with deleting the attachment in _shutdown_instance prior to rescheduling is it could leave a window of time where the volume is marked as \u0027available\u0027 because the attachment is what makes it \u0027reserved\u0027 during boot from volume. When we eventually call attachment_complete is when the volume goes to in-use status.\n\nLooking at the old flow, _shutdown_instance will call the os-detach volume action API in cinder which I think will get us here:\n\nhttps://github.com/openstack/cinder/blob/b0e9ee1d501fb83b7ebbc59584aa6255dbaec086/cinder/volume/manager.py#L1314\n\nAnd that would also make the volume available again for a short window of time while we reschedule. If that\u0027s the case, then we haven\u0027t made anything any worse...","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9d965c5656338c385c2db4714a2eab6f92874fdb","unresolved":false,"context_lines":[{"line_number":1628,"context_line":"        \"\"\"Set up the block device for an instance with error logging.\"\"\""},{"line_number":1629,"context_line":"        try:"},{"line_number":1630,"context_line":"            self._add_missing_dev_names(bdms, instance)"},{"line_number":1631,"context_line":"            self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":1632,"context_line":"            block_device_info \u003d driver.get_block_device_info(instance, bdms)"},{"line_number":1633,"context_line":"            mapping \u003d driver.block_device_info_get_mapping(block_device_info)"},{"line_number":1634,"context_line":"            driver_block_device.attach_block_devices("}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_98f59955","line":1631,"in_reply_to":"5f7c97a3_b58740b4","updated":"2018-08-02 14:33:39.000000000","message":"Also, if we don\u0027t cleanup attachments in _shutdown_instance prior to reschedule, we could reschedule, get to conductor, find we\u0027re out of retries and then we\u0027re done and conductor won\u0027t cleanup any attachments and the volume will be left in \u0027reserved\u0027 status until the instance is deleted (or the volume status is force-reset by an admin).","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"6be999988a5145d688aab6a9ccc38b6973e16539","unresolved":false,"context_lines":[{"line_number":1653,"context_line":"            bdm.update(values)"},{"line_number":1654,"context_line":"            bdm.save()"},{"line_number":1655,"context_line":""},{"line_number":1656,"context_line":"    def _validate_existing_attachment_ids(self, context, instance, bdms):"},{"line_number":1657,"context_line":"        \"\"\"Ensure any attachment ids referenced by the bdms exist."},{"line_number":1658,"context_line":""},{"line_number":1659,"context_line":"        New attachments will only be created if the attachment ids referenced"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_1adc1d45","line":1656,"updated":"2018-08-03 18:53:37.000000000","message":"TODO: I guess we could do this in build_instances in conductor if we know we\u0027re doing a reschedule...then we\u0027d avoid the lookup during a normal server build (or rebuild), and then it\u0027s more control-plane(ish)? Maybe see what dansmith thinks.","commit_id":"375c18ff0491c83cc540c7d7e10bcd41a979347f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d0ffd5934ebb45240ec5d51c5c94a2f3f639c014","unresolved":false,"context_lines":[{"line_number":1664,"context_line":"        for bdm in bdms:"},{"line_number":1665,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":1666,"context_line":"                try:"},{"line_number":1667,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":1668,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":1669,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":1670,"context_line":"                                        bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":4,"id":"3f79a3b5_a188471f","line":1667,"updated":"2018-08-06 13:33:30.000000000","message":"Is this enough to make sure that the later code will succeed? Meaning, is there any state the attachment could be in that would be terminal such that the updating of it during boot would fail some sequencing requirement?\n\nAlso, it seems a little weird to have to ping cinder for each attachment on every boot, when we only expect to need to do anything for reschedules. Why did you choose to do this instead of just make the code that prepares for the reschedule re-create empty attachments?\n\nIf it doesn\u0027t make sense to do it during shutdown, then I guess what Matt is suggesting above is letting conductor do it since it has context on the reschedule. And yeah, that seems like a better plan to me.","commit_id":"375c18ff0491c83cc540c7d7e10bcd41a979347f"}],"nova/conductor/manager.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"dc4d5ac2e749100cab084df8fb6e11aab9d7a1eb","unresolved":false,"context_lines":[{"line_number":524,"context_line":"        host will delete attachments before rescheduling."},{"line_number":525,"context_line":"        \"\"\""},{"line_number":526,"context_line":"        for bdm in bdms:"},{"line_number":527,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":528,"context_line":"                try:"},{"line_number":529,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":530,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":531,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":532,"context_line":"                                        bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_9b8573b0","line":529,"range":{"start_line":527,"start_character":0,"end_line":529,"end_character":78},"updated":"2018-08-06 17:16:11.000000000","message":"This seems like a strange thing. I can\u0027t find the code right now which deletes the attachment in compute, but why doesn\u0027t it also update the bdm to remove the attachment_id? It seems like a potential source of bugs to leave a dangling attachment_id around.\n\nIf it unset attachment_id I don\u0027t think we\u0027d have to go back to cinder here, because we\u0027d know it was deleted.","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d9b54f073dffc6d3fa8408ff3b134e6a289da573","unresolved":false,"context_lines":[{"line_number":524,"context_line":"        host will delete attachments before rescheduling."},{"line_number":525,"context_line":"        \"\"\""},{"line_number":526,"context_line":"        for bdm in bdms:"},{"line_number":527,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":528,"context_line":"                try:"},{"line_number":529,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":530,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":531,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":532,"context_line":"                                        bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_ced00054","line":529,"range":{"start_line":527,"start_character":0,"end_line":529,"end_character":78},"in_reply_to":"3f79a3b5_9b8573b0","updated":"2018-08-07 08:36:11.000000000","message":"https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L2503-L2505 - is where we delete the attachment during a reschedule","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"dbc7a77f12fe2ba9c701c9f3688c00371ca6af3d","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e\u003d 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_78a2dd9c","line":726,"updated":"2018-08-06 16:20:48.000000000","message":"Isn\u0027t this in the wrong place? IIUC this function is only called for CellsV1?","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"d9b54f073dffc6d3fa8408ff3b134e6a289da573","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e\u003d 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_4ebcf004","line":726,"in_reply_to":"3f79a3b5_3b91bf9a","updated":"2018-08-07 08:36:11.000000000","message":"ACK thanks for catching that!","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"dc4d5ac2e749100cab084df8fb6e11aab9d7a1eb","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e\u003d 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_3b91bf9a","line":726,"in_reply_to":"3f79a3b5_78a2dd9c","updated":"2018-08-06 17:16:11.000000000","message":"Ignore the above, I see this is used for reschedules.\n\nI think you want \u0027if num_attempts \u003e 1\u0027 here, though. Looks like num_attempts is 1 before first attempt (see schduler_utils.populate_retry(), called by schedule_and_build_instances first time round), then bumped again to 2 when we call build_instances on reschedule. Alternatively just \u0027else\u0027, given the block above?\n\nProbably doesn\u0027t matter in practise, as we should never be called with num_attempts \u003d\u003d 1 except for cellsv1, but it\u0027s a bit confusing.","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cae0e4f4449e963561beca0bbb7d1334bf258f22","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e\u003d 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_bb838f04","line":726,"in_reply_to":"3f79a3b5_78a2dd9c","updated":"2018-08-06 16:53:43.000000000","message":"No, see the NOTE above the method description.","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e58ea44a79890da705e8eef414cd57d98fd39ca6","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_644a2b28","line":726,"updated":"2018-08-07 10:50:46.000000000","message":"My preference[1]: I\u0027d use an else here, as the above block is explicitly for the case when it\u0027s not a reschedule, and this is explicitly for the case when it is a reschedule. Seems less prone to future maintenance errors to have a single test.\n\n[1] Somewhat stronger than a nit, but well short of worthy of a -1. We should define a taxonomy :)","commit_id":"2ca8f8c9e2e32477336644b393b53d902677baaf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"88ca5a3803a0a163da33fab7cd114c5a0be4b873","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_8c189075","line":726,"in_reply_to":"3f79a3b5_644a2b28","updated":"2018-08-09 15:00:30.000000000","message":"Agree with the else, either here or for the \"if is_reschedule:\" on L640.","commit_id":"2ca8f8c9e2e32477336644b393b53d902677baaf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"eeca8d3a5fc49fc1c79e0dc0ea3c0a321859dae1","unresolved":false,"context_lines":[{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_60331763","line":726,"in_reply_to":"3f79a3b5_8c189075","updated":"2018-10-17 16:15:48.000000000","message":"Done","commit_id":"2ca8f8c9e2e32477336644b393b53d902677baaf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff5f51410a2e133122295d97638ccbe9b5475c9","unresolved":false,"context_lines":[{"line_number":720,"context_line":"                        inst_mapping.destroy()"},{"line_number":721,"context_line":"                    return"},{"line_number":722,"context_line":""},{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":12,"id":"3f79a3b5_a5a15eaa","line":726,"range":{"start_line":723,"start_character":0,"end_line":726,"end_character":32},"updated":"2018-10-16 15:57:11.000000000","message":"Still needs to be addressed [1]\n\n[1] https://review.openstack.org/#/c/587071/9/nova/conductor/manager.py@726","commit_id":"3184ea9fbbfc6b7ca6e8ffc0dfb82845aa6873ec"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"eeca8d3a5fc49fc1c79e0dc0ea3c0a321859dae1","unresolved":false,"context_lines":[{"line_number":720,"context_line":"                        inst_mapping.destroy()"},{"line_number":721,"context_line":"                    return"},{"line_number":722,"context_line":""},{"line_number":723,"context_line":"            # NOTE(lyarwood): If this is a reschedule then recreate any"},{"line_number":724,"context_line":"            # attachments that were previously removed when cleaning up after"},{"line_number":725,"context_line":"            # failures to spawn etc."},{"line_number":726,"context_line":"            if num_attempts \u003e 1:"},{"line_number":727,"context_line":"                self._validate_existing_attachment_ids(context, instance, bdms)"},{"line_number":728,"context_line":""},{"line_number":729,"context_line":"            alts \u003d [(alt.service_host, alt.nodename) for alt in host_list]"}],"source_content_type":"text/x-python","patch_set":12,"id":"3f79a3b5_20351f44","line":726,"range":{"start_line":723,"start_character":0,"end_line":726,"end_character":32},"in_reply_to":"3f79a3b5_a5a15eaa","updated":"2018-10-17 16:15:48.000000000","message":"Done","commit_id":"3184ea9fbbfc6b7ca6e8ffc0dfb82845aa6873ec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        New attachments will only be created if the attachment ids referenced"},{"line_number":523,"context_line":"        by the bdms no longer exist. This can happen when an instance is"},{"line_number":524,"context_line":"        rescheduled after a failure to spawn as cleanup code on the original"},{"line_number":525,"context_line":"        host will delete attachments before rescheduling."},{"line_number":526,"context_line":"        \"\"\""},{"line_number":527,"context_line":"        for bdm in bdms:"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_aa83f4a9","line":524,"range":{"start_line":524,"start_character":68,"end_line":524,"end_character":76},"updated":"2018-10-22 19:00:11.000000000","message":"nit: previous","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":521,"context_line":""},{"line_number":522,"context_line":"        New attachments will only be created if the attachment ids referenced"},{"line_number":523,"context_line":"        by the bdms no longer exist. This can happen when an instance is"},{"line_number":524,"context_line":"        rescheduled after a failure to spawn as cleanup code on the original"},{"line_number":525,"context_line":"        host will delete attachments before rescheduling."},{"line_number":526,"context_line":"        \"\"\""},{"line_number":527,"context_line":"        for bdm in bdms:"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_bd7edcbd","line":524,"range":{"start_line":524,"start_character":68,"end_line":524,"end_character":76},"in_reply_to":"3f79a3b5_aa83f4a9","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":527,"context_line":"        for bdm in bdms:"},{"line_number":528,"context_line":"            if bdm.is_volume and bdm.attachment_id:"},{"line_number":529,"context_line":"                try:"},{"line_number":530,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":531,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":532,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":533,"context_line":"                                        bdm.volume_id, instance.uuid)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_0a446872","line":530,"range":{"start_line":530,"start_character":20,"end_line":530,"end_character":78},"updated":"2018-10-22 19:00:11.000000000","message":"This is kind of extraneous if we know that compute is going to cleanup the attachments anyway, but I\u0027m not sure we know that in all cases...there are probably cases where compute can fail, reschedule and we didn\u0027t cleanup any volumes.","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":529,"context_line":"                try:"},{"line_number":530,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":531,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":532,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":533,"context_line":"                                        bdm.volume_id, instance.uuid)"},{"line_number":534,"context_line":"                    bdm.attachment_id \u003d attachment[\u0027id\u0027]"},{"line_number":535,"context_line":"                    bdm.save()"},{"line_number":536,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_2ab9247b","line":533,"range":{"start_line":532,"start_character":67,"end_line":533,"end_character":68},"updated":"2018-10-22 19:00:11.000000000","message":"nit: wrap and align the args","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":529,"context_line":"                try:"},{"line_number":530,"context_line":"                    self.volume_api.attachment_get(context, bdm.attachment_id)"},{"line_number":531,"context_line":"                except exception.VolumeAttachmentNotFound:"},{"line_number":532,"context_line":"                    attachment \u003d self.volume_api.attachment_create(context,"},{"line_number":533,"context_line":"                                        bdm.volume_id, instance.uuid)"},{"line_number":534,"context_line":"                    bdm.attachment_id \u003d attachment[\u0027id\u0027]"},{"line_number":535,"context_line":"                    bdm.save()"},{"line_number":536,"context_line":""}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_9d83e0aa","line":533,"range":{"start_line":532,"start_character":67,"end_line":533,"end_character":68},"in_reply_to":"3f79a3b5_2ab9247b","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"}],"nova/tests/functional/regressions/test_bug_1784353.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":82,"context_line":"                \u0027block_device_mapping_v2\u0027: [{"},{"line_number":83,"context_line":"                    \u0027boot_index\u0027: 0,"},{"line_number":84,"context_line":"                    \u0027uuid\u0027: volume_id,"},{"line_number":85,"context_line":"                    \u0027source_type\u0027: \u0027volume\u0027,"},{"line_number":86,"context_line":"                    \u0027destination_type\u0027: \u0027volume\u0027}],"},{"line_number":87,"context_line":"            }"},{"line_number":88,"context_line":"            server_response \u003d self.api.post_server({\u0027server\u0027: server_request})"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_d5051c18","line":85,"updated":"2018-08-02 14:32:17.000000000","message":"Hmm, what would happen with a boot from volume where nova creates the volume? Let\u0027s say source_type\u003d\u0027image\u0027 so nova creates the volume on host A. I guess currently we don\u0027t create an attachment record in that case:\n\nhttps://github.com/openstack/nova/blob/7125dcb9cb821faf3c68526ac34365a28141e480/nova/virt/block_device.py#L743\n\nBut after my change (eventually) gets in, we would:\n\nhttps://review.openstack.org/#/c/541420/\n\nI don\u0027t think we\u0027d probably have any different issues in that case with a reschedule, except we might try to create a 2nd volume on the rescheduled host since it doesn\u0027t look like we delete the volume in _shutdown_instance.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":89,"context_line":"            server_id \u003d server_response[\u0027id\u0027]"},{"line_number":90,"context_line":""},{"line_number":91,"context_line":"            self._wait_for_state_change(self.api, server_response, \u0027ACTIVE\u0027)"},{"line_number":92,"context_line":"            self.assertTrue(self.cinder.attachments[\u0027attachment_id\u0027])"},{"line_number":93,"context_line":"            self.assertTrue(self.cinder.attachments[\u0027instance_uuid\u0027])"},{"line_number":94,"context_line":"            self.assertIn(server_id, self.cinder.attachments)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_78767d0e","line":92,"range":{"start_line":92,"start_character":17,"end_line":92,"end_character":27},"updated":"2018-08-02 14:32:17.000000000","message":"Use assertIn rather than assertTrue.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"dc4d5ac2e749100cab084df8fb6e11aab9d7a1eb","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        server_response \u003d self.api.post_server({\u0027server\u0027: server_request})"},{"line_number":81,"context_line":"        server_id \u003d server_response[\u0027id\u0027]"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        self._wait_for_state_change(self.api, server_response, \u0027ACTIVE\u0027)"},{"line_number":84,"context_line":"        self.assertIn(volume_id, self.cinder.volume_to_attachment)"},{"line_number":85,"context_line":"        self.assertIn(server_id, self.cinder.attachments[\u0027instance_uuid\u0027])"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_7be65719","line":85,"range":{"start_line":83,"start_character":0,"end_line":85,"end_character":74},"updated":"2018-08-06 17:16:11.000000000","message":"Seems weird to completely reverse the assertion of this test, but it makes sense.","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        self._wait_for_state_change(self.api, server_response, \u0027ACTIVE\u0027)"},{"line_number":84,"context_line":"        self.assertIn(volume_id,"},{"line_number":85,"context_line":"                      self.cinder.volume_ids_for_instance(server_id))"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_4a2fc0a5","line":85,"range":{"start_line":85,"start_character":22,"end_line":85,"end_character":68},"updated":"2018-10-22 19:00:11.000000000","message":"Should we make sure this is length 1 so that we\u0027re not leaking attachments?","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        self._wait_for_state_change(self.api, server_response, \u0027ACTIVE\u0027)"},{"line_number":84,"context_line":"        self.assertIn(volume_id,"},{"line_number":85,"context_line":"                      self.cinder.volume_ids_for_instance(server_id))"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_5d72284c","line":85,"range":{"start_line":85,"start_character":22,"end_line":85,"end_character":68},"in_reply_to":"3f79a3b5_4a2fc0a5","updated":"2018-10-22 19:36:07.000000000","message":"Oh I guess we can\u0027t b/c it yields... :(","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"}],"nova/tests/unit/compute/test_compute.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":1383,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_get\u0027)"},{"line_number":1384,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_create\u0027)"},{"line_number":1385,"context_line":"    @mock.patch.object(block_device_obj.BlockDeviceMapping, \u0027save\u0027)"},{"line_number":1386,"context_line":"    def test_prep_block_device_with_missing_attachments(self, mock_bdm_save,"},{"line_number":1387,"context_line":"                                                        mock_attachment_create,"},{"line_number":1388,"context_line":"                                                        mock_attachment_get):"},{"line_number":1389,"context_line":"        instance \u003d self._create_fake_instance_obj()"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_986ef92b","line":1386,"range":{"start_line":1386,"start_character":13,"end_line":1386,"end_character":30},"updated":"2018-08-02 14:32:17.000000000","message":"This should be called _validate_existing_attachment_ids instead since you\u0027re not actually calling _prep_block_device.","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d832f7e8fd2510e9e65e379eb20b5b59ff66fb29","unresolved":false,"context_lines":[{"line_number":1422,"context_line":"                       side_effect\u003dexception.OverQuota(overs\u003d\u0027something\u0027))"},{"line_number":1423,"context_line":"    @mock.patch.object(compute_manager.ComputeManager,"},{"line_number":1424,"context_line":"                       \u0027_validate_existing_attachment_ids\u0027)"},{"line_number":1425,"context_line":"    def test_prep_block_device_over_quota_failure(self, mock_validate,"},{"line_number":1426,"context_line":"                                                  mock_create):"},{"line_number":1427,"context_line":"        instance \u003d self._create_fake_instance_obj()"},{"line_number":1428,"context_line":"        bdms \u003d ["}],"source_content_type":"text/x-python","patch_set":3,"id":"5f7c97a3_b86bb51a","line":1425,"range":{"start_line":1425,"start_character":56,"end_line":1425,"end_character":69},"updated":"2018-08-02 14:32:17.000000000","message":"assert that this is called","commit_id":"5338638f633b1a1bcf9dae9fc8185cf4806f0bec"}],"nova/tests/unit/conductor/test_conductor.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"dc4d5ac2e749100cab084df8fb6e11aab9d7a1eb","unresolved":false,"context_lines":[{"line_number":1001,"context_line":"                                                       uuids.volume,"},{"line_number":1002,"context_line":"                                                       instance.uuid)"},{"line_number":1003,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1004,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1007,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3f79a3b5_1bc3a3ef","line":1004,"updated":"2018-08-06 17:16:11.000000000","message":"Do we need to test the case where compute failed without having deleted the initial blank attachment? Can\u0027t find the code which does that so haven\u0027t checked.","commit_id":"7bc5c436e84ea4dfffb3b487f9978e8850fc7d30"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"e58ea44a79890da705e8eef414cd57d98fd39ca6","unresolved":false,"context_lines":[{"line_number":1001,"context_line":"                                                       uuids.volume,"},{"line_number":1002,"context_line":"                                                       instance.uuid)"},{"line_number":1003,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1004,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1007,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_643eab2a","line":1004,"updated":"2018-08-07 10:50:46.000000000","message":"Thanks for the pointer to where we delete attachments in _shutdown_instance.\n\nI checked that flow and there\u0027s a pretty wide window of opportunity for a failure in build_and_run_instance which doesn\u0027t result in this cleanup happening: failure to update the instance record a few times, failure to send a notification, tagging or trusted certs not supported, instance  group policy fails validation, synchronous failure of _build_networks_for_instance if we\u0027re rescheduling, or, for ironic, failure of prepare_for_spawn() or prepare_networks_before_block_device_mapping().\n\nIOW, I think we need test here for the behaviour when attachment_id still points to a valid attachment.","commit_id":"2ca8f8c9e2e32477336644b393b53d902677baaf"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2122ce0a6574a527736bffe2357a19a576f8ffdc","unresolved":false,"context_lines":[{"line_number":1001,"context_line":"                                                       uuids.volume,"},{"line_number":1002,"context_line":"                                                       instance.uuid)"},{"line_number":1003,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1004,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1007,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":9,"id":"3f79a3b5_91550720","line":1004,"in_reply_to":"3f79a3b5_643eab2a","updated":"2018-08-09 14:59:02.000000000","message":"I8b1c05317734e14ea73dc868941351bb31210bf0 is relatively new and does the volume detach on compute if the build is aborted or we don\u0027t reschedule. However, we don\u0027t have anything in conductor that detaches the volume if we reschedule and hit MaxRetriesExceeded (unlike how we detach/delete ports in conductor:\n\nhttps://github.com/openstack/nova/blob/93888df0da6adfa0c5f448692e5a480364dbe675/nova/conductor/manager.py#L613\n\n). What I\u0027d probably start with is a separate functional test that starts 2 computes, creates a server with a volume attached (not a root volume), then triggers reschedules so we eventually fail with MaxRetriesExceeded in conductor and assert all volumes attachments are cleaned up. We know we\u0027d have to fix bug 1784353 first though since we\u0027re currently always deleting the volume attachment before even rescheduling.","commit_id":"2ca8f8c9e2e32477336644b393b53d902677baaf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff5f51410a2e133122295d97638ccbe9b5475c9","unresolved":false,"context_lines":[{"line_number":988,"context_line":"            })]"},{"line_number":989,"context_line":"        bdms \u003d block_device_obj.block_device_make_list_from_dicts("},{"line_number":990,"context_line":"            self.context, bdms)"},{"line_number":991,"context_line":"        mock_attachment_get.side_effect \u003d \\"},{"line_number":992,"context_line":"            exc.VolumeAttachmentNotFound(attachment_id\u003duuids.attachment)"},{"line_number":993,"context_line":"        mock_attachment_create.return_value \u003d {\u0027id\u0027: uuids.new_attachment}"},{"line_number":994,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"3f79a3b5_c5fb3abc","line":991,"range":{"start_line":991,"start_character":42,"end_line":991,"end_character":43},"updated":"2018-10-16 15:57:11.000000000","message":"nit: unnecessary","commit_id":"3184ea9fbbfc6b7ca6e8ffc0dfb82845aa6873ec"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff5f51410a2e133122295d97638ccbe9b5475c9","unresolved":false,"context_lines":[{"line_number":1002,"context_line":"                                                       uuids.volume,"},{"line_number":1003,"context_line":"                                                       instance.uuid)"},{"line_number":1004,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1005,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1006,"context_line":""},{"line_number":1007,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_get\u0027)"},{"line_number":1008,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_create\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"3f79a3b5_45af2a9c","line":1005,"range":{"start_line":1005,"start_character":0,"end_line":1005,"end_character":69},"updated":"2018-10-16 15:57:11.000000000","message":"nit: It would be nice to have the following before the call to _validate_existing_attachment_ids:\n\n  self.assertEqual(bdms[0].attachment_id, uuids.attachment)\n\nmerely to reinforce what\u0027s going on here","commit_id":"3184ea9fbbfc6b7ca6e8ffc0dfb82845aa6873ec"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"bff5f51410a2e133122295d97638ccbe9b5475c9","unresolved":false,"context_lines":[{"line_number":1045,"context_line":"                                                    uuids.attachment)"},{"line_number":1046,"context_line":"        mock_attachment_create.assert_not_called()"},{"line_number":1047,"context_line":"        mock_bdm_save.assert_not_called()"},{"line_number":1048,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":1049,"context_line":""},{"line_number":1050,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1051,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":12,"id":"3f79a3b5_65b26607","line":1048,"range":{"start_line":1048,"start_character":0,"end_line":1048,"end_character":65},"updated":"2018-10-16 15:57:11.000000000","message":"ditto","commit_id":"3184ea9fbbfc6b7ca6e8ffc0dfb82845aa6873ec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":992,"context_line":"            attachment_id\u003duuids.attachment)"},{"line_number":993,"context_line":"        mock_attachment_create.return_value \u003d {\u0027id\u0027: uuids.new_attachment}"},{"line_number":994,"context_line":""},{"line_number":995,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":996,"context_line":"        self.conductor_manager._validate_existing_attachment_ids(self.context,"},{"line_number":997,"context_line":"                                                                 instance,"},{"line_number":998,"context_line":"                                                                 bdms)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_ea0ecc3b","line":995,"range":{"start_line":995,"start_character":25,"end_line":995,"end_character":64},"updated":"2018-10-22 19:00:11.000000000","message":"nit: swap the args (expected, actual)","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":992,"context_line":"            attachment_id\u003duuids.attachment)"},{"line_number":993,"context_line":"        mock_attachment_create.return_value \u003d {\u0027id\u0027: uuids.new_attachment}"},{"line_number":994,"context_line":""},{"line_number":995,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":996,"context_line":"        self.conductor_manager._validate_existing_attachment_ids(self.context,"},{"line_number":997,"context_line":"                                                                 instance,"},{"line_number":998,"context_line":"                                                                 bdms)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_dd7e381a","line":995,"range":{"start_line":995,"start_character":25,"end_line":995,"end_character":64},"in_reply_to":"3f79a3b5_ea0ecc3b","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":1002,"context_line":"                                                       uuids.volume,"},{"line_number":1003,"context_line":"                                                       instance.uuid)"},{"line_number":1004,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1005,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1006,"context_line":""},{"line_number":1007,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_get\u0027)"},{"line_number":1008,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_create\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_0a122862","line":1005,"range":{"start_line":1005,"start_character":25,"end_line":1005,"end_character":68},"updated":"2018-10-22 19:00:11.000000000","message":"same","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":1002,"context_line":"                                                       uuids.volume,"},{"line_number":1003,"context_line":"                                                       instance.uuid)"},{"line_number":1004,"context_line":"        mock_bdm_save.assert_called_once()"},{"line_number":1005,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.new_attachment)"},{"line_number":1006,"context_line":""},{"line_number":1007,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_get\u0027)"},{"line_number":1008,"context_line":"    @mock.patch.object(cinder.API, \u0027attachment_create\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_fd81b4fb","line":1005,"range":{"start_line":1005,"start_character":25,"end_line":1005,"end_character":68},"in_reply_to":"3f79a3b5_0a122862","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":1038,"context_line":"            \"id\": uuids.attachment"},{"line_number":1039,"context_line":"        }}"},{"line_number":1040,"context_line":""},{"line_number":1041,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":1042,"context_line":"        self.conductor_manager._validate_existing_attachment_ids(self.context,"},{"line_number":1043,"context_line":"                                                                 instance,"},{"line_number":1044,"context_line":"                                                                 bdms)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_6a02bc0e","line":1041,"range":{"start_line":1041,"start_character":25,"end_line":1041,"end_character":64},"updated":"2018-10-22 19:00:11.000000000","message":"same","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":1038,"context_line":"            \"id\": uuids.attachment"},{"line_number":1039,"context_line":"        }}"},{"line_number":1040,"context_line":""},{"line_number":1041,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":1042,"context_line":"        self.conductor_manager._validate_existing_attachment_ids(self.context,"},{"line_number":1043,"context_line":"                                                                 instance,"},{"line_number":1044,"context_line":"                                                                 bdms)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_9d78c02c","line":1041,"range":{"start_line":1041,"start_character":25,"end_line":1041,"end_character":64},"in_reply_to":"3f79a3b5_6a02bc0e","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"8c4ccf94b0ed8d064e3f13f8a28bb379be0070b7","unresolved":false,"context_lines":[{"line_number":1046,"context_line":"                                                    uuids.attachment)"},{"line_number":1047,"context_line":"        mock_attachment_create.assert_not_called()"},{"line_number":1048,"context_line":"        mock_bdm_save.assert_not_called()"},{"line_number":1049,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":1050,"context_line":""},{"line_number":1051,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1052,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_dd03980b","line":1049,"range":{"start_line":1049,"start_character":25,"end_line":1049,"end_character":64},"updated":"2018-10-22 19:00:11.000000000","message":"same","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ed084c8dff226c16eea20553b7b401bf781da46a","unresolved":false,"context_lines":[{"line_number":1046,"context_line":"                                                    uuids.attachment)"},{"line_number":1047,"context_line":"        mock_attachment_create.assert_not_called()"},{"line_number":1048,"context_line":"        mock_bdm_save.assert_not_called()"},{"line_number":1049,"context_line":"        self.assertEqual(bdms[0].attachment_id, uuids.attachment)"},{"line_number":1050,"context_line":""},{"line_number":1051,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027unshelve_instance\u0027)"},{"line_number":1052,"context_line":"    @mock.patch.object(compute_rpcapi.ComputeAPI, \u0027start_instance\u0027)"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f79a3b5_bd733c4f","line":1049,"range":{"start_line":1049,"start_character":25,"end_line":1049,"end_character":64},"in_reply_to":"3f79a3b5_dd03980b","updated":"2018-10-22 19:36:07.000000000","message":"Done","commit_id":"9b6ee119949c3a83a929a1a0a3f295fc9cccc8a4"}]}