fix(resource-service): fix nats subscription and added retry logic (#…

…7215) * introduces retry logic from POC Signed-off-by: warber <bernd.warmuth@dynatrace.com> * created test Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added entry to int test matrix, implemented correct nats message handling Signed-off-by: warber <bernd.warmuth@dynatrace.com> * fixed compile error and disabled other integration tests for now Signed-off-by: warber <bernd.warmuth@dynatrace.com> * removed comments in integration_tests.yml Signed-off-by: warber <bernd.warmuth@dynatrace.com> * introduce KEPTN_NAMESPACE_SUFFIX Signed-off-by: warber <bernd.warmuth@dynatrace.com> * trigger build Signed-off-by: warber <bernd.warmuth@dynatrace.com> * changed cloud provider to just GKE Signed-off-by: warber <bernd.warmuth@dynatrace.com> * revert to master integration_tests.yml with multiple replicas of resource service Signed-off-by: warber <bernd.warmuth@dynatrace.com> * cleanup Signed-off-by: warber <bernd.warmuth@dynatrace.com> * enable DIRECTORY_STAGE_STRUCTURE in integration tests Signed-off-by: warber <bernd.warmuth@dynatrace.com> * trigger build Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added retry logic in service manager Signed-off-by: warber <bernd.warmuth@dynatrace.com> * try to fix retry logic Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added hard reset before each pull Signed-off-by: warber <bernd.warmuth@dynatrace.com> * moved git reset out of retry Signed-off-by: warber <bernd.warmuth@dynatrace.com> * introduces retry logic from POC Signed-off-by: warber <bernd.warmuth@dynatrace.com> * created test Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added entry to int test matrix, implemented correct nats message handling Signed-off-by: warber <bernd.warmuth@dynatrace.com> * fixed compile error and disabled other integration tests for now Signed-off-by: warber <bernd.warmuth@dynatrace.com> * removed comments in integration_tests.yml Signed-off-by: warber <bernd.warmuth@dynatrace.com> * introduce KEPTN_NAMESPACE_SUFFIX Signed-off-by: warber <bernd.warmuth@dynatrace.com> * changed cloud provider to just GKE Signed-off-by: warber <bernd.warmuth@dynatrace.com> * revert to master integration_tests.yml with multiple replicas of resource service Signed-off-by: warber <bernd.warmuth@dynatrace.com> * cleanup Signed-off-by: warber <bernd.warmuth@dynatrace.com> * enable DIRECTORY_STAGE_STRUCTURE in integration tests Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added retry logic in service manager Signed-off-by: warber <bernd.warmuth@dynatrace.com> * try to fix retry logic Signed-off-by: warber <bernd.warmuth@dynatrace.com> * added hard reset before each pull Signed-off-by: warber <bernd.warmuth@dynatrace.com> * moved git reset out of retry Signed-off-by: warber <bernd.warmuth@dynatrace.com> * reverted moving git reset out of retry Signed-off-by: warber <bernd.warmuth@dynatrace.com> * reverted unintentional commit in distributor module Signed-off-by: warber <bernd.warmuth@dynatrace.com> * magic Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com> * set number of replicas back to 1 Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com> * turn off directory structure Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com> * removed git hard resets Signed-off-by: warber <bernd.warmuth@dynatrace.com> * fix minor issues in integration tests Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com> * fixed backup restore test Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com> Co-authored-by: odubajDT <ondrej.dubaj@dynatrace.com>
keptn · Mar 24, 2022 · 180d833 · 180d833
1 parent a6c0877
commit 180d833
Show file tree

Hide file tree

Showing 22 changed files with 465 additions and 787 deletions.
diff --git a/.github/workflows/integration_tests.yml b/.github/workflows/integration_tests.yml
@@ -362,7 +362,7 @@ jobs:
             else
               DOCKER_ORG="keptn"
             fi
-            
+          
             installer/airgapped/install_keptn.sh \
               "$AIRGAPPED_REGISTRY_URL/" \
               "$DOCKER_ORG" \
@@ -375,6 +375,7 @@ jobs:
               --create-namespace \
               --set control-plane.apiGatewayNginx.type=${KEPTN_SERVICE_TYPE} \
               --set control-plane.resourceService.enabled=true \
+              --set control-plane.resourceService.replicas=1 \
               --set continuous-delivery.enabled=${RUN_CONTINUOUS_DELIVERY_TEST}
           else
             # Install Keptn via CLI
@@ -466,9 +467,9 @@ jobs:
             KEPTN_API_TOKEN=$(kubectl get secret keptn-api-token -n "$KEPTN_NAMESPACE" -o jsonpath='{.data.keptn-api-token}' | base64 --decode)
             kubectl scale deployment/helm-service -n "${KEPTN_NAMESPACE}" --replicas=0
             kubectl scale deployment/jmeter-service -n "${KEPTN_NAMESPACE}" --replicas=0
