| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-01-17 | |||
| 14:15:24 | gibi | thanks | |
| 14:16:18 | sean-k-mooney | im +1 on that and the repoducer below it, thanks for following that pattern it makes it supper clear that the fix is correct | |
| 14:17:26 | sean-k-mooney | you have a seperate change too to stop persiting the numa toloplogy in the request spec correct? | |
| 14:17:56 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/820215 | |
| 14:18:45 | sean-k-mooney | after the current backportable change is done i woudl still be happy to proceed with that too | |
| 14:20:10 | gibi | sean-k-mooney: yeah, I'm happy to move forward with https://review.opendev.org/c/openstack/nova/+/820215 if there is a consensus | |
| 14:20:26 | gibi | the current state of that patch already show that the idea is feasible | |
| 14:21:25 | sean-k-mooney | ya its not required but since it will prevent this type of error in the future its proably worth it in the long run | |
| 15:01:12 | bauzas | gibi: ack, sorry was on meeting | |
| 15:01:21 | bauzas | gibi: will look at it later today | |
| 15:01:39 | gibi | bauzas: thanks, no worries | |
| 15:02:35 | bauzas | gibi: gmann: btw. if we need to update a microversion for unshelve (adding a new host), are we OK if we could remove https://github.com/openstack/nova/blob/1ddb8f83adef964a8ca050994a43adc6175994f1/nova/api/openstack/compute/schemas/shelve.py#L31 ? | |
| 15:03:07 | bauzas | availability_zone is Optional in the doc :) | |
| 15:18:51 | artom | Yeah, it's a weird one | |
| 15:19:00 | gibi | bauzas: looking | |
| 15:19:01 | artom | AZ isn't actually optional, the entire request body is | |
| 15:19:04 | sean-k-mooney | well the az is optional | |
| 15:19:24 | artom | If you have a body, AZ needs to be *something*, could be None | |
| 15:19:35 | bauzas | you need to either provide no dict or if you provide the dict, you need to add the AZ key | |
| 15:19:46 | artom | ... which could be None :P | |
| 15:20:00 | bauzas | so, when adding a new argument like "host", we would also need to add AZ | |
| 15:20:10 | sean-k-mooney | yep but there is no other property you can currently pass in the dict | |
| 15:20:32 | bauzas | yeah, that's why I'm asking to remove this for the new API microversion also adding the new argument | |
| 15:20:41 | bauzas | sean-k-mooney: correct, for the moment | |
| 15:20:43 | sean-k-mooney | artom it would have to be the litral value null potentailly but this is not the only place where its slightly odd | |
| 15:20:55 | gibi | I think both unshelve to an AZ or unshelve to a specific host make sense | |
| 15:21:16 | bauzas | sean-k-mooney: but once Uggla will add a new key like "host", that would mean that the AZ key should be required | |
| 15:21:29 | sean-k-mooney | yep likely you dont need to sepcify both but if you did we shoudl vlaidated the host to az relationship | |
| 15:21:43 | artom | No...? Having to specify both host and AZ seems weird | |
| 15:21:51 | artom | I'd say one or the other, but not both? | |
| 15:21:53 | bauzas | that's why I'm saying we should remove the requiring when we modify this API | |
| 15:22:11 | bauzas | in a new microversion of course | |
| 15:22:15 | sean-k-mooney | bauzas: well as currently specified but in generally unshele:{"host":"my-host"} | |
| 15:22:20 | sean-k-mooney | i think shoudl be valid | |
| 15:22:21 | artom | To my mind what would be clearest is: either empty body, or AZ with a non-null value, or host with a non-null value | |
| 15:22:22 | bauzas | anyway, let's wait Uggla to provide his spec for Z :) | |
| 15:22:32 | sean-k-mooney | i dont think we shoudl required Az if the host is passed | |
| 15:22:38 | gibi | artom: ++ | |
| 15:22:43 | bauzas | sean-k-mooney: agreed, that's my concern | |
| 15:22:54 | bauzas | sean-k-mooney: https://github.com/openstack/nova/blob/1ddb8f83adef964a8ca050994a43adc6175994f1/nova/api/openstack/compute/schemas/shelve.py#L31 doesn't accept it for the moment | |
| 15:23:03 | bauzas | so we need to remove this line | |
| 15:23:16 | bauzas | with a new microversion and when adding a new argument | |
| 15:23:17 | sean-k-mooney | yes in the new microverion | |
| 15:23:26 | sean-k-mooney | in the current one it shoudl remain | |
| 15:23:32 | bauzas | ok, anyway, I think we have a consensus | |
| 15:23:45 | bauzas | sean-k-mooney: of course, I know it | |
| 15:24:00 | sean-k-mooney | yep just make sure Uggla captures it in the spec for next cycle | |
| 15:24:01 | bauzas | I was asking whether it was ok to remove the line at the same time we add a new argument | |
| 15:24:11 | sean-k-mooney | i assume they will start working on the implemantion in parallel | |
| 15:24:26 | bauzas | sean-k-mooney: correct, I discussed that with him :) | |
| 15:24:36 | bauzas | anyway, consensus, that's it | |
| 15:24:39 | bauzas | it was just a question | |
| 15:24:42 | bauzas | a simple one | |
| 15:25:53 | sean-k-mooney | yep i broght that up as we have not created the new spec dir yet but Uggla can totally do that and we can review the spec | |
| 15:26:26 | Uggla | cool, I'll work on this way. | |
| 15:26:32 | sean-k-mooney | but ya my vote is for make it not required and supprot null as well | |
| 15:30:33 | Uggla | passing both option, do you think that passing host should "discard" az | |
| 15:30:40 | Uggla | passing both options, do you think that passing host should "discard" az | |
| 15:31:25 | Uggla | I mean whatever we will have in az will be ignored if host is passed. | |
| 15:31:42 | Uggla | I mean whatever we will have in az will be ignored if host is passed ? | |
| 15:32:13 | gibi | Uggla: no, I think we should not ignore input. we either forbid both to be passed at once, or we should validate that the host is in the az | |
| 15:32:36 | gibi | I more with artom, to forbid to pass both in the same request | |
| 15:33:19 | sean-k-mooney | well either we validate that the host is in the az or it should be an error to pass both in my view | |
| 15:33:37 | gibi | sean-k-mooney: I agree | |
| 15:34:02 | Uggla | ok probably simpler to make it an error. | |
| 15:34:12 | sean-k-mooney | yep simple 400 | |
| 15:34:24 | sean-k-mooney | no existing client will pass host | |
| 15:34:43 | sean-k-mooney | and when optinign into the new microversion they can ensure that only one is generated | |
| 15:35:34 | Uggla | Is it possible to do this kind of exclusive or parameters in the schema validation ? | |
| 15:35:48 | sean-k-mooney | yes | |
| 15:36:03 | artom | Right, but why do more work when less work is enough? :) | |
| 15:37:13 | sean-k-mooney | im pretty sure there is a way to defeint them as a mutualy exclucive group | |
| 15:37:30 | sean-k-mooney | i just cant think of a place off the top of my head whwere we actully do that | |
| 15:37:43 | sean-k-mooney | i suspect this si normally check after the scheme validation | |
| 15:39:32 | Uggla | Is making both options ok in schema validation and then doing the exclusive part in the code, sounds ok ? | |
| 15:39:38 | sean-k-mooney | https://github.com/openstack/nova/blob/1ddb8f83adef964a8ca050994a43adc6175994f1/nova/api/openstack/compute/schemas/servers.py#L172-L173 | |
| 15:39:45 | sean-k-mooney | oneOf | |
| 15:40:09 | Uggla | sean-k-mooney, nice thank you ! | |
| 15:40:42 | sean-k-mooney | as i said we rearly enforce that in teh scheme but no one will object if you do it either | |
| 15:41:40 | sean-k-mooney | there are some other exampels in the samee file | |
| 15:41:41 | sean-k-mooney | https://github.com/openstack/nova/blob/1ddb8f83adef964a8ca050994a43adc6175994f1/nova/api/openstack/compute/schemas/servers.py#L281-L298 | |
| 15:42:58 | bauzas | sean-k-mooney: I'll create the specs directory once we know the Z name :) | |
| 15:43:26 | bauzas | unless I name it 'zombie' directly :p | |
| 15:45:24 | sean-k-mooney | oh ya good point | |
| 15:46:29 | sean-k-mooney | i still quite like "zenith" cause its all down hill from there | |
| 15:53:16 | gmann | bauzas: gibi sean-k-mooney Uggla artom yeah, we can remove the AZ from being 'required' from schema now as it will ne extended to accepting 'host' too. AZ was added as 'required' because this API only accept None or AZ and no other field but that is handled by additionalProperties=False so 'required' is not needed as such. | |
| 15:54:09 | gmann | bauzas: I think you will be right about 'zombie' :) everyone liking it :) | |
| 15:56:36 | sean-k-mooney | if its is zombie we shoudl make it a base for a downstream realse | |
| 15:57:01 | sean-k-mooney | because a decade form now we will still have once custoemr that refuses to move off it and it will never die | |
| 15:57:10 | sean-k-mooney | like any good zombie :) | |
| 15:58:02 | gmann | :) | |
| 15:59:52 | sean-k-mooney | osp 18 actuly should either be the z release of a release depending on timeing so maybe that will happen anyway | |
| 16:00:12 | sean-k-mooney | *Z release or A release | |
| 16:00:59 | bauzas | A = Awesome release I guess | |
| 16:01:17 | bauzas | easy peasy | |
| 16:01:28 | sean-k-mooney | B = Buffy to help with the zombie problem ? | |
| 16:02:01 | bauzas | C = Chilly | |
| 16:02:14 | bauzas | D = Draughful | |
| 16:03:51 | Uggla | I like B especially if Z is zombie ! | |
| 16:14:36 | Uggla | Sarah Michelle Gellar was so cute. | |