)]}'
{"glance/api/v2/images.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":179,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"        # NOTE (abhishekk) Handle race condition"},{"line_number":182,"context_line":"        host_name \u003d socket.gethostname()"},{"line_number":183,"context_line":"        os_glance_host_name \u003d image.extra_properties.get("},{"line_number":184,"context_line":"            \u0027os_glance_host_name\u0027, \u0027\u0027)"},{"line_number":185,"context_line":"        importing \u003d image.extra_properties.get("}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_9fc37089","line":182,"updated":"2020-06-23 18:51:52.000000000","message":"Is this good enough? Two worker processes on the same host will have the same hostname yet they need to be excluded as well right?","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"da638f7dba13f5122ef822cf51b9c91ad0a33bec","unresolved":false,"context_lines":[{"line_number":179,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"        # NOTE (abhishekk) Handle race condition"},{"line_number":182,"context_line":"        host_name \u003d socket.gethostname()"},{"line_number":183,"context_line":"        os_glance_host_name \u003d image.extra_properties.get("},{"line_number":184,"context_line":"            \u0027os_glance_host_name\u0027, \u0027\u0027)"},{"line_number":185,"context_line":"        importing \u003d image.extra_properties.get("}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_bf56345b","line":182,"in_reply_to":"bf51134e_9f925056","updated":"2020-06-23 19:14:36.000000000","message":"Oh I see, you\u0027re checking separately for the case where we raced against ourselves. In that case, this doesn\u0027t abort if we raced against another worker, right? I get that the potential for corruption is worse in the single-node-race case, but in the multi-node case, you could cause glance to DDoS the rbd store, the network, etc. Seems like we need to fix both of those cases.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":179,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"        # NOTE (abhishekk) Handle race condition"},{"line_number":182,"context_line":"        host_name \u003d socket.gethostname()"},{"line_number":183,"context_line":"        os_glance_host_name \u003d image.extra_properties.get("},{"line_number":184,"context_line":"            \u0027os_glance_host_name\u0027, \u0027\u0027)"},{"line_number":185,"context_line":"        importing \u003d image.extra_properties.get("}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_9f925056","line":182,"in_reply_to":"bf51134e_9fc37089","updated":"2020-06-23 19:07:11.000000000","message":"Yes, this will be handled at line #193","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        # NOTE (abhishekk) Handle race condition"},{"line_number":182,"context_line":"        host_name \u003d socket.gethostname()"},{"line_number":183,"context_line":"        os_glance_host_name \u003d image.extra_properties.get("},{"line_number":184,"context_line":"            \u0027os_glance_host_name\u0027, \u0027\u0027)"},{"line_number":185,"context_line":"        importing \u003d image.extra_properties.get("},{"line_number":186,"context_line":"            \u0027os_glance_importing_to_stores\u0027, \u0027\u0027)"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_5f5c38ec","line":184,"range":{"start_line":184,"start_character":13,"end_line":184,"end_character":32},"updated":"2020-06-23 18:51:52.000000000","message":"Shouldn\u0027t this be defined as a constant somewhere to avoid typos breaking the locking mechanism?","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":181,"context_line":"        # NOTE (abhishekk) Handle race condition"},{"line_number":182,"context_line":"        host_name \u003d socket.gethostname()"},{"line_number":183,"context_line":"        os_glance_host_name \u003d image.extra_properties.get("},{"line_number":184,"context_line":"            \u0027os_glance_host_name\u0027, \u0027\u0027)"},{"line_number":185,"context_line":"        importing \u003d image.extra_properties.get("},{"line_number":186,"context_line":"            \u0027os_glance_importing_to_stores\u0027, \u0027\u0027)"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_1f90004a","line":184,"range":{"start_line":184,"start_character":13,"end_line":184,"end_character":32},"in_reply_to":"bf51134e_5f5c38ec","updated":"2020-06-23 19:07:11.000000000","message":"+1","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":188,"context_line":"        if not os_glance_host_name:"},{"line_number":189,"context_line":"            image.extra_properties["},{"line_number":190,"context_line":"                \u0027os_glance_host_name\u0027] \u003d socket.gethostname()"},{"line_number":191,"context_line":"            image_repo.save(image)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        if importing and os_glance_host_name \u003d\u003d host_name:"},{"line_number":194,"context_line":"            msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as prior \""}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_3fd34462","line":191,"updated":"2020-06-23 18:51:52.000000000","message":"I think this is the same race condition.\n\n1. worker1 gets the image with blank properties\n2. worker2 gets the image with blank properties\n3. worker1 \"claims\" the import by saving its hostname\n4. worker2 \"claims\" by also saving its hostname, clobbering worker1\n5. both workers are proceeding.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":188,"context_line":"        if not os_glance_host_name:"},{"line_number":189,"context_line":"            image.extra_properties["},{"line_number":190,"context_line":"                \u0027os_glance_host_name\u0027] \u003d socket.gethostname()"},{"line_number":191,"context_line":"            image_repo.save(image)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        if importing and os_glance_host_name \u003d\u003d host_name:"},{"line_number":194,"context_line":"            msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as prior \""}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_9fa09092","line":191,"in_reply_to":"bf51134e_3fd34462","updated":"2020-06-23 19:07:11.000000000","message":"Yes, this approach does not eliminate the race condition fully, still need to think about this as well.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"f900a51f4596c8b092e9b87300b2559d090e1757","unresolved":false,"context_lines":[{"line_number":189,"context_line":"            image.extra_properties["},{"line_number":190,"context_line":"                \u0027os_glance_host_name\u0027] \u003d socket.gethostname()"},{"line_number":191,"context_line":"            image_repo.save(image)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        if importing and os_glance_host_name \u003d\u003d host_name:"},{"line_number":194,"context_line":"            msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as prior \""},{"line_number":195,"context_line":"                    \"operation is still in progress\") % image_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_7f9c9c0d","line":192,"updated":"2020-06-23 19:19:30.000000000","message":"We need to do image \u003d image_repo.get(image_id) after the image_repo.save(image) ... otherwise we\u0027re just comparing to the same object we just modified, not what\u0027s in the database.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6fa088d95be3e56e528a19f522c49085b769c810","unresolved":false,"context_lines":[{"line_number":189,"context_line":"            image.extra_properties["},{"line_number":190,"context_line":"                \u0027os_glance_host_name\u0027] \u003d socket.gethostname()"},{"line_number":191,"context_line":"            image_repo.save(image)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        if importing and os_glance_host_name \u003d\u003d host_name:"},{"line_number":194,"context_line":"            msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as prior \""},{"line_number":195,"context_line":"                    \"operation is still in progress\") % image_id"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_3a15129d","line":192,"in_reply_to":"bf51134e_7f9c9c0d","updated":"2020-06-23 19:58:45.000000000","message":"That will help us rule out the competing node worker case, but we can\u0027t really know whether or not we were competing with another thread on the same host if we do that.\n\nDoesn\u0027t a task have a uuid on creation, before it\u0027s started? Maybe using that as our test-and-set key would be more robust than hostname? I dunno if glance has any such requirements, but it\u0027s easy to have multiple machines that think their hostname is \u0027localhost\u0027 which breaks this kind of thing :)\n\nThat said, this surely feels to me like it is begging for something at the persistence layer to make sure we don\u0027t allow two images to go into a task state at the same time...","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":190,"context_line":"                \u0027os_glance_host_name\u0027] \u003d socket.gethostname()"},{"line_number":191,"context_line":"            image_repo.save(image)"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        if importing and os_glance_host_name \u003d\u003d host_name:"},{"line_number":194,"context_line":"            msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as prior \""},{"line_number":195,"context_line":"                    \"operation is still in progress\") % image_id"},{"line_number":196,"context_line":"            raise webob.exc.HTTPForbidden(explanation\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_ffe38c32","line":193,"range":{"start_line":193,"start_character":25,"end_line":193,"end_character":44},"updated":"2020-06-23 18:51:52.000000000","message":"This will always be the empty string if we think we won the race, or another worker\u0027s host if we didn\u0027t, so it will never be equal to our hostname, AFAICT.\n\nIf you re-query the image from the database after your transaction is completed above, then you could compare the value in the image to your hostname and know that if it\u0027s different, you lost and abort.\n\nHowever, I think the better thing to do would be to make the DB layer only agree to update the \"locked by host\" key if that key is currently not set, atomically in the same transaction as the update.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":1173,"context_line":"            image_view \u003d dict(image.extra_properties)"},{"line_number":1174,"context_line":"            # Remove os_glance_host_name from display"},{"line_number":1175,"context_line":"            if \u0027os_glance_host_name\u0027 in image_view:"},{"line_number":1176,"context_line":"                image_view.pop(\u0027os_glance_host_name\u0027)"},{"line_number":1177,"context_line":""},{"line_number":1178,"context_line":"            attributes \u003d [\u0027name\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":1179,"context_line":"                          \u0027visibility\u0027, \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_5fa7180c","line":1176,"updated":"2020-06-23 18:51:52.000000000","message":"I was going to say, probably don\u0027t want to show this to the user, so this is good. However, maybe this key should be named something more specific to import or tasks?\n\nCan someone set this property via PUT (i.e image update) if they know the magic? That seems like it would be bad, in case someone clobbers this value without realizing its importance.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":1173,"context_line":"            image_view \u003d dict(image.extra_properties)"},{"line_number":1174,"context_line":"            # Remove os_glance_host_name from display"},{"line_number":1175,"context_line":"            if \u0027os_glance_host_name\u0027 in image_view:"},{"line_number":1176,"context_line":"                image_view.pop(\u0027os_glance_host_name\u0027)"},{"line_number":1177,"context_line":""},{"line_number":1178,"context_line":"            attributes \u003d [\u0027name\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":1179,"context_line":"                          \u0027visibility\u0027, \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_dfd4c8eb","line":1176,"in_reply_to":"bf51134e_5fa7180c","updated":"2020-06-23 19:07:11.000000000","message":"this needs to be restricted in update, good catch.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddb080b58ab32adfd9ac429f345573f4495e56f8","unresolved":false,"context_lines":[{"line_number":171,"context_line":"                        raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":174,"context_line":"            host_time \u003d socket.gethostname() + \"-\" + str(time.time())"},{"line_number":175,"context_line":"            os_glance_hidden_check \u003d image.extra_properties.get("},{"line_number":176,"context_line":"                \u0027os_glance_hidden_check\u0027, \u0027\u0027)"},{"line_number":177,"context_line":"            importing \u003d image.extra_properties.get("}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_4c0c86a4","line":174,"updated":"2020-06-24 15:22:39.000000000","message":"This is just less likely to conflict, but it\u0027s not impossible. We have patterns to solve things like needing to generate unique identifiers, so why not use them?\n\nThe task being created below has a uuid, why not use it as the exclusion key?\n\nhttps://github.com/openstack/glance/blob/master/glance/domain/__init__.py#L476\n\nYes, it means doing the check further down near where the task is created, but there are good reasons for that too. See below.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddb080b58ab32adfd9ac429f345573f4495e56f8","unresolved":false,"context_lines":[{"line_number":181,"context_line":"                image.extra_properties["},{"line_number":182,"context_line":"                    \u0027os_glance_hidden_check\u0027] \u003d host_time"},{"line_number":183,"context_line":"                image_repo.save(image)"},{"line_number":184,"context_line":"                time.sleep(0.5)"},{"line_number":185,"context_line":"                image \u003d image_repo.get(image_id)"},{"line_number":186,"context_line":"                os_glance_hidden_check \u003d image.extra_properties.get("},{"line_number":187,"context_line":"                    \u0027os_glance_hidden_check\u0027, \u0027\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_8c869e12","line":184,"range":{"start_line":184,"start_character":16,"end_line":184,"end_character":31},"updated":"2020-06-24 15:22:39.000000000","message":"This only makes a race condition less likely to occur (and less likely to notice, debug, etc). Delaying isn\u0027t really a reasonable thing to do in an API worker, IMHO, nor really in any attempt to resolve a race. AFAIK, sleep() isn\u0027t monotonic, and is only guaranteed to be longer (wall clock time) than the value you provide. Running in a virtual machine where time can warp can make things like this not behave like you expect.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddb080b58ab32adfd9ac429f345573f4495e56f8","unresolved":false,"context_lines":[{"line_number":198,"context_line":"        if (not all_stores_must_succeed) and (not CONF.enabled_backends):"},{"line_number":199,"context_line":"            msg \u003d (_(\"All_stores_must_succeed can only be set with \""},{"line_number":200,"context_line":"                     \"enabled_backends %s\") % uri)"},{"line_number":201,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":202,"context_line":""},{"line_number":203,"context_line":"        task_input \u003d {\u0027image_id\u0027: image_id,"},{"line_number":204,"context_line":"                      \u0027import_req\u0027: body,"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_ecfaba72","line":201,"updated":"2020-06-24 15:22:39.000000000","message":"If you fail here, you will not release your lock on the image.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddb080b58ab32adfd9ac429f345573f4495e56f8","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                not utils.validate_import_uri(uri)):"},{"line_number":209,"context_line":"            LOG.debug(\"URI for web-download does not pass filtering: %s\", uri)"},{"line_number":210,"context_line":"            msg \u003d (_(\"URI for web-download does not pass filtering: %s\") % uri)"},{"line_number":211,"context_line":"            raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        try:"},{"line_number":214,"context_line":"            import_task \u003d task_factory.new_task(task_type\u003d\u0027api_image_import\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_2cf15296","line":211,"updated":"2020-06-24 15:22:39.000000000","message":"If you fail here you will not release your lock on the image.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ddb080b58ab32adfd9ac429f345573f4495e56f8","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        try:"},{"line_number":214,"context_line":"            import_task \u003d task_factory.new_task(task_type\u003d\u0027api_image_import\u0027,"},{"line_number":215,"context_line":"                                                owner\u003dreq.context.owner,"},{"line_number":216,"context_line":"                                                task_input\u003dtask_input)"},{"line_number":217,"context_line":"            task_repo.add(import_task)"},{"line_number":218,"context_line":"            task_executor \u003d executor_factory.new_task_executor(req.context)"},{"line_number":219,"context_line":"            pool \u003d common.get_thread_pool(\"tasks_eventlet_pool\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_ec6d9a32","line":216,"updated":"2020-06-24 15:22:39.000000000","message":"Right here, you can claim the image with your unique identifier, otherwise fail and you won\u0027t have to worry about all the cleanup paths. You can use the unique ID of the task instead of less precise/random keys.\n\nHowever, I still don\u0027t see how this is going to do anything other than reduce the likelihood of the race. Here\u0027s an execution sequence that will still let the race occur:\n\n1. worker1 gets image, sees no lock\n2. worker2 gets image, sees no lock\n3. worker1 claims lock, sleeps\n4. worker1 gets image, sees it has the lock\n5. worker2 claims lock, sleeps\n6. worker2 gets image, sees it has the lock\n7. both workers are running, thinking they have the lock\n\nHigh system load between steps 2 and 5 cause worker2 to be more delayed in between getting the image initially and writing its claim on the lock.\n\nYou really need something atomic here to prevent this. I haven\u0027t looked at your data structures, but you need something like this:\n\n UPDATE images SET images.os_glance_hidden_check\u003d$me WHERE images.id\u003d\u003d$image_id AND images.os_glance_hidden_check\u003dNULL\n\nWe could ask Mike Bayer to help with this if you want.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2d9aee4af5f3e8fa02deec51ce4c9044656d7d32","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        try:"},{"line_number":214,"context_line":"            import_task \u003d task_factory.new_task(task_type\u003d\u0027api_image_import\u0027,"},{"line_number":215,"context_line":"                                                owner\u003dreq.context.owner,"},{"line_number":216,"context_line":"                                                task_input\u003dtask_input)"},{"line_number":217,"context_line":"            task_repo.add(import_task)"},{"line_number":218,"context_line":"            task_executor \u003d executor_factory.new_task_executor(req.context)"},{"line_number":219,"context_line":"            pool \u003d common.get_thread_pool(\"tasks_eventlet_pool\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_30d080c9","line":216,"in_reply_to":"bf51134e_10861c14","updated":"2020-06-24 17:23:15.000000000","message":"Hey Mike,\n\nYou\u0027re offline on IRC right now so I\u0027ll try pinging you later to chat about this. But, we\u0027re looking for some help with an atomic operation whereby one of many threads of glance-api can kinda stake its claim on an image for a long-running operation. We need to be able to set that property on the image and know whether we \"won\" or \"lost\" so we know whether or not to continue or bail.\n\nI think we probably want/need an atomic \"create if not exists or fail\" ImageProperty row, buried in Image.save() or as a separate operation. Wondering if you could help with the proper pattern here to do this effectively.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"e52622b8eb3af31acf630a0a32a5d846725b2f5b","unresolved":false,"context_lines":[{"line_number":213,"context_line":"        try:"},{"line_number":214,"context_line":"            import_task \u003d task_factory.new_task(task_type\u003d\u0027api_image_import\u0027,"},{"line_number":215,"context_line":"                                                owner\u003dreq.context.owner,"},{"line_number":216,"context_line":"                                                task_input\u003dtask_input)"},{"line_number":217,"context_line":"            task_repo.add(import_task)"},{"line_number":218,"context_line":"            task_executor \u003d executor_factory.new_task_executor(req.context)"},{"line_number":219,"context_line":"            pool \u003d common.get_thread_pool(\"tasks_eventlet_pool\")"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_10861c14","line":216,"in_reply_to":"bf51134e_ec6d9a32","updated":"2020-06-24 17:07:15.000000000","message":"we could ask Mike, good to get more inputs.","commit_id":"25896684d81ac9cd47922fbeb6ae99ad45a16b5a"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"552932ac3d5930dc11b826241a0f640c7353b791","unresolved":false,"context_lines":[{"line_number":170,"context_line":"                                \"\u0027%s\u0027\") % existing_store"},{"line_number":171,"context_line":"                        raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":174,"context_line":"            host_time \u003d socket.gethostname() + \"-\" + str(time.time())"},{"line_number":175,"context_line":"            try:"},{"line_number":176,"context_line":"                image_repo.update_property_atomic("},{"line_number":177,"context_line":"                    image_id, \u0027os_glance_hidden_check\u0027, host_time)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_aebd36b8","line":174,"range":{"start_line":173,"start_character":12,"end_line":174,"end_character":59},"updated":"2020-06-26 10:37:35.000000000","message":"Now we can avoid use of these functions as we can set any value to the property.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c8d7c7287b2952c8f95f2edbce66148cbee933a4","unresolved":false,"context_lines":[{"line_number":170,"context_line":"                                \"\u0027%s\u0027\") % existing_store"},{"line_number":171,"context_line":"                        raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":172,"context_line":""},{"line_number":173,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":174,"context_line":"            host_time \u003d socket.gethostname() + \"-\" + str(time.time())"},{"line_number":175,"context_line":"            try:"},{"line_number":176,"context_line":"                image_repo.update_property_atomic("},{"line_number":177,"context_line":"                    image_id, \u0027os_glance_hidden_check\u0027, host_time)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_95d4ba49","line":174,"range":{"start_line":173,"start_character":12,"end_line":174,"end_character":59},"in_reply_to":"bf51134e_aebd36b8","updated":"2020-06-26 14:41:45.000000000","message":"How about we just set the value to the request-id so that if we needed to we could correlate the job to logs, post-recovery? I also think that using the task id would be good.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c8d7c7287b2952c8f95f2edbce66148cbee933a4","unresolved":false,"context_lines":[{"line_number":721,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":722,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":723,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":724,"context_line":"                            \u0027os_glance_hidden_check\u0027)"},{"line_number":725,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":726,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":727,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_b5cd7eb8","line":724,"updated":"2020-06-26 14:41:45.000000000","message":"This prevents an operator from clobbering the value I assume?","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9a6c659db78d14183bcee54bfb1c82928ae60856","unresolved":false,"context_lines":[{"line_number":721,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":722,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":723,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":724,"context_line":"                            \u0027os_glance_hidden_check\u0027)"},{"line_number":725,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":726,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":727,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_644fac6a","line":724,"in_reply_to":"bf51134e_b5cd7eb8","updated":"2020-06-29 06:44:45.000000000","message":"Yes, These are the properties which are reserved and not allowed to update by any user.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                        msg \u003d _(\"Image is already present at store \""},{"line_number":168,"context_line":"                                \"\u0027%s\u0027\") % existing_store"},{"line_number":169,"context_line":"                        raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"        except exception.Conflict as e:"},{"line_number":172,"context_line":"            raise webob.exc.HTTPConflict(explanation\u003de.msg)"},{"line_number":173,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_3930329f","line":170,"updated":"2020-06-29 17:12:52.000000000","message":"Random whitespace damage","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":167,"context_line":"                        msg \u003d _(\"Image is already present at store \""},{"line_number":168,"context_line":"                                \"\u0027%s\u0027\") % existing_store"},{"line_number":169,"context_line":"                        raise webob.exc.HTTPBadRequest(explanation\u003dmsg)"},{"line_number":170,"context_line":""},{"line_number":171,"context_line":"        except exception.Conflict as e:"},{"line_number":172,"context_line":"            raise webob.exc.HTTPConflict(explanation\u003de.msg)"},{"line_number":173,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_3eb651de","line":170,"in_reply_to":"bf51134e_3930329f","updated":"2020-06-30 10:22:36.000000000","message":"Done","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":195,"context_line":""},{"line_number":196,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":197,"context_line":"            try:"},{"line_number":198,"context_line":"                image_repo.set_property_atomic("},{"line_number":199,"context_line":"                    image, \u0027os_glance_hidden_check\u0027, import_task.task_id)"},{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_398cb234","line":198,"range":{"start_line":198,"start_character":16,"end_line":198,"end_character":46},"updated":"2020-06-29 17:12:52.000000000","message":"This isn\u0027t tested","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":195,"context_line":""},{"line_number":196,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":197,"context_line":"            try:"},{"line_number":198,"context_line":"                image_repo.set_property_atomic("},{"line_number":199,"context_line":"                    image, \u0027os_glance_hidden_check\u0027, import_task.task_id)"},{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_3ebc916b","line":198,"range":{"start_line":198,"start_character":16,"end_line":198,"end_character":46},"in_reply_to":"bf51134e_398cb234","updated":"2020-06-30 10:22:36.000000000","message":"simple way to test this is to mock the method and check whether it is called or not","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":196,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":197,"context_line":"            try:"},{"line_number":198,"context_line":"                image_repo.set_property_atomic("},{"line_number":199,"context_line":"                    image, \u0027os_glance_hidden_check\u0027, import_task.task_id)"},{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":202,"context_line":"                        \"prior operation is still in progress\") % image_id"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_f9199a21","line":199,"range":{"start_line":199,"start_character":28,"end_line":199,"end_character":50},"updated":"2020-06-29 17:12:52.000000000","message":"Would it not be better to name this something that will be more meaningful and potentially useful for other things? What about something like os_glance_task_in_progress? Presumably we\u0027d be well off to preclude other tasks from running on an image at the same time? Or maybe os_glance_import_task or something import-specific?","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":196,"context_line":"            # NOTE (abhishekk) Handle race condition"},{"line_number":197,"context_line":"            try:"},{"line_number":198,"context_line":"                image_repo.set_property_atomic("},{"line_number":199,"context_line":"                    image, \u0027os_glance_hidden_check\u0027, import_task.task_id)"},{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":202,"context_line":"                        \"prior operation is still in progress\") % image_id"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_9e831daa","line":199,"range":{"start_line":199,"start_character":28,"end_line":199,"end_character":50},"in_reply_to":"bf51134e_f9199a21","updated":"2020-06-30 10:22:36.000000000","message":"Done","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":202,"context_line":"                        \"prior operation is still in progress\") % image_id"},{"line_number":203,"context_line":"                raise exception.Conflict(msg)"},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"            task_repo.add(import_task)"},{"line_number":206,"context_line":"            task_executor \u003d executor_factory.new_task_executor(req.context)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_198f2e2a","line":203,"range":{"start_line":203,"start_character":16,"end_line":203,"end_character":45},"updated":"2020-06-29 17:12:52.000000000","message":"This case isn\u0027t tested","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":200,"context_line":"            except exception.Duplicate:"},{"line_number":201,"context_line":"                msg \u003d _(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":202,"context_line":"                        \"prior operation is still in progress\") % image_id"},{"line_number":203,"context_line":"                raise exception.Conflict(msg)"},{"line_number":204,"context_line":""},{"line_number":205,"context_line":"            task_repo.add(import_task)"},{"line_number":206,"context_line":"            task_executor \u003d executor_factory.new_task_executor(req.context)"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_fe9059f0","line":203,"range":{"start_line":203,"start_character":16,"end_line":203,"end_character":45},"in_reply_to":"bf51134e_198f2e2a","updated":"2020-06-30 10:22:36.000000000","message":"added new test test_image_import_duplicate_atomic_property","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":719,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":720,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":721,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":722,"context_line":"                            \u0027os_glance_hidden_check\u0027)"},{"line_number":723,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":724,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":725,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_997a1e21","line":722,"updated":"2020-06-29 17:12:52.000000000","message":"I assume there\u0027s a test to poke all the reserved properties to make sure they\u0027re blocked and that this should be added to that test?","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":719,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":720,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":721,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":722,"context_line":"                            \u0027os_glance_hidden_check\u0027)"},{"line_number":723,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":724,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":725,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_be5d414a","line":722,"in_reply_to":"bf51134e_997a1e21","updated":"2020-06-30 10:22:36.000000000","message":"test_update_reserved_attribute","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":1167,"context_line":"            image_view \u003d dict(image.extra_properties)"},{"line_number":1168,"context_line":"            # Remove os_glance_hidden_check from display"},{"line_number":1169,"context_line":"            if \u0027os_glance_hidden_check\u0027 in image_view:"},{"line_number":1170,"context_line":"                image_view.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":1171,"context_line":""},{"line_number":1172,"context_line":"            attributes \u003d [\u0027name\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":1173,"context_line":"                          \u0027visibility\u0027, \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_d9701643","line":1170,"updated":"2020-06-29 17:12:52.000000000","message":"This is not tested. Presumably you need to re-get the image after this gets set to make sure it\u0027s on the DB object but hidden from the API\u0027s view?","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":1167,"context_line":"            image_view \u003d dict(image.extra_properties)"},{"line_number":1168,"context_line":"            # Remove os_glance_hidden_check from display"},{"line_number":1169,"context_line":"            if \u0027os_glance_hidden_check\u0027 in image_view:"},{"line_number":1170,"context_line":"                image_view.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":1171,"context_line":""},{"line_number":1172,"context_line":"            attributes \u003d [\u0027name\u0027, \u0027disk_format\u0027, \u0027container_format\u0027,"},{"line_number":1173,"context_line":"                          \u0027visibility\u0027, \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_ccd84ff2","line":1170,"in_reply_to":"bf51134e_d9701643","updated":"2020-06-30 10:22:36.000000000","message":"Done","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":722,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":723,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":724,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":725,"context_line":"                            \u0027os_glance_import_task\u0027)"},{"line_number":726,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":727,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":728,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_256f3593","line":725,"updated":"2020-07-02 13:42:58.000000000","message":"Is this tested somewhere?","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":722,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":723,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":724,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":725,"context_line":"                            \u0027os_glance_import_task\u0027)"},{"line_number":726,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":727,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":728,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_859d6168","line":725,"in_reply_to":"bf51134e_256f3593","updated":"2020-07-02 14:49:44.000000000","message":"https://review.opendev.org/#/c/737596/10/glance/tests/unit/v2/test_images_resource.py@3677","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d27c246d09ffdf7466c59360dc74f47aab06183a","unresolved":false,"context_lines":[{"line_number":722,"context_line":"                            \u0027file\u0027, \u0027schema\u0027, \u0027id\u0027, \u0027os_hash_algo\u0027,"},{"line_number":723,"context_line":"                            \u0027os_hash_value\u0027)"},{"line_number":724,"context_line":"    _reserved_properties \u003d (\u0027location\u0027, \u0027deleted\u0027, \u0027deleted_at\u0027,"},{"line_number":725,"context_line":"                            \u0027os_glance_import_task\u0027)"},{"line_number":726,"context_line":"    _base_properties \u003d (\u0027checksum\u0027, \u0027created_at\u0027, \u0027container_format\u0027,"},{"line_number":727,"context_line":"                        \u0027disk_format\u0027, \u0027id\u0027, \u0027min_disk\u0027, \u0027min_ram\u0027, \u0027name\u0027,"},{"line_number":728,"context_line":"                        \u0027size\u0027, \u0027virtual_size\u0027, \u0027status\u0027, \u0027tags\u0027, \u0027owner\u0027,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e6b7043a","line":725,"in_reply_to":"bf51134e_859d6168","updated":"2020-07-02 15:05:01.000000000","message":"Ah thanks, missed it :)","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9283cee5072e1e9c6758b8c5ac9798f6360e2e9a","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_f00cf63a","line":136,"updated":"2020-07-17 20:21:48.000000000","message":"I can appreciate the self-correcting nature of this time-based approach, but it worries me that it is both obscure and mysterious from the perspective of the client, and fragile in that it is susceptible to clock synchronization and time-of-day monotony issues.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c44f1abc8cb37f360197c417e6a69a963955bdf2","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_3b3e3cb9","line":136,"in_reply_to":"bf51134e_035af428","updated":"2020-07-20 18:11:52.000000000","message":"\u003e This is actually not true. Both the os_glance_import_task and\n \u003e updated_at are visible for user and the admin.\n\nOkay, I thought Abhi had said previously that reserved properties were filtered from view. I don\u0027t see where that happens below, so if they are then that helps, although I also don\u0027t see where the importing_to_stores one are protected so I had assumed that there was more elsewhere.\n\n \u003e The whole point of\n \u003e this exercise was to provide the client the indication after 202\n \u003e response if the work is progressing or not. Which seemed to be your\n \u003e problem with the locking at the task.\n\nNo, the point of the locking was to prevent us from starting a ton of parallel import threads, which _was_ happening. However, if those are importing to different places, then one or more clients will have a hard time sussing out whether there is progress being made on their request.\n\n \u003e When documented there is\n \u003e nothing mysterious about it. It\u0027s very clear for the user and admin\n \u003e if the processing has actually started (and finished) unlike using\n \u003e the task ID as hard lock before processing the job and actually\n \u003e having no indication if the processing was ever picked up before\n \u003e the first store in the list either succeeds or fails.\n\nJust to reiterate your logic here, this is saying that the client\u0027s process needs to be this:\n\n 1. Make the request, if 202 then continue\n 2. If the value is \"Pending\", then the task is waiting to get picked up, continue polling until timeout\n 3. If the value is something other than pending (i.e. a uuid), then look to see if our store is in the copying_to_stores list\n 3a. If it is, then keep polling\n 3b. If it is not, then guess about whether or not our request has been queued behind the one for the other store, or if we should wait for that importing_to_stores key to be empty and then try again.\n\nSince glance doesn\u0027t queue currently, the client will have to make it all the way through step 3b before deciding to wait and retry or give up. The way we had this code before, we could know all of this from the always-consistent return value in step 1.\n\n \u003e Unlike 200 or 201, the response 202 is clear on it\u0027s non-committal\n \u003e nature (if the tasks API was not admin only by default we could\n \u003e have used 201 here and provide the link to the tasks api to monitor\n \u003e the status, but that\u0027s not the case so 202 was chosen very\n \u003e intentionally).\n\nUsers can\u0027t see the tasks API AFAICT, but can do copies, so I\u0027m not sure that will help. I know that the RFC says that 202 is non-committal, but of course in reality the 202 is not meant to be \"this is very fragile so I make no promises\" but rather \"this may be queued for so long that it\u0027s no longer processable when the time comes.\" I don\u0027t think 202 implies anything about lack-of-effort :)\n\n201 wouldn\u0027t be appropriate anyway, as it indicates that you\u0027ve at least created the thing they asked for and can pass back a pointer to that thing. 202 seems like the right code here, just that it should be consistent. We\u0027re very capable of returning 202 to one and only one client (as in previous iterations of this patch).\n\n \u003e My main problem is that hard locking without any self recovery\n \u003e mechanism (specially if allowed for 3rd party to initiate) is very\n \u003e unscalable and it provides long term DOS interface directly at the\n \u003e API (obviously this needs already trusted malicious user and would\n \u003e require pre-existing fault condition, but the scope is much\n \u003e larger).\n\nUnfortunately, the ephemeral nature of the task implementation makes that difficult. If there were dedicated workers that did long-running things like this then presumably we\u0027d have a way to signal them to quit. Nova is not good about this either, so I\u0027m not faulting anyone for that for sure. But we do try to maintain consistent results to the client at least.\n\n \u003e I\u0027m starting to be more convinced that it might be better to just\n \u003e refactor the copy-image taskflow to be race resilient rather than\n \u003e trying to prevent the race happening in the first place.\n\nRight now, this is nearly unusable from the Nova side, or in any other automated fashion, in practice. We\u0027ve already experienced starting lots of competing threads just by running tempest against Nova. So I\u0027m keen to see a mitigation of this at the very least, notwithstanding a large refactor.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"303c506ebc208265826392a6c7b7c7e2ffe9c05f","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_bb76cc0b","line":136,"in_reply_to":"bf51134e_035af428","updated":"2020-07-20 18:38:55.000000000","message":"This won\u0027t surprise anyone, but I have always been partial to the Tasks API.  I wonder whether instead of keeping it admin-only, we should make it read-only for non-admins.  Then we can keep the task_id as the lock content and return a 201 as Erno suggests.\n\nThen we can move the locking back to the taskflow task, which would address Erno\u0027s earlier locality concerns--the lock/revert/release code can all be in one place.\n\nWhen a task can\u0027t get the lock, it can go to \u0027failure\u0027 status and return a link to the blocking task in the message.  (And allowing a user to list tasks will solve that problem about expired tasks not being deleted--I don\u0027t think that bug was ever fixed.)\n\nTasks never had a DELETE method (they auto-expire) so we don\u0027t have to worry about that.  We will have to make sure that the \u0027input\u0027 element doesn\u0027t leak anything (I think it doesn\u0027t because all it contains is the info from the original request, though once the API became admin-only, we may not have been so careful when creating \"internal\" tasks).\n\nThus the Tasks API can function as a cross between the Cinder User Messages API and Nova faults.\n\nI went back and looked at the commit that made the Tasks API admin-only:\nhttps://opendev.org/openstack/glance/commit/8f0d6ea9c535f6ab09fc9b2ae84cf0bef2401ee4\n\n(I may have succeeded in coming up with a suggestion that no one will like!)\n\nI think the concern there was that people would confuse the old Tasks API with the new Image Import API.  It\u0027s been admin-only since Mitaka, so I don\u0027t think we need to worry about that anymore.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d979cd9ae651238b3f494c386f8c4ff54d632963","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_cdf2b49b","line":136,"in_reply_to":"bf51134e_52b91fc9","updated":"2020-07-22 21:58:33.000000000","message":"\u003e I think in general, you don\u0027t know, but in this case, the glance\n \u003e task should go to \u0027failure\u0027 very quickly as soon as the taskflow\n \u003e task can\u0027t get the lock.\n\nYeah, that\u0027s kinda my concern. \"Quickly\" is easy for a human with a command-line client. Code making the call and them polling needs to have a definition of how long to wait for it to change state. But, obviously I can just pick a likely-to-be-reasonable value, it\u0027s just less ideal than \"I got 20x, so I should be waiting\".\n\n \u003e Well, each task has an owner which is usually the project\n \u003e (\"usually\" because glance has a deprecated (and maybe even removed\n \u003e by now?) option to make the owner either the tenant or the user). \n \u003e Probably the image_id would show up in the \"input\" element\n \u003e (although that\u0027s stored as a json blob in the database).  There\u0027s a\n \u003e \"type\" element (currently, the only allowed value is \"import\", but\n \u003e we could enhance that) that could narrow things down a bit.  With\n \u003e the current import process, the gating by that is_image_mutable\n \u003e function (in the elif on your copy-image patch) prevents a task\n \u003e from being created for images you can\u0027t access, so the issue of\n \u003e having a task for an image you\u0027re not allowed to see doesn\u0027t come\n \u003e up.  Except for an admin, a user can only see their own tasks.\n\nRight, so I\u0027ve made the situation more complicated where we have one owner of the image, another owner of the task, which is actually running as admin (although probably still with my project_id). If I\u0027m the owner and I go looking for \"hey, what\u0027s this task running with my image?\" and follow the task_id in the lock key, then I\u0027m not going to be able to see it, if someone else initiated it. I guess only the one who gets the task reference back in the original call will really be doing that, but I just want to make sure we won\u0027t be too much plumbing to make sure the task is owned and visible by the caller and not the owner of the image.\n\n \u003e Well, I didn\u0027t say it was a *good* idea :)\n\nI said it was better! Obviously we\u0027ll need a compromise here, so good is better than nothing. I definitely sympathize with the desire to keep the server side code as clean as possible, but I generally do not think that being far more ambiguous with the client and making the API a lot less concrete is a good trade-off there. The reason I found this race is because I wrote the first ever use of this en masse to run in the gate. One single run of tempest and boom.... dozens of threads competing on the glance side, and dozens of nova boots waiting for things that were never going to happen (because of the auth thing). It\u0027s .... not awesome.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"e76a6b3a8d2a3b56a9a03aeaa4bf5b9b479d5853","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_52b91fc9","line":136,"in_reply_to":"bf51134e_7b7ef4af","updated":"2020-07-22 20:54:06.000000000","message":"\u003e As a client, how long do I poll for my task to change status? I\n \u003e kinda need to know if tasks are queued or if there\u0027s only one slot\n \u003e in order to make a good decision, don\u0027t I?\n\nI think in general, you don\u0027t know, but in this case, the glance task should go to \u0027failure\u0027 very quickly as soon as the taskflow task can\u0027t get the lock.\n\n \u003e That\u0027s going to be hard to correlate the task to the original\n \u003e operation amongst a list of them, I think. Presumably you have to\n \u003e filter out tasks that are for images the user can\u0027t see, right? Do\n \u003e we have enough data on the task to know that (efficiently)?\n\nWell, each task has an owner which is usually the project (\"usually\" because glance has a deprecated (and maybe even removed by now?) option to make the owner either the tenant or the user).  Probably the image_id would show up in the \"input\" element (although that\u0027s stored as a json blob in the database).  There\u0027s a \"type\" element (currently, the only allowed value is \"import\", but we could enhance that) that could narrow things down a bit.  With the current import process, the gating by that is_image_mutable function (in the elif on your copy-image patch) prevents a task from being created for images you can\u0027t access, so the issue of having a task for an image you\u0027re not allowed to see doesn\u0027t come up.  Except for an admin, a user can only see their own tasks.\n\n \u003e What you describe is definitely better than what we have, but if it\n \u003e were up to me, I\u0027d want the original import call to be consistent\n \u003e (i.e. only return 20x to one caller) and then use the task and its\n \u003e lack of updating to gate the _busting_ of the lock. Otherwise,\n \u003e instead of the \"is the task old enough?\" code needs to be in the\n \u003e happy path for *everyone* instead of just in the exceptional path\n \u003e for the *error* case.\n\nWell, I didn\u0027t say it was a *good* idea :)","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8f4d76ca242163615e4ab91a1d6a87b179dd8782","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_7b7ef4af","line":136,"in_reply_to":"bf51134e_bb76cc0b","updated":"2020-07-20 18:48:33.000000000","message":"\u003e This won\u0027t surprise anyone, but I have always been partial to the\n \u003e Tasks API.  I wonder whether instead of keeping it admin-only, we\n \u003e should make it read-only for non-admins.  Then we can keep the\n \u003e task_id as the lock content and return a 201 as Erno suggests.\n\nAs a client, how long do I poll for my task to change status? I kinda need to know if tasks are queued or if there\u0027s only one slot in order to make a good decision, don\u0027t I?\n\n \u003e \n \u003e Then we can move the locking back to the taskflow task, which would\n \u003e address Erno\u0027s earlier locality concerns--the lock/revert/release\n \u003e code can all be in one place.\n \u003e \n \u003e When a task can\u0027t get the lock, it can go to \u0027failure\u0027 status and\n \u003e return a link to the blocking task in the message.  (And allowing a\n \u003e user to list tasks will solve that problem about expired tasks not\n \u003e being deleted--I don\u0027t think that bug was ever fixed.)\n\nThat\u0027s going to be hard to correlate the task to the original operation amongst a list of them, I think. Presumably you have to filter out tasks that are for images the user can\u0027t see, right? Do we have enough data on the task to know that (efficiently)?\n\nWhat you describe is definitely better than what we have, but if it were up to me, I\u0027d want the original import call to be consistent (i.e. only return 20x to one caller) and then use the task and its lack of updating to gate the _busting_ of the lock. Otherwise, instead of the \"is the task old enough?\" code needs to be in the happy path for *everyone* instead of just in the exceptional path for the *error* case.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b2b1af762e968bbe6cbf3efb1a7cdcc76180d7e4","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_fd2aff29","line":136,"in_reply_to":"bf51134e_e5a8af8d","updated":"2020-07-20 14:07:22.000000000","message":"Well, the problem with it being obscure and mysterious is that it\u0027s a time-based lock with no outward visual indication to the user or the admin that it\u0027s locked, but will become unlocked at some other time.\n\nAs far as time of day issues, any issue with synchronization of the host clock or any time warp inside the VM that this may be running in can cause this to wait longer than this time, or more worrisome, shorter.\n\nIt also has nothing to do with whether or not the task has actually started, nor whether it will. Assuming your task executor is running with a bounded pool, I could presumably ask for a thousand images to be copied, which queue up for the  executor and eventually get run, after the short or long timers have expired, the lock is mysteriously broken, and someone may have queued something else for this image.\n\nAll this to avoid using the already-existing unique identifier we have for the task. It just doesn\u0027t make sense to me.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"2aa71d7f19360180398c32c0bc4d3e678b8a39d8","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_e5a8af8d","line":136,"in_reply_to":"bf51134e_f00cf63a","updated":"2020-07-20 01:17:03.000000000","message":"\u003e I can appreciate the self-correcting nature of this time-based\n \u003e approach, but it worries me that it is both obscure and mysterious\n \u003e from the perspective of the client, and fragile in that it is\n \u003e susceptible to clock synchronization and time-of-day monotony\n \u003e issues.\n\nWould you mind to elaborate on the problems you see this causing?","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"a6377b1ff8a1a43d1719e4e361052687e7a9b484","unresolved":false,"context_lines":[{"line_number":133,"context_line":"                    msg \u003d (_(\"New operation on image \u0027%s\u0027 is not permitted as \""},{"line_number":134,"context_line":"                             \"prior operation is still in progress\") %"},{"line_number":135,"context_line":"                           image_id)"},{"line_number":136,"context_line":"                    raise exception.Conflict(msg)"},{"line_number":137,"context_line":"            if image.status \u003d\u003d \u0027active\u0027 and import_method !\u003d \"copy-image\":"},{"line_number":138,"context_line":"                msg \u003d _(\"Image with status active cannot be target for import\")"},{"line_number":139,"context_line":"                raise exception.Conflict(msg)"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_035af428","line":136,"in_reply_to":"bf51134e_fd2aff29","updated":"2020-07-20 15:49:05.000000000","message":"This is actually not true. Both the os_glance_import_task and updated_at are visible for user and the admin. The whole point of this exercise was to provide the client the indication after 202 response if the work is progressing or not. Which seemed to be your problem with the locking at the task. When documented there is nothing mysterious about it. It\u0027s very clear for the user and admin if the processing has actually started (and finished) unlike using the task ID as hard lock before processing the job and actually having no indication if the processing was ever picked up before the first store in the list either succeeds or fails.\n\nUnlike 200 or 201, the response 202 is clear on it\u0027s non-committal nature (if the tasks API was not admin only by default we could have used 201 here and provide the link to the tasks api to monitor the status, but that\u0027s not the case so 202 was chosen very intentionally).\n\nMy main problem is that hard locking without any self recovery mechanism (specially if allowed for 3rd party to initiate) is very unscalable and it provides long term DOS interface directly at the API (obviously this needs already trusted malicious user and would require pre-existing fault condition, but the scope is much larger).\n\nI\u0027m starting to be more convinced that it might be better to just refactor the copy-image taskflow to be race resilient rather than trying to prevent the race happening in the first place.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"}],"glance/async_/flows/api_image_import.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                          \"os_glance_importing_to_stores.\", self.backend)"},{"line_number":285,"context_line":"        # NOTE(flaper87): We need to save the image again after"},{"line_number":286,"context_line":"        # the locations have been set in the image."},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        self.image_repo.save(image)"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    def revert(self, result, **kwargs):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_ffbc2c07","line":287,"updated":"2020-06-23 18:51:52.000000000","message":"extra whitespace :)","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":284,"context_line":"                          \"os_glance_importing_to_stores.\", self.backend)"},{"line_number":285,"context_line":"        # NOTE(flaper87): We need to save the image again after"},{"line_number":286,"context_line":"        # the locations have been set in the image."},{"line_number":287,"context_line":""},{"line_number":288,"context_line":"        self.image_repo.save(image)"},{"line_number":289,"context_line":""},{"line_number":290,"context_line":"    def revert(self, result, **kwargs):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_5f027870","line":287,"in_reply_to":"bf51134e_ffbc2c07","updated":"2020-06-23 19:07:11.000000000","message":"will remove it :D","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9efd4da9093327999b1cdc7f14c89a1ddb0b9b22","unresolved":false,"context_lines":[{"line_number":296,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":297,"context_line":"        # Clear the os_glance_host_name property"},{"line_number":298,"context_line":"        image.extra_properties[\u0027os_glance_host_name\u0027] \u003d \"\""},{"line_number":299,"context_line":"        self.image_repo.save(image)"},{"line_number":300,"context_line":""},{"line_number":301,"context_line":"        for i, location in enumerate(image.locations):"},{"line_number":302,"context_line":"            if location.get(\u0027metadata\u0027, {}).get(\u0027store\u0027) \u003d\u003d self.backend:"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_ff51ccda","line":299,"updated":"2020-06-23 18:51:52.000000000","message":"Will this get called if the process is interrupted by a power outage or kill -9? Meaning, is the state of the task persisted in the database such that if we never finish or revert, that we will resume the task and call this revert to clear the image? I\u0027m just concerned that an image may remain \"locked\" after a failure, and since I can\u0027t see that property from the API, I don\u0027t know that it\u0027s set after a recovery, and don\u0027t know why my tasks never start.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"da638f7dba13f5122ef822cf51b9c91ad0a33bec","unresolved":false,"context_lines":[{"line_number":296,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":297,"context_line":"        # Clear the os_glance_host_name property"},{"line_number":298,"context_line":"        image.extra_properties[\u0027os_glance_host_name\u0027] \u003d \"\""},{"line_number":299,"context_line":"        self.image_repo.save(image)"},{"line_number":300,"context_line":""},{"line_number":301,"context_line":"        for i, location in enumerate(image.locations):"},{"line_number":302,"context_line":"            if location.get(\u0027metadata\u0027, {}).get(\u0027store\u0027) \u003d\u003d self.backend:"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_9f01b03b","line":299,"in_reply_to":"bf51134e_dfede8b7","updated":"2020-06-23 19:14:36.000000000","message":"Okay then it seems problematic to me to lock the image via property here that is only lifted (and liftable) after successful completion of the task (either success or failure). \n\nPerhaps we need some startup-time cleanup of any task that is stuck running? If you do use the hostname as the lock, then cleaning up any image that has an active lock \u003d\u003d $hostname at service startup time might be good, although I\u0027m not sure how well that will work with actual WSGI stuff.\n\nAnyway, this error recovery case seems like an important thing to get right...","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"0a94cbdb475da1dc25262351240516fcb5191025","unresolved":false,"context_lines":[{"line_number":296,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":297,"context_line":"        # Clear the os_glance_host_name property"},{"line_number":298,"context_line":"        image.extra_properties[\u0027os_glance_host_name\u0027] \u003d \"\""},{"line_number":299,"context_line":"        self.image_repo.save(image)"},{"line_number":300,"context_line":""},{"line_number":301,"context_line":"        for i, location in enumerate(image.locations):"},{"line_number":302,"context_line":"            if location.get(\u0027metadata\u0027, {}).get(\u0027store\u0027) \u003d\u003d self.backend:"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_dfede8b7","line":299,"in_reply_to":"bf51134e_ff51ccda","updated":"2020-06-23 19:07:11.000000000","message":"Nope, glance tasks doesn\u0027t picked up on service starts/restarts.","commit_id":"8a377ec1d7f20912c08568af96f93633e9b64f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":296,"context_line":"        # Clear the os_glance_hidden_check property"},{"line_number":297,"context_line":"        if \u0027os_glance_hidden_check\u0027 in image.extra_properties:"},{"line_number":298,"context_line":"            image.extra_properties.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        self.image_repo.save(image)"},{"line_number":301,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_b996024b","line":298,"updated":"2020-06-29 17:12:52.000000000","message":"Where is this tested?","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":296,"context_line":"        # Clear the os_glance_hidden_check property"},{"line_number":297,"context_line":"        if \u0027os_glance_hidden_check\u0027 in image.extra_properties:"},{"line_number":298,"context_line":"            image.extra_properties.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":299,"context_line":""},{"line_number":300,"context_line":"        self.image_repo.save(image)"},{"line_number":301,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_ec02730b","line":298,"in_reply_to":"bf51134e_b996024b","updated":"2020-06-30 10:22:36.000000000","message":"added new test","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":352,"context_line":""},{"line_number":353,"context_line":"        # Clear the os_glance_hidden_check property"},{"line_number":354,"context_line":"        if \u0027os_glance_hidden_check\u0027 in new_image.extra_properties:"},{"line_number":355,"context_line":"            new_image.extra_properties.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        self.image_repo.save(new_image)"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_99917e55","line":355,"updated":"2020-06-29 17:12:52.000000000","message":"Ditto.","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":352,"context_line":""},{"line_number":353,"context_line":"        # Clear the os_glance_hidden_check property"},{"line_number":354,"context_line":"        if \u0027os_glance_hidden_check\u0027 in new_image.extra_properties:"},{"line_number":355,"context_line":"            new_image.extra_properties.pop(\u0027os_glance_hidden_check\u0027)"},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"        self.image_repo.save(new_image)"},{"line_number":358,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_47fd3807","line":355,"in_reply_to":"bf51134e_99917e55","updated":"2020-06-30 10:22:36.000000000","message":"added new test","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                     \" in format \u0027file://\u003cabsolute-path\u003e\u0027\") %"},{"line_number":139,"context_line":"                   {\u0027task_id\u0027: self.task_id,"},{"line_number":140,"context_line":"                    \u0027task_type\u0027: self.task_type})"},{"line_number":141,"context_line":"            raise exception.BadTaskConfiguration(msg)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"        if not CONF.enabled_backends:"},{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e5253dad","line":141,"range":{"start_line":141,"start_character":12,"end_line":141,"end_character":53},"updated":"2020-07-02 13:42:58.000000000","message":"I think you could leak your lock here","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d27c246d09ffdf7466c59360dc74f47aab06183a","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                     \" in format \u0027file://\u003cabsolute-path\u003e\u0027\") %"},{"line_number":139,"context_line":"                   {\u0027task_id\u0027: self.task_id,"},{"line_number":140,"context_line":"                    \u0027task_type\u0027: self.task_type})"},{"line_number":141,"context_line":"            raise exception.BadTaskConfiguration(msg)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"        if not CONF.enabled_backends:"},{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e6e5443a","line":141,"range":{"start_line":141,"start_character":12,"end_line":141,"end_character":53},"in_reply_to":"bf51134e_4526a918","updated":"2020-07-02 15:05:01.000000000","message":"This still seems fragile, and given the DB surgery that will be required to unlock an image, it makes me nervous. However, see my comment below about another task step to make sure we always revert.","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                     \" in format \u0027file://\u003cabsolute-path\u003e\u0027\") %"},{"line_number":139,"context_line":"                   {\u0027task_id\u0027: self.task_id,"},{"line_number":140,"context_line":"                    \u0027task_type\u0027: self.task_type})"},{"line_number":141,"context_line":"            raise exception.BadTaskConfiguration(msg)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"        if not CONF.enabled_backends:"},{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_4526a918","line":141,"range":{"start_line":141,"start_character":12,"end_line":141,"end_character":53},"in_reply_to":"bf51134e_e5253dad","updated":"2020-07-02 14:49:44.000000000","message":"we don\u0027t pass image_id or image_repo to this task, so either we need to pass these or it will be difficult to do cleanup.\n\nAnother this is copy-image will not be used in case of single stores so we will not break in this task as we already set/reserve staging store in advance using os_glance_staging_dir","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"},{"line_number":145,"context_line":"            # verifying that we actually can build the store will allow us to"},{"line_number":146,"context_line":"            # fail the flow early with clear message why that happens."},{"line_number":147,"context_line":"            self._build_store()"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"    def _build_store(self):"},{"line_number":150,"context_line":"        # TODO(abhishekk): After removal of backend module from glance_store"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_253735e6","line":147,"updated":"2020-07-02 13:42:58.000000000","message":"And here","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"},{"line_number":145,"context_line":"            # verifying that we actually can build the store will allow us to"},{"line_number":146,"context_line":"            # fail the flow early with clear message why that happens."},{"line_number":147,"context_line":"            self._build_store()"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"    def _build_store(self):"},{"line_number":150,"context_line":"        # TODO(abhishekk): After removal of backend module from glance_store"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_65d64df4","line":147,"in_reply_to":"bf51134e_253735e6","updated":"2020-07-02 14:49:44.000000000","message":"ditto","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d27c246d09ffdf7466c59360dc74f47aab06183a","unresolved":false,"context_lines":[{"line_number":144,"context_line":"            # NOTE(jokke): We really don\u0027t need the store for anything but"},{"line_number":145,"context_line":"            # verifying that we actually can build the store will allow us to"},{"line_number":146,"context_line":"            # fail the flow early with clear message why that happens."},{"line_number":147,"context_line":"            self._build_store()"},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"    def _build_store(self):"},{"line_number":150,"context_line":"        # TODO(abhishekk): After removal of backend module from glance_store"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_46823051","line":147,"in_reply_to":"bf51134e_65d64df4","updated":"2020-07-02 15:05:01.000000000","message":"Ah right, definitely makes sense here.","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":447,"context_line":"    flow.add(_VerifyStaging(task_id, task_type, task_repo, file_uri))"},{"line_number":448,"context_line":""},{"line_number":449,"context_line":"    for plugin in import_plugins.get_import_plugins(**kwargs):"},{"line_number":450,"context_line":"        flow.add(plugin)"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"    for idx, store in enumerate(stores, 1):"},{"line_number":453,"context_line":"        set_active \u003d (not all_stores_must_succeed) or (idx \u003d\u003d len(stores))"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_253c55be","line":450,"updated":"2020-07-02 13:42:58.000000000","message":"Any of these steps could fail and leak the lock since they run before _ImportToStore right?","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":447,"context_line":"    flow.add(_VerifyStaging(task_id, task_type, task_repo, file_uri))"},{"line_number":448,"context_line":""},{"line_number":449,"context_line":"    for plugin in import_plugins.get_import_plugins(**kwargs):"},{"line_number":450,"context_line":"        flow.add(plugin)"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"    for idx, store in enumerate(stores, 1):"},{"line_number":453,"context_line":"        set_active \u003d (not all_stores_must_succeed) or (idx \u003d\u003d len(stores))"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e5ea5daa","line":450,"in_reply_to":"bf51134e_253c55be","updated":"2020-07-02 14:49:44.000000000","message":"image_conversion.py and image_decompression.py these two plugins can cause failures.\n\nI will add revert method to those plugins and will do the cleanup.","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d27c246d09ffdf7466c59360dc74f47aab06183a","unresolved":false,"context_lines":[{"line_number":447,"context_line":"    flow.add(_VerifyStaging(task_id, task_type, task_repo, file_uri))"},{"line_number":448,"context_line":""},{"line_number":449,"context_line":"    for plugin in import_plugins.get_import_plugins(**kwargs):"},{"line_number":450,"context_line":"        flow.add(plugin)"},{"line_number":451,"context_line":""},{"line_number":452,"context_line":"    for idx, store in enumerate(stores, 1):"},{"line_number":453,"context_line":"        set_active \u003d (not all_stores_must_succeed) or (idx \u003d\u003d len(stores))"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_26e8dc14","line":450,"in_reply_to":"bf51134e_e5ea5daa","updated":"2020-07-02 15:05:01.000000000","message":"Would the safest thing to do be add a task that always runs first, with a revert action of removing the lock? So if we fail, we always run that revert, and then only remove it on success in the last step?","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":466,"context_line":"        import_task.add(import_to_store)"},{"line_number":467,"context_line":"        flow.add(import_task)"},{"line_number":468,"context_line":""},{"line_number":469,"context_line":"    delete_task \u003d lf.Flow(task_type).add(_DeleteFromFS(task_id, task_type))"},{"line_number":470,"context_line":"    flow.add(delete_task)"},{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    save_task \u003d _SaveImage(task_id,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_256575e2","line":469,"updated":"2020-07-02 13:42:58.000000000","message":"I guess if we fail here, we will run revert on _ImportToStore and do our cleanup right?","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":466,"context_line":"        import_task.add(import_to_store)"},{"line_number":467,"context_line":"        flow.add(import_task)"},{"line_number":468,"context_line":""},{"line_number":469,"context_line":"    delete_task \u003d lf.Flow(task_type).add(_DeleteFromFS(task_id, task_type))"},{"line_number":470,"context_line":"    flow.add(delete_task)"},{"line_number":471,"context_line":""},{"line_number":472,"context_line":"    save_task \u003d _SaveImage(task_id,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_e011abae","line":469,"in_reply_to":"bf51134e_256575e2","updated":"2020-07-02 14:49:44.000000000","message":"yes","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e3766d01c5df129a9cbc09b14d66f5659cfa3901","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_0602d4b4","line":130,"updated":"2020-07-06 16:03:52.000000000","message":"Hmm, I think I like this a lot less than setting the locking property early. The fact that you need to check to see if we\u0027re running revert because of this conflict also makes it a bit dangerous if anything later in the flow raises an exception that inherits from Conflict, right? At the very least, I would create a specific and dedicated exception (likely just scoped to this file to avoid it being re-used) to  indicate that we lost the race and check for that specifically.\n\nAlso, the API caller will not receive a consistent result, as they can get Conflict if they\u0027re very late to the game, but they will get a 202 if they just barely lose the race. So, an API client that wants to import to store foo might lose, and will have to examine the importing_to_stores key to determine that they have, in fact, lost the race to another client trying to import to store bar. It would be much easier client logic to be able to rely on the status code from the import call.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"d61546573135ec029246002ad2e1f1521cd6734e","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_716d3603","line":130,"in_reply_to":"bf51134e_0602d4b4","updated":"2020-07-07 14:38:15.000000000","message":"I can see why conflict might get risky, so bit more specific exception might indeed be appropriate.\n\nWe had lenghty chat with Abhishek about the timing and where these things should be done. I think there\u0027s three parts to it:\nFirst of all we shouldn\u0027t be reverting stuff in the task that the task did not set. Doing so will very easily lead to more spagetti and unmaintainable code for anyone trying to understand later on what\u0027s going on here.\nSecond thing is, that if there is an issue where the taskflow doesn\u0027t actually start for some reason, we would be placing lock and never removing it as placing the lock happens outside of taskflow but removing it happens inside. We\u0027ve seen examples of this before for example in the uwsgi case where taskflow thinks it has started correctly but the worker actually never comes up.\nThird point is indeed the failing as early as possible that speaks for the otherwise more problematic behaviour. I would 100% agree to that being the most important factor should this call be something where operational success was actually tied to returning to the client. But as async nature of the import call, the client needs to be prepared that the processing might fail for some reason. If this reason is very unlucky timing of the calls (we\u0027re still talking about very simultaneous requests if they are not caught in the check at API) that is one part of it.\n\nLike always, migitations like this are compromise and I think processing the locking both ways in the same process is the better compromise long term.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e3d8ea2fc8e782242bc26642b128695fdbffa513","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_13192279","line":130,"in_reply_to":"bf51134e_1500f100","updated":"2020-07-10 13:34:53.000000000","message":"\u003e Abhishek and Erno are going to hate me for this, but ...\n \u003e \n \u003e (1) if we do set the lock early, I\u0027d really prefer to do it in the\n \u003e API request -- no sense going through task creation, etc.  I think\n \u003e that would mean that the AtomicPropertyUpdate task would only need\n \u003e a revert() (and you wouldn\u0027t have to filter for a Conflict\n \u003e exception) and could be renamed something like ReleaseImageLockOnFailure\n \u003e and the docstring would explain where the lock is set and where it\n \u003e is normally released.\n\nWell, hate aside, this was my preference as well, and what we had in this patch initially. It makes it very clear up front in the API code that we\u0027re staking our claim on this instance. But, if having it in get_flow() makes Erno happier that seems like a reasonable compromise - keeping the code closer together, but still keeping the API tight, so I\u0027m also okay with that.\n\nA task with just a revert() makes sense to me, but I\u0027m not an expert on TaskFlow and its conventions.\n\n \u003e (2) I think it might be worth abstracting the lock mechanism (the\n \u003e new DB atomic property API and using an image property as a lock)\n \u003e from this code, so that it doesn\u0027t manipulate the image property\n \u003e directly.  What I mean is something like ImportLock.obtain() and\n \u003e ImportLock.release(), where you pass an image object as the\n \u003e parameter.  What\u0027s bugging me is popping the image property in the\n \u003e SaveImage task, because it\u0027s not so obvious that what\u0027s really\n \u003e happening is that a lock is being released.  On the other hand, in\n \u003e the current code for the SaveImage task, the lock is released\n \u003e simultaneously with the change to \u0027active\u0027 when you do the\n \u003e image_repo.save(new_image).  I\u0027m not sure what the implications are\n \u003e of releasing the lock just before (or after) the image status is\n \u003e changed.\n\nThis also makes sense to me, if it fits better into Glance\u0027s modeling convention. You could imagine a generic ImageLock which has a class variable of the lock name, and then a subclass which is our ImportLock. Then you could more easily use this for a bunch of the other places where the same situation exists.\n\n \u003e (3) I think we definitely need some tooling added to glance-manage\n \u003e to list locked images and unlock them.  I think you\u0027re storing the\n \u003e task UUID as part of the value of the os_glance_import_task\n \u003e property?  The cleaning code could verify that the task is in a\n \u003e terminal state before releasing the lock on an image (though I\n \u003e guess that wouldn\u0027t help if there\u0027s been a catastrophic failure and\n \u003e the task states are all inaccurate).  We\u0027ll probably need some more\n \u003e elaborate cleanup code like Dan describes above.\n\n++","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1aedd898610f35b5edcfdfa2c8e41a0448d9be42","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_c3ab991a","line":130,"in_reply_to":"bf51134e_716d3603","updated":"2020-07-07 15:14:33.000000000","message":"Having just written a client to consume this and then trying to figure out what\u0027s going on, I can say it\u0027s not great to have to guess. In the original case where glance wasn\u0027t configured to run standalone, and the task never got started, my code would wait for the looong timeout before giving up, because it didn\u0027t know that the task hadn\u0027t even started. By moving this check to async, that same thing can/will happen, even if glance *is* properly configured which means the consuming code has to have two timeouts and more complex state logic. It has to have a short timeout to see that the requested store has at least shown up in \"importing_to_stores\" and then once that is satisfied, go into a longer wait for it to go into \"stores\" or \"import_failed\". Those really need to be tunable, which means nova has to have more config to support this.\n\nRight now, the get_flow() function below sets the state of the image to \"importing\" which is a (non-atomic) image lock in a very similar way. That will run before the API call returns and would fail if something else raced with us to set that state, with good reason. Similarly, the task updates the state to \"active\" on completion (for non-copy import methods), even though it wasn\u0027t the task itself that set the status to \"importing\" in the first place. This seems like the same thing to me as this lock the same behavior and requirement, although it makes it truly atomic, which is why I think it should be set before the API returns.\n\nDoes doing it in the get_flow() function (instead of here so that it is synchronous with the API call) make it more integrated with the task to satisfy your concern about being tight coupling between the API and the task behavior?","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ae995a0159b5f2cea4aabeea81632a9e095b54c0","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_d7a84121","line":130,"in_reply_to":"bf51134e_b7e02d28","updated":"2020-07-09 22:08:11.000000000","message":"The thing is, you have that case _regardless_ so you really have to have some way for the operator to fix it. If you\u0027re running one of these tasks and the power goes out, or the OOM killer nukes glance, or some overzealous container manager/fencing agent kill -9s everything, you\u0027re going to leave lock (and other, as we noted in a couple other bugs) residue that has to be cleaned up.\n\nI think the way taskflow expects you to solve that is by using its persistence mechanism (although I\u0027m far from a taskflow expert) so that the reverts can run after you come back and clean up the task repo. The way we tend to do this sort of thing in nova is that we scope things like that to a node that is doing them, and on service startup, we look for remnants of past unfinished tasks.\n\nSo, point being, please let\u0027s make the API consistent for the client, since we have to solve the out-of-band cleanup problem anyway. I previously brought up the post-disaster cleanup case on an earlier patch set and Abhi acknowledged that we\u0027d need to have some mechanism for that anyway.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"8fc1ff26a4f724a84066e5e0cbed8e0e03ed0ac5","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_b7e02d28","line":130,"in_reply_to":"bf51134e_c3ab991a","updated":"2020-07-09 21:32:19.000000000","message":"I\u0027ve thought about this for a while, and I see Dan\u0027s point, but ... if we go the route of setting the lock in get_flow() (which at least would keep it right here in this file for people reading the code), at some point we may have a failure that prevents the tasks from starting, and so the revert never happens.  That will leave the lock on the image, and it looks like there would be no way to remove it short of the operator going directly to the database since an operator can\u0027t update it (or even see it).  I suppose that since we\u0027re talking about really long-running processes here, you could have a catastrophic failure on the node running the tasks and the lock might never be released.  What are our plans for operator cleanup?  I guess what I\u0027m thinking is that if we have to have some kind of cleanup ability anyway, maybe it\u0027s OK to set the lock a little early?  I don\u0027t think I\u0027m being much help here.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"0943595296c3ed50071f24a611b4e87244c2a1c2","unresolved":false,"context_lines":[{"line_number":127,"context_line":"        super(_AtomicPropertyUpdate, self).__init__("},{"line_number":128,"context_line":"            name\u003d\u0027%s-AtomicPropertyUpdate-%s\u0027 % (task_type, task_id))"},{"line_number":129,"context_line":""},{"line_number":130,"context_line":"    def execute(self):"},{"line_number":131,"context_line":"        try:"},{"line_number":132,"context_line":"            image \u003d self.image_repo.get(self.image_id)"},{"line_number":133,"context_line":"            # Handle race condition"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_1500f100","line":130,"in_reply_to":"bf51134e_d7a84121","updated":"2020-07-10 01:46:09.000000000","message":"Abhishek and Erno are going to hate me for this, but ...\n\n(1) if we do set the lock early, I\u0027d really prefer to do it in the API request -- no sense going through task creation, etc.  I think that would mean that the AtomicPropertyUpdate task would only need a revert() (and you wouldn\u0027t have to filter for a Conflict exception) and could be renamed something like ReleaseImageLockOnFailure and the docstring would explain where the lock is set and where it is normally released.\n\n(2) I think it might be worth abstracting the lock mechanism (the new DB atomic property API and using an image property as a lock) from this code, so that it doesn\u0027t manipulate the image property directly.  What I mean is something like ImportLock.obtain() and ImportLock.release(), where you pass an image object as the parameter.  What\u0027s bugging me is popping the image property in the SaveImage task, because it\u0027s not so obvious that what\u0027s really happening is that a lock is being released.  On the other hand, in the current code for the SaveImage task, the lock is released simultaneously with the change to \u0027active\u0027 when you do the image_repo.save(new_image).  I\u0027m not sure what the implications are of releasing the lock just before (or after) the image status is changed.\n\n(3) I think we definitely need some tooling added to glance-manage to list locked images and unlock them.  I think you\u0027re storing the task UUID as part of the value of the os_glance_import_task property?  The cleaning code could verify that the task is in a terminal state before releasing the lock on an image (though I guess that wouldn\u0027t help if there\u0027s been a catastrophic failure and the task states are all inaccurate).  We\u0027ll probably need some more elaborate cleanup code like Dan describes above.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e3766d01c5df129a9cbc09b14d66f5659cfa3901","unresolved":false,"context_lines":[{"line_number":143,"context_line":"                                                       \u0027task\u0027: self.task_id})"},{"line_number":144,"context_line":"            raise"},{"line_number":145,"context_line":""},{"line_number":146,"context_line":"    def revert(self, result, **kwargs):"},{"line_number":147,"context_line":"        \"\"\""},{"line_number":148,"context_line":"        Clean up `os_glance_import_task` image property"},{"line_number":149,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_a677a83b","line":146,"updated":"2020-07-06 16:03:52.000000000","message":"Definitely nicer to have this revert happen in a single place as part of the flow abort.","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"8fc1ff26a4f724a84066e5e0cbed8e0e03ed0ac5","unresolved":false,"context_lines":[{"line_number":150,"context_line":"        :param result: taskflow result object"},{"line_number":151,"context_line":"        \"\"\""},{"line_number":152,"context_line":"        if isinstance(result, failure.Failure) and isinstance("},{"line_number":153,"context_line":"                result.exception, exception.Conflict):"},{"line_number":154,"context_line":"            return"},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":12,"id":"bf51134e_b72f8d86","line":153,"range":{"start_line":153,"start_character":34,"end_line":153,"end_character":52},"updated":"2020-07-09 21:32:19.000000000","message":"agree with Dan that we want this to be super-local so that it could only have come from our own execute().","commit_id":"7b2dd00e7efc3e002e37863568f30392ad9f192d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9283cee5072e1e9c6758b8c5ac9798f6360e2e9a","unresolved":false,"context_lines":[{"line_number":627,"context_line":""},{"line_number":628,"context_line":"    with action_wrapper as action:"},{"line_number":629,"context_line":"        if import_method !\u003d \u0027copy-image\u0027:"},{"line_number":630,"context_line":"            action.set_image_status(\u0027importing\u0027)"},{"line_number":631,"context_line":"        action.add_importing_stores(stores)"},{"line_number":632,"context_line":"        action.remove_failed_stores(stores)"},{"line_number":633,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_cd6affeb","side":"PARENT","line":630,"updated":"2020-07-17 20:21:48.000000000","message":"This changes the API behavior for other types of import operations. Right now, when the API returns 202, the image has already changed to importing. After this change, a quick re-poll may see the image as whatever the original state was and not importing, which may cause a polling loop to fail. It\u0027s already unfortunate that copy-image doesn\u0027t get an externally-visible indication that things are happening, so I wouldn\u0027t extend that to the rest of the methods :)","commit_id":"2a51843138e27071bf84269f6b2a601b3ba9978f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9283cee5072e1e9c6758b8c5ac9798f6360e2e9a","unresolved":false,"context_lines":[{"line_number":302,"context_line":"                        \u0027os_glance_import_task\u0027] \u003d\u003d \"Pending\":"},{"line_number":303,"context_line":"                    with self.action_wrapper as action:"},{"line_number":304,"context_line":"                        action.remove_import_task()"},{"line_number":305,"context_line":"            # Handle race condition"},{"line_number":306,"context_line":"            with self.action_wrapper as action:"},{"line_number":307,"context_line":"                action.set_import_task(self.task_id)"},{"line_number":308,"context_line":"                if self.import_method !\u003d \u0027copy-image\u0027:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_adb86be5","line":305,"range":{"start_line":305,"start_character":12,"end_line":305,"end_character":35},"updated":"2020-07-17 20:21:48.000000000","message":"Right here, you open the race back up. You\u0027ve removed the Pending and saved the image, which means another API thread has the opportunity to jump in here. You will fail on L307, but you\u0027ve already reported to the API client of *this* thread that you\u0027re going to do what they asked. So if there\u0027s a race, the two threads can invert their positions...the first one that grabbed the lock will die and the later one that should have been kept out will succeed.\n\nWill taskflow run the revert() of this step if execute() fails? If so, then you will delete the *other* thread\u0027s pending lock as you try to clean up your own.\n\nOne benefit of using the task ID as the value is that we *can* check to make sure we\u0027re reverting our own lock and not someone else\u0027s, but if everyone uses the same pending value we can\u0027t do that.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b2b1af762e968bbe6cbf3efb1a7cdcc76180d7e4","unresolved":false,"context_lines":[{"line_number":302,"context_line":"                        \u0027os_glance_import_task\u0027] \u003d\u003d \"Pending\":"},{"line_number":303,"context_line":"                    with self.action_wrapper as action:"},{"line_number":304,"context_line":"                        action.remove_import_task()"},{"line_number":305,"context_line":"            # Handle race condition"},{"line_number":306,"context_line":"            with self.action_wrapper as action:"},{"line_number":307,"context_line":"                action.set_import_task(self.task_id)"},{"line_number":308,"context_line":"                if self.import_method !\u003d \u0027copy-image\u0027:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_3d78f729","line":305,"range":{"start_line":305,"start_character":12,"end_line":305,"end_character":35},"in_reply_to":"bf51134e_25a0e791","updated":"2020-07-20 14:07:22.000000000","message":"\u003e Are you saying that the set_property_atomic is not atomic after all\n \u003e and you can just overwrite it by calling it again?\n\nNo? You\u0027re not overwriting it, you\u0027re deleting it and then re-adding it. If you *update* the value in place, that is atomic. Deleting and setting it again opens a gap where it\u0027s unset. That\u0027s the situation I describe above, where the newer thread can jump in front of the old one.\n\n \u003e I understood\n \u003e that as long as that we go that route instead of the normal update\n \u003e and save dance we never have collision like you described.\n\nAs I explained above, since you\u0027ve deleted the value, only one of the old and new thread will succeed in calling the atomic set, but you can\u0027t be guaranteed that it will be *this* one. Since *this* one is the one that originally won the lock race, reported 202 to the client, we want to exclude other tasks from starting until this one finishes. If we instead swap places with another task,\nwe have to fail as we\u0027ve lost our lock. If that happens continually, nothing makes progress.\n\n \u003e Updating the pending here to task id I can see being problematic\n \u003e due to the shortish self healing timeout of the pending soft lock\n \u003e for the api layer (that\u0027s why release and atomic lock again), but I\n \u003e do not see how two tasks would go beyond this point unless the\n \u003e set_property_atomic is not atomic after all.\n\nI didn\u0027t say \"two tasks would go beyond this point\", I said that this task will voluntarily drop its lock, and then fail to re-gain it on L307, because the other task grabbed it. We will fail, so two will not proceed, but you end up with a DoS potential, and you have lied to your client when you reported 202, indicating that the request would be acted upon.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"a6377b1ff8a1a43d1719e4e361052687e7a9b484","unresolved":false,"context_lines":[{"line_number":302,"context_line":"                        \u0027os_glance_import_task\u0027] \u003d\u003d \"Pending\":"},{"line_number":303,"context_line":"                    with self.action_wrapper as action:"},{"line_number":304,"context_line":"                        action.remove_import_task()"},{"line_number":305,"context_line":"            # Handle race condition"},{"line_number":306,"context_line":"            with self.action_wrapper as action:"},{"line_number":307,"context_line":"                action.set_import_task(self.task_id)"},{"line_number":308,"context_line":"                if self.import_method !\u003d \u0027copy-image\u0027:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_c3babcbe","line":305,"range":{"start_line":305,"start_character":12,"end_line":305,"end_character":35},"in_reply_to":"bf51134e_3d78f729","updated":"2020-07-20 15:49:05.000000000","message":"\"\"\"10.2.3 202 Accepted\n\nThe request has been accepted for processing, but the processing has not been completed. The request might or might not eventually be acted upon, as it might be disallowed when processing actually takes place. There is no facility for re-sending a status code from an asynchronous operation such as this.\n\nThe 202 response is intentionally non-committal. Its purpose is to allow a server to accept a request for some other process (perhaps a batch-oriented process that is only run once per day) without requiring that the user agent\u0027s connection to the server persist until the process is completed. The entity returned with this response SHOULD include an indication of the request\u0027s current status and either a pointer to a status monitor or some estimate of when the user can expect the request to be fulfilled. \"\"\"\n- https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html\n\nLike mentioned before, with 202 we never made that promise. We just responded that \"Your request has everything that needs to be there for success and we assume it will be processed eventually\" The 202 is very clear not promising success, just indicating that the request is acceptable for processing.\n\nI think this is where our understanding differs as you seem to think that because we accepted the request, there is desired result guaranteed, which is not the case and all this is being implemented to make the clients life easier. If you (as main stakeholder trying to implement the usecase for it) disagree with that being useful, we can just drop the indication and truly leave the guess work to the client side. But personally I do think that anything we can give to the client to indicate where we are at towards success or failure would be improving the user experience.\n\nI\u0027m open for the DoS potentian if you can show me an example how to exploit it. AFAIK being able to build exploit for this would require either very specific and predictable timing or non-ratelimited API and malicious already trusted user (as this is policy controlled for 3rd party already).","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"2aa71d7f19360180398c32c0bc4d3e678b8a39d8","unresolved":false,"context_lines":[{"line_number":302,"context_line":"                        \u0027os_glance_import_task\u0027] \u003d\u003d \"Pending\":"},{"line_number":303,"context_line":"                    with self.action_wrapper as action:"},{"line_number":304,"context_line":"                        action.remove_import_task()"},{"line_number":305,"context_line":"            # Handle race condition"},{"line_number":306,"context_line":"            with self.action_wrapper as action:"},{"line_number":307,"context_line":"                action.set_import_task(self.task_id)"},{"line_number":308,"context_line":"                if self.import_method !\u003d \u0027copy-image\u0027:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_25a0e791","line":305,"range":{"start_line":305,"start_character":12,"end_line":305,"end_character":35},"in_reply_to":"bf51134e_adb86be5","updated":"2020-07-20 01:17:03.000000000","message":"Are you saying that the set_property_atomic is not atomic after all and you can just overwrite it by calling it again? I understood that as long as that we go that route instead of the normal update and save dance we never have collision like you described.\n\nUpdating the pending here to task id I can see being problematic due to the shortish self healing timeout of the pending soft lock for the api layer (that\u0027s why release and atomic lock again), but I do not see how two tasks would go beyond this point unless the set_property_atomic is not atomic after all. \n\nYes it will execute revert(), catch on lines 325-326 that we failed here and not delete it.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c44f1abc8cb37f360197c417e6a69a963955bdf2","unresolved":false,"context_lines":[{"line_number":302,"context_line":"                        \u0027os_glance_import_task\u0027] \u003d\u003d \"Pending\":"},{"line_number":303,"context_line":"                    with self.action_wrapper as action:"},{"line_number":304,"context_line":"                        action.remove_import_task()"},{"line_number":305,"context_line":"            # Handle race condition"},{"line_number":306,"context_line":"            with self.action_wrapper as action:"},{"line_number":307,"context_line":"                action.set_import_task(self.task_id)"},{"line_number":308,"context_line":"                if self.import_method !\u003d \u0027copy-image\u0027:"}],"source_content_type":"text/x-python","patch_set":13,"id":"bf51134e_7b8f1475","line":305,"range":{"start_line":305,"start_character":12,"end_line":305,"end_character":35},"in_reply_to":"bf51134e_c3babcbe","updated":"2020-07-20 18:11:52.000000000","message":"\u003e Like mentioned before, with 202 we never made that promise. We just\n \u003e responded that \"Your request has everything that needs to be there\n \u003e for success and we assume it will be processed eventually\" The 202\n \u003e is very clear not promising success, just indicating that the\n \u003e request is acceptable for processing.\n\nBut we don\u0027t \"have everything that needs to be there for success.\" One of the things required for success, which is implied given Glance doesn\u0027t have a serialization or queuing mechanism, is \"you\u0027re the only one asking for an import operation on this image right now.\" If we put the request into a queue so it would be processed before or after all the other competing ones, then I\u0027d totally agree with you :)\n\n \u003e I think this is where our understanding differs as you seem to\n \u003e think that because we accepted the request, there is desired result\n \u003e guaranteed, which is not the case and all this is being implemented\n \u003e to make the clients life easier. If you (as main stakeholder trying\n \u003e to implement the usecase for it) disagree with that being useful,\n \u003e we can just drop the indication and truly leave the guess work to\n \u003e the client side. But personally I do think that anything we can\n \u003e give to the client to indicate where we are at towards success or\n \u003e failure would be improving the user experience.\n\nI\u0027m not sure how we could be providing *less* information to the client than we are. I\u0027m assuming you\u0027re just saying that to indicate that I should be happy with what I\u0027ve got and not complain -- not that you actually want to provide less info right? :)\n\nI _am_ the stakeholder here, and AFAIK, I\u0027m potentially the only one who has written a client-side cloudy use of this feature. Even being a developer, able to talk to the other developers, read and modify the code, etc, it\u0027s almost unusable as it is. All I\u0027m trying to do is bring my experience _actually_ writing a client against this to help make it better.\n\n \u003e I\u0027m open for the DoS potentian if you can show me an example how to\n \u003e exploit it. AFAIK being able to build exploit for this would\n \u003e require either very specific and predictable timing or\n \u003e non-ratelimited API and malicious already trusted user (as this is\n \u003e policy controlled for 3rd party already).\n\nAs I said above, the DoS would be a constant state of priority inversion where I create new tasks at a rate such that I\u0027m consistently jumping into this gap where you have dropped the lock and cause no tasks to make forward progress. If I can consistently cause new tasks to grab the lock while you drop it, then no task will ever make it past this point.","commit_id":"221672a1bf111e50fa00cb69004c00d238c45cc4"}],"glance/db/__init__.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"715adb7190e491072784b529fbcbf961d2c0e930","unresolved":false,"context_lines":[{"line_number":220,"context_line":"        new_values \u003d self.db_api.image_destroy(self.context, image.image_id)"},{"line_number":221,"context_line":"        image.updated_at \u003d new_values[\u0027updated_at\u0027]"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def update_property_atomic(self, image_id, name, value):"},{"line_number":224,"context_line":"        self.db_api.image_set_property_atomic("},{"line_number":225,"context_line":"            image_id, name, value)"},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_20248e5d","line":223,"updated":"2020-06-26 15:48:19.000000000","message":"Also this should be \"set_property_atomic\" since it can only set properties that are not set (i.e. not present or deleted).","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"596d66e75b41103418db7c2a891bfaab0ef82e0e","unresolved":false,"context_lines":[{"line_number":220,"context_line":"        new_values \u003d self.db_api.image_destroy(self.context, image.image_id)"},{"line_number":221,"context_line":"        image.updated_at \u003d new_values[\u0027updated_at\u0027]"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def update_property_atomic(self, image_id, name, value):"},{"line_number":224,"context_line":"        self.db_api.image_set_property_atomic("},{"line_number":225,"context_line":"            image_id, name, value)"},{"line_number":226,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_20800ef5","line":223,"range":{"start_line":223,"start_character":37,"end_line":223,"end_character":45},"updated":"2020-06-26 15:55:23.000000000","message":"Also, shouldn\u0027t this take image instead of image_id like the rest of these repo methods?","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c8d7c7287b2952c8f95f2edbce66148cbee933a4","unresolved":false,"context_lines":[{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def update_property_atomic(self, image_id, name, value):"},{"line_number":224,"context_line":"        self.db_api.image_set_property_atomic("},{"line_number":225,"context_line":"            image_id, name, value)"},{"line_number":226,"context_line":""},{"line_number":227,"context_line":""},{"line_number":228,"context_line":"class ImageProxy(glance.domain.proxy.Image):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_f547f61b","line":225,"updated":"2020-06-26 14:41:45.000000000","message":"Ah, this should go back to my patch to add it.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"}],"glance/db/simple/api.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"715adb7190e491072784b529fbcbf961d2c0e930","unresolved":false,"context_lines":[{"line_number":433,"context_line":"        LOG.warn(_LW(\u0027Could not find image %s\u0027), image_id)"},{"line_number":434,"context_line":"        raise exception.ImageNotFound()"},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"    prop \u003d _image_property_format(\u0027image_id\u0027,"},{"line_number":437,"context_line":"                                  \u0027name\u0027,"},{"line_number":438,"context_line":"                                  \u0027value\u0027)"},{"line_number":439,"context_line":"    image[\u0027properties\u0027].append(prop)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_60c84673","line":436,"range":{"start_line":436,"start_character":34,"end_line":436,"end_character":44},"updated":"2020-06-26 15:48:19.000000000","message":"This should be image_id instead of \u0027image_id\u0027, along with name and value below.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"552932ac3d5930dc11b826241a0f640c7353b791","unresolved":false,"context_lines":[{"line_number":427,"context_line":""},{"line_number":428,"context_line":""},{"line_number":429,"context_line":"def image_set_property_atomic(image_id, name, value):"},{"line_number":430,"context_line":"    try:"},{"line_number":431,"context_line":"        image \u003d DATA[\u0027images\u0027][image_id]"},{"line_number":432,"context_line":"    except KeyError:"},{"line_number":433,"context_line":"        LOG.warn(_LW(\u0027Could not find image %s\u0027), image_id)"},{"line_number":434,"context_line":"        raise exception.ImageNotFound()"},{"line_number":435,"context_line":""},{"line_number":436,"context_line":"    prop \u003d _image_property_format(\u0027image_id\u0027,"},{"line_number":437,"context_line":"                                  \u0027name\u0027,"},{"line_number":438,"context_line":"                                  \u0027value\u0027)"},{"line_number":439,"context_line":"    image[\u0027properties\u0027].append(prop)"},{"line_number":440,"context_line":""},{"line_number":441,"context_line":""},{"line_number":442,"context_line":"def _image_get(context, image_id, force_show_deleted\u003dFalse, status\u003dNone):"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_2ec92658","line":439,"range":{"start_line":430,"start_character":4,"end_line":439,"end_character":36},"updated":"2020-06-26 10:37:35.000000000","message":"This simple api is used in unit tests, need to change this code as per sqlalchemy api","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"}],"glance/db/sqlalchemy/api.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c8d7c7287b2952c8f95f2edbce66148cbee933a4","unresolved":false,"context_lines":[{"line_number":778,"context_line":"            setattr(image_ref, k, values[k])"},{"line_number":779,"context_line":""},{"line_number":780,"context_line":""},{"line_number":781,"context_line":"def image_set_property_atomic(context, image_id, name, value):"},{"line_number":782,"context_line":"    \"\"\""},{"line_number":783,"context_line":"    Atomically set an image property to a value."},{"line_number":784,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_b55bbef3","side":"PARENT","line":781,"range":{"start_line":781,"start_character":30,"end_line":781,"end_character":37},"updated":"2020-06-26 14:41:45.000000000","message":"We keep context as the first argument in a lot of our interfaces like this, and it looks to me that things like the big onion of proxy/helper interfaces assume some use of context. Obviously fine to remove it if you want, but I had it here for consistency.","commit_id":"e2efdea631d3ba7da1c0e70f3c24dc5a50cc002d"}],"glance/domain/proxy.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"cc4fc6102c0e9ffc6dc496f1a650c541d5e13bc3","unresolved":false,"context_lines":[{"line_number":104,"context_line":"        result \u003d self.base.remove(base_item)"},{"line_number":105,"context_line":"        return self.helper.proxy(result)"},{"line_number":106,"context_line":""},{"line_number":107,"context_line":"    def update_property_atomic(self, image_id, name, value):"},{"line_number":108,"context_line":"        self.base.update_property_atomic(image_id, name, value)"},{"line_number":109,"context_line":""},{"line_number":110,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_80191ad8","line":107,"updated":"2020-06-26 15:58:58.000000000","message":"Same, this should take item? Is the reason you didn\u0027t do this because this would only apply to images and not other things that this Repo can wrap? I\u0027m struggling to identify the benefit all these proxy layers bring, but I\u0027m guessing it\u0027s ... \"legacy\".\n\nStill, I would say make this item and assert that item is an image if this could ever be not an image. I\u0027ll change this as I\u0027m pulling this stuff back into my base patch, but if there\u0027s some reason it needs to not be this way, I\u0027ll change it back.","commit_id":"78cf10223969c77677e5721430a313c22aba7eee"}],"glance/tests/unit/v2/test_images_resource.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d72bf4430204b08348a5c4eeabc441524cdff4af","unresolved":false,"context_lines":[{"line_number":740,"context_line":"            # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not"},{"line_number":741,"context_line":"            # executing. Once it is fixed instead of mocking spawn_n method"},{"line_number":742,"context_line":"            # we should mock execute method of _ImportToStore task."},{"line_number":743,"context_line":"            with mock.patch.object("},{"line_number":744,"context_line":"                    glance.api.authorization.ImageRepoProxy, \u0027save\u0027):"},{"line_number":745,"context_line":"                with mock.patch.object(eventlet.GreenPool, \u0027spawn_n\u0027,"},{"line_number":746,"context_line":"                                       side_effect\u003dValueError):"},{"line_number":747,"context_line":"                    self.assertRaises(webob.exc.HTTPBadRequest,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_d9e936e3","line":744,"range":{"start_line":743,"start_character":0,"end_line":744,"end_character":69},"updated":"2020-06-29 17:12:52.000000000","message":"Is this still needed? And if you had to mock this out before, why are you not having to mock out the atomic operation? Maybe because the simple backend doesn\u0027t really do anything?\n\nSame goes for all the other new save() mocks below.","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"5ebb88c5b2e75ae611e44c1da6d55a97c96d04a7","unresolved":false,"context_lines":[{"line_number":740,"context_line":"            # https://bugs.launchpad.net/glance/+bug/1712463 taskflow is not"},{"line_number":741,"context_line":"            # executing. Once it is fixed instead of mocking spawn_n method"},{"line_number":742,"context_line":"            # we should mock execute method of _ImportToStore task."},{"line_number":743,"context_line":"            with mock.patch.object("},{"line_number":744,"context_line":"                    glance.api.authorization.ImageRepoProxy, \u0027save\u0027):"},{"line_number":745,"context_line":"                with mock.patch.object(eventlet.GreenPool, \u0027spawn_n\u0027,"},{"line_number":746,"context_line":"                                       side_effect\u003dValueError):"},{"line_number":747,"context_line":"                    self.assertRaises(webob.exc.HTTPBadRequest,"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_dea755a7","line":744,"range":{"start_line":743,"start_character":0,"end_line":744,"end_character":69},"in_reply_to":"bf51134e_d9e936e3","updated":"2020-06-30 10:22:36.000000000","message":"removed, this is not required now","commit_id":"03183f87af88fb84bcfc5f5a10b4be4e269752bc"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":5272,"context_line":"                        disk_format\u003d\u0027raw\u0027,"},{"line_number":5273,"context_line":"                        container_format\u003d\u0027bare\u0027,"},{"line_number":5274,"context_line":"                        status\u003d\u0027uploading\u0027,"},{"line_number":5275,"context_line":"                        properties\u003d{\u0027os_glance_import_task\u0027: \u0027fake-taks-uuid\u0027},"},{"line_number":5276,"context_line":"                        created_at\u003dDATETIME + datetime.timedelta(seconds\u003d1)),"},{"line_number":5277,"context_line":"            _db_fixture(UUID9, owner\u003dTENANT3,"},{"line_number":5278,"context_line":"                        name\u003d9,"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_c5d0f956","line":5275,"range":{"start_line":5275,"start_character":67,"end_line":5275,"end_character":71},"updated":"2020-07-02 13:42:58.000000000","message":"Probably doesn\u0027t matter but ... s/taks/task/","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6853d46b8bdd2f5a8ebadf1f883bd05b20352724","unresolved":false,"context_lines":[{"line_number":5322,"context_line":"        for prop in image[\u0027properties\u0027]:"},{"line_number":5323,"context_line":"            self.assertEqual(UUID9, prop[\u0027image_id\u0027])"},{"line_number":5324,"context_line":"            self.assertEqual(\u0027os_glance_import_task\u0027, prop[\u0027name\u0027])"},{"line_number":5325,"context_line":"            self.assertIsNotNone(prop[\u0027value\u0027])"},{"line_number":5326,"context_line":"            self.assertEqual(False, prop[\u0027deleted\u0027])"},{"line_number":5327,"context_line":""},{"line_number":5328,"context_line":"    def test_image_import_with_active_image(self):"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_a56345b1","line":5325,"range":{"start_line":5325,"start_character":12,"end_line":5325,"end_character":47},"updated":"2020-07-02 13:42:58.000000000","message":"You could mock out the task creation to get the task_id, or at least assert this is a uuid...","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"1e783abbcba60cfe3197ddf8450ab4d8630034dc","unresolved":false,"context_lines":[{"line_number":5322,"context_line":"        for prop in image[\u0027properties\u0027]:"},{"line_number":5323,"context_line":"            self.assertEqual(UUID9, prop[\u0027image_id\u0027])"},{"line_number":5324,"context_line":"            self.assertEqual(\u0027os_glance_import_task\u0027, prop[\u0027name\u0027])"},{"line_number":5325,"context_line":"            self.assertIsNotNone(prop[\u0027value\u0027])"},{"line_number":5326,"context_line":"            self.assertEqual(False, prop[\u0027deleted\u0027])"},{"line_number":5327,"context_line":""},{"line_number":5328,"context_line":"    def test_image_import_with_active_image(self):"}],"source_content_type":"text/x-python","patch_set":10,"id":"bf51134e_608e3bd1","line":5325,"range":{"start_line":5325,"start_character":12,"end_line":5325,"end_character":47},"in_reply_to":"bf51134e_a56345b1","updated":"2020-07-02 14:49:44.000000000","message":"will try these suggestions","commit_id":"77fcb9c8a301f758eddeb0729fb96919408b4c19"}]}