| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-11-30 | |||
| 16:09:29 | mdbooth | dansmith: I did :) | |
| 16:09:43 | mdbooth | I didn't forget about migration uuid, though | |
| 16:09:55 | mriedem | mdbooth: that was commented in here yes https://review.openstack.org/#/c/242603/23/nova/objects/block_device.py | |
| 16:10:01 | mdbooth | Most of the changes in there are pretty obvious, though | |
| 16:10:03 | mriedem | mdbooth: it's just a matter of cleaning up those old patches a bit | |
| 16:10:44 | mriedem | there are other reviews that have referenced those at this point, so i'd prefer to use those | |
| 16:10:49 | mriedem | plus, original author and all that | |
| 16:12:40 | mriedem | dansmith: "but if we're going to implement the full keys/dirs approach at the bottom, then maybe we need to" yeah that's where i'm at | |
| 16:12:46 | mriedem | i.e., | |
| 16:13:04 | mriedem | is it easier to just copy the instance_list stuff, or retool the marker encoding behavior for this api | |
| 16:13:47 | dansmith | well, there's three options I think: | |
| 16:13:49 | mdbooth | dansmith: Anyway, I would fix up the old patch by writing my patch instead. All the obvious bits are identical, and all the extra bits are required but not present in the older patch. | |
| 16:13:52 | dansmith | 1. retool instance_list to be usable by migrations | |
| 16:13:56 | dansmith | 2. Copy and modify | |
| 16:14:18 | dansmith | 3. Implement something limited from that approach that is slimmer and specific to migrations | |
| 16:14:21 | mdbooth | dansmith: Some bits of it are very similar, but we've written way more similar patches before :) | |
| 16:14:34 | mdbooth | I forgot about it. However, the work's now done. | |
| 16:14:42 | openstackgerrit | melanie witt proposed openstack/nova master: Remove old-style quotas code https://review.openstack.org/511689 | |
| 16:14:43 | openstackgerrit | melanie witt proposed openstack/nova master: Follow up on removing old-style quotas code https://review.openstack.org/524234 | |
| 16:14:58 | mdbooth | I can either throw the work away, or... not the work away. | |
| 16:15:09 | dansmith | huh? | |
| 16:15:29 | dansmith | my patch has history back to 2015 on it.. I just think it makes sense to do the uuid field and object change in that review so we keep that context | |
| 16:15:31 | dansmith | not sure why we wouldn't | |
| 16:15:48 | dansmith | I think that's all mriedem is saying, and I agree | |
| 16:16:00 | mdbooth | Ok, but those parts of my patch are so similar to your patch... | |
| 16:16:04 | mdbooth | What do you want me to do? | |
| 16:16:13 | mdbooth | Rebase my patch on your patch and subtract the older work? | |
| 16:16:17 | dansmith | your patch also puts the full online migration change in with the schema change, which just makes it bigger | |
| 16:17:03 | mdbooth | dansmith: There's a reason for that. | |
| 16:17:18 | mdbooth | Although I could split it. | |
| 16:17:32 | dansmith | what's the reason? | |
| 16:17:47 | mdbooth | I was thinking that you'd want to run the online migration asap after the schema change, because it's more efficient. | |
| 16:18:05 | dansmith | but that doesn't fit the way we do these, | |
| 16:18:17 | dansmith | because you have to be able to apply the schema change before rolling any code | |
| 16:18:22 | mdbooth | I can split it, it's no hassle. | |
| 16:18:23 | dansmith | if you land that schema change before any code to handle it, | |
| 16:18:31 | dansmith | then it kinda self-tests that you haven't broken anything | |
| 16:18:36 | dansmith | we've done a lot of these | |
| 16:18:42 | melwitt | mriedem: that's really weird what you're seeing in that func test. I'm gonna look into it more today. yesterday I didn't find what's going wrong yet | |
| 16:18:55 | melwitt | (what's going wrong with CellDatabases) | |
| 16:18:59 | mdbooth | dansmith: I'm talking about efficiency on a large deployment | |
| 16:19:11 | dansmith | um, me too? | |
| 16:19:24 | mdbooth | You run the schema change, you roll the code, then you want to run the online migration asap | |
| 16:19:33 | mdbooth | But you don't have to, it's just more efficient if you do | |
| 16:19:46 | mdbooth | Anyway, I can split that out | |
| 16:19:47 | dansmith | how does that affect whether the patch is large or small? | |
| 16:20:00 | mdbooth | My change also makes the field non nullable in the object | |
| 16:20:30 | mriedem | umm | |
| 16:20:33 | dansmith | what does that, or the other things have to do with re-using the review for context? | |
| 16:20:36 | mriedem | https://review.openstack.org/#/c/242603/23/nova/objects/block_device.py is already non-nullable | |
| 16:21:06 | mriedem | mdbooth: seriously just invest the time on splitting up your duplicate change and just rebase and address gaps in the older patches | |
| 16:21:34 | mdbooth | mriedem: Ok. It does seem like a bunch of busy work for little gain, though. | |
| 16:21:42 | mdbooth | I just forgot about the old review. | |
| 16:22:07 | mdbooth | I'm going to have to reconcile any minor changes with a bunch of code I've already written and tested. | |
| 16:22:41 | mriedem | how about i take over dan's patches then | |
| 16:22:47 | mriedem | will everyone be happy with that solution? | |
| 16:23:10 | dansmith | mriedem: so I'm going to try to strip out the instanceness of the list routines and see how bad that is in order to make a call on the migration stuff | |
| 16:23:16 | mdbooth | mriedem: No, because I'm working on this actively right now, including a ton of follow-on | |
| 16:23:24 | mdbooth | If anybody's going to do the busy work, it needs to be me | |
| 16:23:28 | mdbooth | It's just a pita | |
| 16:33:04 | AJaeger | nova cores, please review https://review.openstack.org/520373 to update relesaenotes to allow infra to improve the job | |
| 16:34:03 | AJaeger | yummy | |
| 16:37:24 | mriedem | melwitt: i was doing something wrong in my assertion code | |
| 16:37:33 | mriedem | i wasn't using the targeted context to query the compute nodes out of the cell | |
| 16:37:36 | mriedem | seeing if that changes tihngs | |
| 16:38:24 | stephenfin | AJaeger: -1 for use of double quotes ;) | |
| 16:38:30 | stephenfin | AJaeger: I kid. +2 | |
| 16:38:45 | AJaeger | stephenfin: ;9 | |
| 16:40:03 | mriedem | nope still fails | |
| 16:40:06 | mriedem | host2 shows up in cell1 | |
| 16:41:21 | melwitt | sad town :( | |
| 16:41:46 | mriedem | looking at CellDatabasesTestCase i'm not sure why this doesn't work | |
| 16:41:50 | mriedem | except, | |
| 16:42:02 | mriedem | when the compute node record is created, it's via the start of the service, | |
| 16:42:07 | mriedem | which has an untargeted context, | |
| 16:42:16 | mriedem | so it's going to create the service/computenode in the default context, which is cell1 | |
| 16:42:18 | mriedem | so i guess that's why | |
| 16:42:46 | mriedem | to hack around this, i'd have to create the compute node records in the cells before actually starting the services | |
| 16:43:00 | mriedem | or allow temporarily mutating the default context in the CellDatabase fixture | |
| 16:43:21 | openstackgerrit | Eric Fried proposed openstack/nova master: Nix log translations from scheduler.client.report https://review.openstack.org/524245 | |
| 16:43:27 | melwitt | well, what I did in my patch is wrap the Service class and target the context during start | |
| 16:44:02 | melwitt | if that's not working then I messed up what I tried to do | |
| 16:46:17 | kashyap | dansmith: When you get a moment, maybe you'd know top off your head -- during 'evacuate', certainly CPU model compatibility is checked before an instance is moved away, yes? | |
| 16:46:45 | dansmith | kashyap: if we run the scheduler then I assume so | |
| 16:46:45 | mriedem | kashyap: would depend on the scheduler filters | |
| 16:46:56 | mriedem | unless, of course, you specify a host with the force option | |
| 16:46:56 | dansmith | if a host is provided, I don't think so | |
| 16:47:02 | mriedem | then we bypass the scheduler altogether | |
| 16:47:14 | mriedem | it's not the same | |
| 16:47:16 | mriedem | evacuate is a move | |
| 16:47:16 | kashyap | mriedem: Yep, scheduler filters is what I was going toc heck | |
| 16:47:17 | mriedem | rebuild is not | |
| 16:47:42 | mriedem | you can evacuate with a target host and if you don't specify the force flag, we'll validate that host against the scheduler filters, | |
| 16:47:50 | mriedem | if you specify a host + force, we don't run through the scheduler filters | |
| 16:47:59 | mriedem | because you've decided you want to shoot yourself in the foot | |
| 16:48:04 | mriedem | and we hand you the gun to do that | |
| 16:48:47 | kashyap | dansmith: mriedem: Do you have a pointer to the relevant Scheduler code? libvirt / QEMU has reworked how CPU models are detected, so I'm trying to investigate what action items are there on the Nova side | |
| 16:48:55 | mriedem | if you rebuild (not evacuate) with a new image, well, then it depends on which roll of the cve dice you've landed on | |
| 16:48:55 | kashyap | mriedem: Heh, noted | |
| 16:49:15 | mriedem | kashyap: the compute capabilities filter | |