-  
+          
             KEPTN_API_HOSTNAME=$(echo "${KEPTN_ENDPOINT}" | awk -F[/] '{print $3}')
-
+          
             helm install helm-service "${HELM_SERVICE_HELM_CHART_NAME}" \
               -n ${{ env.KEPTN_NAMESPACE }}-helm-service \
               --set remoteControlPlane.enabled=true \
@@ -483,7 +484,7 @@ jobs:
               --set remoteControlPlane.api.hostname="${KEPTN_API_HOSTNAME}" \
               --set remoteControlPlane.api.token="${KEPTN_API_TOKEN}" \
               --create-namespace
-
+          
             helm test jmeter-service -n ${{ env.KEPTN_NAMESPACE }}-jmeter-service
             helm test helm-service -n ${{ env.KEPTN_NAMESPACE }}-helm-service
           else
@@ -494,7 +495,7 @@ jobs:
             helm test jmeter-service -n ${{ env.KEPTN_NAMESPACE }}
             helm test helm-service -n ${{ env.KEPTN_NAMESPACE }}
           fi
-            
+
 
       - name: Prepare test run
         id: prepare_test_run
@@ -578,6 +579,7 @@ jobs:
         if: always() && env.CLOUD_PROVIDER == 'GKE'
         run: |
           echo "Cleaning up test resources..."
+
           readarray -t namespaces <<< "$(kubectl get namespaces | awk '{ print $1 }' | grep ${{ env.KEPTN_NAMESPACE }})"
           readarray -t clusterrolebindings <<< "$(kubectl get clusterrolebindings | awk '{ print $1 }' | grep ${{ env.KEPTN_NAMESPACE }})"
 
@@ -624,7 +626,7 @@ jobs:
                 fi
               done
           fi
-          
+
       - name: Cleanup Minishift cluster
         if: env.CLOUD_PROVIDER == 'minishift-on-GHA'
         timeout-minutes: 3
@@ -728,7 +730,7 @@ jobs:
               console.log(`Reading file: ${fileName}`);
               const platformReportFile = fs.readFileSync(TEST_REPORTS_PATH + fileName, {encoding:'utf8', flag:'r'});
               const platformTestReportRaw = ndJsonParser(platformReportFile);
