)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ca865fc520fd2bc58ea007990f034b973f6424ed","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"9ba3fb80_e77f7376","updated":"2021-12-07 19:16:16.000000000","message":"Some nits inline.  I\u0027d like you to consider configuring this on the Glance side instead of in Cinder (or explain why that\u0027s not a good idea).  Otherwise, I think the proposal is fine.","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"0b047be785defa32a173988320b6badeafde3708","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"d1a50c07_1c7e38ad","updated":"2021-11-02 05:09:19.000000000","message":"recheck","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2c16eb1f0da32039721e9a028ab58e3a1d8168fa","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"1d1fd8fb_f2e81890","in_reply_to":"9ba3fb80_e77f7376","updated":"2021-12-10 05:54:59.000000000","message":"Thanks for the review Brian. I understand your concern but please find my reason inline.","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":20813,"name":"Sofia Enriquez","email":"lsofia.enriquez@gmail.com","username":"enriquetaso"},"change_message_id":"59373ddeb71385e40b0f093d4ae942821d05484f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"cbba6adc_bf65e933","updated":"2021-12-21 14:40:53.000000000","message":"Eric comments were addressed and Glance team is fine with this approach. Looks good to me, nice optimization to have!","commit_id":"ef9f435679975596dbbe3ee192651f501040c5c1"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"73606e5134546b2af68042acca726dbeaf15b26e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"ddafe9ff_57d34dff","updated":"2021-12-21 13:51:46.000000000","message":"Revisions look good to me, and Rajat has explained why my proposal for controlling whether the optimized path is used or not can\u0027t really be controlled on the Glance side.  LGTM.","commit_id":"ef9f435679975596dbbe3ee192651f501040c5c1"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"0e25749150c09aabf0e50d6507fe60661328845a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"bed05d23_3c3ccbf9","updated":"2021-12-16 10:49:10.000000000","message":"Thanks for the review Eric.","commit_id":"ef9f435679975596dbbe3ee192651f501040c5c1"}],"specs/yoga/optimize-upload-volume-to-rbd-store.rst":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ca865fc520fd2bc58ea007990f034b973f6424ed","unresolved":true,"context_lines":[{"line_number":39,"context_line":"in this case) via stores info API [2]_."},{"line_number":40,"context_line":"This can be extended to other stores by providing store specific details with"},{"line_number":41,"context_line":"the stores info API like we do with get pools API [3]_ on cinder side."},{"line_number":42,"context_line":"Further information regarding it\u0027s implementation can be provided with a"},{"line_number":43,"context_line":"glance side spec."},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"2) Cinder"}],"source_content_type":"text/x-rst","patch_set":7,"id":"aa59e0b6_40c0f216","line":42,"range":{"start_line":42,"start_character":30,"end_line":42,"end_character":34},"updated":"2021-12-07 19:16:16.000000000","message":"nit: its","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2c16eb1f0da32039721e9a028ab58e3a1d8168fa","unresolved":false,"context_lines":[{"line_number":39,"context_line":"in this case) via stores info API [2]_."},{"line_number":40,"context_line":"This can be extended to other stores by providing store specific details with"},{"line_number":41,"context_line":"the stores info API like we do with get pools API [3]_ on cinder side."},{"line_number":42,"context_line":"Further information regarding it\u0027s implementation can be provided with a"},{"line_number":43,"context_line":"glance side spec."},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"2) Cinder"}],"source_content_type":"text/x-rst","patch_set":7,"id":"ae88c694_cff03202","line":42,"range":{"start_line":42,"start_character":30,"end_line":42,"end_character":34},"in_reply_to":"aa59e0b6_40c0f216","updated":"2021-12-10 05:54:59.000000000","message":"Done","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ca865fc520fd2bc58ea007990f034b973f6424ed","unresolved":true,"context_lines":[{"line_number":49,"context_line":"from the volume pool to the images pool (pool name provided by glance) and"},{"line_number":50,"context_line":"register the image location on glance side."},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"We will also introduce a new config parameter ``enable_clone_optimization`` to"},{"line_number":53,"context_line":"enable/disable this optimization on cinder side. More information about it"},{"line_number":54,"context_line":"can be found in `Security impact`_ section."},{"line_number":55,"context_line":""}],"source_content_type":"text/x-rst","patch_set":7,"id":"24f05fda_849b0a04","line":52,"range":{"start_line":52,"start_character":48,"end_line":52,"end_character":73},"updated":"2021-12-07 19:16:16.000000000","message":"An alternative would be to design the Glance feature so that the store info you need isn\u0027t exposed if the operator is worried about the security impact.  I think it needs to be handled on the Glance side, because from the cinder point of view, given the performance data you cite below, the cinder operator will definitely want to have this feature enabled.","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2c16eb1f0da32039721e9a028ab58e3a1d8168fa","unresolved":true,"context_lines":[{"line_number":49,"context_line":"from the volume pool to the images pool (pool name provided by glance) and"},{"line_number":50,"context_line":"register the image location on glance side."},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"We will also introduce a new config parameter ``enable_clone_optimization`` to"},{"line_number":53,"context_line":"enable/disable this optimization on cinder side. More information about it"},{"line_number":54,"context_line":"can be found in `Security impact`_ section."},{"line_number":55,"context_line":""}],"source_content_type":"text/x-rst","patch_set":7,"id":"af3f562e_d7031097","line":52,"range":{"start_line":52,"start_character":48,"end_line":52,"end_character":73},"in_reply_to":"24f05fda_849b0a04","updated":"2021-12-10 05:54:59.000000000","message":"I understand the reasoning and need for glance to provide confirmation and take part in this operation but my only concern is there is no good place to put the check throughout the flow of this operation.\nThere are two calls to glance and I don\u0027t find either to be suitable for this check for the following reasons:\n\n1) calling the stores detail API[1]: As mentioned in the glance spec[2], this sounds good for this particular usecase to add the check here but what happens when we extend this API for other stores or use the rbd store properties for any other purpose than this optimization? This makes the new API restricted to this optimization only which is not the intention and doesn\u0027t seem right to me.\n\n2) registering the location[3]: During the time this is called, the image clone is already created so if the glance check fails, we have to revert the whole process and i think the check only makes sense at the start of operation to avoid all this cleanup.\n\nRegarding the cinder operator using this config option for performance benefit, we can explain the security impact in the description of this new config option so they just don\u0027t go with performance but also understand the security risks of enabling it.\nThat seems to be the only available option to me, what do you think?\n\n[1] https://review.opendev.org/c/openstack/cinder/+/809523/4/cinder/volume/drivers/rbd.py#1712\n[2] https://review.opendev.org/c/openstack/glance-specs/+/817391/comment/f24b209a_351da811/\n[3] https://review.opendev.org/c/openstack/cinder/+/809523/4/cinder/volume/drivers/rbd.py#1733","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"73606e5134546b2af68042acca726dbeaf15b26e","unresolved":false,"context_lines":[{"line_number":49,"context_line":"from the volume pool to the images pool (pool name provided by glance) and"},{"line_number":50,"context_line":"register the image location on glance side."},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"We will also introduce a new config parameter ``enable_clone_optimization`` to"},{"line_number":53,"context_line":"enable/disable this optimization on cinder side. More information about it"},{"line_number":54,"context_line":"can be found in `Security impact`_ section."},{"line_number":55,"context_line":""}],"source_content_type":"text/x-rst","patch_set":7,"id":"c64717d4_c81a50be","line":52,"range":{"start_line":52,"start_character":48,"end_line":52,"end_character":73},"in_reply_to":"af3f562e_d7031097","updated":"2021-12-21 13:51:46.000000000","message":"Thanks for the detailed response.  Your point #1 is very convincing.  Plus, from the discussion on the Glance spec, the Glance team is fine with this approach.  So I withdraw my objection.","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ca865fc520fd2bc58ea007990f034b973f6424ed","unresolved":true,"context_lines":[{"line_number":75,"context_line":"also skip the checksum and hash value calculated in that scenario."},{"line_number":76,"context_line":"We will introduce a new config parameter ``enable_clone_optimization`` to enable/disable"},{"line_number":77,"context_line":"this optimization and also mention the security risks that comes with enabling it. By default,"},{"line_number":78,"context_line":"It will be disabled."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"Active/Active HA impact"},{"line_number":81,"context_line":"-----------------------"}],"source_content_type":"text/x-rst","patch_set":7,"id":"a2a21b4d_45372bfb","line":78,"range":{"start_line":78,"start_character":0,"end_line":78,"end_character":2},"updated":"2021-12-07 19:16:16.000000000","message":"nit: it\n\nAlso, see comment above about configuring this on the glance side.","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2c16eb1f0da32039721e9a028ab58e3a1d8168fa","unresolved":false,"context_lines":[{"line_number":75,"context_line":"also skip the checksum and hash value calculated in that scenario."},{"line_number":76,"context_line":"We will introduce a new config parameter ``enable_clone_optimization`` to enable/disable"},{"line_number":77,"context_line":"this optimization and also mention the security risks that comes with enabling it. By default,"},{"line_number":78,"context_line":"It will be disabled."},{"line_number":79,"context_line":""},{"line_number":80,"context_line":"Active/Active HA impact"},{"line_number":81,"context_line":"-----------------------"}],"source_content_type":"text/x-rst","patch_set":7,"id":"d3728a88_2f802d4e","line":78,"range":{"start_line":78,"start_character":0,"end_line":78,"end_character":2},"in_reply_to":"a2a21b4d_45372bfb","updated":"2021-12-10 05:54:59.000000000","message":"Done","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"ca865fc520fd2bc58ea007990f034b973f6424ed","unresolved":true,"context_lines":[{"line_number":95,"context_line":"Performance Impact"},{"line_number":96,"context_line":"------------------"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"Uploading volume to image incase of cinder RBD to glance RBD will be"},{"line_number":99,"context_line":"significantly improved."},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"+------------------------------------+---------------+---------------+---------------+"}],"source_content_type":"text/x-rst","patch_set":7,"id":"3e8dcf3f_0dc6a0f5","line":98,"range":{"start_line":98,"start_character":26,"end_line":98,"end_character":32},"updated":"2021-12-07 19:16:16.000000000","message":"nit: in case","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"2c16eb1f0da32039721e9a028ab58e3a1d8168fa","unresolved":false,"context_lines":[{"line_number":95,"context_line":"Performance Impact"},{"line_number":96,"context_line":"------------------"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"Uploading volume to image incase of cinder RBD to glance RBD will be"},{"line_number":99,"context_line":"significantly improved."},{"line_number":100,"context_line":""},{"line_number":101,"context_line":"+------------------------------------+---------------+---------------+---------------+"}],"source_content_type":"text/x-rst","patch_set":7,"id":"100e258e_fee6c70c","line":98,"range":{"start_line":98,"start_character":26,"end_line":98,"end_character":32},"in_reply_to":"3e8dcf3f_0dc6a0f5","updated":"2021-12-10 05:54:59.000000000","message":"Done","commit_id":"06461317ebe0799325a8ad0217c2232f5d7e9f23"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"b45c8d87178be94f7ef85d94e3513589139f46af","unresolved":true,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"Other deployer impact"},{"line_number":113,"context_line":"---------------------"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"None"},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"Developer impact"}],"source_content_type":"text/x-rst","patch_set":9,"id":"63353a64_686acbab","line":114,"updated":"2021-12-15 14:47:00.000000000","message":"The Cinder service user must now have write access to the Glance pool, when previously it only needed read access, if I understand this correctly.","commit_id":"4d396e2a4087232ad5a5a1bd2008bf3a54006822"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"0e25749150c09aabf0e50d6507fe60661328845a","unresolved":false,"context_lines":[{"line_number":111,"context_line":""},{"line_number":112,"context_line":"Other deployer impact"},{"line_number":113,"context_line":"---------------------"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"None"},{"line_number":116,"context_line":""},{"line_number":117,"context_line":"Developer impact"}],"source_content_type":"text/x-rst","patch_set":9,"id":"d2a03c0b_09ed389c","line":114,"in_reply_to":"63353a64_686acbab","updated":"2021-12-16 10:49:10.000000000","message":"Done","commit_id":"4d396e2a4087232ad5a5a1bd2008bf3a54006822"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"f2615f7f6b47d3992b889b8c1d04ef5e4376b2fc","unresolved":true,"context_lines":[{"line_number":165,"context_line":"References"},{"line_number":166,"context_line":"----------"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":".. [1] https://github.com/openstack/cinder/commit/edc11101cbc06bdce95b10cfd00a4849f6c01b33"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":".. [2] https://docs.openstack.org/api-ref/image/v2/index.html?expanded\u003dlist-stores-detail#list-stores"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-rst","patch_set":9,"id":"3890a1e2_bb752f5f","line":168,"range":{"start_line":168,"start_character":6,"end_line":168,"end_character":7},"updated":"2021-12-15 15:01:51.000000000","message":"It would be better to avoid using github links for permanent references in specs...\n    https://opendev.org/openstack/cinder/commit/edc11101cbc06bdce95b10cfd00a4849f6c01b33","commit_id":"4d396e2a4087232ad5a5a1bd2008bf3a54006822"},{"author":{"_account_id":27615,"name":"Rajat Dhasmana","email":"rajatdhasmana@gmail.com","username":"whoami-rajat"},"change_message_id":"0e25749150c09aabf0e50d6507fe60661328845a","unresolved":false,"context_lines":[{"line_number":165,"context_line":"References"},{"line_number":166,"context_line":"----------"},{"line_number":167,"context_line":""},{"line_number":168,"context_line":".. [1] https://github.com/openstack/cinder/commit/edc11101cbc06bdce95b10cfd00a4849f6c01b33"},{"line_number":169,"context_line":""},{"line_number":170,"context_line":".. [2] https://docs.openstack.org/api-ref/image/v2/index.html?expanded\u003dlist-stores-detail#list-stores"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-rst","patch_set":9,"id":"939f9183_5bbfe430","line":168,"range":{"start_line":168,"start_character":6,"end_line":168,"end_character":7},"in_reply_to":"3890a1e2_bb752f5f","updated":"2021-12-16 10:49:10.000000000","message":"Done","commit_id":"4d396e2a4087232ad5a5a1bd2008bf3a54006822"}]}