)]}'
{"glance/api/v2/images.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"233105ed663d5b91783c05ecdfb16da955b62721","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"    def _bust_import_lock(self, admin_image_repo, admin_task_repo,"},{"line_number":104,"context_line":"                          image, task, task_id):"},{"line_number":105,"context_line":"        if task:"},{"line_number":106,"context_line":"            # FIXME(danms): It would be good if we had a \u0027canceled\u0027 or"},{"line_number":107,"context_line":"            # \u0027aborted\u0027 status here."},{"line_number":108,"context_line":"            task.fail(\u0027Expired lock preempted\u0027)"},{"line_number":109,"context_line":"            admin_task_repo.save(task)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_6cec038f","line":108,"range":{"start_line":105,"start_character":0,"end_line":108,"end_character":47},"updated":"2020-08-04 18:50:02.000000000","message":"We might get exception.InvalidTaskStatusTransition thrown at us here if the current status at this point of time is either \"failure\" or \"success\".","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"89e327ab6ccac6373e10b5acf76f90bc9896cdb0","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"    def _bust_import_lock(self, admin_image_repo, admin_task_repo,"},{"line_number":104,"context_line":"                          image, task, task_id):"},{"line_number":105,"context_line":"        if task:"},{"line_number":106,"context_line":"            # FIXME(danms): It would be good if we had a \u0027canceled\u0027 or"},{"line_number":107,"context_line":"            # \u0027aborted\u0027 status here."},{"line_number":108,"context_line":"            task.fail(\u0027Expired lock preempted\u0027)"},{"line_number":109,"context_line":"            admin_task_repo.save(task)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_f7e0dc28","line":108,"range":{"start_line":105,"start_character":0,"end_line":108,"end_character":47},"in_reply_to":"9f560f44_6cec038f","updated":"2020-08-04 19:07:00.000000000","message":"Ah, right, so if we got to one of those terminal states but didn\u0027t drop our lock, it\u0027s probably that actually did finish and failed to drop it for some error case reason. So, can I just ignore (but log) that failure here?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"ae394e86876b12d4de470d2d2266304f71ea5235","unresolved":false,"context_lines":[{"line_number":102,"context_line":""},{"line_number":103,"context_line":"    def _bust_import_lock(self, admin_image_repo, admin_task_repo,"},{"line_number":104,"context_line":"                          image, task, task_id):"},{"line_number":105,"context_line":"        if task:"},{"line_number":106,"context_line":"            # FIXME(danms): It would be good if we had a \u0027canceled\u0027 or"},{"line_number":107,"context_line":"            # \u0027aborted\u0027 status here."},{"line_number":108,"context_line":"            task.fail(\u0027Expired lock preempted\u0027)"},{"line_number":109,"context_line":"            admin_task_repo.save(task)"},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_4d8ac269","line":108,"range":{"start_line":105,"start_character":0,"end_line":108,"end_character":47},"in_reply_to":"9f560f44_f7e0dc28","updated":"2020-08-05 22:33:20.000000000","message":"That or another is racing with us to it. So we probably blow up here before hitting the L 114","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1b12912fff1635e7b82827756e3d78ada4ea39ba","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            age \u003d oslo_timeutils.utcnow() - task.updated_at"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        if not task or (task.status in bustable_states and age \u003e\u003d expiry):"},{"line_number":156,"context_line":"            self._bust_import_lock(admin_image_repo, admin_task_repo,"},{"line_number":157,"context_line":"                                   image, task, other_task)"},{"line_number":158,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_89f069ff","line":155,"range":{"start_line":155,"start_character":24,"end_line":155,"end_character":54},"updated":"2020-08-04 16:55:13.000000000","message":"I guess \"processing\" is not a task state (correct me if I am wrong) it is running instead.\n\nNow consider task state is \"pending\" i.e. taskflow is build lock is acquired and something went wrong and that task never go in running state, then with this condition it will never release lock for that image without admin intervention?\n\nSame thing might happen with reverted state?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"89e327ab6ccac6373e10b5acf76f90bc9896cdb0","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            age \u003d oslo_timeutils.utcnow() - task.updated_at"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        if not task or (task.status in bustable_states and age \u003e\u003d expiry):"},{"line_number":156,"context_line":"            self._bust_import_lock(admin_image_repo, admin_task_repo,"},{"line_number":157,"context_line":"                                   image, task, other_task)"},{"line_number":158,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_97c9a0ad","line":155,"range":{"start_line":155,"start_character":24,"end_line":155,"end_character":54},"in_reply_to":"9f560f44_4c63ff42","updated":"2020-08-04 19:07:00.000000000","message":"\u003e What comes to the pending, it was my concern when we were talking\n \u003e about this with Abhishek last week. The time_to_live only applies\n \u003e to the task (and thus gets cleaned up) [1] once it hits either\n \u003e \"success\" or \"failure\" so pending will leave it locked until manual\n \u003e action is taken.\n\nAck. Can we maybe have a longer timeout for the bust-on-pending case then?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"33746445e3bdc92e9e77aa078be236533302170b","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            age \u003d oslo_timeutils.utcnow() - task.updated_at"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        if not task or (task.status in bustable_states and age \u003e\u003d expiry):"},{"line_number":156,"context_line":"            self._bust_import_lock(admin_image_repo, admin_task_repo,"},{"line_number":157,"context_line":"                                   image, task, other_task)"},{"line_number":158,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_8c06b71b","line":155,"range":{"start_line":155,"start_character":24,"end_line":155,"end_character":54},"in_reply_to":"9f560f44_89f069ff","updated":"2020-08-04 17:46:32.000000000","message":"I think it\u0027s \"processing\":\n https://github.com/openstack/glance/blob/master/glance/api/v2/tasks.py#L172\n\nBut yeah, if it\u0027s stuck in pending state I don\u0027t break the lock. I thought we discussed here:\n\n https://etherpad.opendev.org/p/glance-import-locking\n\n...that we would only bust the lock on \"processing\" state (and success/fail are obvious). I think the reason to avoid busting pending is that the task can\u0027t be heartbeating if it\u0027s not yet running. That may be because we failed to start the task at all, or because it\u0027s queued (either blocked from running due to resources, or in some future where there\u0027s actually a queue) and it\u0027s going to start. I suppose that with the obsessive task lock checking in the import flow, maybe we could enable busting in \u0027pending\u0027 here and the task would abort super quick if it started running after the lock got busted.\n\nI dunno, I felt like if we\u0027re not starting tasks we\u0027re probably broken for other reasons and I wanted to be very specific about the cases when and where the lock busting happens.\n\nIf the task is stuck in pending due to some really bad failure, the task will eventually be culled by the cleanup I assume and thus will become unlocked at some point? Or does that never happen for tasks in that state?\n\nAnyway, I dunno, maybe we get some input from others?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"233105ed663d5b91783c05ecdfb16da955b62721","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            age \u003d oslo_timeutils.utcnow() - task.updated_at"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        if not task or (task.status in bustable_states and age \u003e\u003d expiry):"},{"line_number":156,"context_line":"            self._bust_import_lock(admin_image_repo, admin_task_repo,"},{"line_number":157,"context_line":"                                   image, task, other_task)"},{"line_number":158,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_4c63ff42","line":155,"range":{"start_line":155,"start_character":24,"end_line":155,"end_character":54},"in_reply_to":"9f560f44_8c06b71b","updated":"2020-08-04 18:50:02.000000000","message":"I guess it would have been too much to expect that we would have actually used the same states for the tasks as taskflow does [0] :(\n\nWhat comes to the pending, it was my concern when we were talking about this with Abhishek last week. The time_to_live only applies to the task (and thus gets cleaned up) [1] once it hits either \"success\" or \"failure\" so pending will leave it locked until manual action is taken. \n\n[0] https://github.com/openstack/glance/blob/master/glance/domain/__init__.py#L353-L404\n[1] https://github.com/openstack/glance/blob/master/glance/domain/__init__.py#L373-L375","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"ae394e86876b12d4de470d2d2266304f71ea5235","unresolved":false,"context_lines":[{"line_number":152,"context_line":"        else:"},{"line_number":153,"context_line":"            age \u003d oslo_timeutils.utcnow() - task.updated_at"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":"        if not task or (task.status in bustable_states and age \u003e\u003d expiry):"},{"line_number":156,"context_line":"            self._bust_import_lock(admin_image_repo, admin_task_repo,"},{"line_number":157,"context_line":"                                   image, task, other_task)"},{"line_number":158,"context_line":"            return"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_cdac12ef","line":155,"range":{"start_line":155,"start_character":24,"end_line":155,"end_character":54},"in_reply_to":"9f560f44_97c9a0ad","updated":"2020-08-05 22:33:20.000000000","message":"Sounds good to me. With normal eventlet running case something should be wrong if this happens but handful of threads with uwsgi might cause us sitting in pending for a while.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"233105ed663d5b91783c05ecdfb16da955b62721","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                         \u0027status\u0027: task.status,"},{"line_number":167,"context_line":"                         \u0027expire\u0027: (expiry - age).total_seconds()})"},{"line_number":168,"context_line":"        else:"},{"line_number":169,"context_line":"            LOG.warning(\u0027Image %(image)s has import task %(task)s in status \u0027"},{"line_number":170,"context_line":"                        \u0027%(status)s and does not qualify for expiry.\u0027)"},{"line_number":171,"context_line":"        raise exception.Conflict(\u0027Image has active task\u0027)"},{"line_number":172,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_5700a801","line":169,"range":{"start_line":169,"start_character":16,"end_line":169,"end_character":23},"updated":"2020-08-04 18:50:02.000000000","message":"Am I missing something or is this warning not warranted? Isn\u0027t this what would be locked if we get multiple concurrent requests for copy-image and the processing just haven\u0027t started yet even if it\u0027s freshly new. Like for those 200 concurrent requests nova would be sending.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"89e327ab6ccac6373e10b5acf76f90bc9896cdb0","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                         \u0027status\u0027: task.status,"},{"line_number":167,"context_line":"                         \u0027expire\u0027: (expiry - age).total_seconds()})"},{"line_number":168,"context_line":"        else:"},{"line_number":169,"context_line":"            LOG.warning(\u0027Image %(image)s has import task %(task)s in status \u0027"},{"line_number":170,"context_line":"                        \u0027%(status)s and does not qualify for expiry.\u0027)"},{"line_number":171,"context_line":"        raise exception.Conflict(\u0027Image has active task\u0027)"},{"line_number":172,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_973ec088","line":169,"range":{"start_line":169,"start_character":16,"end_line":169,"end_character":23},"in_reply_to":"9f560f44_5700a801","updated":"2020-08-04 19:07:00.000000000","message":"Yes, you\u0027d hit this in the race case. I had this here so I could see something happen when I poked it from a test, and I\u0027ve been trying to log *lots* of information around this. Warning is probably too strong though; do you want no logging in this case or info/debug?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"ae394e86876b12d4de470d2d2266304f71ea5235","unresolved":false,"context_lines":[{"line_number":166,"context_line":"                         \u0027status\u0027: task.status,"},{"line_number":167,"context_line":"                         \u0027expire\u0027: (expiry - age).total_seconds()})"},{"line_number":168,"context_line":"        else:"},{"line_number":169,"context_line":"            LOG.warning(\u0027Image %(image)s has import task %(task)s in status \u0027"},{"line_number":170,"context_line":"                        \u0027%(status)s and does not qualify for expiry.\u0027)"},{"line_number":171,"context_line":"        raise exception.Conflict(\u0027Image has active task\u0027)"},{"line_number":172,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_8da7ba0b","line":169,"range":{"start_line":169,"start_character":16,"end_line":169,"end_character":23},"in_reply_to":"9f560f44_973ec088","updated":"2020-08-05 22:33:20.000000000","message":"debug would be fine. Just don\u0027t want to flood the logs in production with something that is absolute noop for admin and likely to occur.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"c0d72661b5e13760c1f425ae290d75c8418ffcf8","unresolved":false,"context_lines":[{"line_number":228,"context_line":"                raise webob.exc.HTTPForbidden("},{"line_number":229,"context_line":"                    explanation\u003d_(\"Operation not permitted\"))"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"            if \u0027os_glance_import_task\u0027 in image.extra_properties:"},{"line_number":232,"context_line":"                # NOTE(danms): This will raise exception.Conflict if the"},{"line_number":233,"context_line":"                # lock is present and valid, or return if absent or invalid."},{"line_number":234,"context_line":"                self._enforce_import_lock(req, image)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f560f44_f66eecc6","line":231,"range":{"start_line":231,"start_character":16,"end_line":231,"end_character":37},"updated":"2020-08-07 06:11:57.000000000","message":"we also need to block this property during image-update and should not show it to end user right?","commit_id":"580bab3845a8abb8a0d3b89f3f1909dba133c951"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b06bcfeeea4d0310c51048a38ffc4b1ac51b3e4","unresolved":false,"context_lines":[{"line_number":228,"context_line":"                raise webob.exc.HTTPForbidden("},{"line_number":229,"context_line":"                    explanation\u003d_(\"Operation not permitted\"))"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"            if \u0027os_glance_import_task\u0027 in image.extra_properties:"},{"line_number":232,"context_line":"                # NOTE(danms): This will raise exception.Conflict if the"},{"line_number":233,"context_line":"                # lock is present and valid, or return if absent or invalid."},{"line_number":234,"context_line":"                self._enforce_import_lock(req, image)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f560f44_39acd0eb","line":231,"range":{"start_line":231,"start_character":16,"end_line":231,"end_character":37},"in_reply_to":"9f560f44_f66eecc6","updated":"2020-08-07 13:33:04.000000000","message":"Yes, we do need to block it from update. Personally, I\u0027d like to see it exposed to the user because it gives them a pointer to be able to go look at the task. Since the task gives granular information about progress now, it seems like it would be good to just expose it.","commit_id":"580bab3845a8abb8a0d3b89f3f1909dba133c951"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"ec418bd698ad3bf2e976d65dbb0aaeba1810b8ae","unresolved":false,"context_lines":[{"line_number":140,"context_line":"        admin_context \u003d req.context.elevated()"},{"line_number":141,"context_line":"        admin_image_repo \u003d self.gateway.get_repo(admin_context)"},{"line_number":142,"context_line":"        admin_task_repo \u003d self.gateway.get_task_repo(admin_context)"},{"line_number":143,"context_line":"        other_task \u003d image.extra_properties[\u0027os_glance_import_task\u0027]"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"        expiry \u003d datetime.timedelta(minutes\u003d60)"},{"line_number":146,"context_line":"        bustable_states \u003d (\u0027pending\u0027, \u0027processing\u0027, \u0027success\u0027, \u0027failure\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_c67f9912","line":143,"range":{"start_line":143,"start_character":8,"end_line":143,"end_character":18},"updated":"2020-08-10 17:38:37.000000000","message":"may be existing_task?","commit_id":"f62b959efbea79c35776eacbbd13933d113fc39e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"59984089ba7712e8b03c5ec8c945004ab7f38c31","unresolved":false,"context_lines":[{"line_number":140,"context_line":"        admin_context \u003d req.context.elevated()"},{"line_number":141,"context_line":"        admin_image_repo \u003d self.gateway.get_repo(admin_context)"},{"line_number":142,"context_line":"        admin_task_repo \u003d self.gateway.get_task_repo(admin_context)"},{"line_number":143,"context_line":"        other_task \u003d image.extra_properties[\u0027os_glance_import_task\u0027]"},{"line_number":144,"context_line":""},{"line_number":145,"context_line":"        expiry \u003d datetime.timedelta(minutes\u003d60)"},{"line_number":146,"context_line":"        bustable_states \u003d (\u0027pending\u0027, \u0027processing\u0027, \u0027success\u0027, \u0027failure\u0027)"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_69713ea8","line":143,"range":{"start_line":143,"start_character":8,"end_line":143,"end_character":18},"in_reply_to":"9f560f44_c67f9912","updated":"2020-08-10 17:59:04.000000000","message":"Sure.","commit_id":"f62b959efbea79c35776eacbbd13933d113fc39e"}],"glance/async_/flows/api_image_import.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5344d4657260caf294fa48fc4bb5daabc70b652b","unresolved":false,"context_lines":[{"line_number":346,"context_line":"    def execute(self):"},{"line_number":347,"context_line":"        pass"},{"line_number":348,"context_line":""},{"line_number":349,"context_line":"    def revert(self, result, **kwargs):"},{"line_number":350,"context_line":"        \"\"\"Drop our claim on the image."},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"        If we have failed, we need to drop our import_task lock on the image"},{"line_number":353,"context_line":"        so that something else can have a try. Note that we may have been"},{"line_number":354,"context_line":"        preempted so we should only drop *our* lock."},{"line_number":355,"context_line":"        \"\"\""},{"line_number":356,"context_line":"        try:"},{"line_number":357,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":358,"context_line":"        except exception.NotFound:"},{"line_number":359,"context_line":"            LOG.warning(\u0027Image %(image)s import task %(task)s lost its \u0027"},{"line_number":360,"context_line":"                        \u0027lock during execution!\u0027,"},{"line_number":361,"context_line":"                        {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":362,"context_line":"                         \u0027task\u0027: self.task_id})"},{"line_number":363,"context_line":"        else:"},{"line_number":364,"context_line":"            LOG.debug(\u0027Image %(image)s import task %(task)s dropped \u0027"},{"line_number":365,"context_line":"                      \u0027its lock after failure\u0027,"},{"line_number":366,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":367,"context_line":"                       \u0027task\u0027: self.task_id})"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":""},{"line_number":370,"context_line":"class _VerifyStaging(task.Task):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_fa29339b","line":367,"range":{"start_line":349,"start_character":4,"end_line":367,"end_character":45},"updated":"2020-07-30 08:53:26.000000000","message":"I got the basic idea why this class is added so that we can avoid writing same code in all the reverts of each task including internal plugins but,\n\nIMO this is something which is breaking basic idea of taskflow. AFAIK, task should revert only those changes which were performed by the task.","commit_id":"551a7264a3ab1868eaf7281047bec2e0cd357d47"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"14419b7fbbf4df4747370c33f8ddf5c3ab7fbb6d","unresolved":false,"context_lines":[{"line_number":346,"context_line":"    def execute(self):"},{"line_number":347,"context_line":"        pass"},{"line_number":348,"context_line":""},{"line_number":349,"context_line":"    def revert(self, result, **kwargs):"},{"line_number":350,"context_line":"        \"\"\"Drop our claim on the image."},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"        If we have failed, we need to drop our import_task lock on the image"},{"line_number":353,"context_line":"        so that something else can have a try. Note that we may have been"},{"line_number":354,"context_line":"        preempted so we should only drop *our* lock."},{"line_number":355,"context_line":"        \"\"\""},{"line_number":356,"context_line":"        try:"},{"line_number":357,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":358,"context_line":"        except exception.NotFound:"},{"line_number":359,"context_line":"            LOG.warning(\u0027Image %(image)s import task %(task)s lost its \u0027"},{"line_number":360,"context_line":"                        \u0027lock during execution!\u0027,"},{"line_number":361,"context_line":"                        {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":362,"context_line":"                         \u0027task\u0027: self.task_id})"},{"line_number":363,"context_line":"        else:"},{"line_number":364,"context_line":"            LOG.debug(\u0027Image %(image)s import task %(task)s dropped \u0027"},{"line_number":365,"context_line":"                      \u0027its lock after failure\u0027,"},{"line_number":366,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":367,"context_line":"                       \u0027task\u0027: self.task_id})"},{"line_number":368,"context_line":""},{"line_number":369,"context_line":""},{"line_number":370,"context_line":"class _VerifyStaging(task.Task):"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_baccd6bc","line":367,"range":{"start_line":349,"start_character":4,"end_line":367,"end_character":45},"in_reply_to":"9f560f44_fa29339b","updated":"2020-07-30 13:33:09.000000000","message":"\u003e I got the basic idea why this class is added so that we can avoid\n \u003e writing same code in all the reverts of each task including\n \u003e internal plugins but,\n\nThat\u0027s not really the reason. We wouldn\u0027t want to put this code in the revert of every task because we just want this to run once and always at the end of revert without fail, right?\n\n \u003e IMO this is something which is breaking basic idea of taskflow.\n \u003e AFAIK, task should revert only those changes which were performed\n \u003e by the task.\n\nI know that\u0027s the ideal, but since nothing in the flow grabbed the lock, there is no place to mirror that in the removal during a revert. I certainly don\u0027t think we should avoid having a consistent API or a race-free import thread because of an ideal design goal like this.\n\nThe alternative would be to let the flow do the locking and block the API from returning until it got a report from the thread that it was successful or not in grabbing the lock. I could certainly do that, but it seems crazily over-complicated just to avoid having a task that only does a revert on cleanup. Is that really worth it?\n\nWhat if we made the execute() of this task just *confirm* the lock that the API grabbed for us? Would that make this more balanced and alleviate that concern?","commit_id":"551a7264a3ab1868eaf7281047bec2e0cd357d47"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1b12912fff1635e7b82827756e3d78ada4ea39ba","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                                \u0027os_glance_import_task\u0027,"},{"line_number":137,"context_line":"                                                self._task_id)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _assert_task_lock(self, image):"},{"line_number":140,"context_line":"        task_lock \u003d image.extra_properties.get(\u0027os_glance_import_task\u0027)"},{"line_number":141,"context_line":"        if task_lock !\u003d self._task_id:"},{"line_number":142,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s attempted to \u0027"},{"line_number":143,"context_line":"                      \u0027take action on image, but other task %(other)s holds \u0027"},{"line_number":144,"context_line":"                      \u0027the lock; Aborting.\u0027,"},{"line_number":145,"context_line":"                      {\u0027image\u0027: self._image_id,"},{"line_number":146,"context_line":"                       \u0027task\u0027: self._task_id,"},{"line_number":147,"context_line":"                       \u0027other\u0027: task_lock})"},{"line_number":148,"context_line":"            raise exception.TaskAbortedError()"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"    def assert_task_lock(self):"},{"line_number":151,"context_line":"        \"\"\"Assert that we own the task lock on the image."},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        :raises: TaskAbortedError if we do not"},{"line_number":154,"context_line":"        \"\"\""},{"line_number":155,"context_line":"        image \u003d self._image_repo.get(self._image_id)"},{"line_number":156,"context_line":"        self._assert_task_lock(image)"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"class _ImportActions(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_293ddd8e","line":156,"range":{"start_line":139,"start_character":4,"end_line":156,"end_character":37},"updated":"2020-08-04 16:55:13.000000000","message":"should we combine this in one function unless _asser_task_lock is used somewhere else? and if it is should we atleast change the name as it seems same.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"33746445e3bdc92e9e77aa078be236533302170b","unresolved":false,"context_lines":[{"line_number":136,"context_line":"                                                \u0027os_glance_import_task\u0027,"},{"line_number":137,"context_line":"                                                self._task_id)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"    def _assert_task_lock(self, image):"},{"line_number":140,"context_line":"        task_lock \u003d image.extra_properties.get(\u0027os_glance_import_task\u0027)"},{"line_number":141,"context_line":"        if task_lock !\u003d self._task_id:"},{"line_number":142,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s attempted to \u0027"},{"line_number":143,"context_line":"                      \u0027take action on image, but other task %(other)s holds \u0027"},{"line_number":144,"context_line":"                      \u0027the lock; Aborting.\u0027,"},{"line_number":145,"context_line":"                      {\u0027image\u0027: self._image_id,"},{"line_number":146,"context_line":"                       \u0027task\u0027: self._task_id,"},{"line_number":147,"context_line":"                       \u0027other\u0027: task_lock})"},{"line_number":148,"context_line":"            raise exception.TaskAbortedError()"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"    def assert_task_lock(self):"},{"line_number":151,"context_line":"        \"\"\"Assert that we own the task lock on the image."},{"line_number":152,"context_line":""},{"line_number":153,"context_line":"        :raises: TaskAbortedError if we do not"},{"line_number":154,"context_line":"        \"\"\""},{"line_number":155,"context_line":"        image \u003d self._image_repo.get(self._image_id)"},{"line_number":156,"context_line":"        self._assert_task_lock(image)"},{"line_number":157,"context_line":""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"class _ImportActions(object):"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_2c8a0b69","line":156,"range":{"start_line":139,"start_character":4,"end_line":156,"end_character":37},"in_reply_to":"9f560f44_293ddd8e","updated":"2020-08-04 17:46:32.000000000","message":"The inner is used on L104 above, since we\u0027re already fetching the image in that case.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"233105ed663d5b91783c05ecdfb16da955b62721","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_f7b29c48","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"updated":"2020-08-04 18:50:02.000000000","message":"We likely won\u0027t ever get here in such case. If the Transition fails on line 611, it will also fail on line 625 which is not caught and either case will trigger the whole flow being reverted.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"6f0a81ccdea4d7eac7927165cc112876b706ec53","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_ed4bd627","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"in_reply_to":"9f560f44_1700f0aa","updated":"2020-08-05 22:44:22.000000000","message":"No, so that\u0027s what makes splitting thigs into bunch of internal functions hard to follow. So on L 649 you call the _finish_task, which will likely fail on L 611 and then trigger revert failing on line 625 so _drop_lock will never be called in the case we\u0027re hitting the race that would cause this NotFound. If someone else tried to bust the lock in between _finish_task and _drop_lock, they should have get caught on bustable state at that point.\n\nSo unless I missed something there literally should not be way to hit this piece of code ever in any other way than mocking the behaviour to the state that would never happen in reality.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"9c019b7ea28fb868f9501d5421e16e33dd6f0c67","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_cda4a9bc","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"in_reply_to":"9f560f44_4c111604","updated":"2020-08-13 15:31:01.000000000","message":"\u003e Here\u0027s the real-world scenario where this can happen:\n \u003e \n \u003e 1. We call task.succeed() on L611, which actually happens\n \u003e 2. While waiting for the database call to return we switch to\n \u003e another local thread (green or not)\n \u003e 3. That thread looks up the task, finds that its lock is bustable,\n \u003e busts it, and grabs the lock itself\n\nBut how does it get the task in a state that is bustable. If we succeed on the succeed() call, we already have the update queued in DB, when we query that same row after, we will get the updated value, no? So we should not get bustable state in locking code. If the lock busting happens first, then we\u0027re on the same situation opposite way around the task.succeed() call will fail, we catch that, we try to call taks.fail() which will also fail and we jump into revert with never reaching the code that was supposed to prevent reverting at this point anymore.\n\n \u003e 4. We get scheduled because our call has returned and move on to\n \u003e drop_lock_for_task(), which does not find our lock.\n \u003e \n \u003e It\u0027s really not related to splitting things into a bunch of\n \u003e functions, it\u0027s just that in a distributed system you can\u0027t rely on\n \u003e everything being atomic and orderly. This scenario could have\n \u003e happened in the case where we grab the lock as the first task in\n \u003e the flow too.\n\nYes only thing I was referring to around splitting the code was as a human it\u0027s not easy to follow what happens when you need to bounce around. Not that it actually affects the execution itself.\n\nNow this whole situation also requires that our execution is scheduled to other thread long enough that getting over _SaveImage (which is either no-op or single query) has taken more time than our timeout and we expect the other thread not being impacted by that.\n\nMy whole point is that timing wise, _taken_ that all these things happens, we\u0027re much more likely to hit InvalidTaskStateTransition in the _finish_task and not address that and go to revert than catching up here which specifically states on the comment that it prevents us reverting if we already got this far. I wasn\u0027t saying logging it here is necessary bad thing, but not catching the condition already in _finish_task() is and likely prevents us ever getting here.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"53990fb7a832075b5e8421bd6d57e8cd783cd2f8","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_ad9175f4","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"in_reply_to":"9f560f44_cda4a9bc","updated":"2020-08-13 15:45:56.000000000","message":"\u003e \u003e Here\u0027s the real-world scenario where this can happen:\n \u003e \u003e\n \u003e \u003e 1. We call task.succeed() on L611, which actually happens\n \u003e \u003e 2. While waiting for the database call to return we switch to\n \u003e \u003e another local thread (green or not)\n \u003e \u003e 3. That thread looks up the task, finds that its lock is\n \u003e bustable,\n \u003e \u003e busts it, and grabs the lock itself\n \u003e \n \u003e But how does it get the task in a state that is bustable. If we\n \u003e succeed on the succeed() call, we already have the update queued in\n \u003e DB, when we query that same row after, we will get the updated\n \u003e value, no?\n\nYou mean the other thread reads the task state and sees \u0027success\u0027 right? It surely will, but the lock will still be set in image.os_glance_import_task, which means that thread is still going to check it for bust-ability. If suitable delay happens between step 2 and 3, the lock could be bustable by the time step 3 happens. When we finally get to run our step 4, we will fail to delete our lock from the instance, because it is gone.\n\nMy point was that you\u0027ve got multiple steps here: Marking the task as success and dropping the lock from the image. Those don\u0027t happen atomically, which means at any point some other thread can see the task marked as success but the image still locked, and that lock will be bustable if the delay between marking the task as success (the last update to the task\u0027s updated_at) and the dropping of our lock here is long enough.\n\n \u003e So we should not get bustable state in locking code. If\n \u003e the lock busting happens first, then we\u0027re on the same situation\n \u003e opposite way around the task.succeed() call will fail, we catch\n \u003e that, we try to call taks.fail() which will also fail and we jump\n \u003e into revert with never reaching the code that was supposed to\n \u003e prevent reverting at this point anymore.\n\n \u003e My whole point is that timing wise, _taken_ that all these things\n \u003e happens, we\u0027re much more likely to hit InvalidTaskStateTransition\n \u003e in the _finish_task and not address that and go to revert than\n \u003e catching up here which specifically states on the comment that it\n \u003e prevents us reverting if we already got this far. I wasn\u0027t saying\n \u003e logging it here is necessary bad thing, but not catching the\n \u003e condition already in _finish_task() is and likely prevents us ever\n \u003e getting here.\n\nIf we fail to mark the task as successful, then we *should* call the revert path, right? If task.succeed() fails, we\u0027ll enter the exception handler, task.fail() will fail as well, which means our revert will be called, which doesn\u0027t do anything, but will chain backwards to the revert()s of previous steps in the flow. Isn\u0027t that desired?\n\nOr are you saying that we should just not fail the flow if task.succeed() fails because we\u0027ve made it this far? That doesn\u0027t seem like a good plan since the task will be exposed publicly as \"this thing failed\" and yet, the work of the task was not reverted.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"003d41d761d6cc5f0e38b05c9eecb2a057be1593","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_4c111604","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"in_reply_to":"9f560f44_ed4bd627","updated":"2020-08-06 17:30:05.000000000","message":"Here\u0027s the real-world scenario where this can happen:\n\n 1. We call task.succeed() on L611, which actually happens\n 2. While waiting for the database call to return we switch to another local thread (green or not)\n 3. That thread looks up the task, finds that its lock is bustable, busts it, and grabs the lock itself\n 4. We get scheduled because our call has returned and move on to drop_lock_for_task(), which does not find our lock.\n\nIt\u0027s really not related to splitting things into a bunch of functions, it\u0027s just that in a distributed system you can\u0027t rely on everything being atomic and orderly. This scenario could have happened in the case where we grab the lock as the first task in the flow too.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"89e327ab6ccac6373e10b5acf76f90bc9896cdb0","unresolved":false,"context_lines":[{"line_number":630,"context_line":"    def _drop_lock(self):"},{"line_number":631,"context_line":"        try:"},{"line_number":632,"context_line":"            self.action_wrapper.drop_lock_for_task()"},{"line_number":633,"context_line":"        except exception.NotFound:"},{"line_number":634,"context_line":"            # NOTE(danms): This would be really bad, but there is probably"},{"line_number":635,"context_line":"            # not much point in reverting all the way back if we got this"},{"line_number":636,"context_line":"            # far. Log the carnage for forensics."},{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_1700f0aa","line":636,"range":{"start_line":633,"start_character":8,"end_line":636,"end_character":49},"in_reply_to":"9f560f44_f7b29c48","updated":"2020-08-04 19:07:00.000000000","message":"AFAICT, there isn\u0027t any DB-specific atomicity around the task state transitions, so I think that we could race between completing the task and trying to drop the lock here right? Are you saying that can\u0027t happen for some reason, or that it\u0027s really unlikely? If the latter, I agree, but I wanted to log the case if we ever hit it. Is there some reason we should not do that?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1b12912fff1635e7b82827756e3d78ada4ea39ba","unresolved":false,"context_lines":[{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":640,"context_line":"                       \u0027task\u0027: self.task_id})"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def execute(self):"},{"line_number":643,"context_line":"        \"\"\"Finishing the task flow"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_e94e4536","line":640,"updated":"2020-08-04 16:55:13.000000000","message":"should we add debug message here in else: saying Lock has been released or something like that?","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"16bf6e1bdf65c040c96804a57e71320e5f078c4c","unresolved":false,"context_lines":[{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":640,"context_line":"                       \u0027task\u0027: self.task_id})"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def execute(self):"},{"line_number":643,"context_line":"        \"\"\"Finishing the task flow"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_f6554cfb","line":640,"in_reply_to":"9f560f44_cc98afc1","updated":"2020-08-07 06:07:56.000000000","message":"Yes, that makes sense.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"33746445e3bdc92e9e77aa078be236533302170b","unresolved":false,"context_lines":[{"line_number":637,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s did not hold the \u0027"},{"line_number":638,"context_line":"                      \u0027lock upon completion!\u0027,"},{"line_number":639,"context_line":"                      {\u0027image\u0027: self.action_wrapper.image_id,"},{"line_number":640,"context_line":"                       \u0027task\u0027: self.task_id})"},{"line_number":641,"context_line":""},{"line_number":642,"context_line":"    def execute(self):"},{"line_number":643,"context_line":"        \"\"\"Finishing the task flow"}],"source_content_type":"text/x-python","patch_set":8,"id":"9f560f44_cc98afc1","line":640,"in_reply_to":"9f560f44_e94e4536","updated":"2020-08-04 17:46:32.000000000","message":"In the non-failure case you mean? We certainly can, but we already LOG.info() right after this that the task is completed. I doubt an operator is going to know (or need to know) the difference.","commit_id":"84b86597c921f2a05881e803280b989da025a82b"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"ec418bd698ad3bf2e976d65dbb0aaeba1810b8ae","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                                                self._task_id)"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    def _assert_task_lock(self, image):"},{"line_number":147,"context_line":"        task_lock \u003d image.extra_properties.get(\u0027os_glance_import_task\u0027)"},{"line_number":148,"context_line":"        if task_lock !\u003d self._task_id:"},{"line_number":149,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s attempted to \u0027"},{"line_number":150,"context_line":"                      \u0027take action on image, but other task %(other)s holds \u0027"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_26bdb5b6","line":147,"range":{"start_line":147,"start_character":8,"end_line":147,"end_character":17},"updated":"2020-08-10 17:38:37.000000000","message":"may be existing_task?","commit_id":"f62b959efbea79c35776eacbbd13933d113fc39e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"59984089ba7712e8b03c5ec8c945004ab7f38c31","unresolved":false,"context_lines":[{"line_number":144,"context_line":"                                                self._task_id)"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    def _assert_task_lock(self, image):"},{"line_number":147,"context_line":"        task_lock \u003d image.extra_properties.get(\u0027os_glance_import_task\u0027)"},{"line_number":148,"context_line":"        if task_lock !\u003d self._task_id:"},{"line_number":149,"context_line":"            LOG.error(\u0027Image %(image)s import task %(task)s attempted to \u0027"},{"line_number":150,"context_line":"                      \u0027take action on image, but other task %(other)s holds \u0027"}],"source_content_type":"text/x-python","patch_set":15,"id":"9f560f44_c96b2ab4","line":147,"range":{"start_line":147,"start_character":8,"end_line":147,"end_character":17},"in_reply_to":"9f560f44_26bdb5b6","updated":"2020-08-10 17:59:04.000000000","message":"Also sure :)","commit_id":"f62b959efbea79c35776eacbbd13933d113fc39e"}],"glance/tests/functional/v2/test_images_import_locking.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"42503c3ecd20574280cca147ef2b9ae4422c834f","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        else:"},{"line_number":228,"context_line":"            self.assertEqual(409, resp.status_code)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"    def test_import_copy_locked(self):"},{"line_number":231,"context_line":"        self._test_import_copy(warp_time\u003dFalse)"},{"line_number":232,"context_line":""},{"line_number":233,"context_line":"    def test_import_copy_bust_lock(self):"},{"line_number":234,"context_line":"        self._test_import_copy(warp_time\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":12,"id":"9f560f44_36a504a3","line":234,"range":{"start_line":230,"start_character":0,"end_line":234,"end_character":46},"updated":"2020-08-07 06:45:04.000000000","message":"I like these tests :D","commit_id":"580bab3845a8abb8a0d3b89f3f1909dba133c951"}],"glance/tests/unit/async_/test_async.py":[{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"fbe1a36308287adc67db953b0c7e218580a57b28","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            \u0027glance-direct\u0027, \u0027web-download\u0027, \u0027copy-image\u0027])"},{"line_number":80,"context_line":"        self.config(node_staging_uri\u003d\u0027file:///tmp/staging\u0027)"},{"line_number":81,"context_line":"        store.create_stores(CONF)"},{"line_number":82,"context_line":"        self.base_flow \u003d [\u0027ConfigureStaging\u0027, \u0027ImportToStore\u0027,"},{"line_number":83,"context_line":"                          \u0027DeleteFromFS\u0027, \u0027VerifyImageState\u0027,"},{"line_number":84,"context_line":"                          \u0027CompleteTask\u0027]"},{"line_number":85,"context_line":"        self.import_plugins \u003d [\u0027Convert_Image\u0027,"},{"line_number":86,"context_line":"                               \u0027Decompress_Image\u0027,"},{"line_number":87,"context_line":"                               \u0027InjectMetadataProperties\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_1a1f8c5b","line":84,"range":{"start_line":82,"start_character":8,"end_line":84,"end_character":41},"updated":"2020-08-20 21:38:13.000000000","message":"I suggest adding \u0027ImageLock\u0027 here.","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3ce9f83c4d3d53438b9257e1ef51d91d482beb7a","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            \u0027glance-direct\u0027, \u0027web-download\u0027, \u0027copy-image\u0027])"},{"line_number":80,"context_line":"        self.config(node_staging_uri\u003d\u0027file:///tmp/staging\u0027)"},{"line_number":81,"context_line":"        store.create_stores(CONF)"},{"line_number":82,"context_line":"        self.base_flow \u003d [\u0027ConfigureStaging\u0027, \u0027ImportToStore\u0027,"},{"line_number":83,"context_line":"                          \u0027DeleteFromFS\u0027, \u0027VerifyImageState\u0027,"},{"line_number":84,"context_line":"                          \u0027CompleteTask\u0027]"},{"line_number":85,"context_line":"        self.import_plugins \u003d [\u0027Convert_Image\u0027,"},{"line_number":86,"context_line":"                               \u0027Decompress_Image\u0027,"},{"line_number":87,"context_line":"                               \u0027InjectMetadataProperties\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_35c9c358","line":84,"range":{"start_line":82,"start_character":8,"end_line":84,"end_character":41},"in_reply_to":"9f560f44_1a1f8c5b","updated":"2020-08-20 22:23:35.000000000","message":"Yeah I actually had a different patch that added a new test to do much of what this represents (i.e. ensure all the steps are in there that we expect). Erno pointed out that this was already mostly covered here (although confusingly to me, not in the actual test for the import flow). But yeah, ImageLock should be in here and I will add it in a follow-up to avoid disturbing the emerging almost-consensus we\u0027ve got going here :)","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ed160248357f4835512b2063b635c54cb03e57d8","unresolved":false,"context_lines":[{"line_number":79,"context_line":"            \u0027glance-direct\u0027, \u0027web-download\u0027, \u0027copy-image\u0027])"},{"line_number":80,"context_line":"        self.config(node_staging_uri\u003d\u0027file:///tmp/staging\u0027)"},{"line_number":81,"context_line":"        store.create_stores(CONF)"},{"line_number":82,"context_line":"        self.base_flow \u003d [\u0027ConfigureStaging\u0027, \u0027ImportToStore\u0027,"},{"line_number":83,"context_line":"                          \u0027DeleteFromFS\u0027, \u0027VerifyImageState\u0027,"},{"line_number":84,"context_line":"                          \u0027CompleteTask\u0027]"},{"line_number":85,"context_line":"        self.import_plugins \u003d [\u0027Convert_Image\u0027,"},{"line_number":86,"context_line":"                               \u0027Decompress_Image\u0027,"},{"line_number":87,"context_line":"                               \u0027InjectMetadataProperties\u0027]"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_b57e73e1","line":84,"range":{"start_line":82,"start_character":8,"end_line":84,"end_character":41},"in_reply_to":"9f560f44_35c9c358","updated":"2020-08-20 22:31:36.000000000","message":"https://review.opendev.org/#/c/747305","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"fbe1a36308287adc67db953b0c7e218580a57b28","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        flow \u003d self._get_flow()"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"        flow_comp \u003d self._get_flow_tasks(flow)"},{"line_number":121,"context_line":"        # assert flow has 6 tasks"},{"line_number":122,"context_line":"        self.assertEqual(6, len(flow_comp))"},{"line_number":123,"context_line":"        for c in self.base_flow:"},{"line_number":124,"context_line":"            self.assertIn(c, flow_comp)"},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_ba52e011","line":122,"range":{"start_line":121,"start_character":0,"end_line":122,"end_character":43},"updated":"2020-08-20 21:38:13.000000000","message":"if you take my suggestion at line 84, this can become:\n\n  # assert flow has all base tasks\n  self.assertEqual(len(self.base_flow), len(flow_comp))\n\nwhich eliminates the magical 6.","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"fbe1a36308287adc67db953b0c7e218580a57b28","unresolved":false,"context_lines":[{"line_number":121,"context_line":"        # assert flow has 6 tasks"},{"line_number":122,"context_line":"        self.assertEqual(6, len(flow_comp))"},{"line_number":123,"context_line":"        for c in self.base_flow:"},{"line_number":124,"context_line":"            self.assertIn(c, flow_comp)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def test_get_flow_web_download_enabled(self):"},{"line_number":127,"context_line":"        # This test will ensure that without import plugins"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_da84f460","line":124,"updated":"2020-08-20 21:38:13.000000000","message":"And, if you take the suggestion at line 84, lines 122-124 will once again show that the flow_comp contains all and only the base tasks.","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"fbe1a36308287adc67db953b0c7e218580a57b28","unresolved":false,"context_lines":[{"line_number":137,"context_line":"        flow \u003d self._get_flow(import_req\u003dimport_req)"},{"line_number":138,"context_line":""},{"line_number":139,"context_line":"        flow_comp \u003d self._get_flow_tasks(flow)"},{"line_number":140,"context_line":"        # assert flow has 7 tasks"},{"line_number":141,"context_line":"        self.assertEqual(7, len(flow_comp))"},{"line_number":142,"context_line":"        for c in self.base_flow:"},{"line_number":143,"context_line":"            self.assertIn(c, flow_comp)"},{"line_number":144,"context_line":"        self.assertIn(\u0027WebDownload\u0027, flow_comp)"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f560f44_da76b4a4","line":141,"range":{"start_line":140,"start_character":0,"end_line":141,"end_character":43},"updated":"2020-08-20 21:38:13.000000000","message":"this would become\n\n  # assert flow has all base tasks plus web download\n  self.assertEqual(len(self.base_flow) + 1, len(flow_comp))\n\nand the same throughout. I think it will make the tests easier to read and modify as the code changes.","commit_id":"a309fdcd8ac8973a43dd94d8703006a29dcfba39"}]}