Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-09
15:18:36 kashyap mdbooth: aarents: lyarwood: Added comparison notes of the three possible ways we can take: https://review.opendev.org/#/c/696084/
15:20:54 efried slaweq: catching up...
15:23:18 efried slaweq: Any time I see spurious grenade fails in the last couple months, I attribute it to oversubscribed CI nodes, per mriedem's "State of the Gate" thread, started here http://lists.openstack.org/pipermail/openstack-discuss/2019-October/thread.html#10484 and continued here http://lists.openstack.org/pipermail/openstack-discuss/2019-November/thread.html#10502
15:24:48 efried slaweq: and per the first note in that thread, yes, the bug you identified (bug 1844929) is the one we're "tracking" the issue with.
15:24:48 openstack bug 1844929 in OpenStack Compute (nova) "grenade jobs failing due to "Timed out waiting for response from cell" in scheduler" [High,Confirmed] https://launchpad.net/bugs/1844929
15:26:10 efried ...and the right solution is to get the CI providers to tweak their environments accordingly. We would rather have lower job throughput and lower failure rates.
15:26:18 efried But so far there has been no reaction from them.
15:26:45 mriedem note that for that particular bug the vast majority of fails are on ovh nodes
15:27:33 mriedem i don't know why it's mostly on grenade jobs
15:27:58 mriedem must have something to do with restarting mysql a few times, but idk
15:29:00 mriedem note that it also started with train, i don't know why though
15:29:12 mriedem tl;dr i don't really know much of anything
15:33:26 sean-k-mooney do we also restart rabbitmq during the grenade upgrade. i assume so but i have not checked
15:33:58 sean-k-mooney it is porably restarted at least once by devstack on the second stacking
15:35:11 sean-k-mooney i noticed that the subnode hand messaging timeouts in both the n-cpu and q-agt services
15:35:57 sean-k-mooney so it looked like we lost messages or something
15:45:14 slaweq mriedem: efried thx for confirmation that this is the bug which we hit most of the times now
15:45:18 efried dansmith: it would seem that the 422 event code is masked when there's only one event (as is the case with cyborg)
15:45:25 slaweq sean-k-mooney: and thx for info about rabbitmq too
15:45:36 efried slaweq: It would help to get some more voices complaining at the node providers.
15:45:54 dansmith efried: really? I didn't see that condition
15:45:58 efried dansmith: when all events are dropped, the handler turns the whole thing into a 404
15:46:06 efried https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/server_external_events.py#L146
15:46:41 efried which is arguably a bug
15:46:44 dansmith efried: ah, yeah, that code isn't really right.. "no instances found for any event" isn't true, just no hosts..
15:46:46 dansmith yeah
15:47:05 efried to "fix" the bug would be a microversion?
15:47:07 dansmith unless mriedem feels that's a microversion problem, I can change that
15:47:31 dansmith I dunno, it's correcting the error code, which I thought was allowable in some situations
15:47:51 efried what would you correct it to? 207?
15:48:26 efried Meaning there's actually no way the API ever returns an error.
15:48:28 dansmith that's what we return per-event in that case right?
15:48:32 dansmith no, there is
15:48:42 dansmith if the instance itself is not found
15:48:50 efried instances plural, right?
15:49:21 dansmith yeah
15:49:22 efried so if some of the instances are not found, that's a 207 with some 404s
15:49:32 efried but if none of the instances are found, that's an overall 404?
15:50:28 efried or,
15:50:28 efried If all events tank, 400 with the granular payload
15:50:28 efried If some events succeed, 207 with the granular payload
15:50:51 efried or,
15:50:51 efried If all events tank for the same reason, make that the overall status code
15:51:10 efried otherwise use 400 or 207
15:51:33 dansmith it really should be that if everything was a 404, then you get overall 404. otherwise it's 207 with granular statuses right?
15:51:58 dansmith or maybe if everything is 200, you return 200 as well, I dunno
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 efried would we then do an overall 422 if none of the instances were mapped to hosts?
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: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

Earlier   Later