Add infrastructure for managing system indices #65604

pugnascotia · 2020-11-30T13:42:11Z

Part of #61656.

Add the necessary support for automatically creating and updating system
indices. This works by making it possible to create a system index
descriptor with all the information needed to manage the mappings,
settings and aliases.

Follow-up work will opt existing indices into this framework.

Part of elastic#61656. Add the necessary support for automatically creating and updating system indices. This works by making it possible to create a system index descriptor with all the information needed to manage the mappings, settings and aliases. Follow-up work will opt existing indices into this framework.

elasticmachine · 2020-11-30T13:42:14Z

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

jaymode

I took a brief look through the PR and left a few comments.

jaymode · 2020-11-30T19:08:54Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/AutoCreateAction.java

+
+                    final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(indexName);
+
+                    if (descriptor != null && descriptor.isAutomaticallyManaged()) {


Can we simplify this method a bit? We don't really share logic between non-system index creation and system index creation as far as I can see so I think we can test for the non system case and simply return the update request there?

jaymode · 2020-11-30T19:11:27Z

.../src/main/java/org/elasticsearch/action/admin/indices/create/TransportCreateIndexAction.java

                .waitForActiveShards(request.waitForActiveShards());

+        if (isSystemIndex) {


Similar to the comment in AutoCreateAction, can we break up the system and non-system case since we don't really need to share anything between the two cases?

server/src/main/java/org/elasticsearch/indices/SystemIndexManager.java

- Separate logic for building create index requests into sytem-index and non-system-index versions - Use an AtomicBoolean to prevent updates to system index mappings overloading the master

…-system-indices-infra

pugnascotia · 2020-12-02T12:47:12Z

Note to reviewers: I'm holding off creating follow-up PRs to port over existing systems indices until this is merged, so I'd appreciate any and all feedback. In particular, some guidance would be good for setting up a BWC test that exercises the mappings upgrade logic in SystemIndexManager.

jaymode

I left some more feedback. I think this is close. Regarding testing overall, I think it might be hard without an index that uses this framework. So I think once we get this PR in and migrate one to use this then we can add tests that attempt to put mappings or settings that would trigger a violation.

jaymode · 2020-12-02T16:16:49Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/AutoCreateAction.java

+                        updateRequest.aliases(Set.of(new Alias(aliasName)));
+                    }
+
+                    logger.info("Auto-creating system index {}", concreteIndexName);


I think we should make this a debug log like the regular message

jaymode · 2020-12-02T16:21:32Z

.../java/org/elasticsearch/action/admin/indices/settings/put/TransportUpdateSettingsAction.java

+                    .stream()
+                    .map(entry -> "[" + entry.getKey() + "] -> " + entry.getValue())
+                    .collect(Collectors.joining(", "));
+            logger.warn(message);


On backport, we will need to make this a deprecation warning

Can I clarify - in the backport, should this raise a deprecation warning but not fail the request?

Correct on backport this should result in a deprecation warning and still allow the request to go through.

jaymode · 2020-12-02T16:21:48Z

.../main/java/org/elasticsearch/action/admin/indices/mapping/put/TransportPutMappingAction.java

+                    + violations
+                    + ": system indices can only use mappings from their descriptors, "
+                    + "but the mappings in the request did not match those in the descriptors(s)";
+                logger.warn(message);


On backport, we will need to make this a deprecation warning

server/src/main/java/org/elasticsearch/common/Strings.java

server/src/main/java/org/elasticsearch/indices/SystemIndexDescriptor.java

jaymode · 2020-12-02T16:27:43Z

server/src/main/java/org/elasticsearch/indices/SystemIndexDescriptor.java

+        public SystemIndexDescriptor build() {
+            String mappings = mappingsBuilder == null ? null : Strings.toString(mappingsBuilder);
+
+            Strings.requireNonEmpty(indexPattern, "indexPattern must be supplied");


Can all validation be moved into the constructor?

server/src/main/java/org/elasticsearch/indices/SystemIndexManager.java

jaymode · 2020-12-02T16:37:42Z

server/src/main/java/org/elasticsearch/indices/SystemIndexManager.java

+        originSettingClient.admin().indices().putMapping(request, new ActionListener<>() {
+            @Override
+            public void onResponse(AcknowledgedResponse acknowledgedResponse) {
+                isUpgradeInProgress.set(false);


this sets to false early. There can still be in flight items and isUpgradeInProgress would be incorrect. You may want to use something like a GroupedActionListener

I'm not sure I follow. The upgradeIndexMetadata method is called once for each index that needs upgraded, and the first thing the method does is attempt to set isUpgradeInProgress to true. So only one index will be upgraded, and the rest need to wait until isUpgradeInProgress becomes false again. I wrote it that way so as to be cautious about adding load to the master.

Were you thinking that we'd dispatch all the mapping updates in one go, and use a GroupedActionListener to release the isUpgradeInProgress once they'd all finished?

I'm not sure I follow.

That's because I did not follow the code properly. I think it would be best to fire off all of the upgrades at once rather than sequentially since IIRC the code is computing everything that needs upgraded each time.

Were you thinking that we'd dispatch all the mapping updates in one go, and use a GroupedActionListener to release the isUpgradeInProgress once they'd all finished?

Yes that is what I was thinking. I wanted to avoid resending each one every time there was a cluster state update if other cluster state updates happened prior to the mapping updates that we had previously sent.

I've reworked the locking and upgrading to use a GroupedActionListener.

…-system-indices-infra

Run all upgrades at once, and use a GroupedActionListener to release the lock once all have completed.

jaymode

Left one comment. Otherwise LGTM. Thanks for the iterations!

jaymode · 2020-12-03T15:20:59Z

server/src/main/java/org/elasticsearch/indices/SystemIndexManager.java

+                );
+
+                descriptors.forEach(descriptor -> upgradeIndexMetadata(descriptor, listener));
+            }


Suggested change

}

} else {

isUpgradeInProgress.set(false);

}

Good catch! 😅

…-system-indices-infra

Part of elastic#61656. Add the necessary support for automatically creating and updating system indices. This works by making it possible to create a system index descriptor with all the information needed to manage the mappings, settings and aliases. Follow-up work will opt existing indices into this framework.

Backport of #65604. Part of #61656. Add the necessary support for automatically creating and updating system indices. This works by making it possible to create a system index descriptor with all the information needed to manage the mappings, settings and aliases. Follow-up work will opt existing indices into this framework.

pugnascotia added >enhancement :Core/Infra/Core Core issues without another label v8.0.0 v7.11.0 labels Nov 30, 2020

pugnascotia requested review from gwbrown and jaymode November 30, 2020 13:42

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Nov 30, 2020

pugnascotia mentioned this pull request Nov 30, 2020

Create system indices on-demand #64533

Closed

Checkstyle

f6382f4

jaymode requested changes Nov 30, 2020

View reviewed changes

pugnascotia added 2 commits December 1, 2020 13:33

Address review feedback

ac8888b

- Separate logic for building create index requests into sytem-index and non-system-index versions - Use an AtomicBoolean to prevent updates to system index mappings overloading the master

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

3fb68a4

…-system-indices-infra

pugnascotia requested a review from jaymode December 2, 2020 12:34

jaymode requested changes Dec 2, 2020

View reviewed changes

pugnascotia added 3 commits December 2, 2020 21:16

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

fc1e958

…-system-indices-infra

Review feedback

8adb25b

Apply all mappings updates at once

37bfbee

Run all upgrades at once, and use a GroupedActionListener to release the lock once all have completed.

pugnascotia requested a review from jaymode December 3, 2020 14:09

jaymode approved these changes Dec 3, 2020

View reviewed changes

pugnascotia added 2 commits December 3, 2020 15:35

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

751a895

…-system-indices-infra

Remember to release the lock

77d3163

pugnascotia merged commit cbd5d12 into elastic:master Dec 4, 2020

pugnascotia deleted the 61656-auto-create-system-indices-infra branch December 4, 2020 09:24

pugnascotia mentioned this pull request Dec 7, 2020

Add infrastructure for managing system indices #65970

Merged

gwbrown mentioned this pull request Dec 7, 2020

Introduce the notion of system composable index templates #65957

Open

This was referenced Jan 7, 2021

PUT #67146

Closed

PUT /_settings causes NullPointerException if a system index is present #67147

Closed

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

Add infrastructure for managing system indices #65604

Add infrastructure for managing system indices #65604

pugnascotia commented Nov 30, 2020

elasticmachine commented Nov 30, 2020

jaymode left a comment

jaymode Nov 30, 2020

jaymode Nov 30, 2020

pugnascotia commented Dec 2, 2020

jaymode left a comment

jaymode Dec 2, 2020

jaymode Dec 2, 2020

pugnascotia Dec 2, 2020

jaymode Dec 2, 2020

jaymode Dec 2, 2020

jaymode Dec 2, 2020

jaymode Dec 2, 2020

pugnascotia Dec 2, 2020

jaymode Dec 2, 2020

pugnascotia Dec 3, 2020

jaymode left a comment

jaymode Dec 3, 2020

pugnascotia Dec 3, 2020


		final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(indexName);

		if (descriptor != null && descriptor.isAutomaticallyManaged()) {

		.waitForActiveShards(request.waitForActiveShards());

		if (isSystemIndex) {

Add infrastructure for managing system indices #65604

Add infrastructure for managing system indices #65604

Conversation

pugnascotia commented Nov 30, 2020

elasticmachine commented Nov 30, 2020

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented Dec 2, 2020

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment