fix(slo): rollback when errors happen during the update use case (ela…

…stic#168142)
kdelemme · Oct 6, 2023 · 377a3e4 · 377a3e4
1 parent 96e785d
commit 377a3e4
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 17 deletions.
diff --git a/x-pack/plugins/observability/server/services/slo/update_slo.test.ts b/x-pack/plugins/observability/server/services/slo/update_slo.test.ts
@@ -45,7 +45,7 @@ describe('UpdateSLO', () => {
     const newSettings = { ...slo.settings, timestamp_field: 'newField' };
     await updateSLO.execute(slo.id, { settings: newSettings });
 
-    expectDeletionOfObsoleteSLOData(slo);
+    expectDeletionOfOriginalSLO(slo);
     expect(mockRepository.save).toBeCalledWith(
       expect.objectContaining({
         ...slo,
@@ -70,7 +70,7 @@ describe('UpdateSLO', () => {
       },
     });
 
-    expectDeletionOfObsoleteSLOData(slo);
+    expectDeletionOfOriginalSLO(slo);
     expectInstallationOfNewSLOTransform();
   });
 
@@ -86,7 +86,7 @@ describe('UpdateSLO', () => {
       },
     });
 
-    expectDeletionOfObsoleteSLOData(slo);
+    expectDeletionOfOriginalSLO(slo);
     expectInstallationOfNewSLOTransform();
   });
 
@@ -102,7 +102,7 @@ describe('UpdateSLO', () => {
       },
     });
 
-    expectDeletionOfObsoleteSLOData(slo);
+    expectDeletionOfOriginalSLO(slo);
     expectInstallationOfNewSLOTransform();
   });
 
@@ -119,7 +119,7 @@ describe('UpdateSLO', () => {
     expect(mockEsClient.index.mock.calls[0]).toMatchSnapshot();
   });
 
