Migrate watcher to system indices infrastructure by pugnascotia · Pull Request #67588 · elastic/elasticsearch

pugnascotia · 2021-01-15T16:08:37Z

Part of #61656.

Migrate the .watches and .triggered_watches system indices to use the auto-create infrastructure. The watcher history indices are left alone.

As part of this work, a SystemIndexDescriptor now inspects its mappings to determine whether it has any dynamic mappings. This influences how strict Elasticsearch is with enforcing the descriptor's mappings, since ES cannot know in advanced what all the mappings will be.

This PR also fixes the SystemIndexManager so that it doesn't fall over when attempting to inspect the state of an index that hasn't been created yet.

…-watcher

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

…-watcher

elasticmachine · 2021-01-19T14:29:09Z

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

elasticmachine · 2021-01-19T14:29:10Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

jaymode

LGTM

…-watcher

mark-vieira · 2021-02-03T00:16:21Z

@elasticmachine update branch

…-watcher

pugnascotia · 2021-02-09T15:07:15Z

There's something very peculiar going on here. As far as I can tell:

The Watcher asciidoc tests create a new watch. In the request payload, the new watch populates a metadata key with { color: red }, which is legitimate.
Watcher submits an index request for the new watch
This request gets turned into a bulk request.
The bulk request notices that the .watches index doesn't exist, and submits an AutoCreateAction to create it.
The index is created, as far as I can tell with the correct mappings from the system index descriptor
But then a TransportAutoPutMappingAction fails because it tries to apply some incorrect mappings to the .watches system index - and it so happens that these new mappings match the metadata from the create watch payload. There's no reason that I can see why this metadata from the watcher payload should have any bearing on the mappings of the .watches index (but I could be wrong)
As far as I can see, this action is raised as part of the replication code, at which point I'm lost.

jaymode · 2021-02-09T16:28:34Z

But then a TransportAutoPutMappingAction fails because it tries to apply some incorrect mappings to the .watches system index - and it so happens that these new mappings match the metadata from the create watch payload. There's no reason that I can see why this metadata from the watcher payload should have any bearing on the mappings of the .watches index (but I could be wrong)

I think the issue is that watches have dynamic objects within the mapping including metadata among others. So when the watch is stored with metadata then the mappings need to be updated to account for the metadata.color field. I think Security will have the same issue for dynamic fields.

In order to resolve this, I think a change is needed to how mappings are validated. If a mapping contains dynamic fields then any fields contained within that object should be allowed. Make sense?

…-watcher

pugnascotia · 2021-02-16T13:57:21Z

@elasticmachine update branch

martijnvg

From Watcher side LGTM. I did left a question for my own understanding.

martijnvg · 2021-02-18T12:52:14Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

        module.addIndexOperationListener(listener);
    }

