| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-11 | |||
| 21:03:12 | Sundar | I think the disconnect is the understanding of the object model. Please see https://review.opendev.org/#/c/631243/46/nova/accelerator/cyborg.py@26 | |
| 21:04:31 | dansmith | Sundar: I'm not sure what your point is | |
| 21:05:03 | dansmith | Sundar: I will eventually be able to have one crypto accelerator and one gzip accelerator, right? | |
| 21:05:20 | Sundar | dansmith: Yes, for the same instance | |
| 21:05:30 | dansmith | and those are two device profiles? | |
| 21:05:42 | Sundar | No, one device profile with 2 request groups | |
| 21:05:54 | dansmith | okay, so an instance will only ever have one device profile? | |
| 21:06:10 | Sundar | Yes. That single device profile's name is set in the flavor. | |
| 21:06:25 | dansmith | so, why are you setting the tag in the event to the device profile? | |
| 21:07:24 | Sundar | That was the only logical choice for a tag that seemed relevant. | |
| 21:07:59 | dansmith | setting the tag means that the event needs to be multiplexed for the instance | |
| 21:08:22 | dansmith | which is why we set it to the port id for neutron ports, for example, because there are multiple ports per instance | |
| 21:08:54 | dansmith | I'm not sure why it was decided the encompass all of the accelerators for an instance into a single entity, but alas | |
| 21:09:29 | Sundar | Would we ever need multiple events per instance? For example, for hot adds/deletes in the future, by updating the device profile? | |
| 21:09:47 | dansmith | Sundar: those would be different event types | |
| 21:10:47 | Sundar | Then what exactly is the problem -- that the tag is superfluous? | |
| 21:11:08 | dansmith | setting the tag implies that there can be multiples, so ... yes | |
| 21:12:01 | Sundar | dansmith: Got the disconnect. We don;t use the tag, at least not today. With hot adds/deletes, since it it is going to be another event type, we still don;t need it. | |
| 21:12:38 | Sundar | dansmith: BTW, the idea of one device profile per instance came from the idea of setting one device profile name in the instance. | |
| 21:12:42 | dansmith | I dunno what those would look like, but I would hope that there is some indication of what device the "thing got added" event pertains to | |
| 21:13:16 | dansmith | ...which would be done with a tag | |
| 21:14:44 | Sundar | dansmith: It may make sense to have a single notification for an update too -- because there is not much that Nova can with a partial update knowing that the next event may indicate a failure and things need to be rolled back | |
| 21:15:00 | Sundar | That is the same reasoning as for the bind logic here | |
| 21:15:48 | dansmith | Sundar: "something failed" events are pretty terrible | |
| 21:16:33 | Sundar | dansmith: If you mean that the event should say what exactly failed, agreed. | |
| 21:16:55 | dansmith | Sundar: I mean we should at least know which thing failed, and tag is the "which" | |
| 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. | |