Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-12-16
16:12:04 openstackgerrit Balazs Gibizer proposed openstack/nova master: Reproduce bug 1907522 in functional test https://review.opendev.org/c/openstack/nova/+/766791
16:12:04 openstack bug 1907522 in OpenStack Compute (nova) "test_*_with_qos_min_bw_allocation fails in the nova-multi-cell job with: nova.exception.MigrationPreCheckError: Migration pre-check error: Failed to create port bindings for host " [High,In progress] https://launchpad.net/bugs/1907522 - Assigned to Balazs Gibizer (balazs-gibizer)
16:13:26 openstackgerrit Balazs Gibizer proposed openstack/nova master: Fallback to same-cell resize with qos ports https://review.opendev.org/c/openstack/nova/+/766925
16:15:35 gibi stephenfin: and fixued up this as well ^^
16:17:03 gmann gibi: for specific budy I mean, rest of the action APIs like lock does not take any parameter in request body so they are allowed everything since legacy time. But in unshelve API, 2.77 version added AZ to be a valid param in request body so in this action API we said if user can pass AZ as valid param then let's allow null or AZ only and block everything else.
16:17:40 gibi gmann: lock takes a lock_reason optionally similarly how unshelve takes AZ optionally
16:18:23 gibi https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/lock_server.py
16:21:16 gmann gibi: ah yeah, we have not added 'locked_reason' as required.
16:21:33 gibi we not added AZ as required either
16:22:02 gmann gibi: so we can pass anything in cok API like lock: {'unknown': 'unknown'}
16:22:10 gibi no you cannot
16:22:16 gibi as additionalProperties false
16:22:21 gmann https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/schemas/shelve.py#L31
16:22:24 gmann here it is ^^
16:23:26 gmann that required is specially to block the empty dict so that we can have more strict shcema.
16:23:36 gmann let me check comments again if any other reason
16:23:44 gibi gmann: but why we need more sticter schema?
16:24:13 gibi the meaning of unshelve: {} and unshelve: null is the same from the impl perspective (except the schema)
16:24:42 gmann gibi: schema main idea is to have a very strict checks so that user will only be able to pass the usable parameters and everything else will be 400
16:25:09 gibi sure, I'm OK not to allow 'unknown':'unknown'
16:26:28 gibi what I see that our API acceptr "<action>":{} and "<action>": null in mot of the cases but not for unshelve
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

Earlier   Later