| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 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 | |
| 22:11:04 | dansmith | efried: if you pass the ARQs down in the spawn call, you need an RPC version, so you get a service version, which is something you can just check for easily in conductor, alternately just bump the service version without the rpc and check for that in conductor | |
| 22:11:23 | dansmith | efried: or expose it as a trait and have conductor/scheduler filter or check for that trait before agreeing to use that compute | |
| 22:11:55 | dansmith | efried: perhaps the easiest thing would be just "if we have accel resources, also add CAN_DO_CYBORG_SHIT trait into the requirements" before we call placement during scheduling? | |
| 22:12:05 | dansmith | you'll need to do it again for attach when we add that | |
| 22:12:14 | dansmith | so CAN_DO_CYBORG_SHIT_V2alpha | |
| 22:13:02 | efried | dansmith: I'm not offended by the thought of those capability traits, which seem on par with what we have for e.g. "can do multiattach" or whatever | |
| 22:13:09 | efried | so IMO we should do those regardless | |
| 22:13:12 | dansmith | ack | |
| 22:13:17 | dansmith | so cyborg can look for that, | |
| 22:13:21 | efried | those are simply flags on that virt driver dict | |
| 22:13:25 | dansmith | and we throw that into the scheduling request also if we're doing accel stuff? | |
| 22:13:49 | efried | Well, I don't think we need it in the sched request, if cyborg is using it to decide whether to present the inventory in the first place. | |
| 22:14:09 | efried | but if you think it should be there for form's sake, I wouldn't object. | |
| 22:14:11 | dansmith | again, that puts the onus on cyborg to be responsible, which I don't like | |
| 22:14:19 | efried | where's the trust?? | |
| 22:14:29 | Sundar | So, the use case for all this is that, the operator added accelerator to a compute node but did not upgrade n-cpu on that node? | |
| 22:14:31 | dansmith | right, thank you :) | |