| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-01-31 | |||
| 21:17:00 | opendevreview | Jonathan Race proposed openstack/nova master: Adds Pick guest CPU architecture based on host arch in libvirt driver support https://review.opendev.org/c/openstack/nova/+/822053 | |
| #openstack-nova - 2022-02-01 | |||
| 00:16:52 | gmann | gibi: ack | |
| 02:30:43 | gmann | gibi: sean-k-mooney[m] melwitt just tested that nova API query param also allow duplicate query param and take latest one. I remember somewhere we convert that to python dict that is why the last one is considered. | |
| 02:31:13 | melwitt | well that's fun | |
| 02:31:52 | sean-k-mooney[m] | there are some query parmaters that we all to be repeated like field that can be used to generate an array | |
| 02:32:19 | sean-k-mooney[m] | but unless we expressly document that as a supported usecase in teh api ref using it should be considerd a bug | |
| 02:32:49 | sean-k-mooney[m] | if its not in the api-ref or in the orginal spec it is not part of the public api contract | |
| 02:33:16 | sean-k-mooney[m] | gmann which nova api query parmater were you testing? | |
| 02:33:26 | gmann | list server | |
| 02:33:40 | gmann | limit filter | |
| 02:37:54 | sean-k-mooney[m] | no user should ever depend on the last instance of a parmater being used in the nova api we are free to change that at any point unless there is api documenation that expressless states that for any given parmater. | |
| 02:39:39 | sean-k-mooney[m] | put another way i do not think we should ever accpeat a bug if we change that behaivor without a microversion as it is an out of contract use fo the api and governed by the micorversion contract as a result | |
| 02:48:42 | sean-k-mooney[m] | ack i would consider depending on that behavior to be unsupported, https://docs.openstack.org/api-ref/compute/?expanded=list-servers-detail#list-servers does not declare you are allowed to repate the args as part of the filter support | |
| 02:49:46 | sean-k-mooney[m] | we should be retrunnign a 400 but we are not validateing that. if we change the behavior in the future it should not require a microversion as this is not part of the api specification today | |
| 02:52:35 | gmann | gibi: melwitt sean-k-mooney[m] we validate it by converting it to set which will add multiple passed value in query param - https://github.com/openstack/nova/blob/master/nova/api/validation/__init__.py#L176 | |
| 02:52:41 | gmann | which is good. | |
| 02:52:54 | gmann | but here this guy just pick latest one https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/servers.py#L179 | |
| 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 | |