Skip to content

Commit

Permalink
fix(slo): rollback when errors happen during the update use case (#16…
Browse files Browse the repository at this point in the history
  • Loading branch information
kdelemme authored Oct 6, 2023
1 parent 96e785d commit 377a3e4
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -70,7 +70,7 @@ describe('UpdateSLO', () => {
},
});

expectDeletionOfObsoleteSLOData(slo);
expectDeletionOfOriginalSLO(slo);
expectInstallationOfNewSLOTransform();
});

Expand All @@ -86,7 +86,7 @@ describe('UpdateSLO', () => {
},
});

expectDeletionOfObsoleteSLOData(slo);
expectDeletionOfOriginalSLO(slo);
expectInstallationOfNewSLOTransform();
});

Expand All @@ -102,7 +102,7 @@ describe('UpdateSLO', () => {
},
});

expectDeletionOfObsoleteSLOData(slo);
expectDeletionOfOriginalSLO(slo);
expectInstallationOfNewSLOTransform();
});

Expand All @@ -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' }),
});
Expand All @@ -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,
Expand All @@ -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() {
Expand All @@ -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({
Expand Down
40 changes: 31 additions & 9 deletions x-pack/plugins/observability/server/services/slo/update_slo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
}
Expand Down

0 comments on commit 377a3e4

Please sign in to comment.