From 25cfcdb12688b0b679cd4beb923662e580ef2da3 Mon Sep 17 00:00:00 2001 From: Martin Malina Date: Mon, 7 Aug 2023 14:48:24 +0200 Subject: [PATCH] fix(RHTAPBUGS-560): ensure each pipelinerun uses its own subdir When release pipelineruns are created, they will always use the same shared workspace (provided by the default PVC, or one defined in the given ReleaseStrategy). This wasn't an issue before, but now that we started using the workspace to share snapshot and other data from task to task, we found out that two simultaneously running pipelineruns can alter each other's data. We need to prevent that. This typically happens when you have two or more components in your app and so there are multiple pipeline runs, oftentimes running around the same time. The solution is to use a subdir when passing the PVC in the PR. The subdir is based on the Release name plus the PR's UID, so it should always be unique. Signed-off-by: Martin Malina --- controllers/release/adapter.go | 5 +++-- tekton/pipeline_run.go | 12 +++++++----- tekton/pipeline_run_test.go | 11 ++++++----- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index e480dd31..d2a169e2 100644 --- a/controllers/release/adapter.go +++ b/controllers/release/adapter.go @@ -19,9 +19,10 @@ package release import ( "context" "fmt" - "github.com/redhat-appstudio/operator-toolkit/controller" "strings" + "github.com/redhat-appstudio/operator-toolkit/controller" + "github.com/go-logr/logr" "github.com/redhat-appstudio/release-service/api/v1alpha1" "github.com/redhat-appstudio/release-service/gitops" @@ -306,7 +307,7 @@ func (a *adapter) createReleasePipelineRun(resources *loader.ProcessingResources resources.ReleasePlanAdmission, resources.ReleaseStrategy, resources.Snapshot). WithOwner(a.release). WithReleaseAndApplicationMetadata(a.release, resources.Snapshot.Spec.Application). - WithReleaseStrategy(resources.ReleaseStrategy). + WithReleaseStrategy(resources.ReleaseStrategy, a.release). WithEnterpriseContractConfigMap(resources.EnterpriseContractConfigMap). WithEnterpriseContractPolicy(resources.EnterpriseContractPolicy). AsPipelineRun() diff --git a/tekton/pipeline_run.go b/tekton/pipeline_run.go index d19667ea..674402b8 100644 --- a/tekton/pipeline_run.go +++ b/tekton/pipeline_run.go @@ -145,7 +145,7 @@ func (r *ReleasePipelineRun) WithReleaseAndApplicationMetadata(release *v1alpha1 } // WithReleaseStrategy adds Pipeline reference and parameters to the release PipelineRun. -func (r *ReleasePipelineRun) WithReleaseStrategy(strategy *v1alpha1.ReleaseStrategy) *ReleasePipelineRun { +func (r *ReleasePipelineRun) WithReleaseStrategy(strategy *v1alpha1.ReleaseStrategy, release *v1alpha1.Release) *ReleasePipelineRun { r.Spec.PipelineRef = getPipelineRef(strategy) valueType := tektonv1beta1.ParamTypeString @@ -163,9 +163,9 @@ func (r *ReleasePipelineRun) WithReleaseStrategy(strategy *v1alpha1.ReleaseStrat } if strategy.Spec.PersistentVolumeClaim == "" { - r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), os.Getenv("DEFAULT_RELEASE_PVC")) + r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), os.Getenv("DEFAULT_RELEASE_PVC"), release.Name) } else { - r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), strategy.Spec.PersistentVolumeClaim) + r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), strategy.Spec.PersistentVolumeClaim, release.Name) } r.WithServiceAccount(strategy.Spec.ServiceAccount) @@ -182,9 +182,10 @@ func (r *ReleasePipelineRun) WithServiceAccount(serviceAccount string) *ReleaseP } // WithWorkspace adds a workspace to the PipelineRun using the given name and PersistentVolumeClaim. +// A subdir consisting of the provided Release name and the PipelineRun uid context variable. // If any of those values is empty, no workspace will be added. -func (r *ReleasePipelineRun) WithWorkspace(name, persistentVolumeClaim string) *ReleasePipelineRun { - if name == "" || persistentVolumeClaim == "" { +func (r *ReleasePipelineRun) WithWorkspace(name, persistentVolumeClaim string, releaseName string) *ReleasePipelineRun { + if name == "" || persistentVolumeClaim == "" || releaseName == "" { return r } @@ -193,6 +194,7 @@ func (r *ReleasePipelineRun) WithWorkspace(name, persistentVolumeClaim string) * PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ ClaimName: persistentVolumeClaim, }, + SubPath: releaseName + "-$(context.pipelineRun.uid)", }) return r diff --git a/tekton/pipeline_run_test.go b/tekton/pipeline_run_test.go index ca11eeda..2cf9b33c 100644 --- a/tekton/pipeline_run_test.go +++ b/tekton/pipeline_run_test.go @@ -190,7 +190,7 @@ var _ = Describe("PipelineRun", func() { }) It("can add the ReleaseStrategy information and bundle resolver if present to a PipelineRun object ", func() { - releasePipelineRun.WithReleaseStrategy(strategy) + releasePipelineRun.WithReleaseStrategy(strategy, release) Expect(releasePipelineRun.Spec.PipelineRef.ResolverRef).NotTo(Equal(tektonv1beta1.ResolverRef{})) Expect(releasePipelineRun.Spec.PipelineRef.ResolverRef.Resolver).To(Equal(tektonv1beta1.ResolverName("bundles"))) Expect(releasePipelineRun.Spec.PipelineRef.ResolverRef.Params).To(HaveLen(3)) @@ -208,9 +208,10 @@ var _ = Describe("PipelineRun", func() { }) It("can add a workspace to the PipelineRun using the given name and PVC", func() { - releasePipelineRun.WithWorkspace(workspace, persistentVolumeClaim) + releasePipelineRun.WithWorkspace(workspace, persistentVolumeClaim, release.Name) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("Name", Equal(workspace)))) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("PersistentVolumeClaim.ClaimName", Equal(persistentVolumeClaim)))) + Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("SubPath", Equal(release.Name+"-$(context.pipelineRun.uid)")))) }) It("can add the EC task bundle parameter to the PipelineRun", func() { @@ -248,7 +249,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "") os.Setenv("DEFAULT_RELEASE_PVC", "bar") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy) + releasePipelineRun.WithReleaseStrategy(strategy, release) Expect(releasePipelineRun.Spec.Workspaces).To(BeNil()) }) }) @@ -257,7 +258,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "foo") os.Setenv("DEFAULT_RELEASE_PVC", "") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy) + releasePipelineRun.WithReleaseStrategy(strategy, release) Expect(releasePipelineRun.Spec.Workspaces).To(BeNil()) }) }) @@ -266,7 +267,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "foo") os.Setenv("DEFAULT_RELEASE_PVC", "bar") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy) + releasePipelineRun.WithReleaseStrategy(strategy, release) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("Name", Equal("foo")))) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("PersistentVolumeClaim.ClaimName", Equal("bar")))) })