Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-16
16:29:46 openstackgerrit Balazs Gibizer proposed openstack/nova master: stabilize unit test asserting log https://review.opendev.org/c/openstack/nova/+/764796
16:31:13 openstackgerrit Balazs Gibizer proposed openstack/nova master: stabilize set_host libvirt driver unit test https://review.opendev.org/c/openstack/nova/+/764797
16:38:58 gmann gibi: this was the original discussion on disallow the {} and make it more strict like pass None or AZ. - https://review.opendev.org/c/openstack/nova/+/663851/34/nova/api/openstack/compute/schemas/servers.py#695
16:39:08 gmann gibi: that time we did not update api-ref for that
16:39:51 gmann I personally did not see passing or allowing {} is more easy or ok than None
16:42:16 gibi but now has two different type of server action API, the unshelve where {} is disallowed and other actions where {} is allowed
16:42:26 gibi for me the meaning of null and {} are the same
16:43:43 gibi so unshelve now seems special, and I guess this was why bauzas was surprised
16:44:15 bauzas gibi: also the fact that we changed the behaviour between 2.1 and 2.77
16:44:25 bauzas even if that's a microversion
16:44:48 gibi so in pre 2.77 {} is OK but later it is disallowed?
16:45:10 gibi that also feels a bit unnecesserly hars for the users
16:45:11 gmann yeah there is no change for pre-277
16:45:53 gibi the meaning of {} pre 2.77 is the same as null. And we could keep this meaning after 2.77 but we chose to change it
16:45:57 gmann for all other action except lock afrer 2.52 and unshelve after277, everything is allowed.
16:46:42 gmann bauzas: pre 2.77 there is no change, you can pass {} or any unknown param
16:48:15 gmann bauzas: gibi all action APIs does not use schema so they accept anything in request body and we improved that in lock and unshelve action as they started accepting request body. where two different pattern were selected for both of these APIs. in latest which is 2.77 unshelve we started disallow {} too
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

Earlier   Later