| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-02-01 | |||
| 02:53:34 | gmann | sean-k-mooney[m]: additional param in query param was also not written behavior in api-ref but we change that with microversion so that we do not break users | |
| 02:54:13 | sean-k-mooney[m] | i really think this raised to the level of a v3 api change if we dont reject it without a microversion change | |
| 02:54:34 | sean-k-mooney[m] | i really dont think this is something we shoudl require a microversion change for | |
| 02:54:34 | gmann | I consider API usage as what code allow than what is there in api-ref. api-ref are not complete set of 'how not to use' its just 'how to use' | |
| 02:55:20 | sean-k-mooney[m] | to me the is a unaccpable burden of mantaince to contineu to supprot. the api is not defiend by the implemantion its defiend by the specification and this is not part of the specificaiont | |
| 02:55:21 | gmann | it is like we allowed our API to be used that wrong way and for many users it was working that as successful case. | |
| 02:55:56 | gmann | sean-k-mooney[m]: not our API :) api -ref was written based on what implementation was. | |
| 02:56:33 | gmann | we try to match that but if api-ref mention something and code does not behave that way then yes we consider as bug | |
| 02:57:03 | gmann | but if api-ref which is not complete does not mention anything does not mean code is wrong and we can change that without versioning | |
| 02:57:34 | gmann | in past we used to document that as known bug/behavior and fix in new microversion | |
| 02:57:51 | sean-k-mooney[m] | i disagree if the api and spec for the feature do not refernce the behaivor it is not part of the public contract | |
| 02:58:32 | sean-k-mooney[m] | i dont think we should leak the impleation into the public contract | |
| 02:58:40 | gmann | it should if we have designed our API that way but it is not. it was always what implementation was and that is being used. | |
| 02:58:52 | sean-k-mooney[m] | we did design our apis that way | |
| 02:59:02 | sean-k-mooney[m] | its why we require a spec for all api changes | |
| 02:59:40 | sean-k-mooney[m] | the really issue i guess is some fo the apis are form pre microversion and did not have a spec | |
| 02:59:59 | gmann | that is fine. I am saying for old and existing behavior. that is why microversion was introduced. for new APis/feature I agree as per spec driven. | |
| 03:00:01 | sean-k-mooney[m] | for example the regex support today in list servers is directly tied to the db you use | |
| 03:00:54 | sean-k-mooney[m] | i really think it would reduce the quality of our software to not fix what in my mind is broken validation code | |
| 03:01:04 | gmann | even rejecting unknown and invalid query param (which we ignored silently in code) was also changed with microversion so that we do not break users | |
| 03:01:48 | gmann | I am not saying that is good and should not be fixed. it should be but with microversion | |
| 03:03:14 | sean-k-mooney[m] | i guess with my downstream hat on i would be much less willing to supprot this. upstream we might be able to allow invalid input but form a downstream perspecitive i dont think i would accpete this form a customer bug. the api contract exits for a reason and not enforcing that contract exposes us to suppporting thing we never intended that is harmful to the healt of the product in my view | |
| 03:04:29 | gmann | my 1 month old saying to stop arguing on APIs :) its time to feed him. :P | |
| 03:04:41 | sean-k-mooney[m] | :) | |
| 03:04:52 | sean-k-mooney[m] | go do that | |
| 03:05:35 | sean-k-mooney[m] | but if we cant fix this without a microverion or validation middleware i think we have to reconsider raising the min microversion to fix this | |
| 08:04:56 | gibi | the more I think about API contract the more I feel that we are on the wrong path. Saying that we should not change the behavior of our APIs without a microversion could be abused to the extent that forbid any kind of change of the implementation behind the API. | |
| 08:06:52 | gibi | say we have a placement sql bug that breaks resource acconting today in a certain situation. As this is how the service works today one can say that a client might depend on the buggy behavior of the resource accounting hence we cannot fix the accounting outside of a microversion so placement servers out on the field will have the buggy behavior until everybody upgrades to future mastyer | |
| 08:07:17 | gibi | so imagine two customers | |
| 08:07:41 | gibi | i) customer depending on the buggy behavior and getting a stable bugfix would break their usage of placement | |
| 08:07:57 | gibi | ii) customer is facing the buggy behavior and their system is broken until the buggy behavior is removed | |
| 08:08:22 | gibi | clearly we cannot support both customer | |
| 08:08:54 | gibi | so it boils down to which customer we would like to support more? i) who depends on a bug ii) who are hit by the same bug | |
| 08:36:45 | bauzas | gibi: agreed with you | |
| 08:37:08 | bauzas | I wonder why we should support the i) customer if they already have an issue | |
| 08:38:26 | gibi | i) has no issue it is happy who the api behaves today | |
| 08:38:55 | gibi | s/who/how/ | |
| 08:42:37 | bauzas | gibi: well, I'm not sure this customer knows about the behaviour actually | |
| 08:42:52 | bauzas | maybe he's just happy because it works | |
| 08:44:40 | gibi | yeah, it can be accidental use | |
| 08:51:18 | bauzas | just a thought, we always said to avoid as much as possible to directly pass a placement request by flavors | |
| 08:52:33 | bauzas | gibi: so that would mean that customers doing this (and passing two same request params) looks to me not a bug | |
| 08:55:34 | gibi | I don't see why repeating required param today is a good idea as it does not give a consistent result. E.g. you say required=A&required=B but A is ignored. Why would you do that intentionally? | |
| 08:56:01 | gibi | so next time you add required=C to the list, B also gets ignored | |
| 08:56:36 | gibi | so whathever client does this that client is wrong and a real bug is about to happen with that client | |
| 08:57:41 | gibi | (btw openstack client is good, --required A --required B creates a REST request with required=A,B) | |
| 08:59:53 | gibi | also imagine that the client today does required=A,B&required=A,B and that produce a good result today | |
| 09:00:19 | gibi | then a new requirement came to add C to the request in that client | |
| 09:00:42 | gibi | the dev looking at the current client code can assume that required parameters can be repeated | |
| 09:00:57 | gibi | as it is repeated today and works | |
| 09:01:13 | gibi | but he might decide not to add C to both instance of required | |
| 09:01:20 | gibi | then he just implemented a bug | |
| 09:06:43 | bauzas | gibi: when I say "not a bug" I mean that's something we can close without a microversion honestly | |
| 10:32:34 | opendevreview | Rajat Dhasmana proposed openstack/nova master: WIP: Add support for volume backed server rebuild https://review.opendev.org/c/openstack/nova/+/820368 | |
| 11:24:57 | sean-k-mooney[m] | so i really think we should fix this however if we cant agree to just do it i would like to add a new default middleway to reject repated args outside a allowed set | |
| 11:28:41 | sean-k-mooney[m] | by the way changes to policy i confider much more likely to break peole then this. we dont reqiure micoro versions for default policy changes for some reason which to me feels like a feautre yet the assertion is we cant fix broken behavior without one feels wrong | |
| 11:37:15 | gibi | sean-k-mooney[m]: I don't see how we would be able to agree on the middleware being a fix without a microversion. Or do you propose that as a microversion bump? | |
| 12:01:44 | sean-k-mooney[m] | no | |
| 12:01:57 | sean-k-mooney[m] | middleware is not part of the versioned api | |
| 12:02:16 | sean-k-mooney[m] | its operator configurable via paste.ini | |
| 12:03:28 | sean-k-mooney[m] | so my proposal in order of preference would 1.) fix without a microversion, 2.) fix with a micorversion and provide middleware for older microverion that returns 400 if there is an unsupported repeated argument | |
| 12:04:18 | sean-k-mooney[m] | 3.) fix with a microverion and serioiusly consider raising min micorversion over the next few cycle until we finally get to this one | |
| 12:05:28 | sean-k-mooney[m] | gibi at some point i stongly feel this needs to be fixed by default for everyone that uses nova and placment since the same broken behavior affects both | |
| 12:06:03 | sean-k-mooney[m] | so if we force a microverion for this we need to look at starting the process of raising or min micorversion | |
| 12:09:55 | gibi | sean-k-mooney[m]: ahh I see, so the middleware would be optional therefore does not need a microversion. Yeah that could be tried. Probably on can say that becomes a config driven API but paste.ini is created exactly for that kind of things | |
| 12:10:37 | sean-k-mooney1 | ya i was lookign are our exising middelware it does not look like it would be that hard to create one that did this | |
| 12:11:14 | sean-k-mooney1 | basically using https://github.com/openstack/nova/blob/master/nova/api/openstack/identity.py as a template | |
| 12:12:07 | sean-k-mooney | we would then add a new filter lin and add that filter to the pipeline https://github.com/openstack/nova/blob/master/etc/nova/api-paste.ini#L68-L81 | |
| 12:12:53 | sean-k-mooney | i belive that is how that work but i have never had a need to do that before but that seam to be the patteren | |
| 12:14:24 | sean-k-mooney | i read over most of our docs last night to try and find if they provdied any guidence on if repated arges were ever intended to be support and i cant fine any to that efffect for what its worth | |
| 12:16:43 | sean-k-mooney | with that said the https://github.com/openstack/nova/blob/master/doc/source/reference/stable-api.rst#v2-api-compatibility-mode-based-on-v21-api doc does imply that addtion request parmaters are not intended to be allowed | |
| 12:16:57 | sean-k-mooney | """v2.1 API is exactly same as v2 API except strong input validation with no additional request parameter allowed and Microversion feature.""" | |
| 12:21:16 | gibi | "additional" probably means "unknown to nova" not "repeated know" params | |
| 12:21:38 | gibi | as we allowed (and ignored) unknown params in the past | |
| 12:22:51 | gibi | but I agree, I think we need to put a rule to repeated params | |
| 12:27:25 | pmonteir | Good morning everybody! Does anybody know how the "live_migration_downtime" parameter was tested? Been having some trouble trying to understand how this works | |
| 12:29:21 | sean-k-mooney | pmonteir: its not really. we just pass that to libvirt nova is not in contol of the downtime | |
| 12:29:43 | sean-k-mooney | once nova start the migration libvirt is basically in charge until its done | |
| 12:30:17 | sean-k-mooney | we can call libvirt with addtional commands like force complete or abort | |
| 12:31:11 | sean-k-mooney | but nova does not activly monitor the downtime and progress we passivly recive the events form libvirt and we can query it in resonce to an api request but we just get out of libvirts way and let it do its thing | |
| 12:34:15 | pmonteir | Oh... I see, so if the max downtime is hit, that's just something we let libvirt handle? | |
| 12:35:33 | sean-k-mooney | i can check, in the case of max im not sure but the normal pauses ectra are not done by nova | |
| 12:36:53 | sean-k-mooney | we configure the max down time on the guest https://github.com/openstack/nova/blob/master/nova/virt/libvirt/migration.py#L475-L527 | |
| 12:37:17 | sean-k-mooney | im just checkign to see if we handel when its exceeded in nova or preconfigure the action to take | |
| 12:38:25 | pmonteir | Yh, that's exactly the thing I'm having some trouble trying to find out. If something is actually done when its exceeded | |
| 12:39:23 | sean-k-mooney | i know we have action if the over all migration timeout is exceeded but not sure about max_downtime | |
| 12:39:32 | sean-k-mooney | kashyap: do you happen to know ^ | |
| 12:41:01 | kashyap | I don't remember off-hand, have to dig in too | |
| 12:42:00 | sean-k-mooney | pmonteir: what are you trying to achive by modifyign this by the way | |
| 12:42:10 | sean-k-mooney | pmonteir: i assume you have seen https://github.com/openstack/nova/blob/50fdbc752a9ca9c31488140ef2997ed59d861a41/doc/source/admin/configuring-migrations.rst#advanced-configuration-for-kvm-and-qemu | |
| 12:42:45 | kashyap | Yeah, let's step back to understand the bigger picture | |
| 12:43:15 | sean-k-mooney | the auto convergence and post copy options might be of interest to you if you are seeign excessive downtime | |
| 12:43:47 | sean-k-mooney | if you are seeing network downtime/ping loss that is likely unrealted to this | |
| 12:44:23 | pmonteir | Yh, I have. So basically I was trying to understand this parameter and test it out. But I wasn't able to see it in action (the max downtime value being exceeded and a timeout being triggered) | |
| 12:45:09 | sean-k-mooney | unless the vm is hevially loaded and dirtying memory you wont hit this in a normal migration | |
| 12:47:03 | pmonteir | I was trying to migrate whilst dirtying memory, the migration kept going for some time and (once) I used "virsh list" and the vm being migrated got paused and stayed like that for a while, which I found it weird | |
| 12:47:50 | sean-k-mooney | pmonteir: the migration timeout proably got exceeded and use force-compelte | |
| 12:48:14 | sean-k-mooney | https://github.com/openstack/nova/blob/0d84833e9688e0df97f3d24e06025e512bca3ce3/nova/conf/libvirt.py#L372-L388 | |
| 12:48:21 | sean-k-mooney | its our default | |
| 12:49:12 | sean-k-mooney | that is based on live_migration_completion_timeout not based on downtime and downtime has no effect on how long the vm will be paused | |