| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-03-26 | |||
| 14:06:51 | sean-k-mooney | but as i said it was a compromise not the ideal solution | |
| 14:06:53 | dansmith | sure, but we're using a different http method because we just want to abuse the WSGI plumbing to call different code for this operation vs. the existing put | |
| 14:07:04 | dansmith | so when the next thing comes along, we could use HEAD and avoid having to abstract that one too :) | |
| 14:07:50 | sean-k-mooney | well head would only be apporpriate if the new action match the semantics of head but i get your point and i think you get mine | |
| 14:08:09 | sean-k-mooney | dansmith: in your view you would prefer that we keep with PUT correct | |
| 14:08:14 | dansmith | yes | |
| 14:08:27 | dansmith | and yes, HEAD is a silly example for the sake of being silly | |
| 14:09:04 | sean-k-mooney | i have not looked at gmann specific objection in a while but i belive it was related to the semaintic of the payload | |
| 14:09:16 | sean-k-mooney | do you have a proposal to adress his concern? | |
| 14:09:26 | dansmith | I don't know what his concern is | |
| 14:10:08 | sean-k-mooney | i think there was a question about including the volume id in the payload vs the url and how to correctly apply differnet policies to each sub filed | |
| 14:10:28 | sean-k-mooney | i belive we can already do a polciy check on the payload so that is not really a concern | |
| 14:10:37 | openstackgerrit | Sundar Nadathur proposed openstack/nova master: Block unsupported instance operations with accelerators. https://review.opendev.org/674726 | |
| 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 | |