Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-10-05
20:03:43 openstackgerrit Eric Fried proposed openstack/nova master: Use ksa adapter for cinder client https://review.openstack.org/509892
20:03:57 cdent dansmith: I agree with you, for the most part, I’m just reporting on “things we say"
20:04:05 penick mriedem indeed we are, I think he asked about it in channel last week. I've been meaning to reply to the thread
20:04:07 cdent “opaque"
20:04:07 dansmith cdent: yep, understand
20:04:17 mriedem penick: ah cool, on a call with him now
20:04:23 mriedem this is over my head
20:04:40 dansmith cdent: the only opaqueness I think we need is just between scheduler and the things downstream of it which need to throw it back at placement
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 cdent and we’re planning to change it with the new format
20:09:15 penick Is he right about plaintext http only? That seems an easy fix
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 edleafe jaypipes: having to have this conversation about opaqueness gets pretty old, too
20:11:05 mriedem and yeah we do the normal ksa stuff
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 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:08 jaypipes cdent: like I said... attitude.
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 cdent I’m so sick of this.
20:12:58 jaypipes fine with me.
20:13:01 dansmith edleafe: your argument against a version is just that it's not needed, right?
20:13:25 dansmith edleafe: doesn't hurt anything, just isn't strictly required, right?
20:13:25 edleafe for allocation_requests, yes, it's not needed
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 dansmith call me out if I'm being biased
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: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/

Earlier   Later