Failed retentionScript creation should properly clean up underlying c…

…ronscripts

Summary:
We had an issue where if a retentionscript failed to create, the underlying cronscript wouldn't be properly cleaned up. This leads to "zombie" running cronscripts that can not be deleted by the user.
Although most of the plugin service is written to use transactions, the cronscript lives in a separate service. This means that we have to do some manual cleanup to ensure consistency.

Test Plan: New unit test

Reviewers: philkuz, vihang, nserrino

Reviewed By: philkuz

Signed-off-by: Michelle Nguyen <michellenguyen@pixielabs.ai>

Differential Revision: https://phab.corp.pixielabs.ai/D12533

GitOrigin-RevId: 6d18a389ef019a61cccd8f5b247980e719947c5f

src/cloud/plugin/controllers/server.go

@@ -845,10 +845,21 @@ func (s *Server) createRetentionScript(ctx context.Context, txn *sqlx.Tx, orgID
 
 	query := `INSERT INTO plugin_retention_scripts (org_id, plugin_id, script_id, script_name, description, export_url, is_preset) VALUES ($1, $2, $3, $4, $5, PGP_SYM_ENCRYPT($6, $7), $8)`
 	_, err = txn.Exec(query, orgID, pluginID, utils.UUIDFromProtoOrNil(scriptID), rs.ScriptName, rs.Description, rs.ExportURL, s.dbKey, rs.IsPreset)
-	if err != nil {
-		return nil, status.Errorf(codes.Internal, "Failed to create retention script")
+	if err == nil {
+		return scriptID, nil
 	}
-	return scriptID, nil
+
+	// Create failed, make sure we clean up the cron script.
+	_, delErr := s.cronScriptClient.DeleteScript(ctx, &cronscriptpb.DeleteScriptRequest{
+		ID:    scriptID,
+		OrgID: utils.ProtoFromUUID(orgID),
+	})
+	if delErr != nil {
+		log.WithError(delErr).Error("Failed to delete underlying cron script")
+		return nil, err
+	}
+
+	return nil, err
 }
 
 // CreateRetentionScript creates a script that is used for long-term data retention.

src/cloud/plugin/controllers/server_test.go

@@ -1363,6 +1363,69 @@ func TestServer_CreateRetentionScript(t *testing.T) {
 	assert.Equal(t, "", script.ExportURL)
 }
 
+func TestServer_CreateRetentionScriptNameConflict(t *testing.T) {
+	mustLoadTestData(db)
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mockCSClient := mock_cronscriptpb.NewMockCronScriptServiceClient(ctrl)
+
+	orgConfig := map[string]string{
+		"license_key2": "12345",
+	}
+
+	config := &scripts.Config{
+		OtelEndpointConfig: &scripts.OtelEndpointConfig{
+			URL:      "https://localhost1:8080",
+			Headers:  orgConfig,
+			Insecure: true,
+		},
+	}
+
+	mConfig, err := yaml.Marshal(&config)
+	require.Nil(t, err)
+
+	mockCSClient.EXPECT().CreateScript(gomock.Any(), &cronscriptpb.CreateScriptRequest{
+		Script: "px.display()",
+		ClusterIDs: []*uuidpb.UUID{
+			utils.ProtoFromUUIDStrOrNil("323e4567-e89b-12d3-a456-426655440000"),
+		},
+		Configs:    string(mConfig),
+		FrequencyS: 20,
+		OrgID:      utils.ProtoFromUUIDStrOrNil("223e4567-e89b-12d3-a456-426655440000"),
+		Disabled:   true,
+	}).Return(&cronscriptpb.CreateScriptResponse{
+		ID: utils.ProtoFromUUIDStrOrNil("323e4567-e89b-12d3-a456-426655440000"),
+	}, nil)
+
+	mockCSClient.EXPECT().DeleteScript(gomock.Any(), &cronscriptpb.DeleteScriptRequest{
+		ID:    utils.ProtoFromUUIDStrOrNil("323e4567-e89b-12d3-a456-426655440000"),
+		OrgID: utils.ProtoFromUUIDStrOrNil("223e4567-e89b-12d3-a456-426655440000"),
+	}).Return(&cronscriptpb.DeleteScriptResponse{}, nil)
+
+	s := controllers.New(db, "test", mockCSClient)
+	_, err = s.CreateRetentionScript(createTestContext(), &pluginpb.CreateRetentionScriptRequest{
+		Script: &pluginpb.DetailedRetentionScript{
+			Script: &pluginpb.RetentionScript{
+				ScriptName:  "testScript",
+				Description: "This is a new script!",
+				FrequencyS:  20,
+				ClusterIDs: []*uuidpb.UUID{
+					utils.ProtoFromUUIDStrOrNil("323e4567-e89b-12d3-a456-426655440000"),
+				},
+				PluginId: "test-plugin",
+				Enabled:  false,
+				IsPreset: false,
+			},
+			Contents:  "px.display()",
+			ExportURL: "",
+		},
+		OrgID: utils.ProtoFromUUIDStrOrNil("223e4567-e89b-12d3-a456-426655440000"),
+	})
+
+	require.Error(t, err)
+}
+
 func TestServer_UpdateRetentionScript(t *testing.T) {
 	mustLoadTestData(db)