)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"0b5da51b98ba6ec6876c3725ba275a04445ccf5e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"99cceddf_901db488","updated":"2025-06-11 07:22:16.000000000","message":"I wonder if we should make src_type parameter optional and keep the existing behavior when no value is provided for it. Note that making the parameter optional would be backwards compatible, so I think we would only have to document it.\n\nOther than that lgtm","commit_id":"fe5b9fdb4d3ded82fd5c6a6cd464703fcdba61a5"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"a17b07c5008d7b50eb169b64f69ddd1cca1bec80","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"fa3daf04_76c96c14","in_reply_to":"99cceddf_901db488","updated":"2025-06-11 12:53:10.000000000","message":"that is a very good point, I\u0027ll give it a try in the next patchset and then we can make a decision","commit_id":"fe5b9fdb4d3ded82fd5c6a6cd464703fcdba61a5"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"c6c5d0cc6506abc98c66ebd24e2c5087a30395f3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"7242f052_60845394","updated":"2025-06-20 09:52:19.000000000","message":"I think this deserves a release note.\n\nI think in addition to the tests you added, which are good, it\u0027d be good to test the entire strategy execution with different options and combinations, and checking the provided solution. It already has some, but i think we may do those more complex.","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"6ef69cd94767f26c5ef39e27e5177a283d7ba734","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"fe94c88f_2da58054","updated":"2025-06-20 08:44:22.000000000","message":"check-rdo","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5c57a7b1d612a50dd9be3fb2d6c25351148e3c6a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"99d8c822_52c40b38","in_reply_to":"7242f052_60845394","updated":"2025-06-25 12:57:46.000000000","message":"fair point about the release note and testing, I\u0027ve added bot in patchset 4","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4499604cc7d5b730e5921bbd78dd5422b184383d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"b8754e34_51191d6b","updated":"2025-09-02 11:35:24.000000000","message":"-1: is just for the backslash thing, but otherwise the fix looks good.","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"fa5b1874e689d0a37725d13754c0784ce455a776","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"b86d0664_e47834bf","updated":"2025-07-18 13:21:37.000000000","message":"recheck","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"824bec8271b5befe24e7526e35d7b43b8410d3fb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"737ca14e_6a861346","updated":"2025-09-09 13:23:20.000000000","message":"lgtm, thanks Joan","commit_id":"77ed67928b758fa48bbbcf4f7d398c33f33d50b3"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"88f5db293af5ce4ace19e1ecc53d023c9dee1301","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"6e31a893_49998ac3","updated":"2025-09-15 12:41:23.000000000","message":"lgtm","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3970f8a4a9198d63b1d8af336ff24fd2abd4c8a4","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"a428cced_daa742dc","updated":"2025-09-15 17:34:29.000000000","message":"while you have added a lot of unit test the over all quality fo them is not great\nmainly it not explicitly clear why the shoudl hold true.\n\nthat a genera problem with these tests too however so im not going to hold the patch on that but i think e need to think how we can test this better with less mocking and more clarity in the test cases.\n\ni don\u0027t really have high confidence that we will be able to easily debug these in the further. in general i don\u0027t think the comments in the test are actually adding value because they describe what is done not why.\n\nill hold +w for a day or so in case anyone has an idea of how to improve ti but ill add it  tomorrow or Wednesday if there are no other comments.","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"cb97b4383f3b36fbb80a3ede6d221690fcdee85a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"13bd9475_a9e8b78a","in_reply_to":"2bdd1ac4_6a81b1bf","updated":"2025-09-16 17:19:06.000000000","message":"ack ok that works for me.","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"364ce29814ac84da307cf06e7a30344205f7c360","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":9,"id":"2bdd1ac4_6a81b1bf","in_reply_to":"a428cced_daa742dc","updated":"2025-09-16 09:11:31.000000000","message":"I have a patch for improving this unit tests https://review.opendev.org/c/openstack/watcher/+/954350/2 that is rebase on top of this series. I can address the issues you\u0027ve pointed out in that one","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"}],"watcher/decision_engine/strategy/strategies/zone_migration.py":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"c6c5d0cc6506abc98c66ebd24e2c5087a30395f3","unresolved":true,"context_lines":[{"line_number":543,"context_line":"            return src_type is None or \\"},{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"}],"source_content_type":"text/x-python","patch_set":2,"id":"72b71e5e_1eeb1a2a","line":546,"range":{"start_line":546,"start_character":18,"end_line":546,"end_character":47},"updated":"2025-06-20 09:52:19.000000000","message":"this is doing a call to cinder service, shouldn\u0027t we use the list of volumes in the model instead? that can lead to a lack of sync issues and making extra load cinder service which is not needed.","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5c57a7b1d612a50dd9be3fb2d6c25351148e3c6a","unresolved":false,"context_lines":[{"line_number":543,"context_line":"            return src_type is None or \\"},{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3c532419_ebc23d61","line":546,"range":{"start_line":546,"start_character":18,"end_line":546,"end_character":47},"in_reply_to":"4aca273b_3e219dfc","updated":"2025-06-25 12:57:46.000000000","message":"Done","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"b3804e81bdd99e7cc5fb345a3f5891c0a02522ed","unresolved":true,"context_lines":[{"line_number":543,"context_line":"            return src_type is None or \\"},{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"}],"source_content_type":"text/x-python","patch_set":2,"id":"4aca273b_3e219dfc","line":546,"range":{"start_line":546,"start_character":18,"end_line":546,"end_character":47},"in_reply_to":"72b71e5e_1eeb1a2a","updated":"2025-06-20 11:10:10.000000000","message":"good point, I used the same cinder calls as we had before the patch, but we could use the opportunity here to use better the model, I\u0027ll look into it","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"c6c5d0cc6506abc98c66ebd24e2c5087a30395f3","unresolved":true,"context_lines":[{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":550,"context_line":"            src_type \u003d migrate_input.get(\"src_type\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"f3bf51f1_f55dace0","line":547,"range":{"start_line":547,"start_character":8,"end_line":547,"end_character":27},"updated":"2025-06-20 09:52:19.000000000","message":"wouldn\u0027t be easier to create a list here and just add volumes in line 555?. In this implementation you are creating a dict to extract the values into a list later.","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"a200b73957719fe04202f70f6c5c53c3ba59d744","unresolved":true,"context_lines":[{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":550,"context_line":"            src_type \u003d migrate_input.get(\"src_type\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"ccf5b7f7_801cb3c1","line":547,"range":{"start_line":547,"start_character":8,"end_line":547,"end_character":27},"in_reply_to":"10f3a5d6_8dacf5b4","updated":"2025-06-20 11:18:26.000000000","message":"right, we may have the same src_pool with different src_types. You may also use a set, but this will work, anyway.","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5c57a7b1d612a50dd9be3fb2d6c25351148e3c6a","unresolved":false,"context_lines":[{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":550,"context_line":"            src_type \u003d migrate_input.get(\"src_type\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"3acab09d_162047a5","line":547,"range":{"start_line":547,"start_character":8,"end_line":547,"end_character":27},"in_reply_to":"1ad4ff54_b2774530","updated":"2025-06-25 12:57:46.000000000","message":"Done","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5a078730f9685fa00c303b20c88d817e44d7327d","unresolved":true,"context_lines":[{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":550,"context_line":"            src_type \u003d migrate_input.get(\"src_type\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"1ad4ff54_b2774530","line":547,"range":{"start_line":547,"start_character":8,"end_line":547,"end_character":27},"in_reply_to":"ccf5b7f7_801cb3c1","updated":"2025-06-20 11:29:35.000000000","message":"you\u0027re right, I assumed the Volume objects would not be hashable, but they are, so we could use a set here, regardless I think it\u0027s still better performance wise to iterate over the volumes first","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"b3804e81bdd99e7cc5fb345a3f5891c0a02522ed","unresolved":true,"context_lines":[{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        volumes \u003d self.cinder.get_volume_list()"},{"line_number":547,"context_line":"        target_volumes \u003d {}"},{"line_number":548,"context_line":"        for migrate_input in self.migrate_storage_pools:"},{"line_number":549,"context_line":"            src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":550,"context_line":"            src_type \u003d migrate_input.get(\"src_type\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"10f3a5d6_8dacf5b4","line":547,"range":{"start_line":547,"start_character":8,"end_line":547,"end_character":27},"in_reply_to":"f3bf51f1_f55dace0","updated":"2025-06-20 11:10:10.000000000","message":"I used a dict to avoid duplication, because this version is iterating multiple times over the list of volumes, and using directly a list could lead to duplications if the user passed mutiple sources, e.g one with src_pool and another with src_type, both could match the same volume and list it for migration twice. However, I just realized it would be better to swap the loop and iterate over the volumes in the outermost loop. With that change we would not need the dict, I\u0027ll do that, thanks!","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"c6c5d0cc6506abc98c66ebd24e2c5087a30395f3","unresolved":true,"context_lines":[{"line_number":551,"context_line":"            for volume in volumes:"},{"line_number":552,"context_line":"                if getattr(volume, \u0027os-vol-host-attr:host\u0027) \u003d\u003d src_pool and \\"},{"line_number":553,"context_line":"                        _is_src_type(volume, src_type) and \\"},{"line_number":554,"context_line":"                        self.storage_model.get_volume_by_uuid(volume.id):"},{"line_number":555,"context_line":"                    target_volumes[volume.id] \u003d volume"},{"line_number":556,"context_line":""},{"line_number":557,"context_line":"        return list(target_volumes.values())"}],"source_content_type":"text/x-python","patch_set":2,"id":"8535b1cb_6bd5bfea","line":554,"range":{"start_line":554,"start_character":24,"end_line":554,"end_character":72},"updated":"2025-06-20 09:52:19.000000000","message":"we need this to make sure the volume is in the model? we may avoid that by using the model to get the list of volumes.","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5c57a7b1d612a50dd9be3fb2d6c25351148e3c6a","unresolved":false,"context_lines":[{"line_number":551,"context_line":"            for volume in volumes:"},{"line_number":552,"context_line":"                if getattr(volume, \u0027os-vol-host-attr:host\u0027) \u003d\u003d src_pool and \\"},{"line_number":553,"context_line":"                        _is_src_type(volume, src_type) and \\"},{"line_number":554,"context_line":"                        self.storage_model.get_volume_by_uuid(volume.id):"},{"line_number":555,"context_line":"                    target_volumes[volume.id] \u003d volume"},{"line_number":556,"context_line":""},{"line_number":557,"context_line":"        return list(target_volumes.values())"}],"source_content_type":"text/x-python","patch_set":2,"id":"06aad669_e4cbdd32","line":554,"range":{"start_line":554,"start_character":24,"end_line":554,"end_character":72},"in_reply_to":"6efa2d7a_598077d5","updated":"2025-06-25 12:57:46.000000000","message":"Done","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"b3804e81bdd99e7cc5fb345a3f5891c0a02522ed","unresolved":true,"context_lines":[{"line_number":551,"context_line":"            for volume in volumes:"},{"line_number":552,"context_line":"                if getattr(volume, \u0027os-vol-host-attr:host\u0027) \u003d\u003d src_pool and \\"},{"line_number":553,"context_line":"                        _is_src_type(volume, src_type) and \\"},{"line_number":554,"context_line":"                        self.storage_model.get_volume_by_uuid(volume.id):"},{"line_number":555,"context_line":"                    target_volumes[volume.id] \u003d volume"},{"line_number":556,"context_line":""},{"line_number":557,"context_line":"        return list(target_volumes.values())"}],"source_content_type":"text/x-python","patch_set":2,"id":"6efa2d7a_598077d5","line":554,"range":{"start_line":554,"start_character":24,"end_line":554,"end_character":72},"in_reply_to":"8535b1cb_6bd5bfea","updated":"2025-06-20 11:10:10.000000000","message":"yes, that was my assumption as well, I\u0027ll change the calls to use the model","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4499604cc7d5b730e5921bbd78dd5422b184383d","unresolved":true,"context_lines":[{"line_number":540,"context_line":"        :returns: volume list on src pools and storage scope"},{"line_number":541,"context_line":"        \"\"\""},{"line_number":542,"context_line":"        def _is_src_type(volume, src_type):"},{"line_number":543,"context_line":"            return src_type is None or \\"},{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        target_volumes \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"fd066c2d_8eda9501","line":543,"range":{"start_line":543,"start_character":39,"end_line":543,"end_character":40},"updated":"2025-09-02 11:35:24.000000000","message":"we should avoid using backslash to wrap, we should use parentheses instead[1]\n\n[1] https://docs.openstack.org/hacking/latest/user/hacking.html#general","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"6648ec8f39688757fd0e026ac81ca2325d73c802","unresolved":false,"context_lines":[{"line_number":540,"context_line":"        :returns: volume list on src pools and storage scope"},{"line_number":541,"context_line":"        \"\"\""},{"line_number":542,"context_line":"        def _is_src_type(volume, src_type):"},{"line_number":543,"context_line":"            return src_type is None or \\"},{"line_number":544,"context_line":"                (src_type is not None and volume.volume_type \u003d\u003d src_type)"},{"line_number":545,"context_line":""},{"line_number":546,"context_line":"        target_volumes \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"3f7697b3_7e97d64b","line":543,"range":{"start_line":543,"start_character":39,"end_line":543,"end_character":40},"in_reply_to":"fd066c2d_8eda9501","updated":"2025-09-02 14:29:44.000000000","message":"thanks,fixed","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4499604cc7d5b730e5921bbd78dd5422b184383d","unresolved":true,"context_lines":[{"line_number":552,"context_line":"            for migrate_input in self.migrate_storage_pools:"},{"line_number":553,"context_line":"                src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":554,"context_line":"                src_type \u003d migrate_input.get(\"src_type\")"},{"line_number":555,"context_line":"                if getattr(volume, \u0027os-vol-host-attr:host\u0027) \u003d\u003d src_pool and \\"},{"line_number":556,"context_line":"                        _is_src_type(volume, src_type):"},{"line_number":557,"context_line":"                    target_volumes.append(volume)"},{"line_number":558,"context_line":"                    # once the volume satisfies one the storage_pools"}],"source_content_type":"text/x-python","patch_set":6,"id":"7a2c277d_91e53c70","line":555,"range":{"start_line":555,"start_character":76,"end_line":555,"end_character":77},"updated":"2025-09-02 11:35:24.000000000","message":"ditto","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"6648ec8f39688757fd0e026ac81ca2325d73c802","unresolved":false,"context_lines":[{"line_number":552,"context_line":"            for migrate_input in self.migrate_storage_pools:"},{"line_number":553,"context_line":"                src_pool \u003d migrate_input[\"src_pool\"]"},{"line_number":554,"context_line":"                src_type \u003d migrate_input.get(\"src_type\")"},{"line_number":555,"context_line":"                if getattr(volume, \u0027os-vol-host-attr:host\u0027) \u003d\u003d src_pool and \\"},{"line_number":556,"context_line":"                        _is_src_type(volume, src_type):"},{"line_number":557,"context_line":"                    target_volumes.append(volume)"},{"line_number":558,"context_line":"                    # once the volume satisfies one the storage_pools"}],"source_content_type":"text/x-python","patch_set":6,"id":"2de29ad3_2582bd1c","line":555,"range":{"start_line":555,"start_character":76,"end_line":555,"end_character":77},"in_reply_to":"7a2c277d_91e53c70","updated":"2025-09-02 14:29:44.000000000","message":"fixed","commit_id":"3f1d39c9cd078093a8db58f0ec995dc66ce71586"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3970f8a4a9198d63b1d8af336ff24fd2abd4c8a4","unresolved":true,"context_lines":[{"line_number":560,"context_line":"                    # inputs, we don\u0027t need to check the rest"},{"line_number":561,"context_line":"                    break"},{"line_number":562,"context_line":""},{"line_number":563,"context_line":"        return target_volumes"},{"line_number":564,"context_line":""},{"line_number":565,"context_line":"    def filtered_targets(self):"},{"line_number":566,"context_line":"        \"\"\"Filter targets"}],"source_content_type":"text/x-python","patch_set":9,"id":"66584f99_3af20f6f","line":563,"updated":"2025-09-15 17:34:29.000000000","message":"meta nit: this cod is a littel dense/complex\n\ni feel like we could simpfly this but i also agree that we can proceed witht eh current code for now.","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"}],"watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py":[{"author":{"_account_id":16312,"name":"Alfredo Moralejo","email":"amoralej@redhat.com","username":"amoralej"},"change_message_id":"c6c5d0cc6506abc98c66ebd24e2c5087a30395f3","unresolved":true,"context_lines":[{"line_number":421,"context_line":"        volume_on_src4 \u003d self.fake_volume(host\u003d\"src2@back1#pool1\","},{"line_number":422,"context_line":"                                          id\u003dvolume_uuid_mapping[\"volume_0\"],"},{"line_number":423,"context_line":"                                          name\u003d\"volume_4\")"},{"line_number":424,"context_line":"        self.m_migrate_storage_pools.return_value \u003d ["},{"line_number":425,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst1@back1#pool1\","},{"line_number":426,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type1\"},"},{"line_number":427,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst2@back2#pool1\","},{"line_number":428,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type3\"}"},{"line_number":429,"context_line":"        ]"},{"line_number":430,"context_line":"        self.m_c_helper.get_volume_list.return_value \u003d ["},{"line_number":431,"context_line":"            volume_on_src1,"}],"source_content_type":"text/x-python","patch_set":2,"id":"17cd7f19_453b9d9b","line":428,"range":{"start_line":424,"start_character":0,"end_line":428,"end_character":54},"updated":"2025-06-20 09:52:19.000000000","message":"This is an interesting one.\n\nQuestion unrelated to the get_volumes() itself. This should not be accepted as valid by the strategy as has two diferent dst for the same source, do we have any protection for this?","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"5c57a7b1d612a50dd9be3fb2d6c25351148e3c6a","unresolved":true,"context_lines":[{"line_number":421,"context_line":"        volume_on_src4 \u003d self.fake_volume(host\u003d\"src2@back1#pool1\","},{"line_number":422,"context_line":"                                          id\u003dvolume_uuid_mapping[\"volume_0\"],"},{"line_number":423,"context_line":"                                          name\u003d\"volume_4\")"},{"line_number":424,"context_line":"        self.m_migrate_storage_pools.return_value \u003d ["},{"line_number":425,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst1@back1#pool1\","},{"line_number":426,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type1\"},"},{"line_number":427,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst2@back2#pool1\","},{"line_number":428,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type3\"}"},{"line_number":429,"context_line":"        ]"},{"line_number":430,"context_line":"        self.m_c_helper.get_volume_list.return_value \u003d ["},{"line_number":431,"context_line":"            volume_on_src1,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4a034dc2_1e4bc471","line":428,"range":{"start_line":424,"start_character":0,"end_line":428,"end_character":54},"in_reply_to":"17cd7f19_453b9d9b","updated":"2025-06-25 12:57:46.000000000","message":"I just checked in a devstack deployment and no, there is no protection for this, it seems to be using the first input to create the actions and ignores the second","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":30002,"name":"Douglas Viroel","email":"viroel@gmail.com","username":"dviroel"},"change_message_id":"4499604cc7d5b730e5921bbd78dd5422b184383d","unresolved":true,"context_lines":[{"line_number":421,"context_line":"        volume_on_src4 \u003d self.fake_volume(host\u003d\"src2@back1#pool1\","},{"line_number":422,"context_line":"                                          id\u003dvolume_uuid_mapping[\"volume_0\"],"},{"line_number":423,"context_line":"                                          name\u003d\"volume_4\")"},{"line_number":424,"context_line":"        self.m_migrate_storage_pools.return_value \u003d ["},{"line_number":425,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst1@back1#pool1\","},{"line_number":426,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type1\"},"},{"line_number":427,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst2@back2#pool1\","},{"line_number":428,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type3\"}"},{"line_number":429,"context_line":"        ]"},{"line_number":430,"context_line":"        self.m_c_helper.get_volume_list.return_value \u003d ["},{"line_number":431,"context_line":"            volume_on_src1,"}],"source_content_type":"text/x-python","patch_set":2,"id":"8bd2ab99_e9e2c379","line":428,"range":{"start_line":424,"start_character":0,"end_line":428,"end_character":54},"in_reply_to":"4a034dc2_1e4bc471","updated":"2025-09-02 11:35:24.000000000","message":"I think that in general, the strategy needs a sanity check on these parameters provided by the user, before start building a solution. But maybe this is something for another bug","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":34452,"name":"Joan Gilabert","display_name":"jgilaber","email":"jgilaber@redhat.com","username":"jgilaber"},"change_message_id":"6648ec8f39688757fd0e026ac81ca2325d73c802","unresolved":true,"context_lines":[{"line_number":421,"context_line":"        volume_on_src4 \u003d self.fake_volume(host\u003d\"src2@back1#pool1\","},{"line_number":422,"context_line":"                                          id\u003dvolume_uuid_mapping[\"volume_0\"],"},{"line_number":423,"context_line":"                                          name\u003d\"volume_4\")"},{"line_number":424,"context_line":"        self.m_migrate_storage_pools.return_value \u003d ["},{"line_number":425,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst1@back1#pool1\","},{"line_number":426,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type1\"},"},{"line_number":427,"context_line":"            {\"src_pool\": \"src2@back1#pool1\", \"dst_pool\": \"dst2@back2#pool1\","},{"line_number":428,"context_line":"             \"src_type\": \"type1\", \"dst_type\": \"type3\"}"},{"line_number":429,"context_line":"        ]"},{"line_number":430,"context_line":"        self.m_c_helper.get_volume_list.return_value \u003d ["},{"line_number":431,"context_line":"            volume_on_src1,"}],"source_content_type":"text/x-python","patch_set":2,"id":"2a4ac25e_dceea583","line":428,"range":{"start_line":424,"start_character":0,"end_line":428,"end_character":54},"in_reply_to":"8bd2ab99_e9e2c379","updated":"2025-09-02 14:29:44.000000000","message":"indeed, we need further validation for the parameters. We have already a bug https://bugs.launchpad.net/watcher/+bug/2111113 that is related to this. I opened a new one to have some tracker of the problem https://bugs.launchpad.net/watcher/+bug/2121870","commit_id":"888050c38a3e02c03c4cad6035ec7d144e848ce4"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3970f8a4a9198d63b1d8af336ff24fd2abd4c8a4","unresolved":false,"context_lines":[{"line_number":257,"context_line":"        volumes \u003d self.strategy.get_volumes()"},{"line_number":258,"context_line":""},{"line_number":259,"context_line":"        # src1, src2 are in volumes"},{"line_number":260,"context_line":"        # src3 is not in volumes"},{"line_number":261,"context_line":"        self.assertIn(volume_on_src1, volumes)"},{"line_number":262,"context_line":"        self.assertIn(volume_on_src2, volumes)"},{"line_number":263,"context_line":"        self.assertNotIn(volume_on_src3, volumes)"}],"source_content_type":"text/x-python","patch_set":9,"id":"92690f99_6c33a633","line":260,"updated":"2025-09-15 17:34:29.000000000","message":"this isnt exactly a great comment as it does not say why we expect the follwoing assertion to hold it just repates the same info","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"21414b4b472cb6a06d398922ba836cc13a42d4f9","unresolved":true,"context_lines":[{"line_number":291,"context_line":"        self.assertIn(volume_on_src1, volumes)"},{"line_number":292,"context_line":"        self.assertIn(volume_on_src4, volumes)"},{"line_number":293,"context_line":"        self.assertNotIn(volume_on_src3, volumes)"},{"line_number":294,"context_line":"        self.assertNotIn(volume_on_src2, volumes)"},{"line_number":295,"context_line":""},{"line_number":296,"context_line":"    def test_get_volumes_different_types_same_pool(self):"},{"line_number":297,"context_line":"        volume_on_src1 \u003d self.fake_volume(host\u003d\"src1@back1#pool1\","}],"source_content_type":"text/x-python","patch_set":9,"id":"e9022936_b5638141","line":294,"updated":"2025-09-15 18:44:18.000000000","message":"just to be clear why i don\u0027t particular like this testing\n\nthis is the fucntion that is being tested\n```\n    def get_volumes(self):\n        \"\"\"Get migrate target volumes\n\n        :returns: volume list on src pools and storage scope\n        \"\"\"\n\n        src_pool_list \u003d self.get_src_pool_list()\n\n        return [i for i in self.cinder.get_volume_list()\n                if getattr(i, \u0027os-vol-host-attr:host\u0027) in src_pool_list and\n                self.storage_model.get_volume_by_uuid(i.id)]\n```\n\nto test this properly we should be mocking\n\nget_src_pool_list(), cinder.get_volume_list() and storage_model.get_volume_by_uuid\nwe should not be mocking anything else.\n\nwe shoudl b asserting that all 3 are called.\n\nget_src_pool_list is a pure fucnton of the storage modele\n\n```\n\n    def get_src_pool_list(self):\n        \"\"\"Get src pools from migrate_storage_pools\n\n        :returns: src pool name list\n        \"\"\"\n\n        return [v for dic in self.migrate_storage_pools\n                for k, v in dic.items() if k \u003d\u003d \"src_pool\"]\n```\nso it would be accpatble to not mock that if we mock the storage model.\n\nwe do setup a stroage mock \n\n```\n        p_s_model \u003d mock.patch.object(\n            strategies.ZoneMigration, \"storage_model\",\n            new_callable\u003dmock.PropertyMock)\n        self.m_s_model \u003d p_s_model.start()\n        self.addCleanup(p_s_model.stop)\n```\n\nwhich is accsabel via self.m_s_model     \n        \nwhich uses a fake model\n```\n        model \u003d self.fake_s_cluster.generate_scenario_1()\n        self.m_s_model.return_value \u003d model\n```\n\nbut none of this is visable in this unit test.\n\nit is defiend here \n\nhttps://github.com/openstack/watcher/blob/master/watcher/tests/decision_engine/model/faker_cluster_state.py#L314\n\nwhich in-turn uses https://github.com/openstack/watcher/blob/master/watcher/tests/decision_engine/model/data/storage_scenario_1.xml\n\nas i said setting up the storage or compute model mocks in the setup is ok.\nits a bit indierctly bug it become  aproblem when we setup a lost of other mocks that shoud only be mocked in specific test not in the setup function.\n\nit makes it vary hard to see what the input are to the fucntion tha cause this behivoar.\n\nin thie case the xml file.","commit_id":"598b28656673bd63945d6c16e23960d40a5db735"}]}