Converting an Existing Data Stream to a System DataStream is Broken #121392

alexey-ivanov-es · 2025-01-31T13:08:42Z

Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

Closes ES-10526

elasticsearchmachine · 2025-01-31T13:09:07Z

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

elasticsearchmachine · 2025-01-31T13:09:07Z

Hi @alexey-ivanov-es, I've created a changelog YAML for you.

alexey-ivanov-es · 2025-01-31T13:18:47Z

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

At https://github.com/elastic/elasticsearch/pull/121392/files#diff-1123b904a4743231e7321993f73a75ae603c6658b1e6acf6bf8656b69370a410L70 we currently only check for indices that should become system indices. Should we also check for data streams there?

It seems that if a data stream exists, it always has at least one backing index. If the data stream is non-system, the backing index will also be non-system. Then, when the data stream is marked as system, the index will be picked up by the task, and both will be converted to system ones. Does this approach make sense, or should we explicitly check for data streams as well?

Hm, good question. For now that is the case, but we have talked about changing this one day but there are no concrete plans.

If I am not mistaken, this is just looping over one more data set, so I think it would be better to be "thorough" and avoid surprises in the future. What do you think?

we have talked about changing this one day

I'll add check for data streams here as well. Thank you!

JVerwolf

LGTM in general, just a few questions. Should the Data Store team also review this?

JVerwolf · 2025-02-03T22:19:05Z

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

