| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-20 | |||
| 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 | mriedem | like, | |
| 18:44:39 | superdan | why/when? | |
| 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. | |
| 19:05:50 | fried_rice | I would buy it if you could only assign True rather than any other True-ish (except I still would have expected the BooleanField to do the casting). But it doesn't care if you assign other True-ish values, like whatever's in `root`. | |
| 19:06:28 | superdan | fried_rice: that's because it's not nullable and ovo specifically checks for None-ness | |
| 19:06:45 | superdan | given how often boolean coercions confuse things, I don' | |
| 19:06:50 | superdan | don't think I'd "curse ovo" for that :) | |
| 19:07:21 | mriedem | i would curse your bones availability zones | |
| 19:09:28 | fried_rice | OVOClass(non_nullable_bool_field=None) can be distinguished from OVOClass() ('non_nullable_bool_field' in kwargs, kind of thing) which might make more sense from a consumer standpoint. | |
| 19:10:18 | fried_rice | I suppose ovoinstance = None is the established mnemonic for "null this field". | |
| 19:10:35 | fried_rice | Is it possible for a BooleanField to be nullable? | |
| 19:10:38 | fried_rice | The whole thing is gross. | |
| 19:12:30 | tssurya | superdan, mriedem: sorry having a bad internet connection, but.. by default queued_for_delete would be False right ? not NULL.. | |
| 19:12:42 | superdan | tssurya: on the object, but not in the database | |
| 19:13:06 | superdan | tssurya: your default=False in the model schema doesn't mean it's actually set that way in SQL, only in SQLA's view | |
| 19:15:32 | tssurya | I thought I remembered my devstack DB to have False by default after the migration.. | |
| 19:46:28 | openstackgerrit | Matt Riedemann proposed openstack/nova master: compute: Ensure pre-migrating instances are destroyed during init_host https://review.openstack.org/562284 | |
| 19:54:02 | openstackgerrit | Dan Smith proposed openstack/nova master: Online data migration for queued_for_delete flag https://review.openstack.org/584504 | |
| 19:54:04 | superdan | mriedem: tssurya ^ | |
| 19:54:29 | mriedem | that can't be correctly done, it was too fast | |
| 19:54:42 | superdan | definitely not | |
| 19:55:05 | superdan | I think I need at least one more test for an edge case there, | |
| 19:55:18 | superdan | but I was shooting to replicate the functional test in the existing patch | |
| 19:56:00 | superdan | and going fast because I feel bad for throwing a wrench into things at this point | |