Handle error events in HCS notifications. #2526
Conversation
The shim parses the JSON result document and handle the error events (via `processHcsResult`) returned by HCS calls (e.g.,`vmcompute.HcsCreateComputeSystem`), but ignores the JSON payload for notifications (which are either received from a `processAsyncHcsResult` in the appropriate system or process call in `"internal.hcs"`, or via `waitForNotification` in `waitBackground`). This leads to ambiguous failure errors (e.g., `"The data is invalid."`) that require ETW traces to track down the appropriate HCS logs, when the error events could have provided enough context to identify the issue. Parse the `notificationData` JSON payload provided by HCS to the `notificationWatcher` callback into the appropriate `hcsResult` struct. Validate that the JSON data matches the notification HResult. Create a new error type (`hcsResult`) to handle both the error and events. Since notification results are always subsequently passed to either `make(System|Process)Error`, update those functions to handle the events provided by `hcsResult` errors. Since `ErrorEvent`s are always converted to strings in the context of serializing several of them into a string, add an `(*ErrorEvent).writeTo(*string.Builder)` function to provide more efficient error string generation for `(HCS|System|Process)Error`s. Additionally, consolidate the joining and formatting of error events for those error types. Signed-off-by: Hamza El-Saawy <[email protected]>
|
With the upcoming projects, it is important that shim supports HCS V2 APIs. Also, HCS already wants to deprecate V1 APIs and therefore, it might be prudent for us to move too. Should we have an effort to move shim from HCS V1 to V2? |
Ideally, yes, we should, but I am not sure the work involved would be easily addressed in a single PR, so this PR should be a good stopgap until we move entirely over to the computecore dlls |
| } | ||
| if ev.EventID != 0 { | ||
| evs = fmt.Sprintf("%s EventID: %d", evs, ev.EventID) | ||
| fmt.Fprintf(b, " EventID: %d", ev.EventID) |
There was a problem hiding this comment.
We should perhaps use the same semantics for writing i.e. WriteString.
There was a problem hiding this comment.
wed have to call b.WriteString(fmt.Sprintf(" EventID: %d", ev.EventID)) to handle the formatting, which feels more convoluted than Fprintf
| _, _ = b.WriteString("[Event Detail: " + ev.Message) | ||
| if ev.StackTrace != "" { | ||
| evs += " Stack Trace: " + ev.StackTrace | ||
| _, _ = b.WriteString(" Stack Trace: " + ev.StackTrace) |
There was a problem hiding this comment.
Should we have a mechanism to handle the error in the builder method?
There was a problem hiding this comment.
the error is always nil, so there shouldn't be any error handling needed
| } | ||
| evs += "]" | ||
| return evs | ||
| _ = b.WriteByte(']') |
There was a problem hiding this comment.
Why are we using WriteByte instead of WriteString?
3 participants
The shim parses the JSON result document and handle the error events (via
processHcsResult) returned by HCS calls (e.g.,vmcompute.HcsCreateComputeSystem), but ignores the JSON payload for notifications (which are either received from aprocessAsyncHcsResultin the appropriate system or process call in"internal.hcs", or viawaitForNotificationinwaitBackground).This leads to ambiguous failure errors (e.g.,
"The data is invalid.") that require ETW traces to track down the appropriate HCS logs, when the error events could have provided enough context to identify the issue.Parse the
notificationDataJSON payload provided by HCS to thenotificationWatchercallback into the appropriatehcsResultstruct.Validate that the JSON data matches the notification HResult.
Create a new error type (
hcsResult) to handle both the error and events.Since notification results are always subsequently passed to either
make(System|Process)Error, update those functions to handle the events provided byhcsResulterrors.Since
ErrorEvents are always converted to strings in the context of concatenating several of them together, add an(*ErrorEvent).writeTo(*string.Builder)function to provide more efficient error string generation for(HCS|System|Process)Errors. Additionally, consolidate the joining and formatting of error events for those error types.