-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: artifact subdir error when using volumeMount #12638
Conversation
Signed-off-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
Signed-off-by: Tianchu Zhao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome back!
Signed-off-by: Isitha Subasinghe <[email protected]>
@@ -204,6 +204,11 @@ func (we *WorkflowExecutor) LoadArtifacts(ctx context.Context) error { | |||
// the file is a tarball or not. If it is, it is first extracted then renamed to | |||
// the desired location. If not, it is simply renamed to the location. | |||
tempArtPath := artPath + ".tmp" | |||
// Ensure parent directory exist, create if missing | |||
tempArtDir := filepath.Dir(tempArtPath) | |||
if err := os.MkdirAll(tempArtDir, 0o700); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why do we set this 700?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here creates a temporary path, then artifact driver will load(download) the object to this path.
The minimum dir permission across the artifact drivers is 700.
Are you experiencing an issue with this?
Fixes #12174
I tried contacting original author from #12200 but met no success
We want to expedite this fix and have this ready for the next patch release
Motivation
To make sure if an artifact contains a subdir and is mounted using volume claim, the workflow runs without failing.
VolumeMount is necessary if the user wants to use ephemeral storage.
Modifications
Make sure the executor creates a subdirectory before downloading files
Verification
Added new e2e test -> e2e failed (verified original issue)
Added code change -> e2e succeed (verified fixed the issue)