fix: artgc delete dir with fallback as directory. Fixes #14565 (#14631) #14631
Conversation
argoproj#14631) Signed-off-by: antoinetran <[email protected]>
antoinetran
force-pushed
the
feature/issue_14565_fix_gc_directory
branch
from
74e0607 to
04755f2
Compare
antoinetran
changed the title
| return err | ||
| } | ||
| // If we get here, the error was a NoSuchKey. The key might be an s3 "directory" | ||
| log.Infof("S3 Delete artifact attempt as file failed, trying as directory (consider adding \"/\" to the key to avoid this failed attempt): key: %s", artifact.S3.Key) |
There was a problem hiding this comment.
I've approved the doc, it make sense #14589 (review).
However, for this code change, user may already code their yamls in a way that adapts to the existing behaviour, changing here will cause some issue.
Also like you mentioned, its best to use suffix "/" if its a dir, that's the proper way rather than wasintg 1 Delete request
There was a problem hiding this comment.
However, for this code change, user may already code their yamls in a way that adapts to the existing behaviour, changing here will cause some issue.
I don't think so! This code only add a fallback behavior, that is consistent with the already existing loadS3Artifact (https://github.com/argoproj/argo-workflows/blob/v3.6.7/workflow/artifacts/s3/s3.go#L105).
Old code works exactly the same, except that in the case of no archive tgz + directory + key without "/", now the artgc can delete without issue. So there is no compatibility issue with old yamls (with or without "/" in keys).
Besides, we could also implement a similar code to the loadS3Artifact in fact (the "/" as a convention to avoid a double s3 request).
However, not merging is ok for me too, as long as the doc is clear and now it is merged (thanks) :)
There was a problem hiding this comment.
The only downside of not merging is, if people haven't seen the doc in the particular section of artgc for a directory, the user will not see any error in GUI nor in Argo workflows controller. There will be a directory in s3 that is not deleted and the only relevant logs are in a artgc pod, that is deleted very quick. It is too bad we cannot at least write a warning in Argo GUI or in Argo logs.
argoproj#14631) Signed-off-by: antoinetran <[email protected]>
Fixes #14565 artgc Artifact Garbage Collect cannot delete directories - fallback as directory
Motivation
Modifications
In case the delete fail because of NoSuchKey, it tries to delete a directory. Same code already existed in loadS3Artifact() https://github.com/argoproj/argo-workflows/blob/v3.6.7/workflow/artifacts/s3/s3.go#L105
Verification
Nothing verified!!
TODO: how to?
Documentation
Another PR for doc, link later