Skip to content

Commit

Permalink
Add subPath to WorkspacePipelineTaskBinding
Browse files Browse the repository at this point in the history
Use case: Use two instances of a task that writes to the same workspace volume - but
they write to different path of the volume. A typical use case is two git-clone tasks
that clone two git repositories, but the files should be located in two different
directories on the workspace volume.

This commit solves this by adding the `subPath` field to the WorkspacePipelineTaskBinding.
In addition, it forbids the `subPath` field in the PipelineRun/TaskRun when using
`volumeClaimTemplate`. It also validates the workspaceBinding on `PipelineRun` to check
that the subPath field is not used on a volumeClaimTemplate workspace binding - it should
instead be set when binding the workspace in the PipelineTask.
  • Loading branch information
jlpettersson committed Apr 24, 2020
1 parent 35d5121 commit 30ee0ed
Show file tree
Hide file tree
Showing 11 changed files with 206 additions and 8 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
apiVersion: tekton.dev/v1beta1
kind: Task
metadata:
name: writer
spec:
steps:
- name: write
image: ubuntu
script: echo bar > $(workspaces.task-ws.path)/foo
workspaces:
- name: task-ws
---
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
name: pipeline-using-different-subpaths
spec:
tasks:
- name: writer-1
taskRef:
name: writer
workspaces:
- name: task-ws
workspace: ws
subPath: dir-1
- name: writer-2
taskRef:
name: writer
workspaces:
- name: task-ws
workspace: ws
subPath: dir-2
- name: read-all
runAfter:
- writer-1
- writer-2
taskSpec:
steps:
- name: read-1
image: ubuntu
script: cat $(workspaces.local-ws.path)/dir-1/foo | grep bar
- name: read-2
image: ubuntu
script: cat $(workspaces.local-ws.path)/dir-2/foo | grep bar
workspaces:
- name: local-ws
workspaces:
- name: local-ws
workspace: ws
workspaces:
- name: ws
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
generateName: pr-
spec:
pipelineRef:
name: pipeline-using-different-subpaths
workspaces:
- name: ws
volumeClaimTemplate:
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ func TestPipeline_Validate(t *testing.T) {
p: tb.Pipeline("name", tb.PipelineSpec(
tb.PipelineWorkspaceDeclaration("foo"),
tb.PipelineTask("taskname", "taskref",
tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", "pipelineWorkspaceName")),
tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", "pipelineWorkspaceName", "")),
)),
failureExpected: true,
}, {
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipelinerun_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) *apis.FieldError {
}
}
wsNames[ws.Name] = idx

if err := ws.Validate(ctx); err != nil {
return err
}
}
}

