| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-11-15 | |||
| 10:59:57 | kashyap | Yea; thx | |
| 11:02:03 | gibi | ack, I will be on the meeting | |
| 11:17:59 | elodilles | bauzas: (sorry for the late reply) thanks, i've created the python-novaclient release: https://review.opendev.org/c/openstack/releases/+/817930 | |
| 11:18:29 | elodilles | and rebased the ussuri-em patch on top of this | |
| 11:43:03 | opendevreview | Takashi Kajinami proposed openstack/nova master: Deprecate [wsgi] secure_proxy_ssl_header https://review.opendev.org/c/openstack/nova/+/817927 | |
| 12:22:32 | sean-k-mooney | gibi: so technially the guest abi is mostly maintained when goign form cirrus to virtio as virtio has a vga compatiable interface that is compatiable with cirrus. it would be a guest visabel change | |
| 12:22:51 | sean-k-mooney | it apparently is not a harmful guest visable change | |
| 12:25:03 | sean-k-mooney | if we are ok with the change in default affectign existing instnace the kasahyap patch is workable if not then i would like him to adress the comments i left in the patch | |
| 12:27:28 | kashyap | Yep, this is a benign case. | |
| 12:27:52 | kashyap | sean-k-mooney[m]: What else comments? I addressed what you (incorrectly) thought was a problem | |
| 12:27:58 | kashyap | I don't see any outstanding questions there. | |
| 12:28:07 | kashyap | Also, recall: | |
| 12:28:27 | sean-k-mooney | you ignored all the other comment i made and you have not | |
| 12:28:43 | kashyap | In the patch comments, I've also outlined the integration tests (Windows + Linux) done by Red Hat QE as well | |
| 12:28:50 | sean-k-mooney | what i sated as a probelm is that the xml would change for existign instace after a hard reboot | |
| 12:29:14 | sean-k-mooney | it still will for vms create before the patch | |
| 12:29:30 | kashyap | I need to evaluate the merits of the other comments and see if they make sense. | |
| 12:29:39 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/798680/3/nova/virt/libvirt/driver.py | |
| 12:29:39 | kashyap | Some of them maybe non-problems | |
| 12:31:12 | sean-k-mooney | kashyap: the disconenct is that when we discussed this with dan and other before we said you should follow the patteren we used for machine type and recorerd the current video model for all instnace and just change the behavior for new ones | |
| 12:31:35 | sean-k-mooney | now you have shown that changing the behaivor for new isntnace should be safe | |
| 12:31:53 | sean-k-mooney | that is great it does not change the fact that we had previously agreed not too | |
| 12:31:55 | kashyap | Correct | |
| 12:31:59 | kashyap | Well | |
| 12:32:14 | kashyap | We should revisit previous "agreements" if they don't make sense in light of new info | |
| 12:32:16 | sean-k-mooney | if people are ok with makeing the change we can but we should at least disucss that | |
| 12:32:44 | kashyap | Yes, but I hope it won't become a big drag. | |
| 12:35:35 | gibi | kashyap, sean-k-mooney: OK, so the cirros -> virtio change is considered harmless during a hard reboot, then I'm OK with the direction of the patch | |
| 12:35:58 | kashyap | Cool; yes. And that is proved by the tests. Thanks. | |
| 12:44:22 | kashyap | sean-k-mooney: Wait: what is still not correct in the broader direction? On hard-reboot, we recreate the XML, and yes, it *will* change from CirrOS to virtio - the QEMU folks confirmed that harmless ABI change is okay | |
| 12:44:33 | kashyap | sean-k-mooney: So what is still "not correct in this patch"? | |
| 12:44:38 | kashyap | Please keep the explanation short. | |
| 12:50:30 | opendevreview | Dmitrii Shcherbakov proposed openstack/nova-specs master: Integration With Off-path Network Backends https://review.opendev.org/c/openstack/nova-specs/+/787458 | |
| 12:54:00 | sean-k-mooney | kashyap: gibi has just now indicated he is ok with the change in direction. other have not bar perhaps stephen. the patch as written assumed that change in direction would happen without disucssing it | |
| 12:54:53 | kashyap | sean-k-mooney: Okay, for that part - I'll explicitly state it in the commit message; and as a comment somewhere. | |
| 12:54:55 | sean-k-mooney | the other comment i left tell you how to modify the patch to confrom to what we had previosuly agreed. e.g. record the current used video model | |
| 12:55:55 | kashyap | sean-k-mooney: What is the use-case for still recording the used video model? | |
| 12:56:25 | kashyap | When we know that changing it even for existing instances on hard-reboot is benign and harmless? | |
| 12:56:32 | sean-k-mooney | consitency with the change fo default of machine type | |
| 12:57:28 | kashyap | Sorry, that's not a valid use-case. (While I agree in general to be consistent where it makes sense) | |
| 12:57:33 | sean-k-mooney | in this case you are instead following what we did for the rng which is ok | |
| 12:58:03 | kashyap | We should not add needless extra code for "consistency" if it is not solving any problem or improving maintainability | |
| 12:58:19 | kashyap | Yep; like for RNG indeed | |
| 13:03:55 | dmitriis | agree about the need to extend has_bind_time_event (which I added to the spec) but, from what I can see, this won't help with the actual problem of the virt driver code trying to wait for the `network-vif-plugged` event that gets discarded by Nova in the absence of a waiter. | |
| 13:03:55 | dmitriis | sean-k-mooney, gibi: updated the spec https://review.opendev.org/c/openstack/nova-specs/+/787458. This comment may need further discussion https://review.opendev.org/c/openstack/nova-specs/+/787458/comment/4337b974_a214c6f9/ during the implementation (I added code references and the actual log to give an impression of what I am talking about). I | |
| 13:05:50 | sean-k-mooney | dmitriis: the wait in the virt driver shoudl be filtered by the bindtyime/plugtime event fucntions | |
| 13:09:22 | sean-k-mooney | dmitriis: if we know that a backend only sends bind time event reliably its oke to ignore events sent at other times | |
| 13:10:49 | dmitriis | sean-k-mooney: so you are proposing that _get_neutron_events https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L7191-L7198 calls get_bind_time_events https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/network/model.py#L567-L572 for filtering, right? | |
| 13:10:52 | sean-k-mooney | dmitriis: if you modify has_bind_time_event to account for the new vnic type | |
| 13:11:23 | sean-k-mooney | https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L7221-L7222 shoudl return an empty [] | |
| 13:11:26 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: libvirt: Introduce config class for QEMU's "tb-cache" https://review.opendev.org/c/openstack/nova/+/816823 | |
| 13:12:03 | opendevreview | Takashi Kajinami proposed openstack/nova master: Deprecate [wsgi] secure_proxy_ssl_header https://review.opendev.org/c/openstack/nova/+/817927 | |
| 13:12:17 | sean-k-mooney | dmitriis: yes more or less | |
| 13:12:31 | opendevreview | Takashi Kajinami proposed openstack/nova master: Deprecate [wsgi] secure_proxy_ssl_header https://review.opendev.org/c/openstack/nova/+/817927 | |
| 13:12:33 | sean-k-mooney | i belive that shoudl eb using the plugtime event funciton | |
| 13:12:51 | sean-k-mooney | dmitriis: since we have already passed binding the port | |
| 13:13:02 | sean-k-mooney | dmitriis: currently it is https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L7191-L7198 | |
| 13:15:01 | sean-k-mooney | dmitriis: effectvly it s implementaiton shoudl be like this https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/network/model.py#L581-L586 | |
| 13:15:09 | sean-k-mooney | i suspect that is a latent bug actully | |
| 13:16:06 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Remove unnecessary warning filters https://review.opendev.org/c/openstack/nova/+/817748 | |
| 13:16:06 | opendevreview | Stephen Finucane proposed openstack/nova master: db: Remove use of 'bind' arguments https://review.opendev.org/c/openstack/nova/+/817747 | |
| 13:17:08 | sean-k-mooney | actully dmitriis :) this might be the cause of our revert resize issue | |
| 13:17:09 | dmitriis | sean-k-mooney: the VIF object itself has has_bind_time_event actually https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/network/model.py#L488-L497 . So I can extend it to accept `None` for the migration object and add handling for VNIC_TYPE since that's available in the VIF itself. | |
| 13:17:35 | dmitriis | sean-k-mooney: oh well, good thing we looked at this code then :^) | |
| 13:18:58 | sean-k-mooney | yes i would extend the vif class | |
| 13:20:32 | sean-k-mooney | artom has a repoducer patch up for the resize bug i think | |
| 13:20:53 | sean-k-mooney | so we might fix that issue for you in a seperate patch so we can backport it | |
| 13:22:30 | sean-k-mooney | this is where we repoduced our downstream bug https://review.opendev.org/c/openstack/nova/+/817303 | |
| 13:23:14 | sean-k-mooney | the revert resize test fail in a very specific situation whic i think is basially the same thing you are seeing | |
| 13:23:16 | sean-k-mooney | https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_f60/817303/2/check/nova-ovs-hybrid-plug/f60d54c/testr_results.html | |
| 13:24:51 | dmitriis | sean-k-mooney: ok, I'll fix it in my WIP spec implementation proposal https://review.opendev.org/c/openstack/nova/+/812111 and try it out on the actual test environment. We can have a separate patch to fix the issue and that will leave my patch with an extension to has_bind_time_event to handle VNIC_TYPE_SMARTNIC. | |
| 13:25:15 | sean-k-mooney | yep that makes sense to me | |
| 13:29:52 | sean-k-mooney | hum actully for the revert case we pass in the filtered events list | |
| 13:29:54 | sean-k-mooney | https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L11204-L11210 | |
| 13:30:07 | opendevreview | Kashyap Chamarthy proposed openstack/nova master: Deprecate support for floppy drives https://review.opendev.org/c/openstack/nova/+/802022 | |
| 13:30:08 | sean-k-mooney | dmitriis: but this is still broken for your usecase | |
| 13:30:13 | sean-k-mooney | so we should still fix it | |
| 13:31:10 | dmitriis | sean-k-mooney: yes, it looks like the "active" state check might be there for that case but it doesn't help in my case | |
| 13:31:47 | sean-k-mooney | the active state chagne on the neutron port? | |
| 13:32:39 | dmitriis | sean-k-mooney: yes, on this line https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L7198 | |
| 13:32:56 | dmitriis | this state gets updated via the info cache from what I understand | |
| 13:33:15 | sean-k-mooney | not quite | |
| 13:33:35 | sean-k-mooney | it will but that is not related | |
| 13:33:40 | sean-k-mooney | this is the admin state | |
| 13:33:56 | dmitriis | I see | |
| 13:34:03 | sean-k-mooney | we skip port that have been marked as down | |
| 13:34:52 | dmitriis | sean-k-mooney: is it OK if I extend has_bind_time_event to accept `None` for the `migration` argument? https://github.com/openstack/nova/blob/0e0196d979cf1b8e63b9656358116a36f1f09ede/nova/network/model.py#L488-L497 | |
| 13:35:37 | dmitriis | I don't have it in _get_neutron_events https://github.com/openstack/nova/blob/e28afc564700a1a35e3bf0269687d5734251b88a/nova/virt/libvirt/driver.py#L7191-L7198 and I'd rather not try to instantiate one there | |
| 13:35:40 | sean-k-mooney | i think so i have been trying to figure out what that should look like and or if you should jsut replace migration with same_host=True/False | |
| 13:36:20 | kashyap | sean-k-mooney: Hmm, this page should also list "bochs" as an option, right? - https://docs.openstack.org/glance/xena/admin/useful-image-properties.html | |
| 13:36:37 | kashyap | I think I forgot to updte the docs for that :-( I'm going to do that now | |
| 13:36:49 | sean-k-mooney | yes eventually and you should update the glance metadefs if you have not already done so | |
| 13:36:51 | kashyap | For hw_video_model, I mean | |
| 13:37:24 | sean-k-mooney | https://github.com/openstack/glance/blob/master/etc/metadefs/compute-libvirt-image.json#L71-L84 | |
| 13:37:44 | sean-k-mooney | that used to generate the horizon and heat dropdown menues | |
| 13:38:16 | sean-k-mooney | you should be able to do both in the same patch | |
| 13:39:22 | kashyap | Right; I updated this file: etc/metadefs/compute-libvirt-image.json and also this file: doc/source/admin/useful-image-properties.rst | |
| 13:39:44 | dmitriis | sean-k-mooney: yes, making `has_bind_time_event` accept a boolean seems cleaner than passing None. There aren't any other migration attributes used so far in it. | |
| 13:40:43 | kashyap | sean-k-mooney: I wonder if I should add a belated release note to the Glance docs? | |
| 13:41:31 | dmitriis | sean-k-mooney: I've put generic wording into the spec for now https://review.opendev.org/c/openstack/nova-specs/+/787458/12/specs/yoga/approved/integration-with-off-path-network-backends.rst#343 I think it encompasses whatever we decide implementation-wise. | |