| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-20 | |||
| 18:31:08 | mriedem | which uses a marker | |
| 18:31:28 | superdan | the creating of the reqspec? | |
| 18:31:30 | mriedem | yeah | |
| 18:31:33 | superdan | yeah, | |
| 18:31:40 | mriedem | i'm not sure that queued_for_delete will be NULL will it? | |
| 18:31:49 | mriedem | maybe it will, i forget how the defaults work in the new column | |
| 18:31:59 | superdan | this is different since we've already got all the records and we're just patching up | |
| 18:32:01 | superdan | it's not nullable=False, so I think it defaults to being nullable | |
| 18:32:02 | superdan | default is not server_default, | |
| 18:32:04 | superdan | so it's not in the schema AFAIK | |
| 18:32:42 | mriedem | so for every instance mapping with a NULL queued_for_delete, lookup it's instance and see if the instance is deleted (or soft deleted)? | |
| 18:33:09 | superdan | "Defaults to True unless primary_key is also True" | |
| 18:33:28 | superdan | you can still do bulk queries of each I think | |
| 18:33:47 | mriedem | by instance uuid? | |
| 18:34:05 | mriedem | so gimme 50 instance mappings with queued_for_delete=NULL, | |
| 18:34:06 | superdan | yeah, so you query out max_limit mappings, | |
| 18:34:11 | mriedem | right | |
| 18:34:17 | mriedem | and those instances might be spread across multiple cells | |
| 18:34:20 | superdan | and then do a get_by_filters({'uuid': uuids}) | |
| 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 | |