client: 404 when accessing files for GC'ed alloc #18232
Conversation
When an allocation is garbage collected from the client, but not from the servers, the API request is routed to the client and the client does attempt to read the file, but the alloc dir has already been deleted, resulting in a 500 error. This happens because the client GC only destroys the alloc runner (deleting the alloc dir), but it keeps a reference to the alloc runner until the alloc is garbage collected from the servers as well. This commit adjusts this logic by checking if the alloc runner (and the alloc files) has been destroyed, returning a 404 if so.
lgfa29
force-pushed
the
b-return-404-on-gc-alloc
branch
from
b914de8
to
23d06cd
Compare
lgfa29
changed the title
client/fs_endpoint.go
Outdated
| if err != nil { | ||
| handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(404)), encoder) | ||
| return | ||
| } | ||
| if ar.IsDestroyed() { | ||
| handleStreamResultError( | ||
| fmt.Errorf("allocation %s destroyed from client", req.AllocID), |
There was a problem hiding this comment.
Not sure if this is the best message to return. I think GC is only one of the reasons the alloc runner may have been destroyed.
There was a problem hiding this comment.
I wonder if something along the lines of "allocation %s not found on client" would be better? Destroyed seems like an internal aspect of the client which operators might find confusing to the idea that an alloc is not found.
There was a problem hiding this comment.
Yeah, I wasn't happy with the message. You suggestion is good, I just adjusted it to state for allocation %s not found on client because, weirdly, the alloc is still there, it's just that the GC deletes the state (alloc runner)
client/fs_endpoint.go
Outdated
| if err != nil { | ||
| handleStreamResultError(structs.NewErrUnknownAllocation(req.AllocID), pointer.Of(int64(404)), encoder) | ||
| return | ||
| } | ||
| if ar.IsDestroyed() { | ||
| handleStreamResultError( | ||
| fmt.Errorf("allocation %s destroyed from client", req.AllocID), |
There was a problem hiding this comment.
I wonder if something along the lines of "allocation %s not found on client" would be better? Destroyed seems like an internal aspect of the client which operators might find confusing to the idea that an alloc is not found.
Good question, maybe this could be considered a bug? |
lgfa29
added
backport/1.4.x
When an allocation is garbage collected from the client, but not from the servers, the API request is routed to the client and the client does attempt to read the file, but the alloc files have already been deleted, resulting in a 500 error.
This happens because the client GC only destroys the alloc runner (deleting the alloc dir), but it keeps a reference to the alloc runner until the alloc is garbage collected from the servers as well.
This commit adjusts this logic by checking if the alloc runner (and the alloc files) has been destroyed, returning a 404 if so.
Closes #18216