| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-01-31 | |||
| 17:03:43 | melwitt | I'm trying to think of other past cases where the behavior was clearly wrong, have we never introduced 400 for that before? | |
| 17:03:47 | stephenfin | bauzas: :D nw, thank you! | |
| 17:04:05 | gmann | gibi: and no change in 1.39 ? | |
| 17:04:31 | gibi | gmann: in 1.39 there a new `in:` prefix is introduced | |
| 17:04:50 | gibi | required=in:A,B means A or B | |
| 17:05:01 | gibi | but we keep required=A,B as A and B | |
| 17:05:54 | gmann | gibi: ok, so that is different new things which is ok. what about required=A&required=B ? return A and B or 400 in >1.39 ? | |
| 17:06:07 | melwitt | gmann: 1.38 is the current microversion | |
| 17:06:28 | gibi | I propose that in 1.39 required=A&required=B is translated to required=A,B | |
| 17:06:29 | gmann | melwitt: I mean gibi new microversion 1.39 | |
| 17:06:37 | melwitt | earlier I wrongly thought 1.39 was current | |
| 17:06:51 | gibi | gmann: but sean suggest to make that 400 instead there too | |
| 17:06:52 | gmann | I too initially until I checked in doc :) | |
| 17:07:14 | gibi | sorry for not being clear about that | |
| 17:07:18 | gmann | gibi: +1 on making 400 there which will be a clear usage instead of supporting multiple way | |
| 17:07:27 | melwitt | I think translation is fine in 1.39 | |
| 17:07:29 | gmann | I mean with mivroversion | |
| 17:07:45 | melwitt | I was thinking what to do for <= 1.38 | |
| 17:08:10 | gmann | melwitt: but then we end up supporting 1. required=A&required=B 2. required=in:A,B 3. required=A,B | |
| 17:08:28 | gibi | to note that is the easiest to support from code perspective :) | |
| 17:08:40 | gibi | to reject 3. I have to add extra logic | |
| 17:08:41 | gmann | IMO, in new microversion we can make first two to avoid confusion | |
| 17:09:08 | melwitt | what's wrong with supporting repeated and non repeated in 1.39? | |
| 17:09:15 | gibi | also note that repeating required=in: is needed to be able to express (A or B) and (C or D) | |
| 17:09:43 | gmann | anyways with new microversion, we can see what all to support. but my only concern is for older microversion we do not change anything what it is today | |
| 17:10:05 | gmann | and document in api-ref that required=A&required=B is unknown behavior until v1.39 | |
| 17:10:19 | gibi | gmann: do we have a description that defines the line between API behavior (that is unfixable in a bugfix) and non API behavior that fixabelk in a bugfix | |
| 17:10:22 | gibi | ? | |
| 17:10:24 | melwitt | even honoring valid traits? I think at the very least it should be changed to honor the valid traits | |
| 17:12:54 | gmann | melwitt: yeah, for invalid I am ok. my concern is on required=VALID_A&required=VALID_B return VALID_B response today | |
| 17:13:54 | gmann | gibi: that is always debatable :). But IMO anything working successfully today even with wrong usage of API is what we should not change. in this case required=A&required=B, user gets 'B' response is successfully case. | |
| 17:14:24 | gibi | gmann: does successfull means http 2xx response? | |
| 17:14:32 | gmann | and we do not know user expectation is only to get B and by mistake they added A too in early query param | |
| 17:14:39 | gmann | gibi: ^^ | |
| 17:14:54 | gmann | this case ^^ not just 200 | |
| 17:15:30 | melwitt | hm, ok. I guess we disagree there, I think that it should be fixed to return VALID_A and VALID_B even for the current microversion | |
| 17:15:57 | gmann | melwitt: I am fine with that but not with returning 400 in this case for current microversion | |
| 17:15:58 | gibi | melwitt: I agree that we disagree :) and I would simply reject repeated params in the current microverison | |
| 17:16:25 | gibi | gmann: I don't get your last point to melwitt | |
| 17:16:50 | gibi | gmann: if we start returning A and B response for required=A&required=B but the user wants to get B only then it is a behavior change | |
| 17:16:59 | melwitt | fwiw I'm not as concerned about the 400, my main concern is honoring the valid traits in the current microversion | |
| 17:17:04 | gibi | as she get B so far but not any more | |
| 17:17:22 | gibi | melwitt: yeah, it seems 3 of us has 3 different area of concern :) | |
| 17:17:27 | gibi | fun :0 | |
| 17:17:29 | gibi | ;) | |
| 17:17:32 | melwitt | yeah 😂 | |
| 17:17:44 | gmann | gibi: well, we at least does break them in term of return code. and say required=A&required=B we meant for return A and B and we fixed it now. | |
| 17:18:01 | gmann | but returning 400 for them break them as they get something and then they get error | |
| 17:18:44 | gibi | gmann: I'm pretty sure if the relied on gettin RPs with B traits only and now getting empty result as we returning onyl RPs with both A and B will break tem | |
| 17:18:47 | gibi | them | |
| 17:18:48 | gmann | I mean we fix the code to make it return A and B is ok but making them error is issue | |
| 17:19:33 | gmann | gibi: we had similar (not exactly same ) issue in nova query param for silently ignoring few/unknown also and we could not change it without microversion | |
| 17:20:44 | gibi | OK, I'm dropping the bugfix from the patchseries of microversion 1.39. I still intend to land 1.39 this cycle. I don't have such mandate for the bugfix itself. so I prioritize | |
| 17:20:45 | gmann | gibi: sure, in that case we can just leave the current behavior as it is and not breaking anything and say "new microversion gives correct behavior" | |
| 17:21:27 | gmann | I am if we want to change for current microversion then we "make it more correct is fine but rejecting the request is not" | |
| 17:21:36 | melwitt | wait, I don't understand why the bug fix can't go in 1.39 if 1.39 is not released yet? | |
| 17:21:57 | gmann | I think gibi saying bug fix for older microversion also | |
| 17:22:08 | gibi | melwitt: the bug will disappera in 1.39 | |
| 17:22:20 | gibi | melwitt: but I will not try to fix it in <1.39 now | |
| 17:22:28 | gibi | as it seem we cannot agree | |
| 17:22:59 | melwitt | ah ok | |
| 17:23:00 | gibi | 1.39 alway planned to allow repeating the required query param | |
| 17:23:17 | gibi | as in: needs to be repeated for (A or B) and (C or D) case | |
| 17:24:11 | melwitt | ok, so repeated was not officially supported < 1.39. sorry I had missed that | |
| 17:24:44 | gmann | yeah, it was always nor documented neither we knew how it work until gibi found it? | |
| 17:24:47 | gibi | melwitt: <1.39 there was not defined behavior for repeat | |
| 17:25:08 | gibi | the code happened to pares the last repetition | |
| 17:25:12 | gibi | parse | |
| 17:25:15 | gibi | and ignore the rest | |
| 17:25:49 | melwitt | gotcha.. thanks | |
| 17:28:48 | opendevreview | Balazs Gibizer proposed openstack/placement master: Extra tests around required traits https://review.opendev.org/c/openstack/placement/+/825846 | |
| 17:28:49 | opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP db query to support any-traits https://review.opendev.org/c/openstack/placement/+/825848 | |
| 17:28:49 | opendevreview | Balazs Gibizer proposed openstack/placement master: Refactor trait normalization https://review.opendev.org/c/openstack/placement/+/825847 | |
| 17:28:50 | opendevreview | Balazs Gibizer proposed openstack/placement master: Enhance doc of _get_trees_with_traits https://review.opendev.org/c/openstack/placement/+/825780 | |
| 17:28:50 | opendevreview | Balazs Gibizer proposed openstack/placement master: DB layer should only depend on trait id not names https://review.opendev.org/c/openstack/placement/+/826490 | |
| 17:28:51 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for listing resource providers https://review.opendev.org/c/openstack/placement/+/826491 | |
| 17:28:51 | opendevreview | Balazs Gibizer proposed openstack/placement master: Extend the RP tree DB query to support any-traits https://review.opendev.org/c/openstack/placement/+/825849 | |
| 17:28:52 | opendevreview | Balazs Gibizer proposed openstack/placement master: Remove unused compatibility code https://review.opendev.org/c/openstack/placement/+/826493 | |
| 17:28:52 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add any-traits support for allocation candidates https://review.opendev.org/c/openstack/placement/+/826492 | |
| 17:28:54 | opendevreview | Balazs Gibizer proposed openstack/placement master: Add microversion 1.39 to support any-trait queries https://review.opendev.org/c/openstack/placement/+/826719 | |
| 17:29:23 | melwitt | dansmith, bauzas: just a fyi, oslo.limit got a new release so I cleaned up the unified limits set to use it and removed the haxx | |
| 17:30:12 | gibi | sean-k-mooney, gmann, melwitt, efried: I restored the patch series of microversion 1.39 not to change the behavior of the repeated required param handling in < 1.39 microversion https://review.opendev.org/q/topic:any-traits-support | |
| 17:30:42 | gibi | so < 1.39 if the required param is repeated only the _last_ instance is parsed a rest is ignored | |
| 17:30:46 | bauzas | melwitt: cool, as I said, you're my next priority once I'm done with trying to work on Tempest :p | |
| 17:31:40 | melwitt | k :) | |
| 17:34:04 | dansmith | melwitt: nice | |
| 18:07:23 | sean-k-mooney | gibi: ack | |
| 18:20:57 | opendevreview | Rajat Dhasmana proposed openstack/nova master: WIP: Add support for volume backed server rebuild https://review.opendev.org/c/openstack/nova/+/820368 | |
| 18:27:49 | opendevreview | Rajat Dhasmana proposed openstack/python-novaclient master: WIP: Add parameter to rebuild boot volume https://review.opendev.org/c/openstack/python-novaclient/+/827163 | |
| 18:38:08 | 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 | |
| 18:45:38 | 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 | |
| 18:49:53 | opendevreview | Merged openstack/nova master: Add nova-ovs-hybrid-plug job https://review.opendev.org/c/openstack/nova/+/817303 | |
| 19:35:27 | opendevreview | Merged openstack/nova master: Deprecate the powervm driver https://review.opendev.org/c/openstack/nova/+/819366 | |
| 19:42:19 | chateaulav | can i get a second look on https://review.opendev.org/c/openstack/nova/+/822053, not sure what happened but it seems to be wanting to merge... i think im stuck and not sure what i broke | |
| 19:56:23 | melwitt | chateaulav: it show it's in merge conflict, should just need a rebase | |
| 19:57:01 | melwitt | *shows | |
| 19:57:31 | chateaulav | melwitt: yeah, tried, i think i got it. had to hard reset the branch. gonna add all the changes back and see if it resolves now | |
| 19:57:33 | chateaulav | thanks | |
| 20:04:04 | melwitt | ack | |
| 20:43:07 | 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 | |