Auto create data streams using index templates v2 #55377

martijnvg · 2020-04-17T10:15:05Z

This PR adds the ability to auto create data streams using index templates v2.
Index templates (v2) now have a data_steam field that includes a timestamp field,
if provided and index name matches with that template then a data stream
(plus first backing index) is auto created. The index/bulk apis will redirect
to the create data stream api instead of the create index api.

Relates to #53100

This commit adds the ability to auto create data streams using index templates v2. Index templates (v2) now have a data_steam field that includes a timestamp field, if provided and index name matches with that template then a data stream (plus first backing index) is auto created. The index/bulk apis will redirect to the create data stream api instead of the create index api. Relates to elastic#53100

elasticmachine · 2020-04-17T10:15:07Z

Pinging @elastic/es-core-features (:Core/Features/Data streams)

…w to de-serialize it

removed unused imports

danhermann

LGTM. Left some questions about minor, non-essential items.

danhermann · 2020-04-22T20:42:57Z

...pec/src/main/resources/rest-api-spec/test/indices.data_stream/30_auto_create_data_stream.yml

+      allowed_warnings:
+        - "index template [test] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [test] will take precedence during new index creation"
+      indices.put_index_template:
+        name: generic_logs_template


Do we not need to remove this ITv2 at the end of the test?

V2 templates are removed at the end of each test automatically, so shouldn't be a concern

danhermann · 2020-04-22T20:44:50Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

+                if (!(ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException)) {
+                    // fail all requests involving this index, if create didn't work
+                    for (int i = 0; i < bulkRequest.requests.size(); i++) {
+                        DocWriteRequest<?> item = bulkRequest.requests.get(i);
+                        if (item != null && setResponseFailureIfIndexMatches(responses, i, item, name, e)) {
+                            bulkRequest.requests.set(i, null);
+                        }
+                    }
+                }


Duplicates code in the onFailure handler in the doExecute method. Might be worth consolidating.

danhermann · 2020-04-22T20:45:59Z

server/src/test/java/org/elasticsearch/action/bulk/TransportBulkActionTests.java

+        Map<String, DataStreamTemplate> result = TransportBulkAction.resolveAutoCreateDataStreams(metadata, Set.of());
+        assertThat(result, anEmptyMap());
+
+        Set<String> autoCreateIndices = new HashSet<>(Set.of("logs-foobar", "logs-barbaz"));


Would this variable be better named autoCreateDataStreams?

henningandersen

Thanks @martijnvg. I added a few comments that I believe need to be addressed.

henningandersen · 2020-04-23T09:52:58Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

        }
    }

