lxd/operations: Return failed operation state from wait endpoint - #18830
lxd/operations: Return failed operation state from wait endpoint#18830IzzaldinSamir wants to merge 3 commits into
Conversation
Fixes canonical#18715 Signed-off-by: Izzaldin <izzaldinsamir@gmail.com>
|
Hi @IzzaldinSamir I suspect that changing the semantics of I'd want @markylaing to take a look at this as he has been focussing on operations lately. Also in the bug report the OP mentions the operation is failing with:
Do you know why this is not resulting in an error being returned from
So from what I can see piecing the info together:
Is this occurring and where is it being converted into a "success" response in the CLI? Thanks |
|
@tomponline Thanks for the review! The core issue is that w.WriteHeader(http.StatusOK) and f.Flush() are called before op.Wait(ctx) blocks. If the operation fails later, SmartError can't change the HTTP status code because the 200 OK was already flushed. The CLI sees the 200 OK without a proper Failure payload and assumes it succeeded. By using op.Render(), we fetch the final Operation object (which contains the Failure status and error message) so the CLI can correctly parse it and exit with code 1. Happy to hear @markylaing's thoughts on this! |
Got you, makes sense. As long as we are happy to define |
|
And what ever we do here also needs to be mirrored in the lxd-agent Although I notice they are not the same right now, so this is a good opportunity to align them. |
|
@tomponline That makes perfect sense. I will go ahead and update the lxd-agent implementation of operationWaitGet right now so they are perfectly aligned. I'll push the commit to this PR shortly! |
Signed-off-by: Izzaldin <izzaldinsamir@gmail.com>
I'm still not quite following why |
|
@tomponline Good question. The issue is how the client handles the response after a Once the status is
{
"type": "error",
"error": "The network already exists",
"error_code": 500
}Because there is no Using {
"type": "sync",
"metadata": { ... }
}The client can then read the operation status and error message correctly and report the failure to the user. |
OK this makes sense, thanks. So really we want this Which also uses Lines 72 to 79 in cc3af01 |
|
In which case I am in agreement with your approach - although could you add a comment to the op.Wait lines explaining why you are ignoring the error. |
|
@tomponline Absolutely. I will add a comment above both _ = op.Wait(ctx) calls explaining that the error is intentionally ignored so we can render the final failure state to the client. I'll push that up right now! |
Signed-off-by: Izzaldin <izzaldinsamir@gmail.com>
Root Cause
The wait endpoint commits to an HTTP 200 OK before blocking on the operation. When
op.Waitreturned an operation error, the handler attempted to render a second HTTP error response instead of returning the operation state. As a result, clients (likelxc network) interpreted failed asynchronous operations as successful because they received a 200 OK without the failure payload.The Fix
Instead of checking the error from
op.Wait(ctx), we now callop.Render()after the wait returns to fetch the actual final state of the operation (which includes failures and timeouts) and return it to the client via aSyncResponse.Added regression coverage in
test/suites/operations.shto ensure duplicate network creations properly return the failure state.Fixes #18715