| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-15 | |||
| 14:42:20 | dansmith | so, there's a cinder tempest test that is running assertEqual() and failing because an updated_at stamp isn't exactly what it expects... | |
| 14:42:35 | dansmith | I don't see that up on e-r | |
| 14:44:56 | dansmith | it's the minimum basic scenario, where it expects the post-volume-create list to exactly match a show a couple statements later, but seems like something else has touched that volume | |
| 14:45:10 | dansmith | mriedem: know anything about such a thing? | |
| 14:51:09 | mriedem | updated_at or something is different right? | |
| 14:51:16 | mriedem | i've seen that before, maybe a new regression | |
| 14:51:38 | mriedem | stephenfin: it was a log from my devstack patch to default USE_PYTHON3=True which your patch depends on | |
| 14:51:53 | mriedem | note i brought it up in -tc since they were talking about this upgrade testing yesterday | |
| 14:51:56 | dansmith | mriedem: yep. | |
| 14:53:38 | mriedem | dansmith: got a link to a job failure? | |
| 14:53:56 | dansmith | mriedem: https://365c4224c221ec730c2d-019bc8f0795daf4dab730f80e83974fa.ssl.cf1.rackcdn.com/627891/62/check/nova-next/58f7f91/testr_results.html.gz | |
| 14:53:56 | stephenfin | mriedem: I've a minimal reproducer here: http://paste.openstack.org/show/783995/ | |
| 14:53:59 | mriedem | oh https://bugs.launchpad.net/tempest/+bug/1838202 | |
| 14:53:59 | openstack | Launchpad bug 1838202 in tempest "TestMinimumBasicScenario.test_minimum_basic_scenario race fail comparing volume to expected values with updated_at diff" [Undecided,New] | |
| 14:54:00 | mriedem | dansmith: ^ | |
| 14:54:00 | openstackgerrit | Merged openstack/nova master: VMware: Update flavor-related metadata on resize https://review.opendev.org/681004 | |
| 14:54:08 | stephenfin | Stupid pip | |
| 14:54:14 | dansmith | mriedem: ah nice, I didn't see that on e-r | |
| 14:54:38 | mriedem | because it's not....let me look | |
| 14:55:35 | dansmith | if my logstashing is right, looks like it started around 10/7 although not sure how much history we have, that's about a week ago | |
| 14:56:13 | mriedem | yeah the pain in the ass is the mismatch is singleline indexing | |
| 14:56:23 | dansmith | yeah | |
| 14:56:24 | mriedem | logstash only goes back 10 days | |
| 14:56:30 | mriedem | i reported that bug 78 days ago | |
| 14:56:30 | openstack | bug 78 in Baz (deprecated) "When asking you to sign something; baz should tell you what" [Medium,Won't fix] https://launchpad.net/bugs/78 | |
| 14:56:49 | mriedem | heh bug 666 | |
| 14:56:49 | openstack | bug 666 in Launchpad itself "can't file a bug on Ubuntu" [Medium,Invalid] https://launchpad.net/bugs/666 | |
| 14:56:49 | dansmith | yeah | |
| 15:05:08 | mriedem | it looks like the dict keys are at least sorted so i can do: | |
| 15:05:11 | mriedem | http://logstash.openstack.org/#dashboard/file/logstash.json?query=message%3A%5C%22actual%20%20%20%20%3D%20%7B'attachments'%3A%20%5B%5D%2C%5C%22%20AND%20tags%3A%5C%22console%5C%22&from=7d | |
| 15:06:03 | dansmith | hmm, that only shows two hits/ | |
| 15:06:14 | dansmith | maybe those are the two I looked at | |
| 15:07:57 | stephenfin | mriedem: https://review.opendev.org/688731 | |
| 15:08:31 | mriedem | dansmith: yeah it's on 627891 | |
| 15:08:36 | mriedem | it's just rare | |
| 15:08:37 | dansmith | yeah | |
| 15:10:21 | mriedem | stephenfin: ack i'll make my devstack change depend on that | |
| 15:10:59 | mriedem | oh i lost all of my beautiful meaningless +1s......what a world what a world | |
| 15:11:15 | stephenfin | Cool. Looks like a fix in pip _is_ underway but it could be months/years before that lands https://pradyunsg.me/blog/2019/06/23/oss-update-1/ | |
| 15:12:12 | mriedem | cool like what? write a dep resolver for pip? | |
| 15:12:40 | mriedem | fwiw i think lifeless shed many years from his life working on a dep resolver for pip.... | |
| 15:13:01 | mriedem | before or during just saying f it and doing the constraints stuff in openstack | |
| 15:13:48 | dansmith | ah the good old days of openstack | |
| 15:14:26 | sean-k-mooney | mdbooth: is this https://review.opendev.org/#/c/663382/ the patch form stephenfin you planned to add functional test too? if not i might add one in a few days | |
| 15:20:38 | sean-k-mooney | stephenfin: by the way we still need to land this https://review.opendev.org/#/c/675776/ and backport it to train | |
| 15:21:42 | sean-k-mooney | although it looks like it has lots of other change mixed in | |
| 15:24:00 | sean-k-mooney | so ya we shoudl merge v5 https://review.opendev.org/#/c/675776/5 not v6 ill -1 the patch and ask for the patch to be fixed. | |
| 15:49:56 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Remove compute compat checks for aborting queued live migrations https://review.opendev.org/688409 | |
| 17:54:20 | mriedem | i'm finding that writing a functional test for bug 1836754 is so hacky to order the events that it's probably not worth it and we should just throw a retry decorator on delete_allocation_for_instance when we hit a consumer generation conflict | |
| 17:54:20 | openstack | bug 1836754 in OpenStack Compute (nova) "Conflict when deleting allocations for an instance that hasn't finished building" [Low,Confirmed] https://launchpad.net/bugs/1836754 | |
| 17:55:27 | mriedem | efried: also conductor deletes the allocations created by the scheduler if the server is gone by the time we get back from scheduling https://github.com/openstack/nova/blob/149327a3abb12418cdf65316e7c1d4924767bfdf/nova/conductor/manager.py#L1402 | |
| 17:55:30 | mriedem | so that's covered | |
| 17:59:54 | artom | mriedem, we never hit it that often, did we? | |
| 17:59:56 | artom | And only in CI | |
| 18:00:00 | artom | So I think no func test is fine | |
| 18:00:17 | artom | We can use the e-r query to see if merging the commit makes the hits disappear | |
| 18:01:25 | openstackgerrit | Dan Smith proposed openstack/python-novaclient master: Add aggregate-cache-images command and client routines https://review.opendev.org/687141 | |
| 18:15:56 | mriedem | artom: yeah it's very rare | |
| 18:34:35 | openstackgerrit | Merged openstack/nova master: Filter migrations by user_id/project_id https://review.opendev.org/674243 | |
| 18:55:29 | efried | mriedem: are you working on the forced alloc delete rn? | |
| 19:01:19 | mriedem | yeah, need to write a test | |
| 19:01:56 | efried | mriedem: imo we should use DELETE when we're serious rather than using a retry. | |
| 19:02:08 | efried | and indicate seriousness explicitly from the caller | |
| 19:02:41 | mriedem | why was it using PUT with an empty allocations dict to begin with then? | |
| 19:03:41 | efried | mriedem: exactly so we can capture 409s. | |
| 19:03:50 | efried | point being, we only want to do that some of the time. | |
| 19:04:04 | efried | IIRC we used to use DELETE until I made a stink about it. | |
| 19:05:09 | efried | mriedem: I was about to dig in; if you want to write a test, I can take a swing at the patch. | |
| 19:05:18 | efried | unless you're doing both already | |
| 19:05:27 | mriedem | so you must have gotten stinky here https://review.opendev.org/#/c/591597/ | |
| 19:06:52 | efried | yeah, that was a step on the path. I think I remember complaining about the fact that that wasn't really helping us, because of the tininess of the window we were leaving. | |
| 19:07:02 | efried | though clearly we're hitting that window, hence the bug. | |
| 19:08:21 | efried | mriedem: is this something you think we should backport? Because I'd like to do a series where we split the existing one into forced-or-normal, also getting rid of @safe_connect. But can do a "tactical" version for backport if that's a consideration. | |
| 19:09:00 | efried | artom: did you see this bug in real life or just in CI? | |
| 19:09:04 | mriedem | reading some of the comments on that patch it sounds like you were pushing for keeping DELETE rather than going with PUT and consumer generation handling: | |
| 19:09:05 | mriedem | "This patch does not make a conflict visible unless it happens to occur in the teeny window *within* the method itself. IMO introducing this code could make one *think* we're doing so, which is bad. Because the vast, vast majority of cases where such conflicts occur (allocation gets mucked with between when the instance is created and when the deletion process begins) will go completely unnoticed." | |
| 19:09:24 | efried | wise words. Who said them? | |
| 19:09:32 | mriedem | you | |
| 19:09:41 | mriedem | but you said above you made a stink about using DELETE | |
| 19:10:08 | efried | Yes, I pushed for s/DELETE/PUT{...generation}/ in, I think, Dublin. | |
| 19:10:21 | mriedem | because you wanted the caller to decide what to do, | |
| 19:10:25 | efried | because the way it *should* be done is that the GET happens somewhere earlier in the process | |
| 19:10:36 | mriedem | so in this case the API would catch the conflict and retry rather than blindly adding a @retries decorator to the method | |
| 19:10:56 | efried | right, that would be one way to approach it. | |
| 19:11:40 | efried | the other way would be, when we don't care about conflicts and really just want to do the delete, we use DELETE (again, via a new reportclient method, or a flag to the existing, which amounts to the same thing) and never have to do multiple calls. | |
| 19:12:34 | efried | I would advocate for the latter, because this isn't the only place we're going to want to force from, and it's easier to force=True than to write the same retry logic from multiple places, not to mention the fewer calls. | |
| 19:12:37 | mriedem | going back to your other question i would not mangle up removing @safe_connect and all that in the same patch that resolves the bug | |
| 19:13:06 | mriedem | speaking for artom (i'll take the liberty here) i think it's seen in CI only but that doesn't mean it's not in the real world | |
| 19:13:12 | efried | yeah, I was going to do a bottom patch with @safe_connect backward compat and then kill it after the bugfix. | |
| 19:13:35 | mriedem | in fact, if you see this in real life i'm not sure a normal user can even retry the delete if we changed the task_state and didn't reset it on error | |
| 19:14:00 | efried | right, you have to heal allocations via nova-manage or whatever. | |
| 19:14:09 | efried | I think | |
| 19:14:10 | mriedem | no, | |
| 19:14:26 | mriedem | i mean you as a user should be able to just retry the delete of the server since you got a 409 from the compute API (not a 500) | |
| 19:14:45 | efried | right, you *should* be able to, but you can't, you have to use nova-manage today. | |
| 19:15:08 | efried | I gotta chauffeur a kid, back in 20. | |
| 19:15:20 | mriedem | i think we're talking past each other but ok | |
| 19:33:19 | efried | mriedem: I get what you're saying now -- in this particular path we don't actually leak the allocation, we just fail to finish deleting the instance. | |
| 19:33:30 | mriedem | correct | |
| 19:34:25 | mriedem | i'm working something up and will post it so you can see before working on tests | |