Posted	Nick	Remark
#openstack-nova - 2019-12-11
21:42:46	dansmith	efried: yes, just like we do for ports and volumes now during build
21:42:47	dansmith	however,
21:42:56	dansmith	we use the same event during attach-one-later type operations
21:43:02	dansmith	and this should be the same.
21:43:35	efried	sure, but iiuc the (anticipated, future) design is still going to involve wrapping "attach later" arqs in a device profile-like thing.
21:43:48	Sundar	If Nova starts sending per-ARQ delete for an instance, that is going to complicate things on both sides
21:43:58	efried	and again, wouldn't we want to fail the whole operation if a subset of attachments fail?
21:44:08	dansmith	efried: during build
21:44:16	dansmith	efried: just like we do for network and storage
21:44:24	dansmith	efried: but during attach, the granularity matters of course
21:45:06	efried	Okay, if we do post-build attach of networks/volumes, we support "partial success" and don't revert the whole operation?
21:45:13	Sundar	efried: Exactly. That is why it makes sense to wait for the whole thing.I Cyborg gets an abort in the middle of a device prep task, there may not be much to do, except wait for it to complete
21:45:28	Sundar	*If
21:45:39	dansmith	efried: post-build attach sends one event per attachable thing, exactly the same cardinality as one-per during build
21:46:01	dansmith	efried: and yes, failure to attach just the new thing is not a completly totally fatal thing, of course
21:46:26	dansmith	Sundar: waiting for them all to complete has nothing to do with whether or not there are multiple events
21:46:41	dansmith	I don't even care if you currently batch them up and send them all at once today
21:46:51	efried	dansmith: tbf, it's not "of course"; that's an architectural decision that was made at some point. Which is fine if that's the way it is. And
21:46:51	efried	Sundar: I agree we want to have parity with the network/volume operations, even if it doesn't make sense for accels right now.
21:46:53	dansmith	it's about the data structure and the protocol, and how that affects the future
21:47:16	efried	so yeah, it happens that for build we'll abort the whole thing if any subset fails.
21:47:42	efried	but this will allow us to do the future thing without having to like rearchitect the API on both sides and sweat upgrades etc.
21:47:56	efried	Sundar: the added complexity is negligible, really.
21:48:01	dansmith	that's the thing I want ... to be granular now so we can be granular in the future
21:49:12	Sundar	dansmith: efried: just to be sure, you are ok if Cyborg bunches up all the ARQ events for an instance, after all ARQs have bound?
21:49:42	efried	Sundar: multiple events in the same POST /os-server-external-events call, yes.
21:49:44	dansmith	Sundar: yes, I'm okay with you batching them all so they arrive at the same time as they do today, I just care that you represent them separately
21:51:01	efried	dansmith: presumably if we run into the no-host-yet race, it would impact all the events in that payload, because all same instance, and that instance is loaded just once in the method...
21:51:14	Sundar	The current logic to wait_for_instance_event waits for a single event, right? So, if if we poll Cyborg at that point, and find that all have resolved, can we still exit early, like today?
21:51:33	dansmith	Sundar: you're going to need to do one of two things, I think, which is to poll cyborg once more without the binding=complete filter to get those, or pass them down from conductor so you have them. And yes, I'm fine with that and think it's worth the overhead
21:51:41	dansmith	efried: it doesn't matter
21:52:01	efried	(I just checked it, the instance is loaded once, so yeah, if one 422s, all will 422.)
21:52:12	dansmith	efried: the whole reason we got into this conversation is because he wasn't doing the skip granularly.. so we still do that, and only skip the ones we're missing, which is why we started having this discussion
21:52:29	dansmith	efried: ah you mean the api side, yeah I think that's fine
21:52:37	eandersson	mriedem, sean-k-mooney for the max_queues patch we don't need it as we just internally changed the logic to always allow 256 queues, but if I get some time over this week I can help backport it.
21:53:26	efried	Sundar: wait_for_instance_event takes multiple events to wait for. If you query cyborg and a subset have already bound, you cancel just those events -- dansmith's patch accounts for that -- and then wait_for_instance_event will continue to wait for the remainder.
21:53:29	efried	dansmith: right ^ ?
21:53:44	dansmith	precisely
21:54:45	Sundar	efried: There's a problem with that. Nobva doesn't keep ARQ UUIDs around, to use as tags in the event to wait for.
21:54:55	dansmith	Sundar: I just addressed that above
21:55:04	dansmith	[13:51:32] <dansmith>Sundar: you're going to need to do one of two things, I think, which is to poll cyborg once more without the binding=complete filter to get those, or pass them down from conductor so you have them. And yes, I'm fine with that and think it's worth the overhead
21:56:18	efried	so
21:56:18	efried	all_arqs = poll without binding=complete
21:56:18	efried	wait_for_instance_event(all_arqs):
21:56:18	efried	done_arqs = poll with binding=complete
21:56:18	efried	if done_arqs: cancel(done_arqs)
21:56:27	efried	dansmith: is that what you meant ^
21:56:40	dansmith	that's one option yes
21:56:59	efried	"pass down from conductor" like change RPC signatures and stuff?
21:57:09	dansmith	you could also stash the ARQ UUIDs in sysmeta in the conductor and avoid having to dick with the rpc signature
21:57:18	efried	I would prefer the other, just because mucking with RPC gorp scares me.
21:57:37	dansmith	efried: so, something I've had in my back pocket on this is that you're already breaking upgrades
21:57:58	dansmith	efried: because you are assuming that computes are new enough to do the thing you're promising to the user, but no checks are made for that
21:58:09	dansmith	efried: an RPC change would bring that to the forefront
21:58:12	efried	well
21:58:23	efried	you'll never get scheduled to such a compute.
21:58:31	efried	Because such a compute will never advertise the accel inventory
21:58:43	efried	so I think that's n/a
21:58:45	dansmith	efried: doesn't cyborg manage that?
21:58:54	efried	the cyborg agent on the compute
21:59:03	dansmith	right, so you're wrong :)
21:59:16	dansmith	new cyborg agent, old compute agent = breakage
21:59:21	efried	so as long as there's a xdep between the cyborg agent and compute on the same host
21:59:29	efried	which, that must be a thing, no??
21:59:32	dansmith	eh?
21:59:42	efried	I mean, we enforce that for placement
21:59:45	dansmith	you mean like an RPM or DEB dependency?
22:00:01	efried	no, I mean like cyborg agent refuses to start up if compute isn't at version xyz
22:00:07	dansmith	efried: how would it know?
22:00:30	Sundar	dansmith: An old Nova would not query CYborg for device profiles or create/bind ARQs
22:00:39	dansmith	Sundar: a new nova control plane would
22:00:50	dansmith	Sundar: nova supports backlevel nova-compute services, which is the scenario I'm talking about
22:01:28	Sundar	dansmith: You mean new n-api, n-super-cond, n-sch but old n-cpu?
22:01:42	efried	...do we seriously not have a way to query the nova version on a compute?
22:01:50	dansmith	Sundar: yes, a very important scenario we have supported for a long time that people depend on
22:01:53	efried	I mean, obv the RPC API does, which is what you're leading up to.
22:02:08	dansmith	efried: we don't and shouldn't
22:03:43	dansmith	I guess I better go ahead and drop that in the review somewhere before I go poof for the year and ya'll try to merge this :)
22:03:58	Sundar	dansmith: I may be missing something. We had the create/bind and the wait all in n-cpu, which would have avoided this issue (at the expense of less concurrency), right?
22:04:08	efried	Sundar: no
22:04:18	dansmith	Sundar: no, you still have the same problem with that arrangement
22:04:21	efried	So, ugh, what we actually need is something like:
22:04:21	efried	compute advertises a COMPUTE_CAN_DO_CYBORG_SHIT trait so that
22:04:21	efried	cyborg (which already queries that RP to know where to hang the accel RPs) can know whether it's even allowed to do that.
22:04:43	dansmith	efried: or do it with service version but you might as well do an RPC version in that case
22:04:47	efried	Sundar: The create/bind happens at deploy time, when you've already picked a host. We're talking about the bootstrapping process where cyborg decides to advertise accel resources in the first place.
22:04:55	dansmith	efried: the trait is more expensive unless you've already got them
22:05:18	efried	dansmith: expensive how? Because an extra placement query, once, on startup? Not worried about that.
22:05:23	efried	or were you talking about something else?
22:05:37	dansmith	something else, and I misunderstood what you mean so let me 'splain:
22:06:08	dansmith	efried: you could either try to avoid cyborg hanging the inventory off the compute at bootstrap time, or you could avoid letting the conductor/scheduler start down this path for a compute that can't handle it
22:06:27	efried	the former gets you the latter for free
22:06:31	dansmith	IMHO, the latter is the job of nova anyway,
22:06:32	dansmith	and would fit with a "this compute isn't new enough to do that thing you want" sort of check
22:06:50	dansmith	efried: it depends on another service for upgrade correctness though
22:06:57	efried	if you expose the inventory but have something else that prevents you from using it, that's a waste, isn't it?
22:07:10	dansmith	i.e. it depends on cyborg being well-behaved, and/or not being modified to just always do it
22:07:29	dansmith	efried: well, it means that cyborg can be upgraded and working before you've upgraded all your computes
22:07:37	efried	heck, on that theory we could hang the inventory off a *really* old compute and it mushroom clouds because it can't handle nested providers at all.
22:07:38	dansmith	so if you upgrade service by service, you don't have to loop back around

Earlier   Later