)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"178629b84bac6f5821a4342b2adf78a87af9edc1","unresolved":false,"context_lines":[{"line_number":10,"context_line":"`get_all` method of nova.compute.api.API is called with 2"},{"line_number":11,"context_line":"conflicting search options: {\u0027tenant_id\u0027: XX, \u0027project_id\u0027: YY}."},{"line_number":12,"context_line":"But because, latter on, in that `get_all` method we define a dict"},{"line_number":13,"context_line":"called filter_mapping, on which we iter uppon, which value takes"},{"line_number":14,"context_line":"precedence depends on the order in which the dict is iterated upon."},{"line_number":15,"context_line":""},{"line_number":16,"context_line":"As the order in unpredictable and varies between Py2 and Py3, this"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"3a461143_1b5b60f7","line":13,"updated":"2017-01-30 15:08:58.000000000","message":"nit: upon","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffa1b018511596bd436d56cf84826551203cd734","unresolved":false,"context_lines":[{"line_number":9,"context_line":"When an admin calls /v2.1/servers/detail?tenant_id\u003dXX, then the"},{"line_number":10,"context_line":"`get_all` method of nova.compute.api.API is called with 2"},{"line_number":11,"context_line":"conflicting search options: {\u0027tenant_id\u0027: XX, \u0027project_id\u0027: YY}."},{"line_number":12,"context_line":"But because, latter on, in that `get_all` method we define a dict"},{"line_number":13,"context_line":"called filter_mapping, on which we iter upon, which value takes"},{"line_number":14,"context_line":"precedence depends on the order in which the dict is iterated upon."},{"line_number":15,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"1a430d35_0a8cfcd8","line":12,"range":{"start_line":12,"start_character":13,"end_line":12,"end_character":19},"updated":"2017-02-07 17:31:23.000000000","message":"nit: later","commit_id":"f702762934e0a46449091243a39ad5d57108b23f"}],"nova/api/openstack/compute/servers.py":[{"author":{"_account_id":6849,"name":"Roman Podoliaka","email":"roman.podoliaka@gmail.com","username":"rpodolyaka"},"change_message_id":"36aa3a7f06f3d8fa53deb502203da0b64fcd7ec7","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                context.can(server_policies.SERVERS % \u0027index:get_all_tenants\u0027)"},{"line_number":320,"context_line":"            elevated \u003d context.elevated()"},{"line_number":321,"context_line":"        else:"},{"line_number":322,"context_line":"            # As explained in lp:#1185290, if `all_tenants` is not passed"},{"line_number":323,"context_line":"            # we must ignore the `tenant_id` search option. As explained"},{"line_number":324,"context_line":"            # in a above code comment, any change to this behavior would"},{"line_number":325,"context_line":"            # require a microversion bump."}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_fb6bf4b6","line":322,"updated":"2017-01-30 15:01:34.000000000","message":"FWIW, despite the fact it\u0027s confusing (passing both params), looks like it\u0027s what we agreed on, at least until https://review.openstack.org/#/c/198184/ is approved and implemented.","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"0f2cb459405d44161acfa8a9fea2ded6726d3522","unresolved":false,"context_lines":[{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"},{"line_number":327,"context_line":"            if context.project_id:"},{"line_number":328,"context_line":"                search_opts[\u0027project_id\u0027] \u003d context.project_id"},{"line_number":329,"context_line":"            else:"},{"line_number":330,"context_line":"                search_opts[\u0027user_id\u0027] \u003d context.user_id"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_6c474cbc","line":328,"updated":"2017-01-30 16:16:29.000000000","message":"Won\u0027t this be the project_id of the admin making the request? So if the admin project_id is 123 and the tenant_id filter parameter is 456, then won\u0027t we lose the filter on instances under the 456 tenant project?","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"081298f16bc866bb08c3638346e5a9891e4d743a","unresolved":false,"context_lines":[{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"},{"line_number":327,"context_line":"            if context.project_id:"},{"line_number":328,"context_line":"                search_opts[\u0027project_id\u0027] \u003d context.project_id"},{"line_number":329,"context_line":"            else:"},{"line_number":330,"context_line":"                search_opts[\u0027user_id\u0027] \u003d context.user_id"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_2c60e400","line":328,"in_reply_to":"3a461143_0cd408bb","updated":"2017-01-30 16:23:17.000000000","message":"\u003e The current behavior is to ignore tenant_id if \"all_tenants\" is not passed.\n\nWhat defines that? Because it\u0027s not clear to me from the comment here.","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"16d1e0ad2162204d4206b2bd003fe1e2c9fa04af","unresolved":false,"context_lines":[{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"},{"line_number":327,"context_line":"            if context.project_id:"},{"line_number":328,"context_line":"                search_opts[\u0027project_id\u0027] \u003d context.project_id"},{"line_number":329,"context_line":"            else:"},{"line_number":330,"context_line":"                search_opts[\u0027user_id\u0027] \u003d context.user_id"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_72a2c24c","line":328,"in_reply_to":"3a461143_2c60e400","updated":"2017-01-30 16:46:06.000000000","message":"So get_all iterates over search_opts.items() which is random. If we have tenant_id: 123 and project_id: 456, if we process project_id first, we\u0027ll map that to filters[\u0027project_id\u0027] \u003d 456, and then handle tenant_id and map filters[\u0027project_id\u0027] \u003d 123 (this is what I think we\u0027d want since tenant_id is a filter parameter on the request). The bug is if we randomly handle tenant_id first, map filters[\u0027project_id\u0027] to 123, and then overwrite that to 456 which isn\u0027t what you\u0027re trying to filter on (since 456 in this example is the project id for the admin, not the tenant user).\n\nSo wouldn\u0027t the proper fix just be to sort search_opts.items() in the get_all() method? That way project_id would be handled before tenant_id in get_all()\u0027s filter mapping routine and you\u0027d ultimately end up querying the database for instances with project_id\u003d123.","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"568c66131a7de2d1a603105b9f611f47e4f57b43","unresolved":false,"context_lines":[{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"},{"line_number":327,"context_line":"            if context.project_id:"},{"line_number":328,"context_line":"                search_opts[\u0027project_id\u0027] \u003d context.project_id"},{"line_number":329,"context_line":"            else:"},{"line_number":330,"context_line":"                search_opts[\u0027user_id\u0027] \u003d context.user_id"},{"line_number":331,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a461143_0cd408bb","line":328,"in_reply_to":"3a461143_6c474cbc","updated":"2017-01-30 16:19:49.000000000","message":"Yes, the tenant_id filter parameter 456 will be lost, but that\u0027s the goal. Here we are in the \"else\" clause of the \"is all_tenants flag passed\". The current behavior is to ignore tenant_id if \"all_tenants\" is not passed.","commit_id":"e35a61455250dbfbfc34afa2d6a1b326614c5e4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ce848b69876db8a03c9f6b8afe43d4d5bcf2131c","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                context.can(server_policies.SERVERS % \u0027index:get_all_tenants\u0027)"},{"line_number":320,"context_line":"            elevated \u003d context.elevated()"},{"line_number":321,"context_line":"        else:"},{"line_number":322,"context_line":"            # As explained in lp:#1185290, if `all_tenants` is not passed"},{"line_number":323,"context_line":"            # we must ignore the `tenant_id` search option. As explained"},{"line_number":324,"context_line":"            # in a above code comment, any change to this behavior would"},{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_98566f5a","line":323,"range":{"start_line":322,"start_character":12,"end_line":323,"end_character":59},"updated":"2017-01-30 17:28:50.000000000","message":"FWIW I still don\u0027t really care for this comment, the bug doesn\u0027t really explain things to me, we had to hash this out (no pun intended) for about an hour to come to the conclusion that tenant_id + project_id in this case on py27 didn\u0027t work because python2 defaults to a fixed hash seed, and because of that this never worked properly before (an admin listing instances for a specific tenant_id). Anyway, I don\u0027t really want to hold this up on that, and I don\u0027t have much better wording for this. Maybe another reviewer will.","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ffe0b0ce7ebb12475f26a3c3d64005d053aa22af","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                context.can(server_policies.SERVERS % \u0027index:get_all_tenants\u0027)"},{"line_number":320,"context_line":"            elevated \u003d context.elevated()"},{"line_number":321,"context_line":"        else:"},{"line_number":322,"context_line":"            # As explained in lp:#1185290, if `all_tenants` is not passed"},{"line_number":323,"context_line":"            # we must ignore the `tenant_id` search option. As explained"},{"line_number":324,"context_line":"            # in a above code comment, any change to this behavior would"},{"line_number":325,"context_line":"            # require a microversion bump."},{"line_number":326,"context_line":"            search_opts.pop(\u0027tenant_id\u0027, None)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_9e69e2fb","line":323,"range":{"start_line":322,"start_character":12,"end_line":323,"end_character":59},"in_reply_to":"3a461143_98566f5a","updated":"2017-01-31 09:34:57.000000000","message":"The bug doesn\u0027t explain the weird behavior we are seeing. But  it gives a reference, it justifies why we have to pop tenant_id when all_tenants\u003d0, the reason being \"because it\u0027s how it\u0027s been working for the last 3 years\"","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ffa1b018511596bd436d56cf84826551203cd734","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                context.can(server_policies.SERVERS % \u0027index:get_all_tenants\u0027)"},{"line_number":320,"context_line":"            elevated \u003d context.elevated()"},{"line_number":321,"context_line":"        else:"},{"line_number":322,"context_line":"            # As explained in lp:#1185290, if `all_tenants` is not passed"},{"line_number":323,"context_line":"            # we must ignore the `tenant_id` search option. As explained"},{"line_number":324,"context_line":"            # in a above code comment, any change to this behavior would"},{"line_number":325,"context_line":"            # require a microversion bump."}],"source_content_type":"text/x-python","patch_set":4,"id":"1a430d35_7f329bf3","line":322,"range":{"start_line":322,"start_character":14,"end_line":322,"end_character":41},"updated":"2017-02-07 17:31:23.000000000","message":"I\u0027d really prefer to explain in the comment itself in the code, since in a year from now I\u0027m going to lose all context on this (I\u0027ve already started to after a week), but don\u0027t want to hold this patch up for it.","commit_id":"f702762934e0a46449091243a39ad5d57108b23f"}],"nova/tests/unit/api/openstack/compute/test_serversV21.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"62a6d03569c796f36a6c6d04d27e0417bcc1eff1","unresolved":false,"context_lines":[{"line_number":790,"context_line":"        self.assertEqual(len(servers), 1)"},{"line_number":791,"context_line":""},{"line_number":792,"context_line":"    def test_tenant_id_filter_admin_context(self):"},{"line_number":793,"context_line":"        \"\"\"\"Test that tenant_id search opt is dropped without all_tenants\u003d1\"\"\""},{"line_number":794,"context_line":"        def fake_get_all(context, search_opts\u003dNone, **kwargs):"},{"line_number":795,"context_line":"            self.assertIsNotNone(search_opts)"},{"line_number":796,"context_line":"            self.assertNotIn(\u0027tenant_id\u0027, search_opts)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_d8e397a2","line":793,"range":{"start_line":793,"start_character":74,"end_line":793,"end_character":75},"updated":"2017-01-30 17:31:00.000000000","message":"Don\u0027t you mean 0 here? We want all_tenants to be False to get into that else block.","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ffe0b0ce7ebb12475f26a3c3d64005d053aa22af","unresolved":false,"context_lines":[{"line_number":790,"context_line":"        self.assertEqual(len(servers), 1)"},{"line_number":791,"context_line":""},{"line_number":792,"context_line":"    def test_tenant_id_filter_admin_context(self):"},{"line_number":793,"context_line":"        \"\"\"\"Test that tenant_id search opt is dropped without all_tenants\u003d1\"\"\""},{"line_number":794,"context_line":"        def fake_get_all(context, search_opts\u003dNone, **kwargs):"},{"line_number":795,"context_line":"            self.assertIsNotNone(search_opts)"},{"line_number":796,"context_line":"            self.assertNotIn(\u0027tenant_id\u0027, search_opts)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_9e500234","line":793,"range":{"start_line":793,"start_character":74,"end_line":793,"end_character":75},"in_reply_to":"3a461143_d8e397a2","updated":"2017-01-31 09:34:57.000000000","message":"No, I said \"without all_tenants\u003d1\", so it means \u0027with all_tenants\u003d0\u0027 (more or less). I\u0027ll rephrase anyway.","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"2bc08f3ccbf286b0b7c142b12707ef606dc49752","unresolved":false,"context_lines":[{"line_number":793,"context_line":"        \"\"\"\"Test that tenant_id search opt is dropped without all_tenants\u003d1\"\"\""},{"line_number":794,"context_line":"        def fake_get_all(context, search_opts\u003dNone, **kwargs):"},{"line_number":795,"context_line":"            self.assertIsNotNone(search_opts)"},{"line_number":796,"context_line":"            self.assertNotIn(\u0027tenant_id\u0027, search_opts)"},{"line_number":797,"context_line":"            self.assertEqual(search_opts[\u0027project_id\u0027], \u0027fake\u0027)"},{"line_number":798,"context_line":"            return [fakes.stub_instance_obj(100)]"},{"line_number":799,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_2e4b635a","line":796,"range":{"start_line":796,"start_character":12,"end_line":796,"end_character":54},"updated":"2017-01-31 07:01:09.000000000","message":"along with this, i was thinking to check the final value of project_id in filter at - https://github.com/openstack/nova/blob/master/nova/compute/api.py#L2424\n\nBut you have to do deep level mocking for this. I think this is enough as tempest test making sure we get right tenant servers.","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":7350,"name":"Jordan Pittier","email":"jordan.pittier@gmail.com","username":"JordanP"},"change_message_id":"ffe0b0ce7ebb12475f26a3c3d64005d053aa22af","unresolved":false,"context_lines":[{"line_number":793,"context_line":"        \"\"\"\"Test that tenant_id search opt is dropped without all_tenants\u003d1\"\"\""},{"line_number":794,"context_line":"        def fake_get_all(context, search_opts\u003dNone, **kwargs):"},{"line_number":795,"context_line":"            self.assertIsNotNone(search_opts)"},{"line_number":796,"context_line":"            self.assertNotIn(\u0027tenant_id\u0027, search_opts)"},{"line_number":797,"context_line":"            self.assertEqual(search_opts[\u0027project_id\u0027], \u0027fake\u0027)"},{"line_number":798,"context_line":"            return [fakes.stub_instance_obj(100)]"},{"line_number":799,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_be3a1ee5","line":796,"range":{"start_line":796,"start_character":12,"end_line":796,"end_character":54},"in_reply_to":"3a461143_2e4b635a","updated":"2017-01-31 09:34:57.000000000","message":"It\u0027s already asserted L783.","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"62a6d03569c796f36a6c6d04d27e0417bcc1eff1","unresolved":false,"context_lines":[{"line_number":794,"context_line":"        def fake_get_all(context, search_opts\u003dNone, **kwargs):"},{"line_number":795,"context_line":"            self.assertIsNotNone(search_opts)"},{"line_number":796,"context_line":"            self.assertNotIn(\u0027tenant_id\u0027, search_opts)"},{"line_number":797,"context_line":"            self.assertEqual(search_opts[\u0027project_id\u0027], \u0027fake\u0027)"},{"line_number":798,"context_line":"            return [fakes.stub_instance_obj(100)]"},{"line_number":799,"context_line":""},{"line_number":800,"context_line":"        req \u003d self.req(\u0027/fake/servers?tenant_id\u003dnewfake\u0027,"}],"source_content_type":"text/x-python","patch_set":3,"id":"3a461143_b8016b3a","line":797,"updated":"2017-01-30 17:31:00.000000000","message":"nit: the args here should be reversed (expected, actual)","commit_id":"72a0718a32702f72fbcf8e815aa8596da421aaa3"}]}