Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-09-20
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
20:27:59 bauzas definitely a pike regression then
20:28:16 bauzas but we could write a func test anyway
20:29:07 mriedem yeah so just write a functional regression test like we have for others
20:29:44 mriedem should be pretty simple, create 2 computes and 2 instances forced to 1 compute, then live migrate one of the instances and it should fail with novalidhost
20:30:43 mriedem melwitt: dansmith: cells meeting rodeo in 30 minutes
20:30:53 dansmith yup
20:31:03 mriedem trying to wrap my head around this heapq craziness
20:31:25 dansmith do you want to do this one as a hangout?
20:31:30 dansmith I could do some dansplaining
20:31:35 dansmith see what I did there?
20:31:57 mriedem how could i not
20:32:05 melwitt heh
20:33:19 dansmith mriedem: so, hangout? or have you seen enough of me for six months?
20:36:52 mriedem i'm gearing up
20:37:22 melwitt for a hangout?
20:37:44 mriedem and the apocalypse
20:37:50 mriedem but more a hangout right now yes
20:37:53 melwitt :)
20:38:15 mriedem https://hangouts.google.com/call/QDaiUUdRHaNIQmbJi5NVAAkE
20:39:09 dansmith oh now?
20:39:16 mriedem yeah
20:39:25 mriedem my kid will be home in 20 making a bunch of noise
20:43:40 mriedem process_sort_params in the db api
20:43:46 mriedem default_keys=['created_at', 'id'],
20:58:57 bauzas dansmith: mriedem: oh fun, rediscovered https://review.openstack.org/#/c/446446/5/specs/pike/approved/az-block-name-update.rst
20:59:09 bauzas I should copyright that :)
21:20:16 mriedem dansmith: talking about this https://github.com/openstack/nova/commit/c4820305d2f9ee8d62bcc708baf3fa6dfe7ca960
21:42:16 efried stephenfin Ic05c2c8364e015f6878b0bc25449216624568ad5 ouch. This means folks who paid attention to the deprecation and moved to [vnc]vncserver_proxyclient_address are now busted, without a deprecation period on the old-name-in-the-new-group.
21:44:28 efried Arguably the rename should have been done as part of the move. But it warn't. mriedem Can I get a ruling ^ ? (https://review.openstack.org/#/c/498387/)
21:49:50 mriedem wuh
21:51:16 mriedem (1) vncserver_listen was in the DEFAULT group, and moved to the [vnc] group, (2) vncserver_listen was in the [vnc] group and renamed to server_listen in the [vnc] group
21:51:38 mriedem so now [DEFAULT]vncserver_listen just won't work, right?

Earlier   Later