)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3823aab09400d767b7aa9c24f861d1e217533e16","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"dd1eb79e_30dd1d1f","updated":"2022-05-23 15:33:25.000000000","message":"I agree that a test would be helpful. But I don\u0027t think there is a mismatch of interfaces here.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"c3ce2fa1a472da032d5e3e995848ec784f588b16","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"3030d2db_bed96a58","updated":"2022-05-23 07:39:02.000000000","message":"I think unit test should be also fixed here, as it seems it\u0027s missing that assertion. I guess that\u0027s the proper one that should contain check https://opendev.org/zuul/nodepool/src/branch/master/nodepool/tests/unit/test_builder.py#L172","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"de198955c52d5cbb5eca92126b2079357f3960d0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e31ec619_e00fea83","updated":"2022-05-23 07:40:26.000000000","message":"Oh, sorry, I\u0027m wrong. Just another test should be written, like `test_post_upload_hook_failure`, as only positive test is present (https://opendev.org/zuul/nodepool/src/branch/master/nodepool/tests/unit/test_builder.py#L560)","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":32053,"name":"Denys Mishchenko","email":"denis@mischenko.org.ua","username":"arddennis"},"change_message_id":"ff0d49d27b3409a1d779e0c606111de62ae980bb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"50bf42c4_8653c39a","in_reply_to":"e31ec619_e00fea83","updated":"2022-05-24 13:54:10.000000000","message":"I tried to write a test, but faced the following problem:\nNodepool skips failures and retry image building and uploading continuously. And this can\u0027t be tracked trough zookeeper as failed image marked failed. There is no way to check that typeerror was raised during image upload. And there is no way to get a list of images from fake provider to compare them during builder run. At lease I haven\u0027t found how.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":32053,"name":"Denys Mishchenko","email":"denis@mischenko.org.ua","username":"arddennis"},"change_message_id":"ff0d49d27b3409a1d779e0c606111de62ae980bb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"b463ec8f_f8c540ce","updated":"2022-05-24 13:54:10.000000000","message":"Trying to write a test which could cover deleteImage invocation. It seems that most of the tests rely on zookeeper information and miss a lot. \n\nI added an test to check that the image is failed on post_upload_hook failure and it is marked failed in zookeeper. But exception isn\u0027t tracked. Will try to find how to get a list of images from fake provider to verify.","commit_id":"1ae68996c7ab023fa17a94f83ebbda41e49efe67"},{"author":{"_account_id":32053,"name":"Denys Mishchenko","email":"denis@mischenko.org.ua","username":"arddennis"},"change_message_id":"e2249156f0f63ed7ef891572a41629db3f41b0a3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"1ef0ab74_8074f7f9","updated":"2022-05-31 07:13:07.000000000","message":"Wrote a test scenario which creates files during the image creation and then remove them on a delete. This is done within image directory so being removed after testing. \n\nI do not know any other way to test the behaviour. ","commit_id":"6f8a0192b36afb35df930d078fd12e0ae74f83f5"}],"nodepool/builder.py":[{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"c45c82eccc712628e5b3e11039a8e7812848ba92","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"fb48e40c_f30131fe","line":1128,"updated":"2022-05-23 10:46:09.000000000","message":"Also are you sure that this should be patched instead of patching https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/openstack/provider.py#L216 to require only uuid and get name by it?\n\nAt this point it feels like providers are not in sync about amount of arguments to accept. And it also feels, like accepting single argument would be more correct, but not sure.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":34932,"name":"Ihor Krokhin","email":"cyberukr@gmail.com","username":"nautics889"},"change_message_id":"ec762fcd349533b25f208e119ea62009753b8185","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"0b303390_6880fc89","line":1128,"updated":"2022-05-30 07:59:44.000000000","message":"It definetely will keep failing with TypeError at least because following methods in AWS, Azure and IBM VPC drivers accept only one parameter:\n\nhttps://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/azure/adapter.py#L479\n\nhttps://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/aws/adapter.py#L343\n\nhttps://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/ibmvpc/adapter.py#L523\n\nI learned sources for a while and found an interface-class for adapters (https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/statemachine.py#L808) with same named method `deleteImage(external_id)` taking exactly one parameter.\nThough provider for OpenStack (https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/openstack/provider.py#L40) doesn\u0027t inherit that class excplicitly, they both have a common methods supposed to provide the same functionality and probably might have same signatures (`deleteImage()`, `uploadImage()`). \n\nSo i would propose getting rid from `name` parameter in the following lines:\nhttps://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/openstack/provider.py#L216\nhttps://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/statemachine.py#L667\nas Dmitriy suggested above. As a quick fix. \n\nFurther maybe there will be good to see reimplementing of base classes for all drivers to enhance uniformity of interfaces.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":32053,"name":"Denys Mishchenko","email":"denis@mischenko.org.ua","username":"arddennis"},"change_message_id":"e2249156f0f63ed7ef891572a41629db3f41b0a3","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"f50b1088_9305cb82","line":1128,"in_reply_to":"0b303390_6880fc89","updated":"2022-05-31 07:13:07.000000000","message":"I agree that it would be cool to rewrite code for the openstack provider and make it using statemachine, but does this mean that purposed fix isn\u0027t an option and It should be solved by the provider refactor?","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"b4be0d6e4518997f8e4de46d8a066bc78d02995c","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"ab54508e_563790f0","line":1128,"in_reply_to":"88e57212_5bf74395","updated":"2022-06-14 15:43:28.000000000","message":"No, I was suggesting that the change as written is correct. You don\u0027t need to rewrite openstack to use statemachine in order to fix this. I was responding to the suggestion above that suggested patching a different part of the code. The providers are in sync. They all use the same interface.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":32053,"name":"Denys Mishchenko","email":"denis@mischenko.org.ua","username":"arddennis"},"change_message_id":"e2249156f0f63ed7ef891572a41629db3f41b0a3","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"88e57212_5bf74395","line":1128,"in_reply_to":"a5c0b61e_ac9c2dfa","updated":"2022-05-31 07:13:07.000000000","message":"does this mean that I have to refactor openstack provider to use statemachine instead of just additional argument passing?","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"b4be0d6e4518997f8e4de46d8a066bc78d02995c","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"d21e9cf3_49b3f755","line":1128,"in_reply_to":"f50b1088_9305cb82","updated":"2022-06-14 15:43:28.000000000","message":"As mentioned perviously the manager.deleteImage() call should be https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/statemachine.py#L667 which then calls the methods listed above that only accept the id parameter. What this means is that everything is currently in sync except for this driver.\n\nWe should not need to rewrite the openstack driver to use statemanager to fix this. The proposed fix should be fine. Separately rewriting the driver to use statemanager may be a good idea but I do not think it is necessary here and a significant amount of work in comparison.\n\nBasically lets fix the bug in the most straightforward manner (which I believe this change already does), then followup with any suggested refactoring to reduce confusion.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"},{"author":{"_account_id":4146,"name":"Clark Boylan","email":"cboylan@sapwetik.org","username":"cboylan"},"change_message_id":"3823aab09400d767b7aa9c24f861d1e217533e16","unresolved":true,"context_lines":[{"line_number":1125,"context_line":"                    self.log.exception(\u0027Unknown exception during upload hook\u0027)"},{"line_number":1126,"context_line":""},{"line_number":1127,"context_line":"                try:"},{"line_number":1128,"context_line":"                    manager.deleteImage(ext_image_name, external_id)"},{"line_number":1129,"context_line":"                except Exception:"},{"line_number":1130,"context_line":"                    # Image delete failed but we cannot do anything about this"},{"line_number":1131,"context_line":"                    # right now so just log the exception."}],"source_content_type":"text/x-python","patch_set":1,"id":"a5c0b61e_ac9c2dfa","line":1128,"in_reply_to":"fb48e40c_f30131fe","updated":"2022-05-23 15:33:25.000000000","message":"I don\u0027t think this code was ever correct. This should call into https://opendev.org/zuul/nodepool/src/branch/master/nodepool/driver/statemachine.py#L667 for all of the drivers which has the same signature whether aws or openstack or azure.","commit_id":"64cdd177bde3b16f294966a566d14684168a8c5d"}]}