| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-01-05 | |||
| 14:10:17 | sean-k-mooney | stephenfin: do you mean https://bugzilla.redhat.com/show_bug.cgi?id=1911710 | |
| 14:10:18 | openstack | bugzilla.redhat.com bug 1911710 in openstack-nova "[osp 13][neutron] cold migration fails for sriov instance with: Port update failed for port <uuid>c: Unable to correlate PCI slot" [Urgent,New] - Assigned to nova-maint | |
| 14:10:28 | stephenfin | yup | |
| 14:10:32 | sean-k-mooney | cause that is not unshelve | |
| 14:10:44 | sean-k-mooney | different code path | |
| 14:10:58 | gibi | sean-k-mooney: yeah :) | |
| 14:12:00 | sean-k-mooney | the error is raised form here https://github.com/openstack/nova/blob/34c5df7b2b8c7eebbfc8aa0bc3edba501dddcb61/nova/network/neutron.py#L3396-L3398 | |
| 14:12:29 | sean-k-mooney | and it only happens if we have not claimed the pci device we are trying to update the neutron port with if i recall correctly | |
| 14:12:41 | stephenfin | My thinking was that there had been an unshelve on the host before that was using an old record, and the cold migrated instance was trying to use the same record | |
| 14:13:01 | sean-k-mooney | no that gives you a differnet error | |
| 14:13:06 | stephenfin | I'm trying to write a functional test to prove out that theory but have got stuck in the weeds | |
| 14:13:46 | sean-k-mooney | you get someting like libvirtError: Requested operation is not valid: PCI device 0000:5d:17.4 is in use by driver QEMU, domain instance-00003be0 | |
| 14:13:55 | sean-k-mooney | in that case e.g. a libvirt error | |
| 14:14:28 | stephenfin | Ah | |
| 14:16:10 | sean-k-mooney | im donwloadign the customer logs to take a quick look but i would suspect that the device its trying to use is unclaimed | |
| 14:16:46 | stephenfin | hmm, okay if you're looking at that I'll got back to the os-hypervisors work | |
| 14:17:50 | sean-k-mooney | im only going to look at it breifly but want to clarify a few things as peopel have mentioned rebuilds in it so i want to confirm what operation is actully happening | |
| 14:18:03 | sean-k-mooney | ill let you know if i find anything | |
| 14:18:56 | stephenfin | On that note, gibi: currently each entry in the 'hypervisors' field of the '/os-hypervisors/detail' response is identical to what '/os-hypervisors/{hypervisor}' returns. If I'm adding 'uptime' to the latter, I assume I should add it to the former also? | |
| 14:19:39 | stephenfin | as an aside, the whole '/os-hypervisors/detail' API is a bit weird and fairly "non-REST'ful" | |
| 14:19:57 | sean-k-mooney | /os-hypervisors/detail is the list endpoint | |
| 14:20:03 | sean-k-mooney | right | |
| 14:20:12 | sean-k-mooney | so i would expect it to be the same | |
| 14:20:18 | sean-k-mooney | well the detailed list endpoint | |
| 14:20:26 | sean-k-mooney | vs just /os-hypervisors | |
| 14:20:31 | stephenfin | Yeah, it's a variant of the list endpoint but with additional detail in each resource | |
| 14:20:38 | stephenfin | I'd expect it to be the same too | |
| 14:21:19 | sean-k-mooney | for me you are reall adding the uptime to the os-hyperviors resource | |
| 14:21:34 | sean-k-mooney | rather then just that specific endpoint | |
| 14:21:44 | sean-k-mooney | so showing it in detail makes sense | |
| 14:22:09 | sean-k-mooney | where as previously it was on the stats resouce | |
| 14:23:50 | stephenfin | makes sense | |
| 14:25:34 | sean-k-mooney | it is a migration by the way | |
| 14:25:40 | sean-k-mooney | not a rebuild | |
| 14:29:37 | bauzas | gibi: see https://review.opendev.org/c/openstack/nova/+/729563/26/nova/api/openstack/compute/shelve.py#59 | |
| 14:29:45 | bauzas | unfortunately, ship has sailed for a while | |
| 14:30:11 | bauzas | stephenfin: ^ look too | |
| 14:30:37 | bauzas | AFAIK, we were not blocking flavors using this key before | |
| 14:31:49 | bauzas | stephenfin: what's the correct behaviour when I ask to create a new instance with an unknown key ? | |
| 14:31:58 | bauzas | like 'sylvainb:nice' | |
| 14:32:16 | sean-k-mooney | stephenfin: http://paste.openstack.org/show/801414/ | |
| 14:32:20 | bauzas | I guess this would silently be accepted, right? | |
| 14:33:25 | sean-k-mooney | bauzas: you mean a custom extra spec | |
| 14:33:48 | stephenfin | bauzas: Depends on the API version | |
| 14:33:49 | sean-k-mooney | we allwo that as it might be used by a custome schduler filter | |
| 14:34:14 | bauzas | sean-k-mooney: I mean, what would happen if I was creating a flavor with accel:device_profile metadata key inside without using Cyborg at all ? | |
| 14:34:36 | bauzas | I would expect my instance to be created | |
| 14:34:39 | sean-k-mooney | oh thats allowed | |
| 14:34:45 | sean-k-mooney | it will just not do what you want | |
| 14:34:49 | bauzas | sure | |
| 14:34:54 | bauzas | but here, see | |
| 14:34:58 | sean-k-mooney | we will likely fail because we cant look it up | |
| 14:35:06 | sean-k-mooney | e.g. the device profile | |
| 14:35:19 | bauzas | people creating flavors with this key would previously get a 200 | |
| 14:35:32 | sean-k-mooney | ya | |
| 14:35:34 | bauzas | and now we're returning a 403 because of the cyborg deco | |
| 14:35:36 | bauzas | this is said | |
| 14:35:38 | bauzas | sad | |
| 14:35:43 | sean-k-mooney | sure | |
| 14:35:51 | bauzas | this would have required a microversion | |
| 14:35:57 | sean-k-mooney | no | |
| 14:36:05 | sean-k-mooney | extra specs never require a microverion | |
| 14:36:14 | sean-k-mooney | they are an unversioned part of the api | |
| 14:36:19 | bauzas | changing a 200 to 403 ? | |
| 14:36:32 | dansmith | anything could become a 403 at any point, right? | |
| 14:36:39 | dansmith | like you stop having permission to do something | |
| 14:36:52 | sean-k-mooney | well via a policy change yep | |
| 14:36:55 | bauzas | 403s are said to be accepted without new microversions | |
| 14:37:10 | bauzas | but there is a but | |
| 14:37:11 | bauzas | https://docs.openstack.org/nova/latest/contributor/microversions.html#f2 | |
| 14:37:35 | bauzas | getting a 403 because keystone check obviously doesn't require a new microversion | |
| 14:37:37 | sean-k-mooney | bauzas: to get the flavor extra spec validation you have to opt into it via a new microverion | |
| 14:38:01 | bauzas | either way, the ship has sailed | |
| 14:38:07 | bauzas | this deco is there for a while | |
| 14:38:20 | bauzas | and fwiw, happy to see you around dansmith :) | |
| 14:38:26 | sean-k-mooney | right but fundementally there is noting broken or wrong with this | |
| 14:38:38 | dansmith | bauzas: :) | |
| 14:38:56 | bauzas | sean-k-mooney: sure | |
| 14:39:09 | bauzas | anyway, let's continue to review the change | |
| 14:39:22 | sean-k-mooney | the only way to prevent this is to require that operators addign custom extra specs use either no namespace or custom: | |
| 14:39:47 | sean-k-mooney | that would be an improvment in my view but it would be an api change which would need a microverion for sure | |
| 14:39:57 | bauzas | sean-k-mooney: on a side note, https://review.opendev.org/c/openstack/nova/+/729563/26/nova/api/openstack/compute/shelve.py#59 means that now when you shelve, you're getting a 500 with cyborg, right? | |
| 14:40:19 | bauzas | since the exception wrapper is just added but the decorator was existing... | |
| 14:40:42 | sean-k-mooney | well we have patchs to enable cyborg shelve support right | |
| 14:41:02 | sean-k-mooney | oh it is the patch | |
| 14:41:39 | sean-k-mooney | exception.ForbiddenWithAccelerators should not be raised here anymore | |
| 14:42:34 | sean-k-mooney | bauzas: i havent review this in a while | |
| 14:43:03 | bauzas | sean-k-mooney: well, we decorated shelve() a while ago | |
| 14:43:05 | sean-k-mooney | why would self.compute_api.shelve(context, instance) raise that with this chagne? | |
| 14:43:16 | sean-k-mooney | bauzas: yes i know | |
| 14:43:24 | sean-k-mooney | but that decorator should be removed with this patch | |
| 14:43:28 | bauzas | so we were returning ForbiddenWithAccelerators up to the API | |
| 14:43:29 | sean-k-mooney | that was the whole point | |
| 14:43:48 | sean-k-mooney | we were not going to require a new micorverion for unshelve with cyborg | |
| 14:43:49 | bauzas | so in theory we should have managed such exception | |
| 14:44:06 | bauzas | because afaics, we were just passing it thru | |
| 14:44:11 | bauzas | leading to a 500 | |
| 14:44:20 | dansmith | I'm super confused | |
| 14:44:26 | bauzas | now, this patch addes the exception handle | |
| 14:44:41 | dansmith | the point of the decorator was to make the 500 into 403s, acting like "you don't have permission" instead of "this doesn't work" | |