-    static void validAutoCreateIndex(Settings settings, Logger logger) {


👍 (never liked this logic here)

martijnvg · 2021-02-18T13:05:11Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java

+                builder.field("dynamic", "strict");
+                {
+                    builder.startObject("_meta");
+                    builder.field("version", Version.CURRENT);


The current template doesn't have a version field under they _meta object.
If I read the code correctly in SystemIndexManager this means that irregardless of that, a mapping update will be triggered?

@martijnvg it turns out that the SystemIndexManager would never have updated the mappings, because without a value for _metadata.version, the manager would always think that the version was the latest (because Version.fromString(null) returns Version.CURRENT). I've pushed a fix for that case.

Thanks for fixing that!

`SystemIndexManager.readMappingVersion` didn't take into account that calling `Version.readString(null)` would return `Version.CURRENT`, which would mean that an index with version metadata would never have its mappings updated. Change the logic to return `Version.EMPTY` in this case, so that the mappings will be updated.

…-watcher

Part of elastic#61656. Migrate the `.watches` and `.triggered_watches` system indices to use the auto-create infrastructure. The watcher history indices are left alone. As part of this work, a `SystemIndexDescriptor` now inspects its mappings to determine whether it has any dynamic mappings. This influences how strict Elasticsearch is with enforcing the descriptor's mappings, since ES cannot know in advanced what all the mappings will be. This PR also fixes the `SystemIndexManager` so that (1) it doesn't fall over when attempting to inspect the state of an index that hasn't been created yet, and (2) does update mappings if there's no version field in the mapping metadata.

Backport of #67588. Part of #61656. Migrate the `.watches` and `.triggered_watches` system indices to use the auto-create infrastructure. The watcher history indices are left alone. As part of this work, a `SystemIndexDescriptor` now inspects its mappings to determine whether it has any dynamic mappings. This influences how strict Elasticsearch is with enforcing the descriptor's mappings, since ES cannot know in advanced what all the mappings will be. This PR also fixes the `SystemIndexManager` so that (1) it doesn't fall over when attempting to inspect the state of an index that hasn't been created yet, and (2) does update mappings if there's no version field in the mapping metadata.

Since #67588, .triggered_watches and .watches indices are no longer created on node startup. This PR removing them from the warnings for comparison.

Since elastic#67588, .triggered_watches and .watches indices are no longer created on node startup. This PR removing them from the warnings for comparison.

Since #67588, .triggered_watches and .watches indices are no longer created on node startup. This PR removing them from the warnings for comparison.

We moved watcher to run on the system indices infrastructure in elastic#67588. This commit makes sure Watcher cleans up the legacy templates it used before it was migrated to system indices.

pugnascotia added 2 commits January 13, 2021 13:44

WIP - migrate watcher to system indices infrastructure

2322853

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

01e5df8

…-watcher

pugnascotia added >enhancement :Core/Infra/Core Core issues without another label :Distributed/Watcher v8.0.0 v7.12.0 labels Jan 15, 2021

pugnascotia requested a review from jaymode January 15, 2021 16:08

jaymode reviewed Jan 15, 2021

View reviewed changes

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java Outdated Show resolved Hide resolved

pugnascotia added 2 commits January 19, 2021 14:27

Remove redundant action.auto_create_index check

8544579

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

463ef5e

…-watcher

pugnascotia marked this pull request as ready for review January 19, 2021 14:29

elasticmachine added Team:Data Management (obsolete) DO NOT USE. This team no longer exists. Team:Core/Infra Meta label for core/infra team labels Jan 19, 2021

pugnascotia requested a review from jaymode January 19, 2021 14:29

jaymode approved these changes Jan 28, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

1fb3f00

…-watcher

pugnascotia added 2 commits February 3, 2021 14:41

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

13a0e88

…-watcher

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

22c0535

…-watcher

pugnascotia added 6 commits February 10, 2021 20:31

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

4ffc75b

…-watcher

Check for dynamic fields in a SystemIndexDescriptor's mappings

e06eed2

Merge snafu

a0ed834

Revert whitespace changes

fe7ad62

Remove references to deleted templates

cbeda93

Remove redundant tests

7aed925

pugnascotia added 3 commits February 11, 2021 16:53

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

8867d97

…-watcher

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

c87ad05

…-watcher

Handle non-existent indices in SystemIndexManager

8aaad75

Merge branch 'master' into 61656-auto-create-watcher

4ee4031

martijnvg self-requested a review February 18, 2021 07:31

martijnvg approved these changes Feb 18, 2021

View reviewed changes

williamrandolph added v7.13.0 and removed v7.12.0 labels Feb 18, 2021

pugnascotia added 3 commits February 19, 2021 20:39

Merge commit '0daae1ec2de' into 61656-auto-create-watcher

de502eb

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

ce2d550

…-watcher

pugnascotia force-pushed the 61656-auto-create-watcher branch from 086ebdf to ce2d550 Compare February 19, 2021 20:46

pugnascotia merged commit ae94701 into elastic:master Feb 22, 2021

pugnascotia deleted the 61656-auto-create-watcher branch February 22, 2021 10:28

pugnascotia mentioned this pull request Feb 22, 2021

Migrate watcher to system indices infrastructure (#67588) #69328

Merged

ywangd mentioned this pull request Feb 23, 2021

[Test] Remove some watcher indices from comparison #69497

Merged

ywangd added a commit that referenced this pull request Feb 24, 2021

[Test] Remove some watcher indices from comparison (#69497)

9711975

Since #67588, .triggered_watches and .watches indices are no longer created on node startup. This PR removing them from the warnings for comparison.

ywangd mentioned this pull request Feb 24, 2021

[Test] Remove some watcher indices from comparison (#69497) #69502

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate watcher to system indices infrastructure#67588

Migrate watcher to system indices infrastructure#67588
pugnascotia merged 21 commits intoelastic:masterfrom
pugnascotia:61656-auto-create-watcher

pugnascotia commented Jan 15, 2021 •

edited

Loading

Uh oh!

Uh oh!

elasticmachine commented Jan 19, 2021

Uh oh!

elasticmachine commented Jan 19, 2021

Uh oh!

jaymode left a comment

Uh oh!

mark-vieira commented Feb 3, 2021

Uh oh!

pugnascotia commented Feb 9, 2021 •

edited

Loading

Uh oh!

jaymode commented Feb 9, 2021

Uh oh!

pugnascotia commented Feb 16, 2021

Uh oh!

martijnvg left a comment •

edited

Loading

Uh oh!

martijnvg Feb 18, 2021

Uh oh!

martijnvg Feb 18, 2021

Uh oh!

pugnascotia Feb 19, 2021

Uh oh!

martijnvg Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

pugnascotia commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jan 19, 2021

Uh oh!

elasticmachine commented Jan 19, 2021

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

mark-vieira commented Feb 3, 2021

Uh oh!

pugnascotia commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaymode commented Feb 9, 2021

Uh oh!

pugnascotia commented Feb 16, 2021

Uh oh!

martijnvg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

martijnvg Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

pugnascotia Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

martijnvg Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pugnascotia commented Jan 15, 2021 •

edited

Loading

pugnascotia commented Feb 9, 2021 •

edited

Loading

martijnvg left a comment •

edited

Loading