| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-06-22 | |||
| 13:45:31 | mriedem | artom: are you fixing the unit tests on that series? | |
| 13:45:40 | mriedem | the tagged attach one i mean | |
| 13:45:49 | mriedem | although i guess it's after midnight for you now | |
| 13:46:32 | openstackgerrit | Zhenyu Zheng proposed openstack/nova master: Don't delete neutron port when attach failed https://review.openstack.org/403568 | |
| 13:51:08 | artom | mriedem, no, it's 16:50 | |
| 13:51:18 | artom | mriedem, I was just about to address your comments | |
| 13:51:19 | mriedem | oh heh, well i'm just fixing this up | |
| 13:51:24 | mriedem | or you can | |
| 13:51:37 | artom | How far along are you? I've yet to open my editor | |
| 13:51:43 | mriedem | just need to run the tests | |
| 13:51:54 | mriedem | i'll finish it | |
| 13:52:02 | artom | mriedem, cool, thanks! | |
| 13:53:12 | dane-fichter | dansmith: Could you take a look at https://review.openstack.org/#/c/457711/8 when you get a chance? I've resolved the unit test issues, so I think it's ready to go. | |
| 13:53:42 | stvnoyes | mriedem: thanks for the review on the migrate change, i have a couple of questions | |
| 13:54:00 | dansmith | dane-fichter: no blueprint link? | |
| 13:54:36 | stvnoyes | i was originally going to use vol_id as the key in that old_attachments dict, but saw that nova allows you to connect a volume multiple times to the same instance. Is that a bug or a feature? | |
| 13:55:00 | stvnoyes | if it's a bug, then I can use the vol_id as a key. | |
| 13:55:38 | mriedem | stvnoyes: by connect do you mean calling os-brick.connect_volume or os-initialize_connection in cinder? | |
| 13:55:46 | dane-fichter | dansmith: it's this one: https://blueprints.launchpad.net/nova/+spec/nova-validate-certificates | |
| 13:55:58 | mriedem | stvnoyes: either way the volume_id and bdm.id are for the same bdm record | |
| 13:56:10 | dansmith | dane-fichter: needs linkage in the commit message | |
| 13:56:27 | mriedem | stvnoyes: at least i think so | |
| 13:56:31 | dane-fichter | dansmith: ok. I'll link it. | |
| 13:56:42 | stvnoyes | i was concerned that if the volume was connected multiple times to the instance, we could swap the attachment_ids bentween the bdms on rollback | |
| 13:56:53 | dansmith | dane-fichter: the spec isn't merged though right? | |
| 13:57:03 | mriedem | stvnoyes: but i think it's the same bdm record regardless | |
| 13:57:25 | mriedem | otherwise when you get the bdm by volume id and instance uuid, you'd have more than one which would be a failure | |
| 13:57:34 | stvnoyes | mriedem: ok I see what you mean. | |
| 13:57:42 | mriedem | i do think we connect on the dest and the source at the same time | |
| 13:57:44 | mriedem | during the migration | |
| 13:57:55 | mriedem | and then if successful, we disconnect from the source host in post live migration | |
| 13:58:07 | dane-fichter | dansmith: The spec has merged. See https://review.openstack.org/#/c/357151/ | |
| 13:58:12 | mriedem | johnthetubaguy really knows the internals of this | |
| 13:58:32 | stvnoyes | btw, it does seem to be a bug to allow a volume to attach to the same instance multiple times. Things definitely break when the volume is unmounted. you lose both attachments. | |
| 13:58:35 | stvnoyes | in cinder | |
| 13:58:58 | dansmith | dane-fichter: ah, this is linked from the BP: https://review.openstack.org/#/c/441538/ | |
| 13:59:01 | stvnoyes | I guess I'll file a bug... | |
| 13:59:02 | dansmith | which is abandoned I se | |
| 13:59:04 | dansmith | *see | |
| 13:59:30 | dane-fichter | dansmith: Dammit. I'll fix that too. | |
| 13:59:58 | mriedem | artom: maybe i will let you handle this one, because there are some other things to do here | |
| 14:00:04 | artom | mriedem, sure | |
| 14:00:05 | dansmith | dane-fichter: anyway, I have no context on this so it'd take me some time to get up to speed and be able to review it and there's a lot of important stuff flying around. So, I'll add it to the queue, but... | |
| 14:00:42 | dansmith | dane-fichter: jaypipes is very busy with those same things, so you might try to get johnthetubaguy to look at it first | |
| 14:01:43 | mriedem | artom: ok comments posted | |
| 14:01:48 | mriedem | some issues with the tests | |
| 14:02:46 | dane-fichter | dansmith: ok | |
| 14:02:57 | artom | mriedem, I'm on it | |
| 14:03:04 | dane-fichter | this change is really simple and doesn't require a ton of context | |
| 14:06:24 | openstackgerrit | dane-fichter proposed openstack/nova master: Add trusted certificates to InstanceExtras https://review.openstack.org/457711 | |
| 14:06:28 | stvnoyes | mriedem: one other question. I like the idea of putting old_attachments in the base class LiveMigrateData. But I didn't see any examples of how to add new fields to a base class, they don't seem to be versioned. Do you know of an example where something like that was done? | |
| 14:08:06 | mriedem | dane-fichter: see comments in PS9 | |
| 14:08:16 | mriedem | *PS8 | |
| 14:08:23 | mriedem | dane-fichter: missing some tests | |
| 14:08:24 | dansmith | dane-fichter: for sure, but it's adding something to the data model which is important, and I see no dependent patches that actually use it | |
| 14:08:34 | dansmith | dane-fichter: so knowing the full context of what the plan here is is.. kindof important | |
| 14:09:31 | mriedem | stvnoyes: i believe if you add a field to LiveMigrateData, it will require a version bump on each subclass that is versioned | |
| 14:09:41 | mriedem | stvnoyes: if it doesn't, we'll have to ask dansmith | |
| 14:10:17 | dansmith | mriedem: yeah, I think so.. I don't think we've had to do that yet, but yeah.. | |
| 14:10:37 | stvnoyes | ok thanks. I'll give that a try. | |
| 14:14:05 | ftersin | mdbooth: hi. are you around? | |
| 14:16:45 | mriedem | dansmith: melwitt: are these good for review now? https://review.openstack.org/#/c/428481/7 | |
| 14:16:52 | mriedem | i wasn't sure if we were holding up for placement things or something | |
| 14:18:38 | mriedem | johnthetubaguy: if/when you're around, the pike review priorities are up and would be nice to get in https://review.openstack.org/#/c/470420/ | |
| 14:18:52 | mriedem | jaypipes: dansmith: ^ you too | |
| 14:19:01 | mriedem | super late this cycle | |
| 14:19:45 | jaypipes | mriedem: done | |
| 14:21:37 | mriedem | thanks suge | |
| 14:22:05 | mriedem | i'm going to play a sassy southerner from the 30s today | |
| 14:24:03 | alex_xu | mriedem: do you know why we need to mock this method https://review.openstack.org/#/c/459593/30/nova/tests/unit/conductor/test_conductor.py@1419? | |
| 14:24:52 | alex_xu | mriedem: I remember that mock, it raise InstanceNotFound, but why the test can get InstanceAction and Tags from the db... | |
| 14:24:59 | alex_xu | s/I remember/I remove/ | |
| 14:27:13 | mriedem | alex_xu: you mean why do the instance tag db api methods call _check_instance_exists_in_project(context, instance_uuid) first? | |
| 14:27:53 | alex_xu | mriedem: no, I mean why we can't remove the mock of '_check_instance_exists_in_project' | |
| 14:28:04 | mriedem | alex_xu: that would be a question for Kevin_Zheng :) | |
| 14:29:20 | alex_xu | mriedem: yea, :) Kevin_Zheng and I talk about that, but we found if we remove that mock, that method raise NotFound. just want check with you is there any mock for the instance db call which we are really missing | |
| 14:31:11 | mriedem | alex_xu: oh, well that is checking to make sure that the instance belongs to the same project that's in the context | |
| 14:31:17 | mriedem | so maybe the test setup is missing something? | |
| 14:32:51 | alex_xu | mriedem: yea, let me check that | |
| 14:32:58 | Kevin_Zheng | but we can get instance_action and instance_tags correctly | |
| 14:33:31 | mriedem | do you have a stacktrace from when it blows up if you remove the mock? | |
| 14:33:43 | mriedem | i do'nt really have context here without actually pulling the change down and testing it out myself | |
| 14:33:54 | Kevin_Zheng | yeah I have | |
| 14:37:22 | mriedem | dansmith: btw, i think this is the extent of what's needed for paging compute nodes by uuid http://paste.openstack.org/show/613404/ | |
| 14:37:24 | Kevin_Zheng | http://paste.openstack.org/show/613405/ | |
| 14:37:24 | mriedem | seems too easy | |
| 14:37:52 | dansmith | mriedem: I'm in meetings for the next two hours | |
| 14:38:05 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Update api-ref to indicate swap param https://review.openstack.org/474517 | |
| 14:38:24 | mriedem | Kevin_Zheng: you're using nosetests? :) | |
| 14:38:44 | Kevin_Zheng | my code is a little bit wrong, the loop is for cell in cells, and first loop is cell0, so it should be not found, but I still cannot get it in cell1 | |
| 14:38:46 | Kevin_Zheng | yes | |
| 14:39:21 | Kevin_Zheng | It still happens using tox | |
| 14:40:19 | Kevin_Zheng | Alex said he used tox | |
| 14:40:33 | mriedem | yeah i don't think that's related, i just didn't know people were still using nose is all | |
| 14:41:13 | openstackgerrit | Alex Szarka proposed openstack/nova master: [WIP]Transform instance-live_migration_abort notification https://review.openstack.org/420453 | |
| 14:41:18 | Kevin_Zheng | :P | |
| 14:41:28 | Kevin_Zheng | https://www.irccloud.com/pastebin/b1GCjanL/ | |
| 14:42:20 | Kevin_Zheng | here, instanceActionList can be correctly get in cell1, but instance can't | |
| 14:42:56 | mriedem | Kevin_Zheng: InstanceActionList.get_by_instance_uuid isn't using _check_instance_exists_in_project in the db api though | |
| 14:43:07 | mriedem | so it has to be some issue with the actual project_id set on the instance in the cell db | |