)]}'
{"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"90c53d602f1952eb1975125605961430bcbe2292","unresolved":false,"context_lines":[{"line_number":8616,"context_line":"                                             self.EXPECT_SUCCESS)"},{"line_number":8617,"context_line":""},{"line_number":8618,"context_line":"    def test_live_migration_monitor_migrapi_true(self):"},{"line_number":8619,"context_line":"        # A normal sequence which finish by an undecided behavior,"},{"line_number":8620,"context_line":"        # migrateToURI* return True."},{"line_number":8621,"context_line":"        domain_info_records \u003d ["},{"line_number":8622,"context_line":"            libvirt_guest.JobInfo("},{"line_number":8623,"context_line":"                type\u003dfakelibvirt.VIR_DOMAIN_JOB_NONE),"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f044301_b4d6264f","line":8620,"range":{"start_line":8619,"start_character":0,"end_line":8620,"end_character":36},"updated":"2017-05-04 09:23:15.000000000","message":"nit: Seeing as you\u0027re moving these about, these lines would be good as docstrings. That would mean if they ever failed you get a helpful description in your terminal","commit_id":"07b80a078b67c31bb2a1855c79fcbbbfdf75761c"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"429d59ac2cb29b4000bbd8cbd6de81f74f74bfd7","unresolved":false,"context_lines":[{"line_number":6320,"context_line":"                        info.type \u003d libvirt.VIR_DOMAIN_JOB_FAILED"},{"line_number":6321,"context_line":"                        LOG.debug(\"Migrate API returned a fail\","},{"line_number":6322,"context_line":"                                  instance\u003dinstance)"},{"line_number":6323,"context_line":"                    else:"},{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_f6d95458","line":6327,"range":{"start_line":6323,"start_character":0,"end_line":6327,"end_character":63},"updated":"2017-01-05 15:06:31.000000000","message":"Do we still need this call (and the actual function) now that we\u0027re querying libvirt directly? Seems like libvirt can be treated as the sole source of truth","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"ad297e9333e53b06aa0d454c265714b70e8c62b1","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_a453ef15","line":6327,"in_reply_to":"ba5201f7_0eb4dc7d","updated":"2017-01-06 14:19:01.000000000","message":"So basically what you want to do is to set the status as failed, so we start a rollback ?\n\n...hum yes I think it makes sense in such situation","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ffc9bdd4f5b0320248315164a4d17068912c7d69","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_0eb4dc7d","line":6327,"in_reply_to":"ba5201f7_153eab27","updated":"2017-01-06 13:49:43.000000000","message":"Good point. How about we raise an exception here? If libvirt truly tells us the migration status then there\u0027s no reason to keep this around as it would be dead code. If it doesn\u0027t then that\u0027s a bug and we should highlight it as soon as possible, IMO","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"5870378cf34e6d241d2fa9dfe43b51023871f507","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_153eab27","line":6327,"in_reply_to":"ba5201f7_3c881ac5","updated":"2017-01-06 12:12:25.000000000","message":"I imagine your are right we should never be in such situation, but i feel scary to let the \u0027else\u0027 condition not handled.\n\nThat method is doing extra verifications in such case I guess we can\u0027t do better but I\u0027m open for suggestion.","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"e524d44945ea71ff335fe896dfc44f8cf1884e3a","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_df332761","line":6327,"in_reply_to":"ba5201f7_64010706","updated":"2017-01-09 11:30:23.000000000","message":"This might be a guard when libvirt migration API will not raise an exception but gracefully end after failed migration.\n\nHowever, I cannot provide a real case there. I\u0027d say that if it fails it will be a corner case which is very unlikely. If this piece of code stays here, I\u0027m okay, but I\u0027m also okay with removing it.","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"1dd7d8dca84b4b45e7ee68bb44c3ab93b5d5f483","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_64010706","line":6327,"in_reply_to":"ba5201f7_a453ef15","updated":"2017-01-06 14:49:13.000000000","message":"Yes, exactly. Something has clearly gone wrong and we should alert the user rather than trying to guess what they want","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"d10079b472048d6dfe1e2c3aedcb7030d4999960","unresolved":false,"context_lines":[{"line_number":6324,"context_line":"                        info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6325,"context_line":"                            guest, instance)"},{"line_number":6326,"context_line":"                        LOG.debug(\"Fixed incorrect job type to be %d\","},{"line_number":6327,"context_line":"                                  info.type, instance\u003dinstance)"},{"line_number":6328,"context_line":""},{"line_number":6329,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6330,"context_line":"                # Migration is not yet started"}],"source_content_type":"text/x-python","patch_set":5,"id":"ba5201f7_3c881ac5","line":6327,"in_reply_to":"ba5201f7_f6d95458","updated":"2017-01-05 16:52:30.000000000","message":"It\u0027s good point I need to check that","commit_id":"e86aa2c9143d46bd219cc2b3b82800eb35ad33b5"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"bfef93b2ff91e098ed7efb701d5442a9dd6f8c80","unresolved":false,"context_lines":[{"line_number":6322,"context_line":"                if not finish_event.ready():"},{"line_number":6323,"context_line":"                    LOG.debug(\"Operation thread is still running\","},{"line_number":6324,"context_line":"                              instance\u003dinstance)"},{"line_number":6325,"context_line":"                else:"},{"line_number":6326,"context_line":"                    if migrapi_result is True:"},{"line_number":6327,"context_line":"                        info.type \u003d libvirt.VIR_DOMAIN_JOB_COMPLETED"},{"line_number":6328,"context_line":"                        LOG.debug(\"Migrate API returned a success\","}],"source_content_type":"text/x-python","patch_set":7,"id":"ba5201f7_8ae15f9d","line":6325,"updated":"2017-01-09 11:58:35.000000000","message":"One question - can all this block be moved to find_job_type? This method is long enough and Daniel worked a lot on shortening it, so we should try not to extend it if it is not required.\n\nIf you don\u0027t want to move it find_job_type then you should remove find_job_type entirely as it was the only place where this method was used. But i\u0027d prefer to move it.","commit_id":"d9cb52ecced0b6dedf31c0c5e6ccb88543ed24a8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"320194c7fbd58bf6a17179f2fcbe8c8496656137","unresolved":false,"context_lines":[{"line_number":6322,"context_line":"                if not finish_event.ready():"},{"line_number":6323,"context_line":"                    LOG.debug(\"Operation thread is still running\","},{"line_number":6324,"context_line":"                              instance\u003dinstance)"},{"line_number":6325,"context_line":"                else:"},{"line_number":6326,"context_line":"                    if migrapi_result is True:"},{"line_number":6327,"context_line":"                        info.type \u003d libvirt.VIR_DOMAIN_JOB_COMPLETED"},{"line_number":6328,"context_line":"                        LOG.debug(\"Migrate API returned a success\","}],"source_content_type":"text/x-python","patch_set":7,"id":"ba5201f7_ba7d111e","line":6325,"in_reply_to":"ba5201f7_8ae15f9d","updated":"2017-01-11 16:05:54.000000000","message":"+1 - we should rework find_job_type to reflect this new functionality, replacing what\u0027s there at the moment","commit_id":"d9cb52ecced0b6dedf31c0c5e6ccb88543ed24a8"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"90c53d602f1952eb1975125605961430bcbe2292","unresolved":false,"context_lines":[{"line_number":6288,"context_line":"                    info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6289,"context_line":"                        instance, self.active_migrations)"},{"line_number":6290,"context_line":""},{"line_number":6291,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6292,"context_line":"                # Migration is not yet started"},{"line_number":6293,"context_line":"                LOG.debug(\"Migration not running yet\","},{"line_number":6294,"context_line":"                          instance\u003dinstance)"},{"line_number":6295,"context_line":"            elif info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_UNBOUNDED:"},{"line_number":6296,"context_line":"                # Migration is still running"},{"line_number":6297,"context_line":"                #"}],"source_content_type":"text/x-python","patch_set":14,"id":"3f044301_b40986de","line":6294,"range":{"start_line":6291,"start_character":0,"end_line":6294,"end_character":44},"updated":"2017-05-04 09:23:15.000000000","message":"Doesn\u0027t this say something different to line 6284-6286 for the exact same case? Not related to this but a potential cleanup item?","commit_id":"07b80a078b67c31bb2a1855c79fcbbbfdf75761c"},{"author":{"_account_id":17922,"name":"Huan Xie","email":"huan.xie@citrix.com","username":"huan"},"change_message_id":"c8cb90e2f02813a961b102a7c648cead8a1376b6","unresolved":false,"context_lines":[{"line_number":6288,"context_line":"                    info.type \u003d libvirt_migrate.find_job_type("},{"line_number":6289,"context_line":"                        instance, self.active_migrations)"},{"line_number":6290,"context_line":""},{"line_number":6291,"context_line":"            if info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_NONE:"},{"line_number":6292,"context_line":"                # Migration is not yet started"},{"line_number":6293,"context_line":"                LOG.debug(\"Migration not running yet\","},{"line_number":6294,"context_line":"                          instance\u003dinstance)"},{"line_number":6295,"context_line":"            elif info.type \u003d\u003d libvirt.VIR_DOMAIN_JOB_UNBOUNDED:"},{"line_number":6296,"context_line":"                # Migration is still running"},{"line_number":6297,"context_line":"                #"}],"source_content_type":"text/x-python","patch_set":14,"id":"ff0f0b1f_7be26903","line":6294,"range":{"start_line":6291,"start_character":0,"end_line":6294,"end_character":44},"in_reply_to":"3f044301_b40986de","updated":"2017-05-22 01:20:29.000000000","message":"+1","commit_id":"07b80a078b67c31bb2a1855c79fcbbbfdf75761c"}],"nova/virt/libvirt/migration.py":[{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"37c3907ec5cc6331f78551d443135e3273b94556","unresolved":false,"context_lines":[{"line_number":207,"context_line":"    return xml_doc"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"def find_job_type(guest, instance, active_migrations):"},{"line_number":211,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"    :param guest: a nova.virt.libvirt.guest.Guest"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a3c09a3_5987460f","line":210,"range":{"start_line":210,"start_character":35,"end_line":210,"end_character":52},"updated":"2017-01-13 11:13:51.000000000","message":"Can this be just a single active migration? Not sure that his method needs all active migrations","commit_id":"77262d48943be74492f4150cc3aa803eedfcab8e"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"37c3907ec5cc6331f78551d443135e3273b94556","unresolved":false,"context_lines":[{"line_number":207,"context_line":"    return xml_doc"},{"line_number":208,"context_line":""},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"def find_job_type(guest, instance, active_migrations):"},{"line_number":211,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"    :param guest: a nova.virt.libvirt.guest.Guest"}],"source_content_type":"text/x-python","patch_set":8,"id":"7a3c09a3_16202e9b","line":210,"range":{"start_line":210,"start_character":18,"end_line":210,"end_character":23},"updated":"2017-01-13 11:13:51.000000000","message":"This argument is not used anymore so it can be removed","commit_id":"77262d48943be74492f4150cc3aa803eedfcab8e"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"37c3907ec5cc6331f78551d443135e3273b94556","unresolved":false,"context_lines":[{"line_number":210,"context_line":"def find_job_type(guest, instance, active_migrations):"},{"line_number":211,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"    :param guest: a nova.virt.libvirt.guest.Guest"},{"line_number":214,"context_line":"    :param instance: a nova.objects.Instance"},{"line_number":215,"context_line":"    :param active_migrations: dict of active migrations"},{"line_number":216,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"7a3c09a3_36144af4","line":213,"range":{"start_line":213,"start_character":4,"end_line":213,"end_character":49},"updated":"2017-01-13 11:13:51.000000000","message":"And this one also","commit_id":"77262d48943be74492f4150cc3aa803eedfcab8e"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"37c3907ec5cc6331f78551d443135e3273b94556","unresolved":false,"context_lines":[{"line_number":222,"context_line":"    :returns: a libvirt job type constant"},{"line_number":223,"context_line":""},{"line_number":224,"context_line":"    \"\"\""},{"line_number":225,"context_line":"    for _ in range(1):"},{"line_number":226,"context_line":"        state \u003d active_migrations[instance.uuid].state"},{"line_number":227,"context_line":"        if state is True:"},{"line_number":228,"context_line":"            LOG.debug(\"Migrate API returned a success\","}],"source_content_type":"text/x-python","patch_set":8,"id":"7a3c09a3_79e7a212","line":225,"updated":"2017-01-13 11:13:51.000000000","message":"This will iterate exactly once, it should be range(2). \n\nAnyway I\u0027d rather do that on while loop and subtract from a \u0027retry_count\u0027 variable. Also this would make it possible to run time.sleep conditionally.","commit_id":"77262d48943be74492f4150cc3aa803eedfcab8e"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"bbe008deb8576e4e5a48aef8d5b38b2a2e1f3f5e","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_e1929e9e","line":237,"range":{"start_line":237,"start_character":8,"end_line":237,"end_character":23},"updated":"2017-01-16 15:27:16.000000000","message":"I was thinking about it and not sure this is the right thing to do. If somehow nova will run into this sleep and in the next iteration the state will be True, it will cause additional networking downtime which might break some connections.\n\nHere we must be confident that the greenthread finished and we know the result of the API, i.e., nova will never run into this else block https://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6323 if live_migration_operation thread is still running.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"867d4c8b02cab69885fcd27c99b6ac3302bb1574","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_02813ec3","line":237,"in_reply_to":"7a3c09a3_02077e18","updated":"2017-01-17 11:33:27.000000000","message":"Okay, so let me explain this case. Live migration monitor works in a simple routine (this is *current* state):\n\n1. Check job type whether it is None.\n* If yes - try to fix job type and set correct job type\n* If no - just go to step 2\n2. Check once again job type, take correct action basing on job type:\n* If None - migration not yet started\n* If Unbounded - migration still running\n* If completed - trigger post live migration steps, stop monitoring\n** In case of DVR VM *does not* have connectivity until this step\n* If failed - trigger rollback, stop monitoring\n* If cancelled - trigger rollback, stop monitoring\n3. Sleep 0.5\n4. Go to step 1\n\nWhat you do here is that you add additional sleep to first step, so that in total with step number 3 it will be even more time until post live migration steps get executed. Consider case when LM ends up right at the beginning of step #3 and nova will run into this sleep as well. Assuming that execution time of other commands is negligible, this will add 0.5 seconds more to VM connectivity downtime (because post live migration is postponed, step that is configuring networking).\n\nThis might fix issue with live migration monitor (but honestly, is there any? what this fix is really doing? can you provide a case so that I can reproduce it?), but adds additional downtime to VM connectivity.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"be094f9c270b66c3147e494cb694761da5036b73","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_cb759198","line":237,"in_reply_to":"7a3c09a3_02813ec3","updated":"2017-01-17 12:15:27.000000000","message":"So it\u0027s not exactly how it will work... in the case where we hit the time.sleep(0.5) in find_by_job it means that the thread running migrateToURI did not updated active_migrations, meaning that the migration was still in progress when time.sleep(0.5) of the monitor was running. So we actually do not add any additional downtime since that extra time was necessary.\n\nWhich is why I added this sleep.\n\nThat said if you need more information about why we should refer to libvirt migration API, feel free to ask libvirt maintainers, then how to reproduce it, it\u0027s related to an issue in QEMU.\n\n  https://bugzilla.redhat.com/show_bug.cgi?id\u003d1388465\n  https://bugzilla.redhat.com/show_bug.cgi?id\u003d1401173\n\nSo except your doubts on why we should have this fix I think we are OK we the actual implementation.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4db3891813ba0c3328c7569e483cec0b753170b5","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_b1e0b697","line":237,"in_reply_to":"7a3c09a3_2b01b56a","updated":"2017-01-17 13:03:05.000000000","message":"Yes that is the point we should have reach since the beginning, the treads are actually synchronized we can\u0027t be in a situation of calling find_by_job when migrateToURI is not finished.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"04eec73d389efc4499471caaa87c27179a29794e","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_cc077216","line":237,"in_reply_to":"7a3c09a3_38012906","updated":"2017-01-17 09:31:24.000000000","message":"Pawel not sure why your are talking about downtime... You are mixing several processes. The migration job can return None in case of the migration did not have started yet or have finished or a bug (which is the case we try to avoid)\n\nWe already handle the case where migration did not have started so we do not call libvirt_migrate.find_job_type()\n\nNow we try to handle the race condition when the migration finish in the time of [1] so active_migrations did not have been updated so it\u0027s still None, so the point is to do an other attempt. it could be even 10seconds that is not going to change anything.\n\n[1] https://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6455","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"f5a3d2353330ef15e0bbfba5f07ee31656cbc847","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_38012906","line":237,"in_reply_to":"7a3c09a3_524ee296","updated":"2017-01-16 21:06:27.000000000","message":"So I don\u0027t think this is something we can afford. I think that time.sleep(0.5) in live_migration_monitor is long enough https://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6455\nThis one would make possible downtime even longer (overall it can be \u003e1 sec in the worst case scenario).","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"bf04cbc28035b1e51bb227077f3aa11f48a1dc32","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_02077e18","line":237,"in_reply_to":"7a3c09a3_8c39eaa6","updated":"2017-01-17 11:17:18.000000000","message":"Yes 10 seconds.. but that is not going to break anything.\n\nI\u0027m not going to remove this sleep since it\u0027s clear this one is to let a chance for the other thread to update active_migrations.\n\nInstead of arguing on everything I would like understand why you do not consider there is a potential race and why we should not let a chance for the other thread to be updated.\n\nYour previous comments was - \"we must be confident\" where I\u0027m not since the two threads are not sync and now your are saying - that is going to add 0.5 extra seconds in worst case to the user to retrieve the connection which I consider OK since in the other case we rollback.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"919d20789f3c4e9ffee3199de290a8904201cf3b","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_2b01b56a","line":237,"in_reply_to":"7a3c09a3_cb759198","updated":"2017-01-17 12:42:05.000000000","message":"Can you explain one more thing there - how can nova run into this else block\n\nhttps://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6323\n\nwhen the linked thread is still operating? The threads are linked there\n\nhttps://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6517\n\nand according to Evenlet documentation:\n\nhttp://eventlet.net/doc/modules/greenthread.html#eventlet.greenthread.GreenThread.link\n\nthis finish_event.ready() will not return True\n\nhttps://review.openstack.org/#/c/409261/10/nova/virt/libvirt/driver.py@6320\n\nas long as greenthread is operating.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":12299,"name":"Paweł Koniszewski","email":"pawel.koniszewski@gmail.com","username":"pawel.koniszewski"},"change_message_id":"4812660d7d8db2c7994a07c8f9527f03fdff988f","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_8c39eaa6","line":237,"in_reply_to":"7a3c09a3_cc077216","updated":"2017-01-17 09:35:44.000000000","message":"That\u0027s going to postpone post live migration steps that are setting up networking at destination node. Case that you described would add additional 10 seconds downtime when DVR is used.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"169cc466a96d7b54b36d18585b49b627f82e7b3e","unresolved":false,"context_lines":[{"line_number":234,"context_line":"        # Make a second attempt reading the result"},{"line_number":235,"context_line":"        # of migration API to ensure we handle any"},{"line_number":236,"context_line":"        # race conditions"},{"line_number":237,"context_line":"        time.sleep(0.5)"},{"line_number":238,"context_line":"    # We should never be in such situation: To"},{"line_number":239,"context_line":"    # have the finish_event completed and the"},{"line_number":240,"context_line":"    # libvirt migration API result to None."}],"source_content_type":"text/x-python","patch_set":10,"id":"7a3c09a3_524ee296","line":237,"in_reply_to":"7a3c09a3_e1929e9e","updated":"2017-01-16 16:14:50.000000000","message":"There are no mechanisms to sync them so we can\u0027t be confident.","commit_id":"66083617689e702947c91f76a86ff260890233af"},{"author":{"_account_id":14819,"name":"Timofey Durakov","email":"timofei.nd@gmail.com","username":"tdurakov"},"change_message_id":"82b5c396b3a7bd6d38161866e6b0150a68d51048","unresolved":false,"context_lines":[{"line_number":205,"context_line":"    return xml_doc"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"def find_job_type(instance, active_migrations):"},{"line_number":209,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    :param instance: a nova.objects.Instance"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a3c09a3_016495b6","line":208,"range":{"start_line":208,"start_character":28,"end_line":208,"end_character":45},"updated":"2017-01-18 12:44:53.000000000","message":"do we really need all active_migrations here? why not to pass the state here only, or at least ActiveMigration for one instance.","commit_id":"26ab0d280280c8fd1d6f856b2b96137de515631f"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"c357bdf3c402164c1a11267f7b9b839c42835e94","unresolved":false,"context_lines":[{"line_number":205,"context_line":"    return xml_doc"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"def find_job_type(instance, active_migrations):"},{"line_number":209,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    :param instance: a nova.objects.Instance"}],"source_content_type":"text/x-python","patch_set":11,"id":"7a3c09a3_213a71e7","line":208,"in_reply_to":"7a3c09a3_016495b6","updated":"2017-01-18 12:56:35.000000000","message":"I followed the pattern we already have (see run_tasks), then we only pass a reference here and IMHO that is making the code cleaner by avoiding to create a local variable which will be not used in the method _live_migration_monitor.","commit_id":"26ab0d280280c8fd1d6f856b2b96137de515631f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5e6d11cb8464777af23f96b5d5ae9f8e837f5a8d","unresolved":false,"context_lines":[{"line_number":205,"context_line":"    return xml_doc"},{"line_number":206,"context_line":""},{"line_number":207,"context_line":""},{"line_number":208,"context_line":"def find_job_type(instance, active_migrations):"},{"line_number":209,"context_line":"    \"\"\"Determine the current migration job type"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"    :param instance: a nova.objects.Instance"}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_f446ee4f","line":208,"in_reply_to":"7a3c09a3_213a71e7","updated":"2017-05-04 09:18:41.000000000","message":"Yeah, this is grand as is, IMO","commit_id":"26ab0d280280c8fd1d6f856b2b96137de515631f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5e6d11cb8464777af23f96b5d5ae9f8e837f5a8d","unresolved":false,"context_lines":[{"line_number":221,"context_line":"    \"\"\""},{"line_number":222,"context_line":"    state \u003d active_migrations[instance.uuid].state"},{"line_number":223,"context_line":"    if state is True:"},{"line_number":224,"context_line":"        LOG.debug(\"Migrate API returned a success\","},{"line_number":225,"context_line":"                  instance\u003dinstance)"},{"line_number":226,"context_line":"        return libvirt.VIR_DOMAIN_JOB_COMPLETED"},{"line_number":227,"context_line":"    LOG.debug(\"Migrate API returned a fail\","}],"source_content_type":"text/x-python","patch_set":11,"id":"3f044301_74741e9a","line":224,"range":{"start_line":224,"start_character":31,"end_line":224,"end_character":49},"updated":"2017-05-04 09:18:41.000000000","message":"nit: reported success?","commit_id":"26ab0d280280c8fd1d6f856b2b96137de515631f"}]}