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

Jump to bottom

[Ingest Manager] Make setupIngestManager wait if setup is in progress #70008

Merged
merged 4 commits into from
Jun 30, 2020
Merged

[Ingest Manager] Make setupIngestManager wait if setup is in progress #70008

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6134a54
Make setupIngestManager wait if setup is in progress
Jun 25, 2020
431acdd
Remove the example 3 parallel calls
Jun 26, 2020
a8cbc6a
Merge branch 'master' into 69843-two-default-configs
elasticmachine Jun 26, 2020
417f45f
Merge branch 'master' into 69843-two-default-configs
elasticmachine Jun 30, 2020
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
137 changes: 80 additions & 57 deletions x-pack/plugins/ingest_manager/server/services/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,70 +30,93 @@ import { appContextService } from './app_context';
const FLEET_ENROLL_USERNAME = 'fleet_enroll';
const FLEET_ENROLL_ROLE = 'fleet_enroll';

// the promise which tracks the setup
let setupIngestStatus: Promise<void> | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

This is not going to work if we have multiple instances of Kibana no? Can we use a saved object, to keep the state of the setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wondered about the same things. From Slack the other day:

I initially had some logic for dealing with pending & succeeded state but they way we tried initially had issues in multi-kibana setups (and tests, iirc). We might need to move the state out of JS and into ES or somewhere else that's durable & shared by multiple Kibana

I did this because it's the simplest thing change that could possibly work, but I do think we'll want to move it to Saved Objects eventually.

Along those lines and https://github.com/elastic/kibana/issues/63123 perhaps we want a GET route to match /fleet/setup?

Do you think these need to be done in this PR or can they be discussed/implemented in a followup issue?

Copy link
Member

Choose a reason for hiding this comment

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

I am in favor of doing it in this PR as I think this is probably not going to solve our problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the failure in multi-kibana exist in the code before #69505, e.g. in 7.8?

I'll see how quickly I can do a dead-simple boolean but, like I said in the description, I think we'll want more fine-grained permissions and I'd like to avoid designing that during the PR or writing such short-lived code if this works. I know, "if" it works.

// default resolve & reject to guard against "undefined is not a function" errors
let onSetupResolve = () => {};
let onSetupReject = (error: Error) => {};