-              
+            
               // Only pick the test results that have a format like this: Test_GKE/Test_name
               // There are some other results in there for the whole package for example, that we don't want
               platformTestReportRaw.forEach(platformTestResult => {
@@ -742,12 +744,12 @@ jobs:
             
             jsonReportData.forEach(testResult => {
               const trimmedTestResult = { ...testResult, test: testResult.test.split("/")[1] }
-              
+            
               // If this is a new testcase the result list needs to be created first
               if (!(trimmedTestResult.test in testResultMap)) {
                 testResultMap[trimmedTestResult.test] = [];
               }
-              
+            
               // Add to result list
               testResultMap[trimmedTestResult.test].push(trimmedTestResult)
             });
@@ -762,7 +764,7 @@ jobs:
               testResults.forEach(result => {
                 const {test, ...results} = result;
                 Object.assign(finalResult, results);
-                
+            
                 const currentPlatform = Object.keys(results)[0];
                 if(!allPlatforms.includes(currentPlatform)) {
                   allPlatforms.push(currentPlatform);

diff --git a/resource-service/common/fake/git_mock.go b/resource-service/common/fake/git_mock.go
diff --git a/resource-service/common/git.go b/resource-service/common/git.go
@@ -41,6 +41,7 @@ type IGit interface {
 	GetCurrentRevision(gitContext common_models.GitContext) (string, error)
 	GetDefaultBranch(gitContext common_models.GitContext) (string, error)
 	MigrateProject(gitContext common_models.GitContext, newMetadatacontent []byte) error
+	ResetHard(gitContext common_models.GitContext, revision string) error
 }
 
 type Git struct {
@@ -248,7 +249,7 @@ func (g Git) StageAndCommitAll(gitContext common_models.GitContext, message stri
 		return "", fmt.Errorf(kerrors.ErrMsgCouldNotCommit, gitContext.Project, err)
 	}
 	rollbackFunc := func() {
-		err := g.resetHard(gitContext)
+		err := g.ResetHard(gitContext, "HEAD~1")
 		if err != nil {
 			logger.WithError(err).Warn("could not reset")
 		}
@@ -682,12 +683,12 @@ func (g *Git) getWorkTree(gitContext common_models.GitContext) (*git.Repository,
 	return repo, worktree, nil
 }
 
-func (g Git) resetHard(gitContext common_models.GitContext) error {
+func (g Git) ResetHard(gitContext common_models.GitContext, rev string) error {
 	r, w, err := g.getWorkTree(gitContext)
 	if err != nil {
 		return fmt.Errorf(kerrors.ErrMsgCouldNotGitAction, "reset", gitContext.Project, err)
 	}
-	revision, err := r.ResolveRevision("HEAD~1")
+	revision, err := r.ResolveRevision(plumbing.Revision(rev))
 	if err != nil {
 		return fmt.Errorf(kerrors.ErrMsgCouldNotGitAction, "reset", gitContext.Project, err)
 	}

diff --git a/resource-service/go.mod b/resource-service/go.mod
@@ -3,7 +3,6 @@ module github.com/keptn/keptn/resource-service
 go 1.17
 
 require (
-	github.com/cloudevents/sdk-go/v2 v2.5.0
 	github.com/gin-gonic/gin v1.7.7
 	github.com/go-git/go-billy/v5 v5.3.1
 	github.com/go-git/go-git-fixtures/v4 v4.3.1
@@ -31,6 +30,7 @@ require (
 	github.com/andybalholm/brotli v1.0.1 // indirect
 	github.com/benbjohnson/clock v1.3.0 // indirect
 	github.com/cloudevents/sdk-go/observability/opentelemetry/v2 v2.0.0-20211001212819-74757a691209 // indirect
+	github.com/cloudevents/sdk-go/v2 v2.5.0 // indirect
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/dsnet/compress v0.0.2-0.20210315054119-f66993602bf5 // indirect
 	github.com/emirpasic/gods v1.12.0 // indirect

diff --git a/resource-service/handler/nats/eventhandler.go b/resource-service/handler/nats/eventhandler.go
@@ -18,7 +18,7 @@ func EventHandler(projectManager handler.IProjectManager) *EventMsgHandler {
 		pm: projectManager,
 	}
 }
-func (eh *EventMsgHandler) Process(event models.Event, sync bool) error {
+func (eh *EventMsgHandler) Process(event models.Event) error {
 	e := &keptnv2.ProjectDeleteFinishedEventData{}
 	if err := keptnv2.Decode(event.Data, e); err != nil {
 		return err

diff --git a/resource-service/handler/nats/eventhandler_test.go b/resource-service/handler/nats/eventhandler_test.go
@@ -19,7 +19,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 				Status: keptnv2.StatusUnknown,
 			},
 		}
-		err := eh.Process(event, false)
+		err := eh.Process(event)
 		require.Nil(t, err)
 		require.Equal(t, 0, len(pm.DeleteProjectCalls()))
 
@@ -28,7 +28,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 				Status: keptnv2.StatusErrored,
 			},
 		}
-		err = eh.Process(event, false)
+		err = eh.Process(event)
 		require.Nil(t, err)
 		require.Equal(t, 0, len(pm.DeleteProjectCalls()))
 
@@ -37,7 +37,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 				Status: keptnv2.StatusAborted,
 			},
 		}
-		err = eh.Process(event, false)
+		err = eh.Process(event)
 		require.Nil(t, err)
 		require.Equal(t, 0, len(pm.DeleteProjectCalls()))
 	})
@@ -51,7 +51,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 				Status: keptnv2.StatusSucceeded,
 			},
 		}
-		err := eh.Process(event, false)
+		err := eh.Process(event)
 		require.Nil(t, err)
 		require.Equal(t, 0, len(pm.DeleteProjectCalls()))
 	})
@@ -69,7 +69,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 				Project: "a-project",
 			},
 		}
-		err := eh.Process(event, false)
+		err := eh.Process(event)
 
 		require.Equal(t, 1, len(pm.DeleteProjectCalls()))
 		require.Equal(t, "a-project", pm.DeleteProjectCalls()[0].ProjectName)
@@ -82,7 +82,7 @@ func TestEventMsgHandler_Process(t *testing.T) {
 		}
 		eh := EventHandler(pm)
 		event := models.Event{Data: "something-strange!!!11!"}
-		err := eh.Process(event, false)
+		err := eh.Process(event)
 
 		require.Equal(t, 0, len(pm.DeleteProjectCalls()))
 		require.NotNil(t, err)

