Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-10-07
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
15:09:44 gibi thanks
16:21:16 lyarwood bauzas: https://review.opendev.org/c/x/gerrit-dash-creator/+/813050
16:25:14 bauzas gibi: oh, I wasn't knowing
16:27:56 melwitt bauzas: there's some instructions here too https://docs.openstack.org/nova/latest/contributor/ptl-guide.html
16:28:47 bauzas melwitt: /me facepalms
16:29:03 lyarwood https://review.opendev.org/c/openstack/nova/+/812392 - bauzas / gibi / melwitt ; this should be good now the stable/xena devstack change is in the gate
16:29:04 bauzas I have this link open as a tab since a month and I forgot about this
16:29:44 melwitt :)
16:44:59 opendevreview Sylvain Bauza proposed openstack/nova-specs master: Move Xena implemented specs https://review.opendev.org/c/openstack/nova-specs/+/813051
16:45:10 bauzas gibi: melwitt: there there ^
16:53:55 melwitt bauzas: I just remembered you'll want to do the placement repo too. have to do it manually afaik, placement doesn't use launchpad https://docs.openstack.org/placement/latest/specs/index.html#xena they are located under placement/doc/source/specs/
16:56:31 melwitt gmann: in case you didn't see my comment on https://review.opendev.org/c/openstack/devstack/+/812092 I think stable/train is also broken in the same way cc elodilles
16:57:54 gmann melwitt: checking, stable/train still run tempest master so it should be ok.
16:59:05 melwitt gmann: oh, so my patch has jsonschema fail bc of the stable/stein side of grenade /facepalm ok I understand now
16:59:58 gmann melwitt: yeah, once stable/stein patch is merged then we can recheck or, we can make depends-on just to be sure
17:00:44 melwitt yeah I see. I'm writing up a comment about the novnc issue. I don't understand why it's happening though so can't fix it yet :(
17:01:44 gmann thanks, I will try to dig into that next week or so.
17:03:31 elodilles melwitt: thanks for the heads up, i will also add this to my TODO list and try to debug it :S
17:04:35 melwitt elodilles: to correct myself it's stable/stein that needs a fix but it's failing for a novnc issue that I don't understand how is happening. I'm writing a comment for gmann's patch ^ with what I found so far
17:13:28 elodilles melwitt: ack, thanks!
18:54:17 melwitt gmann: just a fyi, I think I finally figured out why the tempest 20.0.0 patch failed on stable/stein https://review.opendev.org/c/openstack/devstack/+/812092/1#message-348d913f2bbcf909533d62c3cb4e02d6a0100664 cc elodilles
22:21:40 gmann melwitt: ah thanks, let me try 24.0.0 which should work as it has old jsonschema in requirement filr https://github.com/openstack/tempest/blob/24.0.0/requirements.txt#L6
22:22:04 gmann oh, you already updated, great
22:22:30 melwitt gmann: I already updated your patch, I hope you don't mind. I did not update the commit message or code comment yet as it's just for testing. if it works well, we can update the writings
22:23:09 gmann melwitt: +1.
22:27:55 opendevreview Merged openstack/nova master: zuul: Move live migration jobs back to voting https://review.opendev.org/c/openstack/nova/+/812392

Earlier   Later