| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-10-07 | |||
| 08:27:01 | bauzas | this sounds more reasonable | |
| 08:27:08 | bauzas | as we don't really have an agenda | |
| 08:27:40 | bauzas | ok, anything to add before I close ? | |
| 08:29:09 | gibi | - | |
| 08:32:13 | bauzas | #endmeeting | |
| 08:32:13 | opendevmeet | Meeting ended Thu Oct 7 08:32:13 2021 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) | |
| 08:32:13 | opendevmeet | Minutes: https://meetings.opendev.org/meetings/nova_extra/2021/nova_extra.2021-10-07-08.04.html | |
| 08:32:13 | opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/nova_extra/2021/nova_extra.2021-10-07-08.04.txt | |
| 08:32:13 | opendevmeet | Log: https://meetings.opendev.org/meetings/nova_extra/2021/nova_extra.2021-10-07-08.04.log.html | |
| 08:39:58 | opendevreview | Jinsheng Zhang proposed openstack/nova master: Nova support live-migrate instance between different ipxe efi rom https://review.opendev.org/c/openstack/nova/+/812796 | |
| 08:50:33 | lyarwood | gibi: https://bugs.launchpad.net/nova/+bug/1946339 you might be interested in this | |
| 08:54:47 | gibi | looking | |
| 08:56:00 | gibi | lyarwood: thanks,yes, that is one of the recently added test case | |
| 08:56:25 | gibi | lyarwood: is it high frequency? | |
| 09:06:22 | opendevreview | Dmitriy Rabotyagov proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947 | |
| 09:17:26 | lyarwood | gibi: sorry not looked at the frequency, just saw one hit and raised the bug | |
| 09:17:33 | gibi | lyarwood: ack, I will check it | |
| 09:18:14 | gibi | I need to juggle multiple things right now downstream so it will take a bit of time to get to it | |
| 09:20:45 | lyarwood | ack np | |
| 09:42:15 | stephenfin | now that we've forked, any chance we could get these in nice and early? https://review.opendev.org/c/openstack/nova/+/810291 | |
| 09:44:31 | gibi | stephenfin: I want them, I just need to find the time to review them :/ | |
| 09:45:43 | gibi | (there is an ongoing discussion to upgrade a mitaka cloud to victoria downstream in a single step and that discussion is killing meg right now) | |
| 09:46:34 | opendevreview | Lee Yarwood proposed openstack/nova master: compute: Update volume_id within connection_info during swap_volume https://review.opendev.org/c/openstack/nova/+/807025 | |
| 09:46:34 | opendevreview | Lee Yarwood proposed openstack/nova master: fup: Move _wait_for_volume_{attach,detach} to os-volume_attachments https://review.opendev.org/c/openstack/nova/+/810775 | |
| 09:46:35 | opendevreview | Lee Yarwood proposed openstack/nova master: fup: Refactor and simplify Cinder fixture GET volume mock https://review.opendev.org/c/openstack/nova/+/810776 | |
| 10:11:56 | opendevreview | Dmitriy Rabotyagov proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947 | |
| 10:22:26 | opendevreview | Lee Yarwood proposed openstack/nova master: zuul: Move live migration jobs back to voting https://review.opendev.org/c/openstack/nova/+/812392 | |
| 10:22:26 | opendevreview | Lee Yarwood proposed openstack/nova master: Revert "zuul: Skip block migration with attached volumes tests due to bug #1931702" https://review.opendev.org/c/openstack/nova/+/812473 | |
| 10:22:40 | lyarwood | ^ also needs the stable/xena backport of my devstack fix to allow grenade to work /o\ | |
| 10:42:11 | opendevreview | sean mooney proposed openstack/nova master: Add autopep8 to tox and pre-commit https://review.opendev.org/c/openstack/nova/+/806182 | |
| 10:42:49 | sean-k-mooney | i hope people will be open to ^ | |
| 10:57:29 | sean-k-mooney | well this is new "flake8.exceptions.PluginRequestedUnknownParameters: "pycodestyle" requested unknown parameters causing 'FileProcessor' object has no attribute 'indent_size'" | |
| 10:59:10 | sean-k-mooney | ah ok that with my patch im not capping the deps when installing autopep8 | |
| 11:01:03 | sean-k-mooney | hum no thats not the issue | |
| 11:17:12 | noonedeadpunk | sean-k-mooney: hey! regarding https://review.opendev.org/c/openstack/nova/+/811947 - setting lower would require adjusting a lot of existing unit tests because all of them have assumption about mac being in upper case | |
| 11:17:28 | noonedeadpunk | Having that in mind - should I still convert it to lower()? | |
| 11:18:22 | noonedeadpunk | Because what I didn't want to do - to change tests to fit my change :) | |
| 11:18:41 | noonedeadpunk | But if you think it's fine - no problems | |
| 11:20:59 | sean-k-mooney | i kind of dont like sprinkeling random upper and lower cases converstion in the code in general and would prefer to have normalise utility function that is used consitently | |
| 11:21:26 | sean-k-mooney | i would prefer to have it normalise to lower case but i think your bug is just a sincel case of thsi | |
| 11:21:39 | sean-k-mooney | i think there could be other places that woudl still break | |
| 11:24:21 | sean-k-mooney | noonedeadpunk: with that said i also think this is a neuton bug | |
| 11:24:31 | sean-k-mooney | i think neutron should be normalising this internally themselves | |
| 11:26:56 | noonedeadpunk | Well... Maybe it should, but, we take xml from libvirt and from neutron api. So these are 2 different sources of truth nova takes info from. And current assumption that they shoudl be equal | |
| 11:27:15 | sean-k-mooney | correct | |
| 11:27:24 | sean-k-mooney | a mac is justa 48bit int | |
| 11:27:25 | noonedeadpunk | Also, that patch is pretty much simple and backportable | |
| 11:27:40 | sean-k-mooney | its not a strirng so we shoudl not be doing a case sensitive comparison | |
| 11:27:44 | noonedeadpunk | While fixing neutron will require db migrations | |
| 11:28:15 | sean-k-mooney | that or a conversion on load and normalisation in store | |
| 11:28:35 | sean-k-mooney | but ya we can normalise in nova but i think neutron should also fix this themseleves | |
| 11:29:22 | noonedeadpunk | I don't disagree here :) | |
| 11:30:49 | sean-k-mooney | at the end of the day it does not really matter which way we go but lower case is my prefernce we can see what other think in general your patch looks fine | |
| 11:31:28 | noonedeadpunk | yeah, it was mine as well:) but I'd need to fix all other unit tests if change that to lower case | |
| 11:31:34 | sean-k-mooney | this is really an internal fix in the nova side so i dont think you need a release note but that is the only other thing i was thinking of | |
| 11:31:54 | sean-k-mooney | noonedeadpunk: yep i done see any issue with fixing the unit tests other then time | |
| 11:32:14 | sean-k-mooney | i would guess there is a mix of case already | |
| 11:32:58 | sean-k-mooney | yep https://github.com/openstack/nova/blob/8d9785b965657d42f20e1ad7234f570077a387d7/nova/tests/unit/virt/libvirt/test_vif.py#L81 | |
| 11:33:20 | noonedeadpunk | well, I was also unsure if I haven't break anything else accidentally:) so with current change log I was more sure about zero impact on current functionality | |
| 11:33:22 | sean-k-mooney | im prettty sure i have always used lower case in our tests | |
| 11:34:25 | noonedeadpunk | ah, well, so it's more test_migration thing I guess | |
| 11:36:17 | sean-k-mooney | what im more concerneed about is https://github.com/openstack/nova/blob/66574018b517f14dc26e581d0ddaa7788806f83e/nova/objects/virtual_interface.py#L44 | |
| 11:36:29 | sean-k-mooney | and other place we store the adress in our code | |
| 11:36:53 | sean-k-mooney | i had alway assumed that neutron was already normalising | |
| 11:39:30 | sean-k-mooney | noonedeadpunk: when we lookup pci device mac adress we do not normalise there currently either | |
| 11:39:32 | sean-k-mooney | https://github.com/openstack/nova/blob/66574018b517f14dc26e581d0ddaa7788806f83e/nova/pci/utils.py#L173-L191 | |
| 11:39:45 | sean-k-mooney | we just take the value form /sys which should be lower case | |
| 11:40:26 | sean-k-mooney | sean@p50:~/repos/nova$ cat /sys/class/net/enp0s31f6/address | |
| 11:40:28 | sean-k-mooney | 8c:16:45:81:48:b5 | |
| 11:41:26 | sean-k-mooney | for the unique constraint in the db for the virtual interfaces table we are also useing the mac un normalised | |
| 11:41:28 | sean-k-mooney | https://github.com/openstack/nova/blob/66574018b517f14dc26e581d0ddaa7788806f83e/nova/network/neutron.py#L1307-L1308 | |
| 11:42:26 | sean-k-mooney | noonedeadpunk: likely we should just fix your inital case in this patch but we should think about the other cases at some point | |
| 11:43:17 | noonedeadpunk | well neutron is not normalazing https://paste.opendev.org/show/809839/ | |
| 11:43:58 | sean-k-mooney | yep that would appear to be the case | |
| 11:44:40 | sean-k-mooney | whcih mean sif you were ever to change the mac on the neutron port it could break things in nova unless we propagate that update in some cases | |
| 11:44:53 | noonedeadpunk | actually I thought that in domain xml mac is also stored non-normalized | |
| 11:45:13 | noonedeadpunk | And I saw that issue first when was live-migrating instances from bionic to focal computes | |
| 11:45:37 | sean-k-mooney | libvirt normalises the xml we provide so the one we provide is not the one that it actully uses | |
| 11:45:47 | sean-k-mooney | so we likely dont normalise it when we give it to libvirt | |
| 11:45:55 | noonedeadpunk | was it always like that ? | |
| 11:46:01 | sean-k-mooney | yep | |
| 11:46:24 | noonedeadpunk | huh, okay then. Wondering why it's haven't been spotted before then... | |
| 11:46:25 | sean-k-mooney | the xml you see with virsh dumpxml is not the one that nova provided | |
| 11:46:37 | sean-k-mooney | well neutron default to lower case | |
| 11:46:39 | noonedeadpunk | Yeah, that's what I saw as well | |
| 11:46:49 | sean-k-mooney | so unless you provdided a mac it wont be an issue | |
| 11:47:15 | noonedeadpunk | yeah, maybe we saw that because we were doing really mass migrations of all vms in our deployments... | |
| 11:47:16 | sean-k-mooney | so i think this is only an issue for user provided macs | |
| 11:47:36 | noonedeadpunk | yeah, indeed | |
| 11:47:46 | noonedeadpunk | Ok, so then I will change that to lower() | |
| 11:48:54 | sean-k-mooney | if the test fallout of that is no horrific then yes please do | |
| 11:49:29 | sean-k-mooney | i suspect it will not be that bad | |
| 12:22:06 | opendevreview | Dmitriy Rabotyagov proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947 | |
| 12:22:18 | noonedeadpunk | yeah you was right, it's not bad at all) | |
| 13:48:01 | opendevreview | Dmitriy Rabotyagov proposed openstack/nova master: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/811947 | |
| 15:08:58 | bauzas | gibi: sean-k-mooney: shit, I remember we need to move the merged specs into implemented https://specs.openstack.org/openstack/nova-specs/specs/xena/ | |
| 15:09:18 | gibi | bauzas: yeah, we tend to forget that | |
| 15:09:31 | bauzas | gibi: I can do it | |
| 15:09:38 | gibi | there is script for that | |
| 15:09:41 | gibi | thans | |