Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[esArchiver] Do not perform SO migration on cleanup #163302

Merged
merged 22 commits into from
Sep 1, 2023
Merged

[esArchiver] Do not perform SO migration on cleanup #163302

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
43cbad0
[esArchiver] Do not perform SO migration on cleanup
gsoldevila Aug 7, 2023
aae12cc
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 8, 2023
fc9e679
Unskip flaky suite
gsoldevila Aug 8, 2023
c603793
Fix incorrect tests
gsoldevila Aug 8, 2023
15ca6b3
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 8, 2023
0b9ba20
Make sure we go to the right Dashboard page
gsoldevila Aug 9, 2023
baa243c
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 9, 2023
1cdecea
Add missing await statement
gsoldevila Aug 9, 2023
2d0ee39
Fix the clickLink() issues
gsoldevila Aug 9, 2023
04481ef
Make sure we go to the right Dashboards page
gsoldevila Aug 10, 2023
ba9f2d3
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 10, 2023
7ed084d
Make sure `Download Report` popup is closed by each test
gsoldevila Aug 11, 2023
da54bc9
Clear toast notifications before generating / getting a report
gsoldevila Aug 28, 2023
d186adb
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 28, 2023
c8c5f9a
Wait for the welcome interstitial to be fully visible (500ms)
gsoldevila Aug 28, 2023
d9b2ccf
Add more logging for failed requests at Kibana level
gsoldevila Aug 29, 2023
733f8ff
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 29, 2023
7708424
Update snapshots with latest logging enhancements
gsoldevila Aug 29, 2023
ea25549
Update logged requests to NOT include the hostname
gsoldevila Aug 30, 2023
fda0195
Revert changes in router.ts (separate PR)
gsoldevila Aug 31, 2023
e1d93c1
Undo changes in router.test.ts
gsoldevila Aug 31, 2023
4870c7f
Merge branch 'main' into kbn-127545-do-not-migrate-on-cleanup
gsoldevila Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/core/http/core-http-router-server-internal/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,25 +200,30 @@ export class Router<Context extends RequestHandlerContextBase = RequestHandlerCo
try {
kibanaRequest = CoreKibanaRequest.from(request, routeSchemas);
} catch (e) {
this.log.error(`400 Bad Request - ${request.url}`);
this.log.error(e);
return hapiResponseAdapter.toBadRequest(e.message);
}

try {
const kibanaResponse = await handler(kibanaRequest, kibanaResponseFactory);
return hapiResponseAdapter.handle(kibanaResponse);
} catch (e) {
// log and capture error
this.log.error(e);
// capture error
apm.captureError(e);

// forward 401 errors from ES client
if (isElasticsearchUnauthorizedError(e)) {
this.log.error(`401 Unauthorized - ${request.url}`);
this.log.error(e);
return hapiResponseAdapter.handle(
kibanaResponseFactory.unauthorized(convertEsUnauthorized(e))
);
}

// return a generic 500 to avoid error info / stack trace surfacing
this.log.error(`500 Server Error - ${request.url}`);
this.log.error(e);
return hapiResponseAdapter.toInternalError();
}
}
Expand Down
16 changes: 3 additions & 13 deletions packages/kbn-es-archiver/src/actions/empty_kibana_index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,15 @@

import type { Client } from '@elastic/elasticsearch';
import { ToolingLog } from '@kbn/tooling-log';
import { KbnClient } from '@kbn/test';

import { ALL_SAVED_OBJECT_INDICES } from '@kbn/core-saved-objects-server';
import { migrateSavedObjectIndices, createStats, cleanSavedObjectIndices } from '../lib';
import { createStats, cleanSavedObjectIndices } from '../lib';

