)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"28536fcef17d252b7668d0504ad8480ebdb4ff7e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"c53c5a0f_f893f857","updated":"2021-12-06 16:24:48.000000000","message":"Thanks for the discussion in last week\u0027s Midcycle meeting.\nI\u0027ve updated the docs to clarify the implications of this feature.\n\nPTAL","commit_id":"384405801f8f77f1ccad91653ca877932eff7228"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"1ccb8e2b_6e9bdc86","updated":"2022-02-22 15:29:20.000000000","message":"Added some enhancements in the new patch, comments below","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d451b647e651443e621e70a04219b8e3c908766f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"b443f624_732f9f38","updated":"2022-01-24 10:16:45.000000000","message":"I bluntly added Brian to the list of reviewers. We discussed about reasons for this change during the weekly call on 2021-11-17. Would be nice if we could still get this into Yoga.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"ad8b20bef3a54fda11da22a5d9eed451744f6224","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"4e78189a_e92ba046","updated":"2022-02-02 13:55:02.000000000","message":"I sincerely hope to not start a big discussion on how the various OpenStack projects define \"unlimited\" or \"undefined\" config values differently.\n\nBut I looked at oslo.config so see how the foundational library used for configuration specifies things and then dug a little into Nova and Designate config values.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"f18184f8b098881f7510868b37c02e50bf095c03","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":6,"id":"d1f1b978_8057b21c","updated":"2022-01-10 16:05:23.000000000","message":"I was wondering if there is anything I can do to help with the review process. If so, please let me know.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"561cdb05403dc42c29136ca402491be5c293c508","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"85e208cd_23aef935","in_reply_to":"d1f1b978_8057b21c","updated":"2024-04-15 15:13:13.000000000","message":"Done","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"44bbc9943b641fc32cfac1284264dadfab669eee","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":8,"id":"aa980349_266af58d","updated":"2022-04-11 08:52:30.000000000","message":"I have uploaded a new PS to resolve the merge conflict.\nWhat is the current status on this patch?","commit_id":"0ccd6aeeee321e065b98f34ad5adf50095cf8e97"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"d8ec5f42_eb91a6ad","in_reply_to":"aa980349_266af58d","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"0ccd6aeeee321e065b98f34ad5adf50095cf8e97"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"9b1abe80a4abc7433d324d43e21b20048eb577fb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"3c333512_adae0235","updated":"2022-05-10 14:29:26.000000000","message":"I just updated the PS to resolve the merge conflict.\nMay I kindly ask for the current status of this patch and for feedback on my comments?","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"007a364653c80041ec01fd15b2c9cf66e2939c0e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"aef2ccdd_0ebd39b1","updated":"2022-05-10 14:58:24.000000000","message":"My issue has been resolved","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":28801,"name":"Cisco Cinder CI","email":"cisco-cinder-ci@cisco.com","username":"cisco-cinder-ci"},"change_message_id":"082c3d104f0533122723f6c62b9e3b62b94fb5cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"33730b53_0c21eec4","updated":"2022-05-26 11:40:48.000000000","message":"cisco-cinder-ci","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":28801,"name":"Cisco Cinder CI","email":"cisco-cinder-ci@cisco.com","username":"cisco-cinder-ci"},"change_message_id":"b2419f326cf9bb646734abeec91cc6738ffe9177","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"5405d882_6559dbfe","updated":"2022-05-25 07:43:33.000000000","message":"cisco-cinder-ci","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"f4bc58310882aa3599b146f1b7cc779c68ed4e0a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"00bd1a94_762c42c7","updated":"2022-05-24 09:18:45.000000000","message":"recheck","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"950cf906e8424115b01026e8662cb951a3244726","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"03620550_8e44ae6c","updated":"2022-05-11 15:12:38.000000000","message":"recheck","commit_id":"8afd4494240748497fd14791375e98cc8fe168a6"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a2867afb4c3b81e6ad44ac5a93552a3637e3f085","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"0c7977ed_8248afca","updated":"2023-09-21 07:35:43.000000000","message":"Could you kindly consider this to be reviewed and merged for Bobcat?\nIt\u0027s just an \u003eoption\u003c, not changing the default behavior in any way.\n\nAnd as a result it would greatly reduce the amount of storage used in some (our) cases where lots of incremental backups following each other are used.","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":27339,"name":"Michal Arbet","email":"michal.arbet@ultimum.io","username":"michalarbet"},"change_message_id":"2567866c564185c6d297175c90e5437f931c4f1a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"19fa14ba_e49102ad","updated":"2024-02-07 07:45:01.000000000","message":"Hi, can you please review/approve this patch ? I think it\u0027s quite important patch, without this cinder-backup is quite unusable in bigger scale...\n\nOr is there anything we are waiting for ?","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"99b2b12648333e1bb8c4098fb5061710257b6cc8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"eceae927_343fc20d","updated":"2023-04-12 12:24:09.000000000","message":"May I ask if there is a chance this chance could be reviewed and merged at some point?","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"abddb58542563030af394d0a7bb6c6f5d0d17059","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"7fd1117b_93647a39","updated":"2024-02-08 00:42:40.000000000","message":"Minor issue noted inline.  I\u0027m still thinking about the logic, just ran out of time this evening.","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"b93330c2b37fb6988b23cb4028bf632707a6b701","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"da9db096_65bc53af","updated":"2023-06-30 07:42:27.000000000","message":"Simon recently triggered the PureStore CI. Is there any chance this change can make it for Bobcat?\n\nAs said, we still believe this is very valid to save on space and it is only optional for operators like us that chose to limit the number of snapshots.","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"253816cb5d25e43c469218dbb9c52fff91a750ed","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"61af4596_7d26e3bc","updated":"2023-12-13 16:24:15.000000000","message":"There was positive feedback on this change during the discussion at the PTR, see https://etherpad.opendev.org/p/caracal-ptg-cinder#L615.\n\nCould someone kindly review this?","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"634afa033de8147dea55182df18ebf7f97abde46","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":10,"id":"6c97f875_3e5632f8","updated":"2022-06-22 15:53:06.000000000","message":"Uploaded another PS to resolve another merge conflict.\n\nSimon already said that the issue he had with the implementation has been resolved. May I ask for further feedback from other reviewers as well?","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7d4efc425f7368a9a096ec1886e9a8a42a8c59d2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"5a1ac051_ddfd37a5","updated":"2024-01-26 13:13:54.000000000","message":"happystacker ... do you mind taking a look at this one?","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"f841645141be55bd8c69dd9d51ab543a140f20ab","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"27436adc_e7122356","updated":"2022-06-24 15:13:56.000000000","message":"run Pure Storage CI","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"4fde1e73_9bce5b7b","in_reply_to":"0c7977ed_8248afca","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"4e99660531c8f7060ed3908a592fcfd2a70b0c21","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"288b6a1f_83b0b59f","in_reply_to":"19fa14ba_e49102ad","updated":"2024-02-07 15:12:41.000000000","message":"I brought this to the attention of the cinder core team at today\u0027s weekly IRC meeting. See https://meetings.opendev.org/meetings/cinder/2024/cinder.2024-02-07-14.00.log.html","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"471deb0e_95ca9185","in_reply_to":"61af4596_7d26e3bc","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"053daf8c_40573c85","in_reply_to":"6c97f875_3e5632f8","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"bd2b1e8e_7efbf681","in_reply_to":"da9db096_65bc53af","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"59eb8f71e4c01ed0c3ae1cd649868118f264eeb5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"eeb5d096_b64b736b","updated":"2024-03-20 13:51:20.000000000","message":"Could you kindly take a look at this one Jon?","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"4a90e907755fcc181421ad1c93b734456181db22","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"15d6ae32_306ad15a","updated":"2024-02-15 05:05:08.000000000","message":"I think I see a fundamental issue with this patch. However, I\u0027m not 100% sure, and it would be great if someone followed my chain of reasoning and verified it.\n\nConsider the following scenario:\n- The snaps_to_keep is nonzero (say 20).\n- Administrator forgets to initiate full backups or left the organization.\n\nEventually, as snaps get deleted, we end in a situation where there\u0027s no base snap (this is where I\u0027m particularly unclear on the mechanism, but it looks like that). Then, we initiate a full backup.\n\nWhat if the full backup that we started fails to complete? At this point, is it possible to restore this volume at all, given that _get_backup_snap_name fails?","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"712d5f0d7629b2dcaec82485cc7469f99dbad469","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"3b287260_3c2354d6","updated":"2024-02-23 14:44:23.000000000","message":"Would this be considered a \"feature\" affected by the upcoming feature freeze?\nIf so, is there any chance to review this for Caracal?","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"06c04c52d9f81cd02552a7aa47faf98a49ceb454","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"2f8e7764_40fdc076","in_reply_to":"15d6ae32_306ad15a","updated":"2024-02-19 09:26:34.000000000","message":"I believe you have a misconception here. The whole mechanism of Ceph cinder-backup driver relies on snapshots and their transfer via \"rbd export | rbd import\" to a different storage. In case of incremental backups it\u0027s actually and \"rbd export-diff\", just exporting the differences (changed blocks) between one snapshot and another.\n\nThe patch is about source snapshots being deleted. They, by definition, cannot be relied on to exist for a restore. Restores exclusively depend on data on the backup storage (site). In essence, the last snapshot missing on the source only causes the next incremental backup to not have a common parent and (thus this very patch) to require a full backup.\n\n\"_get_backup_snap_name\" determines the snapshot for a certain backup_id ([1]). If it\u0027s missing a new incremental backup, the task fails currently. But with this patch a full backup (\"rbd export $volume\") is done instead, as we are \"expecting\" not all source snapshots to exist anymore.\n\n\n[1] https://github.com/openstack/cinder/blob/7245ec027b1902dc24c30cf3885a5dfec1866a6a/cinder/backup/drivers/ceph.py#L982","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"2e5c8cd5605b54c0b827caef1cab465f87a8bd75","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"6e2813db_2d711c4a","in_reply_to":"2f8e7764_40fdc076","updated":"2024-03-07 03:52:42.000000000","message":"Acknowledged","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d2d0027e3182f69e26ffcae16c732f2c4a96af0e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"9ff27418_df9500b7","updated":"2024-04-17 13:03:38.000000000","message":"I uploaded a new PS.","commit_id":"f938e5c0c74383dd99ec513049b208f0226273d7"},{"author":{"_account_id":34150,"name":"Simon Hensel","email":"simon.hensel@inovex.de","username":"shensel"},"change_message_id":"66c5480f50a7e263b6da9e64c6705c8dc02676c3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"f842f6d3_28fd335e","updated":"2024-09-10 08:56:33.000000000","message":"Any chance to get this completed and merged into master? It looks like this patch is completed so far.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"992d4a70c6930b1bd10eefe8778d2341cd9b43d5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"4343b1b7_dd2716b7","updated":"2024-06-03 09:14:37.000000000","message":"Any change this might get a Workflow+1 from someone?","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"083fdf41b7da0a75bf5943e52524d9326bff8047","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"82786f93_ae2b7edf","updated":"2024-04-23 15:58:25.000000000","message":"CI issues aside, I\u0027m offering my +1 and will monitor feedback from core reviewers.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"7ff57b8722bbb1d55598d82192beb644ba578d3a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"fb06907f_1207a6a7","updated":"2024-07-23 07:05:10.000000000","message":"It looks like this one is just staring at the finish line?\nIs there any chance this could move forward and be merged?","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"e2a877c493426c0804109886b8b7650776d430ff","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"8a88f6fe_6b9e2145","updated":"2024-06-10 15:22:17.000000000","message":"May I gently ask again if there is anything left for this change to be merged?","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"249c5f06d80a74a7f1f3452f7f36de97288b785e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"ec422954_a5318245","updated":"2024-05-18 16:32:01.000000000","message":"Thanks for your review and 2x +2 !\nWhat is left to also have this receive a Workflow+1 ?","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"ee1710b5_4114316c","updated":"2024-05-17 15:43:39.000000000","message":"This needs a follow up patch to change 2 debug messages to warnings and to add the configuration option name to the release notes, but I think we can merge this patch as it is.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"04e8e07aae6707ad4a74de6fab0f2409099a5d86","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"c2f1a815_025c00eb","updated":"2024-08-28 08:49:08.000000000","message":"ping to get this merged in master, would really like to get the backport train started on this.\n\njust a FYI as well we\u0027re running this in production with very slight modifications","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"c79dc1db5e6e8b40071d8cefd686506e3b6b0aea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"24821e15_12eb21ff","updated":"2024-04-24 12:11:30.000000000","message":"recheck cinder-sqlalchemy-2x","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"56e55c7b83771e080ac8c2c9739b8e29e8273e75","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"b9bcda27_9ba3c47d","updated":"2024-05-15 18:32:47.000000000","message":"recheck cinder-tempest-plugin-lvm-lio-barbican","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"71e850dd9b4e975dc23668979a823ce65236b2f4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"32c497b9_c65daba9","updated":"2024-05-15 14:44:31.000000000","message":"recheck tempest-slow-py3","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"d54f59c3d12d2cefa2cd8dc7a7d18f0b89445859","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":15,"id":"fcd360b6_00cb52f2","updated":"2024-05-20 13:22:07.000000000","message":"would really like the warning changes (and probably the return if snapshot list fails) but can be done in a follow-up.\n\ni\u0027ve tested the removing source snapshot part of this code (as we\u0027re already running parts of the patch)","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"}],"cinder/backup/drivers/ceph.py":[{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"c16033593d3fd91101e4dc2539fe81763c27ed7c","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"920c6196_4ebe79b8","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"updated":"2022-01-26 13:56:03.000000000","message":"Given your tests for this are \u003e0, why have this set to -1 and not 0?","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"ddcec854e7c77f29251ad2aea020fccf24759707","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"ff05f011_b723414c","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"022a5ab5_7d102562","updated":"2022-01-26 14:48:28.000000000","message":"\u003e Is that actually documented? If not it should be, at least, mentioned here\n\nWould a rewording be of the help string to e.g: \"Settings this to \u003e\u003d1 can save disk space [...]\" be sufficient? \n\nIt\u0027s simply NOT limited by default (without this code or with backup_ceph_keep_snapshots_count\u003d-1 which is the default with the new code).","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"ad8b20bef3a54fda11da22a5d9eed451744f6224","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"d269df3e_8efb54d9","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"5c87089a_14eb3823","updated":"2022-02-02 13:55:02.000000000","message":"\u003e -1: Defaults is -1 (unlimited), but 0 is also unlimited.  We should have only 1 acceptable value for unlimited.\n\nLooking at the various config parameters available I agree with your point that cinder uses \"0\" to set something to \"unlimited\" in case of integers (https://docs.openstack.org/cinder/xena/configuration/block-storage/samples/cinder.conf.html)\n\n\nBut looking at the oslo.config reference for integers parameters at https://docs.openstack.org/oslo.config/latest/reference/api/oslo_config.html#oslo_config.types.Integer there actually is the \u0027None\u0027 value.\n\nLooking at how other projects like e.g. Nova or Designate are using \u0027None\u0027:\n\n * https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.metadata_workers\n * https://docs.openstack.org/designate/latest/admin/config.html#service:api.default_limit_v2\n\nit seems a default of \u0027None\u0027 and then a minimum value of \u00271\u0027 would make the most sense here: \n\n 1) You either enable a functionality by setting any value but the default of \u0027None\u0027.\n 2) But you then limit the range to the sensible values. This rules out \"0\" which renders incremental backups non-functional. (Setting a e.g. a \"service:api.default_limit_v2\" to \"0\" defaults to not return any records ;-)","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"fc955d4b_855e9af1","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"5c87089a_14eb3823","updated":"2022-02-22 15:29:20.000000000","message":"While I think a value of 0 for \"unlimited\" could cause confusion because 0 rather indicates \"none\", I agree on maintaining Cinder\u0027s current default value for \"unlimited\", which turns out to be 0.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"22df598f77dea70d1a159bd3ebab616f459fce28","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"95b948c8_80bc6ec1","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"920c6196_4ebe79b8","updated":"2022-01-26 14:09:26.000000000","message":"-1 \u003d unlimited","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"14b1224eafc1a9efb13b87a4ab775e01d36a6186","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"022a5ab5_7d102562","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"95b948c8_80bc6ec1","updated":"2022-01-26 14:20:19.000000000","message":"Is that actually documented? If not it should be, at least, mentioned here","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"5c87089a_14eb3823","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"d28ff682_ba917cfd","updated":"2022-01-31 10:07:59.000000000","message":"-1: Defaults is -1 (unlimited), but 0 is also unlimited.  We should have only 1 acceptable value for unlimited.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"84faabb760d4a2fc63ee79e19065087b520acecb","unresolved":true,"context_lines":[{"line_number":91,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":92,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":93,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"}],"source_content_type":"text/x-python","patch_set":6,"id":"d28ff682_ba917cfd","line":94,"range":{"start_line":94,"start_character":51,"end_line":94,"end_character":61},"in_reply_to":"ff05f011_b723414c","updated":"2022-01-26 15:14:30.000000000","message":"Yeah - that would work I think","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"},{"line_number":98,"context_line":"                    \u0027However, if a user deletes more incremental backups of a\u0027"},{"line_number":99,"context_line":"                    \u0027volume than the set value, the following incremental\u0027"},{"line_number":100,"context_line":"                    \u0027backup will automatically be a full backup.\u0027),"}],"source_content_type":"text/x-python","patch_set":6,"id":"aaa34425_f352ce7e","line":97,"range":{"start_line":97,"start_character":58,"end_line":97,"end_character":73},"updated":"2022-01-31 10:07:59.000000000","message":"nit: source volume storage.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":94,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d-1,"},{"line_number":95,"context_line":"               help\u003d\u0027Number of the most recent snapshots to keep.\u0027"},{"line_number":96,"context_line":"                    \u0027Enabling this option can save disk space by only keeping\u0027"},{"line_number":97,"context_line":"                    \u0027a limited number of snapshots on the volume storage.\u0027"},{"line_number":98,"context_line":"                    \u0027However, if a user deletes more incremental backups of a\u0027"},{"line_number":99,"context_line":"                    \u0027volume than the set value, the following incremental\u0027"},{"line_number":100,"context_line":"                    \u0027backup will automatically be a full backup.\u0027),"}],"source_content_type":"text/x-python","patch_set":6,"id":"b1446d43_8086d575","line":97,"range":{"start_line":97,"start_character":58,"end_line":97,"end_character":73},"in_reply_to":"aaa34425_f352ce7e","updated":"2022-02-22 15:29:20.000000000","message":"Ack","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":730,"context_line":"                from_snap \u003d self._get_backup_snap_name(base_rbd,"},{"line_number":731,"context_line":"                                                       base_name,"},{"line_number":732,"context_line":"                                                       last_incr)"},{"line_number":733,"context_line":"                if from_snap is None:"},{"line_number":734,"context_line":"                    msg \u003d (_("},{"line_number":735,"context_line":"                        \"Can\u0027t find snapshot from parent %(incr)s and \""},{"line_number":736,"context_line":"                        \"base name image %(base)s.\") %"},{"line_number":737,"context_line":"                        {\u0027incr\u0027: last_incr, \u0027base\u0027: base_name})"},{"line_number":738,"context_line":"                    LOG.error(msg)"},{"line_number":739,"context_line":"                    raise exception.BackupRBDOperationFailed(msg)"},{"line_number":740,"context_line":"            finally:"},{"line_number":741,"context_line":"                base_rbd.close()"},{"line_number":742,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"cdf4bc9c_2d88017d","line":739,"range":{"start_line":733,"start_character":0,"end_line":739,"end_character":65},"updated":"2022-01-31 10:07:59.000000000","message":"-1: Previously this would not happen unless an operator had manually deleted snapshot in the Ceph cluster, in which case they are on their own, but now this can easily happen with the new config option, so we have to handle it better.\n\nIn my opinion there are 2 things that need to happen in this case:\n\n- The backup has to be marked as not being an incremental backup in Cinder\u0027s database.  This is important in case someone is using the records to charge, it avoids creating a false chain of backups (which prevent deleting the false parent before the new backup), and it also makes it easier for user to understand why the backup is taking longer.  This is done by returning {\"parent_id\": None} as part of the updates dictionary of the \"backup\" method.  It\u0027s dirty and is part of the reason why I didn\u0027t want this new feature to be done like this, because the user doesn\u0027t get what they asked for.\n\n- Need a new \"user message\" indicating that the backup will not be incremental even if it was asked to be, otherwise they could go crazy because they asked for incremental and they didn\u0027t get it. (look into cinder/messages)","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[{"line_number":730,"context_line":"                from_snap \u003d self._get_backup_snap_name(base_rbd,"},{"line_number":731,"context_line":"                                                       base_name,"},{"line_number":732,"context_line":"                                                       last_incr)"},{"line_number":733,"context_line":"                if from_snap is None:"},{"line_number":734,"context_line":"                    msg \u003d (_("},{"line_number":735,"context_line":"                        \"Can\u0027t find snapshot from parent %(incr)s and \""},{"line_number":736,"context_line":"                        \"base name image %(base)s.\") %"},{"line_number":737,"context_line":"                        {\u0027incr\u0027: last_incr, \u0027base\u0027: base_name})"},{"line_number":738,"context_line":"                    LOG.error(msg)"},{"line_number":739,"context_line":"                    raise exception.BackupRBDOperationFailed(msg)"},{"line_number":740,"context_line":"            finally:"},{"line_number":741,"context_line":"                base_rbd.close()"},{"line_number":742,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"18a1f855_41211fb3","line":739,"range":{"start_line":733,"start_character":0,"end_line":739,"end_character":65},"in_reply_to":"05ce93ad_5501f7fc","updated":"2024-04-15 15:12:04.000000000","message":"Done","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"677f05c9fb9308a6211b8e2e25cc3dd97a4a3e9e","unresolved":true,"context_lines":[{"line_number":730,"context_line":"                from_snap \u003d self._get_backup_snap_name(base_rbd,"},{"line_number":731,"context_line":"                                                       base_name,"},{"line_number":732,"context_line":"                                                       last_incr)"},{"line_number":733,"context_line":"                if from_snap is None:"},{"line_number":734,"context_line":"                    msg \u003d (_("},{"line_number":735,"context_line":"                        \"Can\u0027t find snapshot from parent %(incr)s and \""},{"line_number":736,"context_line":"                        \"base name image %(base)s.\") %"},{"line_number":737,"context_line":"                        {\u0027incr\u0027: last_incr, \u0027base\u0027: base_name})"},{"line_number":738,"context_line":"                    LOG.error(msg)"},{"line_number":739,"context_line":"                    raise exception.BackupRBDOperationFailed(msg)"},{"line_number":740,"context_line":"            finally:"},{"line_number":741,"context_line":"                base_rbd.close()"},{"line_number":742,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"05ce93ad_5501f7fc","line":739,"range":{"start_line":733,"start_character":0,"end_line":739,"end_character":65},"in_reply_to":"231bee02_7242a84f","updated":"2024-02-13 12:58:25.000000000","message":"Done, I adapted the message level to warning.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":true,"context_lines":[{"line_number":730,"context_line":"                from_snap \u003d self._get_backup_snap_name(base_rbd,"},{"line_number":731,"context_line":"                                                       base_name,"},{"line_number":732,"context_line":"                                                       last_incr)"},{"line_number":733,"context_line":"                if from_snap is None:"},{"line_number":734,"context_line":"                    msg \u003d (_("},{"line_number":735,"context_line":"                        \"Can\u0027t find snapshot from parent %(incr)s and \""},{"line_number":736,"context_line":"                        \"base name image %(base)s.\") %"},{"line_number":737,"context_line":"                        {\u0027incr\u0027: last_incr, \u0027base\u0027: base_name})"},{"line_number":738,"context_line":"                    LOG.error(msg)"},{"line_number":739,"context_line":"                    raise exception.BackupRBDOperationFailed(msg)"},{"line_number":740,"context_line":"            finally:"},{"line_number":741,"context_line":"                base_rbd.close()"},{"line_number":742,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"f0d0d7a4_146eef66","line":739,"range":{"start_line":733,"start_character":0,"end_line":739,"end_character":65},"in_reply_to":"cdf4bc9c_2d88017d","updated":"2022-02-22 15:29:20.000000000","message":"To address this issue, I have added a new pre-check to see whether an incremental backup would be possible (if one was requested). If not possible, we fall back to a full backup instead and therefore do not run into this exception.\nThe backup will then be marked as full in the database by having no parent_id.\n\nI also added a new user message for that case, like you suggested, though it\u0027s still WIP. It\u0027s not fully clear to me yet where exactly it will/should be displayed. Right now, I can only see it via openstack volume message list/show.\nAdditionally, the mapping from Detail ID to Detail message seems to not work. The new ID is always mapped to the default \"unknown error\". Am I missing something here?\nProbably the message level should also be something different than \"error\".","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"abddb58542563030af394d0a7bb6c6f5d0d17059","unresolved":true,"context_lines":[{"line_number":730,"context_line":"                from_snap \u003d self._get_backup_snap_name(base_rbd,"},{"line_number":731,"context_line":"                                                       base_name,"},{"line_number":732,"context_line":"                                                       last_incr)"},{"line_number":733,"context_line":"                if from_snap is None:"},{"line_number":734,"context_line":"                    msg \u003d (_("},{"line_number":735,"context_line":"                        \"Can\u0027t find snapshot from parent %(incr)s and \""},{"line_number":736,"context_line":"                        \"base name image %(base)s.\") %"},{"line_number":737,"context_line":"                        {\u0027incr\u0027: last_incr, \u0027base\u0027: base_name})"},{"line_number":738,"context_line":"                    LOG.error(msg)"},{"line_number":739,"context_line":"                    raise exception.BackupRBDOperationFailed(msg)"},{"line_number":740,"context_line":"            finally:"},{"line_number":741,"context_line":"                base_rbd.close()"},{"line_number":742,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"231bee02_7242a84f","line":739,"range":{"start_line":733,"start_character":0,"end_line":739,"end_character":65},"in_reply_to":"f0d0d7a4_146eef66","updated":"2024-02-08 00:42:40.000000000","message":"I agree about the message level, make it either \u0027WARNING\u0027 or \u0027INFO\u0027 (or you could make something else up, it\u0027s just a string and I don\u0027t think we check anywhere to make sure it conforms to some particular enum).","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":799,"context_line":""},{"line_number":800,"context_line":"            # only keep last n snapshots and delete older ones"},{"line_number":801,"context_line":"            if CONF.backup_ceph_keep_snapshots_count \u003e 0:"},{"line_number":802,"context_line":"                snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":803,"context_line":"                remaining_snaps \u003d len(snap_list)"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":"                LOG.debug(\"Snapshot list: %s\", snap_list)"}],"source_content_type":"text/x-python","patch_set":6,"id":"8181e227_6afdc2e6","line":802,"updated":"2022-01-31 10:07:59.000000000","message":"-1: Failure to list snapshots at this point should never prevent the backup from being marked as ok.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":799,"context_line":""},{"line_number":800,"context_line":"            # only keep last n snapshots and delete older ones"},{"line_number":801,"context_line":"            if CONF.backup_ceph_keep_snapshots_count \u003e 0:"},{"line_number":802,"context_line":"                snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":803,"context_line":"                remaining_snaps \u003d len(snap_list)"},{"line_number":804,"context_line":""},{"line_number":805,"context_line":"                LOG.debug(\"Snapshot list: %s\", snap_list)"}],"source_content_type":"text/x-python","patch_set":6,"id":"34514cb6_82df11c7","line":802,"in_reply_to":"8181e227_6afdc2e6","updated":"2022-02-22 15:29:20.000000000","message":"Ack, use try/except to handle errors","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":806,"context_line":""},{"line_number":807,"context_line":"                if remaining_snaps \u003e CONF.backup_ceph_keep_snapshots_count:"},{"line_number":808,"context_line":"                    snaps_to_delete \u003d \\"},{"line_number":809,"context_line":"                        remaining_snaps - CONF.backup_ceph_keep_snapshots_count"},{"line_number":810,"context_line":"                    LOG.debug(\"There are %s snapshots and %s should be kept, \""},{"line_number":811,"context_line":"                              \"deleting the oldest %s snapshots\","},{"line_number":812,"context_line":"                              remaining_snaps,"}],"source_content_type":"text/x-python","patch_set":6,"id":"da0788f8_ae64bd2b","line":809,"range":{"start_line":809,"start_character":42,"end_line":809,"end_character":79},"updated":"2022-01-31 10:07:59.000000000","message":"nit: maybe assign this to a variable so the code in this method is a bit easier to read","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":806,"context_line":""},{"line_number":807,"context_line":"                if remaining_snaps \u003e CONF.backup_ceph_keep_snapshots_count:"},{"line_number":808,"context_line":"                    snaps_to_delete \u003d \\"},{"line_number":809,"context_line":"                        remaining_snaps - CONF.backup_ceph_keep_snapshots_count"},{"line_number":810,"context_line":"                    LOG.debug(\"There are %s snapshots and %s should be kept, \""},{"line_number":811,"context_line":"                              \"deleting the oldest %s snapshots\","},{"line_number":812,"context_line":"                              remaining_snaps,"}],"source_content_type":"text/x-python","patch_set":6,"id":"e12ca6fc_15049c74","line":809,"range":{"start_line":809,"start_character":42,"end_line":809,"end_character":79},"in_reply_to":"da0788f8_ae64bd2b","updated":"2022-02-22 15:29:20.000000000","message":"Ack","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"                    for i in range(snaps_to_delete):"},{"line_number":817,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":818,"context_line":"                        source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":819,"context_line":"                else:"},{"line_number":820,"context_line":"                    LOG.debug(\"There are %s snapshots and %s should be kept, \""},{"line_number":821,"context_line":"                              \"not deleting any snapshots\", remaining_snaps,"}],"source_content_type":"text/x-python","patch_set":6,"id":"b56e4f50_e41f0b3d","line":818,"updated":"2022-01-31 10:07:59.000000000","message":"-1: Failure to delete an image will prevent the driver from marking the backup as successful.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":815,"context_line":""},{"line_number":816,"context_line":"                    for i in range(snaps_to_delete):"},{"line_number":817,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":818,"context_line":"                        source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":819,"context_line":"                else:"},{"line_number":820,"context_line":"                    LOG.debug(\"There are %s snapshots and %s should be kept, \""},{"line_number":821,"context_line":"                              \"not deleting any snapshots\", remaining_snaps,"}],"source_content_type":"text/x-python","patch_set":6,"id":"fcb08aec_dd67af75","line":818,"in_reply_to":"b56e4f50_e41f0b3d","updated":"2022-02-22 15:29:20.000000000","message":"Use try/except here as well","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"abddb58542563030af394d0a7bb6c6f5d0d17059","unresolved":true,"context_lines":[{"line_number":104,"context_line":"                    \u0027However, if a user deletes at least the set value of\u0027"},{"line_number":105,"context_line":"                    \u0027incremental backups of a volume, the following\u0027"},{"line_number":106,"context_line":"                    \u0027incremental backup will automatically become a\u0027"},{"line_number":107,"context_line":"                    \u0027full backup.\u0027),"},{"line_number":108,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"},{"line_number":109,"context_line":"                help\u003d\u0027If True, always discard excess bytes when restoring \u0027"},{"line_number":110,"context_line":"                     \u0027volumes i.e. pad with zeroes.\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"936dbd5b_4cd0a5b9","line":107,"updated":"2024-02-08 00:42:40.000000000","message":"The help text all runs together due to lack of spaces.  Since it\u0027s long (and may require more editing) I suggest trying this:\n\nimport textwrap (around line 50), and then rewrite this option definition as:\n\n    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d0,\n               help\u003dtextwrap.dedent(\"\"\"\\\n    Number of the most recent snapshots to keep.\n\n    0 indicates to keep an unlimited amount of snapshots.\n\n    Enabling this option can save disk space by only keeping a limited number\n    of snapshots on the source volume storage. However, if a user deletes at\n    least the set value of incremental backups of a volume, the following\n    incremental backup will automatically become a full backup.\n    \"\"\")\n               ),","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"677f05c9fb9308a6211b8e2e25cc3dd97a4a3e9e","unresolved":false,"context_lines":[{"line_number":104,"context_line":"                    \u0027However, if a user deletes at least the set value of\u0027"},{"line_number":105,"context_line":"                    \u0027incremental backups of a volume, the following\u0027"},{"line_number":106,"context_line":"                    \u0027incremental backup will automatically become a\u0027"},{"line_number":107,"context_line":"                    \u0027full backup.\u0027),"},{"line_number":108,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"},{"line_number":109,"context_line":"                help\u003d\u0027If True, always discard excess bytes when restoring \u0027"},{"line_number":110,"context_line":"                     \u0027volumes i.e. pad with zeroes.\u0027)"}],"source_content_type":"text/x-python","patch_set":10,"id":"25e7a466_79e77fad","line":107,"in_reply_to":"936dbd5b_4cd0a5b9","updated":"2024-02-13 12:58:25.000000000","message":"Done","commit_id":"903a93b464c5e057b526e023a52ddea96ca6cb53"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"faf10f97d5fad0e3aab3b6947c3bf04a0461266c","unresolved":true,"context_lines":[{"line_number":97,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":98,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":99,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":100,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d0,"},{"line_number":101,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":102,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":103,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"65f32a2e_fc44ec25","line":100,"range":{"start_line":100,"start_character":16,"end_line":100,"end_character":48},"updated":"2024-04-12 15:40:34.000000000","message":"\u0027backup_ceph_max_snapshots\u0027 seems clearer to me.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":true,"context_lines":[{"line_number":97,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":98,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":99,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":100,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d0,"},{"line_number":101,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":102,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":103,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"41bd4b16_ff7c92e2","line":100,"updated":"2024-05-17 15:43:39.000000000","message":"maybe also add `min\u003d0`","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":false,"context_lines":[{"line_number":97,"context_line":"    cfg.BoolOpt(\u0027backup_ceph_image_journals\u0027, default\u003dFalse,"},{"line_number":98,"context_line":"                help\u003d\u0027If True, apply JOURNALING and EXCLUSIVE_LOCK feature \u0027"},{"line_number":99,"context_line":"                     \u0027bits to the backup RBD objects to allow mirroring\u0027),"},{"line_number":100,"context_line":"    cfg.IntOpt(\u0027backup_ceph_keep_snapshots_count\u0027, default\u003d0,"},{"line_number":101,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":102,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":103,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"93a6b781_1d57b519","line":100,"range":{"start_line":100,"start_character":16,"end_line":100,"end_character":48},"in_reply_to":"65f32a2e_fc44ec25","updated":"2024-04-16 11:07:03.000000000","message":"Acknowledged","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"faf10f97d5fad0e3aab3b6947c3bf04a0461266c","unresolved":true,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"fbda92b2_d951f6a4","line":109,"range":{"start_line":108,"start_character":29,"end_line":109,"end_character":51},"updated":"2024-04-12 15:40:34.000000000","message":"I don\u0027t understand what this means. The \"at least the set value of\" part confuses me.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"faf10f97d5fad0e3aab3b6947c3bf04a0461266c","unresolved":true,"context_lines":[{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"cd74a6a6_bdfc0cda","line":109,"range":{"start_line":109,"start_character":57,"end_line":109,"end_character":66},"updated":"2024-04-12 15:40:34.000000000","message":"nit: \"the next backup\"","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7c6a40b6afa1399b5ab84ef1c83d3f6a0787b6f2","unresolved":true,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"158961ad_20542a05","line":109,"range":{"start_line":108,"start_character":29,"end_line":109,"end_character":51},"in_reply_to":"05a39b3b_a1a8e5a5","updated":"2024-04-16 15:00:53.000000000","message":"Thanks, the update is clearer to me. I note a few grammar nits, but they are minor.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d2d0027e3182f69e26ffcae16c732f2c4a96af0e","unresolved":false,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"624ecc16_9d7ba807","line":109,"range":{"start_line":108,"start_character":29,"end_line":109,"end_character":51},"in_reply_to":"158961ad_20542a05","updated":"2024-04-17 13:03:38.000000000","message":"Acknowledged","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":false,"context_lines":[{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"8fe4a343_2f524bc1","line":109,"range":{"start_line":109,"start_character":57,"end_line":109,"end_character":66},"in_reply_to":"cd74a6a6_bdfc0cda","updated":"2024-04-16 11:07:03.000000000","message":"Acknowledged","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":true,"context_lines":[{"line_number":105,"context_line":""},{"line_number":106,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":107,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":108,"context_line":"                    However, if a user deletes at least the set value of"},{"line_number":109,"context_line":"                    incremental backups of a volume, the following incremental"},{"line_number":110,"context_line":"                    backup will automatically become a full backup."},{"line_number":111,"context_line":"                \"\"\")),"},{"line_number":112,"context_line":"    cfg.BoolOpt(\u0027restore_discard_excess_bytes\u0027, default\u003dTrue,"}],"source_content_type":"text/x-python","patch_set":11,"id":"05a39b3b_a1a8e5a5","line":109,"range":{"start_line":108,"start_character":29,"end_line":109,"end_character":51},"in_reply_to":"fbda92b2_d951f6a4","updated":"2024-04-16 11:07:03.000000000","message":"I have changed the description. Maybe it is now clearer. Let me know.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"faf10f97d5fad0e3aab3b6947c3bf04a0461266c","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"31a6c2c2_4750ec07","line":821,"updated":"2024-04-12 15:40:34.000000000","message":"I would revise the flow of logic. The original code essentially does this:\n\n```\n  if backup.parent_id is None:\n    create a full backup\n  else\n    create an incremental backup\n```\n\nI think the new code could add a block before the current code, so it would look like this:\n\n```\n  # Add new code\n  if backup.parent_id is not None:\n    look for source snaps, etc.\n    if no_source_snaps:\n      set backup.parent and backup_parent_id to None, etc.\n  # End of new code\n\n  # Resume original code \n  if backup.parent_id is None:\n    create a full backup\n  else\n    create an incremental backup\n ```","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"559c7eda241139409b5af00580a4c5b2125c1f7d","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"92e504c7_603b3748","line":821,"in_reply_to":"0683a5a3_c4b15eee","updated":"2024-04-24 06:37:50.000000000","message":"\u003e I agree with Alan.\n\u003e \n\u003e Should we really revert to a full backup here if the user explicitly requested an incremental? Before we\u0027d fail the operation and now we try to solve it.\n\u003e \n\u003e I agree with the whole trying to cleanup as much snapshots as possible on the source image, that\u0027s what I\u0027m after, but returning an error message in `fail_reason` that you must create a new full backup, isn\u0027t that also reasonable?\n\nIt certainly is ALSO reasonable. See previous discussion on handling this case\n\nhttps://review.opendev.org/c/openstack/cinder/+/810457/comment/cdf4bc9c_2d88017d/\n\n\n\n\u003e The saving space is the operators pain-point but the configuration option added is also what restricts the end-user to how many incremental backups they can do, no?\n\nNo. Removing source snapshots does NOT change the ability for users to do more incremental backups. It means they can only DELETE so many incremental backups (at the destination) before there is no common source anymore and either a full is done automagically or the request fails.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"d5cc8063_e67b6919","line":821,"in_reply_to":"31a6c2c2_4750ec07","updated":"2024-04-16 11:07:03.000000000","message":"I have not understand what is the benefit of extracting the lookup of snapshots before the actual incremental backup happens. Can you please give more information on your requested change.\n\nIn my opinion this code should stay were it is to be more descriptive in the flow when reading it. Another solution would be to extract the mentioned code into its own method for more clarity.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d4c90b49a1a06a68005fbcfda1d4826481925830","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"cfcbb4ca_3fce18d8","line":821,"in_reply_to":"482dc636_ebb69f8f","updated":"2024-04-24 14:00:34.000000000","message":"\u003e IMO the fallback should be configurable, if I have software that handles creation of backups I want to explicitly fail if the operation cannot be performed (incremental backup) so that I can take a decision on what to do next instead of it being transparent.\n\nI agree that both approaches have valid reasons depending on either the user initiating a backup or the operator doing so. I can only second my reference to https://review.opendev.org/c/openstack/cinder/+/810457/comment/cdf4bc9c_2d88017d/ where Gorka asked for the \"automatic\" handling of missing snapshots by changing the backup to a full.\nThe issue is that the user does not have any idea about source snapshots and why their absence would cause their back to fail.\nAnd apparently the use of the backup API by operators is somewhat of a less common scenario.\n\nThe longer this change here is discussed, the more arguments are repeated and re-iterated. If there is an agreement of how this should be done I gladly change / add it.\n\n\n\u003e I also think the logic could be more clear and not nested like Alan said, it also warrants some more tests just due to the sensitivity of data and backups.\n\nIf you look at the two conditionals\n\n1) \n```\n# If backup.parent_id is None performs full RBD backup\nif backup.parent_id is None:\n```\n\n2) \n```\n# Otherwise performs incremental rbd backup\nelse:\n```\n\n\nthere already is the clean distinction between the full-backup path and the incremental-backup path. Handling the snapshots is only relevant when doing an incremental backups. So I don\u0027t really like adding yet another conditional in front creating somewhat of a third path or that repeats checking if this backup is an incremental one.\n\nBut yes, the logic to determine if the incremental can happen should be externalized, I shall push another change to do so soon.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"cc795004bf474baa07b9b9e98544bc6ba74101d4","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"0683a5a3_c4b15eee","line":821,"in_reply_to":"554cecd4_f117803f","updated":"2024-04-23 20:40:12.000000000","message":"I agree with Alan.\n\nShould we really revert to a full backup here if the user explicitly requested an incremental? Before we\u0027d fail the operation and now we try to solve it.\n\nI agree with the whole trying to cleanup as much snapshots as possible on the source image, that\u0027s what I\u0027m after, but returning an error message in `fail_reason` that you must create a new full backup, isn\u0027t that also reasonable?\n\nThe saving space is the operators pain-point but the configuration option added is also what restricts the end-user to how many incremental backups they can do, no?","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"125f3d2a08d17f715ea8d2ec40f5198849830112","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"482dc636_ebb69f8f","line":821,"in_reply_to":"92e504c7_603b3748","updated":"2024-04-24 13:38:59.000000000","message":"I think I understand the edge cases it creates now.\n\nIMO the fallback should be configurable, if I have software that handles creation of backups I want to explicitly fail if the operation cannot be performed (incremental backup) so that I can take a decision on what to do next instead of it being transparent.\n\nI also think the logic could be more clear and not nested like Alan said, it also warrants some more tests just due to the sensitivity of data and backups.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7c6a40b6afa1399b5ab84ef1c83d3f6a0787b6f2","unresolved":true,"context_lines":[{"line_number":818,"context_line":"                                                             length)"},{"line_number":819,"context_line":"        # Otherwise performs incremental rbd backup"},{"line_number":820,"context_line":"        else:"},{"line_number":821,"context_line":"            # Check if there is at least one snapshot to base an incremental"},{"line_number":822,"context_line":"            # backup on. If not, we cannot perform an incremental backup and"},{"line_number":823,"context_line":"            # fall back to full backup."},{"line_number":824,"context_line":"            no_source_snaps \u003d snaps_to_keep \u003e 0 and \\"}],"source_content_type":"text/x-python","patch_set":11,"id":"554cecd4_f117803f","line":821,"in_reply_to":"d5cc8063_e67b6919","updated":"2024-04-16 15:00:53.000000000","message":"This may be a minor issue and a matter of personal preference. The current code adds some nested logic, which results in a full backup being called from two separate places (L825 and L863). My suggestion would relocate the code that verifies an incremental backup is possible. The logic isn\u0027t as deeply nested, and doesn\u0027t introduce a second call to self._full_rbd_backup().\n\nI respect your own preference, and will let other reviewers add their comments.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"10729f924fb11f226cddc17d76de46e0da0b30b7","unresolved":true,"context_lines":[{"line_number":834,"context_line":"                base_name \u003d self.\\"},{"line_number":835,"context_line":"                    _get_backup_base_name(volume_id, backup\u003dbackup)"},{"line_number":836,"context_line":""},{"line_number":837,"context_line":"                # The backup will be a full one, so it has no parent ID."},{"line_number":838,"context_line":"                # This will mark the backup as a full backup in the database."},{"line_number":839,"context_line":"                backup.parent_id \u003d None"},{"line_number":840,"context_line":"                backup.save()"},{"line_number":841,"context_line":""},{"line_number":842,"context_line":"                LOG.info(\"Incremental backup was requested, but there are no \""},{"line_number":843,"context_line":"                         \"snapshots present to use as base, \""}],"source_content_type":"text/x-python","patch_set":11,"id":"868daebe_517359aa","line":840,"range":{"start_line":837,"start_character":1,"end_line":840,"end_character":29},"updated":"2024-04-12 15:27:26.000000000","message":"nit: Maybe do these 2 together with the `backup.parent \u003d None` code","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":false,"context_lines":[{"line_number":834,"context_line":"                base_name \u003d self.\\"},{"line_number":835,"context_line":"                    _get_backup_base_name(volume_id, backup\u003dbackup)"},{"line_number":836,"context_line":""},{"line_number":837,"context_line":"                # The backup will be a full one, so it has no parent ID."},{"line_number":838,"context_line":"                # This will mark the backup as a full backup in the database."},{"line_number":839,"context_line":"                backup.parent_id \u003d None"},{"line_number":840,"context_line":"                backup.save()"},{"line_number":841,"context_line":""},{"line_number":842,"context_line":"                LOG.info(\"Incremental backup was requested, but there are no \""},{"line_number":843,"context_line":"                         \"snapshots present to use as base, \""}],"source_content_type":"text/x-python","patch_set":11,"id":"7be38d4b_6c294b0b","line":840,"range":{"start_line":837,"start_character":1,"end_line":840,"end_character":29},"in_reply_to":"868daebe_517359aa","updated":"2024-04-16 11:07:03.000000000","message":"Done","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"10729f924fb11f226cddc17d76de46e0da0b30b7","unresolved":true,"context_lines":[{"line_number":894,"context_line":"            LOG.debug(\"Differential backup transfer completed in %.4fs\","},{"line_number":895,"context_line":"                      (time.time() - before))"},{"line_number":896,"context_line":""},{"line_number":897,"context_line":"            # only keep last n snapshots and delete older ones"},{"line_number":898,"context_line":"            if snaps_to_keep \u003e 0:"},{"line_number":899,"context_line":"                snap_list \u003d []"},{"line_number":900,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"9f51d572_5d828186","line":897,"updated":"2024-04-12 15:27:26.000000000","message":"This should be done asynchronously to the backup, because this can take a while to be completed, and until then the user will not see the backup as completed.\n\nAnd we have to be careful to preven race conditions with a possible backup while this is going on.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"2deb1763cc5726853297f6cf56a723d987abf4cd","unresolved":false,"context_lines":[{"line_number":894,"context_line":"            LOG.debug(\"Differential backup transfer completed in %.4fs\","},{"line_number":895,"context_line":"                      (time.time() - before))"},{"line_number":896,"context_line":""},{"line_number":897,"context_line":"            # only keep last n snapshots and delete older ones"},{"line_number":898,"context_line":"            if snaps_to_keep \u003e 0:"},{"line_number":899,"context_line":"                snap_list \u003d []"},{"line_number":900,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"87aa4e9f_1cab9505","line":897,"in_reply_to":"9f51d572_5d828186","updated":"2024-04-15 15:12:04.000000000","message":"As agreed during the PTG, async can be added if this really proves to be an issue.\nAccording to https://docs.ceph.com/en/reef/rbd/rbd-snapshot/#delete-a-snapshot the \"rbd snap rm\" actually only queues a snapshot for deletion. The \"snaptrim\", so the deletion of the individual RADOS objects will be done in the background anyways.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":false,"context_lines":[{"line_number":894,"context_line":"            LOG.debug(\"Differential backup transfer completed in %.4fs\","},{"line_number":895,"context_line":"                      (time.time() - before))"},{"line_number":896,"context_line":""},{"line_number":897,"context_line":"            # only keep last n snapshots and delete older ones"},{"line_number":898,"context_line":"            if snaps_to_keep \u003e 0:"},{"line_number":899,"context_line":"                snap_list \u003d []"},{"line_number":900,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"5ae9d1df_854e281d","line":897,"in_reply_to":"9f51d572_5d828186","updated":"2024-05-17 15:43:39.000000000","message":"Probably not for now, only if customers complain there is actually a problem, since doing it async is considerably more complex.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"8fe34e1c72bd1a225b2e2f343aacb07c5ee39950","unresolved":true,"context_lines":[{"line_number":919,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":920,"context_line":""},{"line_number":921,"context_line":"                        try:"},{"line_number":922,"context_line":"                            source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":923,"context_line":"                        except Exception as e:"},{"line_number":924,"context_line":"                            LOG.debug(\"Failed to delete snapshot %s: %s\","},{"line_number":925,"context_line":"                                      snap_list[i], e)"}],"source_content_type":"text/x-python","patch_set":11,"id":"9082465e_8d4beb9d","line":922,"updated":"2024-04-12 15:54:29.000000000","message":"Is there a known contract were snapshots are returned in the oldest to newest order?","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"f0ffd2f28800e91b4a59da1b36b3d3839abf66cb","unresolved":true,"context_lines":[{"line_number":919,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":920,"context_line":""},{"line_number":921,"context_line":"                        try:"},{"line_number":922,"context_line":"                            source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":923,"context_line":"                        except Exception as e:"},{"line_number":924,"context_line":"                            LOG.debug(\"Failed to delete snapshot %s: %s\","},{"line_number":925,"context_line":"                                      snap_list[i], e)"}],"source_content_type":"text/x-python","patch_set":11,"id":"c1b3b516_7ac268b1","line":922,"in_reply_to":"115b7abd_5f72598f","updated":"2024-04-24 11:53:42.000000000","message":"Just randomly I was interested in this as well, tested with rados/rbd python bindings and it does return an ascending ordered list by \"snap ID\" which is an integer that increments for each snapshot.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":34149,"name":"Niklas Schwarz","email":"niklas.schwarz@inovex.de","username":"nschwarz"},"change_message_id":"f1a9aed4f324ecfa84bcba83114ba39c903c7399","unresolved":true,"context_lines":[{"line_number":919,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":920,"context_line":""},{"line_number":921,"context_line":"                        try:"},{"line_number":922,"context_line":"                            source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":923,"context_line":"                        except Exception as e:"},{"line_number":924,"context_line":"                            LOG.debug(\"Failed to delete snapshot %s: %s\","},{"line_number":925,"context_line":"                                      snap_list[i], e)"}],"source_content_type":"text/x-python","patch_set":11,"id":"115b7abd_5f72598f","line":922,"in_reply_to":"9082465e_8d4beb9d","updated":"2024-04-16 11:07:03.000000000","message":"I have not found any contract regards rbd snapshots, but there is a documentation for cephFS (https://docs.ceph.com/en/quincy/dev/cephfs-snapshots/#generating-a-snapcontext) which describes the the name as a sequence. It might be similar.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"a312f44b523dd7f395d35083549fd7537d95a496","unresolved":false,"context_lines":[{"line_number":919,"context_line":"                        LOG.debug(\"Deleting snapshot %s\", snap_list[i])"},{"line_number":920,"context_line":""},{"line_number":921,"context_line":"                        try:"},{"line_number":922,"context_line":"                            source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":923,"context_line":"                        except Exception as e:"},{"line_number":924,"context_line":"                            LOG.debug(\"Failed to delete snapshot %s: %s\","},{"line_number":925,"context_line":"                                      snap_list[i], e)"}],"source_content_type":"text/x-python","patch_set":11,"id":"9e7ae830_015d183a","line":922,"in_reply_to":"c1b3b516_7ac268b1","updated":"2024-04-24 12:01:56.000000000","message":"Done","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7c6a40b6afa1399b5ab84ef1c83d3f6a0787b6f2","unresolved":true,"context_lines":[{"line_number":102,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":103,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited amount of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":108,"context_line":"                    limited number of snapshots on the source volume storage."}],"source_content_type":"text/x-python","patch_set":12,"id":"5d30a12b_179a9627","line":105,"range":{"start_line":105,"start_character":53,"end_line":105,"end_character":59},"updated":"2024-04-16 15:00:53.000000000","message":"nit: \"number\"","commit_id":"f938e5c0c74383dd99ec513049b208f0226273d7"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d2d0027e3182f69e26ffcae16c732f2c4a96af0e","unresolved":false,"context_lines":[{"line_number":102,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":103,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited amount of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":108,"context_line":"                    limited number of snapshots on the source volume storage."}],"source_content_type":"text/x-python","patch_set":12,"id":"5fa051fa_a878d43f","line":105,"range":{"start_line":105,"start_character":53,"end_line":105,"end_character":59},"in_reply_to":"5d30a12b_179a9627","updated":"2024-04-17 13:03:38.000000000","message":"Acknowledged","commit_id":"f938e5c0c74383dd99ec513049b208f0226273d7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"7c6a40b6afa1399b5ab84ef1c83d3f6a0787b6f2","unresolved":true,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited amount of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":108,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":109,"context_line":"                    However, if a user deletes the available incremental"},{"line_number":110,"context_line":"                    backups which still have snapshots on source side, the next"}],"source_content_type":"text/x-python","patch_set":12,"id":"cd558e31_de7c4868","line":107,"range":{"start_line":107,"start_character":20,"end_line":107,"end_character":28},"updated":"2024-04-16 15:00:53.000000000","message":"nit: \"Configuring this option...\" (\"Enabling\" makes is sound like a boolean).","commit_id":"f938e5c0c74383dd99ec513049b208f0226273d7"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d2d0027e3182f69e26ffcae16c732f2c4a96af0e","unresolved":false,"context_lines":[{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited amount of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Enabling this option can save disk space by only keeping a"},{"line_number":108,"context_line":"                    limited number of snapshots on the source volume storage."},{"line_number":109,"context_line":"                    However, if a user deletes the available incremental"},{"line_number":110,"context_line":"                    backups which still have snapshots on source side, the next"}],"source_content_type":"text/x-python","patch_set":12,"id":"ffffdf89_b20c0acd","line":107,"range":{"start_line":107,"start_character":20,"end_line":107,"end_character":28},"in_reply_to":"cd558e31_de7c4868","updated":"2024-04-17 13:03:38.000000000","message":"Acknowledged","commit_id":"f938e5c0c74383dd99ec513049b208f0226273d7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"5550698d300a11f5d20444fd6807c6d77db0ba9a","unresolved":true,"context_lines":[{"line_number":102,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":103,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited number of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Configuring this option can save disk space by only keeping"},{"line_number":108,"context_line":"                    a limited number of snapshots on the source volume storage."}],"source_content_type":"text/x-python","patch_set":13,"id":"e4e73316_04b2a3e6","line":105,"updated":"2024-04-22 16:34:48.000000000","message":"Having read the documentation and release note, it may be better to use -1 to indicate keeping an unlimited number. This is because a \"max of 0\" could easily be interpreted to mean \"don\u0027t keep any snaps\" which is the direct opposite \"keep an unlimited number of them.\"\n\nBut this raises the question whether \"don\u0027t keep any snapshots\" is a meaningful use case. Maybe? Perhaps it means \"always do full backups,\" which is wasteful but perhaps is what a cloud admin might want.","commit_id":"e02cdf1a907fd913e69ba19a6e53b45d00eb21e3"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"ce3a62f6006dd462ed2b03d2839a7159c0488706","unresolved":true,"context_lines":[{"line_number":102,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":103,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited number of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Configuring this option can save disk space by only keeping"},{"line_number":108,"context_line":"                    a limited number of snapshots on the source volume storage."}],"source_content_type":"text/x-python","patch_set":13,"id":"fc9d7880_c86558ee","line":105,"in_reply_to":"e4e73316_04b2a3e6","updated":"2024-04-22 19:07:44.000000000","message":"Thanks for your continuous review and feedback!\n\nIt\u0027s fair to discuss which value stands for \"unlimited\" - but this has been done for this very change is the past. Please see https://review.opendev.org/c/openstack/cinder/+/810457/comment/920c6196_4ebe79b8/\n\nThere is not objection to change this once again, but apparently there is no common perception which value is the correct one?\n\nKeeping no snapshots makes any incremental backups impossible.","commit_id":"e02cdf1a907fd913e69ba19a6e53b45d00eb21e3"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"083fdf41b7da0a75bf5943e52524d9326bff8047","unresolved":true,"context_lines":[{"line_number":102,"context_line":"               help\u003dtextwrap.dedent(\"\"\"\\"},{"line_number":103,"context_line":"                    Number of the most recent snapshots to keep."},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"                    0 indicates to keep an unlimited number of snapshots."},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"                    Configuring this option can save disk space by only keeping"},{"line_number":108,"context_line":"                    a limited number of snapshots on the source volume storage."}],"source_content_type":"text/x-python","patch_set":13,"id":"6afa62ee_8442f609","line":105,"in_reply_to":"fc9d7880_c86558ee","updated":"2024-04-23 15:58:25.000000000","message":"Thank you for sharing the background on why we\u0027re using 0 to mean unlimited. It seems that other people agree it may be confusing, but it\u0027s also important to be consistent with other cinder options that use 0 to mean unlimited.","commit_id":"e02cdf1a907fd913e69ba19a6e53b45d00eb21e3"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"125f3d2a08d17f715ea8d2ec40f5198849830112","unresolved":true,"context_lines":[{"line_number":934,"context_line":"        try:"},{"line_number":935,"context_line":"            snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":936,"context_line":"        except Exception as e:"},{"line_number":937,"context_line":"            LOG.debug("},{"line_number":938,"context_line":"                \"Failed to get snapshot list for %s: %s\", source_rbd_image, e"},{"line_number":939,"context_line":"            )"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"850899db_8113424e","line":937,"range":{"start_line":937,"start_character":12,"end_line":937,"end_character":21},"updated":"2024-04-24 13:38:59.000000000","message":"As an operator I would like this to be a warning, since it failed as well we can just return after that","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":true,"context_lines":[{"line_number":934,"context_line":"        try:"},{"line_number":935,"context_line":"            snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":936,"context_line":"        except Exception as e:"},{"line_number":937,"context_line":"            LOG.debug("},{"line_number":938,"context_line":"                \"Failed to get snapshot list for %s: %s\", source_rbd_image, e"},{"line_number":939,"context_line":"            )"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"b71ea616_4ba37359","line":937,"updated":"2024-05-17 15:43:39.000000000","message":"Yes this definitely needs to be a warning!\nI\u0027m ok if it\u0027s done in a follow up patch so we can finally merge this one.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d4c90b49a1a06a68005fbcfda1d4826481925830","unresolved":false,"context_lines":[{"line_number":934,"context_line":"        try:"},{"line_number":935,"context_line":"            snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":936,"context_line":"        except Exception as e:"},{"line_number":937,"context_line":"            LOG.debug("},{"line_number":938,"context_line":"                \"Failed to get snapshot list for %s: %s\", source_rbd_image, e"},{"line_number":939,"context_line":"            )"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"a81c0a9c_62f6265f","line":937,"range":{"start_line":937,"start_character":12,"end_line":937,"end_character":21},"in_reply_to":"850899db_8113424e","updated":"2024-04-24 14:00:34.000000000","message":"Acknowledged","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":597,"name":"Pete Zaitcev","email":"zaitcev@kotori.zaitcev.us","username":"zaitcev"},"change_message_id":"3477d59e8b164a7e83361d31ddc6820bd29c3019","unresolved":true,"context_lines":[{"line_number":934,"context_line":"        try:"},{"line_number":935,"context_line":"            snap_list \u003d self.get_backup_snaps(source_rbd_image)"},{"line_number":936,"context_line":"        except Exception as e:"},{"line_number":937,"context_line":"            LOG.debug("},{"line_number":938,"context_line":"                \"Failed to get snapshot list for %s: %s\", source_rbd_image, e"},{"line_number":939,"context_line":"            )"},{"line_number":940,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"0eca57a9_ab8ef7c5","line":937,"in_reply_to":"b71ea616_4ba37359","updated":"2024-05-17 16:32:21.000000000","message":"In addition I hate \"except Exception\" so much.\n\nIn Swift, there\u0027s now a piece of code that makes exceptions that inherits from BaseException, just to cut through the middleware layers where except Exeception may exist.\n\nBut oh well. I want this merged.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":16137,"name":"Tobias Urdin","email":"tobias.urdin@binero.com","username":"tobasco"},"change_message_id":"125f3d2a08d17f715ea8d2ec40f5198849830112","unresolved":true,"context_lines":[{"line_number":957,"context_line":"                try:"},{"line_number":958,"context_line":"                    source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":959,"context_line":"                except Exception as e:"},{"line_number":960,"context_line":"                    LOG.debug("},{"line_number":961,"context_line":"                        \"Failed to delete snapshot %s: %s\", snap_list[i], e"},{"line_number":962,"context_line":"                    )"},{"line_number":963,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":15,"id":"31168eb3_d8a4b76e","line":960,"range":{"start_line":960,"start_character":20,"end_line":960,"end_character":29},"updated":"2024-04-24 13:38:59.000000000","message":"Same, I would like to know that this failed with a warning instead","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":true,"context_lines":[{"line_number":957,"context_line":"                try:"},{"line_number":958,"context_line":"                    source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":959,"context_line":"                except Exception as e:"},{"line_number":960,"context_line":"                    LOG.debug("},{"line_number":961,"context_line":"                        \"Failed to delete snapshot %s: %s\", snap_list[i], e"},{"line_number":962,"context_line":"                    )"},{"line_number":963,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":15,"id":"8d8feccc_cdd11244","line":960,"updated":"2024-05-17 15:43:39.000000000","message":"Yes this definitely needs to be a warning!\nI\u0027m ok if it\u0027s done in a follow up patch so we can finally merge this one.","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"d4c90b49a1a06a68005fbcfda1d4826481925830","unresolved":false,"context_lines":[{"line_number":957,"context_line":"                try:"},{"line_number":958,"context_line":"                    source_rbd_image.remove_snap(snap_list[i][\"name\"])"},{"line_number":959,"context_line":"                except Exception as e:"},{"line_number":960,"context_line":"                    LOG.debug("},{"line_number":961,"context_line":"                        \"Failed to delete snapshot %s: %s\", snap_list[i], e"},{"line_number":962,"context_line":"                    )"},{"line_number":963,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":15,"id":"1408cce8_c4b2e64c","line":960,"range":{"start_line":960,"start_character":20,"end_line":960,"end_character":29},"in_reply_to":"31168eb3_d8a4b76e","updated":"2024-04-24 14:00:34.000000000","message":"Acknowledged","commit_id":"03cbcd3cd0d7f0f9da748d0f113df7d26741ba21"}],"doc/source/configuration/block-storage/backup/ceph-backup-driver.rst":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":16,"context_line":"instance. If the differential backup fails, the driver falls back to"},{"line_number":17,"context_line":"full backup/copy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"If incremental backups are used, multiple backups of the same volume are"},{"line_number":20,"context_line":"stored as snapshots so that minimal space is consumed in the backup"},{"line_number":21,"context_line":"store. It takes far less time to restore a volume than to take a full"},{"line_number":22,"context_line":"copy."}],"source_content_type":"text/x-rst","patch_set":6,"id":"89e74eea_29aa4ef6","line":19,"updated":"2022-01-31 10:07:59.000000000","message":"-1: This new feature needs a proper explanation with the problem, the solution, and the tradeoff.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":16,"context_line":"instance. If the differential backup fails, the driver falls back to"},{"line_number":17,"context_line":"full backup/copy."},{"line_number":18,"context_line":""},{"line_number":19,"context_line":"If incremental backups are used, multiple backups of the same volume are"},{"line_number":20,"context_line":"stored as snapshots so that minimal space is consumed in the backup"},{"line_number":21,"context_line":"store. It takes far less time to restore a volume than to take a full"},{"line_number":22,"context_line":"copy."}],"source_content_type":"text/x-rst","patch_set":6,"id":"c73442f0_95423070","line":19,"in_reply_to":"89e74eea_29aa4ef6","updated":"2022-02-22 15:29:20.000000000","message":"Ack","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@purestorage.com","username":"sdodsley"},"change_message_id":"c16033593d3fd91101e4dc2539fe81763c27ed7c","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    backup_ceph_pool \u003d backups"},{"line_number":58,"context_line":"    backup_ceph_stripe_unit \u003d 0"},{"line_number":59,"context_line":"    backup_ceph_stripe_count \u003d 0"},{"line_number":60,"context_line":"    backup_ceph_keep_snapshots_count \u003d -1"}],"source_content_type":"text/x-rst","patch_set":6,"id":"8aa51857_acec65c9","line":60,"updated":"2022-01-26 13:56:03.000000000","message":"Why -1 and not 0?","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"22df598f77dea70d1a159bd3ebab616f459fce28","unresolved":true,"context_lines":[{"line_number":57,"context_line":"    backup_ceph_pool \u003d backups"},{"line_number":58,"context_line":"    backup_ceph_stripe_unit \u003d 0"},{"line_number":59,"context_line":"    backup_ceph_stripe_count \u003d 0"},{"line_number":60,"context_line":"    backup_ceph_keep_snapshots_count \u003d -1"}],"source_content_type":"text/x-rst","patch_set":6,"id":"91fab2eb_cdb8f8eb","line":60,"in_reply_to":"8aa51857_acec65c9","updated":"2022-01-26 14:09:26.000000000","message":"-1 \u003d unlimited (as it is until this config flag is introducted)\nKeeping only \"0\" snapshots is not a good default as it changes the behavior and consequently causes no incremental backups to ever be created as there is no source snapshot to start from that\u0027s kept.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":32755,"name":"Christian Rohmann","email":"christian.rohmann@inovex.de","username":"frittentheke"},"change_message_id":"561cdb05403dc42c29136ca402491be5c293c508","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    backup_ceph_pool \u003d backups"},{"line_number":58,"context_line":"    backup_ceph_stripe_unit \u003d 0"},{"line_number":59,"context_line":"    backup_ceph_stripe_count \u003d 0"},{"line_number":60,"context_line":"    backup_ceph_keep_snapshots_count \u003d -1"}],"source_content_type":"text/x-rst","patch_set":6,"id":"0c6239bd_b95a1260","line":60,"in_reply_to":"91fab2eb_cdb8f8eb","updated":"2024-04-15 15:13:13.000000000","message":"Done","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"}],"releasenotes/notes/ceph-add-option-to-keep-only-last-n-snapshots-89dc532656f453f4.yaml":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"d8ccd9901daaf50039b734704f6fa7dc31c87152","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Ceph driver: Added config option to keep only the last n snapshots per backup"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"e60ee60a_db25d143","line":4,"updated":"2022-01-31 10:07:59.000000000","message":"This needs to mention that this is to save space on the expensive volume pool, but that there are tradeoffs and to look at the doc.","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":33634,"name":"Jan Hartkopf","email":"j@hartkopf.io","username":"jhartkopf"},"change_message_id":"966d787a8b2cb4f6b4a8bef8f7a40893a475cc67","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Ceph driver: Added config option to keep only the last n snapshots per backup"}],"source_content_type":"text/x-yaml","patch_set":6,"id":"27ced945_0c6ffa30","line":4,"in_reply_to":"e60ee60a_db25d143","updated":"2022-02-22 15:29:20.000000000","message":"Ack","commit_id":"2175f851939813f022674a2bd13a4c1c84e28750"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"445e554b028c653fab69c40e4f60cbd3ecc4736e","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Ceph driver: Add config option to keep only the last n snapshots per backup"},{"line_number":5,"context_line":"    to save disk space on the source volume storage. Enabling this option can"},{"line_number":6,"context_line":"    cause incremental backups to become full backups instead under special"},{"line_number":7,"context_line":"    circumstances. Please take a look at the Ceph backup driver docs for"}],"source_content_type":"text/x-yaml","patch_set":11,"id":"5f580b45_7647041d","line":4,"updated":"2024-05-17 15:43:39.000000000","message":"I would normally downvote for not mentioning the name of the config option, but like my other comment, we can do that in a follow up patch so we can merge this one.","commit_id":"3d76a4a6dfd785355f31a29335716bb492ef2f0c"}]}