| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-09-16 | |||
| 15:45:41 | gibi | +2+A | |
| 15:45:42 | gibi | ;d | |
| 15:52:03 | melwitt | bauzas: that sounds familiar and I think "probably". I need to look around a bit to find a related/duplicate bug I've seen and add more information | |
| 15:52:31 | bauzas | melwitt: kk, marking it as confirmed for now, but feel free to mark it duplicate if so | |
| 15:52:37 | bauzas | thanks | |
| 15:52:48 | melwitt | sounds good, thanks | |
| 16:03:16 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Make overcommit check for pinned instance pagesize aware https://review.opendev.org/631053 | |
| 17:07:23 | sean-k-mooney | gibi: bauzas melwitt can i get your input on how backportale someting is quickly | |
| 17:08:47 | sean-k-mooney | basically i want to add a call to unplug_vifs in migrate_disk_and_power_off which is caleed in _resize_instance | |
| 17:09:12 | sean-k-mooney | and ideally remove the call to clean up the souce vifs form resize confirm | |
| 17:09:48 | sean-k-mooney | so when we do resize_instance before we go to resize_verify we would unplug the souce vif | |
| 17:09:58 | openstackgerrit | Merged openstack/nova stable/train: post live migration: don't call Neutron needlessly https://review.opendev.org/750670 | |
| 17:10:05 | sean-k-mooney | basicaly right after we call power off just like we remove the cinder attahcments | |
| 17:10:06 | openstackgerrit | Merged openstack/nova master: docs: Improve 'AggregateImagePropertiesIsolation' docs https://review.opendev.org/752236 | |
| 17:10:14 | openstackgerrit | Merged openstack/nova master: functional: Enable real policy fixture by default https://review.opendev.org/745906 | |
| 17:10:23 | openstackgerrit | Merged openstack/nova master: functional: Add and use 'GlanceFixture' https://review.opendev.org/745907 | |
| 17:11:24 | sean-k-mooney | it would fix bug #1895220 and i also need to do it for https://review.opendev.org/#/c/602432 to adress bug #1734320 | |
| 17:11:24 | openstack | bug 1895220 in OpenStack Compute (nova) "Fail to cold migrate between two hosts have difference VIF type" [Medium,In progress] https://launchpad.net/bugs/1895220 - Assigned to Dat Le (datlq) | |
| 17:11:25 | openstack | bug 1734320 in neutron "Eavesdropping private traffic" [High,In progress] https://launchpad.net/bugs/1734320 - Assigned to Rodolfo Alonso (rodolfo-alonso-hernandez) | |
| 17:11:52 | openstackgerrit | melanie witt proposed openstack/nova stable/train: Sanity check instance mapping during scheduling https://review.opendev.org/752279 | |
| 17:13:49 | openstackgerrit | melanie witt proposed openstack/nova stable/train: Sanity check instance mapping during scheduling https://review.opendev.org/752279 | |
| 17:16:42 | sean-k-mooney | lyarwood: elod i guess same question to ye since its about backportablity ^ | |
| 17:17:59 | sean-k-mooney | the current behavior rely on libvirt to unplug the vif so to maintain the correct behavior when we deleate that to os-vif we now need to call unplug_vifs explcitly | |
| 17:18:16 | sean-k-mooney | which we should have been doing along really | |
| 17:21:05 | melwitt | sean-k-mooney: is the unplug_vifs functionality in os-vif available in older branches? | |
| 17:22:16 | sean-k-mooney | yes | |
| 17:22:27 | sean-k-mooney | unplug_vifs is a libvirt driver function | |
| 17:22:45 | sean-k-mooney | but the os-vif support is also there in older branches | |
| 17:23:30 | sean-k-mooney | melwitt: it look like im going to either need to make this change to when we do it | |
| 17:23:50 | sean-k-mooney | e.g. from confirm_resize where it is today to resize_instance | |
| 17:24:13 | sean-k-mooney | or im going to have to alter where and if nova waits for vifs in revert | |
| 17:25:01 | sean-k-mooney | or i guess i could have revert call unplug_vifs before it calls plug_vifs but that is slightly more complicated. | |
| 17:26:09 | sean-k-mooney | melwitt: for the CVE fix we have to do a locksetp backport in neutron and nova. | |
| 17:26:28 | sean-k-mooney | its a publci CVE by the way | |
| 17:26:50 | sean-k-mooney | for the other case of cold migrating/resizign between network backends | |
| 17:27:01 | sean-k-mooney | its technically not something tha was offically supported | |
| 17:27:11 | sean-k-mooney | which i noted in the bug | |
| 17:27:18 | sean-k-mooney | but it works in a limited set of cases | |
| 17:27:29 | melwitt | yeah I guess I'm not immediately seeing a backportability issue (but I could be missing something) but I think we'd want to let bake on master for awhile before backporting everywhere, to catch bugs before it's propagated to more branches | |
| 17:27:31 | sean-k-mooney | e.g. ml2/ovs to ml2/ovn | |
| 17:27:59 | sean-k-mooney | melwitt: there is one edge case im not sure how to adress | |
| 17:28:20 | melwitt | maybe it's just me but the fix sounds complicated and something we'd want to see if problems appear once it's running throughout the gate | |
| 17:29:03 | sean-k-mooney | well the first fix is just one addtion fucntion all in resize_instance to unplug_vifs | |
| 17:29:27 | melwitt | is that consistent with what we do with volumes today too? | |
| 17:29:34 | sean-k-mooney | bit that makes it trickin is to fully solve the diffent backend case i need to remove the current call in confirm | |
| 17:29:56 | sean-k-mooney | melwitt: ya for volumes we remove attacments in resize_instance | |
| 17:30:32 | melwitt | ok. it seems like it would make sense to have them be consistent but I don't know the reason vifs are handled differently | |
| 17:30:55 | sean-k-mooney | well technically in migrate_disk_and_poweroff | |
| 17:30:56 | sean-k-mooney | https://github.com/openstack/nova/blob/e0f088c95d05e9cf32d4af4c7cfc20566b17f8e1/nova/virt/libvirt/driver.py#L10362 | |
| 17:31:22 | melwitt | yeah, I did see the volume detach there | |
| 17:31:22 | sean-k-mooney | and im suggesting adding self.unplug_vifs here https://github.com/openstack/nova/blob/e0f088c95d05e9cf32d4af4c7cfc20566b17f8e1/nova/virt/libvirt/driver.py#L10357 | |
| 17:31:38 | sean-k-mooney | basicly 5 lines up | |
| 17:32:30 | melwitt | yeah, seems to make sense to me | |
| 17:32:48 | sean-k-mooney | melwitt: basically power_off call _destory() not destroy() to delete the domain xml without remving the disk images | |
| 17:32:57 | sean-k-mooney | so we can revert without copying data | |
| 17:33:08 | sean-k-mooney | but _destory() does not call unplug_vifs | |
| 17:33:25 | sean-k-mooney | destroy() does via the call to cleanup() | |
| 17:33:30 | sean-k-mooney | which delete the disk too | |
| 17:33:43 | melwitt | right | |
| 17:34:22 | sean-k-mooney | and today conrim does an rpc from the dest to the source to call unplug_vifs | |
| 17:34:50 | sean-k-mooney | here in cleanup_resize https://github.com/openstack/nova/blob/e0f088c95d05e9cf32d4af4c7cfc20566b17f8e1/nova/virt/libvirt/driver.py#L1665 | |
| 17:35:43 | melwitt | yeah I guess I wonder what are the potential issues if the deployment is in the middle of an upgrade | |
| 17:35:50 | sean-k-mooney | for revert we call plug_vifs in create_instace_with_networks here https://github.com/openstack/nova/blob/e0f088c95d05e9cf32d4af4c7cfc20566b17f8e1/nova/virt/libvirt/driver.py#L6704 https://github.com/openstack/nova/blob/e0f088c95d05e9cf32d4af4c7cfc20566b17f8e1/nova/virt/libvirt/driver.py#L10681-L10683 | |
| 17:36:09 | openstackgerrit | Lee Yarwood proposed openstack/nova-specs master: WIP/DNM - Flavor and image defined ephemeral storage encryption https://review.opendev.org/752284 | |
| 17:36:15 | sean-k-mooney | melwitt: right so if i dont remove the unplug_vifs form the cleanup in confim | |
| 17:36:19 | sean-k-mooney | i think its ok if i do | |
| 17:36:26 | lyarwood | sean-k-mooney: sorry just pushing something and I need to drop again, can we catch up about that backport stuff in the morning? | |
| 17:36:42 | lyarwood | s/morning/your\ morning/g ;) | |
| 17:36:47 | sean-k-mooney | then if i have an old souce and a new dest then i think we dont clean up the vifs | |
| 17:37:00 | sean-k-mooney | lyarwood: ya we can no rush | |
| 17:37:19 | sean-k-mooney | melwitt: im not sure how to handel that other then leave the extra call to unplug_vifs in the backport | |
| 17:37:47 | sean-k-mooney | melwitt: unplug_vifs shoudl be callable twice without any ill effects | |
| 17:38:36 | melwitt | yeah, well wouldn't you have to leave it on master (not only backports) for one cycle? to bridge the N-1 to N upgrade? | |
| 17:39:10 | sean-k-mooney | melwitt: well the proablem is i need to backport the cve issue to like queens eventually | |
| 17:39:43 | sean-k-mooney | what i think i would have to do is keep the call to unplug_vifs in cleanup but wrap it in a try excetp | |
| 17:39:59 | melwitt | I'm just saying re: leaving the extra call, I think you'd have to leave it on master too, not only leave it on the backports. because the upgrade issue exists for master as well | |
| 17:40:08 | sean-k-mooney | oh right | |
| 17:40:17 | sean-k-mooney | i woudl have to leave it on master for a cycle ya | |
| 17:40:46 | sean-k-mooney | ill put the patches up for peopel to see as 3 patches | |
| 17:41:10 | sean-k-mooney | 1 for the new call in resize. one for the cve fix which is alredy up for review and one for the extra call removal | |
| 17:41:28 | sean-k-mooney | and we can defer that last one for a cycle | |
| 17:42:04 | sean-k-mooney | and i guess we can see if peopel think of any other reason not to do it this way. | |
| 17:42:10 | melwitt | yeah, so AFAICT the backport concern here would be the complexity and impact to something that is supposed to be "stable" but it's needed for a CVE so I think that makes it backportable (again unless I'm missing something) | |
| 17:43:03 | sean-k-mooney | ya if we think its backportable and we are happy with it on master i guess i dont have to rush the backport either | |
| 17:43:17 | sean-k-mooney | e.g. we coudl wait a while and let it back | |
| 17:43:20 | sean-k-mooney | *bake | |
| 17:44:24 | melwitt | yeah I'm just concerned about there being a bug or edge case that crops up in the gate and then we have a fix to backport 5 times too | |
| 17:44:51 | melwitt | not the end of the world, we did it recently :P but the goal is to avoid that | |
| 17:45:10 | sean-k-mooney | :) ya im ok with that | |
| 17:45:26 | sean-k-mooney | this partcalar issue has been dragin on since 2018 or longer | |
| 17:45:27 | melwitt | let's see what other ppl think. I worry I'm missing something unbackportable about it | |
| 17:45:46 | sean-k-mooney | 2017-11-24 | |
| 17:46:03 | melwitt | yeah, I think that might be the same neutron security bug I emailed about downstream a long time ago | |
| 17:46:22 | sean-k-mooney | i started working on this when i was still at intel | |
| 17:46:31 | sean-k-mooney | we have fixed in in a number of edgecases | |
| 17:46:41 | sean-k-mooney | but there is still 1 that is unfixed | |
| 17:46:51 | sean-k-mooney | just trying to get the fine case closed | |
| 17:47:23 | melwitt | understood | |
| 17:47:52 | sean-k-mooney | https://review.opendev.org/#/c/602432/ fixes the final case but its failing or reverts because of the change in unplug behaivor wehn we get rid of the libvirt race | |
| 17:59:03 | sean-k-mooney | melwitt: oh one related question do we think this could be an RC1 candiate so that we could count victoria as the first release or would wallaby have to be the first release with X for the removal of the second call | |