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

[Draft] Play around with index_mode in Fleet #147684

Closed
wants to merge 1 commit into from
Closed

[Draft] Play around with index_mode in Fleet #147684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions ...eate_package_policy_page/components/steps/components/experimental_datastream_settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
experimentalDataFeatures ?? [],
registryDataStream
);

// Additional dependency here on TSDB feature set
const isSyntheticSourceEnabledByDefault =
registryDataStream.elasticsearch?.source_mode === 'synthetic';

Expand All @@ -65,6 +67,7 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
typeof syntheticSourceExperimentalValue !== 'undefined'
? syntheticSourceExperimentalValue
: isSyntheticSourceEnabledByDefault,
// I initially was looking for this but couldn't find it as I was looking for time_series. Should we align on naming?
Copy link
Member

Choose a reason for hiding this comment

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

It's a good point about alignment here. I went with tsdb because it seemed that's how the Elasticsearch teams were talking about the feature in their issues/PR's. Generally, though, I'm in favor of avoiding acronyms/abbreviations in code wherever possible so I'd be +1 for moving to time_series in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fully honest, I find time_series pretty confusing as all data streams are time series. But unfortunately it is what the Elasticsearch team has selected so at least in the code base, I would align it. What we show as "text" in the UI can be a different story.

tsdb:
getExperimentalFeatureValue('tsdb', experimentalDataFeatures ?? [], registryDataStream) ??
false,
Expand Down Expand Up @@ -93,6 +96,11 @@ export const ExperimentDatastreamSettings: React.FunctionComponent<Props> = ({
});
}

// This sets the features on change, will it update data stream itself?
// Is this were rollover should happen?
// Ideally there would be a "data management" part in Fleet (or outside) where on a change
// the policy API could just make a call with the requested change and would get back if it was applied or
// not, not having to think about templates, data streams etc.
Comment on lines +99 to +103
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 just the frontend state tracking of the opt-in state for each experimental indexing feature. The package_policy API handles actually updating templates here:

kibana/x-pack/plugins/fleet/server/services/package_policy.ts

Lines 182 to 183 in 8652775

// Handle component template/mappings updates for experimental features, e.g. synthetic source
await handleExperimentalDatastreamFeatureOptIn({ soClient, esClient, packagePolicy });

kibana/x-pack/plugins/fleet/server/services/package_policies/experimental_datastream_features.ts

Lines 44 to 152 in 8652775

export async function handleExperimentalDatastreamFeatureOptIn({
soClient,
esClient,
packagePolicy,
}: {
soClient: SavedObjectsClientContract;
esClient: ElasticsearchClient;
packagePolicy: PackagePolicy | NewPackagePolicy;
}) {
if (!packagePolicy.package?.experimental_data_stream_features) {
return;
}
// If we're performing an update, we want to check if we actually need to perform
// an update to the component templates for the package. So we fetch the saved object
// for the package policy here to compare later.
let installation;
if (packagePolicy.package) {
installation = await getInstallation({
savedObjectsClient: soClient,
pkgName: packagePolicy.package.name,
});
}
for (const featureMapEntry of packagePolicy.package.experimental_data_stream_features) {
const existingOptIn = installation?.experimental_data_stream_features?.find(
(optIn) => optIn.data_stream === featureMapEntry.data_stream
);
const isSyntheticSourceOptInChanged =
existingOptIn?.features.synthetic_source !== featureMapEntry.features.synthetic_source;
const isTSDBOptInChanged = existingOptIn?.features.tsdb !== featureMapEntry.features.tsdb;
if (!isSyntheticSourceOptInChanged && !isTSDBOptInChanged) continue;
const componentTemplateName = `${featureMapEntry.data_stream}@package`;
const componentTemplateRes = await esClient.cluster.getComponentTemplate({
name: componentTemplateName,
});
const componentTemplate = componentTemplateRes.component_templates[0].component_template;
if (isSyntheticSourceOptInChanged) {
const body = {
template: {
...componentTemplate.template,
mappings: {
...componentTemplate.template.mappings,
_source: {
mode: featureMapEntry.features.synthetic_source ? 'synthetic' : 'stored',
},
},
},
};
await esClient.cluster.putComponentTemplate({
name: componentTemplateName,
// @ts-expect-error - TODO: Remove when ES client typings include support for synthetic source
body,
});
}
if (isTSDBOptInChanged && featureMapEntry.features.tsdb) {
const mappingsProperties = componentTemplate.template?.mappings?.properties ?? {};
// All mapped fields of type keyword and time_series_dimension enabled will be included in the generated routing path
// Temporarily generating routing_path here until fixed in elasticsearch https://github.com/elastic/elasticsearch/issues/91592
const routingPath = builRoutingPath(mappingsProperties);
if (routingPath.length === 0) continue;
const indexTemplateRes = await esClient.indices.getIndexTemplate({
name: featureMapEntry.data_stream,
});
const indexTemplate = indexTemplateRes.index_templates[0].index_template;
const indexTemplateBody = {
...indexTemplate,
template: {
...(indexTemplate.template ?? {}),
settings: {
...(indexTemplate.template?.settings ?? {}),
index: {
mode: 'time_series',
routing_path: routingPath,
},
},
},
};
await esClient.indices.putIndexTemplate({
name: featureMapEntry.data_stream,
body: indexTemplateBody,
});
}
}
// Update the installation object to persist the experimental feature map
await updateDatastreamExperimentalFeatures(
soClient,
packagePolicy.package.name,
packagePolicy.package.experimental_data_stream_features
);
// Delete the experimental features map from the package policy so it doesn't get persisted
delete packagePolicy.package.experimental_data_stream_features;
}