+            List<DataStream> updatedDataStreams = new ArrayList<>();
+            for (DataStream dataStream : currentState.getMetadata().dataStreams().values()) {
+                if (dataStream.isSystem() == false && systemIndices.isSystemDataStream(dataStream.getName())) {
+                    DataStream updatedDataStream = dataStream.copy().setSystem(true).setHidden(true).build();


Curious, why is there a setHidden(true) here? Do you know if there's a reason this isn't set by default when we define a system DataStream? I'm wondering if there's ever a situation where we'd want this to be true, since the updateIndices() function seems to test and set this separately.

When a data stream is initially created as a non-system one, it's likely set as hidden=false. In any case, it should become hidden=true once it is converted to a system data stream. In the system indices upgrade process, this check is done separately, and as I understand it, this is because there is currently support for making system indices non-system again and we don't change hidden in this case. While I'm pretty sure we don't need this functionality, it currently exists for indices.

For data streams, I don’t think we need to support converting system data streams back to non-system ones.

However... Let me check one thing

I checked, and it seems there is only one case where we might need to convert an existing system index or data stream back to a non-system one - when an upgrade to a newer version starts, fails, and the cluster rolls back to the previous version where the index/data stream was non-system.

I'm not 100% sure if this is a valid scenario, but since it is already handled for indices, I've added support for data streams as well.

JVerwolf · 2025-02-03T22:34:14Z

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

+                if (dataStream.isSystem() == false && systemIndices.isSystemDataStream(dataStream.getName())) {
+                    DataStream updatedDataStream = dataStream.copy().setSystem(true).setHidden(true).build();
+
+                    updatedDataStreams.add(updatedDataStream);


It looks like the indices version also updates any aliases to be hidden. Do we need to do that here?

I verified this and data stream aliases become hidden after the data stream is marked as hidden/system. Don't see an easy way to write a test for this, unfortunately

JVerwolf · 2025-02-03T22:39:25Z

...src/test/java/org/elasticsearch/cluster/metadata/SystemIndexMetadataUpgradeServiceTests.java

+            .setSystem(false)
+            .build();
+
+        assertSystemUpgradeAppliesHiddenSettingForDataStream(dataStream, dsIndexMetadata);


This is only used in one place, should this remain separate?

gmarouli

I think it would be better to check data streams as well, for completeness and to avoid any surprises in the future.

I have one more thing I would like to to discuss. If I understand correctly the code, now we check if a index is a system index or a backing index of a system data stream. But to check the backing index of a data stream we check if it starts with the .ds-<data-stream-name> prefix. Right?

This means that a failure index will not be detected as associated with the system data stream, and a backing index that is not following this naming convention will also not be detected.

I understand it's unlikely we will encounter these cases in the wild but it is possible.

I would like to suggest the following: what if we would update only standalone backing indices in updateIndices and then in updateDataStreams we would loop over all the associated indices of a data stream and convert them to a system index and vice versa, if necessary.

docs/changelog/121392.yaml

gmarouli · 2025-02-10T09:58:46Z

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

Hm, good question. For now that is the case, but we have talked about changing this one day but there are no concrete plans.

If I am not mistaken, this is just looping over one more data set, so I think it would be better to be "thorough" and avoid surprises in the future. What do you think?

alexey-ivanov-es · 2025-02-10T14:28:03Z

@gmarouli

If I understand correctly the code, now we check if a index is a system index or a backing index of a system data stream. But to check the backing index of a data stream we check if it starts with the .ds- prefix. Right?

Yes, you're right.

This means that a failure index will not be detected as associated with the system data stream

Good point. I need to verify this, but at first glance, it looks like an oversight in the overall system data streams implementation. This could have implications not just here but also in security (and possibly other areas) as well.

a backing index that is not following this naming convention will also not be detected.

Is it possible? It looks like there are quite a few places in the code outside of DataStream class that rely on this naming format

I would like to suggest the following: what if we would update only standalone backing indices in updateIndices and then in updateDataStreams we would loop over all the associated indices of a data stream and convert them to a system index and vice versa, if necessary.

I'd prefer not to exclude indices backing system data streams from updateIndices. The idea is that all indices that should be system are detected there - if something isn't detected, it likely indicates a bug with broader implications, and excluding them would just hide the issue. Instead, I'd rather add a check in updateDataStreams to ensure that all indices backing a system data stream have already been marked as system. The only thing I'm unsure about is what we should do if they haven’t been

gmarouli · 2025-02-10T15:57:05Z

Is it possible? It looks like there are quite a few places in the code outside of DataStream class that rely on this naming format

Yes it is, a user is able to modify the backing indices of a data stream and add or remove backing indices. It can be considered an expert move but it's possible. Furthermore, there are lifecycle steps that add prefixes to the indices so this can also have the same effect.

I'd prefer not to exclude indices backing system data streams from updateIndices. The idea is that all indices that should be system are detected there - if something isn't detected, it likely indicates a bug with broader implications, and excluding them would just hide the issue

I think this way of thinking is error prone and is the reason we of the bug you are fixing here. What I mean is, the descriptor matches the data stream and not the backing indices themselves that's why you need to rely on a convention to determine this.

While if you consider that the system resource is the data stream, and it's the responsibility of the updateDataStreams code to ensure that the data stream will be a valid system index and all its indices will be too.

This is more inline with how we use data streams in other places of the code as well. Of course system indices is your area of expertise so maybe there something I do not see, I am just explaining how we handle data streams in other areas of the code.

alexey-ivanov-es · 2025-02-10T20:57:04Z

@gmarouli

Yes it is, a user is able to modify the backing indices of a data stream and add or remove backing indices. It can be considered an expert move but it's possible. Furthermore, there are lifecycle steps that add prefixes to the indices so this can also have the same effect.

I think this way of thinking is error prone and is the reason we of the bug you are fixing here. What I mean is, the descriptor matches the data stream and not the backing indices themselves that's why you need to rely on a convention to determine this.

I will make the change here, that's not a problem. The bigger issue is that many other parts of the codebase rely on system data stream backing indices name starting with ".ds-" + dataStreamName + "-" (missing failure indices now) and where, as I know, we don't have access to a DataStream instance to retrieve the backing indices.

Furthermore, there are lifecycle steps that add prefixes to the indices so this can also have the same effect.

Is there a chance we could make an index name pattern that covers all these cases? Of course, it wouldn't work for indices added manually by users, but since users aren't supposed to have access to system indices anyway, it should still work

gmarouli · 2025-02-11T08:40:03Z

The bigger issue is that many other parts of the codebase rely on system data stream backing indices name starting with ".ds-" + dataStreamName + "-" (missing failure indices now) and where, as I know, we don't have access to a DataStream instance to retrieve the backing indices.

I did a very quick code search for where is the constant BACKING_INDEX_PREFIX used and I did not see anything alarming, mainly used in tests which is fine I believe. Do you remember where you have seen this problematic usages?

Is there a chance we could рфму an index name pattern that covers all these cases? Of course, it wouldn't work for indices added manually by users, but since users aren't supposed to have access to system indices anyway, it should still work

What would be the benefit of that? We could potentially cook something but it's more of hack than a solution. I checked (just a quick check) where isSystemIndexBackingDataStream is used and it looks to me that the necessary information is present. Unless you think there is a performance concern which I haven't considered yet.

alexey-ivanov-es · 2025-02-11T15:40:08Z

@gmarouli

I did a very quick code search for where is the constant BACKING_INDEX_PREFIX used and I did not see anything alarming, mainly used in tests which is fine I believe. Do you remember where you have seen this problematic usages?

The issue comes from this method, which is used in multiple places within the system indices logic. Eventually it affects this method (probably easier to fix) and this method used in security plugin (looks harder to fix, but I am new to the team, so might be completely wrong about the complexity)

What would be the benefit of that? We could potentially cook something but it's more of hack than a solution. I checked (just a quick check) where isSystemIndexBackingDataStream is used and it looks to me that the necessary information is present. Unless you think there is a performance concern which I haven't considered yet.

I agree that it's a hack, and I don’t think system indices logic should rely on the naming of indices backing data streams. However, that is how it currently works, and this approach would allow us to implement a quick fix that's at least slightly more reliable than the current method, where the pattern is hardcoded into the system indices logic.

And thank you for your help here.

gmarouli · 2025-02-12T08:29:32Z

The issue comes from this method, which is used in multiple places within the system indices logic. Eventually it affects this method (probably easier to fix) and this method used in security plugin (looks harder to fix, but I am new to the team, so might be completely wrong about the complexity)

I see, we need to do a more thorough check and see what can be done, I did not go that far :)

I agree that it's a hack, and I don’t think system indices logic should rely on the naming of indices backing data streams. However, that is how it currently works, and this approach would allow us to implement a quick fix that's at least slightly more reliable than the current method, where the pattern is hardcoded into the system indices logic.

Considering that we do not have that many system data streams, I think this buys us some time to think this through. Coming up with prefixes is not that easy. For example, I can think of the following prefixes, this list might not be exhaustive:

.fs-<data-stream-name> for failure indices (currently behind a feature flag, so not released yet)
downsample-<interval> for downsampled indices
shrink-<uuid>- for shrunk indices
restored- for fully mounted snapshots
partial- for partial searchable snapshots

The last four can be combined depending on the steps that an index has been through. partial-restored-shrink-xyz-downsample-1h-.ds-my-ds-xxxx-0001. This is why I am trying to avoid including them.

I propose the following:

As you said, in this PR we can user the correct way of determining if a backing index belongs to a system data stream.
We open a github issue, mentioning that we have identified that the determination of a backing system index is a heuristic and not always correct. We refer to this in this methods you listed earlier. This way we make clear in the code that this is not the way to determine this, but we do take this shortcut here that might be good enough for system backing indices.
We discuss and decide how to approach it without blocking this bug fix.

What do you think?

And thank you for your help here.

Happy to, I am getting to know better this use case. Thank you for thinking along with me.

alexey-ivanov-es · 2025-02-12T14:12:49Z

I propose the following:
As you said, in this PR we can user the correct way of determining if a backing index belongs to a system data stream.
We open a github issue, mentioning that we have identified that the determination of a backing system index is a heuristic and not always correct. We refer to this in this methods you listed earlier. This way we make clear in the code that this is not the way to determine this, but we do take this shortcut here that might be good enough for system backing indices.
We discuss and decide how to approach it without blocking this bug fix.
What do you think?

I agree with your proposal. I'll make the changes in this PR and create a ticket to track the issue so we can prioritize and discuss the right approach. Thank you!

alexey-ivanov-es · 2025-02-12T18:57:25Z

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

            // Fork to the management pool to avoid blocking the cluster applier thread unnecessarily for very large index counts
            // TODO: we should have a more efficient way of getting just the changed indices so that we don't have to fork here
            clusterService.threadPool().executor(ThreadPool.Names.MANAGEMENT).execute(new AbstractRunnable() {
                @Override
                protected void doRun() {
-                    if (triggeredVersion != triggerV) {
-                        // don't run if another newer check task was triggered already
-                        return;


As I understand it, this condition could lead to missing indices. If two cluster change events trigger the checks (state1 -> state2 -> state3), there’s a possibility that the first check would return here, and the second check wouldn’t see all indices. This happens because it compares state3 against state2, meaning any differences between state1 and state2 wouldn’t be checked.

…sterStateTaskExecutor and fix tests

alexey-ivanov-es · 2025-02-13T19:39:34Z

Ticket to stop to rely on data streams naming: ES-10870

JVerwolf

I'm not fully familiar with the usage of ClusterStateTaskExecutor, though overall this looks good to me.

I have some questions, though they aren't blocking. I'll leave it to your judgement.

It would be good to get an approval from Data Management prior to merging.

Nice work, this is complex.

JVerwolf · 2025-02-19T20:40:46Z

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

+                if (dataStream.isSystem() != shouldBeSystem) {
+                    DataStream.Builder dataStreamBuilder = dataStream.copy().setSystem(shouldBeSystem);
+                    if (shouldBeSystem) {
+                        dataStreamBuilder.setHidden(true);


I see the updateIndexIfNecessary function updates the versions with this:

builder.settingsVersion(builder.settingsVersion() + 1);

Do we likewise need to update the generation here? (I'm not suggesting we do, rather, wanting to be sure)

Hm, good question, we update generations when the backing or failure indices get modified (added, deleted, etc) but I do not think the update of system property qualifies as such. @dakrone do you agree?

JVerwolf · 2025-02-19T21:25:05Z

...src/test/java/org/elasticsearch/cluster/metadata/SystemIndexMetadataUpgradeServiceTests.java

+        ClusterService clusterService = mock(ClusterService.class);
+        MasterServiceTaskQueue<ClusterStateTaskListener> queue = mock(MasterServiceTaskQueue.class);
+        when(clusterService.createTaskQueue(eq("system-indices-metadata-upgrade"), eq(Priority.NORMAL), any())).thenAnswer(invocation -> {
+            executor = invocation.getArgument(2, ClusterStateTaskExecutor.class);
+            return queue;
+        });
+        doAnswer(invocation -> {
+            task = invocation.getArgument(1, ClusterStateTaskListener.class);
+            return null;
+        }).when(queue).submitTask(any(), any(), any());


I'm rather impartial, though the general consensus here seems to be to avoid mocks whenever possible. Is it possible/practical to avoid them here? If not, no worries.

There are quite a few tests using mocks, so at least I'm not alone :)
I haven't heard of a consensus to avoid mocks, and I think most tests end up not using them because there are often too many dependencies interacting with each other using complex objects, which makes mock-based tests fragile and harder to maintain. That said, if there's a decision to avoid mocks, let me know. Personally, I prefer using mocks where practical rather than exposing internal implementation details.

…lastic#121392) Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

elasticsearchmachine · 2025-02-21T19:52:41Z

💚 Backport successful

Status	Branch	Result
✅	9.0
✅	8.18
✅	8.x

…lastic#121392) Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

…121392) (#123181) Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

…121392) (#123182) Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

…121392) (#123183) Adds support of converting existing data stream to a system data stream as part of existing system_index_metadata_upgrade_service task

Converting an Existing Data Stream to a System DataStream is Broken

6289eb9

alexey-ivanov-es added >bug :Core/Infra/Core Core issues without another label auto-backport Automatically create backport pull requests when merged v9.0.0 v8.18.0 v8.19.0 v9.1.0 labels Jan 31, 2025

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 31, 2025

Update docs/changelog/121392.yaml

5e92c45

[CI] Auto commit changes from spotless

a1b6ceb

alexey-ivanov-es commented Jan 31, 2025

View reviewed changes

JVerwolf reviewed Feb 3, 2025

View reviewed changes

alexey-ivanov-es requested a review from a team February 4, 2025 19:42

alexey-ivanov-es added 2 commits February 7, 2025 17:53

Address review comments

251e88b

Merge branch 'main' into ES-10526

9a4ee2c

alexey-ivanov-es requested a review from JVerwolf February 7, 2025 17:58

gmarouli self-requested a review February 10, 2025 09:32

gmarouli reviewed Feb 10, 2025

View reviewed changes

alexey-ivanov-es and others added 2 commits February 12, 2025 18:28

Change way how indices backing data streams are handled

4ef97b4

[CI] Auto commit changes from spotless

61f9c4f

alexey-ivanov-es commented Feb 12, 2025

View reviewed changes

alexey-ivanov-es and others added 5 commits February 12, 2025 19:03

break -> return

1a7a5d5

Merge branch 'main' into ES-10526

19f9700

Migrate SystemIndexMetadataUpgradeService from unbatched tasks to Clu…

78e8059

…sterStateTaskExecutor and fix tests

[CI] Auto commit changes from spotless

d20b021

Update wording in changelog

ead1cc8

Merge branch 'main' into ES-10526

4993fdc

JVerwolf approved these changes Feb 19, 2025

View reviewed changes

alexey-ivanov-es merged commit 2bda4c1 into elastic:main Feb 21, 2025
17 checks passed

alexey-ivanov-es deleted the ES-10526 branch February 21, 2025 19:51

This was referenced Feb 21, 2025

[9.0] Converting an Existing Data Stream to a System DataStream is Broken (#121392) #123181

Merged

[8.18] Converting an Existing Data Stream to a System DataStream is Broken (#121392) #123182

Merged

alexey-ivanov-es mentioned this pull request Feb 21, 2025

[8.x] Converting an Existing Data Stream to a System DataStream is Broken (#121392) #123183

Merged

Converting an Existing Data Stream to a System DataStream is Broken #121392

Converting an Existing Data Stream to a System DataStream is Broken #121392

Uh oh!

Conversation

alexey-ivanov-es commented Jan 31, 2025

Uh oh!

elasticsearchmachine commented Jan 31, 2025

Uh oh!

elasticsearchmachine commented Jan 31, 2025

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!

JVerwolf 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!

JVerwolf Feb 3, 2025 • 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!

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!

gmarouli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-ivanov-es commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarouli commented Feb 10, 2025

Uh oh!

alexey-ivanov-es commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarouli commented Feb 11, 2025

Uh oh!

alexey-ivanov-es commented Feb 11, 2025

Uh oh!

gmarouli commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-ivanov-es commented Feb 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-ivanov-es commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JVerwolf 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!

Uh oh!

elasticsearchmachine commented Feb 21, 2025

💚 Backport successful

Uh oh!

JVerwolf Feb 3, 2025 •

edited

Loading

alexey-ivanov-es commented Feb 10, 2025 •

edited

Loading

alexey-ivanov-es commented Feb 10, 2025 •

edited

Loading

gmarouli commented Feb 12, 2025 •

edited

Loading

alexey-ivanov-es commented Feb 13, 2025 •

edited

Loading