Skip to content

Commit a1e08db

Browse files
authored
[Onboarding ingest] [Fix] Prevent reinstall sample data if it already exists (#230311)
## Summary Previously, we always reinstalled the sample data, assuming it could help update versions if needed. However, this reinstall logic wasn’t actually used or triggered by the frontend. This approach led to a problem: during an incident, multiple install requests were triggered simultaneously, which caused race conditions and server crashes. Now if the sample data is already installed, we simply return the existing index without doing anything. This prevents unnecessary reinstallation and avoids concurrent modification issues. ### Checklist - [ ] ~~Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)~~ - [ ] ~~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~~ - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~~ - [ ] ~~This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.~~ - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels.
1 parent d55ae51 commit a1e08db

File tree

4 files changed

+64
-25
lines changed

4 files changed

+64
-25
lines changed

x-pack/platform/plugins/shared/sample_data_ingest/server/services/index_manager/index_manager.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,20 @@ describe('IndexManager', () => {
296296
});
297297
});
298298

299+
describe('hasIndex', () => {
300+
const indexName = 'test-index';
301+
302+
it('should return true when index exists', async () => {
303+
esClient.indices.exists.mockResolvedValue(true);
304+
305+
const result = await indexManager.hasIndex({ indexName, esClient });
306+
307+
expect(result).toBe(true);
308+
expect(esClient.indices.exists).toHaveBeenCalledWith({ index: indexName });
309+
expect(logger.warn).not.toHaveBeenCalled();
310+
});
311+
});
312+
299313
it('should initialize with correct properties', () => {
300314
const customLogger = loggerMock.create();
301315
const customInferenceId = 'custom-inference';

x-pack/platform/plugins/shared/sample_data_ingest/server/services/index_manager/index_manager.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,4 +93,19 @@ export class IndexManager {
9393
this.log.warn(`Failed to delete index [${indexName}]: ${error.message}`);
9494
}
9595
}
96+
97+
async hasIndex({
98+
indexName,
99+
esClient,
100+
}: {
101+
indexName: string;
102+
esClient: ElasticsearchClient;
103+
}): Promise<boolean> {
104+
try {
105+
return await esClient.indices.exists({ index: indexName });
106+
} catch (error) {
107+
this.log.warn(`Failed to check if index exists [${indexName}]: ${error.message}`);
108+
return false;
109+
}
110+
}
96111
}

