)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7634,"name":"Takashi Natsume","email":"takanattie@gmail.com","username":"natsumet"},"change_message_id":"a2c81b98497a52e7c7a9cddad4a616487459ce59","unresolved":false,"context_lines":[{"line_number":14,"context_line":"inventory record for which there are existing allocations."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"SchedulerReportClient was previously identifying this error condition by"},{"line_number":17,"context_line":"parsing the text of the 409 response.  With this change, it instead uses"},{"line_number":18,"context_line":"the provided error code."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Ic621adcadf10cc607455eba48c4cb1882bde23fa"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5f7c97a3_3c8f7976","line":17,"range":{"start_line":17,"start_character":38,"end_line":17,"end_character":39},"updated":"2018-06-05 08:15:04.000000000","message":"nit: Extra white space.","commit_id":"226728c5985551d884e955e090b38c7767d66159"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e629275b42f3f2113fcecbd1fa8cb4cf43a99b74","unresolved":false,"context_lines":[{"line_number":14,"context_line":"inventory record for which there are existing allocations."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"SchedulerReportClient was previously identifying this error condition by"},{"line_number":17,"context_line":"parsing the text of the 409 response.  With this change, it instead uses"},{"line_number":18,"context_line":"the provided error code."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Change-Id: Ic621adcadf10cc607455eba48c4cb1882bde23fa"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"5f7c97a3_1c915d47","line":17,"range":{"start_line":17,"start_character":38,"end_line":17,"end_character":39},"in_reply_to":"5f7c97a3_3c8f7976","updated":"2018-06-15 21:10:45.000000000","message":"Done","commit_id":"226728c5985551d884e955e090b38c7767d66159"}],"nova/api/openstack/placement/errors.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"9db3804980be38d446a618a3dff83e7c49505326","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"# NOTE(efried): This module should become part of placement-lib whenever that"},{"line_number":38,"context_line":"# is a thing, so that API consumers can use these constants to identify the"},{"line_number":39,"context_line":"# relevant error conditions. This makes it even more important that we..."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"# Do not change the string values. Once set, they are set."},{"line_number":42,"context_line":"# Do not reuse string values. There should be only one symbol for any"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_00760c3f","line":39,"updated":"2018-05-15 16:43:55.000000000","message":"I\u0027m not sure I agree with this.\n\nWe want the codes to never change and we want placement (the service) to be authoritative. Clients will only care about some of them, and in order for them to care about them, they must first know about them (somehow). Once they know about them, they know a static thing.\n\nI\u0027d rather we not get in the habit of dumping information into library that becomes a sort of false requirement to be a client of placement. It creates the potential for two bad things:\n\n* complex clients being the norm\n* information coupling between the client and the server being mediated in the wrong place\n\nI\u0027m happy to be wrong on this, and I know we\u0027re not building placement-lib today, but I\u0027d rather not set this particular expectation.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dd2809b5a9f87e942a09aa0d5dd7010e87b3dfc2","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"# NOTE(efried): This module should become part of placement-lib whenever that"},{"line_number":38,"context_line":"# is a thing, so that API consumers can use these constants to identify the"},{"line_number":39,"context_line":"# relevant error conditions. This makes it even more important that we..."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"# Do not change the string values. Once set, they are set."},{"line_number":42,"context_line":"# Do not reuse string values. There should be only one symbol for any"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ab85b908","line":39,"in_reply_to":"5f7c97a3_00760c3f","updated":"2018-05-15 17:37:41.000000000","message":"Seems like we had a similar discussion about os-traits or resource classes or something.\n\nMy main reason to prefer clients import and use the symbols is that misspelling a macro will be caught at \"compile time\" whereas misspelling the value won\u0027t be caught until \"runtime\" - if at all - and be harder to debug at that point.\n\n \u003e I\u0027d rather we not get in the habit of dumping information into\n \u003e library that becomes a sort of false requirement to be a client of\n \u003e placement.\n\nMeh, it\u0027s not a requirement; if they want to hardcode the string, we can\u0027t stop \u0027em.  This just makes it possible for them to use the symbol if they want, for the reasons above.\n\n \u003e * information coupling between the client and the server being\n \u003e mediated in the wrong place\n\nImplying that IF we should ever remove/change a symbol or value, placement could be out of sync with this lib, and the lib would be wrong, because placement is authoritative?\n\nSo yeah, we can\u0027t change the lib for the same reason we couldn\u0027t change placement and vice versa.  If a client hardcodes the string, and placement changes, they\u0027re just as broken, but now instead of upgrading placement-lib, they have to change code.\n\nI\u0027m not super stuck on this, and will remove the comment if it\u0027s going to be contentious right now, but I\u0027m still seeing the pros outweighing the cons.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"8768bdeea7300830c5b881e4a7992a132bf1eb2f","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"# NOTE(efried): This module should become part of placement-lib whenever that"},{"line_number":38,"context_line":"# is a thing, so that API consumers can use these constants to identify the"},{"line_number":39,"context_line":"# relevant error conditions. This makes it even more important that we..."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"# Do not change the string values. Once set, they are set."},{"line_number":42,"context_line":"# Do not reuse string values. There should be only one symbol for any"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_e6d7684a","line":39,"in_reply_to":"5f7c97a3_ab85b908","updated":"2018-05-15 17:55:40.000000000","message":"\u003e Seems like we had a similar discussion about os-traits or resource\n \u003e classes or something.\n\nOr even both. Thus my \"I\"m an alien\" comment elsewhere.\n\n \u003e My main reason to prefer clients import and use the symbols is that\n \u003e misspelling a macro will be caught at \"compile time\" whereas\n \u003e misspelling the value won\u0027t be caught until \"runtime\" - if at all -\n \u003e and be harder to debug at that point.\n\nOn the other hand, you are obliged to have a test for this stuff anyway, hopefully functional, and that will confirm things at what for us is effectively compile time.\n\n \u003e \u003e I\u0027d rather we not get in the habit of dumping information into\n \u003e \u003e library that becomes a sort of false requirement to be a client\n \u003e of\n \u003e \u003e placement.\n \u003e \n \u003e Meh, it\u0027s not a requirement; if they want to hardcode the string,\n \u003e we can\u0027t stop \u0027em.  This just makes it possible for them to use the\n \u003e symbol if they want, for the reasons above.\n\nThus \"false\". The implication, however, is that one \"should\". And that\u0027s now how http apis ought to work on the client side. They should be dependency free where possible.\n\n \u003e I\u0027m not super stuck on this, and will remove the comment if it\u0027s\n \u003e going to be contentious right now, but I\u0027m still seeing the pros\n \u003e outweighing the cons.\n\nNor me, just getting my opinions out in the air.\n\nUnderlying all this is a concern that we\u0027re not buying into the extreme level of decoupling that I think an HTTP API is supposed to allow and imply. Each time we make a lib for the purpose of sharing, that strikes me as a statement of \"we\u0027re only doing HTTP here because we feel like we should, it\u0027s just what\u0027s done\" instead of \"we\u0027re doing HTTP here because we want to enable a multiplicity of consumers doing all sort of shit, some of which we never planned or thought of\".\n\nHaving a convenience library is a nice thing, but when it is used in the first real implementation of the client side it has the potential to set a precedent. Anyone coming into placement and using nova as a model for how to use it could conclude that they need os-traits, placement-lib, maybe os-resources-class, ProviderTrees, a complex client using keystoneauth, etc etc\n\nNone of which is true, of course, there are many ways to do it, but we\u0027re not making that clear.\n\nIf the nova side of the equation wants to use and advertise all those tools and conveniences that\u0027s great, but I think we should limit the advertising for those conveniences to the nova side and keep the placement side indicating that it is open for lots of different kinds of business.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d9016a389064684dcc97ea06e780c68171f858ee","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":"# NOTE(efried): This module should become part of placement-lib whenever that"},{"line_number":38,"context_line":"# is a thing, so that API consumers can use these constants to identify the"},{"line_number":39,"context_line":"# relevant error conditions. This makes it even more important that we..."},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"# Do not change the string values. Once set, they are set."},{"line_number":42,"context_line":"# Do not reuse string values. There should be only one symbol for any"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_666a781a","line":39,"in_reply_to":"5f7c97a3_e6d7684a","updated":"2018-05-15 18:19:38.000000000","message":"\u003e On the other hand, you are obliged to have a test for this stuff\n \u003e anyway, hopefully functional, and that will confirm things at what\n \u003e for us is effectively compile time.\n\nIf you mean in general, agreed, but tests can get missed.\n\nIf you mean specifically in this patch, we\u0027re covered by the existing tests - in particular in test_report_client - which demonstrate backward compatibility by virtue of not needing to be changed (except for the header setup thing).\n\n \u003e If the nova side of the equation wants to use and advertise all\n \u003e those tools and conveniences that\u0027s great, but I think we should\n \u003e limit the advertising for those conveniences to the nova side and\n \u003e keep the placement side indicating that it is open for lots of\n \u003e different kinds of business.\n\nI can buy that.\n\nI still think there\u0027s utility in making this part of placement-lib, and using it from nova in the way that I\u0027m doing in this patch, but we can defer the discussion of the former; I\u0027ll remove this comment for now.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"}],"nova/api/openstack/placement/handlers/inventory.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"9db3804980be38d446a618a3dff83e7c49505326","unresolved":false,"context_lines":[{"line_number":361,"context_line":"              % {\u0027rp_uuid\u0027: resource_provider.uuid})"},{"line_number":362,"context_line":"    except exception.InventoryInUse as ex:"},{"line_number":363,"context_line":"        # NOTE(mriedem): This message cannot change without impacting the"},{"line_number":364,"context_line":"        # nova.scheduler.client.report._RE_INV_IN_USE regex."},{"line_number":365,"context_line":"        raise webob.exc.HTTPConflict(explanation\u003dex.format_message())"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    response \u003d req.response"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_00204cf6","side":"PARENT","line":364,"updated":"2018-05-15 16:43:55.000000000","message":"This comment can go away because the client.report doesn\u0027t call this. Is that right?","commit_id":"ae131868f71700d69053b65a0a37f9c2d65c3770"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dd2809b5a9f87e942a09aa0d5dd7010e87b3dfc2","unresolved":false,"context_lines":[{"line_number":361,"context_line":"              % {\u0027rp_uuid\u0027: resource_provider.uuid})"},{"line_number":362,"context_line":"    except exception.InventoryInUse as ex:"},{"line_number":363,"context_line":"        # NOTE(mriedem): This message cannot change without impacting the"},{"line_number":364,"context_line":"        # nova.scheduler.client.report._RE_INV_IN_USE regex."},{"line_number":365,"context_line":"        raise webob.exc.HTTPConflict(explanation\u003dex.format_message())"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    response \u003d req.response"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ebbdf1b6","side":"PARENT","line":364,"in_reply_to":"5f7c97a3_00204cf6","updated":"2018-05-15 17:37:41.000000000","message":"Well, I suppose technically it could have gone away in [1] for that reason - it should have been moved to L325 at that point.\n\nBut no, the comment gets to disappear because we are no longer dependent on the text content of the message itself, and _RE_INV_IN_USE no longer exists.\n\n[1] https://review.openstack.org/#/c/539712/","commit_id":"ae131868f71700d69053b65a0a37f9c2d65c3770"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d9016a389064684dcc97ea06e780c68171f858ee","unresolved":false,"context_lines":[{"line_number":361,"context_line":"              % {\u0027rp_uuid\u0027: resource_provider.uuid})"},{"line_number":362,"context_line":"    except exception.InventoryInUse as ex:"},{"line_number":363,"context_line":"        # NOTE(mriedem): This message cannot change without impacting the"},{"line_number":364,"context_line":"        # nova.scheduler.client.report._RE_INV_IN_USE regex."},{"line_number":365,"context_line":"        raise webob.exc.HTTPConflict(explanation\u003dex.format_message())"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    response \u003d req.response"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_e60be88a","side":"PARENT","line":364,"in_reply_to":"5f7c97a3_ab6e19fd","updated":"2018-05-15 18:19:38.000000000","message":"Gotcha.  As of PS2 (after the rebase on [1]) we are returning the code.  Before that, this code path was dead nova-wise anyway.\n\n[1] https://review.openstack.org/568578","commit_id":"ae131868f71700d69053b65a0a37f9c2d65c3770"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5ecb7a62ba0c6dc23fe0fb1a09b1913fa95d55bd","unresolved":false,"context_lines":[{"line_number":361,"context_line":"              % {\u0027rp_uuid\u0027: resource_provider.uuid})"},{"line_number":362,"context_line":"    except exception.InventoryInUse as ex:"},{"line_number":363,"context_line":"        # NOTE(mriedem): This message cannot change without impacting the"},{"line_number":364,"context_line":"        # nova.scheduler.client.report._RE_INV_IN_USE regex."},{"line_number":365,"context_line":"        raise webob.exc.HTTPConflict(explanation\u003dex.format_message())"},{"line_number":366,"context_line":""},{"line_number":367,"context_line":"    response \u003d req.response"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ab6e19fd","side":"PARENT","line":364,"in_reply_to":"5f7c97a3_ebbdf1b6","updated":"2018-05-15 17:43:43.000000000","message":"except you\u0027re not returning the code here, that\u0027s why I\u0027m confused.","commit_id":"ae131868f71700d69053b65a0a37f9c2d65c3770"}],"nova/scheduler/client/report.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"9db3804980be38d446a618a3dff83e7c49505326","unresolved":false,"context_lines":[{"line_number":26,"context_line":"from oslo_utils import versionutils"},{"line_number":27,"context_line":"from six.moves.urllib import parse"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"from nova.api.openstack.placement import errors as placement_errors"},{"line_number":30,"context_line":"from nova.compute import provider_tree"},{"line_number":31,"context_line":"from nova.compute import utils as compute_utils"},{"line_number":32,"context_line":"import nova.conf"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_60a02091","line":29,"updated":"2018-05-15 16:43:55.000000000","message":"Since the code is a symbol in and of itself, importing a thing to represent the symbol strikes me as a bit redundant.\n\n(I recognize that I\u0027m a bit of an alien on this front, but I gotta review like I feel)","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dd2809b5a9f87e942a09aa0d5dd7010e87b3dfc2","unresolved":false,"context_lines":[{"line_number":26,"context_line":"from oslo_utils import versionutils"},{"line_number":27,"context_line":"from six.moves.urllib import parse"},{"line_number":28,"context_line":""},{"line_number":29,"context_line":"from nova.api.openstack.placement import errors as placement_errors"},{"line_number":30,"context_line":"from nova.compute import provider_tree"},{"line_number":31,"context_line":"from nova.compute import utils as compute_utils"},{"line_number":32,"context_line":"import nova.conf"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_4bce6563","line":29,"in_reply_to":"5f7c97a3_60a02091","updated":"2018-05-15 17:37:41.000000000","message":"See other comment.  Main reason I prefer this is because typos in symbols get caught earlier than typos in strings.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"9db3804980be38d446a618a3dff83e7c49505326","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    \"\"\""},{"line_number":231,"context_line":"    if resp.status_code !\u003d 409:"},{"line_number":232,"context_line":"        return"},{"line_number":233,"context_line":"    err \u003d resp.json()[\u0027errors\u0027][0]"},{"line_number":234,"context_line":"    if err[\u0027code\u0027] \u003d\u003d placement_errors.INVENTORY_INUSE:"},{"line_number":235,"context_line":"        # The error detail includes the resource class and provider."},{"line_number":236,"context_line":"        raise exception.InventoryInUse(err[\u0027detail\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_40d3a404","line":233,"updated":"2018-05-15 16:43:55.000000000","message":"If for whatever reason you get an IndexError here (just assume it does for sake of the discussion) where will that exception be caught? What will happen?","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"dd2809b5a9f87e942a09aa0d5dd7010e87b3dfc2","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    \"\"\""},{"line_number":231,"context_line":"    if resp.status_code !\u003d 409:"},{"line_number":232,"context_line":"        return"},{"line_number":233,"context_line":"    err \u003d resp.json()[\u0027errors\u0027][0]"},{"line_number":234,"context_line":"    if err[\u0027code\u0027] \u003d\u003d placement_errors.INVENTORY_INUSE:"},{"line_number":235,"context_line":"        # The error detail includes the resource class and provider."},{"line_number":236,"context_line":"        raise exception.InventoryInUse(err[\u0027detail\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ab5db979","line":233,"in_reply_to":"5f7c97a3_40d3a404","updated":"2018-05-15 17:37:41.000000000","message":"update_from_provider_tree will bail out early rather than doing its usual best-effort thing (attempting all providers\u0027 pieces of the sync operation even if one provider\u0027s sync fails).\n\nBeyond than that, it will bubble up through the resource tracker to the catch-all in update_available_resource_for_node, where it gets logged and ignored and we try again on the next periodic task interval.\n\nIs it possible for us to get a 4xx with an empty \u0027errors\u0027 list?  Or with no \u0027errors\u0027 key at all?  And if that happens, what should I do?  Assuming there\u0027s no error and silently passing feels wrong, since if something breaks the response thusly (like a header snafu...) it would mask real InventoryInUse errors.  Which is worse than an ugly IndexError/AttributeError/KeyError/ValueError stack trace in the logs IMO.\n\nThoughts?","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d9016a389064684dcc97ea06e780c68171f858ee","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    \"\"\""},{"line_number":231,"context_line":"    if resp.status_code !\u003d 409:"},{"line_number":232,"context_line":"        return"},{"line_number":233,"context_line":"    err \u003d resp.json()[\u0027errors\u0027][0]"},{"line_number":234,"context_line":"    if err[\u0027code\u0027] \u003d\u003d placement_errors.INVENTORY_INUSE:"},{"line_number":235,"context_line":"        # The error detail includes the resource class and provider."},{"line_number":236,"context_line":"        raise exception.InventoryInUse(err[\u0027detail\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_46fc1c6f","line":233,"in_reply_to":"5f7c97a3_ab47f976","updated":"2018-05-15 18:19:38.000000000","message":"Agreed.  But yeah, the catch-all is sufficient here I believe.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5ecb7a62ba0c6dc23fe0fb1a09b1913fa95d55bd","unresolved":false,"context_lines":[{"line_number":230,"context_line":"    \"\"\""},{"line_number":231,"context_line":"    if resp.status_code !\u003d 409:"},{"line_number":232,"context_line":"        return"},{"line_number":233,"context_line":"    err \u003d resp.json()[\u0027errors\u0027][0]"},{"line_number":234,"context_line":"    if err[\u0027code\u0027] \u003d\u003d placement_errors.INVENTORY_INUSE:"},{"line_number":235,"context_line":"        # The error detail includes the resource class and provider."},{"line_number":236,"context_line":"        raise exception.InventoryInUse(err[\u0027detail\u0027])"}],"source_content_type":"text/x-python","patch_set":1,"id":"5f7c97a3_ab47f976","line":233,"in_reply_to":"5f7c97a3_ab5db979","updated":"2018-05-15 17:43:43.000000000","message":"I was mostly being curious. From the standpoint of being robust we should be prepared to deal with random acts of weirdness from the server.\n\nIf the catchall handling is sufficient, then cool. Such handling being so \"far\" away always makes me antsy.","commit_id":"84a11093b6a2ad5a8f68c176869e48ecdac9fc12"}]}