| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-24 | |||
| 19:22:59 | dansmith | we're not going to call it compute node, so unless edleafe just wants to trap flies, it might as well not be there :) | |
| 19:23:11 | edleafe | if you put "doughnut" in there I wouldn't mind, I just wouldn't vote for it | |
| 19:23:14 | efried | I was trying to figure out a way to settle on something without bikeshedding | |
| 19:23:22 | efried | Now we're bikeshedding about the poll itself. | |
| 19:23:26 | efried | Meta-bikeshedding. | |
| 19:39:51 | mriedem | dansmith: ok https://review.openstack.org/#/c/506420/ | |
| 19:42:43 | dansmith | mriedem: so, let me tell you why some of that is the way it is | |
| 19:42:52 | dansmith | you're going to tell me to clean it up | |
| 19:43:11 | dansmith | but, it's because in the next patch I move this out of the actual task and share it from live migration | |
| 19:43:24 | dansmith | any time I change something here it causes the next patch to conflict out like a mofo | |
| 19:43:34 | dansmith | so I've just been trying to avoid a big refactor | |
| 19:43:45 | dansmith | but I guess that's just not gonna fly | |
| 19:44:28 | mriedem | i hadn't powered ahead | |
| 19:44:33 | mriedem | i gave it my all on this one | |
| 19:44:44 | dansmith | yeah, I know | |
| 19:44:54 | dansmith | I'm just really burned out on this | |
| 19:45:32 | mriedem | couldn't we do a patch in between that handles the refactor to move it into a utils? | |
| 19:45:56 | mriedem | that's what i did when i moved the force=true claim in conductor stuff to scheduler_utils so evacuate and live migration could both use that | |
| 19:46:12 | dansmith | I should just squash out the refactor into this one since it adds stuff and then just removes it | |
| 19:46:33 | dansmith | I don't think there's any reason not to have it in one of the tasks, it just has to be accessible | |
| 19:46:51 | mriedem | i put that other thing here https://github.com/openstack/nova/blob/91f436d6c095573594f660438fe2c72e721d97ce/nova/scheduler/utils.py#L222 | |
| 19:46:57 | mriedem | but sure, it could live in the migrate task | |
| 19:47:14 | mriedem | we kind of end up with this allocationy utility stuff everywhere though | |
| 19:47:57 | dansmith | yeah but unless they're going to actually be shared, putting it in utils just makes that a big bundle of indirected methods | |
| 19:48:01 | dansmith | which doesn't help, IMHO | |
| 19:48:32 | mriedem | i'm not really following what you mean | |
| 19:48:38 | mriedem | either way is fine | |
| 19:48:52 | dansmith | I'd rather it be closer to the user, i.e. in one of the tasks | |
| 19:49:01 | mriedem | but yeah it seems the base patch should then have the utility code | |
| 19:49:38 | mriedem | either way i'd rather not deal with reviewing both the refactor and the live migration task using it in the same change, | |
| 19:49:41 | mriedem | we could split that | |
| 19:49:52 | dansmith | I'm squashing it into the first patch, | |
| 19:49:56 | mriedem | ok | |
| 19:49:57 | dansmith | so it just adds the utility methods and uses them | |
| 19:50:17 | dansmith | and either I'll push it up that way and try to apply your concerns to the refactored code, | |
| 19:50:27 | dansmith | or you won't have heard from me by friday and should pick this up | |
| 19:50:32 | dansmith | and give my cellsv2 talk at SYD | |
| 19:50:50 | mriedem | i'm happy to pick up addressing my comments on this, | |
| 19:50:56 | mriedem | none of them are hard to handle | |
| 19:50:59 | dansmith | no | |
| 19:51:02 | dansmith | I'll do it | |
| 19:51:21 | mriedem | i mean after you rev | |
| 19:51:33 | mriedem | if there is something missed i'll just do it myself, or on top | |
| 19:51:44 | dansmith | okay | |
| 20:02:11 | openstackgerrit | Hongbin Lu proposed openstack/nova master: placement: add API reference for create inventory https://review.openstack.org/511342 | |
| 20:22:56 | mriedem | this trap has been laid since 2014 https://bugs.launchpad.net/nova/+bug/1726871 | |
| 20:22:58 | openstack | Launchpad bug 1726871 in OpenStack Compute (nova) "AttributeError: 'BlockDeviceMapping' object has no attribute 'uuid'" [High,Triaged] - Assigned to Matt Riedemann (mriedem) | |
| 20:48:31 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix AttributeError in BlockDeviceMapping.obj_load_attr https://review.openstack.org/514825 | |
| 20:49:41 | mriedem | dansmith: melwitt: ^ easy fix for a regression i introduced in pike | |
| 20:50:54 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Fix AttributeError in BlockDeviceMapping.obj_load_attr https://review.openstack.org/514825 | |
| 20:53:54 | openstackgerrit | Dan Smith proposed openstack/nova master: Make migration uuid hold allocations for migrating instances https://review.openstack.org/506420 | |
| 20:53:55 | openstackgerrit | Dan Smith proposed openstack/nova master: Make live migration hold resources with a migration allocation https://review.openstack.org/507638 | |
| 20:54:06 | dansmith | mriedem: seams were cleaner than I thought ^ | |
| 20:57:23 | melwitt | mriedem, dansmith: as a general rule, if we do obj_reset_changes on an ObjectField, we should use recursive=True? | |
| 20:57:36 | melwitt | just trying to understand that bit | |
| 20:58:09 | mriedem | melwitt: not sure honestly, because i found a similar problem in the instance object yesterday | |
| 20:58:48 | mriedem | this https://github.com/openstack/nova/blob/master/nova/objects/instance.py#L563 | |
| 20:59:06 | mriedem | after you create an instance object, ec2_ids is still marked as changd | |
| 20:59:09 | mriedem | *changed | |
| 20:59:26 | mriedem | however, a later call to instance.save() won't care https://github.com/openstack/nova/blob/master/nova/objects/instance.py#L650 | |
| 21:00:23 | melwitt | yeah, I'm guessing the "what_changed" logic recurses, checking | |
| 21:00:24 | mriedem | melwitt: in my bdm patch, if you remove recursive=True and replace https://review.openstack.org/#/c/514825/2/nova/tests/unit/objects/test_block_device.py@391 with bdm.save(), you'll see the failure | |
| 21:00:30 | mriedem | it does | |
| 21:01:07 | melwitt | okay, that's why then. I was just thinking (and you already saw) I bet we have other cases of this around in the code | |
| 21:09:09 | openstackgerrit | Jackie Truong proposed openstack/nova master: Implement certificate_utils https://review.openstack.org/479949 | |
| 21:11:32 | mriedem | dansmith: do you want me to address the stuff in https://review.openstack.org/#/c/506420/ ? | |
| 21:13:46 | dansmith | mriedem: no | |
| 21:14:25 | dansmith | melwitt: we should call recursive=True if we've caused all the downstream objects to be saved (or ignored if they have nothing to save) | |
| 21:14:46 | dansmith | it's a detail that came out kinda late in the objectification work so we didn't have a specific pattern from the beginning | |
| 21:14:58 | tonyb | mriedem: the EOL is held up on tooling so it you -W the releases request we can get that last change in | |
| 21:15:15 | mriedem | tonyb: already done, just need the newton change +W | |
| 21:15:21 | mriedem | been waiting for you to wake up | |
| 21:15:32 | mriedem | https://review.openstack.org/#/c/514685/ | |
| 21:15:44 | melwitt | dansmith: okay. so in a lazy-load situation we would always want recursive=True if we've freshly loaded an ObjectField | |
| 21:15:45 | tonyb | mriedem: :) | |
| 21:16:25 | dansmith | melwitt: well, if we've done that, the nested object should have no pending changes and thus it wouldn't matter | |
| 21:17:04 | melwitt | dansmith: mriedem saw that if he didn't use recursive=True it's flagging the bdm.instance field as changed? | |
| 21:17:25 | melwitt | I wonder if there's a different bug in Instance then? | |
| 21:17:55 | dansmith | mriedem: what in the scheduler client init do you think is so heavy that we shouldn't be doing it again in that utility method? | |
| 21:18:17 | tonyb | mriedem: I'll need more coffee before I vote on that. | |
| 21:18:32 | mriedem | dansmith: it's not real heavy, it's just loading up two other objects, just seems unnecessary | |
| 21:19:07 | mriedem | tonyb: just keep in mind you +Wed the same change in stable/ocata | |
| 21:19:15 | dansmith | mriedem: I just hate all our methods that take 27 arguments and then do a ton of inefficient shit anyway, but alright | |
| 21:19:31 | dansmith | melwitt: yeah, I dunno about that fix tbh, but haven't looked closely | |
| 21:20:53 | mriedem | dansmith: oh well maybe it's not as inefficient as i thought, now that i look at LazyLoader | |
| 21:21:04 | mriedem | i guess the queryclient thing in there won't even be loaded unless it's called | |
| 21:21:12 | mriedem | anyway, that's not what my -1 was for | |
| 21:21:19 | tonyb | mriedem: s/+W/+2/ but I take your point. I'm trying to workout how doing this last minute helps. I'm on the fence ATM | |
| 21:21:50 | mriedem | tonyb: belmiro from cern was asking this morning if he should be configuring placement_database when planning the upgrade from newton to ocata | |
| 21:21:55 | mriedem | because it's in the newton code and says to use it | |
| 21:22:24 | mriedem | tonyb: so for those that are <newton and going to get here eventually, we want to avoid the confusion before those newton deployments start thinking about going to >=ocata | |
| 21:22:50 | tonyb | mriedem: okay | |
| 21:39:08 | tonyb | mriedem: Just to be certain (and I shoudl have asked this for the ocata backport) ... if someone is running newton and set those options and we remove themi, they upgrade to $last_release ... the config file still loads right? | |
| 21:40:21 | mriedem | tonyb: i think you can put whatever junk you want in the config file | |
| 21:40:36 | mriedem | i haven't tested that scenario though | |
| 21:40:59 | mriedem | don't have a devstack handy either, but would think you could put [DEFAULT]foo=bar in nova.conf and restart n-cpu and it'd be fine | |
| 21:41:20 | dansmith | mriedem: correct | |
| 21:41:30 | dansmith | mriedem: our support people LOVE it | |
| 21:41:32 | dansmith | (not) | |
| 21:41:46 | dansmith | I've closed bugs before with "you typo'd that thing, which is why it's not working" | |