x-pack/platform/plugins/shared/sample_data_ingest/server/services/sample_data_manager/sample_data_manager.test.ts

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ describe('SampleDataManager', () => {
7373
mockIndexManager = {
7474
createAndPopulateIndex: jest.fn(),
7575
deleteIndex: jest.fn(),
76+
hasIndex: jest.fn(),
7677
setESClient: jest.fn(),
7778
} as any;
7879

@@ -94,6 +95,7 @@ describe('SampleDataManager', () => {
9495

9596
mockIndexManager.createAndPopulateIndex.mockResolvedValue(undefined);
9697
mockIndexManager.deleteIndex.mockResolvedValue(undefined);
98+
mockIndexManager.hasIndex.mockResolvedValue(false);
9799
});
98100

99101
afterEach(() => {
@@ -141,7 +143,7 @@ describe('SampleDataManager', () => {
141143
it('should install sample data successfully', async () => {
142144
const result = await sampleDataManager.installSampleData({ sampleType, esClient });
143145

144-
expect(mockIndexManager.deleteIndex).toHaveBeenCalledWith({
146+
expect(mockIndexManager.hasIndex).toHaveBeenCalledWith({
145147
indexName: expectedIndexName,
146148
esClient,
147149
});
@@ -162,12 +164,6 @@ describe('SampleDataManager', () => {
162164
);
163165
});
164166

165-
it('should remove existing data before installation', async () => {
166-
await sampleDataManager.installSampleData({ sampleType, esClient });
167-
168-
expect(mockIndexManager.deleteIndex).toHaveBeenCalled();
169-
});
170-
171167
it('should call cleanup when artifact preparation fails', async () => {
172168
const error = new Error('Artifact preparation failed');
173169
mockArtifactManager.prepareArtifact.mockRejectedValue(error);
@@ -209,7 +205,7 @@ describe('SampleDataManager', () => {
209205
indexName: expectedIndexName,
210206
esClient,
211207
});
212-
expect(mockIndexManager.deleteIndex).toHaveBeenCalledTimes(2); // Once in removeSampleData, once in error handler
208+
expect(mockIndexManager.deleteIndex).toHaveBeenCalled();
213209
});
214210

215211
it('should handle different sample types correctly', async () => {
@@ -227,6 +223,22 @@ describe('SampleDataManager', () => {
227223
esClient,
228224
});
229225
});
226+
227+
it('should not install anything if index already exists', async () => {
228+
mockIndexManager.hasIndex.mockResolvedValue(true);
229+
230+
const result = await sampleDataManager.installSampleData({ sampleType, esClient });
231+
232+
expect(mockIndexManager.hasIndex).toHaveBeenCalledWith({
233+
indexName: expectedIndexName,
234+
esClient,
235+
});
236+
expect(result).toBe(expectedIndexName);
237+
expect(mockArtifactManager.prepareArtifact).not.toHaveBeenCalled();
238+
expect(mockIndexManager.createAndPopulateIndex).not.toHaveBeenCalled();
239+
expect(mockIndexManager.deleteIndex).not.toHaveBeenCalled();
240+
expect(mockArtifactManager.cleanup).toHaveBeenCalled();
241+
});
230242
});
231243

232244
describe('removeSampleData', () => {
@@ -260,23 +272,21 @@ describe('SampleDataManager', () => {
260272
const expectedIndexName = 'kibana_sample_data_kibana';
261273

262274
it('should return installed status when index exists', async () => {
263-
esClient.indices.exists.mockResolvedValue(true);
275+
mockIndexManager.hasIndex.mockResolvedValue(true);
264276

265277
const result = await sampleDataManager.getSampleDataStatus({ sampleType, esClient });
266278

267-
expect(esClient.indices.exists).toHaveBeenCalledWith({ index: expectedIndexName });
268279
expect(result).toEqual({
269280
status: 'installed',
270281
indexName: expectedIndexName,
271282
});
272283
});
273284

274285
it('should return uninstalled status when index does not exist', async () => {
275-
esClient.indices.exists.mockResolvedValue(false);
286+
mockIndexManager.hasIndex.mockResolvedValue(false);
276287

277288
const result = await sampleDataManager.getSampleDataStatus({ sampleType, esClient });
278289

279-
expect(esClient.indices.exists).toHaveBeenCalledWith({ index: expectedIndexName });
280290
expect(result).toEqual({
281291
status: 'uninstalled',
282292
indexName: undefined,
@@ -285,7 +295,7 @@ describe('SampleDataManager', () => {
285295

286296
it('should handle elasticsearch client errors gracefully', async () => {
287297
const error = new Error('Elasticsearch error');
288-
esClient.indices.exists.mockRejectedValue(error);
298+
mockIndexManager.hasIndex.mockRejectedValue(error);
289299

290300
const result = await sampleDataManager.getSampleDataStatus({ sampleType, esClient });
291301

@@ -312,17 +322,10 @@ describe('SampleDataManager', () => {
312322
expect(indexName).toBe('kibana_sample_data_kibana');
313323

314324
// Check status after installation
315-
esClient.indices.exists.mockResolvedValue(true);
325+
mockIndexManager.hasIndex.mockResolvedValue(true);
316326
status = await sampleDataManager.getSampleDataStatus({ sampleType, esClient });
317327
expect(status.status).toBe('installed');
318328
expect(status.indexName).toBe('kibana_sample_data_kibana');
319-
320-
// Remove sample data
321-
await sampleDataManager.removeSampleData({ sampleType, esClient });
322-
expect(mockIndexManager.deleteIndex).toHaveBeenCalledWith({
323-
indexName: 'kibana_sample_data_kibana',
324-
esClient,
325-
});
326329
});
327330
});
328331
});

x-pack/platform/plugins/shared/sample_data_ingest/server/services/sample_data_manager/sample_data_manager.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export class SampleDataManager {
2626
private readonly log: Logger;
2727
private readonly artifactManager: ArtifactManager;
2828
private readonly indexManager: IndexManager;
29+
private isInstalling: boolean = false;
2930

3031
constructor({
3132
artifactsFolder,
@@ -64,7 +65,12 @@ export class SampleDataManager {
6465
const indexName = getSampleDataIndexName(sampleType);
6566

6667
try {
67-
await this.removeSampleData({ sampleType, esClient });
68+
if ((await this.indexManager.hasIndex({ indexName, esClient })) || this.isInstalling) {
69+
this.log.warn(`Sample data already installed for [${sampleType}]`);
70+
return indexName;
71+
}
72+
73+
this.isInstalling = true;
6874

6975
const {
7076
archive: artifactsArchive,
@@ -97,6 +103,7 @@ export class SampleDataManager {
97103
}
98104

99105
await this.artifactManager.cleanup();
106+
this.isInstalling = false;
100107
}
101108
}
102109

@@ -120,10 +127,10 @@ export class SampleDataManager {
120127
}): Promise<StatusResponse> {
121128
const indexName = getSampleDataIndexName(sampleType);
122129
try {
123-
const isIndexExists = await esClient.indices.exists({ index: indexName });
130+
const hasIndex = await this.indexManager.hasIndex({ indexName, esClient });
124131
return {
125-
status: isIndexExists ? 'installed' : 'uninstalled',
126-
indexName: isIndexExists ? indexName : undefined,
132+
status: hasIndex ? 'installed' : 'uninstalled',
133+
indexName: hasIndex ? indexName : undefined,
127134
};
128135
} catch (error) {
129136
this.log.warn(`Failed to check sample data status for [${sampleType}]: ${error.message}`);

0 commit comments

Comments
 (0)