Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-16
16:48:44 bauzas gmann: gibi: sorry, on a meeting
16:48:57 bauzas but yeah, {} was accepted before 2.77 and now not
16:48:59 gmann I cannot say which one is more correct but I think disallowing {} make is more specific
16:50:38 gibi from api user perspective who is upgrading from pre 2.77 to 2.77 the new schema is overly restrictive for no good reason (from the api user perspective)
16:51:07 gibi if we have technical reasons then fine, but I don't see any usability reasons to disallow {}
16:53:12 openstackgerrit Balazs Gibizer proposed openstack/nova master: Optionally prevent nova-compute start with DB credentials https://review.opendev.org/c/openstack/nova/+/762176
16:53:37 gibi can some core other than stephen look at this simple revert https://review.opendev.org/c/openstack/nova/+/755995 it re-enables a good chunk of test coverage
16:53:46 bauzas gibi: I'll do
16:53:50 gibi thanks
16:53:51 bauzas but then I need to leave
16:54:37 bauzas done
16:55:12 gibi bauzas: thanks.
16:55:16 gibi I need to leave soon too
16:55:24 bauzas (taxi dad)
16:55:28 gibi o/
16:57:40 openstackgerrit Ghanshyam proposed openstack/placement master: Remove deprecated [placement]/policy_file config option https://review.opendev.org/c/openstack/placement/+/764409
16:57:57 gmann gibi: ^^ updated this
16:58:02 gibi gmann: looking
17:02:25 gibi gmann: replied about the new init parameter
17:04:19 gmann checking
17:06:17 gmann gibi: yeah, we can remove that as it was just for testing things and pick of policy file is on oslo side now
17:06:20 gmann updating
17:06:28 gibi thanks
17:09:53 openstackgerrit Ghanshyam proposed openstack/placement master: Remove deprecated [placement]/policy_file config option https://review.opendev.org/c/openstack/placement/+/764409
17:10:20 openstackgerrit Balazs Gibizer proposed openstack/nova master: Use the non polling notification waiter in func test https://review.opendev.org/c/openstack/nova/+/758445
17:10:21 openstackgerrit Balazs Gibizer proposed openstack/nova master: Create a fixture around fake_notifier https://review.opendev.org/c/openstack/nova/+/758446
17:10:54 gmann gibi: done ^^
17:11:24 openstackgerrit Ghanshyam proposed openstack/placement master: [goal] Deprecate the JSON formatted policy file https://review.opendev.org/c/openstack/placement/+/764394
17:12:17 gibi looking
17:13:16 openstackgerrit Balazs Gibizer proposed openstack/nova master: Use NotificationFixture for legacy notifications too https://review.opendev.org/c/openstack/nova/+/758448
17:13:42 gibi gmann: I'm +2 on both policy patch now, thanks for proposing them
17:14:04 gmann gibi: thanks.
17:14:39 gmann I am going to write the unit tests also for placement policy like we did in nova so that new policy changes can be validated correctly. but sometime next week
17:14:41 openstackgerrit Balazs Gibizer proposed openstack/nova master: Test the NotificationFixture https://review.opendev.org/c/openstack/nova/+/758450
17:16:09 openstackgerrit Balazs Gibizer proposed openstack/nova master: Move fake_notifier impl under NotificationFixture https://review.opendev.org/c/openstack/nova/+/758451
17:46:38 sean-k-mooney1 gibi: gmann so im about to drop for the day but where did we settelon on allowing {} in unshleve
17:47:32 sean-k-mooney1 to me blocking it seams wrong and a bug that shoudl be corrected. { "unkonwn" :"unknown"} != {} the former should be blocked but i think the latter shoudl be treaded the same as null
17:49:36 gibi gmann: ack, thanks
17:49:48 gibi sean-k-mooney1: I don't think we had a final agreement
17:49:58 gibi we just stopped arguing:)
17:50:22 sean-k-mooney1 ok i might be back latter after dinner. but that is my perspective on the topic in anycase
18:12:32 gibi sean-k-mooney1: ack, I share your view
18:16:26 melwitt surely there's precedent for this? I thought a lot of the server actions apis are this way
18:21:33 melwitt if other apis treat null == {} then it seems ok. if they don't, I would make it consistent with the other apis
18:38:00 openstackgerrit Merged openstack/nova master: Revert "Temporarily disable parts of heal port allocation test" https://review.opendev.org/c/openstack/nova/+/755995
18:45:38 gmann sean-k-mooney2: ack, as gibi mentioned, no final agreement yet we can discuss further
18:47:21 gmann melwitt: all other action API except lock-2.56 and unshelve-2.77 except everything in request, {} or {unknown: unknown} because there is no schema for those as per legacy compatibility.
18:48:15 gmann lock-2.56 and unshelve-2.77 started accepted more param in request body and new schema for those follow different pattern. lock does not block {} but unshelve block {}
18:49:07 melwitt gmann: sorry not sure I parse that. you're saying that all apis except lock-2.56 and unshelve-2.77 do not accept {} as null. but that lock-2.56 and unshelve-2.77 do accept it?
18:49:22 melwitt and that is because the older apis have legacy behavior
18:50:22 gmann melwitt: no, all other action APIs except {}, null, or {unknown: unknown}
18:50:36 gmann s/expect/accept
18:51:21 gmann lock after 2.56 and unshelve after 2.77 started then schema checks but both in different pattern
18:51:37 melwitt oh, ok. seems like unshelve should too by consistency. I may be missing some additional context
18:51:54 gmann this is more context on that - https://review.opendev.org/c/openstack/nova/+/663851/34/nova/api/openstack/compute/schemas/servers.py#695
18:52:05 melwitt ok so this decision would likely affect all apis going to a newer microversion?
18:52:15 gmann but yeah i agree on consistency part but which one we follow that is for discussion
18:52:55 melwitt I see. so we will either make unshelve allow {} or make lock disallow {}
18:53:28 gmann melwitt: yeah if we improve the other action APIs or when they have some request body change in new microversion then they will follow the pattern we decide now
18:53:33 melwitt got it
18:53:36 gmann yeah
18:54:00 melwitt ok, so I think {unknown: unknown} seems like the weirdest one that makes sense to block in new apis
18:54:10 gmann and brinzhang_ started to document the current behavior in api-ref https://review.opendev.org/c/openstack/nova/+/767251
18:54:11 melwitt (as sean said earlier)
18:54:40 gmann melwitt: yeah {unknown: unknown} will be blocked for all new mprovement
18:54:43 gmann improvement
18:54:59 melwitt the {} makes some sense though and I could imagine veteran users being caught by surprise if we say that is no longer allowed to represent null
18:55:38 melwitt so it will be tradeoff between api purity vs user ease it seems
18:57:19 melwitt I think I'd be slightly in favor of allowing {} for the user ease reason
18:58:01 melwitt I do appreciate that it is redundant with null though
18:58:19 gmann ok, if user considering {} as null then not breaking existing users make sense.
19:00:10 melwitt yeah I mean, I think the use case will be automation or something that is setting values by a dict and when there is nothing they just do {} instead of saying "if nothing then null". but I don't know whether that is a thing people do
19:03:44 gmann i see, i think in python code it make sense as many automation might default to {} if nothing like kwargs or so.
19:06:24 gmann i agree on doing it consistently.
19:07:40 gmann melwitt: sean-k-mooney2 bauzas gibi let's do it in all action APIs to have consistency . i added this in api cleanup etherpad L89. https://etherpad.opendev.org/p/nova-api-cleanup
19:07:57 gmann may be late in this cycle to do? if so we can do in X cycle?
19:08:15 gmann brinzhang_: ^^
19:09:26 gmann or we can do one more 'multiple API improvement' microversion and pick few more improvement to do along with this.
19:10:36 sean-k-mooney2 gmann: i woudl proably pair it with the return codes cleanup
19:10:54 sean-k-mooney2 so number 8 on line 73
19:11:54 gmann sean-k-mooney2: that is huge change and in past discussion we thought it is not worth to do as it may change many users scripts or so.
19:12:11 sean-k-mooney2 gmann: stephenfin has a patch for that up
19:12:12 gmann but I am ok to re-iterate it
19:12:17 gmann yeah
19:12:31 sean-k-mooney2 so i assumed that would get done this cycle
19:13:00 gmann that needed spec which i think stephenfin said not worth or so but let me check that
19:13:31 sean-k-mooney2 it was a lower priority this cycle so i dont know if he filed one or was going to
19:14:35 gmann https://review.opendev.org/c/openstack/nova/+/749505
19:15:00 gmann that is waiting for spec
19:16:13 sean-k-mooney2 stephenfin might have been hoping to do it as a specless blueprint since its resolving exisitng todos in the code but ya i know it lower on his list then the other items he is working on this cycle
19:17:04 gmann yeah. let's discuss it in X cycle PTG to pick few of them which are worth to do
19:17:22 sean-k-mooney2 works for me
19:18:03 sean-k-mooney2 i would like to see teh is_public flavor atitbue change done too at somepoint
19:18:16 gmann :) was typing that.
19:18:19 sean-k-mooney2 but not enought to prioritise it this cycle
19:18:28 gmann that is worth to fix
19:18:57 sean-k-mooney2 it is but on its own it a lot of work for littel benifit
19:19:12 sean-k-mooney2 with other changes it much much more resonable to me
19:19:33 gmann yeah with other improvement
#openstack-nova - 2020-12-17
00:48:15 brinzhang0 gmann: ack, thanks, you have added in the cleanup apis etherpad, works for me, keep the consistency with other apis is better ^^

Earlier   Later