)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":8122,"name":"Cyril Roelandt","email":"cyril@redhat.com","username":"cyril.roelandt.enovance"},"change_message_id":"69c9940667c3974cadf34a017bbeeaacc7576feb","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Fix possible race conditions during status change"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"To eliminate potential race conditions when image status"},{"line_number":10,"context_line":"is changed it\u0027s suggested to use \u0027from_state\u0027 parameter"},{"line_number":11,"context_line":"for \u0027save\u0027 methods everywhere where it\u0027s possible."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Also this code prevents image update when status hasn\u0027t"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"5a18252c_41742efc","line":10,"range":{"start_line":10,"start_character":16,"end_line":10,"end_character":25},"updated":"2016-04-13 13:54:20.000000000","message":"By whom?","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":11391,"name":"Mikhail Fedosin","email":"mfedosin@redhat.com","username":"fedosinme"},"change_message_id":"736f4963f04471db96c3a2cd3d36cbe1e2a7fe6b","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Fix possible race conditions during status change"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"To eliminate potential race conditions when image status"},{"line_number":10,"context_line":"is changed it\u0027s suggested to use \u0027from_state\u0027 parameter"},{"line_number":11,"context_line":"for \u0027save\u0027 methods everywhere where it\u0027s possible."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Also this code prevents image update when status hasn\u0027t"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"dab17558_261f19b8","line":10,"range":{"start_line":10,"start_character":16,"end_line":10,"end_character":25},"in_reply_to":"5a18252c_41742efc","updated":"2016-05-17 12:23:25.000000000","message":"By him I think https://review.openstack.org/#/c/52409/","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"}],"glance/api/v2/image_actions.py":[{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"94ff44baf23d6cb2019d184265290d416c7ac470","unresolved":false,"context_lines":[{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status !\u003d \u0027deactivated\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"},{"line_number":54,"context_line":"            raise webob.exc.HTTPNotFound(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_9df1d6d0","line":51,"range":{"start_line":51,"start_character":15,"end_line":51,"end_character":59},"updated":"2016-03-15 14:42:36.000000000","message":"Should this be:\n\n if status \u003d\u003d \u0027active\u0027:\n      image_repo.save(image, from_state\u003d\u0027active\u0027)\n\n\nI think (from valid_state_targets in glance/domain/__init__.py) that only active-\u003edeactivated is allowed. (eg killed-\u003edeactivated isn\u0027t allowed, so the save will fail.)","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"},{"author":{"_account_id":11391,"name":"Mikhail Fedosin","email":"mfedosin@redhat.com","username":"fedosinme"},"change_message_id":"86457b174b9d12d5e372f82da6a651cdf62fbd5a","unresolved":false,"context_lines":[{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status !\u003d \u0027deactivated\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"},{"line_number":54,"context_line":"            raise webob.exc.HTTPNotFound(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_6179c02f","line":51,"range":{"start_line":51,"start_character":15,"end_line":51,"end_character":59},"in_reply_to":"1af94dfe_9df1d6d0","updated":"2016-03-15 16:20:57.000000000","message":"Done","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"94ff44baf23d6cb2019d184265290d416c7ac470","unresolved":false,"context_lines":[{"line_number":66,"context_line":"            status \u003d image.status"},{"line_number":67,"context_line":"            image.reactivate()"},{"line_number":68,"context_line":"            # not necessary to change the status if it\u0027s already \u0027active\u0027"},{"line_number":69,"context_line":"            if status !\u003d \u0027active\u0027:"},{"line_number":70,"context_line":"                image_repo.save(image, from_state\u003d\u0027deactivated\u0027)"},{"line_number":71,"context_line":"            LOG.info(_LI(\"Image %s is reactivated\"), image_id)"},{"line_number":72,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_1db026dd","line":69,"range":{"start_line":69,"start_character":12,"end_line":69,"end_character":34},"updated":"2016-03-15 14:42:36.000000000","message":"Similarly, should this be:\n\n if status \u003d\u003d \u0027deactivated\u0027","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"},{"author":{"_account_id":11391,"name":"Mikhail Fedosin","email":"mfedosin@redhat.com","username":"fedosinme"},"change_message_id":"86457b174b9d12d5e372f82da6a651cdf62fbd5a","unresolved":false,"context_lines":[{"line_number":66,"context_line":"            status \u003d image.status"},{"line_number":67,"context_line":"            image.reactivate()"},{"line_number":68,"context_line":"            # not necessary to change the status if it\u0027s already \u0027active\u0027"},{"line_number":69,"context_line":"            if status !\u003d \u0027active\u0027:"},{"line_number":70,"context_line":"                image_repo.save(image, from_state\u003d\u0027deactivated\u0027)"},{"line_number":71,"context_line":"            LOG.info(_LI(\"Image %s is reactivated\"), image_id)"},{"line_number":72,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_c176d43d","line":69,"range":{"start_line":69,"start_character":12,"end_line":69,"end_character":34},"in_reply_to":"1af94dfe_1db026dd","updated":"2016-03-15 16:20:57.000000000","message":"Done","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"b0654ea803fb20e67bf03fa77e77d974d4832a5e","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1af94dfe_e0c4a141","line":50,"updated":"2016-03-15 19:03:00.000000000","message":"FWIW, this doesn\u0027t prevent the race condition. If the change/write happens after the `if` but before the `save` call, the race will happen anyway. This, in fact, is a mistake we\u0027ve made in several other places in Glance.\n\nI\u0027m holding this off until Newton so we can start fixing race conditions properly","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":6159,"name":"Flavio Percoco Premoli","display_name":"flaper87","email":"flavio.percoco@flyrlabs.com","username":"flaper87"},"change_message_id":"7bfec9d6ca3ee136945495c4c4acac8bd6cf67be","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_669b2aca","line":50,"updated":"2016-04-25 14:54:35.000000000","message":"I just re-read the patch and paid more attention to the `from_state` keyword being used here. I think that\u0027s the only thing that actually fixes the race. Question: Why did you add this if statement then? If we\u0027re using `from_state` the if statement shouldn\u0027t be required.\n\nStuart, the examples you used are correct. However, you\u0027re missing other examples where 2 requests want to do 2 different things. Say, A wants to (de|re)activate the image and B wants to delete it. Without the `from_state` keyword, this code would still be broken.","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"74a2a6649fd16eea8f16ccd025e52f6bbb53e449","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_833af896","line":50,"in_reply_to":"1a122d0e_669b2aca","updated":"2016-05-03 09:25:55.000000000","message":"\u003e However, you\u0027re missing other examples where 2 requests want to do 2 different things.\n\nNot really. (The logic is the same whether or not the requests are doing the same or different things.)\n\n\u003e Without the `from_state` keyword, this code would still be broken.\n\nAgreed. (Obviously the patch does provide the from_state.)\n\n\u003e Why did you add this if statement then?\n\nIt\u0027s an optimization. It is only \u0027legal\u0027 to deactivate an active image. (ie when you look at the allowed state transitions.) If the image is not currently active there is no need to attempt to deactivate it.\n\nNow ... does the \u0027if\u0027 introduce a new race condition? I don\u0027t think so. If there are two requests to deactivate an image one will \u0027win\u0027. It doesn\u0027t matter which of the two is the winner. Most of the time we will have a clear winner and the \u0027if\u0027 statement will save the loser from attempting to write to the database.\n\nIf we have one request attempting to deactivate and another attempting to delete the logic is the same. If the delete \u0027wins\u0027 the state will have changed either before or after the \u0027if\u0027 statement. If before then we save having to talk to the database. If after we rely on the from_state.","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":11391,"name":"Mikhail Fedosin","email":"mfedosin@redhat.com","username":"fedosinme"},"change_message_id":"736f4963f04471db96c3a2cd3d36cbe1e2a7fe6b","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"dab17558_066655ba","line":50,"in_reply_to":"1a122d0e_833af896","updated":"2016-05-17 12:23:25.000000000","message":"Oh... I\u0027ve already forgotten about this patch. I think there is some misunderstanding why we need \u0027if\u0027 here.\n\nSo, the issue in image.deactivate() call. If you look at the code https://github.com/openstack/glance/blob/master/glance/domain/__init__.py#L249 you may see that 2 statuses are valid: active and deactivated, and in the second case glance does nothing in this method and just rewrites all data in db (i.e. sends a request to db \"change status from \u0027deactivated\u0027 to \u0027deactivated\u0027 and also update all image data). But from_state takes only one parameter, not a list, we cannot say from_state\u003d(\u0027active\u0027, deactivated\u0027). So this \u0027if\u0027 is needed here because we eliminated \u0027deactivated\u0027 path and if image was already \u0027deactivated\u0027 we just return 204 response immediately.","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"608e6a6b1f5d96ecd5d58e251ab8467611eba85f","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_fbfc3e40","line":50,"in_reply_to":"1af94dfe_0926facd","updated":"2016-04-12 16:06:12.000000000","message":"I think this is ok.\n\nI think it might be a little bit clearer to\ndo it like this:\n\n\n\n status \u003d image.status\n if status \u003d\u003d \u0027active\u0027:\n     image.deactivate()\n     image_repo.save(image, from_state\u003d\u0027active\u0027)\n\nwe avoid the race by having the from_state\u003d\u0027active\u0027. (If the state races -- changes to something else after the if statement -- the db update will error.)","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"9d346d75b63e95281f03bb1bac68d398707c2c86","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1af94dfe_0926facd","line":50,"in_reply_to":"1af94dfe_e0c4a141","updated":"2016-03-16 10:33:43.000000000","message":"Hmm.\n\nHere\u0027s my understanding of what\u0027s going on here (I may have it wrong).\n\nAssume that two requests (\u0027A\u0027 and \u0027B\u0027) are made virtually simultaneously.\n\nInitially the image is in an \u0027active\u0027 status.\n\nThe two requests now compete to change the state from \u0027active\u0027 to \u0027deactivated\u0027.\n\nWith this patch there will be one winner and one loser, which (I think) avoids the race.\n\nImagine \u0027A\u0027 arrives just before \u0027B\u0027. This triggers a request to change the image state from \u0027active\u0027 to \u0027deactivated\u0027. This generates a read-modify-write style database transaction (I\u0027m assuming). Ie the current state is read, and if \u0027active\u0027 then the change to \u0027deactivated\u0027 goes ahead. While this transaction is being carried out nothing else can modify the value of that image status.\n\nNow \u0027B\u0027 arrives a fraction of a second later. This also wants to modify the image state from active to deactivated. Once we get to the database transaction and the current image state is seen to be \u0027deactivated\u0027 rather than \u0027active\u0027 the image should not be re-set to \u0027active\u0027. We should get an exception of some kind.\n\nSo we have guaranteed that at most one request will change the image to \u0027deactivated\u0027, and that request will succeed only if the current state of the image is \u0027active\u0027.\n\nThis logic is based on my understanding/assumption of our database transaction behaviour, ie that we really are doing atomic read-modify-write.\nI\u0027d need to test that/get someone else to confirm it.","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"3d4aba0d70b829a774110e48a064e5880ae2a941","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_94305403","line":50,"in_reply_to":"5a18252c_1f1e34ba","updated":"2016-04-13 10:38:04.000000000","message":"\u003e Basically we want to do a update image set status \u003d \u0027active\u0027 where id \u003d image_id and status \u003d from_state\n\nRight. But my understanding is that is what the code currently does.\n\n(I\u0027m not a db guy though, so could *definitely* be wrong.)\n\nLet me give you my understanding of the code, and the relevant sqlalchemy debug to see if we can get on the same page.\n\n\nIn glance/db/sqlalchemy/api.py\n\n\n\n def _image_update(context, values, image_id, purge_props\u003dFalse,\n                  from_state\u003dNone):\n  .\n  .\n  .\n  # We create a query\n  query \u003d session.query(models.Image).filter_by(id\u003dimage_id)\n  if from_state:\n    # we add to it (nothing is sent to the sql server yet)\n    query \u003d query.filter_by(status\u003dfrom_state)\n\n   .\n   .\n   . \n   # now sqlalchemy will send the single query we\u0027ve built up to the server\n   updated \u003d query.update(values, synchronize_session\u003d\u0027fetch\u0027)\n\n \nEnabling debug we see the single call with basically \"set status \u003d \u0027active\u0027 where id \u003d image_id and status \u003d from_state\" call:\n\n\n 2016-04-13 10:21:32.492 ^[[00;36mINFO sqlalchemy.engine.base.Engine   [^[[01;36mreq-e220a231-285c-4cf4-ae43-69930c9d7340  ^[[00;36m1a0468cca5ad40a7aaa2f6165a94d941  7870680f13f04593af23f765605ecc62^[[00;36m] ^[[01;35m^[[00;36mUPDATE  images SET updated_at\u003d%(updated_at)s, id\u003d%(id)s, name\u003d%(name)s,  disk_format\u003d%(disk_format)s, container_format\u003d%(container_format)s,  size\u003d%(size)s, virtual_size\u003d%(virtual_size)s, status\u003d%(status)s,  is_public\u003d%(is_public)s, checksum\u003d%(checksum)s, min_disk\u003d%(min_disk)s,  min_ram\u003d%(min_ram)s, owner\u003d%(owner)s, protected\u003d%(protected)s WHERE  images.id \u003d %(id_1)s AND images.status \u003d %(status_1)s^[[00m\n\n\nFrom the next line of debug you can see \u0027status is \u0027active\u0027 and status_1 is saving:\n\n 2016-04-13 10:21:32.492 ^[[00;36mINFO sqlalchemy.engine.base.Engine [^[[01;36mreq-e220a231-285c-4cf4-ae43-69930c9d7340 ^[[00;36m1a0468cca5ad40a7aaa2f6165a94d941 7870680f13f04593af23f765605ecc62^[[00;36m] ^[[01;35m^[[00;36m{\u0027status\u0027: \u0027active\u0027, \u0027name\u0027: None, \u0027checksum\u0027: \u00274215f4b77571603bee82ef427ea0ef84\u0027, \u0027min_ram\u0027: 0, \u0027disk_format\u0027: u\u0027raw\u0027, \u0027updated_at\u0027: datetime.datetime(2016, 4, 13, 10, 21, 32, 488177), \u0027min_disk\u0027: 0, u\u0027id_1\u0027: u\u00274e809a2f-9102-47ae-a19f-a47fc85881be\u0027, \u0027container_format\u0027: u\u0027bare\u0027, \u0027owner\u0027: u\u00277870680f13f04593af23f765605ecc62\u0027, \u0027is_public\u0027: 0, \u0027protected\u0027: 0, \u0027virtual_size\u0027: None, \u0027id\u0027: u\u00274e809a2f-9102-47ae-a19f-a47fc85881be\u0027, u\u0027status_1\u0027: \u0027saving\u0027, \u0027size\u0027: 37}^[[00m\n\nso the two states are contained in one db call.\n\nSo as far as I can see there is no race here.\n\nDoes that make sense, or am I mis-reading this?\n\nThanks!\n\n(I\u0027ve taken my +2 off until we get a common understanding.)","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":10035,"name":"greghaynes","email":"greg@greghaynes.net","username":"greghaynes"},"change_message_id":"9a8e37350300b9b96a25f2cd49edbcebc592eb11","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_1f1e34ba","line":50,"in_reply_to":"5a18252c_56ff0901","updated":"2016-04-12 23:09:00.000000000","message":"So, Flavio is correct - you can have two transactions succeed, both of them setting the same property on the same image and you get no guarantees on ordering with our current code (all that we are guaranteed is that they are linearizeable).\n\n\nThat being said, the API here actually has all the information needed to fix this. The proper way is to do a compare-and-swap using the from_state (reading dbapi, from_state is used in a separate query rather than inside of a single CAS query). Basically we want to do a update image set status \u003d \u0027active\u0027 where id \u003d image_id and status \u003d from_state; I think this requires breaking through sqlalchemy abstractions, though :(","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":10035,"name":"greghaynes","email":"greg@greghaynes.net","username":"greghaynes"},"change_message_id":"acb6337d2a67f09759a284e1018aa11b7cd7779b","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"1a122d0e_d674b9fe","line":50,"in_reply_to":"5a18252c_94305403","updated":"2016-04-16 14:57:04.000000000","message":"Ah, yep, you\u0027re correct - I was misreading the query appending as being separate queries. So then yes, this should either succeed in setting status to what we want or rollback :).","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"},{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"f1ee5aaf610d13e532a55417eea01336db309243","unresolved":false,"context_lines":[{"line_number":47,"context_line":"            status \u003d image.status"},{"line_number":48,"context_line":"            image.deactivate()"},{"line_number":49,"context_line":"            # not necessary to change the status if it\u0027s already \u0027deactivated\u0027"},{"line_number":50,"context_line":"            if status \u003d\u003d \u0027active\u0027:"},{"line_number":51,"context_line":"                image_repo.save(image, from_state\u003d\u0027active\u0027)"},{"line_number":52,"context_line":"            LOG.info(_LI(\"Image %s is deactivated\"), image_id)"},{"line_number":53,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5a18252c_56ff0901","line":50,"in_reply_to":"5a18252c_fbfc3e40","updated":"2016-04-12 16:08:16.000000000","message":"Also, if the initial state isn\u0027t active what does the code end up returning to the user - 409?","commit_id":"8708273d485c482007fc0ffafb1549fa2c68dae3"}],"glance/api/v2/image_data.py":[{"author":{"_account_id":455,"name":"Stuart McLaren","email":"stuart.mclaren@hpe.com","username":"stuart-mclaren"},"change_message_id":"94ff44baf23d6cb2019d184265290d416c7ac470","unresolved":false,"context_lines":[{"line_number":110,"context_line":"                                     \"Use the existing user token.\"),"},{"line_number":111,"context_line":"                                 encodeutils.exception_to_unicode(e))"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"                image_repo.save(image, from_state\u003d\u0027queued\u0027)"},{"line_number":114,"context_line":"                image.set_data(data, size)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_18aaf4d4","line":113,"range":{"start_line":113,"start_character":39,"end_line":113,"end_character":49},"updated":"2016-03-15 14:42:36.000000000","message":"Nice race fix :-)","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"},{"author":{"_account_id":11391,"name":"Mikhail Fedosin","email":"mfedosin@redhat.com","username":"fedosinme"},"change_message_id":"86457b174b9d12d5e372f82da6a651cdf62fbd5a","unresolved":false,"context_lines":[{"line_number":110,"context_line":"                                     \"Use the existing user token.\"),"},{"line_number":111,"context_line":"                                 encodeutils.exception_to_unicode(e))"},{"line_number":112,"context_line":""},{"line_number":113,"context_line":"                image_repo.save(image, from_state\u003d\u0027queued\u0027)"},{"line_number":114,"context_line":"                image.set_data(data, size)"},{"line_number":115,"context_line":""},{"line_number":116,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":2,"id":"1af94dfe_e193104e","line":113,"range":{"start_line":113,"start_character":39,"end_line":113,"end_character":49},"in_reply_to":"1af94dfe_18aaf4d4","updated":"2016-03-15 16:20:57.000000000","message":"lol :)","commit_id":"a692c63c749c717b86d914e89a34028746d8a1f5"}]}