+    static Map<String, IndexTemplateV2.DataStreamTemplate> resolveAutoCreateDataStreams(Metadata metadata,


Unfortunately, resolving this here on the coordinator is slightly trappy in that the coordinator could be disconnected while the request could still succeed. It may thus create an index if it does not see the template.

I think we could (and should) repair this by checking when creating indices if the template is for a data stream and if so, reject the request. But I also think it would be better to do the switching on master instead by adding a new action that handles the auto-creation of indices/streams. This would also be a good hook for security to validate auto_create privilege against anyway. I also think it could simplify the code in this class a fair bit.

That is trappy indeed, thanks for spotting this. I will update the PR, so that most of the auto create logic here is moved to the to be built auto create action.

henningandersen · 2020-04-23T10:23:59Z

server/src/main/java/org/elasticsearch/action/bulk/TransportBulkAction.java

+        Iterator<String> autoCreateIndicesIterator = autoCreateIndices.iterator();
+        while (autoCreateIndicesIterator.hasNext()) {
+            String indexName = autoCreateIndicesIterator.next();
+            String v2Template = MetadataIndexTemplateService.findV2Template(metadata, indexName, false);


I think this means that if you have both v1 and v2 templates matching the same pattern and the v2 one is a data stream, it takes precedence even when prefer_v2_templates=false. I think this will cause some confusion and I would prefer to stick to respecting the option.

…ansportAction to MetadataCreateDataStreamService class. This new class can then also be used from the to be created auto create transport action.

For index and bulk requests that refer to indices/data streams that don't exist yet, the TransportBulkAction will redirect to the auto create action which will either create an index or data stream. Previously this decision was made in the TransportBulkAction, on the coordinating node, but if that node is currently unaware of the existence of specific data streams it will create an index instead of a data stream.

Currently the TransportBulkAction detects whether an index is missing and then decides whether it should be auto created. The coordination of the index creation also happens in the TransportBulkAction on the coordinating node. This change adds a new transport action that the TransportBulkAction delegates to if missing indices need to be created. The reasons for this change: * Auto creation of data streams can't occur on the coordinating node. Based on the index template (v2) either a regular index or a data stream should be created. However if the coordinating node is unaware of certain index templates then the TransportBulkAction could create an index instead of a data stream. Therefor the decision of whether an index or data stream should be created should happen on the master node. See elastic#55377 * From a security perspective it is useful to know whether index creation originates from the create index api or from auto creating a new index via the bulk or index api. For example a user would be allowed to auto create an index, but not to use the create index api. The auto create action will allow security to distinguish these two different patterns of index creation.

martijnvg · 2020-04-28T13:36:37Z

After adding the auto create action, the size of the PR increased too much to be able to do a careful review. Also the PRs making two different changes. So I've split the auto create action in a new pr (#55858). I will update this pr after that PR gets merged.

Currently the TransportBulkAction detects whether an index is missing and then decides whether it should be auto created. The coordination of the index creation also happens in the TransportBulkAction on the coordinating node. This change adds a new transport action that the TransportBulkAction delegates to if missing indices need to be created. The reasons for this change: * Auto creation of data streams can't occur on the coordinating node. Based on the index template (v2) either a regular index or a data stream should be created. However if the coordinating node is slow in processing cluster state updates then it may be unaware of the existence of certain index templates, which then can load to the TransportBulkAction creating an index instead of a data stream. Therefor the coordination of creating an index or data stream should occur on the master node. See #55377 * From a security perspective it is useful to know whether index creation originates from the create index api or from auto creating a new index via the bulk or index api. For example a user would be allowed to auto create an index, but not to use the create index api. The auto create action will allow security to distinguish these two different patterns of index creation. This change adds the following new transport actions: AutoCreateAction, the TransportBulkAction redirects to this action and this action will actually create the index (instead of the TransportCreateIndexAction). Later via #55377, can improve the AutoCreateAction to also determine whether an index or data stream should be created. The create_index index privilege is also modified, so that if this permission is granted then a user is also allowed to auto create indices. This change does not yet add an auto_create index privilege. A future change can introduce this new index privilege or modify an existing index / write index privilege. Relates to #53100

Backport of elastic#55858 to 7.x branch. Currently the TransportBulkAction detects whether an index is missing and then decides whether it should be auto created. The coordination of the index creation also happens in the TransportBulkAction on the coordinating node. This change adds a new transport action that the TransportBulkAction delegates to if missing indices need to be created. The reasons for this change: * Auto creation of data streams can't occur on the coordinating node. Based on the index template (v2) either a regular index or a data stream should be created. However if the coordinating node is slow in processing cluster state updates then it may be unaware of the existence of certain index templates, which then can load to the TransportBulkAction creating an index instead of a data stream. Therefor the coordination of creating an index or data stream should occur on the master node. See elastic#55377 * From a security perspective it is useful to know whether index creation originates from the create index api or from auto creating a new index via the bulk or index api. For example a user would be allowed to auto create an index, but not to use the create index api. The auto create action will allow security to distinguish these two different patterns of index creation. This change adds the following new transport actions: AutoCreateAction, the TransportBulkAction redirects to this action and this action will actually create the index (instead of the TransportCreateIndexAction). Later via elastic#55377, can improve the AutoCreateAction to also determine whether an index or data stream should be created. The create_index index privilege is also modified, so that if this permission is granted then a user is also allowed to auto create indices. This change does not yet add an auto_create index privilege. A future change can introduce this new index privilege or modify an existing index / write index privilege. Relates to elastic#53100

…ed in the same update that creates the index or data stream.

when creating data streams via api

henningandersen

LGTM.

henningandersen · 2020-05-11T09:14:29Z

server/src/internalClusterTest/java/org/elasticsearch/action/bulk/BulkIntegrationIT.java

+
+        Boolean preferV2Templates = randomBoolean() ? null : true;
+        if (randomBoolean()) {
+            PutIndexTemplateRequest v1Request = new PutIndexTemplateRequest("logs-foo");


This probably works in master, but will break in 7.x. I think we need to add preferV2Templates=true in this block? OK to just do this in backport (or before, up to you).

I will remove this randomization now that the prefer flag has been removed.

henningandersen · 2020-05-11T09:56:17Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java

+        return createDataStream(metadataCreateIndexService, current, request);
+    }
+
+    public static final class CreateDataSteamClusterStateUpdateRequest extends ClusterStateUpdateRequest {


The class name is missing an 'r', should be CreateDataStreamClusterStateUpdateRequest

henningandersen · 2020-05-11T13:33:31Z

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamService.java

+            response -> {
+                if (response.isAcknowledged()) {
+                    String firstBackingIndexName = firstBackingIndexRef.get();
+                    assert finalListener != null;


typo?

Suggested change

assert finalListener != null;

assert firstBackingIndexName != null;

This commit adds the ability to auto create data streams using index templates v2. Index templates (v2) now have a data_steam field that includes a timestamp field, if provided and index name matches with that template then a data stream (plus first backing index) is auto created. Relates to elastic#53100

Backport: #55377 This commit adds the ability to auto create data streams using index templates v2. Index templates (v2) now have a data_steam field that includes a timestamp field, if provided and index name matches with that template then a data stream (plus first backing index) is auto created. Relates to #53100

…egration Relates to elastic#55377

…egration (#56615) Relates to #55377

martijnvg added the :Data Management/Data streams Data streams and their lifecycles label Apr 17, 2020

martijnvg added 6 commits April 17, 2020 12:32

fixed hlrc test compile errors

372980b

do not serialize data stream template to nodes that don't yet know ho…

22c3dc3

…w to de-serialize it

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

f476a21

fixed issues after merge

44de34b

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

d50d664

added tests

b01275b

martijnvg added >non-issue v7.8.0 v8.0.0 labels Apr 22, 2020

rename

25fb8bb

martijnvg marked this pull request as ready for review April 22, 2020 12:40

martijnvg requested review from danhermann and henningandersen April 22, 2020 12:40

fixed test compile errors and

ec137e2

removed unused imports

danhermann approved these changes Apr 22, 2020

View reviewed changes

henningandersen requested changes Apr 23, 2020

View reviewed changes

martijnvg added 4 commits April 25, 2020 12:25

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

2d59ab0

fixed compile errors after merge

6b9f062

extracted the create data stream logic from CreateDataStreamAction.Tr…

baef71f

…ansportAction to MetadataCreateDataStreamService class. This new class can then also be used from the to be created auto create transport action.

martijnvg marked this pull request as draft April 28, 2020 11:40

martijnvg mentioned this pull request Apr 28, 2020

Add auto create action #55858

Merged

martijnvg mentioned this pull request May 4, 2020

Add auto create action #56122

Merged

martijnvg added 5 commits May 6, 2020 11:47

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

dc8de3b

applied smaller feedback items

f013f9e

overload constructor

5ed50fe

Perform the check whether a data stream or index should be auto creat…

5df8079

…ed in the same update that creates the index or data stream.

wait for active shards when auto creating data streams / indices and

ddf2081

when creating data streams via api

martijnvg requested a review from henningandersen May 6, 2020 14:19

martijnvg added 3 commits May 6, 2020 16:25

undo style changes

bb38191

undo other style changes

0fa319a

undo other style changes

7aad010

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

395587c

henningandersen approved these changes May 11, 2020

View reviewed changes

martijnvg added 7 commits May 12, 2020 10:08

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

a2b1d0d

iter

7bdb717

fixed test failure

ff41e0e

adjust serialization logic

c7d6f74

Merge remote-tracking branch 'es/master' into auto_create_ds_with_itv2

fc82c3a

fixed checkstyle

7957919

fixed serialization order

a013a29

martijnvg merged commit 74e2c01 into elastic:master May 12, 2020

martijnvg added the backport pending label May 12, 2020

martijnvg mentioned this pull request May 12, 2020

Backport: auto create data streams using index templates v2 #56596

Merged

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request May 12, 2020

Enable bwc tests after backporting index templates v2 data stream int…

a156081

…egration Relates to elastic#55377

martijnvg mentioned this pull request May 12, 2020

Enable bwc tests after backporting index templates v2 data stream integration #56615

Merged

martijnvg added a commit that referenced this pull request May 12, 2020

Enable bwc tests after backporting index templates v2 data stream int…

c408238

…egration (#56615) Relates to #55377

martijnvg removed the backport pending label May 12, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	assert finalListener != null;
	assert firstBackingIndexName != null;

Auto create data streams using index templates v2 #55377

Auto create data streams using index templates v2 #55377

Uh oh!

Conversation

martijnvg commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Apr 17, 2020

Uh oh!

danhermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Apr 28, 2020

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

martijnvg commented Apr 17, 2020 •

edited

Loading