Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-11
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
22:07:49 dansmith which was always annoying with things like ironic that had kinda circular deps with nova
22:08:00 efried cyborg is polling though, isn't it Sundar?
22:08:05 dansmith efried: well, that might be a good reason to do both then I guess, I dunno
22:08:42 Sundar efried: Sorry.Cyborg is polling for what?
22:08:42 efried Heh, mushroom cloud. That should be a thing.
22:09:04 Sundar Polling for new devices? Yes
22:09:45 dansmith efried: I understand that we avoid asking for the new thing in a roundabout way by not exposing a trait, cyborg not exposing inventory, placement not returning any candidates, and thus us not actually running our control plane code,
22:09:46 efried Sundar: If cyborg agent is shiny and new on a host, but the nova-compute on the host is downlevel
22:09:47 efried and then you upgrade the nova-compute
22:09:47 efried will cyborg eventually figure that out and realize it should now start reporting accel inventory on that host?
22:10:03 dansmith efried: but it would be a lot more in line with our regular checks to just be explicit about it, even if we should never hit it because of the other
22:10:21 efried dansmith: okay, so what is it that you're suggesting exactly? I didn't pick up on that.
22:10:50 efried An RPC version check from conductor to compute?
22:10:51 openstackgerrit Mykola Yakovliev proposed openstack/nova master: Fix boot_roles in InstanceSystemMetadata https://review.opendev.org/698040

Earlier   Later