| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-20 | |||
| 17:47:00 | tasker | http://paste.openstack.org/show/621557/ | |
| 17:47:09 | mriedem | melwitt: yeah | |
| 17:48:54 | mriedem | melwitt: i think this https://github.com/openstack/nova/blob/master/nova/conductor/manager.py#L583 | |
| 17:49:05 | mriedem | conductor build_instances is called from compute on a reschedule | |
| 17:49:14 | melwitt | well, I learned a thing. I had thought retries were a locally tracked thing per request | |
| 17:49:14 | mriedem | and will put the chosen host in the retry object's list of hosts it's tried | |
| 17:49:28 | mriedem | request spec is the persisted object that keeps on giving | |
| 17:49:32 | mriedem | even when you don't want the gift | |
| 17:49:59 | mriedem | so i think this is the fix http://paste.openstack.org/show/621558/ | |
| 17:50:16 | melwitt | yeah. I wouldn't have chosen to track retries _forever_. I thought RequestSpec would only contain the original request requirements so they could be honored in the future | |
| 17:50:17 | mriedem | just like how we don't want the original forced host/node during live migration, we don't want the original retry hosts either probably | |
| 17:51:45 | tasker | mriedem: I'm going to try your patch out. | |
| 17:52:01 | mriedem | tasker: yeah so the request spec had 2 original attempts, on compute-3.openstack.local and compute-2.openstack.local | |
| 17:52:11 | tasker | both targets I'm trying to send it to. | |
| 17:52:16 | mriedem | ok | |
| 17:52:30 | mriedem | and that's why this fails https://github.com/openstack/nova/blob/master/nova/scheduler/filters/retry_filter.py#L44 | |
| 17:52:36 | tasker | which part of nova would I apply that to? compute, conductor, or scheduler? | |
| 17:52:50 | mriedem | conductor, but note, that diff is against master branch code | |
| 17:53:20 | tasker | let me see if I can adapt to the branch that I'm using. | |
| 17:54:09 | mriedem | http://paste.openstack.org/show/621559/ is latest stable/newton | |
| 17:55:12 | tasker | cool, thanks. I'm going to step out for some "fresh air" and then apply this. | |
| 17:55:19 | mriedem | it does stand to reason that if this instance failed to build originally on those 2 hosts, that live migrating it there might fail too...but we don't know why it originally failed, could have been a resource claim issue at the time | |
| 17:55:36 | tasker | that's a fair observation. | |
| 17:57:56 | melwitt | yeah, often it's a failed claim. and also what if that compute host is eventually replaced over the lifetime of the cluster, making it a fresh candidate for several instances that might still avoid it because they once failed to build there back when it was a different machine | |
| 18:06:45 | tasker | live migration successful | |
| 18:08:17 | tasker | mriedem and melwitt -- thank you so much for helping to figure out what was wrong. | |
| 18:11:01 | mriedem | nice | |
| 18:11:07 | mriedem | tasker: want to open a bug? i have a fix with the test locally | |
| 18:11:40 | tasker | sure. let me collect my notes. | |
| 18:12:31 | cdent | dansmith: responded to some of your comments on https://review.openstack.org/#/c/500410/ . you semi-accidentally identified a separate problem. The nullable thing I’m not quite sure how to proceed, depending on what we want to do. | |
| 18:14:56 | mriedem | cdent: congratulations, you have the first complete blueprint in queens https://blueprints.launchpad.net/nova/+spec/placement-deregister-objects | |
| 18:15:35 | dansmith | cdent: replied, I don't think there's an issue.. just make those not nullable and (separately) always set them and I think we're good | |
| 18:24:25 | cdent | dansmith: except that they are only ever used on input, never output, so why bother reading them? | |
| 18:25:01 | tasker | mriedem: https://bugs.launchpad.net/nova/+bug/1718512 | |
| 18:25:02 | openstack | Launchpad bug 1718512 in OpenStack Compute (nova) "migration fails if instance build failed on destination host" [Undecided,New] | |
| 18:25:07 | mriedem | thanks | |
| 18:25:19 | dansmith | cdent: because it's (effectively) free, it makes the object consistent | |
| 18:25:26 | tasker | I hope that summary / description is clear enough. | |
| 18:28:40 | cdent | dansmith: okay, I’m happy to do that. I’m not sure why consistency matters _now_ but if we’d like it as a general rule, that’s fine. | |
| 18:29:48 | dansmith | cdent: clearly it doesn't have a user now, but we're an abstract model on top of the data store and unless there is a reason not to, we should do that thing, IMHO | |
| 18:30:09 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Ignore original retried hosts when live migrating https://review.openstack.org/505771 | |
| 18:30:17 | mriedem | tasker: ^ i'm going to wip that and send something to the dev/ops lists for this, because if we change this for live migration, we also have to do it for cold migrate/evacuate and unshelve | |
| 18:30:23 | mriedem | melwitt: ^ | |
| 18:30:43 | melwitt | ack | |
| 18:30:58 | cdent | dansmith: does YAGNI count as a reason not to? | |
| 18:31:42 | dansmith | cdent: I dunno what GNI is in there, but if you think there's a reason not to, then don't do it and we can just leave my -1 on there | |
| 18:32:52 | cdent | I’ve just always been brought up on the idea of not doing things unless there is a reason to do so, but as I said, I’m happy to do it and will do it. | |
| 18:34:24 | mriedem | if they are in the same table it seems easy peasy to just load them, it'd be one thing if we were lazy loading a big table join or something, | |
| 18:34:42 | mriedem | but if/when someone needs these, it's going to be weird debugging why they just aren't already in the object when it's read from the db | |
| 18:34:45 | mriedem | like every other object we have | |
| 18:34:48 | dansmith | mriedem: it's a join, but it's super tiny | |
| 18:34:52 | mriedem | oh | |
| 18:35:12 | mriedem | like the usages/consumers table join thing? | |
| 18:35:13 | dansmith | mriedem: like a 1:1 integer join | |
| 18:35:15 | dansmith | yes | |
| 18:35:16 | dansmith | consumers | |
| 18:35:39 | mriedem | i'm assuming at some point we actually intend on using these fields? | |
| 18:36:07 | dansmith | mriedem: if they're fields they should be loadable, and if we're never going to use them that way then maybe we shouldn't have them be fields | |
| 18:36:31 | mriedem | yeah i'd agree with that | |
| 18:36:36 | mriedem | i don't have context here though clearly | |
| 18:36:46 | dansmith | since consumer isn't a top level object in placement, getting them with allocations is the only way we can pull them out otherwise | |
| 18:36:51 | dansmith | unless we're querying for a consumer already | |
| 18:37:18 | dansmith | like if we needed to get all allocations against a resource provider | |
| 18:48:45 | mriedem | tasker: http://lists.openstack.org/pipermail/openstack-operators/2017-September/014233.html | |
| 18:49:08 | openstackgerrit | Chris Dent proposed openstack/nova master: WIP: [placement] manage cache headers https://review.openstack.org/495380 | |
| 18:55:54 | tasker | mriedem: thanks! I'll keep an eye out on these. | |
| 18:57:45 | tasker | mriedem and melwitt: I want to thank you two for taking the time to work with me on my issues for the last few days. your support has been a positive influence in my cloudy days. with your help, I've been able to progress with my upgrade trials. | |
| 18:59:20 | tasker | If you two are ever in the Kansas City area, let me know. first round's on me. | |
| 19:05:32 | mriedem | tasker: cool, you're welcome. you hit some weird issues so i'm glad we flushed those out and have fixes | |
| 19:13:18 | bauzas | mriedem: back now, wazzup ? | |
| 19:14:45 | mriedem | bauzas: http://lists.openstack.org/pipermail/openstack-operators/2017-September/014233.html | |
| 19:18:09 | bauzas | holy shit | |
| 19:18:34 | bauzas | mriedem: honestly, no reason to persist that | |
| 19:18:58 | bauzas | mriedem: if some host wasn't accepted for an instance create, there is no reason it will be excluded for a live migration | |
| 19:19:14 | bauzas | at least, we should still verify it by the scheduler | |
| 19:20:17 | bauzas | mriedem: but here, the main problem is that I should possibly modify the ReqSpec object to have a specific way to say which fields are needed to be persisted and others not | |
| 19:20:43 | bauzas | mriedem: that's something we didn't discussed with alaski when he provided the .save() method | |
| 19:21:43 | melwitt | tasker: yep, thanks for helping us fix those bugs. and I learned yet more things I didn't know about live migration and retries | |
| 19:27:55 | mriedem | bauzas: yeah i was thinking about renaming the reset_forced_destinations() method to something like reset_for_move() and that would reset forced_hosts/forced_nodes/retry | |
| 19:28:08 | mriedem | so it could be handled for live migrate/cold migrate/unshelve and evacuate in the same place | |
| 19:29:05 | bauzas | mriedem: yeah, or just modify _get_update_primitives to disallow the non-needed fields | |
| 19:29:29 | bauzas | mriedem: like using a propery that'd give you which fields are non-persistent | |
| 19:29:51 | bauzas | that way a call to .create() or .save() would avoid persist those | |
| 19:29:57 | bauzas | persisting* | |
| 19:30:07 | mriedem | that won't help us for any existing req specs | |
| 19:30:12 | bauzas | but that wouldn't solve existing specs | |
| 19:30:14 | bauzas | yeah | |
| 19:30:49 | bauzas | okay, I think we could just rename reset_forced_dests() | |
| 19:30:59 | bauzas | and modify the comments of course | |
| 19:31:03 | mriedem | yeah | |
| 19:31:09 | mriedem | to fix all 4 move operations | |
| 19:31:13 | mriedem | i'm going to want to backport this too | |
| 19:31:18 | bauzas | want me to help on that or you feel enough brave ? | |
| 19:31:28 | mriedem | i'm brave | |
| 19:31:34 | bauzas | good boy | |
| 19:31:53 | mriedem | anything that can help me procrastinate from reviewing dansmith's efficient multi-cell instance list / sort thing | |
| 19:31:55 | mriedem | :) | |
| 19:32:06 | bauzas | honestly, I think it would be a good opportunity for discussing which fields shouldn't be persisted | |
| 19:32:20 | dansmith | mriedem: my set should be pretty easy to follow I think | |
| 19:32:20 | bauzas | like requested_destination | |