diff --git a/resource-service/handler/project_manager_test.go b/resource-service/handler/project_manager_test.go
@@ -931,30 +931,15 @@ func TestProjectManager_DeleteProject_CannotDeleteDirectory(t *testing.T) {
 func getTestProjectManagerFields() projectManagerTestFields {
 	return projectManagerTestFields{
 		git: &common_mock.IGitMock{
-			ProjectExistsFunc: func(gitContext common_models.GitContext) bool {
-				return true
-			},
-			ProjectRepoExistsFunc: func(projectName string) bool {
-				return true
-			},
-			CloneRepoFunc: func(gitContext common_models.GitContext) (bool, error) {
-				return true, nil
-			},
-			StageAndCommitAllFunc: func(gitContext common_models.GitContext, message string) (string, error) {
-				return "", nil
-			},
-			GetDefaultBranchFunc: func(gitContext common_models.GitContext) (string, error) {
-				return "main", nil
-			},
-			CheckoutBranchFunc: func(gitContext common_models.GitContext, branch string) error {
-				return nil
-			},
-			MigrateProjectFunc: func(gitContext common_models.GitContext, newMetadatacontent []byte) error {
-				return nil
-			},
-			PullFunc: func(gitContext common_models.GitContext) error {
-				return nil
-			},
+			ResetHardFunc:         func(gitContext common_models.GitContext) error { return nil },
+			ProjectExistsFunc:     func(gitContext common_models.GitContext) bool { return true },
+			ProjectRepoExistsFunc: func(projectName string) bool { return true },
+			CloneRepoFunc:         func(gitContext common_models.GitContext) (bool, error) { return true, nil },
+			StageAndCommitAllFunc: func(gitContext common_models.GitContext, message string) (string, error) { return "", nil },
+			GetDefaultBranchFunc:  func(gitContext common_models.GitContext) (string, error) { return "main", nil },
+			CheckoutBranchFunc:    func(gitContext common_models.GitContext, branch string) error { return nil },
+			MigrateProjectFunc:    func(gitContext common_models.GitContext, newMetadatacontent []byte) error { return nil },
+			PullFunc:              func(gitContext common_models.GitContext) error { return nil },
 		},
 		credentialReader: &common_mock.CredentialReaderMock{
 			GetCredentialsFunc: func(project string) (*common_models.GitCredentials, error) {

diff --git a/resource-service/handler/resource_manager.go b/resource-service/handler/resource_manager.go
@@ -126,7 +126,27 @@ func (p ResourceManager) DeleteResource(params models.DeleteResourceParams) (*mo
 
 	resourcePath := configPath + "/" + params.ResourceURI
 
-	return p.deleteResource(gitContext, resourcePath)
+	var resultErr error
+	var resultCommit *models.WriteResourceResponse
+	_ = retry.Retry(func() error {
+		err = p.git.Pull(*gitContext)
+		if err != nil {
+			resultErr = err
+			return nil
+		}
+		response, err := p.deleteResource(gitContext, resourcePath)
+		if err != nil {
+			if errors.Is(err, kerrors.ErrNonFastForwardUpdate) || errors.Is(err, kerrors.ErrForceNeeded) {
+				return err
+			}
+			resultErr = err
+			return nil
+		}
+		resultCommit = response
+		resultErr = err
+		return nil
+	}, retry.NumberOfRetries(5), retry.DelayBetweenRetries(1*time.Second))
+	return resultCommit, resultErr
 }
 
 func (p ResourceManager) establishContext(project models.Project, stage *models.Stage, service *models.Service) (*common_models.GitContext, string, error) {
@@ -206,12 +226,10 @@ func (p ResourceManager) writeAndCommitResource(gitContext *common_models.GitCon
 		err := p.git.Pull(*gitContext)
 		if err != nil {
 			resultErr = err
-			// return nil at this point because retry does not make sense in that case
 			return nil
 		}
 		if err := p.storeResource(resourcePath, resourceContent); err != nil {
 			resultErr = err
-			// return nil at this point because retry does not make sense in that case
 			return nil
 		}
 
@@ -221,7 +239,6 @@ func (p ResourceManager) writeAndCommitResource(gitContext *common_models.GitCon
 				return err
 			}
 			resultErr = err
-			// return nil at this point because retry does not make sense in that case
 			return nil
 		}
 		resultCommit = commit
@@ -238,14 +255,12 @@ func (p ResourceManager) writeAndCommitResources(gitContext *common_models.GitCo
 		err := p.git.Pull(*gitContext)
 		if err != nil {
 			resultErr = err
-			// return nil at this point because retry does not make sense in that case
 			return nil
 		}
 		for _, res := range resources {
 			filePath := directory + "/" + res.ResourceURI
 			if err := p.storeResource(filePath, string(res.ResourceContent)); err != nil {
 				resultErr = err
-				// return nil at this point because retry does not make sense in that case
 				return nil
 			}
 		}
@@ -256,7 +271,6 @@ func (p ResourceManager) writeAndCommitResources(gitContext *common_models.GitCo
 				return err
 			}
 			resultErr = err
-			// return nil at this point because retry does not make sense in that case
 			return nil
 		}
 		resultCommit = commit