CSPL-3499: Fix Segmentation Fault in Splunk Operator Due to App Volume Name Change (#1468)

* handle error from remoteDataClientMgr

* implement unit test for bug fix

* add comment in documentation

* update comment on test case

* udpate error log to be easier to debug

Author: rlieberman-splunk

docs/AppFramework.md

@@ -577,6 +577,8 @@ Here is a typical App framework configuration in a Custom Resource definition:
 
 `appSources` defines the name and scope of the appSource, the remote storage volume, and its location.
 
+NOTE: If an app source name needs to be changed, make sure the name change is persisted across the app framework spec and CR status. The Splunk Operator should automatically update the app path in the CR and on the pod on the next reconciliation.
+
 * `name` uniquely identifies the App source configuration within a CR. This used locally by the Operator to identify the App source.
 * `scope` defines the scope of the app to be installed.
   * If the scope is `local`, the apps will be installed and run locally on the pod referred to by the CR.

pkg/splunk/enterprise/afwscheduler.go

@@ -557,9 +557,6 @@ downloadWork:
 					continue
 				}
 
-				// increment the count in worker waitgroup
-				downloadWorker.waiter.Add(1)
-
 				// update the download state of app to be DownloadInProgress
 				updatePplnWorkerPhaseInfo(ctx, downloadWorker.appDeployInfo, downloadWorker.appDeployInfo.PhaseInfo.FailCount, enterpriseApi.AppPkgDownloadInProgress)
 
@@ -577,7 +574,18 @@ downloadWork:
 				}
 
 				// get the remoteDataClientMgr instance
-				remoteDataClientMgr, _ := getRemoteDataClientMgr(ctx, downloadWorker.client, downloadWorker.cr, downloadWorker.afwConfig, downloadWorker.appSrcName)
+				remoteDataClientMgr, err := getRemoteDataClientMgr(ctx, downloadWorker.client, downloadWorker.cr, downloadWorker.afwConfig, downloadWorker.appSrcName)
+				if err != nil {
+					scopedLog.Error(err, "unable to get remote data client manager")
+					// increment the retry count and mark this app as download error
+					updatePplnWorkerPhaseInfo(ctx, appDeployInfo, appDeployInfo.PhaseInfo.FailCount+1, enterpriseApi.AppPkgDownloadError)
+
+					<-downloadWorkersRunPool
+					continue
+				}
+
+				// increment the count in worker waitgroup
+				downloadWorker.waiter.Add(1)
 
 				// start the actual download
 				go downloadWorker.download(ctx, pplnPhase, *remoteDataClientMgr, localPath, downloadWorkersRunPool)

pkg/splunk/enterprise/afwscheduler_test.go

@@ -1828,6 +1828,124 @@ func TestScheduleDownloads(t *testing.T) {
 	downloadPhaseWaiter.Wait()
 }
 
