Progress Info in Asyncworker #34
Conversation
❌ Spellcheck FailedThere are spelling errors in your PR. Visit the workflow output to see what words are failing. Adding new wordsYou can add new custom words to .github/config/en-custom.txt. |
|
|
||
| Currently async worker has no knowledge of the operation progress till the operation is complete. If the operation times out, there is no mechanism in the current design that any information about the operation progress so far can be communicated to the user. As a result, the user only sees the error message that the operation timed out. | ||
|
|
||
| The proposal here is to add a mechanism so that the running operation can send out progress information to the async worker. As a result, when the deployment operation eventually times out, along with the timed out message, the user will also be able to see the operation progress so far in the error message which could help in troubleshooting the deployment failure. |
There was a problem hiding this comment.
I think this idea is pretty cool overall.
I'm curious what happened to the original idea? https://github.com/radius-project/design-notes/pull/22/files
There was a problem hiding this comment.
I was trying to implement one of those in that design doc - detect readiness failures by looking at pod events and looks like we need this to be implemented to actually communicate the events to the user. Without this, even though the code can detect the failures, the user still sees - operation timed out.
There was a problem hiding this comment.
Without this, even though the code can detect the failures, the user still sees - operation timed out.
Why is that the case? If the code inside the container operation implemented the timeout, it can control how the error reporting functions.
| // ProgressEvents represents the progress of the async operation. | ||
| // The events are represented as a map of key-value pairs where the key is | ||
| // the event type such as Info, Error and the value is the event message. | ||
| ProgressEvents map[string]string `json:"progressEvents,omitempty"` |
There was a problem hiding this comment.
How does client gets this ProgressEvents? What's the protocol to get this error as client perspective? In ARM, operationstatuses api is the interface to get the status of async operation.
Therefore, I do not think we need to add new property for process event. The existing errors.details array property is enough to capture all in-progress error event. See this error format
// Error represents the error occurred during provisioning.
Error *ErrorDetails `json:"error,omitempty"`In terms of implementation, we need to pass additional error channel to async worker controller to allow the controller to update the live error. then consumer side can keep adding the error into error.Details array
There was a problem hiding this comment.
Does that work in the case of success? My concern is that clients will be confused by the presence of the error field for a success case.
There was a problem hiding this comment.
Ah make sense. Then, we can consider using operations array property. What do you think ?
There was a problem hiding this comment.
Where are we going to get the ProgressEvents from? Do we need a formatter for this so that the end user can see it properly?
There was a problem hiding this comment.
I wasn't aware there was an existing API for this. If there is an existing API, we should do another iteration with the proposed usage.
|
|
||
| ## Overview | ||
|
|
||
| Currently async worker has no knowledge of the operation progress till the operation is complete. If the operation times out, there is no mechanism in the current design that any information about the operation progress so far can be communicated to the user. As a result, the user only sees the error message that the operation timed out. |
There was a problem hiding this comment.
There's the ability to indicate the progress as a percentage, but I agree no real way to provide user-facing messages.
I think the ability to indicate progress is separate from the question of: "What error message do users see when an operation fails?"
This sounds like a useful feature, but we should have the debate about whether to use it for this.
| - Enable reporting of operation progress in async worker | ||
|
|
||
| ### Non-Goals | ||
| - Modify reporting of operation progress in CLI |
There was a problem hiding this comment.
I think we can say that this is out of scope of this design doc, but we haven't delivered any value to users until you can see progress in the CLI or other UX.
|
|
||
| The readiness probe for the application pod fails. This is a non-terminal failure condition. The current code keeps waiting for the deployment to complete and eventually the deployment times out. The error message to the user is "Deployment timed out after xxx seconds". | ||
|
|
||
| If we implement https://github.com/radius-project/radius/issues/6284 and look at pod events, we can detect the readiness probe failure. However, we still need a way to report this information to the user. |
There was a problem hiding this comment.
Do you have an example of what this would look like? eg: if we deployed a container what set of progress messages would we create?
Would this become the default behavior for all of our async operations? eg: when an operation times out we'd build an error message from the progress updates?
| When the operation times out, with this change, the user should now be able to see an error message similar to:- | ||
| "Operation (APPLICATIONS.CORE/CONTAINERS|PUT) has timed out because it was processing longer than xx s. Progress events: | ||
| Info - Container ctnr-bad-readiness is running but not ready | ||
| Error - Container failed readiness probe. Reason: Unhealthy, Message: Readiness probe failed: Get \"http://10.244.0.10:5000/bad\": dial tcp 10.244.0.10:5000: connect: connection refused" |
There was a problem hiding this comment.
Does order matter?
The way that this is defined there's a single message allowed for each severity level. That's not straightforward to understand. A design that's more like a log (sequence of events) is what I would expect.
|
|
||
| ### API design (if applicable) | ||
|
|
||
| NA |
There was a problem hiding this comment.
Definitely applicable :) this whole item is API design.
|
This pull request is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
Closing this based on staleness. Please reopen if this project gets started again. |
Reporting operation progress in AsyncWorker