| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-02-16 | |||
| 19:20:23 | melwitt | because it would set it back. okay | |
| 19:20:24 | dansmith | melwitt: because returning from the with will untarget the context you set on instance | |
| 19:20:28 | dansmith | yeah | |
| 19:20:40 | dansmith | alaski: yeah, I like that because it is less work, I just worry about it being super confusing later | |
| 19:21:02 | melwitt | yeah. originally I had two methods for targeted context, one that returns one and one that is the context manager. got rid of one because at the time because there wasn't a strong case for needing it yet | |
| 19:21:46 | dansmith | the event patch needs to be more prescriptive because it has to collate multiple events, | |
| 19:22:00 | dansmith | but I can make the top patch do the magic thing and see how much has to change all at once I guess | |
| 19:22:53 | alaski | dansmith: yeah. I'm slowly remembering that I liked storing the targeted context on object._context and just treating an object as attached to the cell it came from, since nothing else should pull object._context to try to use it for other purposes | |
| 19:23:39 | dansmith | alaski: meaning it's a copy and targeted, whereas the context we pass to methods remains clean? | |
| 19:23:52 | dansmith | that is probably less obscure than how I was thinking about it | |
| 19:24:02 | alaski | yeah | |
| 19:24:23 | alaski | there's still some risk, but it seemed the least magic | |
| 19:24:34 | dansmith | melwitt: if you agree, I'll go do that thing and we can blame alaski for any fallout sometime when he's not looking | |
| 19:25:18 | melwitt | I guess the thing is, will that do the job. because the target_cell modifies the external context, not the internal one. so will things look at instance._context to switch | |
| 19:26:09 | melwitt | dansmith: yeah, I'm not too opinionated about it. alaski's thinking makes sense to me, plus we can blame him | |
| 19:26:37 | dansmith | melwitt: well, we would still target the external context if and when we need to, | |
| 19:26:43 | dansmith | melwitt: but instance.save() would just work (tm) | |
| 19:26:54 | melwitt | I'm just not sure how it's going to work out because of context vs instance._context, but yeah. yeah, true | |
| 19:27:01 | openstackgerrit | Sean Dague proposed openstack/nova master: WIP: verify project_id when quotas are checked https://review.openstack.org/435010 | |
| 19:27:19 | dansmith | melwitt: well, given the fun I just had with the test fixture, | |
| 19:27:33 | dansmith | it'll be interesting to see what doesn't fall into that realm with just the instance context set | |
| 19:27:42 | dansmith | like, looking up BDMs for an instance in the middle of an operation or something | |
| 19:28:12 | dansmith | melwitt: like, we'll make the rpc call with an untargeted context, which means my serializer hack won't work | |
| 19:28:19 | dansmith | which will suck | |
| 19:28:20 | dansmith | a lot. | |
| 19:28:41 | dansmith | unless the rpc layer auto-targets the context before it makes the call | |
| 19:28:50 | melwitt | hm, yeah that's what I was just thinking | |
| 19:28:56 | dansmith | that makes me sad | |
| 19:28:57 | melwitt | we have the lookup things in there | |
| 19:29:19 | dansmith | right, which I thought we wanted to maybe replace by going back to targeting... | |
| 19:29:26 | dansmith | we added transport back into the context, right? | |
| 19:29:33 | dansmith | ugh, I dunno | |
| 19:29:41 | melwitt | really? I didn't know we wanted to :P | |
| 19:29:56 | melwitt | not yet. I have a patch up that does that but you crapped on it | |
| 19:29:58 | dansmith | didn't we discuss that? I thought we added it back in | |
| 19:30:06 | dansmith | hah, well | |
| 19:30:15 | dansmith | link? | |
| 19:30:28 | melwitt | we don't technically need it, it would just save lookups if we did | |
| 19:30:39 | melwitt | https://review.openstack.org/411543 | |
| 19:30:51 | dansmith | right, I thought that was the plan.. to not have to do that lookup again in the rpc layer and thus go with the targeting keeping things pointed in the right place | |
| 19:31:08 | dansmith | melwitt: oh please.. that's like a small dingleberry | |
| 19:31:19 | dansmith | I didn't even -1 | |
| 19:31:25 | melwitt | I did ^ weirdly because we have tried to not pass cell mapping around so user/pass don't get dumped out in some exception somewhere, potentially | |
| 19:31:40 | melwitt | haha, I know | |
| 19:32:26 | melwitt | okay, well, the original plan was to target everything explicitly but then we thought it would be easier if we just looked it up in there | |
| 19:33:20 | dansmith | so I know I raised it as an issue, but I wonder what kind of things might come up from just having a targeted external context all the time | |
| 19:33:22 | melwitt | another thing is belliott had said the construction of the rpc transport objects is expensive, so there was value in caching them that way instead of creating them on-the-fly each target | |
| 19:33:34 | dansmith | the biggest thing would be going back to the api database again, but that shouldn't be affected I think right? | |
| 19:33:56 | dansmith | melwitt: we can still cache them even if we target them early | |
| 19:34:31 | melwitt | yeah. we'd have to find a place for that, maybe nova.context is not the best place is another thing belliott didn't like about it | |
| 19:34:55 | dansmith | yeah, I don't really get those concerns, but.. | |
| 19:34:56 | melwitt | dansmith: yeah going back to api database isn't affected. those can use the global api db context managers | |
| 19:35:18 | dansmith | yeah, so, maybe I'm just being overly concerned about doing the target hard and early | |
| 19:37:26 | melwitt | yeah, I don't think we've thought a lot about targeting a context and just letting it carry the db/mq connections around | |
| 19:37:42 | melwitt | it might work fine | |
| 19:38:05 | dansmith | let me just do the early full target thing and see what explodes | |
| 19:38:12 | melwitt | sure | |
| 19:43:55 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: API support for tagged device attachment https://review.openstack.org/376021 | |
| 19:43:55 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: Delete disk metadata when detaching volume https://review.openstack.org/435082 | |
| 19:46:09 | mriedem | dansmith: on https://review.openstack.org/#/c/433362/4/nova/api/openstack/compute/servers.py i wonder how much of that pattern is going to be everywhere | |
| 19:46:14 | melwitt | dansmith: I'll play around with making my mq connection patch cache early in the target_cell later | |
| 19:46:43 | dansmith | mriedem: lots of places | |
| 19:46:46 | mriedem | we could put some common code into nova/api/openstack/common, but that doesn't help with all of the target_cell stuff | |
| 19:47:18 | dansmith | mriedem: well, it does if it just targets the context permanently, which is what I'm trying now | |
| 19:47:41 | mriedem | why do this in the REST API handlers rather than the compute API class methods themselves? | |
| 19:48:21 | dansmith | mriedem: they have to look up the instance | |
| 19:48:38 | dansmith | mriedem: and we don't want the rest api looking up the mapping, then making compute/api do it again | |
| 19:48:44 | mriedem | oh yeah | |
| 19:51:08 | mriedem | dansmith: before i read scrollback i was thinking we could pass the mapping to every compute API method, like the context, and then we could put a decorator on the compute API methods to do the target cell thing for us | |
| 19:51:15 | mriedem | but i see melwitt doesn't like that | |
| 19:51:22 | dansmith | mriedem: _I_ don't like that | |
| 19:51:25 | mriedem | oh | |
| 19:51:30 | melwitt | we both don't like it | |
| 19:51:33 | dansmith | I asked so someone could say "fuck no" | |
| 19:51:43 | melwitt | heh | |
| 19:52:21 | mriedem | was that "(1:17:41 PM) dansmith: melwitt: yeah, well, glad we agree on that, I'd like to rule that out right here and now :)" ? | |
| 19:52:29 | dansmith | yeah | |
| 19:52:36 | mriedem | because you were also talking about stashing the mapping in the context or the instance at the same time | |
| 19:52:50 | dansmith | yeah, but I meant the param thing | |
| 19:53:04 | jaypipes | mriedem, dansmith, cdent, edleafe, johnthetubaguy, bauzas, melwitt: https://etherpad.openstack.org/p/decision-finding-shared-inventory | |
| 19:54:26 | jaypipes | mriedem, dansmith, cdent, edleafe, johnthetubaguy, bauzas, melwitt: feel free to vote and leave comments... adding that to the scheduler/placement session discussions at PTG | |
| 19:54:32 | cdent | jaypipes: thank you for writing that up. will digest properly tomorrow morning, unless I'm suddenly overcome with desire to think about it now (which is somewhat likely) | |
| 19:55:56 | mriedem | that bile yellow color hurts my eyes | |
| 19:56:38 | jaypipes | mriedem: then lucky for you you didn't see the shirt I was wearing this morning. cdent is still recovering. | |
| 19:56:45 | cdent | jaypipes: for completeness could you add a bit more info on why that particular question needs to be asked? | |
| 19:56:58 | jaypipes | cdent: yuppers. | |
| 19:56:59 | cdent | yeah, I'm going to have nightmares tonight. the combo is... | |
| 19:57:01 | jaypipes | cdent: will do. | |
| 19:57:07 | mriedem | were the nipples cut out? | |
| 19:57:23 | mriedem | because if so, sign me up | |
| 19:57:30 | jaypipes | mriedem: lol | |
| 19:57:52 | mriedem | dansmith: so it sounds like stashing the targeted context is the plan? | |
| 19:58:19 | dansmith | mriedem: potentially, until I find some reason to hate that more | |
| 19:58:27 | cdent | mriedem: great, thanks. I'm totally broken now. | |
| 20:00:05 | mriedem | i'm trying to think of an operatoin where we could have multiple instances in different cells | |
| 20:00:10 | mriedem | but i don't think that's possible right now | |
| 20:00:20 | dansmith | mriedem: it's not now for sure, | |
| 20:00:39 | dansmith | mriedem: and I was thinking of migrations of some sort, but that'll clearly require some special handling anyway | |
| 20:00:39 | mriedem | what apis take multiple instances anyway? | |
| 20:00:53 | dansmith | well, the events api, which is why I said what I did, but that one is already handles | |