-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Issue#896 Workflow steps with non-existant output artifact path will succeed #1277
Changes from 10 commits
4bbf66e
3a0eb0f
1d1b79c
6b9dc76
20cb918
3b86969
f42b23c
1277e74
6704a46
3f258aa
516b315
120c621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# This example demonstrates the input artifacts optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: input-artifact-optional- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. generateName does not match filename. |
||
spec: | ||
entrypoint: http-artifact-example | ||
templates: | ||
- name: http-artifact-example | ||
inputs: | ||
artifacts: | ||
- name: kubectl | ||
path: /bin/kubectl | ||
mode: 0755 | ||
optional: false | ||
http: | ||
url: "" | ||
container: | ||
image: debian:9.4 | ||
command: [sh, -c] | ||
args: ["echo NoKubectl"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# This example demonstrates the output artifacts optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: output-artifact-not-optional- | ||
spec: | ||
entrypoint: artifact-example | ||
templates: | ||
- name: artifact-example | ||
steps: | ||
- - name: generate-artifact | ||
template: whalesay | ||
|
||
- name: whalesay | ||
container: | ||
image: docker/whalesay:latest | ||
command: [sh, -c] | ||
args: ["cowsay hello world | tee /tmp/hello_world12.txt"] | ||
outputs: | ||
artifacts: | ||
- name: hello-art | ||
optional: false | ||
path: /tmp/hello_world.txt |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# This example demonstrates the input artifacts optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: input-artifact-optional- | ||
spec: | ||
entrypoint: http-artifact-example | ||
templates: | ||
- name: http-artifact-example | ||
inputs: | ||
artifacts: | ||
- name: kubectl | ||
path: /bin/kubectl | ||
mode: 0755 | ||
optional: true | ||
http: | ||
url: "" | ||
container: | ||
image: debian:9.4 | ||
command: [sh, -c] | ||
args: ["echo NoKubectl"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# This example demonstrates the output artifacts optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: output-artifact-optional- | ||
spec: | ||
entrypoint: artifact-example | ||
templates: | ||
- name: artifact-example | ||
steps: | ||
- - name: generate-artifact | ||
template: whalesay | ||
|
||
- name: whalesay | ||
container: | ||
image: docker/whalesay:latest | ||
command: [sh, -c] | ||
args: ["cowsay hello world | tee /tmp/hello_world12.txt"] | ||
outputs: | ||
artifacts: | ||
- name: hello-art | ||
optional: true | ||
path: /tmp/hello_world.txt |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
# This example demonstrates the output and input artifacts are optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: output-input-artifact-optional- | ||
spec: | ||
entrypoint: artifact-example | ||
templates: | ||
- name: artifact-example | ||
steps: | ||
- - name: generate-artifact | ||
template: whalesay | ||
- - name: consume-artifact | ||
template: print-message | ||
arguments: | ||
artifacts: | ||
- name: message | ||
from: "{{steps.generate-artifact.outputs.artifacts.hello-art}}" | ||
- name: whalesay | ||
container: | ||
image: docker/whalesay:latest | ||
command: [sh, -c] | ||
args: ["cowsay hello world | tee /tmp/hello_world123.txt"] | ||
outputs: | ||
artifacts: | ||
- name: hello-art | ||
optional: true | ||
path: /tmp/hello_world.txt | ||
|
||
- name: print-message | ||
inputs: | ||
artifacts: | ||
- name: message | ||
path: /tmp/message | ||
optional: true | ||
container: | ||
image: alpine:latest | ||
command: [sh, -c] | ||
args: ["echo /tmp/message"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,7 +65,9 @@ func (d *DockerExecutor) CopyFile(containerID string, sourcePath string, destPat | |
return err | ||
} | ||
if !file.ExistsInTar(sourcePath, tar.NewReader(gzipReader)) { | ||
return errors.InternalErrorf("path %s does not exist (or %s is empty) in archive %s", sourcePath, sourcePath, destPath) | ||
errMsg := fmt.Sprintf("path %s does not exist (or %s is empty) in archive %s", sourcePath, sourcePath, destPath) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the step should not fail if the file exists but empty. Imagine there is an output which holds corrupted lined of a table. Being empty is its normal state. The workflow should not fail in that case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that an empty file is a valid case. I believe the error is misleading and file.ExistsInTar is checking the right thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This does not seem to be the case. The reason I'm writing is that my pods are failing when output files are empty, which is very frustrating and surprising (I've created component that can write either to GCS or BigQuery and has two outputs where one is always supposed to be empty). |
||
log.Warn(errMsg) | ||
return errors.Errorf(errors.CodeNotFound, errMsg) | ||
} | ||
log.Infof("Archiving completed") | ||
return nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -117,6 +117,10 @@ func (we *WorkflowExecutor) LoadArtifacts() error { | |
log.Infof("Downloading artifact: %s", art.Name) | ||
artDriver, err := we.InitDriver(art) | ||
if err != nil { | ||
if art.Optional { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to be specific about the error. With this logic, we can't distinguish a driver initialization errors vs. an error because the artifact location is not supplied. The former should fail the workflow. The latter should not. Alternatively, we can maybe avoid calling InitDriver entirely and simply continue the loop, if we know that the artifact has no location (or has an empty location), and the artifact was optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
log.Warnf("Error in loading Artifacts. Artifact configured as an optional so, Error will be ignored. Error=%v", err) | ||
return nil | ||
} | ||
return err | ||
} | ||
// Determine the file path of where to load the artifact | ||
|
@@ -232,6 +236,10 @@ func (we *WorkflowExecutor) saveArtifact(tempOutArtDir string, mainCtrID string, | |
localArtPath := path.Join(tempOutArtDir, fileName) | ||
err := we.RuntimeExecutor.CopyFile(mainCtrID, art.Path, localArtPath) | ||
if err != nil { | ||
if art.Optional && errors.IsCode(errors.CodeNotFound, err) { | ||
log.Warnf("Error in saving Artifact. Artifact configured as an optional so, Error will be ignored. Error= %v", err) | ||
return nil | ||
} | ||
return err | ||
} | ||
fileName, localArtPath, err = stageArchiveFile(fileName, localArtPath, art) | ||
|
@@ -502,6 +510,7 @@ func (we *WorkflowExecutor) InitDriver(art wfv1.Artifact) (artifact.ArtifactDriv | |
if art.Raw != nil { | ||
return &raw.RawArtifactDriver{}, nil | ||
} | ||
|
||
return nil, errors.Errorf(errors.CodeBadRequest, "Unsupported artifact driver for %s", art.Name) | ||
} | ||
|
||
|
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.
Description is incorrect