From 8df8f61d1601f89d690848f5cf7bf5cd95707c57 Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Wed, 24 Jul 2019 10:19:57 -0500 Subject: [PATCH] Change the behavior of outputs that are also used as inputs. This change makes the handling of Resources within a Task consistent, regardless of whether the same Resource is used as both an input and an output. Previously these were special cased, which made it hard to write Tasks consistently. This commit also makes a few minor changes to the way our bash output gets logged. I discovered this was missing during debugging, and made it consistent with the gsutil wrapper. This is a followup to https://github.com/tektoncd/pipeline/pull/1119 and should be submitted once the next release is cut. --- cmd/bash/main.go | 7 +++-- examples/pipelineruns/output-pipelinerun.yaml | 2 +- .../taskrun/resources/output_resource.go | 27 +++++-------------- .../taskrun/resources/output_resource_test.go | 8 +++--- .../v1alpha1/taskrun/taskrun_test.go | 2 +- test/dag_test.go | 2 ++ test/pipelinerun_test.go | 24 ++++++++++++----- 7 files changed, 35 insertions(+), 37 deletions(-) diff --git a/cmd/bash/main.go b/cmd/bash/main.go index 1c04f0b0548..2f1fd4826e7 100644 --- a/cmd/bash/main.go +++ b/cmd/bash/main.go @@ -42,8 +42,8 @@ package main import ( "flag" - "log" "os/exec" + "strings" "github.com/tektoncd/pipeline/pkg/logging" ) @@ -59,8 +59,7 @@ func main() { cmd := exec.Command("sh", "-c", *args) stdoutStderr, err := cmd.CombinedOutput() if err != nil { - logger.Errorf("Error executing command %q with arguments %s", *args, stdoutStderr) - log.Fatal(err) + logger.Fatalf("Error executing command %q ; error %s; cmd_output %s", strings.Join(cmd.Args, " "), err.Error(), stdoutStderr) } - logger.Infof("Successfully executed command %q", *args) + logger.Infof("Successfully executed command %q; output %s", strings.Join(cmd.Args, " "), stdoutStderr) } diff --git a/examples/pipelineruns/output-pipelinerun.yaml b/examples/pipelineruns/output-pipelinerun.yaml index a608db598b2..f647b8ea61e 100644 --- a/examples/pipelineruns/output-pipelinerun.yaml +++ b/examples/pipelineruns/output-pipelinerun.yaml @@ -33,7 +33,7 @@ spec: - name: write-new-stuff image: ubuntu command: ['bash'] - args: ['-c', 'echo some stuff > /workspace/damnworkspace/stuff'] + args: ['-c', 'mkdir -p /workspace/output/workspace && cp -r /workspace/damnworkspace/. /workspace/output/workspace/ && echo some stuff > /workspace/output/workspace/stuff'] --- # Reads a file from a predefined path in the workspace git PipelineResource apiVersion: tekton.dev/v1alpha1 diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go index bb3ed4d2458..5938df6d47b 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource.go @@ -48,7 +48,7 @@ var ( // upload steps (like upload to blob store ) // // Resource source path determined -// 1. If resource is declared in inputs then target path from input resource is used to identify source path +// 2. If resource has a targetpath that is used. Otherwise: // 2. If resource is declared in outputs only then the default is /output/resource_name func AddOutputResources( kubeclient kubernetes.Interface, @@ -71,15 +71,6 @@ func AddOutputResources( return nil, err } - // track resources that are present in input of task cuz these resources will be copied onto PVC - inputResourceMap := map[string]string{} - - if taskSpec.Inputs != nil { - for _, input := range taskSpec.Inputs.Resources { - inputResourceMap[input.Name] = destinationPath(input.Name, input.TargetPath) - } - } - for _, output := range taskSpec.Outputs.Resources { boundResource, err := getBoundResource(output.Name, taskRun.Spec.Outputs.Resources) if err != nil { @@ -94,18 +85,12 @@ func AddOutputResources( resourceContainers []corev1.Container resourceVolumes []corev1.Volume ) - // if resource is declared in input then copy outputs to pvc - // To build copy step it needs source path(which is targetpath of input resourcemap) from task input source - sourcePath := inputResourceMap[boundResource.Name] - if sourcePath != "" { - logger.Warn(`This task uses the same resource as an input and output. The behavior of this will change in a future release. - See https://github.com/tektoncd/pipeline/issues/1118 for more information.`) + + var sourcePath string + if output.TargetPath == "" { + sourcePath = filepath.Join(outputDir, boundResource.Name) } else { - if output.TargetPath == "" { - sourcePath = filepath.Join(outputDir, boundResource.Name) - } else { - sourcePath = output.TargetPath - } + sourcePath = output.TargetPath } resource.SetDestinationDirectory(sourcePath) diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource_test.go index 37a3e525e88..90470f9667b 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/output_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/output_resource_test.go @@ -171,7 +171,7 @@ func TestValidOutputResources(t *testing.T) { Name: "source-copy-source-git-mz4c7", Image: "override-with-bash-noop:latest", Command: []string{"/ko-app/bash"}, - Args: []string{"-args", "cp -r /workspace/source-workspace/. pipeline-task-name"}, + Args: []string{"-args", "cp -r /workspace/output/source-workspace/. pipeline-task-name"}, VolumeMounts: []corev1.VolumeMount{{ Name: "pipelinerun-pvc", MountPath: "/pvc", @@ -368,7 +368,7 @@ func TestValidOutputResources(t *testing.T) { MountPath: "/var/secret/sname", }}, Command: []string{"/ko-app/gsutil"}, - Args: []string{"-args", "rsync -d -r /workspace/faraway-disk gs://some-bucket"}, + Args: []string{"-args", "rsync -d -r /workspace/output/source-workspace gs://some-bucket"}, Env: []corev1.EnvVar{{ Name: "GOOGLE_APPLICATION_CREDENTIALS", Value: "/var/secret/sname/key.json", }}, @@ -382,7 +382,7 @@ func TestValidOutputResources(t *testing.T) { Name: "source-copy-source-gcs-mssqb", Image: "override-with-bash-noop:latest", Command: []string{"/ko-app/bash"}, - Args: []string{"-args", "cp -r /workspace/faraway-disk/. pipeline-task-path"}, + Args: []string{"-args", "cp -r /workspace/output/source-workspace/. pipeline-task-path"}, VolumeMounts: []corev1.VolumeMount{{Name: "pipelinerun-parent-pvc", MountPath: "/pvc"}}, }}, wantVolumes: []corev1.Volume{{ @@ -771,7 +771,7 @@ func TestValidOutputResourcesWithBucketStorage(t *testing.T) { Name: "artifact-copy-to-source-git-9l9zj", Image: "override-with-gsutil-image:latest", Command: []string{"/ko-app/gsutil"}, - Args: []string{"-args", "cp -P -r /workspace/source-workspace gs://fake-bucket/pipeline-task-name"}, + Args: []string{"-args", "cp -P -r /workspace/output/source-workspace gs://fake-bucket/pipeline-task-name"}, }}, }, { name: "git resource in output only with bucket storage", diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index e3c67005fc3..cb4e50636a0 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -654,7 +654,7 @@ func TestReconcile(t *testing.T) { tb.PodContainer("step-source-copy-git-resource-j2tds", "override-with-bash-noop:latest", tb.Command(entrypointLocation), tb.Args("-wait_file", "/builder/tools/5", "-post_file", "/builder/tools/6", "-entrypoint", "/ko-app/bash", "--", - "-args", "cp -r /workspace/git-resource/. output-folder"), + "-args", "cp -r /workspace/output/git-resource/. output-folder"), tb.WorkingDir(workspaceDir), tb.EnvVar("HOME", "/builder/home"), tb.VolumeMount("test-pvc", "/pvc"), diff --git a/test/dag_test.go b/test/dag_test.go index a11df0db552..6503f4a19e4 100644 --- a/test/dag_test.go +++ b/test/dag_test.go @@ -53,6 +53,8 @@ func TestDAGPipelineRun(t *testing.T) { ), tb.TaskOutputs(tb.OutputsResource("repo", v1alpha1.PipelineResourceTypeGit)), tb.Step("echo-text", "busybox", tb.Command("echo"), tb.Args("${inputs.params.text}")), + tb.Step("mkdir", "busybox", tb.Command("mkdir"), tb.Args("-p", "${outputs.resources.repo.path}")), + tb.Step("cp", "busybox", tb.Command("cp"), tb.Args("-r", "${inputs.resources.repo.path}", "${outputs.resources.repo.path}")), )) if _, err := c.TaskClient.Create(echoTask); err != nil { t.Fatalf("Failed to create echo Task: %s", err) diff --git a/test/pipelinerun_test.go b/test/pipelinerun_test.go index affc3dd88ed..7be3ebd314f 100644 --- a/test/pipelinerun_test.go +++ b/test/pipelinerun_test.go @@ -148,7 +148,7 @@ func TestPipelineRun(t *testing.T) { t.Parallel() c, namespace := setup(t) - knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf) + // knativetest.CleanupOnInterrupt(func() { tearDown(t, c, namespace) }, t.Logf) defer tearDown(t, c, namespace) t.Logf("Setting up test resources for %q test in namespace %s", td.name, namespace) @@ -243,36 +243,48 @@ func getFanInFanOutTasks(namespace string) []*v1alpha1.Task { )), tb.TaskOutputs(outWorkspaceResource), tb.Step("write-data-task-0-step-0", "ubuntu", tb.Command("/bin/bash"), - tb.Args("-c", "echo stuff > /workspace/brandnewspace/stuff"), + tb.Args("-c", "mkdir -p /workspace/output/workspace && echo stuff > /workspace/output/workspace/stuff"), ), tb.Step("write-data-task-0-step-1", "ubuntu", tb.Command("/bin/bash"), - tb.Args("-c", "echo other > /workspace/brandnewspace/other"), + tb.Args("-c", "echo other > /workspace/output/workspace/other"), + ), + tb.Step("find", "ubuntu", tb.Command("/bin/bash"), + tb.Args("-c", "find /workspace"), ), )), tb.Task("check-create-files-exists", namespace, tb.TaskSpec( tb.TaskInputs(inWorkspaceResource), tb.TaskOutputs(outWorkspaceResource), + tb.Step("find", "ubuntu", tb.Command("/bin/bash"), + tb.Args("-c", "find /workspace"), + ), tb.Step("read-from-task-0", "ubuntu", tb.Command("/bin/bash"), - tb.Args("-c", "[[ stuff == $(cat /workspace//workspace/stuff) ]]"), + tb.Args("-c", "[[ stuff == $(cat /workspace/workspace/stuff) ]]"), ), tb.Step("write-data-task-1", "ubuntu", tb.Command("/bin/bash"), - tb.Args("-c", "echo something > /workspace/workspace/something"), + tb.Args("-c", "mkdir -p /workspace/output/workspace && cp -r /workspace/workspace/. /workspace/output/workspace && echo something > /workspace/output/workspace/something"), ), )), tb.Task("check-create-files-exists-2", namespace, tb.TaskSpec( tb.TaskInputs(inWorkspaceResource), tb.TaskOutputs(outWorkspaceResource), + tb.Step("find", "ubuntu", tb.Command("/bin/bash"), + tb.Args("-c", "find /workspace"), + ), tb.Step("read-from-task-0", "ubuntu", tb.Command("/bin/bash"), tb.Args("-c", "[[ other == $(cat /workspace/workspace/other) ]]"), ), tb.Step("write-data-task-1", "ubuntu", tb.Command("/bin/bash"), - tb.Args("-c", "echo else > /workspace/workspace/else"), + tb.Args("-c", "mkdir -p /workspace/output/workspace && cp -r /workspace/workspace/. /workspace/output/workspace && echo else > /workspace/output/workspace/else"), ), )), tb.Task("read-files", namespace, tb.TaskSpec( tb.TaskInputs(tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit, tb.ResourceTargetPath("readingspace"), )), + tb.Step("find", "ubuntu", tb.Command("/bin/bash"), + tb.Args("-c", "find /workspace"), + ), tb.Step("read-from-task-0", "ubuntu", tb.Command("/bin/bash"), tb.Args("-c", "[[ stuff == $(cat /workspace/readingspace/stuff) ]]"), ),