-  it('removes the obsolete data from the SLO previous revision', async () => {
+  it('removes the original data from the original SLO', async () => {
     const slo = createSLO({
       indicator: createAPMTransactionErrorRateIndicator({ environment: 'development' }),
     });
@@ -128,7 +128,6 @@ describe('UpdateSLO', () => {
     const newIndicator = createAPMTransactionErrorRateIndicator({ environment: 'production' });
     await updateSLO.execute(slo.id, { indicator: newIndicator });
 
-    expectDeletionOfObsoleteSLOData(slo);
     expect(mockRepository.save).toBeCalledWith(
       expect.objectContaining({
         ...slo,
@@ -138,6 +137,53 @@ describe('UpdateSLO', () => {
       })
     );
     expectInstallationOfNewSLOTransform();
+    expectDeletionOfOriginalSLO(slo);
+  });
+
+  describe('when error happens during the transform installation step', () => {
+    it('restores the previous SLO definition in the repository', async () => {
+      const slo = createSLO({
+        indicator: createAPMTransactionErrorRateIndicator({ environment: 'development' }),
+      });
+      mockRepository.findById.mockResolvedValueOnce(slo);
+      mockTransformManager.install.mockRejectedValueOnce(new Error('Transform install error'));
+
+      const newIndicator = createAPMTransactionErrorRateIndicator({ environment: 'production' });
+
+      await expect(updateSLO.execute(slo.id, { indicator: newIndicator })).rejects.toThrowError(
+        'Transform install error'
+      );
+
+      expect(mockRepository.save).toHaveBeenCalledWith(slo);
+      expect(mockTransformManager.preview).not.toHaveBeenCalled();
+      expect(mockTransformManager.start).not.toHaveBeenCalled();
+      expect(mockTransformManager.stop).not.toHaveBeenCalled();
+      expect(mockTransformManager.uninstall).not.toHaveBeenCalled();
+      expect(mockEsClient.deleteByQuery).not.toHaveBeenCalled();
+    });
+  });
+
+  describe('when error happens during the transform start step', () => {
+    it('removes the new transform and restores the previous SLO definition in the repository', async () => {
+      const slo = createSLO({
+        indicator: createAPMTransactionErrorRateIndicator({ environment: 'development' }),
+      });
+      mockRepository.findById.mockResolvedValueOnce(slo);
+      mockTransformManager.start.mockRejectedValueOnce(new Error('Transform start error'));
+
+      const newIndicator = createAPMTransactionErrorRateIndicator({ environment: 'production' });
+
+      await expect(updateSLO.execute(slo.id, { indicator: newIndicator })).rejects.toThrowError(
+        'Transform start error'
+      );
+
+      expect(mockTransformManager.uninstall).toHaveBeenCalledWith(
+        getSLOTransformId(slo.id, slo.revision + 1)
+      );
+      expect(mockRepository.save).toHaveBeenCalledWith(slo);
+      expect(mockTransformManager.stop).not.toHaveBeenCalled();
+      expect(mockEsClient.deleteByQuery).not.toHaveBeenCalled();
+    });
   });
 
   function expectInstallationOfNewSLOTransform() {
@@ -146,12 +192,12 @@ describe('UpdateSLO', () => {
     expect(mockTransformManager.start).toBeCalled();
   }
 
-  function expectDeletionOfObsoleteSLOData(originalSlo: SLO) {
+  function expectDeletionOfOriginalSLO(originalSlo: SLO) {
     const transformId = getSLOTransformId(originalSlo.id, originalSlo.revision);
     expect(mockTransformManager.stop).toBeCalledWith(transformId);
     expect(mockTransformManager.uninstall).toBeCalledWith(transformId);
-    expect(mockEsClient.deleteByQuery).toHaveBeenCalledTimes(2);
 
+    expect(mockEsClient.deleteByQuery).toHaveBeenCalledTimes(2);
     expect(mockEsClient.deleteByQuery).toHaveBeenNthCalledWith(
       1,
       expect.objectContaining({

diff --git a/x-pack/plugins/observability/server/services/slo/update_slo.ts b/x-pack/plugins/observability/server/services/slo/update_slo.ts
@@ -36,13 +36,27 @@ export class UpdateSLO {
 
     validateSLO(updatedSlo);
 
-    await this.deleteObsoleteSLORevisionData(originalSlo);
-
     const updatedSloTransformId = getSLOTransformId(updatedSlo.id, updatedSlo.revision);
     await this.repository.save(updatedSlo);
-    await this.transformManager.install(updatedSlo);
-    await this.transformManager.preview(updatedSloTransformId);
-    await this.transformManager.start(updatedSloTransformId);
+
+    try {
+      await this.transformManager.install(updatedSlo);
+    } catch (err) {
+      await this.repository.save(originalSlo);
+      throw err;
+    }
+
+    try {
+      await this.transformManager.preview(updatedSloTransformId);
+      await this.transformManager.start(updatedSloTransformId);
+    } catch (err) {
+      await Promise.all([
+        this.transformManager.uninstall(updatedSloTransformId),
+        this.repository.save(originalSlo),
+      ]);
+
+      throw err;
+    }
 
     await this.esClient.index({
       index: SLO_SUMMARY_TEMP_INDEX_NAME,
@@ -51,13 +65,21 @@ export class UpdateSLO {
       refresh: true,
     });
 
+    await this.deleteOriginalSLO(originalSlo);
+
     return this.toResponse(updatedSlo);
   }
 
-  private async deleteObsoleteSLORevisionData(originalSlo: SLO) {
-    const originalSloTransformId = getSLOTransformId(originalSlo.id, originalSlo.revision);
-    await this.transformManager.stop(originalSloTransformId);
-    await this.transformManager.uninstall(originalSloTransformId);
+  private async deleteOriginalSLO(originalSlo: SLO) {
+    try {
+      const originalSloTransformId = getSLOTransformId(originalSlo.id, originalSlo.revision);
+      await this.transformManager.stop(originalSloTransformId);
+      await this.transformManager.uninstall(originalSloTransformId);
+    } catch (err) {
+      // Any errors here should not prevent moving forward.
+      // Worst case we keep rolling up data for the previous revision number.
+    }
+
     await this.deleteRollupData(originalSlo.id, originalSlo.revision);
     await this.deleteSummaryData(originalSlo.id, originalSlo.revision);
   }