)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":11,"context_line":"would previously use the status of the volume to determine if the volume"},{"line_number":12,"context_line":"was retyping or migrating."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"However in the migration case where a volume is moved directly between"},{"line_number":15,"context_line":"hosts the volume is never given a status  of migrating by Cinder leading"},{"line_number":16,"context_line":"to Nova never calling the os-migrate_volume_completion cinder API to"},{"line_number":17,"context_line":"complete the migration."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_7370b2dd","line":14,"updated":"2019-04-15 13:50:20.000000000","message":"Which is this proposed change, which may or may not get backported:\n\nhttps://review.openstack.org/#/c/638995/","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":12,"context_line":"was retyping or migrating."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"However in the migration case where a volume is moved directly between"},{"line_number":15,"context_line":"hosts the volume is never given a status  of migrating by Cinder leading"},{"line_number":16,"context_line":"to Nova never calling the os-migrate_volume_completion cinder API to"},{"line_number":17,"context_line":"complete the migration."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_335b0a55","line":15,"range":{"start_line":15,"start_character":45,"end_line":15,"end_character":54},"updated":"2019-04-15 13:50:20.000000000","message":"Yeah I guess \"migrating\" isn\u0027t in the status list here:\n\nhttps://developer.openstack.org/api-ref/block-storage/v3/#volumes-volumes","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":12,"context_line":"was retyping or migrating."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"However in the migration case where a volume is moved directly between"},{"line_number":15,"context_line":"hosts the volume is never given a status  of migrating by Cinder leading"},{"line_number":16,"context_line":"to Nova never calling the os-migrate_volume_completion cinder API to"},{"line_number":17,"context_line":"complete the migration."},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_b34ffa90","line":15,"range":{"start_line":15,"start_character":40,"end_line":15,"end_character":42},"updated":"2019-04-15 13:50:20.000000000","message":"s/  / /","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":16,"context_line":"to Nova never calling the os-migrate_volume_completion cinder API to"},{"line_number":17,"context_line":"complete the migration."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"This change switches Nova to use the migration_status of the volume to"},{"line_number":20,"context_line":"ensure that this API is called for both retypes and migrations."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Depends-On: https://review.openstack.org/#/c/639331/"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_733e72db","line":19,"range":{"start_line":19,"start_character":37,"end_line":19,"end_character":53},"updated":"2019-04-15 13:50:20.000000000","message":"Is this shown to non-admins? Seems kind of weird that it would, but maybe it doesn\u0027t expose anything and is more like a task_state in nova and should have been built into the overall volume status, which again is https://review.openstack.org/#/c/638995/.","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"b4d83d1763d9c65fb19f257e390183a69bb74290","unresolved":false,"context_lines":[{"line_number":19,"context_line":"This change switches Nova to use the migration_status of the volume to"},{"line_number":20,"context_line":"ensure that this API is called for both retypes and migrations."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Depends-On: https://review.openstack.org/#/c/639331/"},{"line_number":23,"context_line":"Change-Id: I1bdf3431bda2da98380e0dcaa9f952e6768ca3af"},{"line_number":24,"context_line":"Closes-bug: #1803961"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_ee99ff6d","line":22,"updated":"2019-04-16 16:26:38.000000000","message":"Test runs and passes:\n\nhttp://logs.openstack.org/24/637224/8/check/tempest-slow-py3/69f436a/job-output.txt.gz#_2019-04-09_16_00_30_246742\n\n2019-04-09 16:00:30.246742 | controller | {0} tempest.scenario.test_volume_migrate_attached.TestVolumeMigrateRetypeAttached.test_volume_migrate_attached [111.861291s] ... ok","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"43d264bb10df3b9a0151cf9a0dae30ee66058e76","unresolved":false,"context_lines":[{"line_number":19,"context_line":"This change switches Nova to use the migration_status of the volume to"},{"line_number":20,"context_line":"ensure that this API is called for both retypes and migrations."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Depends-On: https://review.openstack.org/#/c/639331/"},{"line_number":23,"context_line":"Change-Id: I1bdf3431bda2da98380e0dcaa9f952e6768ca3af"},{"line_number":24,"context_line":"Closes-bug: #1803961"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"3fce034c_91b41a37","line":22,"in_reply_to":"3fce034c_ee99ff6d","updated":"2019-04-16 16:57:36.000000000","message":"FWIW that change is only asserting that we haven\u0027t broken the existing retype with migration flow. I501eb0cd5eb101b4dc553c2cdbc414693dd5b681 is an additional tempest change on top of this that covers both retype and standalone volume migration. That change depends on this fix and is also passing:\n\nhttp://logs.openstack.org/27/637527/14/check/tempest-slow/6b02591/job-output.txt.gz#_2019-04-09_20_32_08_087845","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"}],"nova/compute/manager.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"49dc353cf85d6e90a11e2a966e9156dda54df03c","unresolved":false,"context_lines":[{"line_number":5853,"context_line":"        # attachments we could think about cleaning up the cinder-initiated"},{"line_number":5854,"context_line":"        # swap volume API flows."},{"line_number":5855,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5856,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5857,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5858,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5859,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5860,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":3,"id":"9fdfeff1_e8d23734","line":5857,"range":{"start_line":5856,"start_character":0,"end_line":5857,"end_character":79},"updated":"2019-02-25 12:39:07.000000000","message":"From looking at the cinder code I believe this is correct. There are 2 flows we\u0027re interested in: retype and migrate. We\u0027re only interested in retype if it does a migration, which will result in cinder calling VolumeManager.migrate_volume(). migrate always uses this function. migrate_volume() sets migration_status to \u0027migrating\u0027 before beginning, so I believe this test is correct.\n\nHowever, I\u0027d *really* like to see a +1 from a cinder developer on this first.","commit_id":"7fcbd6c40ec2ecf8416b2cd98790bfc80e82bd31"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":5967,"context_line":"        # attachments we could think about cleaning up the cinder-initiated"},{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_7e0cf93a","line":5970,"updated":"2019-04-15 13:50:20.000000000","message":"So this doesn\u0027t get set during a retype? Or it might, but doesn\u0027t matter if it\u0027s a retype but there is a migration? Or does a retype always set migration_status?\n\nThe block storage API reference docs aren\u0027t clear on when this field would be set.\n\nLooking at the cinder volume manager code, it looks like a retype will set migration_status if the volume has to be migrated for the new type:\n\nhttps://github.com/openstack/cinder/blob/931816722d43ed866c500a77eaa79b0ed4579bcd/cinder/volume/manager.py#L2888\n\nAnd migration status is always set for a non-retype migration:\n\nhttps://github.com/openstack/cinder/blob/931816722d43ed866c500a77eaa79b0ed4579bcd/cinder/volume/manager.py#L2459\n\nSo just based on that I guess this change achieves the same logic that the previous code was trying to get (even though \u0027status\u0027 could never be \u0027migrating\u0027).\n\nThe logic you\u0027re replacing was added way back in pike:\n\nhttps://review.openstack.org/#/c/456971/","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8de03f2d70c6567d68dadcbcb1a1945e5f87f6f6","unresolved":false,"context_lines":[{"line_number":5967,"context_line":"        # attachments we could think about cleaning up the cinder-initiated"},{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_36f68efb","line":5970,"in_reply_to":"3fce034c_7e0cf93a","updated":"2019-04-15 20:56:42.000000000","message":"Right, the pure migration case was never tested. FWIW if you haven\u0027t seen it yet I501eb0cd5eb101b4dc553c2cdbc414693dd5b681 introduces this and depends on this change.","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5974,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_be9ee1b5","line":5971,"range":{"start_line":5971,"start_character":65,"end_line":5971,"end_character":67},"updated":"2019-04-15 13:50:20.000000000","message":"I found this in Cinder:\n\nhttps://github.com/openstack/cinder/blob/931816722d43ed866c500a77eaa79b0ed4579bcd/cinder/volume/manager.py#L863\n\nSo will the migration_status sometimes be prefixed? If so, do we need to check for \u0027in\u0027 rather than \u0027\u003d\u003d\u0027 or \u0027endswith\u0027?","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c219ca01bd2c28cedb8f147d7df8cf74a780155e","unresolved":false,"context_lines":[{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5974,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_7ecf19b0","line":5971,"range":{"start_line":5971,"start_character":69,"end_line":5971,"end_character":78},"updated":"2019-04-15 13:50:20.000000000","message":"The other confusing thing about migration_status in cinder is it looks like it\u0027s maybe not cleared out after the migration is complete, so the value could be residual from the previous migration? That\u0027s very confusing if so. Like, could we make the wrong decision here because we first migrate a volume and then retype it without migrating, but because the migration_status is set to \u0027success\u0027 from the last migration we assume we need to complete the current operation?","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8de03f2d70c6567d68dadcbcb1a1945e5f87f6f6","unresolved":false,"context_lines":[{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5974,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_7650960b","line":5971,"range":{"start_line":5971,"start_character":69,"end_line":5971,"end_character":78},"in_reply_to":"3fce034c_7ecf19b0","updated":"2019-04-15 20:56:42.000000000","message":"No, the call to Nova from Cinder is pretty deep in the volume migration flow itself so we wouldn\u0027t end up here if the retype didn\u0027t result in a migration.\n\nI agree it\u0027s pretty weird that Cinder doesn\u0027t clear migration_status once the migration completes. The only case I can think of where that could catch us out is if an admin calls our swap volume API between a source volume that\u0027s already being migrated/retyped and a totally independent volume. That could result in the Nova calling back to os-migrate_volume_completion incorrectly and IMHO is another reason for us to break this API into an admin facing and service facing APIs (added to the etherpad for PTG).","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"909ac301e4eee67195ed091b5e8ee343b20f31bf","unresolved":false,"context_lines":[{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5974,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_79096993","line":5971,"range":{"start_line":5971,"start_character":65,"end_line":5971,"end_character":67},"in_reply_to":"3fce034c_b07925fd","updated":"2019-04-15 22:10:26.000000000","message":"\u003e That\u0027s set on the destination volume:\n \u003e \n \u003e https://github.com/openstack/cinder/blob/73329e16409b8fe37179e262537b27e1864a0ab5/cinder/volume/manager.py#L2185\n\nAh good call.\n\n \u003e \n \u003e We are specifically looking at the src volume that\u0027s always going\n \u003e to have a state of migrating when Nova is called:\n \u003e \n \u003e https://github.com/openstack/cinder/blob/73329e16409b8fe37179e262537b27e1864a0ab5/cinder/volume/manager.py#L2459\n\nYup.\n\n \u003e \n \u003e Thinking about this we could even ERROR out here if the\n \u003e migration_status is set and isn\u0027t `migrating` indicating an active\n \u003e migration being called by c-vol *or* `success` indicating a\n \u003e previous migration/retype.\n\nYeah, or at least log a warning or something - but that could be a follow up hardening change so you don\u0027t distract from the fix at hand.","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8de03f2d70c6567d68dadcbcb1a1945e5f87f6f6","unresolved":false,"context_lines":[{"line_number":5968,"context_line":"        # swap volume API flows."},{"line_number":5969,"context_line":"        is_cinder_migration \u003d False"},{"line_number":5970,"context_line":"        if \u0027migration_status\u0027 in old_volume:"},{"line_number":5971,"context_line":"            is_cinder_migration \u003d old_volume[\u0027migration_status\u0027] \u003d\u003d \u0027migrating\u0027"},{"line_number":5972,"context_line":"        old_vol_size \u003d old_volume[\u0027size\u0027]"},{"line_number":5973,"context_line":"        new_volume \u003d self.volume_api.get(context, new_volume_id)"},{"line_number":5974,"context_line":"        new_vol_size \u003d new_volume[\u0027size\u0027]"}],"source_content_type":"text/x-python","patch_set":8,"id":"3fce034c_b07925fd","line":5971,"range":{"start_line":5971,"start_character":65,"end_line":5971,"end_character":67},"in_reply_to":"3fce034c_be9ee1b5","updated":"2019-04-15 20:56:42.000000000","message":"That\u0027s set on the destination volume:\n\nhttps://github.com/openstack/cinder/blob/73329e16409b8fe37179e262537b27e1864a0ab5/cinder/volume/manager.py#L2185\n\nWe are specifically looking at the src volume that\u0027s always going to have a state of migrating when Nova is called:\n\nhttps://github.com/openstack/cinder/blob/73329e16409b8fe37179e262537b27e1864a0ab5/cinder/volume/manager.py#L2459\n\nThinking about this we could even ERROR out here if the migration_status is set and isn\u0027t `migrating` indicating an active migration being called by c-vol *or* `success` indicating a previous migration/retype.","commit_id":"53c3cfa7a02d684ce27800e22e00a816af44c510"}],"nova/tests/unit/compute/test_compute_mgr.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"49dc353cf85d6e90a11e2a966e9156dda54df03c","unresolved":false,"context_lines":[{"line_number":2461,"context_line":"            attachment_id\u003duuids.old_attachment_id,"},{"line_number":2462,"context_line":"            connection_info\u003d\u0027{\"data\": {}}\u0027)"},{"line_number":2463,"context_line":"        old_volume \u003d {"},{"line_number":2464,"context_line":"            \u0027id\u0027: uuids.old_volume_id, \u0027size\u0027: 1, \u0027status\u0027: \u0027migrating\u0027,"},{"line_number":2465,"context_line":"            \u0027migration_status\u0027: \u0027migrating\u0027, \u0027multiattach\u0027: False"},{"line_number":2466,"context_line":"        }"},{"line_number":2467,"context_line":"        new_volume \u003d {"}],"source_content_type":"text/x-python","patch_set":3,"id":"9fdfeff1_68f9677e","line":2464,"range":{"start_line":2464,"start_character":50,"end_line":2464,"end_character":71},"updated":"2019-02-25 12:39:07.000000000","message":"Looking at the cinder code I don\u0027t think this has ever been a thing. I believe that during a migration the status would be \u0027available\u0027, \u0027in-use\u0027, or \u0027maintenance\u0027. Assuming it\u0027s incorrect we should remove this, as we\u0027re testing invalid data.\n\nAgain, would be good to get confirmation from a cinder dev.","commit_id":"7fcbd6c40ec2ecf8416b2cd98790bfc80e82bd31"}]}