)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"30dd30e10b3239385edd7854ea9a1a2e84c4cf75","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Added filters while fetching backups from DB for incremental"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In create method of backup, while doing an incremental backup all"},{"line_number":10,"context_line":"existing backup entries gets fetched from DB to search for the one"},{"line_number":11,"context_line":"to use in incremental."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Now, only required one would be fetched after adding sort_keys\u003d\u0027created_at\u0027"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"1f1a1f67_3b76b671","line":10,"range":{"start_line":10,"start_character":24,"end_line":10,"end_character":28},"updated":"2017-07-19 13:49:44.000000000","message":"nit: get","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":19138,"name":"Pranali Deore","email":"pdeore@redhat.com","username":"PranaliD"},"change_message_id":"20861febe41c154f09faeb2ab266c6564d510c56","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Added filters while fetching backups from DB for incremental"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In create method of backup, while doing an incremental backup all"},{"line_number":10,"context_line":"existing backup entries gets fetched from DB to search for the one"},{"line_number":11,"context_line":"to use in incremental."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Now, only required one would be fetched after adding sort_keys\u003d\u0027created_at\u0027"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ff346bd7_d57d07e8","line":10,"range":{"start_line":10,"start_character":24,"end_line":10,"end_character":28},"in_reply_to":"1f1a1f67_3b76b671","updated":"2017-07-24 11:51:16.000000000","message":"Done","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5792f024873a6bdc34163d9f9264b15b1e8e81ac","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Optimize getting parent backup for new incremental"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When creating an incremental backup all existing backup entries where fetched"},{"line_number":10,"context_line":"from the DB to then search this potentially large list for the one to use as"},{"line_number":11,"context_line":"parent."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":16,"id":"214cfc56_c44cfe63","line":9,"range":{"start_line":9,"start_character":64,"end_line":9,"end_character":69},"updated":"2024-04-12 14:58:04.000000000","message":"were","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a0a1321ac82e4c33624687340a0b6be397777d75","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Optimize getting parent backup for new incremental"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When creating an incremental backup all existing backup entries where fetched"},{"line_number":10,"context_line":"from the DB to then search this potentially large list for the one to use as"},{"line_number":11,"context_line":"parent."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":16,"id":"e9fcde9b_d6a0adad","line":9,"range":{"start_line":9,"start_character":64,"end_line":9,"end_character":69},"in_reply_to":"214cfc56_c44cfe63","updated":"2026-04-07 14:19:50.000000000","message":"Done","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"64cca988739e2a7baabdb14c940b49463e227424","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":7,"id":"baffa285_ea6a10d8","updated":"2024-01-29 13:17:09.000000000","message":"I now looked at this one in a little more detail when rebasing\n... and there are multiple caveats and pitfalls:\n\n1) Only fetching the \"most recent\" (and maybe filtered here with \"status\u003dfields.BackupStatus.AVAILABLE\") backups for a volume would indeed be a great improvement as it moves the heavy lifting to the DB.\n\n2) But to also support incremental backups of volume snapshots (!) the response would be in correct / NOT sufficient. As Eric commented [1], the max function seems a little redundant at first, but it\u0027s not! If an incremental backup of a snapshot is taken, finding the parent is a little more complicated, than just using the last successful volume backup:\n\nThe list of backups needs to be searched for\n\n  * Case1: if it\u0027s not backing up a snapshot, simply the most recent (successful) backup. Easy, see 1).\n\n  * Case2: if it\u0027s incrementally backing up a snapshot, most recent backup that, is older than the snapshot itself (if it was younger, than changed blocks would be missing)\n\n\nI rebased the code and reworked the filters to determine the proper parent backup_id all within the DB query. I know and see the tests are still broken, but please kindly take a look and give me some feedback on this idea to clean and speed things up.\n\n\n\n\n[1] https://review.opendev.org/c/openstack/cinder/+/484729/comment/ffb9cba7_6d6526e1/","commit_id":"6048217d70342ca5c129535a355e7f0effe36e80"},{"author":{"_account_id":31779,"name":"Jean Pierre Roquesalane","display_name":"happystacker","email":"jeanpierre.roquesalane@dell.com","username":"happystacker"},"change_message_id":"b6c0670365a02a44c5b049f007239cbc60e8bf59","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"356d369b_1a2c0af4","updated":"2024-01-26 11:14:19.000000000","message":"Please resolve conflict and resubmit a new patchset","commit_id":"6048217d70342ca5c129535a355e7f0effe36e80"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"0ce73476d895f7735d7fc062d6d8fefae400d8e7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"fe685dda_7fff7abf","in_reply_to":"baffa285_ea6a10d8","updated":"2026-02-16 11:28:44.000000000","message":"Done","commit_id":"6048217d70342ca5c129535a355e7f0effe36e80"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"0c7f516925a5a6591199b19d4cfec839b7a4f356","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"b0db7205_bb379a2f","updated":"2024-02-07 15:07:16.000000000","message":"I am looking for guidance on how to best add the DB query to find the parent backup and then \"reach\" it or call within the backup.\n\n\nMy current approach is to add \"backup_get_parent_for_incremental\" to db (SQLAlchemy) to do the actual query.\n\n\n\nNow where I\u0027d like some input:\n\n\n1) Should I directly call the backup_get_parent_for_incremental from the \"db\" (and then convert it into a backup object cinder.backup.api.create()? \n\n2) Should I add a method to objects.BackupList or objects.Backup to abstract the DB access away and get a Backup object (which is required as \"parent\" parameter for the to be created backup anyways?","commit_id":"56ebbfb5fabde2e330f11d16770efd2b6ed5aea7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"054fcd1e4d349fa25f716f1a265b86a4afd52e47","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"708a97d4_2d107e2a","updated":"2024-02-07 14:39:28.000000000","message":"It\u0027s unclear how openstack-tox-pep8 passes on this patch -- \"tox -e pep8\" fails with a dozen errors when run locally.","commit_id":"56ebbfb5fabde2e330f11d16770efd2b6ed5aea7"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"3c53e7a5b19af3ecee507a49d85e1ddab903344b","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"5e59972d_d7b236db","in_reply_to":"b0db7205_bb379a2f","updated":"2024-02-07 17:00:24.000000000","message":"I pushed a somewhat complete version now with 2) implemented and (hopefully) no more pep8 or failing tests.\n\nPlease let me know what you think.","commit_id":"56ebbfb5fabde2e330f11d16770efd2b6ed5aea7"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"35e53c2f6a4d11c29df487b78c294615e43e9e2e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"10823397_630952b6","updated":"2024-02-08 08:56:17.000000000","message":"@Michal I added you since you seems to be a heavy user of incremental backups as you replied to the change about keeping only a few snapshots (https://review.opendev.org/c/openstack/cinder/+/810457/comments/19fa14ba_e49102ad).\n\nMaybe you find the time to look into this change here as well?","commit_id":"f7d39a1f495b36d83c7d4b8bddb6bfba569ab5f4"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"cb75610086c36c949e9cefa8c8cabc2827e8fbf9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"f51ee4b6_090a921f","updated":"2024-02-13 06:13:40.000000000","message":"Love it.\n\nI wish I could create my own method all_non_avail() or something. But that one only runs at startup, but this one runs all the time.\n\nThe way comparison with timestamp is programmed down to SQL looks good to me.","commit_id":"440bade7a3912a62d89173f35d4098cba3bc7f43"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"b86114c4544fb2104ca6ae562cac1b5e445d6979","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"12cb3f60_b8aacf5f","updated":"2024-02-13 09:04:41.000000000","message":"recheck","commit_id":"440bade7a3912a62d89173f35d4098cba3bc7f43"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"ae0e127a8477a5489dda54af335bbc754ea22bb3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"73593648_a53e338c","updated":"2024-02-14 13:46:27.000000000","message":"There were some functional tests failing. I found that I did not consider the fix from https://review.opendev.org/c/openstack/cinder/+/720833, which I how (hopefully) did with the latest patchset.\n\nPTAL.","commit_id":"b501817376f000b814d5d4dea38b47d48ea18fba"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"6d662b0840cda9c4a2856ba2010687b32a4e7d3e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"0abefa2d_40cda333","updated":"2024-03-04 12:15:05.000000000","message":"Could you kindly take another look at this one?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"33f014faa01c9a4790736c55aa0cbab9077889f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"ecb1f16a_ad6fee0a","updated":"2024-02-19 15:14:55.000000000","message":"I had to do one more fix as \u0027project_only\u003dTrue\u0027 for _backups_get_query still let\u0027s the admin see it all :-)\n\n\nPlease kindly take another look at this one.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7311e2ddfb75c3da02744c558d70ee976c114b52","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"2a5235ca_0d2a58bc","updated":"2024-04-09 07:28:04.000000000","message":"May I kindly ask for your review again?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"6f3439db4b3e9e7ac9e3dd152fc59ea004b0b0f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"da12c600_428fc582","updated":"2024-03-20 13:56:23.000000000","message":"Pete, could you kindly check this one out again?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"1aecd6bd6100065be3fea37fac5052eeff0f525c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"ea549478_c0864cd7","updated":"2024-03-04 16:08:27.000000000","message":"Peter please see me inline comments and kindly tell me if I got anything wrong here.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"d8399b3d2296a4d6acef7656ac5cdae9589ca9e4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":16,"id":"691ca168_0da083f6","updated":"2024-03-04 15:03:23.000000000","message":"See inline, but basically please explain how this came about.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"5792f024873a6bdc34163d9f9264b15b1e8e81ac","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":17,"id":"388eb730_705dec11","updated":"2024-04-12 14:58:04.000000000","message":"requires UT for sqlalchemy method","commit_id":"3d9b6bf0846531e190c0f4f8758cdf62686e27db"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a59530b986328cd86eb633d57536c7a5fc08f09f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":18,"id":"1a9459c4_779fa9a6","updated":"2024-04-12 15:00:39.000000000","message":"-1: For the missing UTs, here are some samples of how they look like: cinder/tests/unit/test_db_api.py","commit_id":"ff403a1b9119dce47bf4dfcfa6882be2d7d8db80"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"9dfdd4fd568d9d22f7faa7dcf4ddcdf8294aabfe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"7aaf2410_0264051e","updated":"2024-04-16 08:20:55.000000000","message":"As discussed at the PTG on Friday, I pushed a new Patchset including.UTs. PTAL\n\nI also fixed \n\"cinder.tests.unit.api.contrib.test_backups.BackupsAPITestCase.test_create_backup_delta_2_True\".\nThat should actually also have failed with the old code (wrong order to create the full backup and the snapshot, resulting in the data_timestamp of the full to be too recent new to base an incremental snapshot backup on.","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"4d9fd9d57fb6c1324c50dda1392db6614600c9e4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"33b3d39a_18a742ed","updated":"2024-05-17 15:37:11.000000000","message":"Regarding the failure of openstacksdk-functional-devstack. I\u0027m pretty sure Christian narrowed it down correctly to the timestamps. However there may be a mismatch between what the large block comment by xyang says and what the code does. The existing code only sorts the backups and finds the least worst one with max(). Therefore, a backup that fails the timestamp checks still gets selected in the end.\n\nThis behavior needs to be preserved. What the test is doing should continue to work. We constructed even more absurd scenarios during the upstream patch review meeting today, which should still work.\n\nThe consensus suggestion, then, is to run the optimized query as proposed in this patch. But if that fails, run a wider one, without the timestamps in WHERE statement (in filters), and pick up least worst backup from the result, to be the parent of the incremental.","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7a121c3b4050d613894b1b2caa5914d96bf354b1","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":19,"id":"b6da8f16_5c9481e3","updated":"2024-04-18 13:55:29.000000000","message":"There currently is test \"openstack.tests.functional.cloud.test_project_cleanup.TestProjectCleanup.test_block_storage_cleanup\" failing, see https://zuul.opendev.org/t/openstack/build/4f9ea8e583fa4b8495298367091f9f71.\n\nIf you look at what happens at https://github.com/openstack/openstacksdk/blob/8c6a129b8cea97ae8534c921576cac4b347a6194/openstack/tests/functional/cloud/test_project_cleanup.py#L132 onwards you see that:\n\n1. A volume is created\n2. A snapshot of that volume is created\n3. A full backup of the volume is created\n4. A incremental backup of the snapshot (created in step 2) is created, which fails.\n\n\nComparing the former and this new implementation I believe the code was buggy:\n\nOne cannot create an incremental snapshot backup, if there is no full backup (of the volume OR the snapshot) that has a \u0027data_timestamp\u0027 before the one of the snapshot.\n\n\nCould anybody kindly try and understand the possible cases here so I know if\n\na) My code is borken or where\nb) OpenstackSDK needs to create some different resources to \"project cleanup\"","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"8abddc1bbdda6a980aae045a6819e287ec271bff","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"79cbe663_1a605a0d","updated":"2024-04-19 15:38:47.000000000","message":"This looks not worse than before to me. I also found the part that I was missing about the context\u0027s project_id.","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"da1123dc2a2a7ee863414f345e46d06b213d3475","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":19,"id":"b7020ba1_14700c21","updated":"2024-04-29 06:05:08.000000000","message":"Tobias, since you worked on this part of the code as well and seems to be a cinder-backup user, do you mind diving into my question at:\n\nhttps://review.opendev.org/c/openstack/cinder/+/484729/comments/b6da8f16_5c9481e3\n\n\nI\u0027d really love for this to be resolved quickly so we can get this change here merged.","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"dff23c38ac0573b2a345e3fd5be34073ffb8597b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"f10edbf8_cda01d0a","updated":"2024-04-24 11:50:39.000000000","message":"gah wish i saw this before refactoring the same code in my series https://review.opendev.org/c/openstack/cinder/+/916683 – either way I\u0027ll change that to depend on this one","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"1570f9fc55f7a52590765d92b3ed115b5df1c5ed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"04a6a0d1_048d2915","updated":"2024-04-16 11:41:40.000000000","message":"recheck tempest-integrated-storage openstacksdk-functional-devstack","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"6eda3226813ada3550e8c1c1e32cde5759752488","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":19,"id":"e92b8e28_d3ec11cf","updated":"2024-04-17 05:06:13.000000000","message":"recheck tempest-integrated-storage openstacksdk-functional-devstack temptest-slow-py3","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"cd048050524fab36161160b5d1e9e8ec76ce91ab","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":19,"id":"95096300_6702df65","in_reply_to":"b6da8f16_5c9481e3","updated":"2024-05-17 14:48:25.000000000","message":"It may not seem logical to do that operation, but the code worked just fine and the restored data would be consistent with what was backed up.\n\nThe only thing that would happen is that you would be using more data.\n\nSo if that specific operation stops working then it would be a regression (because something that used to work now doesn\u0027t) even if it was a \"weird feature/behavior\".","commit_id":"f34bcabd4e6e6155e06e3d6e357263269fed6fbc"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"6fb827bd12ce33662bf65928e7e999c8cea72ddc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"fee2c923_908c0b81","updated":"2025-07-11 09:37:45.000000000","message":"Could this have its final review and merge?","commit_id":"12afc25347fdbb5358e1d764cf5a9c8f14d0dca8"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"b72903f44a48cbde19f2ece6e34ffd62c658f255","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":23,"id":"c8020077_7db4a868","updated":"2025-07-16 18:13:49.000000000","message":"Need to check why Zuul is failing due to missing pytz module","commit_id":"12afc25347fdbb5358e1d764cf5a9c8f14d0dca8"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"9c2470f7fdb140c5b5ef62e30f47970b8c702ff3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"28d0fb82_516ed537","updated":"2025-08-21 07:31:42.000000000","message":"Is there anything more I can do to help getting this across the finish line?","commit_id":"b9c991aca2f37326335ea1be8e39d0b8e806c81c"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"7c29824da347dda9d6881d0e3d9024c7556051bd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"eef48a41_bc542403","updated":"2025-08-07 18:56:42.000000000","message":"Patch rebased on latest master and only pytz reference updated to builtin zoneinfo.","commit_id":"b9c991aca2f37326335ea1be8e39d0b8e806c81c"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"9cb9259e0ea8be2673d1b1d4375566b00d2cc9f9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"6c7b3e6f_d3a4e50e","updated":"2025-08-07 18:55:47.000000000","message":"pytz was deprecated in favor of builtin zoneinfo, see https://review.opendev.org/c/openstack/requirements/+/875854 for more information.","commit_id":"b9c991aca2f37326335ea1be8e39d0b8e806c81c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d104d4ea313715cc4a18e031368b1cd5cb883156","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":24,"id":"e639a96d_3469482c","updated":"2025-08-08 07:42:00.000000000","message":"recheck cinder-plugin-ceph-tempest","commit_id":"b9c991aca2f37326335ea1be8e39d0b8e806c81c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"23b973da49d12ebc3a1e5157555fe73f38c6833a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":26,"id":"6966e4b0_e534c657","updated":"2026-02-16 21:00:22.000000000","message":"recheck","commit_id":"ed665250e60d8ca870317e2181c2a26617564414"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"4325d676421f2010ad111c82b8ed4c365d19322b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"20baaf24_809593d1","updated":"2026-02-20 15:14:58.000000000","message":"I continue to think this patch is valuable. LGTM.","commit_id":"48ee92caf36874dc4f9c414ce824e62158026769"},{"author":{"_account_id":35075,"name":"Alexander Deiter","email":"adeiter@infinidat.com","username":"adeiter"},"change_message_id":"2d3d1960fde02a83add62e89033b78292d43a53a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":27,"id":"b76df9a9_77193085","updated":"2026-02-20 15:40:30.000000000","message":"Looks good to me - thank you!","commit_id":"48ee92caf36874dc4f9c414ce824e62158026769"},{"author":{"_account_id":36171,"name":"jayaanand borra","display_name":"jayaanand borra","email":"jayaanand.borra@netapp.com","username":"jayaanan","status":"netapp"},"change_message_id":"3ad95e0ffff9ee10b345d119132cb845ec8f7cbc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"b88162c8_15081af9","updated":"2026-04-07 08:41:30.000000000","message":"Also, now backup scope is increased to RESTORING.I agree with other comments.","commit_id":"29594932bee2ef312d9151770bc615221ca41196"},{"author":{"_account_id":36171,"name":"jayaanand borra","display_name":"jayaanand borra","email":"jayaanand.borra@netapp.com","username":"jayaanan","status":"netapp"},"change_message_id":"aade4af9eb6e49742f7924c7e77044c0316aeaf8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"d3ae5c38_7983db36","updated":"2026-04-07 08:40:04.000000000","message":"from __future__ import annotations changes all annotations in the file to be lazily evaluated strings, which can break code that inspects annotations at runtime?","commit_id":"29594932bee2ef312d9151770bc615221ca41196"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"cedec1880ee65bf927776e9bb504c0824b964674","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"b617793e_bdeb2bec","updated":"2026-04-02 06:57:22.000000000","message":"recheck openstacksdk-functional-devstack","commit_id":"29594932bee2ef312d9151770bc615221ca41196"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a0a1321ac82e4c33624687340a0b6be397777d75","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":29,"id":"64e637b8_358a3ee4","in_reply_to":"b88162c8_15081af9","updated":"2026-04-07 14:19:50.000000000","message":"Yes. A backup in RESTORING state is absolutely still a valid (read: \"THE\") valid parent. Not including this status would cause the parent of new backup to move one up which is still working, but not really correct.","commit_id":"29594932bee2ef312d9151770bc615221ca41196"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7f1c4ac8f96bf73d44dad97df01ca29fe57da850","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":30,"id":"206ce926_5f0a3fbf","updated":"2026-04-28 14:48:51.000000000","message":"I believe the failing CI for openstacksdk might actually a bug.\nKindly have a look at https://bugs.launchpad.net/openstacksdk/+bug/2148715","commit_id":"959af97e1ca40126c7bb7b64c3c79b872f9cb3f1"}],"cinder/backup/api.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"30dd30e10b3239385edd7854ea9a1a2e84c4cf75","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        # incremental backup."},{"line_number":250,"context_line":"        latest_backup \u003d None"},{"line_number":251,"context_line":"        if incremental:"},{"line_number":252,"context_line":"            backups \u003d \\"},{"line_number":253,"context_line":"                objects.BackupList.get_all_by_volume(context.elevated(),"},{"line_number":254,"context_line":"                                                     volume_id, limit\u003d1,"},{"line_number":255,"context_line":"                                                     sort_keys\u003d[\u0027created_at\u0027],"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_1b1f7a08","line":252,"range":{"start_line":252,"start_character":22,"end_line":252,"end_character":23},"updated":"2017-07-19 13:49:44.000000000","message":"-1: we should be using a different way to split the line or parenthesis, \\ is not recommended\n\n            backups \u003d objects.BackupList.get_all_by_volume(\n                context.elevated(),\n                volume_id, limit\u003d1,\n                sort_keys\u003d[\u0027created_at\u0027],\n                sort_dirs\u003d[\u0027desc\u0027])\n\nor\n\n            backups \u003d (\n                objects.BackupList.get_all_by_volume(context.elevated(),\n                                                     volume_id, limit\u003d1,\n                                                     sort_keys\u003d[\u0027created_at\u0027],\n                                                     sort_dirs\u003d[\u0027desc\u0027]))","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":19138,"name":"Pranali Deore","email":"pdeore@redhat.com","username":"PranaliD"},"change_message_id":"20861febe41c154f09faeb2ab266c6564d510c56","unresolved":false,"context_lines":[{"line_number":249,"context_line":"        # incremental backup."},{"line_number":250,"context_line":"        latest_backup \u003d None"},{"line_number":251,"context_line":"        if incremental:"},{"line_number":252,"context_line":"            backups \u003d \\"},{"line_number":253,"context_line":"                objects.BackupList.get_all_by_volume(context.elevated(),"},{"line_number":254,"context_line":"                                                     volume_id, limit\u003d1,"},{"line_number":255,"context_line":"                                                     sort_keys\u003d[\u0027created_at\u0027],"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_756c1b36","line":252,"range":{"start_line":252,"start_character":22,"end_line":252,"end_character":23},"in_reply_to":"1f1a1f67_1b1f7a08","updated":"2017-07-24 11:51:16.000000000","message":"Done","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"30dd30e10b3239385edd7854ea9a1a2e84c4cf75","unresolved":false,"context_lines":[{"line_number":253,"context_line":"                objects.BackupList.get_all_by_volume(context.elevated(),"},{"line_number":254,"context_line":"                                                     volume_id, limit\u003d1,"},{"line_number":255,"context_line":"                                                     sort_keys\u003d[\u0027created_at\u0027],"},{"line_number":256,"context_line":"                                                     sort_dirs\u003d[\u0027desc\u0027])"},{"line_number":257,"context_line":"            if backups.objects:"},{"line_number":258,"context_line":"                # NOTE(xyang): The \u0027data_timestamp\u0027 field records the time"},{"line_number":259,"context_line":"                # when the data on the volume was first saved. If it is"}],"source_content_type":"text/x-python","patch_set":2,"id":"1f1a1f67_db70e242","line":256,"updated":"2017-07-19 13:49:44.000000000","message":"-1: Instead of adding new optional parameters to the method we should be using `get_all` method instead since we can filter by the volume_id there and already supports the parameters we want.","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":19138,"name":"Pranali Deore","email":"pdeore@redhat.com","username":"PranaliD"},"change_message_id":"20861febe41c154f09faeb2ab266c6564d510c56","unresolved":false,"context_lines":[{"line_number":253,"context_line":"                objects.BackupList.get_all_by_volume(context.elevated(),"},{"line_number":254,"context_line":"                                                     volume_id, limit\u003d1,"},{"line_number":255,"context_line":"                                                     sort_keys\u003d[\u0027created_at\u0027],"},{"line_number":256,"context_line":"                                                     sort_dirs\u003d[\u0027desc\u0027])"},{"line_number":257,"context_line":"            if backups.objects:"},{"line_number":258,"context_line":"                # NOTE(xyang): The \u0027data_timestamp\u0027 field records the time"},{"line_number":259,"context_line":"                # when the data on the volume was first saved. If it is"}],"source_content_type":"text/x-python","patch_set":2,"id":"ff346bd7_55691723","line":256,"in_reply_to":"1f1a1f67_db70e242","updated":"2017-07-24 11:51:16.000000000","message":"Done","commit_id":"6e110722ac4c60ee8fe8041f5e399c2a8c5df2d5"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"87f9b24227d44d470817e872735e9cb70bd03321","unresolved":false,"context_lines":[{"line_number":268,"context_line":"                # When taking an incremental backup of the snapshot, the"},{"line_number":269,"context_line":"                # parent should be the backup at 8:00, not 8:20, and the"},{"line_number":270,"context_line":"                # \u0027data_timestamp\u0027 of this new backup will be 8:10."},{"line_number":271,"context_line":"                latest_backup \u003d max("},{"line_number":272,"context_line":"                    backups.objects,"},{"line_number":273,"context_line":"                    key\u003dlambda x: x[\u0027data_timestamp\u0027]"},{"line_number":274,"context_line":"                    if (not snapshot or (snapshot and x[\u0027data_timestamp\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"ffb9cba7_6d6526e1","line":271,"range":{"start_line":271,"start_character":32,"end_line":271,"end_character":36},"updated":"2019-04-24 14:22:07.000000000","message":"If the backups query now only returns one item, this should be re-worked, right?\n\n(I think this max logic is being moved into the object/db query above.)","commit_id":"6048217d70342ca5c129535a355e7f0effe36e80"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"4b43ff0efb1f641a046a99a8181a9a3fd6d0b8c4","unresolved":false,"context_lines":[{"line_number":268,"context_line":"                # When taking an incremental backup of the snapshot, the"},{"line_number":269,"context_line":"                # parent should be the backup at 8:00, not 8:20, and the"},{"line_number":270,"context_line":"                # \u0027data_timestamp\u0027 of this new backup will be 8:10."},{"line_number":271,"context_line":"                latest_backup \u003d max("},{"line_number":272,"context_line":"                    backups.objects,"},{"line_number":273,"context_line":"                    key\u003dlambda x: x[\u0027data_timestamp\u0027]"},{"line_number":274,"context_line":"                    if (not snapshot or (snapshot and x[\u0027data_timestamp\u0027]"}],"source_content_type":"text/x-python","patch_set":7,"id":"ffb9cba7_ad81de4c","line":271,"range":{"start_line":271,"start_character":32,"end_line":271,"end_character":36},"in_reply_to":"ffb9cba7_6d6526e1","updated":"2019-04-24 14:23:22.000000000","message":"Also, doesn\u0027t there need to be filtering to ensure we pick a successful backup here?  (In both the old and new code.)","commit_id":"6048217d70342ca5c129535a355e7f0effe36e80"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"054fcd1e4d349fa25f716f1a265b86a4afd52e47","unresolved":true,"context_lines":[{"line_number":43,"context_line":"from cinder import quota_utils"},{"line_number":44,"context_line":"from cinder.scheduler import rpcapi as scheduler_rpcapi"},{"line_number":45,"context_line":"import cinder.volume"},{"line_number":46,"context_line":"import datetime"},{"line_number":47,"context_line":"import traceback"},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"backup_opts \u003d ["}],"source_content_type":"text/x-python","patch_set":11,"id":"da863ed2_25c848c5","line":46,"updated":"2024-02-07 14:39:28.000000000","message":"This is strange -- this import should generate an I100 error for being in the wrong group, but the pep8 check job passes here.","commit_id":"56ebbfb5fabde2e330f11d16770efd2b6ed5aea7"},{"author":{"_account_id":13915,"name":"Silvan Kaiser","email":"silvan@quobyte.com","username":"kaisers"},"change_message_id":"34cf64d1a9c873f22e2f7d55b571a43533aa4977","unresolved":true,"context_lines":[{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."}],"source_content_type":"text/x-python","patch_set":25,"id":"cae7d177_06cefe2e","line":311,"range":{"start_line":311,"start_character":18,"end_line":311,"end_character":19},"updated":"2026-02-06 15:21:08.000000000","message":"Not sure where this parenthesis should close?","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"0ce73476d895f7735d7fc062d6d8fefae400d8e7","unresolved":false,"context_lines":[{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."}],"source_content_type":"text/x-python","patch_set":25,"id":"a34847db_b7ea80e0","line":311,"range":{"start_line":311,"start_character":18,"end_line":311,"end_character":19},"in_reply_to":"cae7d177_06cefe2e","updated":"2026-02-16 11:28:44.000000000","message":"Reworded with the next patchset.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":10058,"name":"Erlon R. Cruz","email":"erlon.rodrigues.cruz@canonical.com","username":"sombrafam"},"change_message_id":"acb21f427f107837c4a4fde4a06e046a1c324d45","unresolved":true,"context_lines":[{"line_number":306,"context_line":"            )"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."},{"line_number":315,"context_line":"                # This should be removed in further patches and replaced with"},{"line_number":316,"context_line":"                # an api error"},{"line_number":317,"context_line":"                LOG.warning(\u0027Switching to unoptimized behaviour to find \u0027"},{"line_number":318,"context_line":"                            \u0027possible parent backup\u0027)"},{"line_number":319,"context_line":"                backups \u003d objects.BackupList.get_all_by_volume("}],"source_content_type":"text/x-python","patch_set":25,"id":"64c36e4d_29d49c52","line":316,"range":{"start_line":309,"start_character":0,"end_line":316,"end_character":30},"updated":"2026-02-06 15:11:12.000000000","message":"Can you better explain this? I\u0027m failing to understand why it would needed to query the database twice (in this case, we made the performance even worse), since the data should be available in the database at the time the first query was run. If that is failing to find it, it seems to me that the query should be fixed in the first place.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"0ce73476d895f7735d7fc062d6d8fefae400d8e7","unresolved":true,"context_lines":[{"line_number":306,"context_line":"            )"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."},{"line_number":315,"context_line":"                # This should be removed in further patches and replaced with"},{"line_number":316,"context_line":"                # an api error"},{"line_number":317,"context_line":"                LOG.warning(\u0027Switching to unoptimized behaviour to find \u0027"},{"line_number":318,"context_line":"                            \u0027possible parent backup\u0027)"},{"line_number":319,"context_line":"                backups \u003d objects.BackupList.get_all_by_volume("}],"source_content_type":"text/x-python","patch_set":25,"id":"ef3b264e_22561a49","line":316,"range":{"start_line":309,"start_character":0,"end_line":316,"end_character":30},"in_reply_to":"64c36e4d_29d49c52","updated":"2026-02-16 11:28:44.000000000","message":"First of all thanks Erlon for reviewing this patch! I must admit that after two YEARS I did not expect any movement on this one anymore. But let me be blunt, this is also a high risk of not getting a hold of a contributor anymore, at the very least it takes a while for the reviewer to dive back into his own code ;-)\n\nTo answer your question please kindly see the conversation at \nhttps://review.opendev.org/c/openstack/cinder/+/484729/comments/b6da8f16_5c9481e3\n\nIn essence ... this does NOT query the database twice, but actually uses the new DB-query method to determine a parent. If, and only IF, no parent is found, the old code is used to attempt to find a parent. Sole to maintain the old behavior.\n\nThis was also discussed at the PTG in Apr 2024 (https://etherpad.opendev.org/p/dalmatian-ptg-cinder#L485) I believe.\n\nSince to me the alternative path only manifests use-cases which should not actually work looking at the volume and backups lifecycle, I would have not issue to remove this alternative and throw an error.\n\nBut, back then it was about allowing the massive speed improvement while not changing the behavior at all.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":10058,"name":"Erlon R. Cruz","email":"erlon.rodrigues.cruz@canonical.com","username":"sombrafam"},"change_message_id":"da25b066bf5cc2cae92e07f18c0ead7183ea7960","unresolved":true,"context_lines":[{"line_number":306,"context_line":"            )"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."},{"line_number":315,"context_line":"                # This should be removed in further patches and replaced with"},{"line_number":316,"context_line":"                # an api error"},{"line_number":317,"context_line":"                LOG.warning(\u0027Switching to unoptimized behaviour to find \u0027"},{"line_number":318,"context_line":"                            \u0027possible parent backup\u0027)"},{"line_number":319,"context_line":"                backups \u003d objects.BackupList.get_all_by_volume("}],"source_content_type":"text/x-python","patch_set":25,"id":"6046c80d_ba1fc90b","line":316,"range":{"start_line":309,"start_character":0,"end_line":316,"end_character":30},"in_reply_to":"8a54eacd_5a101e87","updated":"2026-03-26 13:33:32.000000000","message":"Hey Nilkas, so, doing another pass on this with more attention. The new code added on backup_get_parent_for_incremental() already does what the old code do in all practical scenarios I can think of. If no timestap is passed, it will not use the filter, select all entries, sort and return the first. **It always find a suitable backup**.\n\nThe only case where the DB query returns None is when no AVAILABLE backup exists for that volume/project/timestamp combination. In that exact same scenario, max() returns a non-AVAILABLE backup, and the fallback code then sets parent_backup \u003d None anyway. So the fallback doesn\u0027t rescue anything — it just takes a slower path to the same None.\n\nThis means the fallback is dead code by construction, not just in theory. Keeping it doesn\u0027t preserve any behavior — it only adds confusion, a misleading log warning, and an unnecessary second DB round-trip.\n\nThe old code is confusing and that\u0027s what\u0027s making us to agonize on whether to keep it or not, so we should not defer the action recommended even by the author, \"TODO Should be removed in a further patch and cause api error\", and remove that code in this change.\n\nAddtionally to that, can you please also change the beforeDataTimestamp camel casing, to before_data_timestamp to comply with the coding standard?\n\nps. a way to put a stone on the matter, would be to change the test_create_backup_delta test, creating the edge scenarios you are visualizing, and making asserts showing that the code, can pass in both branches, new and legacy. With that I would be happy to let the code as it is.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"514812223f3a714b9fba8e8da20301372fd0d539","unresolved":true,"context_lines":[{"line_number":306,"context_line":"            )"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."},{"line_number":315,"context_line":"                # This should be removed in further patches and replaced with"},{"line_number":316,"context_line":"                # an api error"},{"line_number":317,"context_line":"                LOG.warning(\u0027Switching to unoptimized behaviour to find \u0027"},{"line_number":318,"context_line":"                            \u0027possible parent backup\u0027)"},{"line_number":319,"context_line":"                backups \u003d objects.BackupList.get_all_by_volume("}],"source_content_type":"text/x-python","patch_set":25,"id":"8a54eacd_5a101e87","line":316,"range":{"start_line":309,"start_character":0,"end_line":316,"end_character":30},"in_reply_to":"cfc2dcef_2778b96f","updated":"2026-02-20 16:32:15.000000000","message":"The new code uses an effective database query to filter for the latest AVAILABLE backup based on a timestamp which is the correct way. It finds the latest backup and uses this for an incremental backup.\n\nThe old code just finds the latest AVAILABLE backup before the timestamp 1.1.1, so it should always find a suitable backup even if it is not the latest. This will result in bigger incremental backup. This situation can happen if, e.g. are currently restoring the latest backup. That\u0027s why it would be a good change to also include additional states in the database query. But as discussed above this is out of scope for this change.\n\nExactly this beforeDataTimestamp is the performance booster for the search for a suitable backup.\n\nThe reason the old code is still included is because, as already mentioned, of backwards capability. What the correct way of handling the edge case of not finding the latest backup is should be another discussion.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":10058,"name":"Erlon R. Cruz","email":"erlon.rodrigues.cruz@canonical.com","username":"sombrafam"},"change_message_id":"92bc192a80972b2f4e197fb67e43f8886246462e","unresolved":true,"context_lines":[{"line_number":306,"context_line":"            )"},{"line_number":307,"context_line":""},{"line_number":308,"context_line":"            if not parent_backup:"},{"line_number":309,"context_line":"                # NOTE (nschwarz) Fetching the parent backup via a database"},{"line_number":310,"context_line":"                # query is the optimized way but due to the previous filtering"},{"line_number":311,"context_line":"                # (including backups in the evaluation because a datetime was"},{"line_number":312,"context_line":"                # set for them which are not in scope for valid parent backups"},{"line_number":313,"context_line":"                # which cannot be implemented in the query it is required to"},{"line_number":314,"context_line":"                # preserve the old behaviour to find a suitable backup."},{"line_number":315,"context_line":"                # This should be removed in further patches and replaced with"},{"line_number":316,"context_line":"                # an api error"},{"line_number":317,"context_line":"                LOG.warning(\u0027Switching to unoptimized behaviour to find \u0027"},{"line_number":318,"context_line":"                            \u0027possible parent backup\u0027)"},{"line_number":319,"context_line":"                backups \u003d objects.BackupList.get_all_by_volume("}],"source_content_type":"text/x-python","patch_set":25,"id":"cfc2dcef_2778b96f","line":316,"range":{"start_line":309,"start_character":0,"end_line":316,"end_character":30},"in_reply_to":"ef3b264e_22561a49","updated":"2026-02-20 16:10:52.000000000","message":"So my point is, both queries filter the same thing. The fallback is effectively dead code: the DB query already applies the same filters, so the only case where it returns None is when no AVAILABLE backup exists. This parent vs latest thing is a bit confusing , so I might be missing something.\n\nAlso, the beforeDataTimestamp filter is out of the contex of this change (performace) and shloud be added in a separate change.","commit_id":"616e382b4b16784de34ebc1393f847d3786f24d2"},{"author":{"_account_id":36171,"name":"jayaanand borra","display_name":"jayaanand borra","email":"jayaanand.borra@netapp.com","username":"jayaanan","status":"netapp"},"change_message_id":"aade4af9eb6e49742f7924c7e77044c0316aeaf8","unresolved":true,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"\"\"\"Handles all requests relating to the volume backups service.\"\"\""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"from __future__ import annotations"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"import random"},{"line_number":23,"context_line":"from typing import Optional"}],"source_content_type":"text/x-python","patch_set":29,"id":"349c02da_9f292601","line":20,"updated":"2026-04-07 08:40:04.000000000","message":"from __future__ import annotations changes all annotations in the file to be lazily evaluated strings. do you see any impact on runtime code?","commit_id":"29594932bee2ef312d9151770bc615221ca41196"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a0a1321ac82e4c33624687340a0b6be397777d75","unresolved":false,"context_lines":[{"line_number":17,"context_line":""},{"line_number":18,"context_line":"\"\"\"Handles all requests relating to the volume backups service.\"\"\""},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"from __future__ import annotations"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"import random"},{"line_number":23,"context_line":"from typing import Optional"}],"source_content_type":"text/x-python","patch_set":29,"id":"c2c30e1b_47c02ae4","line":20,"in_reply_to":"349c02da_9f292601","updated":"2026-04-07 14:19:50.000000000","message":"Wondering how / when that was added. Removed the import now.","commit_id":"29594932bee2ef312d9151770bc615221ca41196"}],"cinder/db/sqlalchemy/api.py":[{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"d8399b3d2296a4d6acef7656ac5cdae9589ca9e4","unresolved":true,"context_lines":[{"line_number":6471,"context_line":"    query \u003d ("},{"line_number":6472,"context_line":"        _backups_get_query(context\u003dcontext, joined_load\u003dFalse,"},{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"}],"source_content_type":"text/x-python","patch_set":16,"id":"668ced84_315f5895","line":6474,"updated":"2024-03-04 15:03:23.000000000","message":"As much as I can tell, filtering by project_id wasn\u0027t done before. What brought this on? Did you run some tests that indicated that it was needed?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7a121c3b4050d613894b1b2caa5914d96bf354b1","unresolved":false,"context_lines":[{"line_number":6471,"context_line":"    query \u003d ("},{"line_number":6472,"context_line":"        _backups_get_query(context\u003dcontext, joined_load\u003dFalse,"},{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"}],"source_content_type":"text/x-python","patch_set":16,"id":"b23790d8_ecd20dcc","line":6474,"in_reply_to":"617a98df_4597c12b","updated":"2024-04-18 13:55:29.000000000","message":"Done","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"1aecd6bd6100065be3fea37fac5052eeff0f525c","unresolved":true,"context_lines":[{"line_number":6471,"context_line":"    query \u003d ("},{"line_number":6472,"context_line":"        _backups_get_query(context\u003dcontext, joined_load\u003dFalse,"},{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"}],"source_content_type":"text/x-python","patch_set":16,"id":"617a98df_4597c12b","line":6474,"in_reply_to":"668ced84_315f5895","updated":"2024-03-04 16:08:27.000000000","message":"It actually was, and there were tests failing.\nPlease see my comments about the last patchset uploads:\n\n* https://review.opendev.org/c/openstack/cinder/+/484729/comments/73593648_a53e338c\n* https://review.opendev.org/c/openstack/cinder/+/484729/comments/ecb1f16a_ad6fee0a\n\n\nIn short, even though I used \"project_only\u003dTrue\" on _backups_get_query, this does not apply when the query is done is admin context, see https://opendev.org/openstack/cinder/src/commit/20fe5b51f6aad2651b460dc43a48d743573ebd02/cinder/db/sqlalchemy/api.py#L315.\n\nAnd this is what https://review.opendev.org/c/openstack/cinder/+/720833 actually fixed.\n\n\nYou may argue filtering on project_id is now done twice. But I thought not setting \"project_only\u003dTrue\" would be even more confusing.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bfcfe9ccc39d15fe9d7b2a6e0ad275dedfb334b3","unresolved":true,"context_lines":[{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"},{"line_number":6478,"context_line":"    )"},{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"}],"source_content_type":"text/x-python","patch_set":16,"id":"4dcdfdae_833c6173","line":6476,"updated":"2024-04-12 14:46:40.000000000","message":"FYI: If the last backup we should be using is being restored, then we would select the wrong backup as parent.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"02596b14584f6976981bbeba864f641f277f1880","unresolved":false,"context_lines":[{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"},{"line_number":6478,"context_line":"    )"},{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"}],"source_content_type":"text/x-python","patch_set":16,"id":"64802bd4_4ae1c60c","line":6476,"in_reply_to":"2b77d8e9_5ff594d7","updated":"2025-10-26 09:18:49.000000000","message":"Done","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a376f36f5e95f0ab074230b264a110672c34a095","unresolved":true,"context_lines":[{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"},{"line_number":6478,"context_line":"    )"},{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"}],"source_content_type":"text/x-python","patch_set":16,"id":"89a1927b_6fccc90e","line":6476,"in_reply_to":"4dcdfdae_833c6173","updated":"2024-04-18 14:44:17.000000000","message":"Should I add the RESTORING status to the list then then?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"f338550bbd6c935f4831cb3ab6f96c36b622e6ff","unresolved":true,"context_lines":[{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"},{"line_number":6478,"context_line":"    )"},{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"}],"source_content_type":"text/x-python","patch_set":16,"id":"2b77d8e9_5ff594d7","line":6476,"in_reply_to":"8753d3c3_9b1622c2","updated":"2024-08-28 09:17:40.000000000","message":"Perhaps add a TODO in the code about that for the future?","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"8abddc1bbdda6a980aae045a6819e287ec271bff","unresolved":true,"context_lines":[{"line_number":6473,"context_line":"                           project_only\u003dTrue)"},{"line_number":6474,"context_line":"        .filter_by(project_id\u003dcontext.project_id)"},{"line_number":6475,"context_line":"        .filter_by(volume_id\u003dvolume_id)"},{"line_number":6476,"context_line":"        .filter_by(status\u003dfields.BackupStatus.AVAILABLE)"},{"line_number":6477,"context_line":"        .order_by(desc(models.Backup.data_timestamp))"},{"line_number":6478,"context_line":"    )"},{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"}],"source_content_type":"text/x-python","patch_set":16,"id":"8753d3c3_9b1622c2","line":6476,"in_reply_to":"89a1927b_6fccc90e","updated":"2024-04-19 15:38:47.000000000","message":"Gorka\u0027s comment makes sense to me but the old code only filtered by AVAILABLE. So this patch at least does not make it worse.","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bfcfe9ccc39d15fe9d7b2a6e0ad275dedfb334b3","unresolved":true,"context_lines":[{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"},{"line_number":6480,"context_line":"    # before a certain time is required add this condition."},{"line_number":6481,"context_line":"    if beforeDataTimestamp:"},{"line_number":6482,"context_line":"        query.filter("},{"line_number":6483,"context_line":"            models.Backup.data_timestamp \u003c beforeDataTimestamp"},{"line_number":6484,"context_line":"        )"},{"line_number":6485,"context_line":"    return query.first()"},{"line_number":6486,"context_line":""},{"line_number":6487,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"556115dc_8ccdc0ad","line":6484,"range":{"start_line":6482,"start_character":1,"end_line":6484,"end_character":9},"updated":"2024-04-12 14:46:40.000000000","message":"-1: Needs to be assigned to the query.\n\n```\nquery \u003d query.filter\n```","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"c593252de9d89dfee4c150eee5544bb7ad15191c","unresolved":false,"context_lines":[{"line_number":6479,"context_line":"    # NOTE(crohmann): In case a backup with a data_timestamp"},{"line_number":6480,"context_line":"    # before a certain time is required add this condition."},{"line_number":6481,"context_line":"    if beforeDataTimestamp:"},{"line_number":6482,"context_line":"        query.filter("},{"line_number":6483,"context_line":"            models.Backup.data_timestamp \u003c beforeDataTimestamp"},{"line_number":6484,"context_line":"        )"},{"line_number":6485,"context_line":"    return query.first()"},{"line_number":6486,"context_line":""},{"line_number":6487,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"520c815b_0d1969a9","line":6484,"range":{"start_line":6482,"start_character":1,"end_line":6484,"end_character":9},"in_reply_to":"556115dc_8ccdc0ad","updated":"2024-04-12 14:53:38.000000000","message":"Acknowledged","commit_id":"b4e46ba5745ebefd3d2092686069e412eac222fa"}],"cinder/objects/backup.py":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"a59530b986328cd86eb633d57536c7a5fc08f09f","unresolved":true,"context_lines":[{"line_number":229,"context_line":"                                                         beforeDataTimestamp)"},{"line_number":230,"context_line":"        if db_backup:"},{"line_number":231,"context_line":"            return cls._from_db_object(context, objects.Backup(), db_backup)"},{"line_number":232,"context_line":"        else:"},{"line_number":233,"context_line":"            return None"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"6e626a5a_298ee032","line":232,"range":{"start_line":232,"start_character":8,"end_line":232,"end_character":12},"updated":"2024-04-12 15:00:39.000000000","message":"nit: This else is not necessary, because on L231 we are returning","commit_id":"3d9b6bf0846531e190c0f4f8758cdf62686e27db"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"9dfdd4fd568d9d22f7faa7dcf4ddcdf8294aabfe","unresolved":false,"context_lines":[{"line_number":229,"context_line":"                                                         beforeDataTimestamp)"},{"line_number":230,"context_line":"        if db_backup:"},{"line_number":231,"context_line":"            return cls._from_db_object(context, objects.Backup(), db_backup)"},{"line_number":232,"context_line":"        else:"},{"line_number":233,"context_line":"            return None"},{"line_number":234,"context_line":""},{"line_number":235,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"30bfab55_79e6ca9a","line":232,"range":{"start_line":232,"start_character":8,"end_line":232,"end_character":12},"in_reply_to":"6e626a5a_298ee032","updated":"2024-04-16 08:20:55.000000000","message":"Acknowledged","commit_id":"3d9b6bf0846531e190c0f4f8758cdf62686e27db"}]}