export async function emptyKibanaIndexAction({
client,
log,
kbnClient,
}: {
client: Client;
log: ToolingLog;
kbnClient: KbnClient;
}) {
export async function emptyKibanaIndexAction({ client, log }: { client: Client; log: ToolingLog }) {
const stats = createStats('emptyKibanaIndex', log);

await cleanSavedObjectIndices({ client, stats, log });
await migrateSavedObjectIndices(kbnClient);
await client.indices.refresh({ index: ALL_SAVED_OBJECT_INDICES });
ALL_SAVED_OBJECT_INDICES.forEach((indexPattern) => stats.createdIndex(indexPattern));

return stats.toJSON();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment we call SO migration during esArchive loading and only if it modified .kibana index

kibana/packages/kbn-es-archiver/src/actions/load.ts

Lines 109 to 118 in a29e4aa

if (Object.keys(result).some((k) => k.startsWith(MAIN_SAVED_OBJECT_INDEX))) {
await migrateSavedObjectIndices(kbnClient);
log.debug('[%s] Migrated Kibana index after loading Kibana data', name);
if (kibanaPluginIds.includes('spaces')) {
// WARNING affected by #104081. Assumes 'spaces' saved objects are stored in MAIN_SAVED_OBJECT_INDEX
await createDefaultSpace({ client, index: MAIN_SAVED_OBJECT_INDEX });
log.debug(`[%s] Ensured that default space exists in ${MAIN_SAVED_OBJECT_INDEX}`, name);
}
}

If I understand correctly, in .kibana cleanup SO migration was from the start (added 5 years ago):
c64624a#diff-b22ea1611642a7e2cdc07bce3086e1b7e260f075fb08fdc9c2ae67ecf3742444R23-R24

Performing a migration as part of the cleanup does not make too much sense (at least in this test suite)

Could you provide more context here. Since many tests call await esArchiver.emptyKibanaIndex(); I would like to understand if not running SO migrations is desired behaviour for cleanup or it should be optionally run for some cases?

Copy link
Member

@dmlemeshko dmlemeshko Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of those esArchiver.emptyKibanaIndex() calls were added in #85778

Enables v2 migrations in all tests
Because of removing the auto-create index behaviour I had to adapt tests 
to always initialize an empty kibana with esArchiver.emptyKibanaIndex() 
or move some before statements around so that we don't set 
UiSettings before the index gets initialized.

@rudolf do you think we can safely remove migration from that call and be sure it will work for all those cases?

started flaky-test-runner to check those tests https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/2851

Copy link
Contributor Author

@gsoldevila gsoldevila Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider the following groups of tests that use esArchiver:

  • 1. Tests where we have deleted the mappings.json definitions. We are implicitly using the "official" saved object indices created by Kibana at startup, which are already aligned with the current stack version. In these cases, there's no need to "realign" them. On the contrary, doing so triggers unnecessary migrations that can cause errors + flakiness.

  • 2. Tests that don't define saved object indices. There are a few tests that use esArchiver archives that DON'T define saved object indices (they define other indices). For these tests, we fall in the same scenario as (1), i.e. we are not recreating the "official" indices created by Kibana, and thus there's no need to "realign" them.

  • 3. Tests that define .kibana index (and perhaps other saved object indices) in mappings.json. If some of these tests are calling esArchiver.emptyKibanaIndex(), they could be impacted by this PR, as we are no longer migrating on cleanup. But so far CI hasn't complained, and I'm on a crusade to remove them all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah quite a lot has changed since #85778 and I agree with @gsoldevila that (3) should be the only tests likely to be impacted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details, especially great to hear you are working on those tests removal.

@gsoldevila I guess there is not much we can do in esArchiver code to prevent someone from adding new test suites with loading SO indexes and calling emptyKibanaIndex later in the test? We still need to load SO indexes with custom mappings for some scenarios, right?
Maybe leaving a comment to the emptyKibanaIndex function is a good thing to have.

Copy link
Contributor Author

@gsoldevila gsoldevila Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if/when I manage to remove all SO index definitions from mappings.json, I can throw an exception from esArchiver side, saying that defining such indices is no longer supported.

}
4 changes: 1 addition & 3 deletions packages/kbn-es-archiver/src/es_archiver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,12 @@ export class EsArchiver {
}

/**
* Delete any Kibana indices, and initialize the Kibana index as Kibana would do
* on startup.
* Cleanup saved object indices, preserving the space:default saved object.
*/
async emptyKibanaIndex() {
return await emptyKibanaIndexAction({
client: this.client,
log: this.log,
kbnClient: this.kbnClient,
});
}

Expand Down
49 changes: 45 additions & 4 deletions src/core/server/integration_tests/http/router.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,11 +576,14 @@ describe('Handler', () => {
'An internal server error occurred. Check Kibana server logs for details.'
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
Array [
[Error: unexpected error],
],
]
"500 Server Error - http://127.0.0.1:50107/",
],
Array [
[Error: unexpected error],
],
]
`);
});

Expand Down Expand Up @@ -616,6 +619,9 @@ describe('Handler', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50113/",
],
Array [
[Error: Unauthorized],
],
Expand All @@ -638,6 +644,9 @@ describe('Handler', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50116/",
],
Array [
[Error: Unexpected result from Route Handler. Expected KibanaResponse, but given: string.],
],
Expand Down Expand Up @@ -672,6 +681,17 @@ describe('Handler', () => {
message: '[request query.page]: expected value of type [number] but got [string]',
statusCode: 400,
});

expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"400 Bad Request - http://127.0.0.1:50119/?page=one",
],
Array [
[Error: [request query.page]: expected value of type [number] but got [string]],
],
]
`);
});