+// This test case is to verify that the operator does not crash when the app name is changed.
+func TestScheduleDownloadsFailRemoteDataClientMgr(t *testing.T) {
+	ctx := context.TODO()
+	var ppln *AppInstallPipeline
+	appDeployContext := &enterpriseApi.AppDeploymentContext{}
+
+	client := spltest.NewMockClient()
+	cr := enterpriseApi.Standalone{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "s1",
+			Namespace: "test",
+		},
+		TypeMeta: metav1.TypeMeta{
+			Kind: "Standalone",
+		},
+		Spec: enterpriseApi.StandaloneSpec{
+			Replicas: 1,
+			AppFrameworkConfig: enterpriseApi.AppFrameworkSpec{
+				PhaseMaxRetries:           3,
+				AppsRepoPollInterval:      60,
+				MaxConcurrentAppDownloads: 5,
+				VolList: []enterpriseApi.VolumeSpec{
+					{
+						Name:      "test_volume",
+						Endpoint:  "https://s3-eu-west-2.amazonaws.com",
+						Path:      "testbucket-rs-london",
+						SecretRef: "s3-secret",
+						Provider:  "aws",
+					},
+				},
+				AppSources: []enterpriseApi.AppSourceSpec{
+					{
+						Name:     "appSrc1",
+						Location: "adminAppsRepo",
+						AppSourceDefaultSpec: enterpriseApi.AppSourceDefaultSpec{
+							VolName: "test_volume",
+							Scope:   "local",
+						},
+					},
+				},
+			},
+		},
+	}
+	sts := &appsv1.StatefulSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "splunk-s1-standalone",
+			Namespace: "test",
+		},
+	}
+	sts.Spec.Replicas = new(int32)
+	*sts.Spec.Replicas = 1
+
+	ppln = initAppInstallPipeline(ctx, appDeployContext, client, &cr)
+	pplnPhase := ppln.pplnPhases[enterpriseApi.PhaseDownload]
+
+	testApps := []string{"app2.tgz"}
+	testHashes := []string{"efgh2222"}
+	testSizes := []int64{20}
+
+	appDeployInfoList := make([]*enterpriseApi.AppDeploymentInfo, 1)
+	for index := range testApps {
+		appDeployInfoList[index] = &enterpriseApi.AppDeploymentInfo{
+			AppName: testApps[index],
+			PhaseInfo: enterpriseApi.PhaseInfo{
+				Phase:     enterpriseApi.PhaseDownload,
+				Status:    enterpriseApi.AppPkgDownloadPending,
+				FailCount: 0,
+			},
+			ObjectHash: testHashes[index],
+			Size:       uint64(testSizes[index]),
+		}
+	}
+
+	// create the local directory
+	localPath := filepath.Join(splcommon.AppDownloadVolume, "downloadedApps", "test" /*namespace*/, "Standalone", cr.Name, "local", "appSrc1") + "/"
+	err := createAppDownloadDir(ctx, localPath)
+	if err != nil {
+		t.Errorf("Unable to create the download directory")
+	}
+	defer os.RemoveAll(splcommon.AppDownloadVolume)
+
+	// create the dummy app package for appSrc1 locally, to test the case
+	// where an app is already downloaded and hence we dont re-download it
+	appFileName := testApps[0] + "_" + testHashes[0]
+	appLoc := filepath.Join(localPath, appFileName)
+	err = createOrTruncateAppFileLocally(appLoc, testSizes[0])
+	if err != nil {
+		t.Errorf("Unable to create the app files locally")
+	}
+	defer os.Remove(appLoc)
+
+	// create pipeline workers
+	ppln.createAndAddPipelineWorker(ctx, enterpriseApi.PhaseDownload, appDeployInfoList[0], "appSrc2", "", &cr.Spec.AppFrameworkConfig, client, &cr, sts)
+
+	maxWorkers := 1
+	downloadPhaseWaiter := new(sync.WaitGroup)
+
+	downloadPhaseWaiter.Add(1)
+	// schedule the download threads to do actual download work
+	go pplnPhase.downloadWorkerHandler(ctx, ppln, uint64(maxWorkers), downloadPhaseWaiter)
+
+	// add the workers to msgChannel so that scheduleDownlads thread can pick them up
+	for _, downloadWorker := range pplnPhase.q {
+		downloadWorker.waiter = &pplnPhase.workerWaiter
+		pplnPhase.msgChannel <- downloadWorker
+		downloadWorker.isActive = true
+	}
+
+	close(pplnPhase.msgChannel)
+
+	downloadPhaseWaiter.Add(1)
+	close(ppln.sigTerm)
+	// schedule the download threads to do actual download work
+	go pplnPhase.downloadWorkerHandler(ctx, ppln, uint64(maxWorkers), downloadPhaseWaiter)
+
+	downloadPhaseWaiter.Wait()
+}
+
 func TestCreateDownloadDirOnOperator(t *testing.T) {
 	ctx := context.TODO()
 	standalone := enterpriseApi.Standalone{

pkg/splunk/enterprise/util.go

@@ -535,7 +535,7 @@ func getRemoteObjectKey(ctx context.Context, cr splcommon.MetaObject, appFramewo
 
 	appSrc, err := getAppSrcSpec(appFrameworkConfig.AppSources, appSrcName)
 	if err != nil {
-		scopedLog.Error(err, "unable to get appSrcSpc")
+		scopedLog.Error(err, "unable to get appSourceSpec")
 		return remoteObjectKey, err
 	}