export async function setupIngestManager(
soClient: SavedObjectsClientContract,
callCluster: CallESAsCurrentUser
) {
const [installedPackages, defaultOutput, config] = await Promise.all([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is from moving the body inside a try/catch

// packages installed by default
ensureInstalledDefaultPackages(soClient, callCluster),
outputService.ensureDefaultOutput(soClient),
agentConfigService.ensureDefaultAgentConfig(soClient),
ensureDefaultIndices(callCluster),
settingsService.getSettings(soClient).catch((e: any) => {
if (e.isBoom && e.output.statusCode === 404) {
const http = appContextService.getHttpSetup();
const serverInfo = http.getServerInfo();
const basePath = http.basePath;

const cloud = appContextService.getCloud();
const cloudId = cloud?.isCloudEnabled && cloud.cloudId;
const cloudUrl = cloudId && decodeCloudId(cloudId)?.kibanaUrl;
const flagsUrl = appContextService.getConfig()?.fleet?.kibana?.host;
const defaultUrl = url.format({
protocol: serverInfo.protocol,
hostname: serverInfo.host,
port: serverInfo.port,
pathname: basePath.serverBasePath,
});

return settingsService.saveSettings(soClient, {
agent_auto_upgrade: true,
package_auto_upgrade: true,
kibana_url: cloudUrl || flagsUrl || defaultUrl,
});
}

return Promise.reject(e);
}),
]);

// ensure default packages are added to the default conifg
const configWithDatasource = await agentConfigService.get(soClient, config.id, true);
if (!configWithDatasource) {
throw new Error('Config not found');
}
if (
configWithDatasource.datasources.length &&
typeof configWithDatasource.datasources[0] === 'string'
) {
throw new Error('Config not found');
// installation in progress
if (setupIngestStatus) {
await setupIngestStatus;
} else {
// create the initial promise
setupIngestStatus = new Promise((res, rej) => {
onSetupResolve = res;
onSetupReject = rej;
});
}
for (const installedPackage of installedPackages) {
const packageShouldBeInstalled = DEFAULT_AGENT_CONFIGS_PACKAGES.some(
(packageName) => installedPackage.name === packageName
);
if (!packageShouldBeInstalled) {
continue;
try {
Copy link
Contributor

@neptunian neptunian Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason we can't put this whole try/catch block into the else above so we don't need to run all this code again since we now know that it ran/is running? Would speed things up a bit. I tested it out and it seems to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would only run the setup one time which isn't the current or previous behavior. I'm trying to avoid any changes to the prior flow / guarantees or only make the smallest incremental changes required.

We'll definitely revisit this and other parts of the setup in later PRs but for now I want small conservative changes.

Copy link
Contributor

@neptunian neptunian Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, but we were only ever running the entire setup twice because we couldn't track the first. Now there is no need. Otherwise this change is going to cause the Ingest Manager setup to take longer because we have to await the first call before running the whole of the second and awaiting that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but we were only ever running the entire setup twice because we couldn't track the first. Now there is no need.

Then maybe we can remove the second call? Either way, I'm starting with stability then we can move on to speed.

Copy link
Contributor

@neptunian neptunian Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We still need the second call, which functions more like a check now, because its purpose is to not let the user interact with Ingest Manager until setup has completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should talk about this outside this PR. In person or another issue. As far as I can tell, this PR fixes the known bugs and tests are passing. I'd like to take the win and do more in another effort.

const [installedPackages, defaultOutput, config] = await Promise.all([
// packages installed by default
ensureInstalledDefaultPackages(soClient, callCluster),
outputService.ensureDefaultOutput(soClient),
agentConfigService.ensureDefaultAgentConfig(soClient),
ensureDefaultIndices(callCluster),
settingsService.getSettings(soClient).catch((e: any) => {
if (e.isBoom && e.output.statusCode === 404) {
const http = appContextService.getHttpSetup();
const serverInfo = http.getServerInfo();
const basePath = http.basePath;

const cloud = appContextService.getCloud();
const cloudId = cloud?.isCloudEnabled && cloud.cloudId;
const cloudUrl = cloudId && decodeCloudId(cloudId)?.kibanaUrl;
const flagsUrl = appContextService.getConfig()?.fleet?.kibana?.host;
const defaultUrl = url.format({
protocol: serverInfo.protocol,
hostname: serverInfo.host,
port: serverInfo.port,
pathname: basePath.serverBasePath,
});

return settingsService.saveSettings(soClient, {
agent_auto_upgrade: true,
package_auto_upgrade: true,
kibana_url: cloudUrl || flagsUrl || defaultUrl,
});
}

return Promise.reject(e);
}),
]);

// ensure default packages are added to the default conifg
const configWithDatasource = await agentConfigService.get(soClient, config.id, true);
if (!configWithDatasource) {
throw new Error('Config not found');
}
if (
configWithDatasource.datasources.length &&
typeof configWithDatasource.datasources[0] === 'string'
) {
throw new Error('Config not found');
}
for (const installedPackage of installedPackages) {
const packageShouldBeInstalled = DEFAULT_AGENT_CONFIGS_PACKAGES.some(
(packageName) => installedPackage.name === packageName
);
if (!packageShouldBeInstalled) {
continue;
}

const isInstalled = configWithDatasource.datasources.some((d: Datasource | string) => {
return typeof d !== 'string' && d.package?.name === installedPackage.name;
});

if (!isInstalled) {
await addPackageToConfig(soClient, installedPackage, configWithDatasource, defaultOutput);
const isInstalled = configWithDatasource.datasources.some((d: Datasource | string) => {
return typeof d !== 'string' && d.package?.name === installedPackage.name;
});
if (!isInstalled) {
await addPackageToConfig(soClient, installedPackage, configWithDatasource, defaultOutput);
}
}

// if everything works, resolve/succeed
onSetupResolve();
} catch (error) {
// if anything errors, reject/fail
onSetupReject(error);
}
}

Expand Down Expand Up @@ -135,7 +158,7 @@ export async function setupFleet(
},
});

await outputService.invalidateCache();
jfsiii marked this conversation as resolved.
Show resolved Hide resolved
outputService.invalidateCache();

// save fleet admin user
const defaultOutputId = await outputService.getDefaultOutputId(soClient);
Expand Down