From c0e9477bdaca6378776ce5a99a266375ad237991 Mon Sep 17 00:00:00 2001 From: Martin Malina Date: Wed, 16 Aug 2023 11:28:35 +0200 Subject: [PATCH] fix(RHTAPBUGS-691): revert subPath creation in PRs Recently we made a change to start using subPath when defining a workspace during PipelineRun creation. This was to fix an issue when 2 or more PRs running in parallel would override each other's data. It turns out that there is a race condition happening often when kubernetes tries to create the subPath in the volume. When there are several pods with the same subPath created at the same time (as is the case with Tekton PRs - there are pods for each task), it can happen that several pods try to create the subPath and only one of them succeeds. See jira for more details. We'll have to find another solution to the problem we were fixing with this. In the meantime, we'll have to revert this. This reverts commit 25cfcdb12688b0b679cd4beb923662e580ef2da3. Signed-off-by: Martin Malina --- controllers/release/adapter.go | 5 ++--- tekton/pipeline_run.go | 12 +++++------- tekton/pipeline_run_test.go | 11 +++++------ 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/controllers/release/adapter.go b/controllers/release/adapter.go index d2a169e2..e480dd31 100644 --- a/controllers/release/adapter.go +++ b/controllers/release/adapter.go @@ -19,9 +19,8 @@ package release import ( "context" "fmt" - "strings" - "github.com/redhat-appstudio/operator-toolkit/controller" + "strings" "github.com/go-logr/logr" "github.com/redhat-appstudio/release-service/api/v1alpha1" @@ -307,7 +306,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, a.release). + WithReleaseStrategy(resources.ReleaseStrategy). WithEnterpriseContractConfigMap(resources.EnterpriseContractConfigMap). WithEnterpriseContractPolicy(resources.EnterpriseContractPolicy). AsPipelineRun() diff --git a/tekton/pipeline_run.go b/tekton/pipeline_run.go index 674402b8..d19667ea 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, release *v1alpha1.Release) *ReleasePipelineRun { +func (r *ReleasePipelineRun) WithReleaseStrategy(strategy *v1alpha1.ReleaseStrategy) *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"), release.Name) + r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), os.Getenv("DEFAULT_RELEASE_PVC")) } else { - r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), strategy.Spec.PersistentVolumeClaim, release.Name) + r.WithWorkspace(os.Getenv("DEFAULT_RELEASE_WORKSPACE_NAME"), strategy.Spec.PersistentVolumeClaim) } r.WithServiceAccount(strategy.Spec.ServiceAccount) @@ -182,10 +182,9 @@ 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, releaseName string) *ReleasePipelineRun { - if name == "" || persistentVolumeClaim == "" || releaseName == "" { +func (r *ReleasePipelineRun) WithWorkspace(name, persistentVolumeClaim string) *ReleasePipelineRun { + if name == "" || persistentVolumeClaim == "" { return r } @@ -194,7 +193,6 @@ func (r *ReleasePipelineRun) WithWorkspace(name, persistentVolumeClaim string, r 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 2cf9b33c..ca11eeda 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, release) + releasePipelineRun.WithReleaseStrategy(strategy) 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,10 +208,9 @@ var _ = Describe("PipelineRun", func() { }) It("can add a workspace to the PipelineRun using the given name and PVC", func() { - releasePipelineRun.WithWorkspace(workspace, persistentVolumeClaim, release.Name) + releasePipelineRun.WithWorkspace(workspace, persistentVolumeClaim) 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() { @@ -249,7 +248,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "") os.Setenv("DEFAULT_RELEASE_PVC", "bar") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy, release) + releasePipelineRun.WithReleaseStrategy(strategy) Expect(releasePipelineRun.Spec.Workspaces).To(BeNil()) }) }) @@ -258,7 +257,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "foo") os.Setenv("DEFAULT_RELEASE_PVC", "") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy, release) + releasePipelineRun.WithReleaseStrategy(strategy) Expect(releasePipelineRun.Spec.Workspaces).To(BeNil()) }) }) @@ -267,7 +266,7 @@ var _ = Describe("PipelineRun", func() { os.Setenv("DEFAULT_RELEASE_WORKSPACE_NAME", "foo") os.Setenv("DEFAULT_RELEASE_PVC", "bar") strategy.Spec.PersistentVolumeClaim = "" - releasePipelineRun.WithReleaseStrategy(strategy, release) + releasePipelineRun.WithReleaseStrategy(strategy) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("Name", Equal("foo")))) Expect(releasePipelineRun.Spec.Workspaces).Should(ContainElement(HaveField("PersistentVolumeClaim.ClaimName", Equal("bar")))) })