Is this where rollover should happen

Automatic rollover should happen in the above server-side code. I don't think it'd be too challenging, but there might be caveats of which I am unaware. Issue at #143448

Ideally there would be a "data management" part in Fleet (or outside) where on a change the policy API could just make a call with the requested change and would get back if it was applied or not, not having to think about templates, data streams etc.

I'm in agreement here. We've talked about something like a /api/fleet/customize API to handle cases like this around pushing custom configuration objects. @joshdover has a great pass at designing this kind of API at #121118, namely the Customize API section feels relevant here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great to see this is already all started to be tracked. The reason I'm keen on #143448 to happen is that I think at the moment when a user "toggles" any of the features, the user assumes it is working now but in reality the user has to trigger a rollover first. We already do rollovers on upgrades when needed so I would assume the functionality + error handling mostly already exists.

/api/fleet/customize

I might go further and not put it into the fleet plugin. From my perspective it goes so far, that this is a data management API that should be exposed by Kibana "core" and is only used by Fleet. Need to read up on the full issue and comment there.

setNewExperimentalDataFeatures(newExperimentalDataStreamFeatures);
};

Expand Down
5 changes: 5 additions & 0 deletions ...icy/create_package_policy_page/components/steps/components/package_policy_input_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ export const PackagePolicyInputPanel: React.FunctionComponent<{
(ds) => ds.dataset === packagePolicyInputStream?.data_stream.dataset
);

// It seems this logic does check for the data stream setting. What if template and data stream are not in sync (no rollover yet)
// Is there a way in Elasticsearch to say: For data stream X, what template will apply? The reason I'm asking
// is that there is a chance that users made some modification in the settings or that for a namespace an additional
// template was set in place. So if we only look at the data stream, the info might be wrong / outdated.
Comment on lines +137 to +140
Copy link
Member

Choose a reason for hiding this comment

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

It seems this logic does check for the data stream setting. What if template and data stream are not in sync (no rollover yet)

This is correct. Our current implementation does not consider whether the currently "visible" value of a given toggle has actually been applied (rolled over) to the given data stream. I think this okay-ish for now, since the features are presented quite heavily as experimental, and I expect them to be removed in favor of package spec support as the source of truth in the near future.

If we had automated rollover #143448, I think this issue would go away, as well. So, if that's low hanging fruit as I think it is, maybe that's the quick solve here.

Is there a way in Elasticsearch to say: For data stream X, what template will apply?

The reason I'm asking is that there is a chance that users made some modification in the settings or that for a namespace an additional template was set in place. So if we only look at the data stream, the info might be wrong / outdated.

This I do not know. Poking around the index template and data stream docs for Elasticserach didn't turn up much, but there might be some internal API to expose this. Or maybe there's a clever search operation we could use here. Would be interesting to look into it further, but not something I'm immediately aware of.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know in case the Fleet team is interested in having such an API, I might be able to contribute it to Elasticsearch.

