| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-26 | |||
| 14:10:38 | openstackgerrit | Sundar Nadathur proposed openstack/nova master: Add cyborg tempest job. https://review.opendev.org/670999 | |
| 14:10:41 | dansmith | I don't see that as a problem yeah | |
| 14:10:50 | sean-k-mooney | but there was a question about if the volume shoudl be set in teh payload or just in the url | |
| 14:11:29 | sean-k-mooney | currently i think the volume id in the url is the current volume and the one in the payload is the one to swap too | |
| 14:12:17 | dansmith | well, that makes it even easier to determine what is going on right? | |
| 14:12:35 | sean-k-mooney | i think we would have to require that its not set in the payload | |
| 14:12:35 | dansmith | if they differ, then check admin policy, if they're the same, user policy and only look for user-changeable options in the body | |
| 14:12:55 | sean-k-mooney | well ya we could require they are the same | |
| 14:13:11 | dansmith | further, PATCH has to be atomic and provides no way to guarantee that the representation hasn't changed since I fetched it on the client side | |
| 14:13:33 | dansmith | so if someone has updated the definition between me fetching it and sending a single field to change, I might not notice | |
| 14:13:41 | dansmith | i.e. there's no generation or other indicator | |
| 14:13:43 | sean-k-mooney | honestly i could be miss remebering what gmann objection was | |
| 14:14:00 | sean-k-mooney | dansmith: yes that is also true | |
| 14:14:05 | dansmith | probably not likely on a volume attachment currently, but certainly could happen in the future | |
| 14:14:20 | dansmith | i.e. some sort of swap-snapshot-swap thing or something | |
| 14:14:32 | dansmith | (this is a major objection of mine to patch in general for most things) | |
| 14:14:46 | sean-k-mooney | i think the most likely issue would be with doing a volume detach via cinder in parralle | |
| 14:15:29 | sean-k-mooney | that said we have the same issue with PUT no? unless we have a genertion field in the payload | |
| 14:15:52 | dansmith | no, with put you have all the original fields there, you can check to see that nothing has changed | |
| 14:15:53 | sean-k-mooney | since put is ment to do a full update replacing all fields at least semantically | |
| 14:16:11 | dansmith | I'm not saying we *have* to for this specific case, but it's at least *possible* | |
| 14:16:18 | dansmith | also, looking at the api-ref here, | |
| 14:16:22 | sean-k-mooney | yes that is true | |
| 14:16:58 | dansmith | there's a big red warning about the whole api being obscure, because people would expect to be able to PUT that.. it would be a lot clearer to revise that and say "changing the volumeId requires special privileges and support" instead of just walling off all of PUT | |
| 14:17:03 | sean-k-mooney | anyway i was pretty nutral on this orginally so if there is a stong feeling (sounds like there is) that it should be PUT then im ok with that too | |
| 14:17:12 | dansmith | because people would say "What? why can't I change this flag without special virt support?" | |
| 14:17:42 | nightmare_unreal | how can I run nova functional tests ? | |
| 14:17:53 | sean-k-mooney | nightmare_unreal: tox -e functional-py36 | |
| 14:18:13 | nightmare_unreal | and for specific file ? -- -n file-name ? | |
| 14:18:34 | sean-k-mooney | you can do -- folowed by any arges supported by stestr | |
| 14:18:51 | sean-k-mooney | normally i just pass a regex for the class name or test name | |
| 14:19:06 | nightmare_unreal | cool :) Thanks | |
| 14:19:21 | sean-k-mooney | e.g. tox -e functional-py36 -- "compute|libvirt" | |
| 14:19:48 | nightmare_unreal | in quotes can i specify exact class name ? | |
| 14:19:55 | nightmare_unreal | like in your example | |
| 14:20:04 | sean-k-mooney | yes | |
| 14:20:11 | nightmare_unreal | awesome, thanks | |
| 14:20:21 | sean-k-mooney | the quote are jsut so the shell does not interperate the | as a pipe | |
| 14:20:54 | sean-k-mooney | so in this case is an or in the regex so that will run all gets with compute or libvirt in the fully qulified function name | |
| 14:21:29 | nightmare_unreal | understood | |
| 14:21:43 | gibi | dansmith, sean-k-mooney: from technical perspective I'm OK to have PUT or PATCH as well. I was convinced about both at some point in the past. From process perspective I feel bad that we might change direction of the API design 2 weeks before FF. | |
| 14:22:26 | gibi | I will raise the discussion again on the team meeting as I hope gmann will be there | |
| 14:22:31 | dansmith | gibi: I would hate to change our API so significantly on a process technicality | |
| 14:22:54 | dansmith | I'd much rather help brinzhang_ get it right, and/or approve a little extra time | |
| 14:23:09 | dansmith | the API stability is serious business to me and much more important to get it right than not | |
| 14:24:05 | gibi | dansmith: OK that is a good point. Let's double check with gmann but I'm OK to give extra time after FF if brinzhang_ needs time to change | |
| 14:24:16 | dansmith | meaning, it's not easily changeable later to allow something to land before a deadline | |
| 14:24:22 | dansmith | we have enough weird "huh, why is this different than *everything* else" warts on the API.. let's not make it worse | |
| 14:25:17 | openstackgerrit | Kevin Zhao proposed openstack/nova master: fix scsi disk unit number of the attaching volume when cdrom bus is scsi https://review.opendev.org/712607 | |
| 14:25:34 | PetrTuma | images (that differ in hw_numa_nodes and few other NUMA related properties). So far none of the rebuilds had been blocked. I added some extra logging to see whether the _validate_numa_rebuild method is actually called or not. It looks like it isn't, I don't see my message logged in any logs. I'm quite at loss what might be wrong. Do you have any | |
| 14:25:34 | PetrTuma | sean-k-mooney Hello, I had some time this week to test the https://review.opendev.org/#/c/703116/3 and the related issue in Rocky. I was able to reproduce original issue (thanks again for the help last week) and now I'm back to testing of the fix for that issue. Once again I updated code in my nova containers and I'm testing the rebuild between two | |
| 14:25:35 | PetrTuma | idea? | |
| 14:26:29 | sean-k-mooney | you updated the nova api container right | |
| 14:26:38 | PetrTuma | yes | |
| 14:26:56 | sean-k-mooney | just checking you didnt update the compute one :) | |
| 14:27:37 | sean-k-mooney | PetrTuma: what release is it? | |
| 14:27:44 | PetrTuma | Rocky | |
| 14:28:06 | dansmith | gibi: do you want me to circle back on that review and re-affirm my -1 with a summary here, or wait for the meeting or something else? | |
| 14:28:09 | sean-k-mooney | i tihnk rocky has the chagne that force the image to be checked if it changes | |
| 14:28:17 | sean-k-mooney | in older releases we did not do that | |
| 14:29:17 | sean-k-mooney | dansmith: you could propose a minor update to the spec with what you want to see? | |
| 14:29:32 | dansmith | I could | |
| 14:29:47 | openstackgerrit | Merged openstack/nova master: Non-Admin user can filter their instances by more filters https://review.opendev.org/701609 | |
| 14:29:53 | openstackgerrit | Merged openstack/nova master: Add default cpu model for AArch64 https://review.opendev.org/709494 | |
| 14:30:53 | PetrTuma | sean-k-mooney do you mean other change than the one I use? | |
| 14:31:49 | hrw | yes! | |
| 14:31:50 | gibi | dansmith: I would be glad for a summary back on the review | |
| 14:32:06 | sean-k-mooney | PetrTuma: yes so a cople of release ago we made a change to cause rebuild with a different image to query the schduler to determin if the current host was valid | |
| 14:32:28 | dansmith | gibi: okay | |
| 14:32:31 | gibi | dansmith: thank you | |
| 14:32:36 | sean-k-mooney | PetrTuma: that is one possiblity as to why this code is not beeing called. if it is not in rocky. although i tought that change was older then that | |
| 14:32:51 | gibi | dansmith: I will try to reach gmann to comment back | |
| 14:33:05 | gibi | dansmith: either on the meeting or separately | |
| 14:34:05 | PetrTuma | sean-k-mooney Ok, I'll try to find the change and confirm this, thanks for the moment | |
| 14:36:37 | gmann | reading logs... | |
| 14:37:00 | sean-k-mooney | PetrTuma: the check for the need to schdule happens here https://zuul.opendev.org/t/openstack/build/cfebf394494246beabb5f417ec1d5c64/logs which is after the check i added | |
| 14:37:45 | sean-k-mooney | PetrTuma: which is here https://github.com/openstack/nova/blob/master/nova/compute/api.py#L3484-L3485 | |
| 14:37:51 | sean-k-mooney | PetrTuma: so that is not the issue | |
| 14:39:20 | sean-k-mooney | PetrTuma: so the only condition that should prevent the _validate_numa_rebuild function running is if orig_image_ref != image_href: | |
| 14:41:20 | dansmith | gibi: done | |
| 14:44:50 | openstackgerrit | jayaditya gupta proposed openstack/nova master: Support for nova-manage placement heal_allocations --cell https://review.opendev.org/714459 | |
| 14:45:07 | gibi | dansmith: thanks again. and sorry that I'm more tense than usual | |
| 14:45:35 | dansmith | gibi: np, I understand the pressure | |
| 14:46:04 | gibi | I guess some of it is new to me :) | |
| 14:46:58 | dansmith | I believe you signed up for it :) | |
| 14:47:03 | gibi | I did | |
| 14:47:09 | gibi | and I'm learning every day | |
| 14:47:22 | gmann | dansmith: gibi sean-k-mooney actually PUT is swap volume thing which is not actually updating resources but kind of migrating the storage from your machine. So PUT(swap) is always confusing and complex API from nova. I am not sure how useful that is for people. I remember a bug when some of our customer trying swap back and forth and getting error. | |
| 14:47:42 | gmann | my main concern on not using PUT was- not to make this API over scoped and keep it for swap only. | |
| 14:48:38 | gmann | i am ok with any method, PUT also ok for consistency to our APIs but new one not in existing PUT which is swap | |
| 14:49:00 | gmann | can we do it via server PUT ? | |
| 14:49:46 | sean-k-mooney | gmann: but thats the thing PUT should not be swap. PUT + changeing the volume id sure | |
| 14:49:56 | sean-k-mooney | but PUT alone should not have been swap | |
| 14:50:35 | dansmith | I have an important call in ten minutes which I need to get ready for and pay close attention to, but after that I'll be back to discuss further if we need | |
| 14:50:46 | gmann | yeah that is not best design but we cannot change that now but can avoid making it more comlex | |
| 14:51:04 | gmann | dansmith: sure. | |
| 14:51:35 | dansmith | IMHO, anything that requires the client to do something different than the obvious thing of PUTting the resource with delete-on-termination changed is more complex | |
| 14:52:06 | sean-k-mooney | gmann: acutlly we could change it in a microversion but not in ussuri at this point. i do tend to agree with ^ on that point | |
| 14:53:37 | sean-k-mooney | * with dansmith | |
| 14:54:08 | gmann | you mean making swap as action(though action are not good but at least consistent to our API ) and PUT for delete-on-termination changed or any future modification in volume things ? | |