| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-09-20 | |||
| 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 | bauzas | like requested_destination | |
| 19:32:20 | dansmith | mriedem: my set should be pretty easy to follow I think | |
| 19:32:34 | dansmith | mriedem: I tried really hard to add functionality in layers | |
| 19:32:51 | mriedem | dansmith: yeah, i'm going to hit it now - i'll wait to deal with this reqspec stuff for tomorrow morning | |
| 19:32:55 | mriedem | gives me something to look forward to | |
| 19:40:17 | cdent | dansmith: there’s loads of pre-existings tests lying around that don’t require AllocationList to have a project and user. That makes it feel like more effort than it is worth to start requiring it in the Allocation object, so what I’m thinking of doing is not requiring it, but if they are there in the DB, loading it up when reading the object. Concur? | |
| 19:41:00 | dansmith | cdent: I'm not sure what you mean.. surely we require it from the API now right? | |
| 19:41:22 | dansmith | I guess one of the early microversions didn't require it so we _could_ have things in the DB that don't? | |
| 19:41:43 | mriedem | consumers requires project_id/user_id in the API in the latest microversion, | |
| 19:41:48 | mriedem | allocations doesn't | |
| 19:42:31 | melwitt | I thought PUT allocations required project/user | |
| 19:42:36 | cdent | they do | |
| 19:42:46 | cdent | but we don’t enforce that at the object level | |
| 19:42:51 | cdent | only the http api level | |
| 19:43:09 | dansmith | cdent: we should have migrated any existing allocations explicitly or incidentally | |
| 19:43:22 | cdent | what I’m saying is: to make the change to requiring things will change a pile of tests | |
| 19:43:31 | dansmith | cdent: if the http api always forces it (which it should) then we should raise an error when we hit one in the DB that doesn't, not just return an object with those things missing | |
| 19:43:45 | melwitt | the existing allocations get overwritten upon update with newer code to have project/user | |
| 19:43:53 | melwitt | (periodic update) | |
| 19:44:01 | dansmith | melwitt: right, that's what I meant by incidentally | |
| 19:44:53 | melwitt | k | |
| 19:45:17 | cdent | so the question I have two things: should I change all the tests and make the enforcement all over the place, or should I just make sure that we read the data when the data is there (which it always will be for real-world allocations) | |
| 19:45:54 | dansmith | if the tests don't mirror reality, then we should update the tests I think | |
| 19:46:35 | dansmith | and maybe we should have a blocker migration that ensures that all the things in the DB have gotten user/project fields going forward to force the issue or something | |
| 19:46:48 | dansmith | so we can remove the nullable on the schema Imean | |
| 19:47:50 | cdent | out of curiosity: why’s that matter? | |
| 19:48:18 | dansmith | we put constraints in the database to make sure we don't store data that violates the schema we want right? | |
| 19:48:40 | dansmith | like, you can't have an info_cache for an instance that doesn't exist, and you can't have more than one instance with the same id | |
| 19:48:48 | cdent | right, and we have two forms that the database is happy to support | |
| 19:48:51 | dansmith | if user/project are not optional, then they should not be optional | |
| 19:49:03 | cdent | they are optional, for hosts that are long lived | |
| 19:49:22 | cdent | as in, started life before all this | |
| 19:49:39 | dansmith | that doesn't mean they're optional, that means some data is in an old format | |
| 19:50:01 | dansmith | hence the migration, blocker, etc to make sure we don't have to deal with two formats forever | |
| 19:50:02 | dansmith | if we want one format, we should get all our data into that one format | |
| 19:53:21 | cdent | Well, I guess I can do all that, but it’s not going to happen tonight I’ve run out of brain | |
| 20:07:03 | bauzas | mriedem: dear god, related issue https://bugs.launchpad.net/nova/+bug/1718455 | |
| 20:07:04 | openstack | Launchpad bug 1718455 in OpenStack Compute (nova) "[pike] Nova host disable and Live Migrate all instances fail." [Undecided,New] | |
| 20:07:11 | bauzas | mriedem: I'm working on the fix now | |
| 20:07:50 | bauzas | mriedem: but we should honestly just persist num_instances=1 for move ops | |
| 20:08:06 | bauzas | given we get the ReqSpec by calling the instance UUID | |
| 20:08:29 | mriedem | bauzas: talked at length about that bug this morning | |
| 20:08:44 | mriedem | i would like to see a functional test for the actual scenario, since a related fix made in pike missed that part | |
| 20:09:02 | bauzas | I know, I co-authored that fix | |
| 20:09:11 | mriedem | i think you authored it.. | |
| 20:09:29 | bauzas | I just passed a new revision but meh | |
| 20:09:40 | bauzas | and yeah, we could functional test it | |
| 20:09:55 | mriedem | this https://review.openstack.org/#/c/491439/ | |
| 20:10:00 | bauzas | lke a regression chnage | |
| 20:10:43 | bauzas | oh fun, it was co-authored because of a pep8 fix :) | |
| 20:11:41 | bauzas | ah nvm, got it :) | |
| 20:11:53 | bauzas | anyway, yeah I can work on a regression test | |
| 20:12:03 | bauzas | mriedem: or a func test, as you want | |
| 20:12:32 | mriedem | i don't know if it was regressed in pike or not | |
| 20:12:38 | mriedem | or if this was a latent bug before pike | |
| 20:15:01 | bauzas | mriedem: that should have been regression when we merged the claims stuff | |
| 20:15:27 | bauzas | mriedem: because before that, when you were asking for 10 instances, it was possibly returning you 10 times the same host | |
| 20:16:17 | bauzas | mmm, wait | |
| 20:26:44 | bauzas | mriedem: holy fuck, we introduced the problem with https://github.com/openstack/nova/commit/2bd7df84 | |
| 20:26:47 | bauzas | whack-a-mole | |
| 20:27:10 | bauzas | we changed _schedule to return the number of hosts per instances | |
| 20:27:27 | bauzas | so it's now returning 1 host | |
| 20:27:33 | bauzas | for a live-migration | |
| 20:27:47 | bauzas | but we haven't fixed the caller, so it's still awaiting 10 | |
| 20:27:51 | bauzas | so we're fscked | |