it('accept to receive an array payload', async () => {
Expand Down Expand Up @@ -1144,6 +1164,9 @@ describe('Response factory', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50185/",
],
Array [
[Error: expected 'location' header to be set],
],
Expand Down Expand Up @@ -1550,6 +1573,9 @@ describe('Response factory', () => {
});
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50239/",
],
Array [
[Error: Unexpected Http status code. Expected from 400 to 599, but given: 200],
],
Expand Down Expand Up @@ -1619,6 +1645,9 @@ describe('Response factory', () => {

expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50248/",
],
Array [
[Error: expected 'location' header to be set],
],
Expand Down Expand Up @@ -1759,6 +1788,9 @@ describe('Response factory', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50266/",
],
Array [
[Error: expected error message to be provided],
],
Expand All @@ -1785,6 +1817,9 @@ describe('Response factory', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50269/",
],
Array [
[Error: expected error message to be provided],
],
Expand All @@ -1810,6 +1845,9 @@ describe('Response factory', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50272/",
],
Array [
[Error: options.statusCode is expected to be set. given options: undefined],
],
Expand All @@ -1835,6 +1873,9 @@ describe('Response factory', () => {
);
expect(loggingSystemMock.collect(logger).error).toMatchInlineSnapshot(`
Array [
Array [
"500 Server Error - http://127.0.0.1:50275/",
],
Array [
[Error: Unexpected Http status code. Expected from 100 to 599, but given: 20.],
],
Expand Down
1 change: 1 addition & 0 deletions test/examples/search/warnings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
// click "see full error" button in the toast
const [openShardModalButton] = await testSubjects.findAll('openShardFailureModalBtn');
await openShardModalButton.click();
await testSubjects.exists('shardFailureModalTitle');
const modalHeader = await testSubjects.find('shardFailureModalTitle');
expect(await modalHeader.getVisibleText()).to.be('2 of 4 shards failed');
// request
Expand Down
4 changes: 3 additions & 1 deletion test/functional/page_objects/home_page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ export class HomePageObject extends FtrService {
}

async isWelcomeInterstitialDisplayed() {
return await this.testSubjects.isDisplayed('homeWelcomeInterstitial');
// give the interstitial enough time to fade in
await new Promise((resolve) => setTimeout(resolve, 500));
return await this.testSubjects.isDisplayed('homeWelcomeInterstitial', 2000);
}

async isGuidedOnboardingLandingDisplayed() {
Expand Down
5 changes: 5 additions & 0 deletions test/functional/services/common/browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ class BrowserService extends FtrService {
return await this.driver.manage().window().getRect();
}

public async getWindowInnerSize(): Promise<{ height: number; width: number }> {
const JS_GET_INNER_WIDTH = 'return { width: window.innerWidth, height: window.innerHeight };';
return await this.driver.executeScript(JS_GET_INNER_WIDTH);
}

/**
* Sets the dimensions of a window.
* https://seleniumhq.github.io/selenium/docs/api/javascript/module/selenium-webdriver/lib/webdriver_exports_Window.html
Expand Down
4 changes: 2 additions & 2 deletions test/functional/services/common/test_subjects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,9 @@ export class TestSubjects extends FtrService {
return await element.isEnabled();
}

public async isDisplayed(selector: string): Promise<boolean> {
public async isDisplayed(selector: string, timeout?: number): Promise<boolean> {
this.log.debug(`TestSubjects.isDisplayed(${selector})`);
const element = await this.find(selector);
const element = await this.find(selector, timeout);
return await element.isDisplayed();
}

Expand Down
8 changes: 4 additions & 4 deletions test/plugin_functional/test_suites/core_plugins/applications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
const navigateTo = async (path: string) =>
await browser.navigateTo(`${deployment.getHostPort()}${path}`);

// FLAKY: https://github.com/elastic/kibana/issues/127545
describe.skip('ui applications', function describeIndexTests() {
describe('ui applications', function describeIndexTests() {
before(async () => {
await esArchiver.emptyKibanaIndex();
await PageObjects.common.navigateToApp('foo');
await PageObjects.common.dismissBanner();
});

it('starts on home page', async () => {
Expand Down Expand Up @@ -126,7 +126,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
expect(await testSubjects.exists('headerGlobalNav')).to.be(false);

const wrapperHeight = await getAppWrapperHeight();
const windowHeight = (await browser.getWindowSize()).height;
const windowHeight = (await browser.getWindowInnerSize()).height;
expect(wrapperHeight).to.eql(windowHeight);
});

Expand All @@ -136,7 +136,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
expect(await testSubjects.exists('headerGlobalNav')).to.be(true);

const wrapperHeight = await getAppWrapperHeight();
const windowHeight = (await browser.getWindowSize()).height;
const windowHeight = (await browser.getWindowInnerSize()).height;
expect(wrapperHeight).to.be.below(windowHeight);
});
});
Expand Down
17 changes: 13 additions & 4 deletions x-pack/test/functional/apps/dashboard/group2/_async_dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
});

it('should launch sample flights data set dashboard', async () => {
await appMenu.clickLink('Dashboard');
await PageObjects.common.navigateToApp('dashboard');
await PageObjects.dashboard.loadSavedDashboard('[Flights] Global Flight Dashboard');
await PageObjects.header.waitUntilLoadingHasFinished();
await PageObjects.timePicker.setCommonlyUsedTime('sample_data range');
Expand All @@ -144,7 +144,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
const hitCount = parseInt(await PageObjects.discover.getHitCount(), 10);
expect(hitCount).to.be.greaterThan(0);
});
await appMenu.clickLink('Dashboard');
await appMenu.clickLink('Dashboard', {
category: 'recentlyViewed',
closeCollapsibleNav: true,
});
await PageObjects.header.waitUntilLoadingHasFinished();
await renderable.waitForRender();
log.debug('Checking charts rendered');
Expand All @@ -157,7 +160,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
const hitCount = parseInt(await PageObjects.discover.getHitCount(), 10);
expect(hitCount).to.be.greaterThan(0);
});
await appMenu.clickLink('Dashboard');
await appMenu.clickLink('Dashboard', {
category: 'recentlyViewed',
closeCollapsibleNav: true,
});
await PageObjects.header.waitUntilLoadingHasFinished();
await renderable.waitForRender();
log.debug('Checking charts rendered');
Expand All @@ -170,7 +176,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
const hitCount = parseInt(await PageObjects.discover.getHitCount(), 10);
expect(hitCount).to.be.greaterThan(0);
});
await appMenu.clickLink('Dashboard');
await appMenu.clickLink('Dashboard', {
category: 'recentlyViewed',
closeCollapsibleNav: true,
});
await PageObjects.header.waitUntilLoadingHasFinished();
await renderable.waitForRender();
log.debug('Checking charts rendered');
Expand Down
5 changes: 4 additions & 1 deletion x-pack/test/functional/apps/dashboard/group2/dashboard_maps_by_value.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
});

if (!saveToDashboard) {
await appsMenu.clickLink('Dashboard');
await appsMenu.clickLink('Dashboard', {
category: 'kibana',
closeCollapsibleNav: true,
});
}
} else {
await PageObjects.maps.clickSaveAndReturnButton();
Expand Down
3 changes: 3 additions & 0 deletions x-pack/test/functional/apps/discover/reporting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
};

const getReport = async () => {
// close any open notification toasts
await PageObjects.reporting.clearToastNotifications();

await PageObjects.reporting.openCsvReportingPanel();
await PageObjects.reporting.clickGenerateReportButton();

Expand Down