Skip to content
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

Merged
merged 12 commits into from
Mar 27, 2019
4 changes: 4 additions & 0 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@
"description": "name of the artifact. must be unique within a template's inputs/outputs.",
"type": "string"
},
"optional": {
"description": "Make Artifacts optional, if Artifacts doesn't generate or exist",
"type": "boolean"
},
"path": {
"description": "Path is the container path to the artifact",
"type": "string"
Expand Down
2 changes: 1 addition & 1 deletion errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const (
CodeBadRequest = "ERR_BAD_REQUEST"
CodeForbidden = "ERR_FORBIDDEN"
CodeNotFound = "ERR_NOT_FOUND"
CodeNotImplemented = "ERR_NOT_INPLEMENTED"
CodeNotImplemented = "ERR_NOT_IMPLEMENTED"
CodeTimeout = "ERR_TIMEOUT"
CodeInternal = "ERR_INTERNAL"
)
Expand Down
7 changes: 7 additions & 0 deletions pkg/apis/workflow/v1alpha1/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/apis/workflow/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,9 @@ type Artifact struct {

// Archive controls how the artifact will be saved to the artifact repository.
Archive *ArchiveStrategy `json:"archive,omitempty"`

// Make Artifacts optional, if Artifacts doesn't generate or exist
Optional bool `json:"optional,omitempty"`
}

// ArchiveStrategy describes how to archive files/directory when saving artifacts
Expand Down
22 changes: 22 additions & 0 deletions test/e2e/expectedfailures/input-artifact-not-optional.yaml
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"]
24 changes: 24 additions & 0 deletions test/e2e/expectedfailures/output-artifact-not-optional.yaml
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
22 changes: 22 additions & 0 deletions test/e2e/functional/input-artifact-optional.yaml
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"]
24 changes: 24 additions & 0 deletions test/e2e/functional/output-artifact-optional.yaml
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
40 changes: 40 additions & 0 deletions test/e2e/functional/output-input-artifact-optional.yaml
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"]
4 changes: 2 additions & 2 deletions workflow/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,10 @@ func ProcessArgs(tmpl *wfv1.Template, args wfv1.Arguments, globalParams, localPa
}
// artifact must be supplied
argArt := args.GetArtifactByName(inArt.Name)
if argArt == nil {
if !inArt.Optional && argArt == nil {
return nil, errors.Errorf(errors.CodeBadRequest, "inputs.artifacts.%s was not supplied", inArt.Name)
}
if !argArt.HasLocation() && !validateOnly {
if !inArt.Optional && !argArt.HasLocation() && !validateOnly {
return nil, errors.Errorf(errors.CodeBadRequest, "inputs.artifacts.%s missing location information", inArt.Name)
}
argArt.Path = inArt.Path
Expand Down
4 changes: 3 additions & 1 deletion workflow/executor/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the error is misleading and file.ExistsInTar is checking the right thing.

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
Expand Down
11 changes: 11 additions & 0 deletions workflow/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,13 @@ 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() && art.Optional {
log.Warnf("Artifact doesn't have location. Artifact configured as an optional so, Artifact will be ignored")
continue
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor improvement to give proper error, instead of failing in InitDriver:

		if !art.HasLocation() {
		    art.Optional {
			log.Warnf("Artifact doesn't have location. Artifact configured as an optional so, Artifact will be  ignored")
			continue
		    }
		    return errors.New("required artifact %s not supplied", art.Name)
		}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. done

Copy link

@qifengz qifengz Oct 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jessesuen Why need this conditon?
if !art.HasLocation() {
}

We found that optional=true in Outputs Artifact don't need this condition.
Or it should be removed ?
like this case:

  • name: print-message
    inputs:
    artifacts:
    • name: my-input-artifact
      path: /tmp/message
      optional: true
      s3:
      endpoint: s3.amazonaws.com
      bucket: my-bucket-name
      key: path/in/bucket
      accessKeySecret:
      name: my-s3-credentials
      key: accessKey
      secretKeySecret:
      name: my-s3-credentials
      key: secretKey

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.
This is not what we expected and we need the behavior be consistent with outputs arifacts with optional=true.

artDriver, err := we.InitDriver(art)
if err != nil {
return err
Expand Down Expand Up @@ -232,6 +238,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)
Expand Down Expand Up @@ -502,6 +512,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)
}

Expand Down