Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-09
15:52:34 efried I'm not offended by the idea that you always return 207
15:52:56 efried the definition of 207 allows for it to be full failure situations
15:53:02 efried "The response MAY be used in success, partial success and also in failure situations."
15:54:25 dansmith yeah, I think the only reason not to do that is just that things like response.raise_for_status() isn't usable for a dumb cleint
15:54:26 efried If clients are properly coded, that should be an acceptable behavior. But I suspect existing clients are written to how the code is currently mad.
15:54:56 efried well, a client coded to pass blindly on 207 is buggy, period.
15:55:19 efried Sundar: o/ We're just discussing the bug you identified in the server_external_events algo, and how to fix it.
15:55:21 dansmith well, sure, but lots of code does "if s<300: continue"
15:55:36 dansmith efried: anyway, I'm really fine with 207 across the board if others are,
15:55:42 dansmith I'm just arguing what I think some people would argue
15:55:45 efried yes, I know, I'm saying code that does that with a response from this particular API is wrong, before or after the fix.
15:55:55 dansmith very few other things use this interface and they're really all openstack projects
15:56:01 dansmith sure
15:56:18 efried so, 200 if all are green, 207 if any/all fail. I'm happy with that.
15:56:44 efried which basically just means removing https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/server_external_events.py#L149-L151
15:57:24 efried dansmith: I can work that up. Unless you're already doing it.
15:57:49 dansmith efried: I'm not, but I'd also like to hear mriedem say he's okay with it
15:58:07 efried okay. I'll put it up so we can talk about it around the patch.
15:58:24 dansmith sure
16:04:11 mriedem doesn't sound crazy to me, and it's an admin-only api by default
16:04:22 efried cool, forthcoming...
16:04:35 mriedem gmann and alex_xu can pounce on you
16:04:50 mriedem you'll want a bug either way i think
16:05:18 efried Sundar: care to open that, since you uncovered this?
16:05:37 mriedem https://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion will be noted by someone (surely not me just now)
16:06:39 mriedem i think it probably falls into this category a bit "Fixing a bug so that a 400+ code is returned rather than a 500 or 503 does not require a microversion change. It’s assumed that clients are not expected to handle a 500 or 503 response and therefore should not need to opt-in to microversion changes that fixes a 500 or 503 response from happening."
16:06:58 mriedem 207 is returned rather than 404 - and this is likely the only api we have that returns 207
16:07:30 mriedem this probably hasn't come up before either since we don't have events coming in on instances that don't have a host (i don't think anyway - maybe shelve offload?)
16:09:42 openstackgerrit Eric Fried proposed openstack/nova master: WIP: Nix os-server-external-events 404 condition https://review.opendev.org/698037
16:09:51 dansmith well, in those cases, they're likely firing and ignoring the status or getting a different error code than they're really expecting,
16:09:54 efried Ima let zuul tell me which tests to fix --^
16:09:56 dansmith so it doesn't seem likely to affect anyone
16:10:51 sean-k-mooney if you did a neutorn port update on a shelve offload intance it would trigger a network changed event so ya that might be one case
16:10:58 openstackgerrit Stephen Finucane proposed openstack/os-traits master: Add COMPUTE_SAME_HOST_COLD_MIGRATE trait https://review.opendev.org/666604
16:11:03 Sundar efried: I will get back in ~30min - 1 hour, since I have some personal matters to attend now. Sorry.
16:11:22 mriedem yeah 404 isn't really appropriate - that's if we didn't find any instances, but clearly we can but they don't have hosts
16:12:10 efried mriedem: as noted above, we could arguably keep 404 for "none of the instances were found"
16:12:18 mriedem yeah that's fine
16:12:19 efried but even that's kind of weird.
16:12:42 sean-k-mooney im not sure if we have added rebuild form cell0 i think that was planed for stien but it would also have no host set
16:12:42 efried would we then do an overall 422 if none of the instances were mapped to hosts?
16:12:56 mriedem sean-k-mooney: we haven't
16:15:33 sean-k-mooney ok so ya then shelve offloaded is likely the only case then prior to cyborg integration
16:18:31 openstackgerrit Mykola Yakovliev proposed openstack/nova master: Fix boot_roles in InstanceSystemMetadata https://review.opendev.org/698040
16:27:06 gmann mriedem: dansmith efried 404 case (else part of if accepted_events) can be due to multiple reason for each event. For example, it can be due to few event are 400, 404 and 422. May be overall status in that case can be 400 saying error message as "none of the event are accepted, check individual event failure reason." ?
16:27:37 efried gmann: Thought about that too
16:27:49 efried The definition of 207 allows for total failure, so that seemed simpler
16:27:52 dansmith 207 means "check the individual statuses" which can always be correct
16:27:54 dansmith yeah
16:28:02 efried gmann: thoughts?
16:31:50 gmann yeah, by RFP definition it can be all failure case "The response MAY be used in success, partial success and also in failure situations. " - https://tools.ietf.org/html/rfc4918#section-13
16:32:20 mriedem how would you return a 400 with a json response body with the individual failure reasons?
16:32:21 gmann considering that as success code (2xx) is not correct usage. I agree to change 404->207.
16:32:25 mriedem that's what the 207 is for as dan noted
16:32:31 gmann yeah
16:32:42 efried cool
16:32:57 gmann it need in build response body always
16:42:37 stephenfin mdbooth: comments on https://review.opendev.org/#/c/631294/ if you're looking for something to do
16:51:57 artom stephenfin, if you're reviewing stuff - https://review.opendev.org/#/c/672595/ is still a thing. It's become like this poisoned thing that no one want to touch :(
16:52:14 artom Not sure how I can help get it out of that status
16:53:46 stephenfin I'll take a look
16:54:15 artom stephenfin, it's the NUMA func test patch, if you hadn't guessed
16:54:30 stephenfin I'd guessed :)
16:54:49 Sundar efried: "care to open that, since you uncovered this?" -- sure, I'll open a bug.
16:56:42 Sundar Good to see the discussion. Is it the final conclusion that 'No host found for instance' for one instance is a 207, and rest are 404? If we hypothetically have N instances, with 'No host found' for each of them, will it still be 207?
17:02:48 efried Sundar: the result code will always be either 200 (all ok) or 207 (some/all not ok)
17:02:56 Sundar Never mind: the description of https://review.opendev.org/#/c/698037/ says it is always 207, and 404 is never returned.
17:03:07 Sundar efried: Ok
17:15:47 efried kashyap or sean-k-mooney: would one of you mind giving the SEV fix a quick sanity check please? https://review.opendev.org/#/c/693072/9
17:21:00 kashyap efried: Hi
17:21:08 efried o/
17:22:07 kashyap I'm just about to head out, as I have a table-tennis "competition"; so I'm trying to switch from the sit-like-a-vegetable-in-front-of-the-screen to get-some-oxygen-going mode run :D
17:22:27 efried The code looks fine to me, but I don't really know what a virtio-serial is, or whether I should have one.
17:23:11 sean-k-mooney ill take a look at it
17:23:29 efried kashyap: thanks for responding to my ping. I don't want to send this one over the net and find out later I've dropped the ball.
17:23:31 kashyap efried: Yes, I just read the commit message, it makes sense
17:23:46 sean-k-mooney ... such a bad joke
17:23:49 sean-k-mooney i love it :)
17:24:07 openstack Launchpad bug 1855752 in OpenStack Compute (nova) "Inappropriate HTTP error status from os-server-external-events" [Undecided,New]
17:24:07 Sundar Bug report for the HTTP 207/404 thingy in server external events: https://bugs.launchpad.net/nova/+bug/1855752
17:24:52 openstackgerrit Eric Fried proposed openstack/nova master: Nix os-server-external-events 404 condition https://review.opendev.org/698037
17:24:54 kashyap efried: BTW, 'virtio-serial' is a not-so-stellar-but-works-alright thing that exposes a character dev for simple I/O between host and guest.
17:25:01 openstackgerrit Merged openstack/nova stable/stein: Do not update root_device_name during guest config https://review.opendev.org/696351
17:25:08 efried gmann, dansmith, mriedem, Sundar: I think that's ready now --^
17:25:08 kashyap efried: Nah, you're being your diligent self
17:25:16 efried kashyap: I was making table tennis jokes
17:25:31 kashyap efried: I'm being a dense potatoe
17:26:46 kashyap sean-k-mooney: Ah, I already see you made the correction of generic "virtio controller" --> "virtio serial controller"
17:27:19 sean-k-mooney yes since there are lots of virt controllers :)
17:28:06 kashyap s/going/is going/
17:32:22 kashyap efried: Yes, the reason for the change makse sense to me, FWIW. And I know the "IOMMU" driver being a requirement for SEV. (From its Nova spec)
17:32:59 kashyap (I'll let sean-k-mooney to take a deeper look, as he also was looking at it before.)
17:33:57 sean-k-mooney i have reviewd the bottm two patchs almost done with the final one
17:34:53 openstackgerrit Merged openstack/os-traits master: Add COMPUTE_SAME_HOST_COLD_MIGRATE trait https://review.opendev.org/666604
17:37:27 stephenfin artom: loads of comments left on that
17:38:34 openstackgerrit Matt Riedemann proposed openstack/nova master: Add confirm_snapshot_based_resize conductor RPC method https://review.opendev.org/637075
17:38:35 openstackgerrit Matt Riedemann proposed openstack/nova master: Add revert_snapshot_based_resize_at_dest compute method https://review.opendev.org/637630
17:38:35 openstackgerrit Matt Riedemann proposed openstack/nova master: Confirm cross-cell resize from the API https://review.opendev.org/637316
17:38:36 openstackgerrit Matt Riedemann proposed openstack/nova master: Add finish_revert_snapshot_based_resize_at_source compute method https://review.opendev.org/637647
17:38:36 openstackgerrit Matt Riedemann proposed openstack/nova master: Deal with cross-cell resize in _remove_deleted_instances_allocations https://review.opendev.org/639453

Earlier   Later