| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-20 | |||
| 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 | |
| 19:56:15 | superdan | so hopefully that's enough to decide if we want to pursue or not | |
| 19:59:13 | mriedem | comments inline | |
| 19:59:22 | mriedem | taint bad | |
| 20:01:25 | superdan | we'll see what tssurya thinks | |
| 20:02:35 | mriedem | also need to test the limit | |
| 20:02:39 | mriedem | but those are my 2 big points | |
| 20:02:52 | superdan | yeah the limit was the missing gap I was thinking of | |
| 20:03:17 | mriedem | obligatory https://www.youtube.com/watch?v=vT8OU5WtfkQ | |
| 20:03:35 | mriedem | i want to be tony, you can be that other guy | |
| 20:04:03 | tssurya | well, considering you are having to change it to NULL from False, as per the commit message means the values were False by default right ? | |
| 20:04:28 | superdan | tssurya: no | |
| 20:04:39 | superdan | tssurya: they're NULL in the db, but SQLA hides it from us | |
| 20:05:05 | tssurya | ah, okay yes this is what you were saying before I got disconnected | |
| 20:05:48 | tssurya | sorry, as for the technique I don't have any issues in making this multi-cell aware | |
| 20:14:42 | superdan | mriedem: hmm, stumped on the soft_deleted=True part | |
| 20:14:51 | superdan | I'm not gettin those instances back from my filter query | |
| 20:15:29 | superdan | but I think I do if I don't provide a uuid filter... | |
| 20:17:29 | mriedem | superdan: see the test in the other patch? | |
| 20:17:39 | mriedem | you need to set the vm_state on a couple of the servers | |
| 20:17:46 | mriedem | non-deleted services | |
| 20:17:54 | mriedem | *servers | |
| 20:18:00 | superdan | right, | |
| 20:18:01 | superdan | I'm saying I do that and I don't get them back from my query | |
| 20:18:28 | melwitt | are you talking about this logic? https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L2105 | |
| 20:19:32 | superdan | generally, yes, but I'm saying I don't get the instances back that I expect if I include a uuid= filter as well | |
| 20:20:22 | melwitt | if you use 'deleted': False you won't get soft_deleted instances | |
| 20:20:40 | superdan | yes, this is deleted=True, soft_deleted=True | |
| 20:20:51 | melwitt | okay | |
| 20:20:59 | mriedem | should be an OR | |
| 20:21:05 | mriedem | https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L2112 | |
| 20:21:12 | superdan | yup | |
| 20:21:36 | mriedem | oh the uuid filter, hmm | |
| 20:21:45 | mriedem | you don't get the soft deleted instances or any instances? | |
| 20:22:04 | superdan | I get some but not all, let me see which ones specifically | |
| 20:22:47 | superdan | yeah I get the non-soft-deleted ones | |
| 20:23:25 | mriedem | should handle your UUID list here https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api.py#L2426 | |
| 20:23:34 | superdan | yeah | |
| 20:24:01 | superdan | okay I actually don't think the uuid filter is breaking me, I'm just getting both deleted instances in that case, | |
| 20:24:07 | superdan | but not the soft_deleted ones ever | |
| 20:27:28 | superdan | hmm, my .save wasn't ... saving | |