| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 18:10:55 | dansmith | heh | |
| 18:14:47 | openstackgerrit | Rafael Folco proposed openstack/nova master: DELETE all inventory for a resource provider https://review.openstack.org/416669 | |
| 18:36:53 | mdbooth | ... when you realise you created dicts called host_cell_mapping_mapping and cell_mapping_host_mapping | |
| 18:40:08 | cburgess | OMG | |
| 18:40:18 | cburgess | You should be taken out back and flogged for that. | |
| 18:40:27 | mdbooth | cburgess: Done ;) | |
| 18:40:44 | efried | mdbooth Clearly the former is mapping the mappings of hosts to cells, and the latter is mapping the .... sh!t, I got nothing. | |
| 18:41:02 | mdbooth | hosts to cell mappings, and cell mappings to hosts | |
| 18:41:22 | mdbooth | It made sense when I wrote it | |
| 18:41:52 | cburgess | Where you are a ballmer peak when you wrote that? | |
| 18:43:10 | openstackgerrit | Lee Yarwood proposed openstack/nova master: objects: Add attachment_id to BlockDeviceMapping https://review.openstack.org/437665 | |
| 18:43:10 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Move detach logic from manager into driver BDM https://review.openstack.org/439520 | |
| 18:43:11 | openstackgerrit | Lee Yarwood proposed openstack/nova master: db: Add attachment_id to block_device_mapping https://review.openstack.org/437597 | |
| 18:43:11 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Only destroy BDMs after successful detach call https://review.openstack.org/440693 | |
| 18:43:12 | openstackgerrit | Lee Yarwood proposed openstack/nova master: Add uuid column to BlockDeviceMapping https://review.openstack.org/242602 | |
| 18:43:12 | openstackgerrit | Lee Yarwood proposed openstack/nova master: Make BlockDeviceMapping object support uuid https://review.openstack.org/242603 | |
| 18:43:13 | openstackgerrit | Lee Yarwood proposed openstack/nova master: compute: Use detach_volume during default rebuild https://review.openstack.org/443932 | |
| 18:57:09 | openstackgerrit | Dan Smith proposed openstack/nova master: Make discover_hosts only query for unmapped ComputeNode records https://review.openstack.org/427903 | |
| 18:57:10 | openstackgerrit | Dan Smith proposed openstack/nova master: Add ComputeNode.mapped field https://review.openstack.org/427901 | |
| 18:57:10 | openstackgerrit | Dan Smith proposed openstack/nova master: Add ability to query for ComputeNodes by their mapped value https://review.openstack.org/427902 | |
| 19:01:41 | oomichi | mriedem: sdague: is nova-network deprecated already? I guess so, but I cannot find clear doc on nova repo | |
| 19:02:47 | mriedem | oomichi: since newton | |
| 19:03:03 | mriedem | oomichi: the config options, release notes, starting the service, etc etc | |
| 19:03:14 | mriedem | the official manuals probably don't say anything about it | |
| 19:03:42 | oomichi | mriedem: thanks, I got it. reno is good for me | |
| 19:05:06 | mriedem | asettle: so https://docs.openstack.org/admin-guide/compute-networking-nova.html | |
| 19:05:16 | openstackgerrit | Jay Pipes proposed openstack/nova master: virt: implement get_inventory() for Ironic https://review.openstack.org/441544 | |
| 19:05:25 | jaypipes | cdent: fixed up per your review ^^ dansmith, you were already +2 on it :) | |
| 19:05:30 | mriedem | asettle: nova-network has been deprecated since newton, and in ocata we made it such that you can only run nova-network in a cells v1 configuration (which is not recommended for new deployments either) | |
| 19:05:39 | mriedem | asettle: i guess we need a big fat warning in that page now right? | |
| 19:06:04 | cdent | thank you jaypipes, I felt meh about dissing dansmith's +2, but seemed better to get it now instead of later | |
| 19:06:20 | jaypipes | no worries, those were good comments, cdent. | |
| 19:06:46 | cdent | i'm in review mode at the moment, so should get back round to the new version shortly | |
| 19:09:55 | cdent | that means you win mdbooth | |
| 19:10:28 | mdbooth | It's a mechanical change. I just added an additional assert and I couldn't see why the hell it didn't still work. | |
| 19:12:09 | mdbooth | dansmith: Is it possible to migrate an instance between cells? Excepting, of course that the answer's no because there's only 1 cell. | |
| 19:12:36 | dansmith | mdbooth: eventually, but not currently | |
| 19:16:10 | mdbooth | external_instance_event() associates a migrating instance with both its hosts. It then assumes that all instances it associated with a particular host have the same cell_mapping. However, if an instance were migrating to a new cell, it would be listed against the destination host, but have a different cell mapping to all other instances in that list. | |
| 19:18:08 | dansmith | mdbooth: https://review.openstack.org/#/c/438025/7 | |
| 19:19:27 | mdbooth | dansmith: Sure. The assumption in this code is kinda obtuse though (I stared at it for an hour without noticing it). It'll break when you eventually allow that, and it won't be at all obvious that it needs to be fixed. | |
| 19:20:07 | mdbooth | dansmith: I wonder if it's worth addressing while it's in mind. | |
| 19:20:59 | dansmith | mdbooth: the context in the instance doesn't have anything to do with where the rpc call will go | |
| 19:21:13 | dansmith | and on the other side of the call, the context is reset to what was used for the rpc call | |
| 19:22:24 | mdbooth | dansmith: In this case the cell_mapping comes from a weird mappings dict passed in | |
| 19:23:02 | mdbooth | The bug is that if the first instance in the host list is actually from the wrong cell, it'll use the wrong cell context | |
| 19:23:05 | cdent | jaypipes: are we decided that step_size of 1 is okay on the ironic inventory because the modulo math comes out right in that (special) case? | |
| 19:23:24 | jaypipes | cdent: I think so | |
| 19:23:34 | cdent | cool, just checking | |
| 19:24:39 | openstackgerrit | melanie witt proposed openstack/nova master: Count resources to check quota for cells https://review.openstack.org/416521 | |
| 19:25:06 | mdbooth | dansmith: NM. I have to shoot, but I'm going to leave this open and look at it again first thing in the morning. | |
| 19:25:12 | dansmith | mdbooth: okay I see what you mean, but I don't think that's right.. the instancemapping will point to the hostmapping where its record currently resides | |
| 19:25:23 | dansmith | mdbooth: then we add in the other destination, | |
| 19:25:40 | dansmith | but the instance gets added to a host list based on instance.host, which matches hostmapping, and thus the cell | |
| 19:25:54 | mdbooth | dansmith: That's the bug, btw | |
| 19:26:06 | mdbooth | instance is added to host list based on _get_relevant_hosts | |
| 19:26:24 | mdbooth | So a migrating instance will be in *2* host lists, one of which it's not actually on | |
| 19:26:50 | mdbooth | If the first entry in the list just happened to be a migrating instance which wasn't actually on this host | |
| 19:26:59 | mdbooth | We'd use its cell_mapping anyway | |
| 19:27:07 | mdbooth | That's ok as long as we're not migrating between cells | |
| 19:27:07 | dansmith | ah, this is actually what you mean: | |
| 19:27:13 | dansmith | https://github.com/openstack/nova/blob/master/nova/compute/api.py#L4082 | |
| 19:27:17 | mdbooth | But it's seriously obtuse | |
| 19:27:24 | dansmith | yeah, I see now | |
| 19:27:33 | dansmith | I was looking at the api side | |
| 19:27:46 | mdbooth | dansmith: Yeah, that. | |
| 19:28:23 | dansmith | we should just pass the mapping in from the api side instead of inferring it late there | |
| 19:28:39 | dansmith | my NOTE was probably really "be lazy and don't add a new parameter" | |
| 19:29:24 | mdbooth | dansmith: As I say, I'll leave it open and poke at it in the morning | |
| 19:29:30 | dansmith | alright | |
| 19:30:02 | openstackgerrit | Dan Smith proposed openstack/nova master: Make discover_hosts only query for unmapped ComputeNode records https://review.openstack.org/427903 | |
| 19:33:59 | mriedem | gah, mocking out a context manager and asserting calls on the mock is not fun | |
| 19:36:42 | openstack | Launchpad bug 1671615 in openstack-manuals "Networking with nova-network in Administrator Guide" [Undecided,New] | |
| 19:36:42 | mriedem | docs bug if someone wants it https://bugs.launchpad.net/openstack-manuals/+bug/1671615 | |
| 19:37:35 | dansmith | mriedem: so this is my patch series to make the discovery faster: https://review.openstack.org/#/c/427903/ | |
| 19:37:41 | dansmith | maybe should have a specless blueprint? | |
| 19:37:53 | mriedem | yeah probably | |
| 19:38:20 | mriedem | mdbooth: dansmith: got any tricks for asserting calls on a context manager method? | |
| 19:38:27 | mriedem | like, i want to use mock.assert_has_calls, | |
| 19:38:33 | mriedem | on just the args to the method, | |
| 19:38:39 | mriedem | but the context manager enter/exit stuff messes that up | |
| 19:38:54 | dansmith | mriedem: mock it with a side effect that is a context manager | |
| 19:39:04 | dansmith | there's some other way to mock just __enter__ but it's always obscure so I don't do it | |
| 19:39:42 | dansmith | mriedem: I have been meaning to switch the way that fixture works a bit so we can more easily mock the non-context-manager set_target_cell in the tests | |
| 19:39:46 | efried | __enter__ can be mock.patched like anything else, neh? | |
| 19:40:15 | mriedem | yes, the problem is i don't need to mock __enter__ and __exit__ | |
| 19:40:17 | mriedem | the problem is, | |
| 19:40:24 | mriedem | assert_has_calls things i have 3 more calls than i actually care about | |
| 19:40:29 | dansmith | yeah | |
| 19:40:32 | dansmith | it's ugly | |
| 19:40:37 | dansmith | I made it work one time, | |
| 19:40:40 | dansmith | and then undid it | |
| 19:41:02 | mriedem | maybe i can just use mock.method_calls | |
| 19:41:08 | mriedem | which doesn't include magic methods | |
| 19:42:07 | asettle | Sorry mriedem was off for the evening (I'm UK based), but basically, yes :) big fat warning. | |
| 19:42:15 | asettle | "This is deprecated as of Ocata" type thing | |
| 19:42:17 | asettle | Thanks for noticing that! | |
| 19:43:14 | efried | mriedem https://github.com/powervm/pypowervm/blob/develop/pypowervm/tests/tasks/test_storage.py#L93-L99 | |
| 19:43:18 | asettle | cdent and jaypipes - how did you both get on? | |
| 19:43:41 | efried | mriedem Or perhaps https://github.com/powervm/pypowervm/blob/develop/pypowervm/tests/tasks/test_storage.py#L355-L363 is closer to what you want. | |
| 19:44:02 | jaypipes | asettle: was there an updated revision pushed? I didn't see one? | |