Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-07-20
18:34:28 superdan well, this appears to be doing one cell at a time no?
18:34:31 superdan it's not targeting
18:34:40 mriedem it's not multi-cell aware
18:34:42 superdan so just limit by cell when you're getting the mappings
18:34:43 superdan right
18:34:44 mriedem like the other migrations
18:35:18 superdan oh I see this is going by cell db first, so it's naturally scoped to a cell
18:35:27 superdan well anyway,
18:35:28 mriedem how do we know what cell we're in?
18:35:42 mriedem we have [database]/connection
18:35:55 superdan should be trivial to limit it per cell even starting from the mapping I think
18:36:37 superdan it seems like it's worth avoiding the need for another marker-record-based migration...am I wrong?
18:36:37 mriedem would you page across cells until you've filled the limit of instances?
18:37:29 mriedem i don't love a marker-based migration, but it works
18:37:37 superdan I think you'd just make this migration thing take a cell mapping, and then let nova-manage tell it what cell it's working on, and then nova-manage can take a cell_uuid or --all-cells like some of our other things
18:38:01 mriedem so this wouldn't be part of nova-manage db online_data_migrations then...
18:38:06 mriedem i don't love that
18:38:07 superdan no
18:38:09 mriedem b/c then it's another one off
18:38:10 superdan it should
18:38:11 superdan no
18:38:15 superdan it should be in there
18:38:31 mriedem we don't currently talk a cell uuid or -all-cells for online_data_migrations
18:38:38 mriedem *take
18:38:39 superdan I thought we had merged or proposed a thing to let that take a cell, but I see not,
18:38:44 mriedem no,
18:38:45 superdan so just make it figure out the current cell or something
18:39:03 mriedem in YVR we asked about making the migration stuff multi-cell aware and ops (cern) said they didn't care about it (nice to have but not necessary for now)
18:39:21 superdan thinking about this I gues: https://review.openstack.org/#/c/519275/
18:39:22 mriedem i'm not sure how we figure out the current cell - we have the [database]/connection config but is that good enough?
18:40:00 superdan we fail to create a cell if you're duplicating that field, so I would think it would be yeah
18:40:08 mriedem about the template?
18:40:13 mriedem if the config is templated,
18:40:18 mriedem how do we do the lookup of the mapping?
18:40:26 superdan config isn't templated, the url in the db is
18:41:20 superdan well, I don't need to throw a wrench into this,
18:41:34 superdan I just... I dunno.. would rather not create a marker-based migration if we have any way to avoid it
18:42:11 mriedem we do have a way to avoid it if we start from the instance mappings rather than the instances, and query for instance mappings with queued_for_delete=NULL as you said
18:42:38 superdan yeah I know, that's what I'm saying :)
18:42:40 mriedem that would also make the migration essentially multi-cell aware if we pull the corresponding instances out of the cells for the mappings we found
18:42:53 superdan if we want to do that, then we can, yeah
18:43:05 superdan we have to have access to the api database to do this anyway,
18:43:13 superdan so it's not like we're adding a requirement to the config that we don't already have
18:44:01 mriedem it's essentially a re-write,
18:44:04 mriedem including the tests
18:44:10 superdan we could just iterate cell_mappings and only select instance_mappings per cell up to max_limit in any one go, so we don't have one batch that spans cells just to make the querying for instances easier
18:44:14 mriedem b/c you'd have to create mappings records directly in the database with null queued_for_delete values
18:44:32 superdan hmm?
18:44:39 superdan why/when?
18:44:39 mriedem like,
18:44:53 mriedem you can't just piggyback of the test that tssurya already wrote but change how the migratoin works,
18:45:01 mriedem because the functional test for what she has uses the objects
18:45:04 mriedem to populate the db
18:45:13 mriedem and creating the mapping from the object will make queued_for_delete not null
18:45:17 superdan oh you mean re-write of the tests
18:45:21 superdan not of the other patches in the series
18:45:26 mriedem yes the entire migration + tests would be a rewrite
18:45:40 superdan yup
18:45:49 superdan also,
18:46:09 superdan we could merge this and then change it later if we decide it's worth it, and just make sure to nuke any record that looks like that marker record
18:46:27 mriedem sure could
18:46:35 mriedem up to you if you want to rewrite this
18:47:13 mriedem i did have one concern with her patch and the marker,
18:47:22 mriedem which is there is handling for an InstanceMappingNotFound but it doesn't halt,
18:47:29 mriedem i'm not sure if it should or not,
18:47:31 superdan well, I don't *want* to rewrite it, but it seems like it's probably a good cost/benefit
18:47:35 mriedem but it would mean your marker could be f'ed up
18:48:24 mriedem like if we process instances a, b, c (missing mapping), d, e and then your marker is e
18:48:30 mriedem you don't have a way to get back to migrate c
18:48:41 mriedem but i'm not sure how likely that is to happen
18:48:52 superdan yeah so you do the whole thing again, which just seems like a good reason not to do marker-based migrations if we can ever help it
18:49:20 superdan we know cern had to nuke their marker and restart with the initial instance mapping stuff while they were migrating
18:50:48 superdan how about I spend an hour trying to write an alternative to this, I'll comment on this patch with that link and we'll decide by monday to either merge the existing patch or not?
18:51:05 superdan and if I decide in that hour that it
18:51:09 superdan isn't as easy,
18:51:15 superdan I'll just +W this as-is
18:51:40 mriedem wfm
18:52:49 mriedem leakypipes: melwitt: still need a final +W on that gate fix for the anti-affinity multi-create stuff (and the test change below it) https://review.openstack.org/#/c/583347/
18:52:53 mriedem as i said in the meeting, it's cleaner now
18:52:57 mriedem b/c i removed the alternates stuff
18:53:57 mriedem fried_rice: you should probably just +W this https://review.openstack.org/#/c/576693/ - i know you cleaned it up a bit but it's so trivial it's not really worth sitting on it
18:54:57 leakypipes mriedem: k, reviewing now.
18:55:02 fried_rice mriedem: I effectively changed the whole logic of it, which is why I hadn't felt great about pushing it myself.
18:55:54 mriedem yeah but it was like a 1 line conditional...
18:55:59 mriedem up to you
18:56:03 mriedem but i think it's ok in this case
18:56:04 mriedem personally
18:57:38 fried_rice Oh, and btw, superdan, `is_bfv = foo and bar or False` <== curse ovo
18:58:02 superdan um, what?
18:58:28 fried_rice The fact that you would ever have to say `x and y or False` rather than just `x and y` is stupid.
18:59:28 fried_rice mriedem: Is retrying 3rd party??
18:59:51 fried_rice I guess it must be, if it's in our requirements files...
18:59:56 fried_rice mahbad
19:02:21 mriedem superdan: for context on random things on your first day back https://review.openstack.org/#/c/580720/4/nova/compute/api.py@892
19:02:47 mriedem fried_rice: that wasn't an ovo thing
19:02:49 superdan fried_rice: that's a python thing. what does it have to do with ovo?
19:02:52 superdan yeah, that
19:02:55 mriedem it was because the bdm list doesn't need to have a root bdm
19:03:10 mriedem you can create a server with volumes attached to it which aren't the root disk
19:03:51 fried_rice No, it was because you can assign a False-ish non-boolean to a BooleanField, but if that False-ish is None, it freaks out when it tries to "coerce" it.

Earlier   Later