| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 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 :) | |
| 22:14:35 | dansmith | efried: let me check my butt, hang on | |
| 22:14:57 | efried | That's not spelled "onus" dansmith | |
| 22:15:06 | dansmith | hehe | |
| 22:15:08 | efried | Sundar: yes, exactly. | |
| 22:15:37 | efried | Sundar: we're trying to make nova account for poorly-behaved a) operators, b) service code from other services. | |
| 22:15:57 | efried | and not-work in a predictable rather than unpredictable (and potentially unrecoverable) way. | |
| 22:16:11 | dansmith | just to be clear, | |
| 22:16:33 | dansmith | I definitely think that the rpc method change is the right thing to do, instead of just coding in new assumptions on both sides | |
| 22:16:57 | dansmith | I understand the hesitation to that, so I won't hold strictly to it, despite it being the safer option, IMHO | |
| 22:17:28 | efried | dansmith: if we did that, we would have to have a post-placement filter to remove hosts based on RPC version, right? | |