| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-10 | |||
| 11:04:28 | stephenfin | \o/ | |
| 11:30:43 | bauzas | stephenfin: yup, I was looking at other series, but I'll review them again this afternoon | |
| 11:40:37 | yonglihe | sean-k-mooney: I try to work out your concern and that bug is about LM/resize, that's out of scope. refer to comments, there are some idea about that. | |
| 11:43:33 | yonglihe | sean-k-mooney: I did not dig into that bug, just say: It's seems that's data out of sync somehow, change how to store the data won't help? (fix me) | |
| 11:54:36 | alex_xu | sean-k-mooney1: yonglihe I looked those two bugs, hope I understand that correctly https://review.opendev.org/c/openstack/nova/+/771363/11/nova/objects/network_request.py#42 | |
| 12:07:36 | sean-k-mooney1 | artom: nova default to creating 1 socket per vcpu | |
| 12:08:11 | sean-k-mooney1 | so in the gate vms you will have 8 socket and 1 numa node generated implcitly by qemu/libvirt | |
| 12:08:19 | sean-k-mooney1 | even for non numa vms | |
| 12:09:16 | sean-k-mooney1 | stephenfin: by the way passing the function form the driver.py to host.py goes away in a later patch where i actully move the fucntions too host.py | |
| 12:09:29 | stephenfin | sean-k-mooney1: could you do that first? | |
| 12:09:31 | sean-k-mooney1 | we dont actully use the function that take the callback until that patch | |
| 12:09:56 | sean-k-mooney1 | maybe but it will be a bit of a pain to split back out the patches | |
| 12:10:24 | sean-k-mooney1 | i can do that it more a question of time | |
| 12:11:09 | sean-k-mooney1 | artom: would you be ok removing the numa toplogy object form the pci tracker and adding in a numa to socket map instead | |
| 12:12:11 | sean-k-mooney1 | although if we have to account for the many socket to 1 numa node of the gate i guess we need to retink that abit | |
| 12:12:42 | sean-k-mooney1 | in the gate all pci devices will be connected to the first socket | |
| 12:12:50 | sean-k-mooney1 | we only have 1 pcie root | |
| 12:13:05 | sean-k-mooney1 | and there is also only 1 numa node by default | |
| 12:13:53 | sean-k-mooney1 | a hack would be to have that numa to socket dict be int->list[int] and for you to just use the first socket form the list | |
| 12:14:08 | sean-k-mooney1 | we did discuss that at one point | |
| 12:17:32 | sean-k-mooney | alex_xu: its the same bug just upstream and downstream version | |
| 12:17:46 | sean-k-mooney | alex_xu: the bug is not related to claiming the pci devics | |
| 12:18:14 | sean-k-mooney | alex_xu: we correctly claim the pci device the issue is we dont update the neutron port profile correctrly as part of unshelve | |
| 12:18:34 | sean-k-mooney | alex_xu: and we generate the xml with the pci address stored in the profile | |
| 12:18:48 | sean-k-mooney | so we use the pci address form its previous host | |
| 12:19:00 | sean-k-mooney | so the vm we are unsleving is the one using the incorrect device | |
| 12:19:31 | alex_xu | sean-k-mooney: yes, so for cyborg, we should update the port with new arq uuid | |
| 12:19:39 | sean-k-mooney | yes | |
| 12:20:04 | alex_xu | so I'm thinking we need the network_request.arq_uuid again to pass that new arq from conductor to compute | |
| 12:20:15 | sean-k-mooney | and we should update the profile with the claimed pci address for sriov too we just dont today | |
| 12:20:25 | alex_xu | yea | |
| 12:20:40 | sean-k-mooney | alex_xu: well network_request is not stored in the db right | |
| 12:20:53 | alex_xu | it is fine, we needn't the old arq | |
| 12:20:56 | sean-k-mooney | im uncomfortable with having the neutron port binding be the only place we persit that | |
| 12:21:09 | sean-k-mooney | we need it for things like hard reboot no? | |
| 12:21:30 | alex_xu | we needn't, I check that also, we build network_info from the neutron directly | |
| 12:21:59 | sean-k-mooney | we do and store it in the network info cache | |
| 12:22:14 | sean-k-mooney | my point is i dont think we should be using the value sotre in neutron to generate the xml | |
| 12:22:28 | sean-k-mooney | we should be tryign to do it without using the network info cache | |
| 12:22:49 | sean-k-mooney | so that user cant acidentally alter the port profile in a way that can brake nova | |
| 12:22:51 | alex_xu | but all the action is using that network info, even for boot instance | |
| 12:23:04 | sean-k-mooney | right and that is a design flaw | |
| 12:23:13 | sean-k-mooney | that info can be currpted | |
| 12:23:43 | sean-k-mooney | i have seen it happen in customer deployment many times | |
| 12:24:03 | sean-k-mooney | generally it can be recoverd but we should really treat that as write only | |
| 12:24:03 | alex_xu | I'm thinking if the user done that, whether it means there already have other thing goes wrong | |
| 12:24:45 | sean-k-mooney | its possible yes | |
| 12:25:01 | alex_xu | so binding profile is only for neutron use, but nova should have it own copy | |
| 12:25:44 | alex_xu | the hard thing is if the nova copy is different with neutron one, what should nova do | |
| 12:25:54 | sean-k-mooney | binding profile was intended to be used to pass info from nova to neutorn in one direction | |
| 12:26:05 | sean-k-mooney | alex_xu: use the nova one | |
| 12:26:11 | artom | sean-k-mooney, yeah, that is a good idea | |
| 12:26:13 | alex_xu | ok | |
| 12:26:29 | alex_xu | then correct the neutron info | |
| 12:26:30 | artom | And to internalize the function that generates that map into the NUMATopology object itself | |
| 12:26:34 | sean-k-mooney | alex_xu: https://github.com/openstack/neutron-lib/blob/master/neutron_lib/api/definitions/portbindings.py#L31-L34 | |
| 12:26:43 | artom | Not sure we have enough time before FF though | |
| 12:26:50 | sean-k-mooney | alex_xu: correct field that differ ideally | |
| 12:27:04 | sean-k-mooney | but we cant do that based on the info cache we have today | |
| 12:27:08 | artom | sean-k-mooney, also, if those _filter functions are really meant to be purely functional, then @classmethod is not the way to do it - you put them outside the class entirely :) | |
| 12:27:26 | artom | I'll see what I can hack up today | |
| 12:27:45 | sean-k-mooney | artom: why class method are prefectly valid ways to do that | |
| 12:28:02 | alex_xu | for yongli's patch, should we block it for that issue? | |
| 12:28:04 | sean-k-mooney | you coudl put them outside the class entirely or even make them static | |
| 12:28:23 | alex_xu | I feel it is hard for yongli to fix that, since it is boarder issue | |
| 12:28:26 | sean-k-mooney | alex_xu: am no we could proceed and clean this up next cycle | |
| 12:28:38 | alex_xu | yea | |
| 12:28:48 | sean-k-mooney | alex_xu: im just uncomfortable realying on that | |
| 12:29:05 | sean-k-mooney | i was hoping we could ask cybrog for the arq using the neutorn port uuid | |
| 12:29:18 | sean-k-mooney | and have cyborg be the souce of truth | |
| 12:29:40 | alex_xu | but nothing we can do it now, I don't think system metadata is good place, since we said before, we shouldn't anything to it, since it without version control | |
| 12:30:18 | sean-k-mooney | alex_xu: actully i thought the preferece was to use it for things linke this over adding more tables | |
| 12:30:29 | sean-k-mooney | alex_xu: we can have version contol by using ovos | |
| 12:30:48 | sean-k-mooney | it allow use to store addtional info without db migrations | |
| 12:31:03 | sean-k-mooney | but ok lets put this issue assign for now and maybe add a todo | |
| 12:31:42 | sean-k-mooney | alex_xu: can we maybe add a test to vaoidate unshelve and ensure that the port profile is updated with the new arq? | |
| 12:32:32 | alex_xu | sean-k-mooney: I don't yongli implement that, I remember he said unshelve is out of scope for the spec | |
| 12:32:53 | sean-k-mooney | didnt we add support for shelve with cyborg recently | |
| 12:33:06 | sean-k-mooney | if its not supproted with neutron port then we would need to block it yes | |
| 12:33:48 | alex_xu | I think when yongli's spec merged, the shelve with cyborg doesn't merge yet | |
| 12:34:21 | alex_xu | I think the missing part is update the new arq uuid | |
| 12:34:45 | sean-k-mooney | yep | |
| 12:34:52 | sean-k-mooney | so we allow shelve now https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4169 | |
| 12:35:15 | sean-k-mooney | if the compute service version is new enough | |
| 12:36:34 | sean-k-mooney | alex_xu: ok what we can do is proceed as it is | |
| 12:37:08 | sean-k-mooney | and when i submit the patch to block shelve for sriov instance with a workaoutnd config option i can include cyborg too | |
| 12:37:51 | sean-k-mooney | infact it will do it for both automaticlly | |
| 12:37:59 | alex_xu | ok, so we want a explictly block for shelve instance? | |
| 12:38:13 | sean-k-mooney | i was just going to check if any of the port vnic types were in VNIC_TYPES_DIRECT_PASSTHROUGH which the cyborg ones are | |
| 12:38:53 | sean-k-mooney | yes becuse a vm form one tenatn can break another tenat by unshelving | |
| 12:39:37 | sean-k-mooney | so like numa migration i want to block shelve for all instance using neutron sriov port with a [workaround]/allow_unsafe_shelve to renable it and backprot that | |
| 12:39:44 | sean-k-mooney | then i plan to fix it as a bug | |
| 12:40:06 | sean-k-mooney | so we can fix shleve for both sriov and cybrog by ensuring the port profile si updated for the new arq | |
| 12:40:33 | sean-k-mooney | so i planned to return a 403 permission deined | |
| 12:41:11 | sean-k-mooney | unless you had [workaround]/allow_unsafe_shelve=true or you had a fixed verion of nova | |
| 12:41:34 | sean-k-mooney | alex_xu: does that sound resaonable? | |
| 12:41:50 | sean-k-mooney | gmann: ^ that should be ok form the api point of view yes? | |
| 12:42:19 | sean-k-mooney | gmann: the exisitng issue for sriov ports is a long term bug so i dont want to have to make an api micorverion bump for that | |
| 12:42:50 | sean-k-mooney | 403 can be retruned form this already and i think that should be valid? | |
| 12:43:50 | alex_xu | sean-k-mooney: or we have a check whether instance have a port with device profile? then we needn't an workaround option | |