if (dataStreamInfo?.elasticsearch?.index_mode === 'time_series') {
if (!packagePolicy.package) return;
if (!packagePolicy.package?.experimental_data_stream_features)
Expand All @@ -143,6 +147,7 @@ export const PackagePolicyInputPanel: React.FunctionComponent<{
const match = packagePolicy.package!.experimental_data_stream_features.find(
(feat) => feat.data_stream === dsName
);
// If the user updates the TSDB setting, will this make its way to the template? Could not find where.
Copy link
Member

Choose a reason for hiding this comment

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

Yep happens here:

kibana/x-pack/plugins/fleet/server/services/package_policies/experimental_datastream_features.ts

Lines 122 to 140 in 8652775

const indexTemplateBody = {
...indexTemplate,
template: {
...(indexTemplate.template ?? {}),
settings: {
...(indexTemplate.template?.settings ?? {}),
index: {
mode: 'time_series',
routing_path: routingPath,
},
},
},
};
await esClient.indices.putIndexTemplate({
name: featureMapEntry.data_stream,
body: indexTemplateBody,
});
}

if (match) {
if (!match.features.tsdb) {
match.features.tsdb = true;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/services/epm/archive/parse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,7 @@ export function parseDataStreamElasticsearchEntry(
parsedElasticsearchEntry.privileges = expandedElasticsearch.privileges;
}

// we parse the settings from the data stream, what abou tthe values from the index template?
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm not following this one. Could you provide any more info?

Copy link
Member Author

Choose a reason for hiding this comment

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

To decide what to show to the user, we parse the value of the current data stream setting. But what if through a package upgrade the value of the template has been adjusted but the data stream still has the old value. This partially goes back to the above discussion around triggering a rollover when this happens which would resolve this mostly.

The second missing part also what we touched on above: There is no way currently to tell which template applies to a data stream. We know it today because of the convention but if we would have templates per namespace too, it gets trickier. An ES API for it would help here.

The more I think about all these scenarios the more I think all this handling should only be temporary in Fleet itself but be done in Elasticsearch itself. Tell Elasticsearch to change a setting for a data stream and it will do all the updates and rollovers needed ...

if (expandedElasticsearch?.source_mode) {
parsedElasticsearchEntry.source_mode = expandedElasticsearch.source_mode;
}
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugins/fleet/server/services/epm/elasticsearch/template/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,10 @@ export function buildComponentTemplates(params: {

const indexTemplateSettings = registryElasticsearch?.['index_template.settings'] ?? {};

// TODO: Is this the place that index_mode should be added as it belongs into template.settings.index.mode
// On package upgrade, what happens if the previous package version had index_mode: time_series and now it is
// switched back? This would work for the template but not when the data stream is rolled over
// Ideally TSDB can be disabled again but is not possible today -> validation on the package side needed?
Comment on lines +273 to +276
Copy link
Member

Choose a reason for hiding this comment

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

It's a good point that we could set index.mode as normal here, but the next rollover would fail. Not a very good experience around upgrading package versions in that case. We could probably catch the case where a package tried to disable tsdb after enabling it in a previous version here and throw an error, but it'd result in a package version that cannot be upgraded to. So, it makes the most sense to add some package spec validation here blocking a release that includes a change from index_mode: time_series -> default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked to the Elasticsearch team about the issue that tsdb currently can't be disabled. elastic/elasticsearch#92544 was opened to make it possible to turn it off again. If this lands, this problem should go away.

const templateSettings = merge(defaultSettings, indexTemplateSettings);

const indexTemplateMappings = registryElasticsearch?.['index_template.mappings'] ?? {};
Expand All @@ -281,6 +285,7 @@ export function buildComponentTemplates(params: {
(dynampingTemplate) => Object.keys(dynampingTemplate)[0]
);

// In case of index_mode: time_series this should be set to true by default except it is overwritten
Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me - we should try to map out the relationship between synthetic source + time series indexing in code to make this clear. Rather have some declarative structure that describes that relationship rather than a bunch of chained conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

++. There should be a single place to look up and change all this logic. Similar to a state machine.

const sourceModeSynthetic =
params.experimentalDataStreamFeature?.features.synthetic_source !== false &&
(params.experimentalDataStreamFeature?.features.synthetic_source === true ||
Expand Down
3 changes: 3 additions & 0 deletions x-pack/test/fleet_api_integration/apis/epm/get.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,9 @@ export default function (providerContext: FtrProviderContext) {
const dataStream = packageInfo?.data_streams?.find(
({ dataset }) => dataset === 'non_epr_fields.test_metrics_2'
);
// These are tests for source_mode set in the package. Couldn't find any integration tests around the policy part?
// Is this all covered by unit tests? I'm especially curious how an upgrade around this feature is tested?
// And how do you test it manually with uploading a package when you run from source -> run your own registry?
Comment on lines +185 to +187
Copy link
Member

Choose a reason for hiding this comment

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

These are tests for source_mode set in the package. Couldn't find any integration tests around the policy part?

I don't think we have integration tests for the toggling functionality/API interactions here, so those would probably be good to add.

We do have unit tests around the policy generation in

There's UI driven tests around the toggling that assert on whether we're generating the expected API payloads, defaulting to the expected values based on the existing data streams, etc here as well: https://github.com/elastic/kibana/blob/86527753a2db307138ea69e0ba04551717a91844/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/components/steps/components/experimental_datastream_settings.test.tsx

And how do you test it manually with uploading a package when you run from source -> run your own registry?

We create a new version of package that includes time series features, copy that new version over to a data directory, then point the local registry at that data directory. Then, the test package version is available as normal in Kibana, Fleet API, etc for testing upgrades. We don't use the upload package feature in this case. The registry service takes a config that includes locations of package manifests on disk, so we can't point it at a few directories that it'll use to source packages.

expect(dataStream?.elasticsearch?.source_mode).equal('default');
});
it('returns package info from the registry if ?full=false', async function () {
Expand Down
3 changes: 2 additions & 1 deletion ...on/apis/fixtures/test_packages/non_epr_fields/1.0.0/data_stream/test_metrics/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ streams:
show_user: true

elasticsearch:
source_mode: "synthetic"
# Good to see these packages for testing. Is there a flow to test upgrades?
source_mode: "synthetic"
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

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

We have some assertions around updates in

kibana/x-pack/test/fleet_api_integration/apis/epm/update_assets.ts

Line 37 in 8652775

describe('updates all assets when updating a package to a different version', async () => {
, but I don't think there are specific integration tests around upgrades where source_mode or index_mode are toggled. Definitely worth adding some, especially since the package spec values will be the sole source of truth in the future and the UI toggles will be removed.