| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 21:40:40 | dansmith | so, it has become clear to me that the cyborg event as currently proposed is being used very differently than what we have for the other services | |
| 21:41:04 | efried | If we're discussing how many events... | |
| 21:41:06 | dansmith | specifically, it's one event per instance with "everything is done" or "everything failed" level granularity | |
| 21:41:25 | dansmith | instead of per-ARQ, akin to per-port or per-volume or anything else | |
| 21:41:27 | efried | I skimmed the reviews briefly and it makes sense to me to have an event per "attachable thingy" | |
| 21:41:33 | dansmith | right | |
| 21:41:43 | dansmith | so I think I'm going to convert to taking a hard line on that | |
| 21:41:50 | efried | That's me looking at a very high level, without really digging in. | |
| 21:41:58 | dansmith | that the event should be per-ARQ | |
| 21:42:06 | dansmith | even though everything is very monolithic right now, | |
| 21:42:09 | efried | unless the contract with cyborg is "all or nothing" | |
| 21:42:15 | Sundar | efried; What will Nova with a per-ARQ event? | |
| 21:42:19 | Sundar | *do with | |
| 21:42:27 | efried | I mean, I'm guessing we would abort the build if we get partial | |
| 21:42:33 | dansmith | eventually this should be similar to what we do with ports and volumes in terms of attachability | |
| 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 | |