diff --git a/pkg/apis/pipeline/v1alpha1/resource_types.go b/pkg/apis/pipeline/v1alpha1/resource_types.go index c46b54f3c9e..074d49811e1 100644 --- a/pkg/apis/pipeline/v1alpha1/resource_types.go +++ b/pkg/apis/pipeline/v1alpha1/resource_types.go @@ -30,6 +30,13 @@ import ( // additional metatdata should be provided for it. type PipelineResourceType string +var ( + AllowedOutputResources = map[PipelineResourceType]bool{ + PipelineResourceTypeStorage: true, + PipelineResourceTypeGit: true, + } +) + const ( // PipelineResourceTypeGit indicates that this source is a GitHub repo. PipelineResourceTypeGit PipelineResourceType = "git" diff --git a/pkg/artifacts/artifact_storage_test.go b/pkg/artifacts/artifact_storage_test.go index f507f02d94e..23f66503b62 100644 --- a/pkg/artifacts/artifact_storage_test.go +++ b/pkg/artifacts/artifact_storage_test.go @@ -57,11 +57,53 @@ var ( quantityComparer = cmp.Comparer(func(x, y resource.Quantity) bool { return x.Cmp(y) == 0 }) + + pipelineWithtasksWithFrom = v1alpha1.Pipeline{ + Spec: v1alpha1.PipelineSpec{ + Resources: []v1alpha1.PipelineDeclaredResource{ + { + Name: "input1", + Type: "git", + }, + { + Name: "output", + Type: "git", + }, + }, + Tasks: []v1alpha1.PipelineTask{ + { + Name: "task1", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Outputs: []v1alpha1.PipelineTaskOutputResource{{ + Name: "foo", + Resource: "output", + }}, + }, + }, + { + Name: "task2", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Inputs: []v1alpha1.PipelineTaskInputResource{{ + Name: "foo", + Resource: "output", + From: []string{"task1"}, + }}, + }, + }, + }, + }, + } ) func GetPersistentVolumeClaim(size string, storageClassName *string) *corev1.PersistentVolumeClaim { pvc := &corev1.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{Name: "pipelineruntest-pvc", Namespace: "foo", OwnerReferences: pipelinerun.GetOwnerReference()}, + ObjectMeta: metav1.ObjectMeta{Name: "pipelineruntest-pvc", Namespace: pipelinerun.Namespace, OwnerReferences: pipelinerun.GetOwnerReference()}, Spec: corev1.PersistentVolumeClaimSpec{ AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, Resources: corev1.ResourceRequirements{Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse(size)}}, @@ -71,7 +113,7 @@ func GetPersistentVolumeClaim(size string, storageClassName *string) *corev1.Per return pvc } -func TestNeedsPVC(t *testing.T) { +func TestConfigMapNeedsPVC(t *testing.T) { logger := logtesting.TestLogger(t) for _, c := range []struct { desc string @@ -141,12 +183,12 @@ func TestNeedsPVC(t *testing.T) { pvcNeeded: false, }} { t.Run(c.desc, func(t *testing.T) { - needed, err := NeedsPVC(c.configMap, nil, logger) + needed, err := ConfigMapNeedsPVC(c.configMap, nil, logger) if err != nil { t.Fatalf("Somehow had error checking if PVC was needed run: %s", err) } if needed != c.pvcNeeded { - t.Fatalf("Expected that NeedsPVC would be %t, but was %t", c.pvcNeeded, needed) + t.Fatalf("Expected that ConfigMapNeedsPVC would be %t, but was %t", c.pvcNeeded, needed) } }) } @@ -158,7 +200,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { for _, c := range []struct { desc string configMap *corev1.ConfigMap - pipelinerun *v1alpha1.PipelineRun expectedArtifactStorage ArtifactStorageInterface storagetype string }{{ @@ -172,7 +213,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { PvcSizeKey: "10Gi", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: GetPersistentVolumeClaim("10Gi", defaultStorageClass), @@ -190,7 +230,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { PvcStorageClassNameKey: customStorageClass, }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: GetPersistentVolumeClaim("5Gi", &customStorageClass), @@ -210,7 +249,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactBucket{ Location: "gs://fake-bucket", Secrets: []v1alpha1.SecretParam{{ @@ -235,7 +273,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, @@ -254,7 +291,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, @@ -269,7 +305,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { Name: v1alpha1.BucketConfigName, }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", PersistentVolumeClaim: persistentVolumeClaim, @@ -287,7 +322,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketLocationKey: "gs://fake-bucket", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactBucket{ Location: "gs://fake-bucket", ShellImage: "busybox", @@ -308,7 +342,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountFieldName: "BOTO_CONFIG", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactBucket{ Location: "s3://fake-bucket", ShellImage: "busybox", @@ -323,20 +356,23 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) - artifactStorage, err := InitializeArtifactStorage(images, c.pipelinerun, fakekubeclient, logger) + artifactStorage, err := InitializeArtifactStorage(images, pipelinerun, &pipelineWithtasksWithFrom.Spec, fakekubeclient, logger) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } + if artifactStorage == nil { + t.Fatal("artifactStorage was nil, expected an actual value") + } // If the expected storage type is PVC, make sure we're actually creating that PVC. if c.storagetype == "pvc" { - _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(c.pipelinerun.Namespace).Get(GetPVCName(c.pipelinerun), metav1.GetOptions{}) + _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) if err != nil { - t.Fatalf("Error getting expected PVC %s for PipelineRun %s: %s", GetPVCName(c.pipelinerun), c.pipelinerun.Name, err) + t.Fatalf("Error getting expected PVC %s for PipelineRun %s: %s", GetPVCName(pipelinerun), pipelinerun.Name, err) } } // Make sure we don't get any errors running CleanupArtifactStorage against the resulting storage, whether it's // a bucket or a PVC. - if err := CleanupArtifactStorage(c.pipelinerun, fakekubeclient, logger); err != nil { + if err := CleanupArtifactStorage(pipelinerun, fakekubeclient, logger); err != nil { t.Fatalf("Error cleaning up artifact storage: %s", err) } if diff := cmp.Diff(artifactStorage.GetType(), c.storagetype); diff != "" { @@ -349,88 +385,192 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { } } -func TestCleanupArtifactStorage(t *testing.T) { +func TestInitializeArtifactStorageNoStorageNeeded(t *testing.T) { logger := logtesting.TestLogger(t) + // This Pipeline has Tasks that use both inputs and outputs, but there is + // no link between the inputs and outputs, so no storage is needed + pipeline := &v1alpha1.Pipeline{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "pipelineruntest", + }, + Spec: v1alpha1.PipelineSpec{ + Tasks: []v1alpha1.PipelineTask{ + { + Name: "task1", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Inputs: []v1alpha1.PipelineTaskInputResource{{ + Name: "input1", + Resource: "resource", + }}, + Outputs: []v1alpha1.PipelineTaskOutputResource{{ + Name: "output", + Resource: "resource", + }}, + }, + }, + { + Name: "task2", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Inputs: []v1alpha1.PipelineTaskInputResource{{ + Name: "input1", + Resource: "resource", + }}, + Outputs: []v1alpha1.PipelineTaskOutputResource{{ + Name: "output", + Resource: "resource", + }}, + }, + }, + }, + }, + } + pipelinerun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelinerun", + Namespace: "namespace", + }, + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: &v1alpha1.PipelineRef{ + Name: "pipeline", + }, + }, + } for _, c := range []struct { - desc string - configMap *corev1.ConfigMap - pipelinerun *v1alpha1.PipelineRun + desc string + configMap *corev1.ConfigMap }{{ - desc: "location empty", + desc: "has pvc configured", configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), - Name: v1alpha1.BucketConfigName, + Name: PvcConfigName, }, Data: map[string]string{ - v1alpha1.BucketLocationKey: "", - v1alpha1.BucketServiceAccountSecretName: "secret1", - v1alpha1.BucketServiceAccountSecretKey: "sakey", + PvcSizeKey: "10Gi", }, }, - pipelinerun: &v1alpha1.PipelineRun{ + }, { + desc: "has bucket configured", + configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo", - Name: "pipelineruntest", + Namespace: system.GetNamespace(), + Name: v1alpha1.BucketConfigName, + }, + Data: map[string]string{ + v1alpha1.BucketLocationKey: "gs://fake-bucket", + v1alpha1.BucketServiceAccountSecretName: "secret1", + v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, }, { - desc: "missing location", + desc: "no configmap", configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), Name: v1alpha1.BucketConfigName, }, Data: map[string]string{ + v1alpha1.BucketLocationKey: "", v1alpha1.BucketServiceAccountSecretName: "secret1", v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: &v1alpha1.PipelineRun{ + }} { + t.Run(c.desc, func(t *testing.T) { + fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) + artifactStorage, err := InitializeArtifactStorage(images, pipelinerun, &pipeline.Spec, fakekubeclient, logger) + if err != nil { + t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) + } + if artifactStorage.GetType() != "none" { + t.Errorf("Expected NoneArtifactStorage when none is needed but got %s", artifactStorage.GetType()) + } + }) + } +} + +func TestCleanupArtifactStorage(t *testing.T) { + pipelinerun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "pipelineruntest", + }, + } + logger := logtesting.TestLogger(t) + for _, c := range []struct { + desc string + configMap *corev1.ConfigMap + }{{ + desc: "location empty", + configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo", - Name: "pipelineruntest", + Namespace: system.GetNamespace(), + Name: v1alpha1.BucketConfigName, + }, + Data: map[string]string{ + v1alpha1.BucketLocationKey: "", + v1alpha1.BucketServiceAccountSecretName: "secret1", + v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, }, { - desc: "no config map data", + desc: "missing location", configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.GetNamespace(), Name: v1alpha1.BucketConfigName, }, + Data: map[string]string{ + v1alpha1.BucketServiceAccountSecretName: "secret1", + v1alpha1.BucketServiceAccountSecretKey: "sakey", + }, }, - pipelinerun: &v1alpha1.PipelineRun{ + }, { + desc: "no config map data", + configMap: &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ - Namespace: "foo", - Name: "pipelineruntest", + Namespace: system.GetNamespace(), + Name: v1alpha1.BucketConfigName, }, }, }} { t.Run(c.desc, func(t *testing.T) { - fakekubeclient := fakek8s.NewSimpleClientset(c.configMap, GetPVCSpec(c.pipelinerun, persistentVolumeClaim.Spec.Resources.Requests["storage"], defaultStorageClass)) - _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(c.pipelinerun.Namespace).Get(GetPVCName(c.pipelinerun), metav1.GetOptions{}) + fakekubeclient := fakek8s.NewSimpleClientset(c.configMap, GetPVCSpec(pipelinerun, persistentVolumeClaim.Spec.Resources.Requests["storage"], defaultStorageClass)) + _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) if err != nil { - t.Fatalf("Error getting expected PVC %s for PipelineRun %s: %s", GetPVCName(c.pipelinerun), c.pipelinerun.Name, err) + t.Fatalf("Error getting expected PVC %s for PipelineRun %s: %s", GetPVCName(pipelinerun), pipelinerun.Name, err) } - if err := CleanupArtifactStorage(c.pipelinerun, fakekubeclient, logger); err != nil { + if err := CleanupArtifactStorage(pipelinerun, fakekubeclient, logger); err != nil { t.Fatalf("Error cleaning up artifact storage: %s", err) } - _, err = fakekubeclient.CoreV1().PersistentVolumeClaims(c.pipelinerun.Namespace).Get(GetPVCName(c.pipelinerun), metav1.GetOptions{}) + _, err = fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) if err == nil { - t.Fatalf("Found PVC %s for PipelineRun %s after it should have been cleaned up", GetPVCName(c.pipelinerun), c.pipelinerun.Name) + t.Fatalf("Found PVC %s for PipelineRun %s after it should have been cleaned up", GetPVCName(pipelinerun), pipelinerun.Name) } else if !errors.IsNotFound(err) { - t.Fatalf("Error checking if PVC %s for PipelineRun %s has been cleaned up: %s", GetPVCName(c.pipelinerun), c.pipelinerun.Name, err) + t.Fatalf("Error checking if PVC %s for PipelineRun %s has been cleaned up: %s", GetPVCName(pipelinerun), pipelinerun.Name, err) } }) } } func TestInitializeArtifactStorageWithoutConfigMap(t *testing.T) { + pipelinerun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelineruntest", + Namespace: "foo", + }, + } logger := logtesting.TestLogger(t) fakekubeclient := fakek8s.NewSimpleClientset() - pvc, err := InitializeArtifactStorage(images, pipelinerun, fakekubeclient, logger) + pvc, err := InitializeArtifactStorage(images, pipelinerun, &pipelineWithtasksWithFrom.Spec, fakekubeclient, logger) if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } @@ -447,6 +587,12 @@ func TestInitializeArtifactStorageWithoutConfigMap(t *testing.T) { } func TestGetArtifactStorageWithConfigMap(t *testing.T) { + pipelinerun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "pipelineruntest", + }, + } logger := logtesting.TestLogger(t) for _, c := range []struct { desc string diff --git a/pkg/artifacts/artifacts_storage.go b/pkg/artifacts/artifacts_storage.go index 9c5fdfd6128..0c071b65660 100644 --- a/pkg/artifacts/artifacts_storage.go +++ b/pkg/artifacts/artifacts_storage.go @@ -57,11 +57,67 @@ type ArtifactStorageInterface interface { StorageBasePath(pr *v1alpha1.PipelineRun) string } +// ArtifactStorageNone is used when no storage is needed. +type ArtifactStorageNone struct{} + +// GetCopyToStorageFromSteps returns no containers because none are needed. +func (a *ArtifactStorageNone) GetCopyToStorageFromSteps(name, sourcePath, destinationPath string) []v1alpha1.Step { + return nil +} + +// GetCopyFromStorageToSteps returns no containers because none are needed. +func (a *ArtifactStorageNone) GetCopyFromStorageToSteps(name, sourcePath, destinationPath string) []v1alpha1.Step { + return nil +} + +// GetSecretsVolumes returns no volumes because none are needed. +func (a *ArtifactStorageNone) GetSecretsVolumes() []corev1.Volume { + return nil +} + +// GetType returns the string "none" to indicate this is the None storage type. +func (a *ArtifactStorageNone) GetType() string { + return "none" +} + +// StorageBasePath returns an empty string because no storage is being used and so +// there is no path that resources should be copied from / to. +func (a *ArtifactStorageNone) StorageBasePath(pr *v1alpha1.PipelineRun) string { + return "" +} + // InitializeArtifactStorage will check if there is there is a -// bucket configured or create a PVC -func InitializeArtifactStorage(images pipeline.Images, pr *v1alpha1.PipelineRun, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { +// bucket configured, create a PVC or return nil if no storage is required. +func InitializeArtifactStorage(images pipeline.Images, pr *v1alpha1.PipelineRun, ps *v1alpha1.PipelineSpec, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { + // Artifact storage is needed under the following condition: + // Any Task in the pipeline contains an Output resource + // AND that Output resource is one of the AllowedOutputResource types. + + needStorage := false + // Build an index of resources used in the pipeline that are an AllowedOutputResource + possibleOutputs := map[string]struct{}{} + for _, r := range ps.Resources { + if _, ok := v1alpha1.AllowedOutputResources[r.Type]; ok { + possibleOutputs[r.Name] = struct{}{} + } + } + + // Use that index to see if any of these are used as OutputResources. + for _, t := range ps.Tasks { + if t.Resources != nil { + for _, o := range t.Resources.Outputs { + if _, ok := possibleOutputs[o.Resource]; ok { + needStorage = true + } + } + } + } + if !needStorage { + return &ArtifactStorageNone{}, nil + } + configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(v1alpha1.BucketConfigName, metav1.GetOptions{}) - shouldCreatePVC, err := NeedsPVC(configMap, err, logger) + shouldCreatePVC, err := ConfigMapNeedsPVC(configMap, err, logger) if err != nil { return nil, err } @@ -80,7 +136,7 @@ func InitializeArtifactStorage(images pipeline.Images, pr *v1alpha1.PipelineRun, // an output workspace or artifacts from one Task to another Task. No other PVCs will be impacted by this cleanup. func CleanupArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface, logger *zap.SugaredLogger) error { configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(v1alpha1.BucketConfigName, metav1.GetOptions{}) - shouldCreatePVC, err := NeedsPVC(configMap, err, logger) + shouldCreatePVC, err := ConfigMapNeedsPVC(configMap, err, logger) if err != nil { return err } @@ -93,9 +149,9 @@ func CleanupArtifactStorage(pr *v1alpha1.PipelineRun, c kubernetes.Interface, lo return nil } -// NeedsPVC checks if the possibly-nil config map passed to it is configured to use a bucket for artifact storage, +// ConfigMapNeedsPVC checks if the possibly-nil config map passed to it is configured to use a bucket for artifact storage, // returning true if instead a PVC is needed. -func NeedsPVC(configMap *corev1.ConfigMap, err error, logger *zap.SugaredLogger) (bool, error) { +func ConfigMapNeedsPVC(configMap *corev1.ConfigMap, err error, logger *zap.SugaredLogger) (bool, error) { if err != nil { if errors.IsNotFound(err) { return true, nil @@ -124,7 +180,7 @@ func NeedsPVC(configMap *corev1.ConfigMap, err error, logger *zap.SugaredLogger) // consumer code to get a container step for copy to/from storage func GetArtifactStorage(images pipeline.Images, prName string, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { configMap, err := c.CoreV1().ConfigMaps(system.GetNamespace()).Get(v1alpha1.BucketConfigName, metav1.GetOptions{}) - pvc, err := NeedsPVC(configMap, err, logger) + pvc, err := ConfigMapNeedsPVC(configMap, err, logger) if err != nil { return nil, xerrors.Errorf("couldn't determine if PVC was needed from config map: %w", err) } diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index d3f6e086b1c..9d0b2087ed8 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -417,7 +417,8 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er rprts := pipelineState.GetNextTasks(candidateTasks) var as artifacts.ArtifactStorageInterface - if as, err = artifacts.InitializeArtifactStorage(c.Images, pr, c.KubeClientSet, c.Logger); err != nil { + + if as, err = artifacts.InitializeArtifactStorage(c.Images, pr, pipelineSpec, c.KubeClientSet, c.Logger); err != nil { c.Logger.Infof("PipelineRun failed to initialize artifact storage %s", pr.Name) return err } diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index f5ddbc15c59..374936192c2 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -830,6 +830,54 @@ func TestReconcileWithTimeout(t *testing.T) { } } +func TestReconcileWithoutPVC(t *testing.T) { + ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec( + tb.PipelineTask("hello-world-1", "hello-world"), + tb.PipelineTask("hello-world-2", "hello-world"), + ))} + + prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run", "foo", + tb.PipelineRunSpec("test-pipeline")), + } + ts := []*v1alpha1.Task{tb.Task("hello-world", "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.Tekton().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) + } + + // Check that the expected TaskRun was created + for _, a := range clients.Kube.Actions() { + if ca, ok := a.(ktesting.CreateAction); ok { + obj := ca.GetObject() + if pvc, ok := obj.(*corev1.PersistentVolumeClaim); ok { + t.Errorf("Did not expect to see a PVC created when no resources are linked. %s was created", pvc) + } + } + } + + 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 TestReconcileCancelledPipelineRun(t *testing.T) { ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec( tb.PipelineTask("hello-world-1", "hello-world", tb.Retries(1)), diff --git a/pkg/reconciler/taskrun/resources/input_resources.go b/pkg/reconciler/taskrun/resources/input_resources.go index 2d77db2b68c..13324855a54 100644 --- a/pkg/reconciler/taskrun/resources/input_resources.go +++ b/pkg/reconciler/taskrun/resources/input_resources.go @@ -86,7 +86,7 @@ func AddInputResource( dPath := destinationPath(input.Name, input.TargetPath) // if taskrun is fetching resource from previous task then execute copy step instead of fetching new copy // to the desired destination directory, as long as the resource exports output to be copied - if allowedOutputResources[resource.GetType()] && taskRun.HasPipelineRunOwnerReference() { + if v1alpha1.AllowedOutputResources[resource.GetType()] && taskRun.HasPipelineRunOwnerReference() { for _, path := range boundResource.Paths { cpSteps := as.GetCopyFromStorageToSteps(boundResource.Name, path, dPath) if as.GetType() == v1alpha1.ArtifactStoragePVCType { diff --git a/pkg/reconciler/taskrun/resources/output_resource.go b/pkg/reconciler/taskrun/resources/output_resource.go index 436f81245d9..65f09920e5d 100644 --- a/pkg/reconciler/taskrun/resources/output_resource.go +++ b/pkg/reconciler/taskrun/resources/output_resource.go @@ -29,13 +29,6 @@ import ( var ( outputDir = "/workspace/output/" - - // allowedOutputResource checks if an output resource type produces - // an output that should be copied to the PVC - allowedOutputResources = map[v1alpha1.PipelineResourceType]bool{ - v1alpha1.PipelineResourceTypeStorage: true, - v1alpha1.PipelineResourceTypeGit: true, - } ) // AddOutputResources reads the output resources and adds the corresponding container steps @@ -94,10 +87,12 @@ func AddOutputResources( mkdirSteps := []v1alpha1.Step{v1alpha1.CreateDirStep(images.ShellImage, boundResource.Name, sourcePath)} taskSpec.Steps = append(mkdirSteps, taskSpec.Steps...) - if allowedOutputResources[resource.GetType()] && taskRun.HasPipelineRunOwnerReference() { + needsPvc := false + if v1alpha1.AllowedOutputResources[resource.GetType()] && taskRun.HasPipelineRunOwnerReference() { var newSteps []v1alpha1.Step for _, dPath := range boundResource.Paths { newSteps = append(newSteps, as.GetCopyToStorageFromSteps(resource.GetName(), sourcePath, dPath)...) + needsPvc = true } taskSpec.Steps = append(taskSpec.Steps, newSteps...) taskSpec.Volumes = append(taskSpec.Volumes, as.GetSecretsVolumes()...) @@ -124,7 +119,9 @@ func AddOutputResources( return taskSpec, nil } } - taskSpec.Volumes = append(taskSpec.Volumes, GetPVCVolume(pvcName)) + if needsPvc { + taskSpec.Volumes = append(taskSpec.Volumes, GetPVCVolume(pvcName)) + } } } return taskSpec, nil diff --git a/pkg/reconciler/taskrun/resources/output_resource_test.go b/pkg/reconciler/taskrun/resources/output_resource_test.go index fbe323ca3fa..a6d2ae9def1 100644 --- a/pkg/reconciler/taskrun/resources/output_resource_test.go +++ b/pkg/reconciler/taskrun/resources/output_resource_test.go @@ -185,6 +185,17 @@ func TestValidOutputResources(t *testing.T) { MountPath: "/pvc", }}, }}}, + wantVolumes: []corev1.Volume{ + { + Name: "pipelinerun-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pipelinerun-pvc", + ReadOnly: false, + }, + }, + }, + }, }, { name: "git resource in output only", desc: "git resource declared as output with pipelinerun owner reference", @@ -247,6 +258,17 @@ func TestValidOutputResources(t *testing.T) { MountPath: "/pvc", }}, }}}, + wantVolumes: []corev1.Volume{ + { + Name: "pipelinerun-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pipelinerun-pvc", + ReadOnly: false, + }, + }, + }, + }, }, { name: "image resource in output with pipelinerun with owner", desc: "image resource declared as output with pipelinerun owner reference should not generate any steps", @@ -432,7 +454,17 @@ func TestValidOutputResources(t *testing.T) { VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{SecretName: "sname"}, }, - }}, + }, + { + Name: "pipelinerun-parent-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pipelinerun-parent-pvc", + ReadOnly: false, + }, + }, + }, + }, }, { name: "storage resource as output", desc: "storage resource defined only in output with pipeline ownder reference", @@ -510,7 +542,16 @@ func TestValidOutputResources(t *testing.T) { VolumeSource: corev1.VolumeSource{ Secret: &corev1.SecretVolumeSource{SecretName: "sname"}, }, - }}, + }, + { + Name: "pipelinerun-pvc", + VolumeSource: corev1.VolumeSource{ + PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pipelinerun-pvc", + }, + }, + }, + }, }, { name: "storage resource as output with no owner", desc: "storage resource defined only in output without pipelinerun reference", @@ -779,20 +820,6 @@ func TestValidOutputResources(t *testing.T) { if d := cmp.Diff(c.wantSteps, got.Steps); d != "" { t.Fatalf("post build steps mismatch (-want, +got): %s", d) } - - if c.taskRun.GetPipelineRunPVCName() != "" { - c.wantVolumes = append( - c.wantVolumes, - corev1.Volume{ - Name: c.taskRun.GetPipelineRunPVCName(), - VolumeSource: corev1.VolumeSource{ - PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ - ClaimName: c.taskRun.GetPipelineRunPVCName(), - }, - }, - }, - ) - } if d := cmp.Diff(c.wantVolumes, got.Volumes); d != "" { t.Fatalf("post build steps volumes mismatch (-want, +got): %s", d) }