)]}'
{"nova/db/sqlalchemy/api.py":[{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2275,"context_line":"    before we update the database."},{"line_number":2276,"context_line":""},{"line_number":2277,"context_line":"    :param instance_ref: The existing instance from the database"},{"line_number":2278,"context_line":"    :param expected_task_states: Either None, or an iterable of task states"},{"line_number":2279,"context_line":"                                 that the existing database instance may be"},{"line_number":2280,"context_line":"                                 in to pass the check."},{"line_number":2281,"context_line":"    :param expected_vm_states: Either None, or an iterable of VM states that"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_0232595d","line":2278,"updated":"2014-07-28 11:23:32.000000000","message":"nit: Can\u0027t be any iterable, because it\u0027s used twice (i.e. can\u0027t be a generator). Not massively important for a private function, though.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2293,"context_line":"    if expected_task_states is not None:"},{"line_number":2294,"context_line":"        if actual_state not in expected_task_states:"},{"line_number":2295,"context_line":"            if actual_state \u003d\u003d task_states.DELETING:"},{"line_number":2296,"context_line":"                raise exception.UnexpectedDeletingTaskStateError("},{"line_number":2297,"context_line":"                        actual\u003dactual_state, expected\u003dexpected_task_states)"},{"line_number":2298,"context_line":"            else:"},{"line_number":2299,"context_line":"                raise exception.UnexpectedTaskStateError("}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_420c5142","line":2296,"updated":"2014-07-28 11:23:32.000000000","message":"This code matches the previous behaviour, so it\u0027s not an issue for this change.\n\nHowever, I looking at how it\u0027s used I wonder if we could be even less fussy about when we raise UnexpectedDeletingTaskStateError. I think we should raise this error when actual_state is DELETING, except when expected_task_states explicitly includes DELETING. i.e. we should also raise it when expected_task_states is None.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"3f4a5e47e956e303788203d6fce5f5ec4147ac03","unresolved":false,"context_lines":[{"line_number":2310,"context_line":"        if new_hostname.lower() !\u003d instance_hostname.lower():"},{"line_number":2311,"context_line":"            _validate_unique_server_name(context,"},{"line_number":2312,"context_line":"                                         session,"},{"line_number":2313,"context_line":"                                         new_hostname)"},{"line_number":2314,"context_line":""},{"line_number":2315,"context_line":""},{"line_number":2316,"context_line":"def _fetch_and_check_instance(context, session, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_b798101c","line":2313,"updated":"2014-07-28 07:16:42.000000000","message":"there is also a race for unique server. can we check this with update?","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2310,"context_line":"        if new_hostname.lower() !\u003d instance_hostname.lower():"},{"line_number":2311,"context_line":"            _validate_unique_server_name(context,"},{"line_number":2312,"context_line":"                                         session,"},{"line_number":2313,"context_line":"                                         new_hostname)"},{"line_number":2314,"context_line":""},{"line_number":2315,"context_line":""},{"line_number":2316,"context_line":"def _fetch_and_check_instance(context, session, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_843d8090","line":2313,"in_reply_to":"baada198_b798101c","updated":"2014-07-28 19:14:32.000000000","message":"We pass new_hostname to _instance_compare_and_update() in our retry loop below to specifically guard against this race.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2310,"context_line":"        if new_hostname.lower() !\u003d instance_hostname.lower():"},{"line_number":2311,"context_line":"            _validate_unique_server_name(context,"},{"line_number":2312,"context_line":"                                         session,"},{"line_number":2313,"context_line":"                                         new_hostname)"},{"line_number":2314,"context_line":""},{"line_number":2315,"context_line":""},{"line_number":2316,"context_line":"def _fetch_and_check_instance(context, session, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_dd6122a9","line":2313,"in_reply_to":"baada198_b798101c","updated":"2014-07-28 11:23:32.000000000","message":"You could handle this in 1 of 2 ways that I can think of:\n\n1. Catch the unique constraint failure and retry.\n\n2. Add an \u0027and not exists()\u0027 clause to the update.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2310,"context_line":"        if new_hostname.lower() !\u003d instance_hostname.lower():"},{"line_number":2311,"context_line":"            _validate_unique_server_name(context,"},{"line_number":2312,"context_line":"                                         session,"},{"line_number":2313,"context_line":"                                         new_hostname)"},{"line_number":2314,"context_line":""},{"line_number":2315,"context_line":""},{"line_number":2316,"context_line":"def _fetch_and_check_instance(context, session, instance_uuid,"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_a457e4c7","line":2313,"in_reply_to":"baada198_dd6122a9","updated":"2014-07-28 19:14:32.000000000","message":"See below... we pass the new_hostname in the expected_values in instance_compare_and_update() below specifically to guard against this issue.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2317,"context_line":"                              columns_to_join,"},{"line_number":2318,"context_line":"                              expected_task_states\u003dNone,"},{"line_number":2319,"context_line":"                              expected_vm_states\u003dNone,"},{"line_number":2320,"context_line":"                              new_hostname\u003dNone):"},{"line_number":2321,"context_line":"    instance_ref \u003d _instance_get_by_uuid(context, instance_uuid,"},{"line_number":2322,"context_line":"                                         session\u003dsession,"},{"line_number":2323,"context_line":"                                         columns_to_join\u003dcolumns_to_join)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_82ed89f2","line":2320,"updated":"2014-07-28 11:23:32.000000000","message":"nit: you don\u0027t really need to default any arguments to this function, as it\u0027s internal and they\u0027re always provided.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2317,"context_line":"                              columns_to_join,"},{"line_number":2318,"context_line":"                              expected_task_states\u003dNone,"},{"line_number":2319,"context_line":"                              expected_vm_states\u003dNone,"},{"line_number":2320,"context_line":"                              new_hostname\u003dNone):"},{"line_number":2321,"context_line":"    instance_ref \u003d _instance_get_by_uuid(context, instance_uuid,"},{"line_number":2322,"context_line":"                                         session\u003dsession,"},{"line_number":2323,"context_line":"                                         columns_to_join\u003dcolumns_to_join)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_a40e04e2","line":2320,"in_reply_to":"baada198_82ed89f2","updated":"2014-07-28 19:14:32.000000000","message":"It\u0027s not internal... it\u0027s on the module level.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":100,"name":"Johannes Erdfelt","email":"johannes@erdfelt.com","username":"johannes.erdfelt"},"change_message_id":"024fbe177aa3cbddf2bb0d97375cb2127f46483c","unresolved":false,"context_lines":[{"line_number":2317,"context_line":"                              columns_to_join,"},{"line_number":2318,"context_line":"                              expected_task_states\u003dNone,"},{"line_number":2319,"context_line":"                              expected_vm_states\u003dNone,"},{"line_number":2320,"context_line":"                              new_hostname\u003dNone):"},{"line_number":2321,"context_line":"    instance_ref \u003d _instance_get_by_uuid(context, instance_uuid,"},{"line_number":2322,"context_line":"                                         session\u003dsession,"},{"line_number":2323,"context_line":"                                         columns_to_join\u003dcolumns_to_join)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_b5809ac0","line":2320,"in_reply_to":"baada198_a40e04e2","updated":"2014-07-30 19:42:02.000000000","message":"Sure it is. It\u0027s prefixed with an _ and it\u0027s not called from outside of this module. It\u0027s a private/internal function.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a88d150da3429c3a65eaf3e99ad7ced274c13ef0","unresolved":false,"context_lines":[{"line_number":2317,"context_line":"                              columns_to_join,"},{"line_number":2318,"context_line":"                              expected_task_states\u003dNone,"},{"line_number":2319,"context_line":"                              expected_vm_states\u003dNone,"},{"line_number":2320,"context_line":"                              new_hostname\u003dNone):"},{"line_number":2321,"context_line":"    instance_ref \u003d _instance_get_by_uuid(context, instance_uuid,"},{"line_number":2322,"context_line":"                                         session\u003dsession,"},{"line_number":2323,"context_line":"                                         columns_to_join\u003dcolumns_to_join)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_e17deab3","line":2320,"in_reply_to":"baada198_b5809ac0","updated":"2014-07-30 20:57:43.000000000","message":"Gah, sorry guys... I thought what Matthew was saying is that this function was scoped inside the _instance_update() function and therefore I didn\u0027t need to pass the various arguments (since the variables would exist from the scope of the _instance_update() function!\n\nI get you now :) Sorry, will fix up!","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2330,"context_line":"    expected_values \u003d {"},{"line_number":2331,"context_line":"        \u0027task_state\u0027: instance_ref[\u0027task_state\u0027],"},{"line_number":2332,"context_line":"        \u0027vm_state\u0027: instance_ref[\u0027vm_state\u0027],"},{"line_number":2333,"context_line":"        \u0027hostname\u0027: instance_ref[\u0027hostname\u0027]"},{"line_number":2334,"context_line":"    }"},{"line_number":2335,"context_line":"    return instance_ref, expected_values"},{"line_number":2336,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_7d9bb672","line":2333,"updated":"2014-07-28 11:23:32.000000000","message":"I don\u0027t think hostname needs to be here. The hostname validation is a uniqueness constraint, so it\u0027s dependent on all rows, not just this one. It doesn\u0027t really hurt, though.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"04c6c6817ff8cf3d0480345b4b1427f04d905668","unresolved":false,"context_lines":[{"line_number":2330,"context_line":"    expected_values \u003d {"},{"line_number":2331,"context_line":"        \u0027task_state\u0027: instance_ref[\u0027task_state\u0027],"},{"line_number":2332,"context_line":"        \u0027vm_state\u0027: instance_ref[\u0027vm_state\u0027],"},{"line_number":2333,"context_line":"        \u0027hostname\u0027: instance_ref[\u0027hostname\u0027]"},{"line_number":2334,"context_line":"    }"},{"line_number":2335,"context_line":"    return instance_ref, expected_values"},{"line_number":2336,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_ff689ef5","line":2333,"in_reply_to":"baada198_442c484a","updated":"2014-08-04 12:55:29.000000000","message":"This isn\u0027t enough here, because we\u0027re checking a unique constraint. i.e. We\u0027re not just interested in whether this instance has been updated behind our back, but also if *any other* instance has been updated too. I think this can be handled by adding:\n\nand not exists(select * from instances where hostname \u003d \u003cfoo\u003e)\n\nAs I said, though, I\u0027d personally be happy to accept this change despite it not quite covering all bases, because it\u0027s architecturally sound and a huge improvement. The rest can be covered later without substantially altering the design.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2330,"context_line":"    expected_values \u003d {"},{"line_number":2331,"context_line":"        \u0027task_state\u0027: instance_ref[\u0027task_state\u0027],"},{"line_number":2332,"context_line":"        \u0027vm_state\u0027: instance_ref[\u0027vm_state\u0027],"},{"line_number":2333,"context_line":"        \u0027hostname\u0027: instance_ref[\u0027hostname\u0027]"},{"line_number":2334,"context_line":"    }"},{"line_number":2335,"context_line":"    return instance_ref, expected_values"},{"line_number":2336,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_442c484a","line":2333,"in_reply_to":"baada198_7d9bb672","updated":"2014-07-28 19:14:32.000000000","message":"It needs to be here in order to guarantee that the database record has not changed in between the time we checked the hostname, and the time we go to update the database (and if the record in the database does NOT contain the value we expect, then we refresh the instance_ref and re-check the new hostname.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2415,"context_line":"                expected_vm_states\u003dexpected_vm_states,"},{"line_number":2416,"context_line":"                new_hostname\u003dnew_hostname)"},{"line_number":2417,"context_line":"    updated \u003d _instance_compare_and_update(session, instance_uuid,"},{"line_number":2418,"context_line":"                                           values, expected_values)"},{"line_number":2419,"context_line":"    attempts \u003d 1"},{"line_number":2420,"context_line":"    max_time \u003d time.time() + 10"},{"line_number":2421,"context_line":"    base_interval \u003d 0.05"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_9817b89c","line":2418,"updated":"2014-07-28 11:23:32.000000000","message":"nit: you could start the loop below with multiplier \u003d 0 and do this check first, then you wouldn\u0027t have to duplicate it.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2415,"context_line":"                expected_vm_states\u003dexpected_vm_states,"},{"line_number":2416,"context_line":"                new_hostname\u003dnew_hostname)"},{"line_number":2417,"context_line":"    updated \u003d _instance_compare_and_update(session, instance_uuid,"},{"line_number":2418,"context_line":"                                           values, expected_values)"},{"line_number":2419,"context_line":"    attempts \u003d 1"},{"line_number":2420,"context_line":"    max_time \u003d time.time() + 10"},{"line_number":2421,"context_line":"    base_interval \u003d 0.05"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_84d90058","line":2418,"in_reply_to":"baada198_9817b89c","updated":"2014-07-28 19:14:32.000000000","message":"But this is an optimistic algorithm, and trying the update above is all about being optimistic :) Only in the extreme scenarios of contention where multiple threads are updating the exact same instance at the same time would the retry loop below ever be executed.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"bbd7d0a0172b5ff40a516d0850427738834dde17","unresolved":false,"context_lines":[{"line_number":2457,"context_line":"               \"fetched in this thread and the time we attempted to update \""},{"line_number":2458,"context_line":"               \"the instance. This is attempt %(attempts)d at a retry of our \""},{"line_number":2459,"context_line":"               \"instance update.\")"},{"line_number":2460,"context_line":"        LOG.debug(msg, dict(uuid\u003dinstance_uuid, attempts\u003dattempts))"},{"line_number":2461,"context_line":"        attempts +\u003d 1"},{"line_number":2462,"context_line":"        updated \u003d _instance_compare_and_update(session, instance_uuid,"},{"line_number":2463,"context_line":"                                               values, expected_values)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_18b64889","line":2460,"updated":"2014-07-28 11:23:32.000000000","message":"nit: why is this log message in between _fetch_and_check() and _compare_and_update()? It looks a bit weird.","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3428071e4240ce520085ef88182b62b797f4ab7","unresolved":false,"context_lines":[{"line_number":2457,"context_line":"               \"fetched in this thread and the time we attempted to update \""},{"line_number":2458,"context_line":"               \"the instance. This is attempt %(attempts)d at a retry of our \""},{"line_number":2459,"context_line":"               \"instance update.\")"},{"line_number":2460,"context_line":"        LOG.debug(msg, dict(uuid\u003dinstance_uuid, attempts\u003dattempts))"},{"line_number":2461,"context_line":"        attempts +\u003d 1"},{"line_number":2462,"context_line":"        updated \u003d _instance_compare_and_update(session, instance_uuid,"},{"line_number":2463,"context_line":"                                               values, expected_values)"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_c4cdb887","line":2460,"in_reply_to":"baada198_18b64889","updated":"2014-07-28 19:14:32.000000000","message":"Because fetch_and_check_instance() will raise an exception if the check fails, and thus there would be no need to log this message (since the exception raising  is a normal code path...)","commit_id":"fbfb5dd273609aa0a3f387b5a03861d3400bfeae"}]}