| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-10-05 | |||
| 20:04:56 | cdent | (under it all I find the allocation_candidates thing way overly-specific and not very api-like, but it is is what we’ve reached as a workable solution when many other things would not, so… hard to keep my guns) | |
| 20:05:03 | dansmith | having a big chunk of data that looks useful being exposed to the client and told that there be dragons within is not a good plan, IMHO | |
| 20:05:06 | openstackgerrit | Eric Fried proposed openstack/nova master: Use ksa adapter for neutron client https://review.openstack.org/509892 | |
| 20:05:44 | edleafe | dansmith: having a big chunk of data being passed around is not a good plan IMO either, but this is what we are working with | |
| 20:06:33 | edleafe | cdent: we do key on rp_uuid from the a-r, which was another design compromise | |
| 20:06:35 | dansmith | how does the jsonschema validation work if you have a change in the a-r? does it just ignore a subtree of something and validate it separately or something? | |
| 20:06:40 | penick | mriedem I'll reply to the ML today or tonight.. he notes that vendordata doesn't allow you to pass parameters, but I think that's something that can be addressed. Or he can write a vendordata driver | |
| 20:06:52 | cdent | edleafe: so it isn’t opaque | |
| 20:07:02 | edleafe | cdent: which is why for a given host there may be several a-rs, and we just take the first one for claiming | |
| 20:07:06 | mriedem | penick: nova passes some stuff to the vendordata service | |
| 20:07:35 | cdent | “passes some stuff” is the new api guideline | |
| 20:07:43 | cdent | what should my api do? “pass some stuff” | |
| 20:07:52 | edleafe | cdent: {$rp_uuid: <opaque>} | |
| 20:08:08 | mriedem | penick: https://github.com/openstack/nova/blob/master/nova/api/metadata/vendordata_dynamic.py#L77 | |
| 20:09:06 | cdent | edleafe: that’s not what an a-r looks like now: https://github.com/openstack/nova/blob/master/nova/api/openstack/placement/handlers/allocation_candidate.py#L55-L68 | |
| 20:09:10 | jaypipes | cdent: the whole "I told you guys that this all sucks but don't have a better idea about how to solve this problem" attitude gets really old. | |
| 20:09:15 | penick | Is he right about plaintext http only? That seems an easy fix | |
| 20:09:15 | cdent | and we’re planning to change it with the new format | |
| 20:09:25 | cdent | jaypipes: it gets especially old when you think that’s what we are doing and we aren’t actually | |
| 20:09:53 | jaypipes | cdent: sure seems like it. | |
| 20:09:56 | penick | er, mriedem: ahah, thanks. Is he right about plaintext only? that'd be an easy fix. I'll reply to him on the ML | |
| 20:10:26 | cdent | jaypipes: my apologies then, but I think you’ll find that this started because I pointed dan at some clarifying info about questions he reaised on a review | |
| 20:10:36 | cdent | since then we’ve been talking, that’s all | |
| 20:10:37 | penick | mriedem nm I see the ssl bit in the code | |
| 20:10:48 | mriedem | penick: yeah we ust send some shit over json in the request and we hope the vendordata service finds it useful | |
| 20:11:05 | mriedem | and yeah we do the normal ksa stuff | |
| 20:11:05 | edleafe | jaypipes: having to have this conversation about opaqueness gets pretty old, too | |
| 20:11:08 | mriedem | for the service user | |
| 20:11:12 | jaypipes | cdent: ""microversion thing way too fluid” was my concern too, but, like I said, I decided to capitulate" <-- attitude. | |
| 20:11:39 | cdent | jaypipes: because at the ptg you declared, with dan, that we should argue less, so I did: I capitulated, as requested | |
| 20:12:08 | jaypipes | cdent: like I said... attitude. | |
| 20:12:08 | cdent | you perceive so much, without confirm it, and place me in this position of being a bad guy. It. Is. Not. Me. | |
| 20:12:40 | cdent | you read into that statement some kind of smug bullshit that is not there | |
| 20:12:47 | cdent | it’s just me saying "okay" | |
| 20:12:48 | dansmith | okay, let's pause the personal stuff for a minute | |
| 20:12:58 | jaypipes | fine with me. | |
| 20:12:58 | cdent | I’m so sick of this. | |
| 20:13:01 | dansmith | edleafe: your argument against a version is just that it's not needed, right? | |
| 20:13:25 | edleafe | for allocation_requests, yes, it's not needed | |
| 20:13:25 | dansmith | edleafe: doesn't hurt anything, just isn't strictly required, right? | |
| 20:13:50 | edleafe | no, it over-engineers things, so it does hurt | |
| 20:13:54 | dansmith | edleafe: okay, so I know I don't have lots of karma to burn with you, but we could just put it in there, call it the dan_is_dumb field, and move forward without costing much else, right? | |
| 20:14:32 | dansmith | a single field over-engineers? | |
| 20:14:41 | edleafe | dansmith: you do realize that it's a lot more than just adding a field, right? | |
| 20:15:14 | edleafe | it's now having to do version checking in placement for a thing that will always be the current version | |
| 20:15:30 | dansmith | we damn sure better be doing that version checking in placement anyway | |
| 20:15:40 | edleafe | it's changing the placement API to add the version | |
| 20:15:51 | dansmith | eh? | |
| 20:16:08 | dansmith | this is protocol stuff. the envelope.. it's built into every call we make to placement if we pass version=something in report client, no? | |
| 20:16:15 | edleafe | where is that a-r version coming from? | |
| 20:17:17 | jaypipes | edleafe: it's currently hard-coded in the report client's claim_resources() method. That would need to be updated to pass an optional kwarg for the microversion override if received in the select_destinations() returned objects. | |
| 20:17:44 | jaypipes | i.e. claim_resources(..., version=$what_i_got_from_scheduler) | |
| 20:18:05 | dansmith | r = self.put(url, payload, version='1.10') | |
| 20:18:06 | dansmith | exactly | |
| 20:18:13 | jaypipes | we would want to have the scheduler package up the user and project ID into the allocation request blob it sends to. | |
| 20:18:24 | jaypipes | to the caller of the select_destinations() method | |
| 20:19:18 | edleafe | jaypipes: So we have an old scheduler, old conductor, and new placement. Are you saying that placement should modify the structure of the a-r part of the response based on the old scheduler/conductor? | |
| 20:19:23 | jaypipes | also, instead of adding a separate version attribute to the Selection object, we could return a SelectionList object that had two fields, version and selections so we don't have to repeat the version field over and over again. | |
| 20:19:39 | dansmith | edleafe: that's what versioning an API is all about | |
| 20:20:03 | dansmith | jaypipes: I said no list object because there was no need. this would be a need, so that's fine with me, if that's desired | |
| 20:20:14 | dansmith | when I argued against it, there was no such extra property | |
| 20:21:34 | edleafe | dansmith: I know what versioning an API is about. | |
| 20:22:04 | edleafe | dansmith: the issue is whether the a-r should be dependent on placement, or on the things that call placement | |
| 20:22:29 | dansmith | anything returned to the client is done so at the maximum version they both support | |
| 20:23:57 | cdent | are we talking rpc api or http api here, because http api, it is the version the client asked for | |
| 20:24:10 | cdent | which we have been generally controlling per request for nova->placement | |
| 20:24:14 | jaypipes | cdent: two different clients here. | |
| 20:24:30 | edleafe | the clients support receiving a-rs. That's all the versioning we need. If they can get an a-r blob, that's sufficient | |
| 20:24:39 | edleafe | http | |
| 20:24:50 | jaypipes | cdent: the placement client embedded in the scheduler that called GET /allocation_candidates is one client. The scheduler client embdedded in the cell conductor that needs to call PUT /allocations/ is a different client. | |
| 20:24:59 | cdent | yes, I know | |
| 20:25:07 | dansmith | cdent: really not much about rpc apis going on here | |
| 20:25:33 | cdent | I was attempting to clarify dan’s statement about “maximum version they supprot” < that’s not true for the report client, it asks for a specific version, not a max | |
| 20:25:46 | dansmith | cdent: we're sending a blob to the rpc consumer, which has an inbuilt version that it should use to communicate with an external thing.. it's opaque to _that_ service | |
| 20:26:15 | dansmith | cdent: it's an rpc client, it's tightly controlled by us, versioned interface, and we have more strict versions that are "compatible" | |
| 20:26:54 | dansmith | cdent: it's the maximum semantically, the server doesn't know anything other than what the client said is it's version (i.e. the max for this request) | |
| 20:27:14 | cdent | yes, per request | |
| 20:29:56 | mriedem | i'll say i remember saying we should include the microversion that we used to build the allocation request in the scheduler and pass that down so the conductor makes the same request at the same microversion later | |
| 20:30:01 | mriedem | rather than 'latest' | |
| 20:30:17 | dansmith | mriedem: it's actually always making 1.10 right now, not even latest | |
| 20:30:24 | dansmith | which makes it even worse, IMHO | |
| 20:30:39 | mriedem | but i'd have to lookup where i said this, which might just have been irc... | |
| 20:33:10 | jaypipes | this is the conversation that edleafe and I were having at a whiteboard in the corner. | |
| 20:33:29 | dansmith | yeah, I wasn't over there, sorry about that | |
| 20:33:49 | dansmith | so, let me just summarize where I think we stand | |
| 20:34:00 | jaypipes | and I said it would be ok to have the placement API service handle seamlessly understanding old formats of allocation request body. | |
| 20:34:00 | dansmith | call me out if I'm being biased | |
| 20:34:36 | mriedem | i don't think the placement api should be trying to retrofit the request | |
| 20:34:41 | mriedem | that's weird | |
| 20:34:42 | dansmith | me either | |
| 20:34:49 | jaypipes | I had previously asked edleafe to include the microversion the scheduler created the a-r for in the returned object from select_destinations() | |
| 20:35:08 | cdent | mikal: I assume you’re not with us at the moment, but when you join does this make sense to you: https://review.openstack.org/#/c/509417/ | |
| 20:36:02 | dansmith | so I was going to summarize... | |
| 20:36:34 | dansmith | edleafe is of the opinion that this should definitely not be versioned | |
| 20:36:40 | dansmith | I feel it should be | |
| 20:37:00 | dansmith | cdent leans towards versioning | |
| 20:37:15 | dansmith | I think jaypipes is saying he does too (is that right?) | |
| 20:37:34 | cdent | I lean toward not doing the microversino fakery failover server side | |
| 20:37:43 | cdent | I’m agnostic about what happens on the client side | |
| 20:37:57 | dansmith | and mriedem seems to think strict microversion style versioning as well | |