| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 21:17:42 | openstackgerrit | Lee Yarwood proposed openstack/nova master: Remove 'nova-xvpvncproxy' https://review.opendev.org/687909 | |
| 21:18:57 | Sundar | dansmith: The problem is that, if I were to send a separate event for each ARQ for binding success/failure, tagged with the ARQ UUID, Nova woudn't know what to do with that . For one, there is no association between the ARQ UUID and the instance in nova -- that would presumably need a db change. | |
| 21:19:18 | Sundar | So, nova wouldn't know which instance is affected. | |
| 21:19:48 | dansmith | huh? the event would still be delivered to the instance, so we know which instance is affected | |
| 21:20:22 | dansmith | I understand we don't store the arq uuid anywhere, although I think we likely still have it before we're going to wait, since we just polled cyborg for the list | |
| 21:20:24 | Sundar | dansmith: true. The event has a server-uuid field. | |
| 21:20:35 | dansmith | that's kindof the whole point. | |
| 21:20:58 | dansmith | it would be much less odd if you did that, even if it doesn't mean that much to us right now | |
| 21:21:11 | dansmith | because we can log the detail, potentially report it in the instance action event, | |
| 21:21:28 | dansmith | instead of "well, I dunno, cyborg said it didn't work.. *shrug*" | |
| 21:21:39 | Sundar | dansmith: However, what does a single ARQ's success mean to Nova? It would have to wait anyway for all of them. If any of them failed, the whole set needs to be rolled back. | |
| 21:21:47 | dansmith | but whatever happens, if you're going to send one per instance, it needs to not have a tag | |
| 21:22:12 | Sundar | dansmith: Ok, I can remove the tag. | |
| 21:22:22 | dansmith | Sundar: in some future we may need to do extra things to the device on the host, and we can do that for devices that complete early while we wait for the others, | |
| 21:22:40 | dansmith | and like I said, being able to report to the user and the admin which and what happened is useful | |
| 21:24:38 | Sundar | Re. report to user/admin, the exact details would be in Cyborg logs. Given the range of errors, it will be tough to state the failure reason in the Nova event. | |
| 21:24:52 | Sundar | Perhaps the Nova logs could point the admin to Cyborg logs. | |
| 21:25:12 | dansmith | no, I mean report *which* thing failed | |
| 21:25:13 | dansmith | not why | |
| 21:26:42 | Sundar | For example, say we had one event per ARQ, tagged by ARQ UUID. Nova logs report that a certain ARQ UUID failed to bind, Would that really be useful, without knowing more about the ARQ? | |
| 21:26:53 | dansmith | yes? | |
| 21:27:22 | dansmith | I grab that arq uuid and hit logstash to see what else has reported stuff about that thing | |
| 21:28:27 | dansmith | We could just always report "error with (neutron|cinder|cyborg). You go figure it out" but that's not likely to be very popular | |
| 21:30:11 | Sundar | dansmith: IMHO, I think it will be better to look at an error in context. Giving the user an ARQ UUID is equivalent to 'go search Cyborg logs', right? | |
| 21:30:33 | dansmith | I can't believe I'm having this conversation. | |
| 21:30:48 | dansmith | Sundar: either make the event look like the rest of our events, or stop passing the tag. | |
| 21:32:17 | Sundar | dansmith: Sure, I said I can stop passing the tag. You had 2 concerns with that -- one was ease of use of logs. We were attempting to resolve that. | |
| 21:32:51 | Sundar | The other was "in some future we may need to do extra things to the device on the host". | |
| 21:33:35 | Sundar | I am still thinking about that, esp. if those extra things will require a different event type, which could have or not have a tag, independent of this event. | |
| 21:34:05 | Sundar | dansmith: Please LMK if I am off. | |
| 21:35:51 | dansmith | Sundar: is there any reason you can't send the event per ARQ in cyborg, and any reason you can't know the ARQ UUIDs at the time we wait? We have that info passed in now right? | |
| 21:38:58 | Sundar | dansmith: We are debating the benefits of sending one event per ARQ, beyond 'this is what we have done in the past.' It seems to me that logging the ARQ UUID gives little info to the user beyond 'look elsewhere for the details'. Please LMk wht that is wrong. | |
| 21:39:32 | Sundar | Secondly, we still have to wait for all events for that instance, right? | |
| 21:39:57 | dansmith | efried: you around now? | |
| 21:40:11 | efried | hi, yes, all the doctors this week. | |
| 21:40:23 | efried | shall I read scrollback or are you going to summarize for me? | |
| 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? | |