)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6608,"name":"Paul Carlton","email":"paul.carlton2@hpe.com","username":"paul-carlton2"},"change_message_id":"9979a2e81ecfdbdf4e239ae2a2e4b3342e0763d9","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"WIP libvirt: Reconnect volumes and encryptors during a hard reboot"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Closes-bug#1597234"},{"line_number":10,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"7a77a97e_dec69d15","line":9,"updated":"2016-11-22 12:48:39.000000000","message":"missing \": \"","commit_id":"80905d159bc398342dec42b3adca216fd02cb8f0"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d8433be838f2cf9323e178f125945b62d1d8805d","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave"},{"line_number":11,"context_line":"vifs, volumes and any associated encryptors present on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"When using encrypted volumes this can lead to failures when the instance"},{"line_number":14,"context_line":"is then recreated, calling down into _get_guest_xml and"},{"line_number":15,"context_line":"_get_guest_storage_config and eventually connect_volume. With the"},{"line_number":16,"context_line":"encryptor attached this call will fail for most backends as the original"},{"line_number":17,"context_line":"path to the volume is now used to present the unencrypted device to the"},{"line_number":18,"context_line":"instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_9d6a7b17","line":15,"range":{"start_line":13,"start_character":0,"end_line":15,"end_character":56},"updated":"2017-10-16 23:04:39.000000000","message":"So, if the existing behavior of calling _destroy and _undefine_domain leaves volumes connected to the host, why is connect_volume being called as part of the reboot flow? It would seem like if disconnect_volume wasn\u0027t called, then we shouldn\u0027t be calling connect_volume either. Wouldn\u0027t that be the bug?\n\nThat is, shouldn\u0027t we avoid \"connecting volumes\" if we\u0027re doing a reboot and they\u0027re already connected?","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe314b9fb263885e8e3db4d1461f820322669636","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave"},{"line_number":11,"context_line":"vifs, volumes and any associated encryptors present on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"When using encrypted volumes this can lead to failures when the instance"},{"line_number":14,"context_line":"is then recreated, calling down into _get_guest_xml and"},{"line_number":15,"context_line":"_get_guest_storage_config and eventually connect_volume. With the"},{"line_number":16,"context_line":"encryptor attached this call will fail for most backends as the original"},{"line_number":17,"context_line":"path to the volume is now used to present the unencrypted device to the"},{"line_number":18,"context_line":"instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_25ed98b6","line":15,"range":{"start_line":13,"start_character":0,"end_line":15,"end_character":56},"in_reply_to":"5f4e5783_3a45c070","updated":"2017-10-17 15:57:35.000000000","message":"Okay, I was thinking something [ugly] like add reboot\u003dFalse to the get_* functions and have _hard_reboot pass reboot\u003dTrue to avoid connect_volume since they haven\u0027t been disconnected in the first place.\n\nBut I understand now that there have been past conversations at summits/PTGs where it was agreed that we should unplug and replug things during hard reboot to give operators a way to get a clean slate if they need to get unstuck from other issues with an instance.\n\nSo given that was a plan already discussed and agreed at the summit/PTG, I\u0027m okay with this the way it is.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"d9446ea690c7d5541f04e1bed12d159f3aadb9ad","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave"},{"line_number":11,"context_line":"vifs, volumes and any associated encryptors present on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"When using encrypted volumes this can lead to failures when the instance"},{"line_number":14,"context_line":"is then recreated, calling down into _get_guest_xml and"},{"line_number":15,"context_line":"_get_guest_storage_config and eventually connect_volume. With the"},{"line_number":16,"context_line":"encryptor attached this call will fail for most backends as the original"},{"line_number":17,"context_line":"path to the volume is now used to present the unencrypted device to the"},{"line_number":18,"context_line":"instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_892fd486","line":15,"range":{"start_line":13,"start_character":0,"end_line":15,"end_character":56},"in_reply_to":"5f4e5783_9d6a7b17","updated":"2017-10-17 08:09:37.000000000","message":"I think it was the case, by that argument \u0027reboot\u003dTrue\u0027 when calling \u0027_create_domain_and_network()\u0027.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"49cc7fb4b1247de65b6cd330b676202aa6f50e36","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave"},{"line_number":11,"context_line":"vifs, volumes and any associated encryptors present on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"When using encrypted volumes this can lead to failures when the instance"},{"line_number":14,"context_line":"is then recreated, calling down into _get_guest_xml and"},{"line_number":15,"context_line":"_get_guest_storage_config and eventually connect_volume. With the"},{"line_number":16,"context_line":"encryptor attached this call will fail for most backends as the original"},{"line_number":17,"context_line":"path to the volume is now used to present the unencrypted device to the"},{"line_number":18,"context_line":"instance."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_3a45c070","line":15,"range":{"start_line":13,"start_character":0,"end_line":15,"end_character":56},"in_reply_to":"5f4e5783_9d6a7b17","updated":"2017-10-17 10:39:02.000000000","message":"There are 2 underlying structural bugs here:\n\n1. A method called get_* has side effects\n2. We treat attaching/detaching an encryptor as a separate operation from attaching/detaching a volume in Nova code.\n\nThe first bug was originally introduced in change I1b1626f4 in 2011 when the method was called _prepare_xml_info (in nova/virt/libvirt/connection.py). Previously the code tried to parse info from the patch of an already-mounted volume device, and was replaced with a call to connect_volume, which also returned an xml fragment.\n\nWe should remove all side effects of this method. Unfortunately, given that the side effects have been there for 6 years at this point I would put good money on calling code assuming them in ways that are hard to see. IOW this is prime regression territory. I\u0027m in favour of cleaning this up, but it needs to be done very carefully and with good test coverage. There\u0027s also the issue that structural code cleanups in libvirt storage code have a history of poor review traction.\n\nI suspect the second issue is a wart of the separation of encryptors and initiators in os-brick. I can\u0027t think of a case where they\u0027re usefully separated. We probably need to combine them in a unified interface, but again that\u0027s not a small change.\n\nI like this change on its own merits because it more robustly \u0027redoes all the things\u0027 during reboot, which is something operators seem to appreciate. I agree that we\u0027re side-stepping other bugs, but fixing those bugs should be a separate project. They\u0027d also have almost no chance of being backported.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"98e271b6e4604e31e0840ed636713180274c7920","unresolved":false,"context_lines":[{"line_number":22,"context_line":"reboot the skipping of any logic associated with attaching the"},{"line_number":23,"context_line":"encryptors will leave the unencrypted volume attached."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"To correct both of these issues _hard_reboot now calls destroy to"},{"line_number":26,"context_line":"disconnect volume and detach encryptors. The reboot parameter is also"},{"line_number":27,"context_line":"dropped from the eventual call to _create_domain_and_network to ensure"},{"line_number":28,"context_line":"these are reconnected and attached prior to the instance being"},{"line_number":29,"context_line":"restarted."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_61cde464","line":26,"range":{"start_line":25,"start_character":0,"end_line":26,"end_character":40},"updated":"2017-10-16 15:44:35.000000000","message":"It sounds like this change is mostly dealing with instances that have attached encrypted block devices. Would it be possible to detect if an instance has those up front and then make a change to determine if the instance should be fully destroyed or not? Otherwise it seems like we\u0027re going through extra unnecessary work.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"1985c2da64440ca83702a89b35d107c1764a13a1","unresolved":false,"context_lines":[{"line_number":22,"context_line":"reboot the skipping of any logic associated with attaching the"},{"line_number":23,"context_line":"encryptors will leave the unencrypted volume attached."},{"line_number":24,"context_line":""},{"line_number":25,"context_line":"To correct both of these issues _hard_reboot now calls destroy to"},{"line_number":26,"context_line":"disconnect volume and detach encryptors. The reboot parameter is also"},{"line_number":27,"context_line":"dropped from the eventual call to _create_domain_and_network to ensure"},{"line_number":28,"context_line":"these are reconnected and attached prior to the instance being"},{"line_number":29,"context_line":"restarted."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"5f4e5783_109c3b68","line":26,"range":{"start_line":25,"start_character":0,"end_line":26,"end_character":40},"in_reply_to":"5f4e5783_61cde464","updated":"2017-10-16 16:48:05.000000000","message":"This change also makes sense to me under the \u0027use reboot to fix all the things\u0027 banner. Reboot isn\u0027t exactly a common operation, and we\u0027ve been overloading it for a while to do stuff like this.\n\nIOW, I think the extra work is a feature.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"f9ecbf261064171594b3917d3823bfc026614ba8","unresolved":false,"context_lines":[{"line_number":28,"context_line":"these are reconnected and attached prior to the instance being"},{"line_number":29,"context_line":"restarted."},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"Closes-bug: #1597234"},{"line_number":32,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":18,"id":"3f4b6375_b5bbbf0c","line":31,"range":{"start_line":31,"start_character":0,"end_line":31,"end_character":20},"updated":"2017-10-20 11:20:45.000000000","message":"So this is the original failure case I was attempting to correct. The additional issue is documented below:\n\nWhen using resume_guests_state_on_host_boot encrypted volumes are directly attached to instances after a host reboot \nhttps://bugs.launchpad.net/nova/+bug/1724573","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"499dab5c391193cf8b023e007ce15ea6af1f91ae","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave vifs"},{"line_number":11,"context_line":"plugged, volumes connected and encryptors attached on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"However _hard_reboot is also used to simply start instances. If used"},{"line_number":14,"context_line":"after a host reboot on an instance with encrypted volumes _hard_reboot"},{"line_number":15,"context_line":"will make no attempt to reattach the required encryptors leaving the"},{"line_number":16,"context_line":"fully encrypted volume attached to the instance."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This change corrects this by calling for a full destroy of the instance"},{"line_number":19,"context_line":"during a _hard_reboot before ensuring vifs are plugged and encryptors"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"3f4b6375_94c89e74","line":16,"range":{"start_line":13,"start_character":0,"end_line":16,"end_character":48},"updated":"2017-10-30 23:04:41.000000000","message":"If I understand the bug report correctly, this explanation isn\u0027t accurate. The problem is that anytime we assemble a guest config, we are trying to connect volumes. If the volumes had never been disconnected in the first place (the existing _hard_reboot) then we eventually try to connect a decrypted volume, which will fail with:\n\n ProcessExecutionError: Unexpected error while running command.\n2016-06-29 16:05:11.925 TRACE oslo_messaging.rpc.server Command: sudo nova-rootwrap /etc/nova/rootwrap.conf scsi_id --page 0x83 --whitelisted /dev/disk/by-path/ip-10.239.48.111:3260-iscsi-iqn.2010-10.org.openstack:volume-72ce7ebf-7400-47da-91f5-3173e01a199e-lun-1\n\nWe\u0027ve discussed this and I think the consensus was it\u0027s too risky to try to change the behavior of _get_storage_config to conditionally connect volumes and avoid connecting volumes if reboot\u003dTrue.\n\nThe alternative is this patch, which disconnects volumes as the first step in _hard_reboot, which sets us up to connect them again in the existing _get_storage_config when bringing up the guest domain.\n\nNOTE: We don\u0027t see this behavior on the master branch because os-brick on master will *retry* connect_volume when it hits the exception described in the bug. During its retry, it *disconnects the volume* and then tries to connect it again. The second attempt succeeds because the retry disconnected the volume first. I consider that a side-effect of os-brick\u0027s connect_volume implementation and I think we still need *this* patch to disconnect volumes before we try to connect them again, so as not to rely on an os-brick side-effect.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"458b1ace4842229c2aff253c89240e04db6d0625","unresolved":false,"context_lines":[{"line_number":10,"context_line":"_undefine_domain when hard rebooting an instance. This would leave vifs"},{"line_number":11,"context_line":"plugged, volumes connected and encryptors attached on the host."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"However _hard_reboot is also used to simply start instances. If used"},{"line_number":14,"context_line":"after a host reboot on an instance with encrypted volumes _hard_reboot"},{"line_number":15,"context_line":"will make no attempt to reattach the required encryptors leaving the"},{"line_number":16,"context_line":"fully encrypted volume attached to the instance."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"This change corrects this by calling for a full destroy of the instance"},{"line_number":19,"context_line":"during a _hard_reboot before ensuring vifs are plugged and encryptors"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"3f4b6375_77449d47","line":16,"range":{"start_line":13,"start_character":0,"end_line":16,"end_character":48},"in_reply_to":"3f4b6375_94c89e74","updated":"2017-11-02 20:41:33.000000000","message":"Correction: I just tested this locally with devstack master and found that after attaching an encrypted volume to an instance, rebooting the devstack VM, and trying to \u0027nova start\u0027 the instance, the instance will start BUT the encrypted device is no longer readable.\n\nBefore compute host reboot:\n\n $ sudo virsh domblklist 1\n Target     Source\n ------------------------------------------------\n vda        /opt/stack/data/nova/instances/e10ffc49-4da1-4594-a1af-a6507f994083/disk\n vdb        /dev/disk/by-id/scsi-360000000000000000e00000000010001\n\n $ ll /dev/disk/by-id/scsi-360000000000000000e00000000010001\n lrwxrwxrwx 1 root root 56 Nov  2 19:58 /dev/disk/by-id/scsi-360000000000000000e00000000010001 -\u003e /dev/mapper/crypt-scsi-360000000000000000e00000000010001\n\nAfter compute host reboot + \u0027nova start\u0027:\n\n $ sudo virsh domblklist 1\n Target     Source\n ------------------------------------------------\n vda        /opt/stack/data/nova/instances/e10ffc49-4da1-4594-a1af-a6507f994083/disk\n vdb        /dev/disk/by-id/scsi-360000000000000000e00000000010001\n $ ll /dev/disk/by-id/scsi-360000000000000000e00000000010001\n lrwxrwxrwx 1 root root 9 Nov  2 20:21 /dev/disk/by-id/scsi-360000000000000000e00000000010001 -\u003e ../../sdc","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5156dc6f492b39c5df58d7ed00f29c1f7957e395","unresolved":false,"context_lines":[{"line_number":19,"context_line":"during a _hard_reboot before ensuring vifs are plugged and encryptors"},{"line_number":20,"context_line":"attached in the resulting call to _create_domain_and_network."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-bug: #1724573"},{"line_number":23,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"3f4b6375_c421acfd","line":22,"range":{"start_line":22,"start_character":0,"end_line":22,"end_character":20},"updated":"2017-10-20 17:56:51.000000000","message":"Is this still the correct bug considering it\u0027s about resume_guests_state_on_host_boot? This patch doesn\u0027t have anything to do with resume_guests_state_on_host_boot, right?\n\nThe connection between the bug summary and this patch isn\u0027t clear to me and it\u0027s not clear if this should be Partial-Bug or if we need to split 1724573 into two bugs, one for the resume_guests_state_on_host_boot part and one for this part.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4b71b76fa9988e893197b6f70a2e98a4df9069dc","unresolved":false,"context_lines":[{"line_number":19,"context_line":"during a _hard_reboot before ensuring vifs are plugged and encryptors"},{"line_number":20,"context_line":"attached in the resulting call to _create_domain_and_network."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"Closes-bug: #1724573"},{"line_number":23,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":19,"id":"ff82abbf_f50a7024","line":22,"range":{"start_line":22,"start_character":0,"end_line":22,"end_character":20},"in_reply_to":"3f4b6375_c421acfd","updated":"2017-11-21 17:06:59.000000000","message":"I don\u0027t think we should repurpose bug 1724573 for this because I think it\u0027s confusing. We should have a separate bug that clearly explains the problem we\u0027re fixing here.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"36eac40d4b58458e336a4355f037fc5316e1c81d","unresolved":false,"context_lines":[{"line_number":43,"context_line":"idempotent. We depend on the fixes for those os-brick drivers."},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"Depends-On: I31d72357c89db53a147c2d986a28c9c6870efad0"},{"line_number":46,"context_line":"Depends-On: I9f52f89b8466d03699cfd5c0e32c672c934cd6fb"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"Closes-bug: #1724573"},{"line_number":49,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"ff82abbf_725f2f72","line":46,"updated":"2017-11-22 18:15:26.000000000","message":"These are not sufficient for ensuring we have a version of os-brick that contains the exit code fixes, unfortunately. The version of os-brick we are guaranteed to get with nova at any moment is described in requirements.txt [1]. So not only do the fixes have to merge to os-brick master, they must be released as a new version and that version must be present in requirements.txt to ensure we get the behavior we need.\n\nUntil we have a new version of os-brick released and included in requirements.txt, we have to handle it in the driver. OR we could hold this back via -2 until the release and pickup of the version in requirements.txt. Is the latter what you intended?\n\n[1] https://github.com/openstack/nova/blob/master/requirements.txt#L55","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"36eac40d4b58458e336a4355f037fc5316e1c81d","unresolved":false,"context_lines":[{"line_number":45,"context_line":"Depends-On: I31d72357c89db53a147c2d986a28c9c6870efad0"},{"line_number":46,"context_line":"Depends-On: I9f52f89b8466d03699cfd5c0e32c672c934cd6fb"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"Closes-bug: #1724573"},{"line_number":49,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"ff82abbf_12d103fc","line":48,"updated":"2017-11-22 18:15:26.000000000","message":"Again, I think this bug either needs to be different or edited in launchpad to concisely describe the problem. I\u0027m concerned about the constant mention of \"resume_guests_state_on_host_boot\" in the bug when it\u0027s not the crux of the bug.\n\nTo someone who isn\u0027t familiar as we are with this bug and patch, I don\u0027t think the bug is clear and it focuses on resume_guests_state_on_host_boot which I think will mislead people who are looking at bugs upstream and trying to apply them to downstream issues, for example.\n\nI want us to be really clear with how this is documented in the bug so that other folks can easily track it.","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"23de09fe519a288a75002d76c874a54ca96d6bb3","unresolved":false,"context_lines":[{"line_number":45,"context_line":"Depends-On: I31d72357c89db53a147c2d986a28c9c6870efad0"},{"line_number":46,"context_line":"Depends-On: I9f52f89b8466d03699cfd5c0e32c672c934cd6fb"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":"Closes-bug: #1724573"},{"line_number":49,"context_line":"Change-Id: Id188d48609f3d22d14e16c7f6114291d547a8986"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":20,"id":"ff82abbf_a81a8d6b","line":48,"in_reply_to":"ff82abbf_12d103fc","updated":"2017-11-29 21:39:07.000000000","message":"I\u0027ve edited the launchpad bug 1724573 to remove mention of resume_guests_state_on_host_boot to boil the problem down to the simplest case.","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"1f2ddc2be21e4bfe171d1a748189acdb0952c754","unresolved":false,"context_lines":[{"line_number":2332,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2333,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2334,"context_line":"        # console for guest will run smoothly."},{"line_number":2335,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2336,"context_line":"                     block_device_info\u003dblock_device_info, migrate_data\u003dNone)"},{"line_number":2337,"context_line":""},{"line_number":2338,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a71b18c_d43aea97","line":2335,"updated":"2016-12-02 13:07:33.000000000","message":"Hum It\u0027s not something we should do, the problem lead to line ~2353 when we call _get_guest_xml. Basically I think for a hard reboot we should backup the domain XML then recreate it. So we avoid to call get_guest_xml which is basically doing more than just returning the guest config.","commit_id":"c14e4a4a99c4dc1cf2316f82ce2af226488da66c"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"60098f5a71749c9374cdd5a8a80fd9c44231a83c","unresolved":false,"context_lines":[{"line_number":2332,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2333,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2334,"context_line":"        # console for guest will run smoothly."},{"line_number":2335,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2336,"context_line":"                     block_device_info\u003dblock_device_info, migrate_data\u003dNone)"},{"line_number":2337,"context_line":""},{"line_number":2338,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a71b18c_6155a1f8","line":2335,"updated":"2016-12-05 07:59:05.000000000","message":"I actually agree with Sahid here, we\u0027ve spoken recently of persisting domains anyway so.... I\u0027m stuck in meetings all week but I\u0027ll try to rework this for review. Thanks both.","commit_id":"c14e4a4a99c4dc1cf2316f82ce2af226488da66c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"3f7d2181b7578b307f90c79bb9e9275a2b85f77e","unresolved":false,"context_lines":[{"line_number":2332,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2333,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2334,"context_line":"        # console for guest will run smoothly."},{"line_number":2335,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2336,"context_line":"                     block_device_info\u003dblock_device_info, migrate_data\u003dNone)"},{"line_number":2337,"context_line":""},{"line_number":2338,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a71b18c_7665531c","line":2335,"in_reply_to":"3a71b18c_6155a1f8","updated":"2016-12-05 14:49:58.000000000","message":"Yes I think it\u0027s reasonable to dig a little more around. Let see what Lee is going to provide. I\u0027m worry about calling destroy, that method is doing much more than what we need to fix the bug and it could reside new issues.","commit_id":"c14e4a4a99c4dc1cf2316f82ce2af226488da66c"},{"author":{"_account_id":6608,"name":"Paul Carlton","email":"paul.carlton2@hpe.com","username":"paul-carlton2"},"change_message_id":"c1052fc6c85323a98b6f119319dbefd53c022293","unresolved":false,"context_lines":[{"line_number":2332,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2333,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2334,"context_line":"        # console for guest will run smoothly."},{"line_number":2335,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2336,"context_line":"                     block_device_info\u003dblock_device_info, migrate_data\u003dNone)"},{"line_number":2337,"context_line":""},{"line_number":2338,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a71b18c_f98ecbce","line":2335,"in_reply_to":"3a71b18c_d43aea97","updated":"2016-12-05 06:40:20.000000000","message":"I see sahid\u0027s point but don\u0027t agree, this fix works and aligns with the current implementation","commit_id":"c14e4a4a99c4dc1cf2316f82ce2af226488da66c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"eae3b1a0774447ce6a0f6b181d0e0d0cea0f3bf6","unresolved":false,"context_lines":[{"line_number":2355,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2356,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2357,"context_line":"        # console for guest will run smoothly."},{"line_number":2358,"context_line":"        self._undefine_domain(instance)"},{"line_number":2359,"context_line":""},{"line_number":2360,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2361,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":8,"id":"1a430d35_73e12fd0","side":"PARENT","line":2358,"updated":"2017-02-06 16:44:16.000000000","message":"So you removed this, but that has been specifically set because of a bug when hard-reboot an instance with serial-consoles.\n\nIf you do not undefine the domain from libvirt the code in get_guest_config is going to think that the ports for serial consoles did not have been released but actually they have... It was something like that I can\u0027t really recall.\n\nCan you just verify whether your change is not going to create any regressions in that way ?","commit_id":"06e400384c432ebbefe75029ba3ae7791f367c40"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"570f24df128a61e990692204575a90f8fb72a12e","unresolved":false,"context_lines":[{"line_number":2355,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2356,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2357,"context_line":"        # console for guest will run smoothly."},{"line_number":2358,"context_line":"        self._undefine_domain(instance)"},{"line_number":2359,"context_line":""},{"line_number":2360,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2361,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":8,"id":"1a430d35_e401bb39","side":"PARENT","line":2358,"in_reply_to":"1a430d35_042263b9","updated":"2017-02-07 09:28:17.000000000","message":"Ok I saw _destroy.","commit_id":"06e400384c432ebbefe75029ba3ae7791f367c40"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"ae55d48fb60835ed3886e8d60961d83da1c58b8a","unresolved":false,"context_lines":[{"line_number":2355,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2356,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2357,"context_line":"        # console for guest will run smoothly."},{"line_number":2358,"context_line":"        self._undefine_domain(instance)"},{"line_number":2359,"context_line":""},{"line_number":2360,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2361,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":8,"id":"1a430d35_042263b9","side":"PARENT","line":2358,"in_reply_to":"1a430d35_73e12fd0","updated":"2017-02-06 17:49:02.000000000","message":"_undefine_domain is called at the very end of cleanup so the behaviour should remain the same AFAICT.","commit_id":"06e400384c432ebbefe75029ba3ae7791f367c40"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"1ee61f34cde255d4f0dbde5b547811d8f1fb868b","unresolved":false,"context_lines":[{"line_number":2477,"context_line":"        self._destroy(instance)"},{"line_number":2478,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2479,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2480,"context_line":"        # console for guest will run smoothly."},{"line_number":2481,"context_line":"        self._undefine_domain(instance)"},{"line_number":2482,"context_line":""},{"line_number":2483,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f436f4f_64d4f948","side":"PARENT","line":2480,"updated":"2017-08-14 08:58:05.000000000","message":"nit: this comment makes somewhat less sense now","commit_id":"f342a42550326b81ad2a625f1aa7e1dd41895714"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"9eb3fc88cd038fc8e1f9e368416a31948f01ec6e","unresolved":false,"context_lines":[{"line_number":2477,"context_line":"        self._destroy(instance)"},{"line_number":2478,"context_line":"        # Domain XML will be redefined so we can safely undefine it"},{"line_number":2479,"context_line":"        # from libvirt. This ensure that such process as create serial"},{"line_number":2480,"context_line":"        # console for guest will run smoothly."},{"line_number":2481,"context_line":"        self._undefine_domain(instance)"},{"line_number":2482,"context_line":""},{"line_number":2483,"context_line":"        # Convert the system metadata to image metadata"}],"source_content_type":"text/x-python","patch_set":17,"id":"7f287b81_14261cb9","side":"PARENT","line":2480,"in_reply_to":"9f436f4f_64d4f948","updated":"2017-08-22 09:15:30.000000000","message":"I would say it still make sense since \u0027destroy\u0027 is going to call \u0027_destroy\u0027 and \u0027undefine\u0027.\n\nBut it\u0027s good point to update the comment and so add something referring that change.","commit_id":"f342a42550326b81ad2a625f1aa7e1dd41895714"},{"author":{"_account_id":24711,"name":"Jackie Truong","email":"jacklyn.truong@jhuapl.edu","username":"jackie-truong"},"change_message_id":"9a91445ae36f7f521be7cc8043a306c56f0cf210","unresolved":false,"context_lines":[{"line_number":5193,"context_line":""},{"line_number":5194,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5195,"context_line":"                                   block_device_info\u003dNone,"},{"line_number":5196,"context_line":"                                   power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":5197,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5198,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5199,"context_line":"                                   destroy_disks_on_failure\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":17,"id":"9f436f4f_ee5d5647","line":5196,"range":{"start_line":5196,"start_character":50,"end_line":5196,"end_character":62},"updated":"2017-08-14 19:46:59.000000000","message":"After this patch, will reboot ever be True? Should this arg be removed so that network initialization will always attach encryptors and connect volumes regardless of a reboot?","commit_id":"b73f9990784deadecc1f97488240b106cc5b127a"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"45d3848c431845dc41698b51f3c75fd5beca9005","unresolved":false,"context_lines":[{"line_number":5193,"context_line":""},{"line_number":5194,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5195,"context_line":"                                   block_device_info\u003dNone,"},{"line_number":5196,"context_line":"                                   power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":5197,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5198,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5199,"context_line":"                                   destroy_disks_on_failure\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":17,"id":"5f4e5783_4dc13280","line":5196,"in_reply_to":"7f287b81_d4d89480","updated":"2017-10-13 12:07:27.000000000","message":"Quick audit says we\u0027d actually be removing the last one here.","commit_id":"b73f9990784deadecc1f97488240b106cc5b127a"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"9eb3fc88cd038fc8e1f9e368416a31948f01ec6e","unresolved":false,"context_lines":[{"line_number":5193,"context_line":""},{"line_number":5194,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5195,"context_line":"                                   block_device_info\u003dNone,"},{"line_number":5196,"context_line":"                                   power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":5197,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5198,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5199,"context_line":"                                   destroy_disks_on_failure\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":17,"id":"7f287b81_d4d89480","line":5196,"in_reply_to":"9f436f4f_ee5d5647","updated":"2017-08-22 09:15:30.000000000","message":"Hum Yes that function is doing so much things that we could probably split it instead of having so many arguments. But I don\u0027t think removing reboot it\u0027s something we can do so far, there are still some events where that function is called and the volumes are already attached.","commit_id":"b73f9990784deadecc1f97488240b106cc5b127a"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe12d0e70d4ccc0d6665be5391750cdd3bc9c5fc","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2489,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_04ba76dd","line":2486,"updated":"2017-10-18 05:04:08.000000000","message":"So this will unplug vifs, because destroy calls cleanup using the default destroy_vifs\u003dTrue, which will unplug the vifs.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"fe12d0e70d4ccc0d6665be5391750cdd3bc9c5fc","unresolved":false,"context_lines":[{"line_number":2520,"context_line":"        # start the instance."},{"line_number":2521,"context_line":"        self._create_domain_and_network(context, xml, instance, network_info,"},{"line_number":2522,"context_line":"                                        block_device_info\u003dblock_device_info,"},{"line_number":2523,"context_line":"                                        vifs_already_plugged\u003dTrue)"},{"line_number":2524,"context_line":"        self._prepare_pci_devices_for_use("},{"line_number":2525,"context_line":"            pci_manager.get_instance_pci_devs(instance, \u0027all\u0027))"},{"line_number":2526,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_c4cb3e70","line":2523,"range":{"start_line":2523,"start_character":40,"end_line":2523,"end_character":65},"updated":"2017-10-18 05:04:08.000000000","message":"But this is indicating the vifs are already plugged, which they\u0027re not with this change. So shouldn\u0027t we stop passing vifs_already_plugged\u003dTrue so we will wait for vif plug events during vif plugging?","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5156dc6f492b39c5df58d7ed00f29c1f7957e395","unresolved":false,"context_lines":[{"line_number":2520,"context_line":"        # start the instance."},{"line_number":2521,"context_line":"        self._create_domain_and_network(context, xml, instance, network_info,"},{"line_number":2522,"context_line":"                                        block_device_info\u003dblock_device_info,"},{"line_number":2523,"context_line":"                                        vifs_already_plugged\u003dTrue)"},{"line_number":2524,"context_line":"        self._prepare_pci_devices_for_use("},{"line_number":2525,"context_line":"            pci_manager.get_instance_pci_devs(instance, \u0027all\u0027))"},{"line_number":2526,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_6409d893","line":2523,"range":{"start_line":2523,"start_character":40,"end_line":2523,"end_character":65},"in_reply_to":"3f4b6375_79754b98","updated":"2017-10-20 17:56:51.000000000","message":"So, this keyword arg controls whether we will wait for vif plug events from neutron -- not whether we will plug vifs. So I think if vifs_already_plugged\u003dTrue, vifs will anyway be plugged but we won\u0027t wait for plug events from neutron. I\u0027m not sure if that\u0027s intentional (to plug vifs regardless of this keyword arg). Perhaps the vif_driver.plug() is a no-op if they\u0027re already plugged.\n\nBut, if we know the vifs are unplugged when we start, I think we should be making sure we wait for neutron vif plug events while we plug them back in.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"f9ecbf261064171594b3917d3823bfc026614ba8","unresolved":false,"context_lines":[{"line_number":2520,"context_line":"        # start the instance."},{"line_number":2521,"context_line":"        self._create_domain_and_network(context, xml, instance, network_info,"},{"line_number":2522,"context_line":"                                        block_device_info\u003dblock_device_info,"},{"line_number":2523,"context_line":"                                        vifs_already_plugged\u003dTrue)"},{"line_number":2524,"context_line":"        self._prepare_pci_devices_for_use("},{"line_number":2525,"context_line":"            pci_manager.get_instance_pci_devs(instance, \u0027all\u0027))"},{"line_number":2526,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_79754b98","line":2523,"range":{"start_line":2523,"start_character":40,"end_line":2523,"end_character":65},"in_reply_to":"3f4b6375_c4cb3e70","updated":"2017-10-20 11:20:45.000000000","message":"Very true, odd that CI didn\u0027t catch this as VIFs wouldn\u0027t be plugged in here during a hard reboot.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"98e271b6e4604e31e0840ed636713180274c7920","unresolved":false,"context_lines":[{"line_number":5195,"context_line":""},{"line_number":5196,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5197,"context_line":"                                   block_device_info\u003dNone,"},{"line_number":5198,"context_line":"                                   power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":5199,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5200,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5201,"context_line":"                                   destroy_disks_on_failure\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f4e5783_e1d43422","line":5198,"range":{"start_line":5198,"start_character":50,"end_line":5198,"end_character":62},"updated":"2017-10-16 15:44:35.000000000","message":"Looks like this is never used now. Should be removed if so, probably in a follow up change.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"6d838619c9f1dfb5efec5640c4533b48a8f7760c","unresolved":false,"context_lines":[{"line_number":5195,"context_line":""},{"line_number":5196,"context_line":"    def _create_domain_and_network(self, context, xml, instance, network_info,"},{"line_number":5197,"context_line":"                                   block_device_info\u003dNone,"},{"line_number":5198,"context_line":"                                   power_on\u003dTrue, reboot\u003dFalse,"},{"line_number":5199,"context_line":"                                   vifs_already_plugged\u003dFalse,"},{"line_number":5200,"context_line":"                                   post_xml_callback\u003dNone,"},{"line_number":5201,"context_line":"                                   destroy_disks_on_failure\u003dFalse):"}],"source_content_type":"text/x-python","patch_set":18,"id":"5f4e5783_104d5bc3","line":5198,"range":{"start_line":5198,"start_character":50,"end_line":5198,"end_character":62},"in_reply_to":"5f4e5783_e1d43422","updated":"2017-10-16 17:00:21.000000000","message":"+1 . Turns out this is trivial, as we never tested it:\n\nhttps://review.openstack.org/512363","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"f9ecbf261064171594b3917d3823bfc026614ba8","unresolved":false,"context_lines":[{"line_number":5210,"context_line":"            if (not reboot and \u0027data\u0027 in connection_info and"},{"line_number":5211,"context_line":"                    \u0027volume_id\u0027 in connection_info[\u0027data\u0027]):"},{"line_number":5212,"context_line":"                volume_id \u003d connection_info[\u0027data\u0027][\u0027volume_id\u0027]"},{"line_number":5213,"context_line":"                encryption \u003d encryptors.get_encryption_metadata("},{"line_number":5214,"context_line":"                    context, self._volume_api, volume_id, connection_info)"},{"line_number":5215,"context_line":""},{"line_number":5216,"context_line":"                if encryption:"},{"line_number":5217,"context_line":"                    encryptor \u003d self._get_volume_encryptor(connection_info,"}],"source_content_type":"text/x-python","patch_set":18,"id":"3f4b6375_75ed4724","line":5214,"range":{"start_line":5213,"start_character":0,"end_line":5214,"end_character":74},"updated":"2017-10-20 11:20:45.000000000","message":"We can\u0027t do this lookup with the admin context provided from init_host in the compute layer. Why this isn\u0027t stashed in the BDMs is beyond me.","commit_id":"94dae1a4ae9453b5198315e620d0ade2b1239dd6"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"458b1ace4842229c2aff253c89240e04db6d0625","unresolved":false,"context_lines":[{"line_number":988,"context_line":"                    # encryptor may report that the volume is still in use."},{"line_number":989,"context_line":"                    encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":990,"context_line":"                                                           encryption)"},{"line_number":991,"context_line":"                    encryptor.detach_volume(**encryption)"},{"line_number":992,"context_line":""},{"line_number":993,"context_line":"            try:"},{"line_number":994,"context_line":"                self._disconnect_volume(connection_info, disk_dev, instance)"}],"source_content_type":"text/x-python","patch_set":19,"id":"3f4b6375_01ec0b8d","line":991,"range":{"start_line":991,"start_character":20,"end_line":991,"end_character":57},"updated":"2017-11-02 20:41:33.000000000","message":"We need to try-except around this to catch ProcessExecutionError and ignore it if exit_code\u003d4 (wrong device) else reraise.\n\nThe reason is because this patch adds a first step of \"disconnect all volumes\" in _hard_reboot before connecting volumes -- but after a compute host reboot, all devices start out disconnected, so an attempt to disconnect already disconnected encrypted volumes will raise ProcessExecutionError with exit_code 4, so we would need to ignore that. Example:\n\nhttps://github.com/openstack/nova/blob/master/nova/virt/libvirt/storage/dmcrypt.py#L72-L84","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cb2b873d84f89e803695c689e0d6b6926bb09f1f","unresolved":false,"context_lines":[{"line_number":988,"context_line":"                    # encryptor may report that the volume is still in use."},{"line_number":989,"context_line":"                    encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":990,"context_line":"                                                           encryption)"},{"line_number":991,"context_line":"                    encryptor.detach_volume(**encryption)"},{"line_number":992,"context_line":""},{"line_number":993,"context_line":"            try:"},{"line_number":994,"context_line":"                self._disconnect_volume(connection_info, disk_dev, instance)"}],"source_content_type":"text/x-python","patch_set":19,"id":"ff82abbf_9e40ee09","line":991,"range":{"start_line":991,"start_character":20,"end_line":991,"end_character":57},"in_reply_to":"3f4b6375_01ec0b8d","updated":"2017-11-21 14:03:50.000000000","message":"I think exit codes are best left to the driver. I submitted a patch to os-brick here:\n\nhttps://review.openstack.org/#/c/521809/","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4b71b76fa9988e893197b6f70a2e98a4df9069dc","unresolved":false,"context_lines":[{"line_number":988,"context_line":"                    # encryptor may report that the volume is still in use."},{"line_number":989,"context_line":"                    encryptor \u003d self._get_volume_encryptor(connection_info,"},{"line_number":990,"context_line":"                                                           encryption)"},{"line_number":991,"context_line":"                    encryptor.detach_volume(**encryption)"},{"line_number":992,"context_line":""},{"line_number":993,"context_line":"            try:"},{"line_number":994,"context_line":"                self._disconnect_volume(connection_info, disk_dev, instance)"}],"source_content_type":"text/x-python","patch_set":19,"id":"ff82abbf_9521a4fe","line":991,"range":{"start_line":991,"start_character":20,"end_line":991,"end_character":57},"in_reply_to":"ff82abbf_9e40ee09","updated":"2017-11-21 17:06:59.000000000","message":"I agree (and thanks for doing that) but until we have an os-brick release and its version in our requirements.txt, we have to cover it here in the driver with a TODO/FIXME comment to remove it upon the next os-brick release and requirements.txt update, else this patch won\u0027t work to recover from a compute host reboot (we have seen \u0027nova start\u0027 of the instance fail with exit code 4 from os-brick after a compute host reboot).","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2d3f68fc9168912727beef6988d5c08665079166","unresolved":false,"context_lines":[{"line_number":2481,"context_line":"        OS cannot ignore this action."},{"line_number":2482,"context_line":"        \"\"\""},{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"3f4b6375_b726f915","line":2484,"updated":"2017-10-31 00:16:46.000000000","message":"I think we should add more to this comment about why this is needed for re-attachment of volumes after a compute host reboots.\n\nWe know that os-brick retries connect_volume when it fails the first time due to the decrypted device and succeeds on the second try after it disconnects the volume and connects it again. But we also know this isn\u0027t sufficient when the compute host has been rebooted.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cb2b873d84f89e803695c689e0d6b6926bb09f1f","unresolved":false,"context_lines":[{"line_number":2481,"context_line":"        OS cannot ignore this action."},{"line_number":2482,"context_line":"        \"\"\""},{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"ff82abbf_9b6fa0c4","line":2484,"in_reply_to":"3f4b6375_b726f915","updated":"2017-11-21 14:03:50.000000000","message":"We\u0027re hitting this particular problem today, but remember that hard reboot is operators\u0027 \"try to fix all the things automatically\" tool. I\u0027m all for expansive comments, but for me the best reason to do a complete destroy of everything here is that it\u0027s what it should have been doing anyway in order to try to fix all the things. I don\u0027t think it\u0027s worth describing a specific rabbit hole here, that\u0027s best left to the commit message. I do think it\u0027s worth making \u0027fix all the things\u0027 explicit, though. This comment would be a good place for that.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4b71b76fa9988e893197b6f70a2e98a4df9069dc","unresolved":false,"context_lines":[{"line_number":2481,"context_line":"        OS cannot ignore this action."},{"line_number":2482,"context_line":"        \"\"\""},{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"ff82abbf_155cb47c","line":2484,"in_reply_to":"ff82abbf_9b6fa0c4","updated":"2017-11-21 17:06:59.000000000","message":"Okay. Yeah, I\u0027m mostly thinking this area of the code is where I could see a future developer thinking \"this is unnecessary, I could optimize this\" and undo what we have here someday, if it\u0027s not explained clearly in a comment. So I think we do need a more explicit comment here.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"499dab5c391193cf8b023e007ce15ea6af1f91ae","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2489,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":19,"id":"3f4b6375_4404b5c6","line":2486,"updated":"2017-10-30 23:04:41.000000000","message":"I think, along with this, we need to try-catch around every (make a helper function) encryptor.detach_volume to catch ProcessExecutionError and ignore exit code 4 (device inactive) because after a reboot of the compute host, all devices will be inactive and this step to detach volumes will raise ProcessExecutionError with exit code 4 from encryptor.detach_volume and fail to proceed.\n\nI\u0027m not sure if try-catching ProcessExecutionError should be part of this change or a separate change, TBH. Because currently any attempt to detach_volume of an encrypted volume that has already been detached, will raise similarly.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"4b71b76fa9988e893197b6f70a2e98a4df9069dc","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2489,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":19,"id":"ff82abbf_d503ac41","line":2486,"in_reply_to":"1f485f77_1648c184","updated":"2017-11-21 17:06:59.000000000","message":"Replied, see above.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"cb2b873d84f89e803695c689e0d6b6926bb09f1f","unresolved":false,"context_lines":[{"line_number":2483,"context_line":"        # NOTE(lyarwood): Start by fully destroying the instance, undefining"},{"line_number":2484,"context_line":"        # the domain and detaching vifs, volumes and encryptors."},{"line_number":2485,"context_line":"        self.destroy(context, instance, network_info, destroy_disks\u003dFalse,"},{"line_number":2486,"context_line":"                     block_device_info\u003dblock_device_info)"},{"line_number":2487,"context_line":""},{"line_number":2488,"context_line":"        # Convert the system metadata to image metadata"},{"line_number":2489,"context_line":"        # NOTE(mdbooth): This is a workaround for stateless Nova compute"}],"source_content_type":"text/x-python","patch_set":19,"id":"1f485f77_1648c184","line":2486,"in_reply_to":"3f4b6375_4404b5c6","updated":"2017-11-21 14:03:50.000000000","message":"See above. I think this is best left to os-brick.","commit_id":"a9bb155cf88b5f865aed5f440133d112a3dde228"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e73b1c4b8002e772c4d4b711d246cf307cc955c6","unresolved":false,"context_lines":[{"line_number":2634,"context_line":"        \"\"\"Power off the specified instance.\"\"\""},{"line_number":2635,"context_line":"        if timeout:"},{"line_number":2636,"context_line":"            self._clean_shutdown(instance, timeout, retry_interval)"},{"line_number":2637,"context_line":"        self._destroy(instance)"},{"line_number":2638,"context_line":""},{"line_number":2639,"context_line":"    def power_on(self, context, instance, network_info,"},{"line_number":2640,"context_line":"                 block_device_info\u003dNone):"}],"source_content_type":"text/x-python","patch_set":20,"id":"df87a7cf_464b3658","line":2637,"range":{"start_line":2637,"start_character":13,"end_line":2637,"end_character":21},"updated":"2017-12-06 14:34:59.000000000","message":"Should this be destroy() now? Or do we want to hold onto the vifs and block devices while the guest is stopped?","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c83435c26023c9f0066efffe4d2b8b35449aa233","unresolved":false,"context_lines":[{"line_number":2642,"context_line":"        # We use _hard_reboot here to ensure that all backing files,"},{"line_number":2643,"context_line":"        # network, and block device connections, etc. are established"},{"line_number":2644,"context_line":"        # and available before we attempt to start the instance."},{"line_number":2645,"context_line":"        self._hard_reboot(context, instance, network_info, block_device_info)"},{"line_number":2646,"context_line":""},{"line_number":2647,"context_line":"    def trigger_crash_dump(self, instance):"},{"line_number":2648,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"df87a7cf_7855c9c5","line":2645,"range":{"start_line":2645,"start_character":13,"end_line":2645,"end_character":25},"updated":"2017-12-06 02:19:25.000000000","message":"This makes the start API quite a bit more destructive with respect to block devices and VIFs...","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4028ea217634daa82fd2fe74d3a0a75be9051c57","unresolved":false,"context_lines":[{"line_number":2642,"context_line":"        # We use _hard_reboot here to ensure that all backing files,"},{"line_number":2643,"context_line":"        # network, and block device connections, etc. are established"},{"line_number":2644,"context_line":"        # and available before we attempt to start the instance."},{"line_number":2645,"context_line":"        self._hard_reboot(context, instance, network_info, block_device_info)"},{"line_number":2646,"context_line":""},{"line_number":2647,"context_line":"    def trigger_crash_dump(self, instance):"},{"line_number":2648,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"df87a7cf_222ff5a1","line":2645,"range":{"start_line":2645,"start_character":13,"end_line":2645,"end_character":25},"in_reply_to":"df87a7cf_267ab287","updated":"2017-12-06 20:19:54.000000000","message":"\u003e The comment above suggests that\u0027s the intention. I think we fixed\n \u003e the code to make it do what the comments says it does :)\n\nSure, but, when that comment was written it thought things were cool with what we did before this change too. So just saying, \"yeah the comment says it\u0027s good so it\u0027s good\" doesn\u0027t really cut it. The reason I worry about stuff like this is because the libvirt driver is so fragile that changing one thing a lot of the time leads to breaking two other things. So rather than \"yeah this is better, trust me\" I like to know that someone has investigated and can explain it before we pull the trigger. Because if/when it does break something else then I know I can go back to that person that understands the original problem and can tap them to help fix the regression.","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"2e719a4170bd104e4f63a9db011a307a044a3ea7","unresolved":false,"context_lines":[{"line_number":2642,"context_line":"        # We use _hard_reboot here to ensure that all backing files,"},{"line_number":2643,"context_line":"        # network, and block device connections, etc. are established"},{"line_number":2644,"context_line":"        # and available before we attempt to start the instance."},{"line_number":2645,"context_line":"        self._hard_reboot(context, instance, network_info, block_device_info)"},{"line_number":2646,"context_line":""},{"line_number":2647,"context_line":"    def trigger_crash_dump(self, instance):"},{"line_number":2648,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"df87a7cf_ec7029df","line":2645,"range":{"start_line":2645,"start_character":13,"end_line":2645,"end_character":25},"in_reply_to":"df87a7cf_7855c9c5","updated":"2017-12-06 16:58:12.000000000","message":"Right. That is indeed the intention. The most common failure mode of the bug we\u0027re trying to fix here is this:\n\n1. Boot an instance with an attached encrypted volume\n2. Reboot the compute host\n3. When the compute host comes back up, the instance will be in state: SHUTDOWN (this is the expected behavior if CONF.resume_guests_state_on_host_boot \u003d False, which is the default)\n4. The instance needs to be started, so \u0027nova start\u0027\n5. The instance starts but the attached encrypted volume cannot be read by the guest\n\nTo fix it, we need to have the encrypted volume decrypted and mounted to the guest during \u0027nova start\u0027. That routine is already part of _create_domain_and_network as long as we don\u0027t pass reboot\u003dTrue.\n\nAs for the \u0027re-initialize everything\u0027 approach, there are earlier comments on this review explaining that this is something that\u0027s been discussed at PTG/summits before as a way to provide operators some way to get everything reset to a clean slate in the case where something has gone wrong with attached vifs or volumes.\n\nAlong that line, this patch will fix one of the issues \"nova VM state inconsistency when compute host reboot\" [1] brought up at the forum at the Public Cloud Working Group session. There, OVH was seeing a problem where after compute host reboot and \u0027nova start\u0027 the instance has no network. With this patch, we will re-attach volumes and re-plug vifs.\n\n[1] https://docs.google.com/spreadsheets/d/1Mf8OAyTzZxCKzYHMgBl-QK_2-XSycSkOjqCyMTIedkA/edit#gid\u003d0\u0026range\u003dF28","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0b5041d350402f85c7df8f34f8bfb785806ee1a9","unresolved":false,"context_lines":[{"line_number":2642,"context_line":"        # We use _hard_reboot here to ensure that all backing files,"},{"line_number":2643,"context_line":"        # network, and block device connections, etc. are established"},{"line_number":2644,"context_line":"        # and available before we attempt to start the instance."},{"line_number":2645,"context_line":"        self._hard_reboot(context, instance, network_info, block_device_info)"},{"line_number":2646,"context_line":""},{"line_number":2647,"context_line":"    def trigger_crash_dump(self, instance):"},{"line_number":2648,"context_line":""}],"source_content_type":"text/x-python","patch_set":20,"id":"df87a7cf_267ab287","line":2645,"range":{"start_line":2645,"start_character":13,"end_line":2645,"end_character":25},"in_reply_to":"df87a7cf_7855c9c5","updated":"2017-12-06 14:35:07.000000000","message":"The comment above suggests that\u0027s the intention. I think we fixed the code to make it do what the comments says it does :)","commit_id":"3f8daf080411b84ec0669f0642524ce8a7d19057"}]}