From 3882f07a8bf3ca2adef14ae39aaa4f6f7e48f13e Mon Sep 17 00:00:00 2001 From: Dan Lorenc Date: Tue, 25 Jun 2019 16:14:25 +0800 Subject: [PATCH] =?UTF-8?q?If=20no=20resources=20are=20linked,=20don't=20m?= =?UTF-8?q?ake=20a=20PVC=20=F0=9F=97=91=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When ouputs of a Task are linked to the inputs of another we create a PVC (or use a bucket) to share the data between executing Tasks. However we were doing this even when no Resources were linked! Now we only do it when we need to. Co-authored-by: Christie Wilson Fixes: #937 --- pkg/artifacts/artifact_storage_test.go | 268 +++++++++++++++--- pkg/artifacts/artifacts_storage.go | 58 +++- pkg/reconciler/pipelinerun/pipelinerun.go | 3 +- .../pipelinerun/pipelinerun_test.go | 49 ++++ 4 files changed, 330 insertions(+), 48 deletions(-) diff --git a/pkg/artifacts/artifact_storage_test.go b/pkg/artifacts/artifact_storage_test.go index f507f02d94e..d9d60abac15 100644 --- a/pkg/artifacts/artifact_storage_test.go +++ b/pkg/artifacts/artifact_storage_test.go @@ -57,11 +57,40 @@ var ( quantityComparer = cmp.Comparer(func(x, y resource.Quantity) bool { return x.Cmp(y) == 0 }) + + tasksWithFrom = []v1alpha1.PipelineTask{ + { + Name: "task1", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Outputs: []v1alpha1.PipelineTaskOutputResource{{ + Name: "output", + Resource: "resource", + }}, + }, + }, + { + Name: "task2", + TaskRef: v1alpha1.TaskRef{ + Name: "task", + }, + Resources: &v1alpha1.PipelineTaskResources{ + Inputs: []v1alpha1.PipelineTaskInputResource{{ + Name: "input1", + Resource: "resource", + From: []string{"task1"}, + }}, + }, + }, + } ) -func GetPersistentVolumeClaim(size string, storageClassName *string) *corev1.PersistentVolumeClaim { + +func GetPersistentVolumeClaim(pipelinerun *v1alpha1.PipelineRun, size, 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 +100,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 +170,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) } }) } @@ -154,11 +183,20 @@ func TestNeedsPVC(t *testing.T) { } func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { + pipelinerun := &v1alpha1.PipelineRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pipelineruntest", + }, + Spec: v1alpha1.PipelineRunSpec{ + PipelineRef: v1alpha1.PipelineRef{ + Name: "pipelineWithFrom", + }, + }, + } logger := logtesting.TestLogger(t) for _, c := range []struct { desc string configMap *corev1.ConfigMap - pipelinerun *v1alpha1.PipelineRun expectedArtifactStorage ArtifactStorageInterface storagetype string }{{ @@ -172,9 +210,9 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { PvcSizeKey: "10Gi", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", +<<<<<<< HEAD PersistentVolumeClaim: GetPersistentVolumeClaim("10Gi", defaultStorageClass), ShellImage: "busybox", }, @@ -195,6 +233,9 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { Name: "pipelineruntest", PersistentVolumeClaim: GetPersistentVolumeClaim("5Gi", &customStorageClass), ShellImage: "busybox", +======= + PersistentVolumeClaim: GetPersistentVolumeClaim(pipelinerun, "10Gi"), +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ }, storagetype: "pvc", }, { @@ -210,7 +251,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactBucket{ Location: "gs://fake-bucket", Secrets: []v1alpha1.SecretParam{{ @@ -235,11 +275,14 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", +<<<<<<< HEAD PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", +======= + PersistentVolumeClaim: GetPersistentVolumeClaim(pipelinerun, DefaultPvcSize), +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ }, storagetype: "pvc", }, { @@ -254,11 +297,14 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketServiceAccountSecretKey: "sakey", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", +<<<<<<< HEAD PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", +======= + PersistentVolumeClaim: GetPersistentVolumeClaim(pipelinerun, DefaultPvcSize), +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ }, storagetype: "pvc", }, { @@ -269,11 +315,14 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { Name: v1alpha1.BucketConfigName, }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", +<<<<<<< HEAD PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", +======= + PersistentVolumeClaim: GetPersistentVolumeClaim(pipelinerun, DefaultPvcSize), +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ }, storagetype: "pvc", }, { @@ -287,7 +336,6 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { v1alpha1.BucketLocationKey: "gs://fake-bucket", }, }, - pipelinerun: pipelinerun, expectedArtifactStorage: &v1alpha1.ArtifactBucket{ Location: "gs://fake-bucket", ShellImage: "busybox", @@ -323,130 +371,270 @@ func TestInitializeArtifactStorageWithConfigMap(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { fakekubeclient := fakek8s.NewSimpleClientset(c.configMap) +<<<<<<< HEAD artifactStorage, err := InitializeArtifactStorage(images, c.pipelinerun, fakekubeclient, logger) +======= + artifactStorage, err := InitializeArtifactStorage(pipelinerun, tasksWithFrom, fakekubeclient, logger) +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ 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 != "" { +<<<<<<< HEAD t.Fatalf("-want +got: %s", diff) } if diff := cmp.Diff(artifactStorage, c.expectedArtifactStorage, quantityComparer); diff != "" { t.Fatalf("-want +got: %s", diff) +======= + t.Fatalf(diff) + } + if diff := cmp.Diff(artifactStorage, c.expectedArtifactStorage, quantityComparer); diff != "" { + t.Fatalf(diff) +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ } }) } } -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(pipelinerun, pipeline.Spec.Tasks, 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) { +<<<<<<< HEAD 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, GetPersistentVolumeClaim(pipelinerun, DefaultPvcSize).Spec.Resources.Requests["storage"])) + _, err := fakekubeclient.CoreV1().PersistentVolumeClaims(pipelinerun.Namespace).Get(GetPVCName(pipelinerun), metav1.GetOptions{}) +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ 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", + }, + } logger := logtesting.TestLogger(t) fakekubeclient := fakek8s.NewSimpleClientset() +<<<<<<< HEAD pvc, err := InitializeArtifactStorage(images, pipelinerun, fakekubeclient, logger) +======= + pvc, err := InitializeArtifactStorage(pipelinerun, tasksWithFrom, fakekubeclient, logger) +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ if err != nil { t.Fatalf("Somehow had error initializing artifact storage run out of fake client: %s", err) } expectedArtifactPVC := &v1alpha1.ArtifactPVC{ Name: "pipelineruntest", +<<<<<<< HEAD PersistentVolumeClaim: persistentVolumeClaim, ShellImage: "busybox", } if diff := cmp.Diff(pvc, expectedArtifactPVC, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Fatalf("-want +got: %s", diff) +======= + PersistentVolumeClaim: GetPersistentVolumeClaim(pipelinerun, DefaultPvcSize), + } + + if diff := cmp.Diff(pvc, expectedArtifactPVC, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { + t.Fatal(diff) +>>>>>>> 1d99c320... If no resources are linked, don't make a PVC 🗑️ } } 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..591b21c648a 100644 --- a/pkg/artifacts/artifacts_storage.go +++ b/pkg/artifacts/artifacts_storage.go @@ -57,11 +57,55 @@ 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, ts []v1alpha1.PipelineTask, c kubernetes.Interface, logger *zap.SugaredLogger) (ArtifactStorageInterface, error) { + // Artifact storage is only required if a pipeline has tasks that take inputs from previous tasks. + needStorage := false + for _, t := range ts { + if t.Resources != nil { + for _, i := range t.Resources.Inputs { + if len(i.From) != 0 { + 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 +124,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 +137,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 +168,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..2a6c3326a7b 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.Tasks, 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..87ea187fa2c 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -830,6 +830,55 @@ 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, + } + + // create fake recorder for testing + fr := record.NewFakeRecorder(2) + + testAssets := getPipelineRunController(t, d, fr) + 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)),