-
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 all 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 not optionals | ||
# from one step to the next. | ||
apiVersion: argoproj.io/v1alpha1 | ||
kind: Workflow | ||
metadata: | ||
generateName: input-artifact-not-optional- | ||
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 not 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 |
---|---|---|
|
@@ -114,7 +114,17 @@ func (we *WorkflowExecutor) LoadArtifacts() error { | |
log.Infof("Start loading input artifacts...") | ||
|
||
for _, art := range we.Template.Inputs.Artifacts { | ||
|
||
log.Infof("Downloading artifact: %s", art.Name) | ||
|
||
if !art.HasLocation() { | ||
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("Artifact %s is not supplied. Artifact configured as an optional so, Artifact will be ignored", art.Name) | ||
continue | ||
} else { | ||
return errors.New("required artifact %s not supplied", art.Name) | ||
} | ||
} | ||
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. Minor improvement to give proper error, instead of failing in InitDriver:
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. sure. done 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. @jessesuen Why need this conditon? We found that optional=true in Outputs Artifact don't need this condition.
if the key path/in/bucket does not exist, optional: true cannot stop to check the key from S3, and it occured an error failed to load artifacts: The specified key does not exist. |
||
artDriver, err := we.InitDriver(art) | ||
if err != nil { | ||
return err | ||
|
@@ -232,6 +242,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 +516,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.
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 comment
The 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 comment
The 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).