)]}'
{"cinder/backup/manager.py":[{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"6411355731c5ffecaf55b14a902103d941655ab7","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"\"\"\""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"from castellan import key_manager"},{"line_number":35,"context_line":"import os"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"from eventlet import tpool"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_15be96d4","line":34,"updated":"2018-01-30 17:42:05.000000000","message":"Please move this down to the third party import group below.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"697669c4893ecdaeb268ec43facfd8a681ef918d","unresolved":false,"context_lines":[{"line_number":31,"context_line":""},{"line_number":32,"context_line":"\"\"\""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"from castellan import key_manager"},{"line_number":35,"context_line":"import os"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":"from eventlet import tpool"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_933079ea","line":34,"in_reply_to":"5f93b717_15be96d4","updated":"2018-01-30 22:11:34.000000000","message":"Done","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"6411355731c5ffecaf55b14a902103d941655ab7","unresolved":false,"context_lines":[{"line_number":425,"context_line":""},{"line_number":426,"context_line":"    def _run_backup(self, context, backup, volume):"},{"line_number":427,"context_line":"        # Save a copy of the encryption key ID in case the volume is deleted."},{"line_number":428,"context_line":"        if volume.encryption_key_id is not None and \\"},{"line_number":429,"context_line":"           backup.encryption_key_id is None:"},{"line_number":430,"context_line":"            backup.encryption_key_id \u003d volume_utils.clone_encryption_key("},{"line_number":431,"context_line":"                context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_b593225b","line":428,"range":{"start_line":428,"start_character":52,"end_line":428,"end_character":53},"updated":"2018-01-30 17:42:05.000000000","message":"Please avoid using \\ to span multiple lines. It is preferred to wrap the conditionals in () to accomplish this.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"697669c4893ecdaeb268ec43facfd8a681ef918d","unresolved":false,"context_lines":[{"line_number":425,"context_line":""},{"line_number":426,"context_line":"    def _run_backup(self, context, backup, volume):"},{"line_number":427,"context_line":"        # Save a copy of the encryption key ID in case the volume is deleted."},{"line_number":428,"context_line":"        if volume.encryption_key_id is not None and \\"},{"line_number":429,"context_line":"           backup.encryption_key_id is None:"},{"line_number":430,"context_line":"            backup.encryption_key_id \u003d volume_utils.clone_encryption_key("},{"line_number":431,"context_line":"                context,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_334f256c","line":428,"range":{"start_line":428,"start_character":52,"end_line":428,"end_character":53},"in_reply_to":"5f93b717_b593225b","updated":"2018-01-30 22:11:34.000000000","message":"Done","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"}],"cinder/tests/unit/backup/drivers/test_backup_driver_base.py":[{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"98aeeb3aa14139aed90ec9624fe8bc9322503312","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    def _create_encrypted_volume_db_entry(self, id, type_id, encrypted):"},{"line_number":288,"context_line":"        if encrypted:"},{"line_number":289,"context_line":"            key_id \u003d str(uuid.uuid4())"},{"line_number":290,"context_line":"            vol \u003d {\u0027id\u0027: id, \u0027size\u0027: 1, \u0027status\u0027: \u0027available\u0027,"},{"line_number":291,"context_line":"                   \u0027volume_type_id\u0027: type_id, \u0027encryption_key_id\u0027: key_id}"},{"line_number":292,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_3050a432","line":289,"range":{"start_line":289,"start_character":25,"end_line":289,"end_character":37},"updated":"2018-01-29 15:51:10.000000000","message":"It\u0027s better to use fake_constants.ENCRYPTION_KEY_ID here, so we don\u0027t have different UUIDs in each unit test run. (It makes it harder to track failure patterns in test logs.)","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"8b582ff4f9621278798ef83a59047436006528d2","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    def _create_encrypted_volume_db_entry(self, id, type_id, encrypted):"},{"line_number":288,"context_line":"        if encrypted:"},{"line_number":289,"context_line":"            key_id \u003d str(uuid.uuid4())"},{"line_number":290,"context_line":"            vol \u003d {\u0027id\u0027: id, \u0027size\u0027: 1, \u0027status\u0027: \u0027available\u0027,"},{"line_number":291,"context_line":"                   \u0027volume_type_id\u0027: type_id, \u0027encryption_key_id\u0027: key_id}"},{"line_number":292,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_5ba8f175","line":289,"range":{"start_line":289,"start_character":25,"end_line":289,"end_character":37},"in_reply_to":"5f93b717_3050a432","updated":"2018-01-29 16:14:20.000000000","message":"Hmm. I don\u0027t think using a hard-coded value here is wise because some tests call this function more than once, which would lead to two DB entries with the same value. I tested the change and it does work (all tests pass), but it feels wrong.\n\nThis file calls uuid4() to create a lot IDs, which suggests a broader cleanup is in order. The changes would be somewhat extensive (uuid4() is called 17 times), so don\u0027t know if it should be bundled into this review.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"19f836aac746acdd2ef7b679821583bc1473d172","unresolved":false,"context_lines":[{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    def _create_encrypted_volume_db_entry(self, id, type_id, encrypted):"},{"line_number":288,"context_line":"        if encrypted:"},{"line_number":289,"context_line":"            key_id \u003d str(uuid.uuid4())"},{"line_number":290,"context_line":"            vol \u003d {\u0027id\u0027: id, \u0027size\u0027: 1, \u0027status\u0027: \u0027available\u0027,"},{"line_number":291,"context_line":"                   \u0027volume_type_id\u0027: type_id, \u0027encryption_key_id\u0027: key_id}"},{"line_number":292,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f93b717_dbbba16f","line":289,"range":{"start_line":289,"start_character":25,"end_line":289,"end_character":37},"in_reply_to":"5f93b717_5ba8f175","updated":"2018-01-29 16:25:07.000000000","message":"Yeah, it looks like this file does need a general cleanup for this -- maybe not worth worrying about here.\n\nWill look at other review details here.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"}],"releasenotes/notes/fix-backup-handling-of-encryption-key-id-f2fa56cadd80d582.yaml":[{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"6411355731c5ffecaf55b14a902103d941655ab7","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    is assigned a clone of the backup\u0027s encryption key ID. This ensures every"},{"line_number":11,"context_line":"    restored backup has a unique encryption key ID, even when multiple volumes"},{"line_number":12,"context_line":"    have been restored from the same backup."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":2,"id":"5f93b717_f5b44ad3","line":13,"updated":"2018-01-30 17:42:05.000000000","message":"I think we should probably have a note in the \"upgrades\" section stating that backups taken with this newer upgraded code when using encryption may not be restored to older versions of the backup service.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"9fd9c2528c3bec6a8a29b5148738271db7d3e393","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    is assigned a clone of the backup\u0027s encryption key ID. This ensures every"},{"line_number":11,"context_line":"    restored backup has a unique encryption key ID, even when multiple volumes"},{"line_number":12,"context_line":"    have been restored from the same backup."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":2,"id":"5f93b717_1d38f83a","line":13,"in_reply_to":"5f93b717_3dba94c0","updated":"2018-01-30 21:08:49.000000000","message":"Hmm, good point. Yeah, let\u0027s leave that for another day.","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"deeabfcd020f8b80e080f1d4e9ebb0311a720715","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    is assigned a clone of the backup\u0027s encryption key ID. This ensures every"},{"line_number":11,"context_line":"    restored backup has a unique encryption key ID, even when multiple volumes"},{"line_number":12,"context_line":"    have been restored from the same backup."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":2,"id":"5f93b717_3dba94c0","line":13,"in_reply_to":"5f93b717_90b39498","updated":"2018-01-30 21:06:14.000000000","message":"I investigated this more and reached a new viewpoint. First, I\u0027m told that down-revving services is not allowed, and so the only way an older backup service might encounter a newer restoration set would be a result of a backup export/import. Export a new backup and import into an old cloud. However, import/export of encrypted backups is broken in a fundamental way because there\u0027s no provision for migrating Barbican secrets between clouds. Even if the secrets are migrated, the encryption_key_id values would be different.\n\nWith this in mind, I\u0027d like to proceed with this patch as-is (after addressing other comments), and handle export/import problems separately. Would that be OK?","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"2c590fd27f262ba3c5098f893689ccf2b2b7db50","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    is assigned a clone of the backup\u0027s encryption key ID. This ensures every"},{"line_number":11,"context_line":"    restored backup has a unique encryption key ID, even when multiple volumes"},{"line_number":12,"context_line":"    have been restored from the same backup."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":2,"id":"5f93b717_90b39498","line":13,"in_reply_to":"5f93b717_b0f7b0d3","updated":"2018-01-30 18:35:12.000000000","message":"Oh, good call. Probably both?","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"be64204f1a9608e632104bf8c7f69f4421cf4c60","unresolved":false,"context_lines":[{"line_number":10,"context_line":"    is assigned a clone of the backup\u0027s encryption key ID. This ensures every"},{"line_number":11,"context_line":"    restored backup has a unique encryption key ID, even when multiple volumes"},{"line_number":12,"context_line":"    have been restored from the same backup."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":2,"id":"5f93b717_b0f7b0d3","line":13,"in_reply_to":"5f93b717_f5b44ad3","updated":"2018-01-30 18:13:56.000000000","message":"Hmm, you raise an interesting point. Do you think a note is sufficient, or should I bump the metadata version to block attempts to restore with an older version of the service? See L332:338 in backup/driver.py","commit_id":"db8254d15ebb8a2dafd68ad393186db5a9ece879"}]}