Expand Down
16 changes: 16 additions & 0 deletions pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,22 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
Message: `workspace "ws" provided by pipelinerun more than once, at index 0 and 1`,
Paths: []string{"spec.workspaces"},
},
}, {
name: "workspace with volumeClaimTemplate can not contain subPath field",
spec: v1beta1.PipelineRunSpec{
PipelineRef: &v1beta1.PipelineRef{
Name: "pipelinerefname",
},
Workspaces: []v1beta1.WorkspaceBinding{{
Name: "ws",
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{},
SubPath: "subdir",
}},
},
wantErr: &apis.FieldError{
Message: `must not set the field(s)`,
Paths: []string{"workspaces.volumeclaimtemplate.subpath"},
},
}}
for _, ps := range tests {
t.Run(ps.name, func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/pipeline/v1beta1/workspace_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,8 @@ type WorkspacePipelineTaskBinding struct {
Name string `json:"name"`
// Workspace is the name of the workspace declared by the pipeline
Workspace string `json:"workspace"`
// SubPath is optionally a directory on the volume which should be used
// for this binding (i.e. the volume will be mounted at this sub directory).
// +optional
SubPath string `json:"subPath,omitempty"`
}
4 changes: 4 additions & 0 deletions pkg/apis/pipeline/v1beta1/workspace_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func (b *WorkspaceBinding) Validate(ctx context.Context) *apis.FieldError {
return apis.ErrMissingField("workspace.persistentvolumeclaim.claimname")
}

if b.VolumeClaimTemplate != nil && b.SubPath != "" {
return apis.ErrDisallowedFields("workspaces.volumeclaimtemplate.subpath")
}

// For a ConfigMap to work, you must provide the name of the ConfigMap to use.
if b.ConfigMap != nil && b.ConfigMap.LocalObjectReference.Name == "" {
return apis.ErrMissingField("workspace.configmap.name")
Expand Down
19 changes: 19 additions & 0 deletions pkg/apis/pipeline/v1beta1/workspace_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,25 @@ func TestWorkspaceBindingValidateInvalid(t *testing.T) {
ClaimName: "pool-party",
},
},
}, {
name: "Provided volumeClaimTemplate with subPath field set",
binding: &WorkspaceBinding{
Name: "beth",
SubPath: "notallowed-when-using-volumeclaimtemplate-in-pipeline",
VolumeClaimTemplate: &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Name: "mypvc",
},
Spec: corev1.PersistentVolumeClaimSpec{
AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
"storage": resource.MustParse("1Gi"),
},
},
},
},
},
}, {
name: "Provided neither pvc nor emptydir",
binding: &WorkspaceBinding{
Expand Down
8 changes: 4 additions & 4 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -733,9 +733,9 @@ func (c *Reconciler) createTaskRun(rprt *resources.ResolvedPipelineRunTask, pr *
pipelineRunWorkspaces[binding.Name] = binding
}
for _, ws := range rprt.PipelineTask.Workspaces {
taskWorkspaceName, pipelineWorkspaceName := ws.Name, ws.Workspace
taskWorkspaceName, taskWorkspaceSubPath, pipelineWorkspaceName := ws.Name, ws.SubPath, ws.Workspace
if b, hasBinding := pipelineRunWorkspaces[pipelineWorkspaceName]; hasBinding {
tr.Spec.Workspaces = append(tr.Spec.Workspaces, taskWorkspaceByWorkspaceVolumeSource(b, taskWorkspaceName, pr.GetOwnerReference()))
tr.Spec.Workspaces = append(tr.Spec.Workspaces, taskWorkspaceByWorkspaceVolumeSource(b, taskWorkspaceName, taskWorkspaceSubPath, pr.GetOwnerReference()))
} else {
return nil, fmt.Errorf("expected workspace %q to be provided by pipelinerun for pipeline task %q", pipelineWorkspaceName, rprt.PipelineTask.Name)
}
Expand All @@ -748,7 +748,7 @@ func (c *Reconciler) createTaskRun(rprt *resources.ResolvedPipelineRunTask, pr *

// taskWorkspaceByWorkspaceVolumeSource is returning the WorkspaceBinding with the TaskRun specified name.
// If the volume source is a volumeClaimTemplate, the template is applied and passed to TaskRun as a persistentVolumeClaim
func taskWorkspaceByWorkspaceVolumeSource(wb v1alpha1.WorkspaceBinding, taskWorkspaceName string, owner metav1.OwnerReference) v1alpha1.WorkspaceBinding {
func taskWorkspaceByWorkspaceVolumeSource(wb v1alpha1.WorkspaceBinding, taskWorkspaceName string, taskWorkspaceSubPath string, owner metav1.OwnerReference) v1alpha1.WorkspaceBinding {
if wb.VolumeClaimTemplate == nil {
binding := *wb.DeepCopy()
binding.Name = taskWorkspaceName
Expand All @@ -757,7 +757,7 @@ func taskWorkspaceByWorkspaceVolumeSource(wb v1alpha1.WorkspaceBinding, taskWork

// apply template
binding := v1alpha1.WorkspaceBinding{
SubPath: wb.SubPath,
SubPath: taskWorkspaceSubPath,
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: volumeclaim.GetPersistentVolumeClaimName(wb.VolumeClaimTemplate, wb, owner),
},
Expand Down
84 changes: 83 additions & 1 deletion pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1718,7 +1718,7 @@ func TestReconcileWithVolumeClaimTemplateWorkspace(t *testing.T) {
claimName := "myclaim"
pipelineRunName := "test-pipeline-run"
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec(
tb.PipelineTask("hello-world-1", "hello-world", tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", workspaceName)),
tb.PipelineTask("hello-world-1", "hello-world", tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", workspaceName, "")),
tb.PipelineTask("hello-world-2", "hello-world"),
tb.PipelineWorkspaceDeclaration(workspaceName),
))}
Expand Down Expand Up @@ -1792,6 +1792,88 @@ func TestReconcileWithVolumeClaimTemplateWorkspace(t *testing.T) {
}
}

// TestReconcileWithVolumeClaimTemplateWorkspaceUsingSubPaths tests that given a pipeline with volumeClaimTemplate workspace and
// two instances of the same task, but using two different subPaths in the volume - is seen as two taskRuns with expected subPaths.
func TestReconcileWithVolumeClaimTemplateWorkspaceUsingSubPaths(t *testing.T) {
workspaceName := "ws1"
subPath1 := "customdirectory"
subPath2 := "otherdirecory"
claimName := "myclaim"
pipelineRunName := "test-pipeline-run"
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec(
tb.PipelineTask("hello-world-1", "hello-world", tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", workspaceName, subPath1)),
tb.PipelineTask("hello-world-2", "hello-world", tb.PipelineTaskWorkspaceBinding("taskWorkspaceName", workspaceName, subPath2)),
tb.PipelineWorkspaceDeclaration(workspaceName),
))}

prs := []*v1alpha1.PipelineRun{tb.PipelineRun(pipelineRunName, tb.PipelineRunNamespace("foo"),
tb.PipelineRunSpec("test-pipeline", tb.PipelineRunWorkspaceBindingVolumeClaimTemplate(workspaceName, claimName))),
}
ts := []*v1alpha1.Task{tb.Task("hello-world", tb.TaskNamespace("foo"))}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
}

testAssets, cancel := getPipelineRunController(t, d)
defer cancel()
c := testAssets.Controller
clients := testAssets.Clients

err := c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run")
if err != nil {
t.Errorf("Did not expect to see error when reconciling PipelineRun but saw %s", err)
}

// Check that the PipelineRun was reconciled correctly
reconciledRun, err := clients.Pipeline.TektonV1alpha1().PipelineRuns("foo").Get("test-pipeline-run", metav1.GetOptions{})
if err != nil {
t.Fatalf("Somehow had error getting reconciled run out of fake client: %s", err)
}

taskRuns, err := clients.Pipeline.TektonV1alpha1().TaskRuns("foo").List(metav1.ListOptions{})
if err != nil {
t.Fatalf("unexpected error when listing TaskRuns: %v", err)
}

if len(taskRuns.Items) != 2 {
t.Fatalf("unexpected number of taskRuns found, expected 2, but found %d", len(taskRuns.Items))
}

hasSeenWorkspaceWithSubPath1 := false
hasSeenWorkspaceWithSubPath2 := false
for _, tr := range taskRuns.Items {
for _, ws := range tr.Spec.Workspaces {

if ws.PersistentVolumeClaim == nil {
t.Fatalf("found taskRun workspace that is not PersistentVolumeClaim workspace. Did only expect PersistentVolumeClaims workspaces")
}

if ws.SubPath == subPath1 {
hasSeenWorkspaceWithSubPath1 = true
}

if ws.SubPath == subPath2 {
hasSeenWorkspaceWithSubPath2 = true
}
}
}

if !hasSeenWorkspaceWithSubPath1 {
t.Fatalf("did not see a taskRun with a workspace using subPath1")
}

if !hasSeenWorkspaceWithSubPath2 {
t.Fatalf("did not see a taskRun with a workspace using subPath2")
}

if !reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsUnknown() {
t.Errorf("Expected PipelineRun to be running, but condition status is %s", reconciledRun.Status.GetCondition(apis.ConditionSucceeded))
}
}

func TestReconcileWithTaskResults(t *testing.T) {
names.TestingSeed()
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec(
Expand Down
3 changes: 2 additions & 1 deletion test/builder/pipeline.go
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,12 @@ func PipelineTaskConditionResource(name, resource string, from ...string) Pipeli
}
}

func PipelineTaskWorkspaceBinding(name, workspace string) PipelineTaskOp {
func PipelineTaskWorkspaceBinding(name, workspace, subPath string) PipelineTaskOp {
return func(pt *v1alpha1.PipelineTask) {
pt.Workspaces = append(pt.Workspaces, v1alpha1.WorkspacePipelineTaskBinding{
Name: name,
Workspace: workspace,
SubPath: subPath,
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/builder/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestPipeline(t *testing.T) {
tb.PipelineTaskConditionParam("param-name", "param-value"),
tb.PipelineTaskConditionResource("some-resource", "my-only-git-resource", "bar", "never-gonna"),
),
tb.PipelineTaskWorkspaceBinding("task-workspace1", "workspace1"),
tb.PipelineTaskWorkspaceBinding("task-workspace1", "workspace1", ""),
),
tb.PipelineTask("bar", "chocolate",
tb.PipelineTaskRefKind(v1alpha1.ClusterTaskKind),
Expand Down

0 comments on commit 30ee0ed

Please sign in to comment.