)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"996194c3f1f8485ec2953e3a3a4e48a43e7493f7","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Check the instance ID before creating it"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"At least with libvirt 1.x instace goes into shutdown state"},{"line_number":10,"context_line":"before it\u0027s ID is changed. This means that there\u0027s a race"},{"line_number":11,"context_line":"condition during reboot where nova might try to create an"},{"line_number":12,"context_line":"instance that\u0027s already defined. By checking if the ID of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAAPH%2F%2Filk%3D","line":9,"updated":"2013-05-30 22:10:21.000000000","message":"s/instace/instance/","commit_id":"5be9e51904c1e47c826180a1bb896b965731bd77"},{"author":{"_account_id":2583,"name":"Ante Karamatić","email":"ante.karamatic@canonical.com","username":"ivoks"},"change_message_id":"44ee9c83094839877fa403b8a0e6e6bc41723d28","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Check the instance ID before creating it"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"At least with libvirt 1.x instace goes into shutdown state"},{"line_number":10,"context_line":"before it\u0027s ID is changed. This means that there\u0027s a race"},{"line_number":11,"context_line":"condition during reboot where nova might try to create an"},{"line_number":12,"context_line":"instance that\u0027s already defined. By checking if the ID of"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAAPX%2F%2FnU0%3D","line":9,"in_reply_to":"AAAAPH%2F%2Filk%3D","updated":"2013-06-14 10:30:37.000000000","message":"Done","commit_id":"5be9e51904c1e47c826180a1bb896b965731bd77"},{"author":{"_account_id":5652,"name":"David Ripton","email":"dripton@redhat.com","username":"dripton"},"change_message_id":"996194c3f1f8485ec2953e3a3a4e48a43e7493f7","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Check the instance ID before creating it"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"At least with libvirt 1.x instace goes into shutdown state"},{"line_number":10,"context_line":"before it\u0027s ID is changed. This means that there\u0027s a race"},{"line_number":11,"context_line":"condition during reboot where nova might try to create an"},{"line_number":12,"context_line":"instance that\u0027s already defined. By checking if the ID of"},{"line_number":13,"context_line":"the new and old instance is the same, we avoid that situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAAPH%2F%2Filg%3D","line":10,"updated":"2013-05-30 22:10:21.000000000","message":"s/it\u0027s/its/","commit_id":"5be9e51904c1e47c826180a1bb896b965731bd77"},{"author":{"_account_id":2583,"name":"Ante Karamatić","email":"ante.karamatic@canonical.com","username":"ivoks"},"change_message_id":"44ee9c83094839877fa403b8a0e6e6bc41723d28","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Check the instance ID before creating it"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"At least with libvirt 1.x instace goes into shutdown state"},{"line_number":10,"context_line":"before it\u0027s ID is changed. This means that there\u0027s a race"},{"line_number":11,"context_line":"condition during reboot where nova might try to create an"},{"line_number":12,"context_line":"instance that\u0027s already defined. By checking if the ID of"},{"line_number":13,"context_line":"the new and old instance is the same, we avoid that situation"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":5,"id":"AAAAPX%2F%2FnUs%3D","line":10,"in_reply_to":"AAAAPH%2F%2Filg%3D","updated":"2013-06-14 10:30:37.000000000","message":"Done","commit_id":"5be9e51904c1e47c826180a1bb896b965731bd77"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"6596c6e31328462fb5ff5ef840627d564592aa8c","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1332,"context_line":"                         power_state.CRASHED]:"},{"line_number":1333,"context_line":"                if old_domid \u003d\u003d new_domid and state \u003d\u003d power_state.SHUTDOWN:"},{"line_number":1334,"context_line":"                    continue"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAPH%2F%2F63A%3D","line":1334,"updated":"2013-05-20 10:00:47.000000000","message":"This continue will cause the sleep to be skipped and the thread won\u0027t yield, but re-query the instance immediately. I don\u0027t think we should do this.\n\nEither add a sleep here too, or refactor the top-level if-elif to consider this case too. Maybe that would be even better as the code does look a bit convoluted atm.\n\nAlso - it would be good to add a NOTE (similar to what the commit message says) explaining why this is special-cased as it would be very difficult to guess when reading the code.","commit_id":"b88f05a64411a17de7ccedf1ddad46bca00a23d6"},{"author":{"_account_id":2583,"name":"Ante Karamatić","email":"ante.karamatic@canonical.com","username":"ivoks"},"change_message_id":"9f87ba24ea5f6ba6f6c5f136a96cc7148acff447","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1332,"context_line":"                         power_state.CRASHED]:"},{"line_number":1333,"context_line":"                if old_domid \u003d\u003d new_domid and state \u003d\u003d power_state.SHUTDOWN:"},{"line_number":1334,"context_line":"                    continue"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAPH%2F%2F6JE%3D","line":1334,"in_reply_to":"AAAAPH%2F%2F63A%3D","updated":"2013-05-20 15:48:23.000000000","message":"You are correct. I\u0027ve missed that. Fixed in new revision.","commit_id":"b88f05a64411a17de7ccedf1ddad46bca00a23d6"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"d9a264ee52d4d883a3e58efc8cbd9653fc9aa505","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            # NOTE(ivoks): By checking domain IDs, we make sure we are"},{"line_number":1332,"context_line":"            #              not recreating domain that\u0027s already running."},{"line_number":1333,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1334,"context_line":"                         power_state.CRASHED] and old_domid !\u003d new_domid:"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAPH%2F%2F3j4%3D","line":1334,"updated":"2013-05-21 11:14:59.000000000","message":"I hate to be nitpicky about these small details - but it does seem that we could group this under a top level if construct\n\nif old_domid !\u003d new_domid:\n\nThis is true however only if this version is correct and not the previous one from patchset 1 (note that they have a difference in that this one will catch the CRASHED state too).\n\nWith this in mind it would be good to have a test actually (once Jenkins is up again)","commit_id":"3596beb354e0ef581cff4d653a24b116d024d88f"},{"author":{"_account_id":5511,"name":"Nikola Dipanov","email":"ndipanov@redhat.com","username":"ndipanov"},"change_message_id":"501595e971398b51b09e49f9735ffea9de1b55de","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            # NOTE(ivoks): By checking domain IDs, we make sure we are"},{"line_number":1332,"context_line":"            #              not recreating domain that\u0027s already running."},{"line_number":1333,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1334,"context_line":"                         power_state.CRASHED] and old_domid !\u003d new_domid:"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAPH%2F%2Fz44%3D","line":1334,"in_reply_to":"AAAAPH%2F%2F3DA%3D","updated":"2013-05-23 15:23:30.000000000","message":"Sure!\n\nLooking at the tests in nova/tests/virt/libvirt.py it seems that the reboot method is not being unit tested at all. It would be good to add some tests to make sure that the logic inside this method does what it\u0027s expected to do, and does not cause regressions.\n\nI would start by taking a look at tests for other driver methods.","commit_id":"3596beb354e0ef581cff4d653a24b116d024d88f"},{"author":{"_account_id":2583,"name":"Ante Karamatić","email":"ante.karamatic@canonical.com","username":"ivoks"},"change_message_id":"0b07be1fd34c5b4998d0e1059f0dc50c0aaba6d1","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            # NOTE(ivoks): By checking domain IDs, we make sure we are"},{"line_number":1332,"context_line":"            #              not recreating domain that\u0027s already running."},{"line_number":1333,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1334,"context_line":"                         power_state.CRASHED] and old_domid !\u003d new_domid:"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAPH%2F%2F3DA%3D","line":1334,"in_reply_to":"AAAAPH%2F%2F3j4%3D","updated":"2013-05-21 14:45:43.000000000","message":"I\u0027m not sure what is expected to be tested? Could you please elaborate on what kind of the test you\u0027d like to see?\n\nThanks","commit_id":"3596beb354e0ef581cff4d653a24b116d024d88f"},{"author":{"_account_id":2583,"name":"Ante Karamatić","email":"ante.karamatic@canonical.com","username":"ivoks"},"change_message_id":"4995c3df9306973c74539949b692469ae5c33f4e","unresolved":false,"context_lines":[{"line_number":1331,"context_line":"            # NOTE(ivoks): By checking domain IDs, we make sure we are"},{"line_number":1332,"context_line":"            #              not recreating domain that\u0027s already running."},{"line_number":1333,"context_line":"            if state in [power_state.SHUTDOWN,"},{"line_number":1334,"context_line":"                         power_state.CRASHED] and old_domid !\u003d new_domid:"},{"line_number":1335,"context_line":"                LOG.info(_(\"Instance shutdown successfully.\"),"},{"line_number":1336,"context_line":"                         instance\u003dinstance)"},{"line_number":1337,"context_line":"                self._create_domain(domain\u003ddom)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAPX%2F%2FnUo%3D","line":1334,"in_reply_to":"AAAAPH%2F%2Fz44%3D","updated":"2013-06-14 10:30:28.000000000","message":"Done","commit_id":"3596beb354e0ef581cff4d653a24b116d024d88f"},{"author":{"_account_id":1561,"name":"Russell Bryant","email":"rbryant@redhat.com","username":"russellb"},"change_message_id":"b91b7a3cab47dcfffaf0590ed790c9519ac5670c","unresolved":false,"context_lines":[{"line_number":1380,"context_line":""},{"line_number":1381,"context_line":"            # NOTE(ivoks): By checking domain IDs, we make sure we are"},{"line_number":1382,"context_line":"            #              not recreating domain that\u0027s already running."},{"line_number":1383,"context_line":"            if old_domid !\u003d new_domid:"},{"line_number":1384,"context_line":"                if state in [power_state.SHUTDOWN,"},{"line_number":1385,"context_line":"                             power_state.CRASHED]:"},{"line_number":1386,"context_line":"                    LOG.info(_(\"Instance shutdown successfully.\"),"}],"source_content_type":"text/x-python","patch_set":9,"id":"AAAAPn%2F%2FoD0%3D","line":1383,"updated":"2013-06-26 14:07:14.000000000","message":"As a style note, I prefer coding to reduce indentation.  So, here I would do:\n\n    if old_domid \u003d\u003d new_domid:\n        continue\n\n    ... rest of the code ...","commit_id":"2402055b955656c7fffa66018c7b8cfcc1596b1b"}]}