| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-09 | |||
| 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? | |
| 19:44:26 | asettle | jaypipes: nope. sorry, that's not what I meant. Just checking in to see if you or cdent had other thoughts/concerns since we last spoke | |
| 19:44:30 | mriedem | efried: that's a bit ugly imo | |
| 19:44:33 | asettle | I see your comments on patch 8 | |
| 19:44:36 | asettle | Thank you :) | |
| 19:44:40 | mriedem | the .__enter__.return_value | |
| 19:44:56 | cdent | asettle: i put more comments on the review, but basically I was able to confirmation that the placement parts are okay | |
| 19:45:09 | efried | mriedem Would it be less ugly if __enter__ wasn't a double-underscore method? | |
| 19:45:32 | openstackgerrit | Dan Smith proposed openstack/nova master: Make discover_hosts only query for unmapped ComputeNode records https://review.openstack.org/427903 | |
| 19:45:33 | openstackgerrit | Dan Smith proposed openstack/nova master: Add ComputeNode.mapped field https://review.openstack.org/427901 | |
| 19:45:33 | openstackgerrit | Dan Smith proposed openstack/nova master: Add ability to query for ComputeNodes by their mapped value https://review.openstack.org/427902 | |
| 19:45:36 | asettle | Terrific! Ugh thank you so much cdent :D | |
| 19:45:56 | jaypipes | asettle: nope, besides those suggestions I put on the initial review, I'm good! :) | |
| 19:47:40 | asettle | Ugh you guys are excellent :D thank you both and to mriedem for looking at this all week :) Crossing my fingers that some of these edits should set us on the right path for both RDO and Ubuntu | |
| 19:47:53 | asettle | I'll check back in with Brian in the morning. | |
| 19:50:08 | openstackgerrit | christopher.uhler proposed openstack/nova master: Libvirt volume driver for Veritas HyperScale https://review.openstack.org/443951 | |
| 20:00:28 | mriedem | guh | |
| 20:00:41 | mriedem | annoying when you have the fix for a thing all day, but spend 2 hours trying to get tests to work | |
| 20:00:45 | mriedem | *unit* tests | |
| 20:01:05 | cdent | mriedem: after all this time you haven't been able to accept that as the new normal? | |
| 20:01:57 | mriedem | sometimes i don't mind, | |
| 20:02:02 | mriedem | i only mind when i have other stuff to be doing | |
| 20:04:50 | efried | mriedem Then you have to rebase the thing... | |
| 20:06:18 | mriedem | i do actually have to rebase this | |
| 20:06:20 | mriedem | but want it working first | |
| 20:06:35 | mriedem | i want the *mocks* working first | |
| 20:08:47 | mriedem | gee thanks everyone | |
| 20:09:09 | dansmith | mriedem: basically, we all acknowledge your mundane plight, now get back to work | |
| 20:09:09 | mriedem | what i really need is coffee | |
| 20:09:11 | mriedem | and metal | |
| 20:09:15 | mriedem | ha | |