)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"297174b0ad2b1457972d26569e5b0e6a9e0dd1e7","unresolved":false,"context_lines":[{"line_number":17,"context_line":"is abnormal, the node where the placement service is down),"},{"line_number":18,"context_line":"cyborg will still update its database. When the placement"},{"line_number":19,"context_line":"service is normal, the cyborg database has been updated."},{"line_number":20,"context_line":"Although the device inspection will be performed in the next"},{"line_number":21,"context_line":"cycle, because it has been reported to the cyborg database before,"},{"line_number":22,"context_line":"there will be no diff at this time, and the placement side will"},{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_1b7c5481","line":24,"range":{"start_line":20,"start_character":0,"end_line":24,"end_character":46},"updated":"2020-03-09 13:27:23.000000000","message":"right so there is an architectural issue here too.\nplacement should be the single source of truth for capacity info. the cyborg data bases should only be used for assignment and state tracking.\n\nso we should be doing the placement update first and only update the cyborg database after that completes successfully so that the cyborg db never gets out of sync with placement.\n\nthe other architecual issue is the placmeent update should be done entirly form the cyborg compute agent and not via the conductor but thats a separate issue to be addressed at a later data.\n\nthe current implementation will not scale  as it centralises the compute and update in the conductor process and will have issue when deploy in a distributed/edge toplogy. effectively the agent will ddos the conductor when deployed in large numbers. ideally each agent would local compute the new state then update placement directly and only after its internal state and placement have been updated, it would finally update the cyborg database via the conductor.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"6648b56c3985cd53e82ecc780ed9670751842fc4","unresolved":false,"context_lines":[{"line_number":17,"context_line":"is abnormal, the node where the placement service is down),"},{"line_number":18,"context_line":"cyborg will still update its database. When the placement"},{"line_number":19,"context_line":"service is normal, the cyborg database has been updated."},{"line_number":20,"context_line":"Although the device inspection will be performed in the next"},{"line_number":21,"context_line":"cycle, because it has been reported to the cyborg database before,"},{"line_number":22,"context_line":"there will be no diff at this time, and the placement side will"},{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_7eb76cf6","line":24,"range":{"start_line":20,"start_character":0,"end_line":24,"end_character":46},"in_reply_to":"1fa4df85_1b7c5481","updated":"2020-03-10 02:00:49.000000000","message":"Hi. Pls see the below reply.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":24872,"name":"YumengBao","email":"yumeng_bao@yahoo.com","username":"Yumeng_Bao"},"change_message_id":"3d7c51188623d38fd56043058cf937dadbd5659f","unresolved":false,"context_lines":[{"line_number":17,"context_line":"is abnormal, the node where the placement service is down),"},{"line_number":18,"context_line":"cyborg will still update its database. When the placement"},{"line_number":19,"context_line":"service is normal, the cyborg database has been updated."},{"line_number":20,"context_line":"Although the device inspection will be performed in the next"},{"line_number":21,"context_line":"cycle, because it has been reported to the cyborg database before,"},{"line_number":22,"context_line":"there will be no diff at this time, and the placement side will"},{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_dc3c6894","line":24,"range":{"start_line":20,"start_character":0,"end_line":24,"end_character":46},"in_reply_to":"1fa4df85_1b7c5481","updated":"2020-03-10 07:34:53.000000000","message":"Many thanks for your valued suggestions, Sean!\nModifying the current architecture of the resource report is one of the cyborg\u0027s important tasks. \n\"Compute agent collect its internal state and report directly to placement\", \"after these are done, update cyborg DB\". These are very good ideas. We\u0027ll discuss with other cyborg contributors in our weekly meeting.\nThanks again!","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"297174b0ad2b1457972d26569e5b0e6a9e0dd1e7","unresolved":false,"context_lines":[{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"},{"line_number":28,"context_line":"an unreachable service exception. In the next cycle, maybe the"},{"line_number":29,"context_line":"placement service is back to normal, and at this point diff can"},{"line_number":30,"context_line":"proceed smoothly."},{"line_number":31,"context_line":"The reason why this is done is because the entry for resource"},{"line_number":32,"context_line":"scheduling is in placement. When the placement data is incorrect,"},{"line_number":33,"context_line":"the data on cyborg side will lose its meaning."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Story: 2007393"},{"line_number":36,"context_line":"Task: 38989"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_7b3ea8c1","line":33,"range":{"start_line":26,"start_character":1,"end_line":33,"end_character":46},"updated":"2020-03-09 13:27:23.000000000","message":"the issue seam totally valid by the way i just not sure if the patch will really help improve the situation.\n\nif we think we should fail fast then you can ignore my try/excpet/finally suggestion.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"789d4c922aa26f78f39dcabab737676ca8765a19","unresolved":false,"context_lines":[{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"},{"line_number":28,"context_line":"an unreachable service exception. In the next cycle, maybe the"},{"line_number":29,"context_line":"placement service is back to normal, and at this point diff can"},{"line_number":30,"context_line":"proceed smoothly."},{"line_number":31,"context_line":"The reason why this is done is because the entry for resource"},{"line_number":32,"context_line":"scheduling is in placement. When the placement data is incorrect,"},{"line_number":33,"context_line":"the data on cyborg side will lose its meaning."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Story: 2007393"},{"line_number":36,"context_line":"Task: 38989"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_51585ef8","line":33,"range":{"start_line":26,"start_character":1,"end_line":33,"end_character":46},"in_reply_to":"1fa4df85_1ee998e4","updated":"2020-03-10 12:49:26.000000000","message":"ok if the agent recovers when placement is operational again then i guess that is fine. what i was worreid about is the service stopping and not restarting.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"af8f614e48666b855cdc67cbcc526b89abe2a0a3","unresolved":false,"context_lines":[{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"},{"line_number":28,"context_line":"an unreachable service exception. In the next cycle, maybe the"},{"line_number":29,"context_line":"placement service is back to normal, and at this point diff can"},{"line_number":30,"context_line":"proceed smoothly."},{"line_number":31,"context_line":"The reason why this is done is because the entry for resource"},{"line_number":32,"context_line":"scheduling is in placement. When the placement data is incorrect,"},{"line_number":33,"context_line":"the data on cyborg side will lose its meaning."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Story: 2007393"},{"line_number":36,"context_line":"Task: 38989"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_f42e188a","line":33,"range":{"start_line":26,"start_character":1,"end_line":33,"end_character":46},"in_reply_to":"1fa4df85_51585ef8","updated":"2020-03-10 13:26:48.000000000","message":"Yes, I have tested it in the devstack env. It will automatically return to normal when the placement-api service returns to normal.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"6648b56c3985cd53e82ecc780ed9670751842fc4","unresolved":false,"context_lines":[{"line_number":23,"context_line":"not be updated. Therefore, this situation will cause the data"},{"line_number":24,"context_line":"stored in the placement side to be dirty data."},{"line_number":25,"context_line":""},{"line_number":26,"context_line":"The solution is to expose the exception of the placement service,"},{"line_number":27,"context_line":"rather than a simple error prompt. Stop the data diff by throwing"},{"line_number":28,"context_line":"an unreachable service exception. In the next cycle, maybe the"},{"line_number":29,"context_line":"placement service is back to normal, and at this point diff can"},{"line_number":30,"context_line":"proceed smoothly."},{"line_number":31,"context_line":"The reason why this is done is because the entry for resource"},{"line_number":32,"context_line":"scheduling is in placement. When the placement data is incorrect,"},{"line_number":33,"context_line":"the data on cyborg side will lose its meaning."},{"line_number":34,"context_line":""},{"line_number":35,"context_line":"Story: 2007393"},{"line_number":36,"context_line":"Task: 38989"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"1fa4df85_1ee998e4","line":33,"range":{"start_line":26,"start_character":1,"end_line":33,"end_character":46},"in_reply_to":"1fa4df85_7b3ea8c1","updated":"2020-03-10 02:00:49.000000000","message":"Thanks sean for your quick review!\nIn my opinion, according to the current architecture where cyborg and placement have two independent databases, when the placement fails, cyborg\u0027s own database should stop updating and throw exceptions quickly.\n\nI manually stopped and started the placement-api service, and did several experiments in the devstack environment. The results show that this solution works well. It can quickly cause the cond and agent services to throw exceptions, but instead of directly bringing the service down, it is still active. When you use systemctl status to view the cond and agent services.\nHowever, the log will show that the placement-api exception caused the data diff to fail. When the placement-api service returns to normal, there are no errors in the log, and the data can continue to diff.\nThe architecture of accelerator resource synchronization and whether placement should be updated in the cond process will be discussed in the next period. I think this patch is a good idea at present, wait to see the opinions of other friends","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"}],"cyborg/common/placement_client.py":[{"author":{"_account_id":14107,"name":"zhurong","email":"aaronzhu1121@gmail.com","username":"zhurong"},"change_message_id":"374be4d22f9d06938af69dcb8861d95882647a00","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    def get(self, url, version\u003dNone, global_request_id\u003dNone):"},{"line_number":37,"context_line":"        res \u003d self._client.get(url, microversion\u003dversion,"},{"line_number":38,"context_line":"                               global_request_id\u003dglobal_request_id)"},{"line_number":39,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":40,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":41,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":42,"context_line":"        return res"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_7f043415","line":39,"range":{"start_line":39,"start_character":30,"end_line":39,"end_character":33},"updated":"2020-03-11 05:58:49.000000000","message":"I don\u0027t think we should only just handle the 503 exception, IMO, should use \u003e\u003d 500.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"c19cb83e5f75a1f7d689f7e84b61aa48454074dc","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    def get(self, url, version\u003dNone, global_request_id\u003dNone):"},{"line_number":37,"context_line":"        res \u003d self._client.get(url, microversion\u003dversion,"},{"line_number":38,"context_line":"                               global_request_id\u003dglobal_request_id)"},{"line_number":39,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":40,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":41,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":42,"context_line":"        return res"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_831afe6c","line":39,"range":{"start_line":39,"start_character":30,"end_line":39,"end_character":33},"in_reply_to":"1fa4df85_7f043415","updated":"2020-03-11 08:25:32.000000000","message":"Done","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"3fa32a1665065eecbe5208df13a0ec7abbc24553","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    def get(self, url, version\u003dNone, global_request_id\u003dNone):"},{"line_number":37,"context_line":"        res \u003d self._client.get(url, microversion\u003dversion,"},{"line_number":38,"context_line":"                               global_request_id\u003dglobal_request_id)"},{"line_number":39,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":40,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":41,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":42,"context_line":"        return res"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_1fb6608b","line":39,"range":{"start_line":39,"start_character":30,"end_line":39,"end_character":33},"in_reply_to":"1fa4df85_7f043415","updated":"2020-03-11 08:10:52.000000000","message":"Emmmm... Sure. 500 Internal Server Error,502 Bad Gateway,503 Service Unavailable,504 Gateway Timeout....Also can make updating placement failed.\nWill update it, thanks.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":26458,"name":"Brin Zhang","email":"zhangbailin@inspur.com","username":"zhangbailin"},"change_message_id":"a56b4d797a9b7593463cb3404c0c0ff0bb08388d","unresolved":false,"context_lines":[{"line_number":36,"context_line":"    def get(self, url, version\u003dNone, global_request_id\u003dNone):"},{"line_number":37,"context_line":"        res \u003d self._client.get(url, microversion\u003dversion,"},{"line_number":38,"context_line":"                               global_request_id\u003dglobal_request_id)"},{"line_number":39,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":40,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":41,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":42,"context_line":"        return res"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_66d8077f","line":39,"range":{"start_line":39,"start_character":30,"end_line":39,"end_character":33},"in_reply_to":"1fa4df85_831afe6c","updated":"2020-03-13 07:24:45.000000000","message":"Yes, \u003e\u003d 500 should be handled.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"05a644e9bd3e6b7ec4ea81c49a7341408adc57d7","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                               global_request_id\u003dglobal_request_id,"},{"line_number":58,"context_line":"                               **kwargs)"},{"line_number":59,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":60,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":61,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":62,"context_line":"        return res"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_a022b1b7","line":60,"range":{"start_line":60,"start_character":9,"end_line":60,"end_character":47},"updated":"2020-03-09 13:14:32.000000000","message":"where is this exception eventurally caught?\nwe dont want this to propagate all the way to the main function and kill the agent or conductor in this case.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"6648b56c3985cd53e82ecc780ed9670751842fc4","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                               global_request_id\u003dglobal_request_id,"},{"line_number":58,"context_line":"                               **kwargs)"},{"line_number":59,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":60,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":61,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":62,"context_line":"        return res"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_bea644c0","line":60,"range":{"start_line":60,"start_character":9,"end_line":60,"end_character":47},"in_reply_to":"1fa4df85_a022b1b7","updated":"2020-03-10 02:00:49.000000000","message":"The current idea is to expose the exception directly to the cond and agent services. This will not cause the service to hang directly, but this exception can be found through the log file. When the placement service returns to normal in the next cycle, cond and agent will return to normal.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"789d4c922aa26f78f39dcabab737676ca8765a19","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                               global_request_id\u003dglobal_request_id,"},{"line_number":58,"context_line":"                               **kwargs)"},{"line_number":59,"context_line":"        if res.status_code \u003d\u003d 503:"},{"line_number":60,"context_line":"            raise exception.ServiceUnavailable("},{"line_number":61,"context_line":"                \"Placement Service is unavailable at this time.\")"},{"line_number":62,"context_line":"        return res"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_510dbee5","line":60,"range":{"start_line":60,"start_character":9,"end_line":60,"end_character":47},"in_reply_to":"1fa4df85_bea644c0","updated":"2020-03-10 12:49:26.000000000","message":"ya so that implies this is being caught somehwere and then logged and the service recovers if that is the case then that is fine. i did not follow the call path to confim that which is why i asked.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"}],"cyborg/conductor/manager.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"05a644e9bd3e6b7ec4ea81c49a7341408adc57d7","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"},{"line_number":121,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(s)]"},{"line_number":122,"context_line":"            # First, get dev_obj_list from hostname"},{"line_number":123,"context_line":"            device_obj_list \u003d Device.get_list_by_hostname(context, host)"},{"line_number":124,"context_line":"            # Then, use controlpath_id.cpid_info to identiy one Device."},{"line_number":125,"context_line":"            cpid_info \u003d new_driver_dev_obj.controlpath_id.cpid_info"},{"line_number":126,"context_line":"            for dev_obj in device_obj_list:"},{"line_number":127,"context_line":"                # get cpid_obj, could be empty or only one value."},{"line_number":128,"context_line":"                cpid_obj \u003d ControlpathID.get_by_device_id_cpidinfo("},{"line_number":129,"context_line":"                    context, dev_obj.id, cpid_info)"},{"line_number":130,"context_line":"                # find the one cpid_obj with cpid_info"},{"line_number":131,"context_line":"                if cpid_obj is not None:"},{"line_number":132,"context_line":"                    break"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            changed_key \u003d [\u0027std_board_info\u0027, \u0027vendor\u0027, \u0027vendor_board_info\u0027,"},{"line_number":135,"context_line":"                           \u0027model\u0027, \u0027type\u0027]"},{"line_number":136,"context_line":"            for c_k in changed_key:"},{"line_number":137,"context_line":"                if getattr(new_driver_dev_obj, c_k) !\u003d getattr("},{"line_number":138,"context_line":"                        old_driver_dev_obj, c_k):"},{"line_number":139,"context_line":"                    setattr(dev_obj, c_k, getattr(new_driver_dev_obj, c_k))"},{"line_number":140,"context_line":"            dev_obj.save(context)"},{"line_number":141,"context_line":"            # diff the internal layer: driver_deployable"},{"line_number":142,"context_line":"            self.drv_deployable_make_diff(context, dev_obj.id, cpid_obj.id,"},{"line_number":143,"context_line":"                                          old_driver_dev_obj.deployable_list,"},{"line_number":144,"context_line":"                                          new_driver_dev_obj.deployable_list,"},{"line_number":145,"context_line":"                                          host_rp)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def drv_deployable_make_diff(self, context, device_id, cpid_id,"},{"line_number":148,"context_line":"                                 old_driver_dep_list, new_driver_dep_list,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_60703909","line":145,"range":{"start_line":104,"start_character":7,"end_line":145,"end_character":50},"updated":"2020-03-09 13:14:32.000000000","message":"reordering does not really help.\nif the delete triggers a 503 responce then the added will not be processed. similarly for added and same.\n\ni would suggest factorign out each loop into a separate inner function then cain this using a try,except,finally\n\n\ndef drv_device_make_diff (...):\n  ...\n\n  error \u003d None\n\n  def remove_deleted (...):\n   for d in deleted:\n            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]\n            for driver_dep_obj in old_driver_dev_obj.deployable_list:\n                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)\n                self._delete_provider_and_sub_providers(context, rp_uuid)\n            old_driver_dev_obj.destroy(context, host)\n\n  def create_added(...):\n    ...\n\n  def update_same(...):\n    ...\n\n\n  stub_cpid_list \u003d [driver_dev_obj.controlpath_id.cpid_info for\n                          driver_dev_obj in new_driver_device_list\n                          if driver_dev_obj.stub]\n        new_cpid_list \u003d [driver_dev_obj.controlpath_id.cpid_info for\n                         driver_dev_obj in new_driver_device_list]\n        old_cpid_list \u003d [driver_dev_obj.controlpath_id.cpid_info for\n                         driver_dev_obj in old_driver_device_list]\n        same \u003d set(new_cpid_list) \u0026 set(old_cpid_list) - set(stub_cpid_list)\n        added \u003d set(new_cpid_list) - same - set(stub_cpid_list)\n        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)\n        host_rp \u003d self._get_root_provider(context, host)\n\ntry:\n   remove_deleted(...)\nexcept ServiceUnaviailable as ex:\n  error \u003d ex\nfinally:\n  try:\n    create_added(...)\n\n   remove_deleted(...)\n  except ServiceUnaviailable as ex:\n      error \u003d ex\n  finally:\n    try:\n      update_same(...)\n    except ServiceUnaviailable as ex:\n      error \u003d ex\n    finally:\n       if error:\n         raise error\n\n\nthis should allow the function to try and continue to make progress if the outage is only temporay and we will still raise the exception at the end if there is one. it might be over kill but there isnt really any value in just reordering the function as it not any more correct then the current order.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"af8f614e48666b855cdc67cbcc526b89abe2a0a3","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"},{"line_number":121,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(s)]"},{"line_number":122,"context_line":"            # First, get dev_obj_list from hostname"},{"line_number":123,"context_line":"            device_obj_list \u003d Device.get_list_by_hostname(context, host)"},{"line_number":124,"context_line":"            # Then, use controlpath_id.cpid_info to identiy one Device."},{"line_number":125,"context_line":"            cpid_info \u003d new_driver_dev_obj.controlpath_id.cpid_info"},{"line_number":126,"context_line":"            for dev_obj in device_obj_list:"},{"line_number":127,"context_line":"                # get cpid_obj, could be empty or only one value."},{"line_number":128,"context_line":"                cpid_obj \u003d ControlpathID.get_by_device_id_cpidinfo("},{"line_number":129,"context_line":"                    context, dev_obj.id, cpid_info)"},{"line_number":130,"context_line":"                # find the one cpid_obj with cpid_info"},{"line_number":131,"context_line":"                if cpid_obj is not None:"},{"line_number":132,"context_line":"                    break"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            changed_key \u003d [\u0027std_board_info\u0027, \u0027vendor\u0027, \u0027vendor_board_info\u0027,"},{"line_number":135,"context_line":"                           \u0027model\u0027, \u0027type\u0027]"},{"line_number":136,"context_line":"            for c_k in changed_key:"},{"line_number":137,"context_line":"                if getattr(new_driver_dev_obj, c_k) !\u003d getattr("},{"line_number":138,"context_line":"                        old_driver_dev_obj, c_k):"},{"line_number":139,"context_line":"                    setattr(dev_obj, c_k, getattr(new_driver_dev_obj, c_k))"},{"line_number":140,"context_line":"            dev_obj.save(context)"},{"line_number":141,"context_line":"            # diff the internal layer: driver_deployable"},{"line_number":142,"context_line":"            self.drv_deployable_make_diff(context, dev_obj.id, cpid_obj.id,"},{"line_number":143,"context_line":"                                          old_driver_dev_obj.deployable_list,"},{"line_number":144,"context_line":"                                          new_driver_dev_obj.deployable_list,"},{"line_number":145,"context_line":"                                          host_rp)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def drv_deployable_make_diff(self, context, device_id, cpid_id,"},{"line_number":148,"context_line":"                                 old_driver_dep_list, new_driver_dep_list,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_310b622a","line":145,"range":{"start_line":104,"start_character":7,"end_line":145,"end_character":50},"in_reply_to":"1fa4df85_1124265d","updated":"2020-03-10 13:26:48.000000000","message":"Yes, after you emphasized that the function of this code is reorder, I believe this is helpful for others to review this code. Thanks again.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"789d4c922aa26f78f39dcabab737676ca8765a19","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"},{"line_number":121,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(s)]"},{"line_number":122,"context_line":"            # First, get dev_obj_list from hostname"},{"line_number":123,"context_line":"            device_obj_list \u003d Device.get_list_by_hostname(context, host)"},{"line_number":124,"context_line":"            # Then, use controlpath_id.cpid_info to identiy one Device."},{"line_number":125,"context_line":"            cpid_info \u003d new_driver_dev_obj.controlpath_id.cpid_info"},{"line_number":126,"context_line":"            for dev_obj in device_obj_list:"},{"line_number":127,"context_line":"                # get cpid_obj, could be empty or only one value."},{"line_number":128,"context_line":"                cpid_obj \u003d ControlpathID.get_by_device_id_cpidinfo("},{"line_number":129,"context_line":"                    context, dev_obj.id, cpid_info)"},{"line_number":130,"context_line":"                # find the one cpid_obj with cpid_info"},{"line_number":131,"context_line":"                if cpid_obj is not None:"},{"line_number":132,"context_line":"                    break"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            changed_key \u003d [\u0027std_board_info\u0027, \u0027vendor\u0027, \u0027vendor_board_info\u0027,"},{"line_number":135,"context_line":"                           \u0027model\u0027, \u0027type\u0027]"},{"line_number":136,"context_line":"            for c_k in changed_key:"},{"line_number":137,"context_line":"                if getattr(new_driver_dev_obj, c_k) !\u003d getattr("},{"line_number":138,"context_line":"                        old_driver_dev_obj, c_k):"},{"line_number":139,"context_line":"                    setattr(dev_obj, c_k, getattr(new_driver_dev_obj, c_k))"},{"line_number":140,"context_line":"            dev_obj.save(context)"},{"line_number":141,"context_line":"            # diff the internal layer: driver_deployable"},{"line_number":142,"context_line":"            self.drv_deployable_make_diff(context, dev_obj.id, cpid_obj.id,"},{"line_number":143,"context_line":"                                          old_driver_dev_obj.deployable_list,"},{"line_number":144,"context_line":"                                          new_driver_dev_obj.deployable_list,"},{"line_number":145,"context_line":"                                          host_rp)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def drv_deployable_make_diff(self, context, device_id, cpid_id,"},{"line_number":148,"context_line":"                                 old_driver_dep_list, new_driver_dep_list,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_1124265d","line":145,"range":{"start_line":104,"start_character":7,"end_line":145,"end_character":50},"in_reply_to":"1fa4df85_1e80b8f4","updated":"2020-03-10 12:49:26.000000000","message":"well over all i think this patch imporves the situation even if it does not fully address all corner cases and the reordering is not harmful so im ok with this i just wanted to highlight the delete works and add fails case so that people were generally aware of that when reviewing.\n\naddressing the architectural question i agree should be handled seperately so im going to change my review to a +1","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"6648b56c3985cd53e82ecc780ed9670751842fc4","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"},{"line_number":121,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(s)]"},{"line_number":122,"context_line":"            # First, get dev_obj_list from hostname"},{"line_number":123,"context_line":"            device_obj_list \u003d Device.get_list_by_hostname(context, host)"},{"line_number":124,"context_line":"            # Then, use controlpath_id.cpid_info to identiy one Device."},{"line_number":125,"context_line":"            cpid_info \u003d new_driver_dev_obj.controlpath_id.cpid_info"},{"line_number":126,"context_line":"            for dev_obj in device_obj_list:"},{"line_number":127,"context_line":"                # get cpid_obj, could be empty or only one value."},{"line_number":128,"context_line":"                cpid_obj \u003d ControlpathID.get_by_device_id_cpidinfo("},{"line_number":129,"context_line":"                    context, dev_obj.id, cpid_info)"},{"line_number":130,"context_line":"                # find the one cpid_obj with cpid_info"},{"line_number":131,"context_line":"                if cpid_obj is not None:"},{"line_number":132,"context_line":"                    break"},{"line_number":133,"context_line":""},{"line_number":134,"context_line":"            changed_key \u003d [\u0027std_board_info\u0027, \u0027vendor\u0027, \u0027vendor_board_info\u0027,"},{"line_number":135,"context_line":"                           \u0027model\u0027, \u0027type\u0027]"},{"line_number":136,"context_line":"            for c_k in changed_key:"},{"line_number":137,"context_line":"                if getattr(new_driver_dev_obj, c_k) !\u003d getattr("},{"line_number":138,"context_line":"                        old_driver_dev_obj, c_k):"},{"line_number":139,"context_line":"                    setattr(dev_obj, c_k, getattr(new_driver_dev_obj, c_k))"},{"line_number":140,"context_line":"            dev_obj.save(context)"},{"line_number":141,"context_line":"            # diff the internal layer: driver_deployable"},{"line_number":142,"context_line":"            self.drv_deployable_make_diff(context, dev_obj.id, cpid_obj.id,"},{"line_number":143,"context_line":"                                          old_driver_dev_obj.deployable_list,"},{"line_number":144,"context_line":"                                          new_driver_dev_obj.deployable_list,"},{"line_number":145,"context_line":"                                          host_rp)"},{"line_number":146,"context_line":""},{"line_number":147,"context_line":"    def drv_deployable_make_diff(self, context, device_id, cpid_id,"},{"line_number":148,"context_line":"                                 old_driver_dep_list, new_driver_dep_list,"}],"source_content_type":"text/x-python","patch_set":1,"id":"1fa4df85_1e80b8f4","line":145,"range":{"start_line":104,"start_character":7,"end_line":145,"end_character":50},"in_reply_to":"1fa4df85_60703909","updated":"2020-03-10 02:00:49.000000000","message":"Hi sean. Adjusting the order of the delete and add methods is helpful. Because the delete method first calls the placement interface, when the placement service is unavailable, the update to cyborg db will stop at this time, so that no intermediate dirty data will be generated. But this does not completely solve all data synchronization problems, because there may be such a situation:\nWhen the delete method is updated, but the placement service is unavailable when the add method is used, no matter what to do, dirty data will be generated in the middle.\nWith regard to fully addressing this issue, further discussion of the design of the architecture may be required. Please see my reply in the commit message, thank you.","commit_id":"e49f3c81296d76645a8b79e149aadb043b375712"},{"author":{"_account_id":26458,"name":"Brin Zhang","email":"zhangbailin@inspur.com","username":"zhangbailin"},"change_message_id":"a56b4d797a9b7593463cb3404c0c0ff0bb08388d","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        added \u003d set(new_cpid_list) - same - set(stub_cpid_list)"},{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_a6811f4d","line":117,"range":{"start_line":103,"start_character":0,"end_line":117,"end_character":66},"updated":"2020-03-13 07:24:45.000000000","message":"About delete and added check, do you thinking about abstracting out a private method? And explain the abstract method. Reduce code redundancy.","commit_id":"6633fa32e887a17770913ceadeb42152ee6eb5ec"},{"author":{"_account_id":28748,"name":"chenker","email":"chen.ke14@zte.com.cn","username":"chenke"},"change_message_id":"a4a846e8d81242d2c4976af6e83ab5a00e67da52","unresolved":false,"context_lines":[{"line_number":100,"context_line":"        added \u003d set(new_cpid_list) - same - set(stub_cpid_list)"},{"line_number":101,"context_line":"        deleted \u003d set(old_cpid_list) - same - set(stub_cpid_list)"},{"line_number":102,"context_line":"        host_rp \u003d self._get_root_provider(context, host)"},{"line_number":103,"context_line":"        # device is deleted."},{"line_number":104,"context_line":"        for d in deleted:"},{"line_number":105,"context_line":"            old_driver_dev_obj \u003d old_driver_device_list[old_cpid_list.index(d)]"},{"line_number":106,"context_line":"            for driver_dep_obj in old_driver_dev_obj.deployable_list:"},{"line_number":107,"context_line":"                rp_uuid \u003d self.get_rp_uuid_from_obj(driver_dep_obj)"},{"line_number":108,"context_line":"                self._delete_provider_and_sub_providers(context, rp_uuid)"},{"line_number":109,"context_line":"            old_driver_dev_obj.destroy(context, host)"},{"line_number":110,"context_line":"        # device is added"},{"line_number":111,"context_line":"        for a in added:"},{"line_number":112,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(a)]"},{"line_number":113,"context_line":"            new_driver_dev_obj.create(context, host)"},{"line_number":114,"context_line":"            for driver_dep_obj in new_driver_dev_obj.deployable_list:"},{"line_number":115,"context_line":"                self.get_placement_needed_info_and_report(context,"},{"line_number":116,"context_line":"                                                          driver_dep_obj,"},{"line_number":117,"context_line":"                                                          host_rp)"},{"line_number":118,"context_line":"        for s in same:"},{"line_number":119,"context_line":"            # get the driver_dev_obj, diff the driver_device layer"},{"line_number":120,"context_line":"            new_driver_dev_obj \u003d new_driver_device_list[new_cpid_list.index(s)]"}],"source_content_type":"text/x-python","patch_set":2,"id":"1fa4df85_cebe9bfe","line":117,"range":{"start_line":103,"start_character":0,"end_line":117,"end_character":66},"in_reply_to":"1fa4df85_a6811f4d","updated":"2020-03-16 01:43:10.000000000","message":"Yes, I thought about this too. We might find the current code a bit redundant, but readability is good. Personally, I think that abstracting it alone will not save a lot of code, but will make the logic of the code more difficult to understand. If you have a good idea or conclusion, you can submit a patch and we can review it together.","commit_id":"6633fa32e887a17770913ceadeb42152ee6eb5ec"}]}