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

Bug Fix: System Data Streams Should Be Restorable #124651

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Bug Fix: System Data Streams Should Be Restorable #124651

wants to merge 18 commits into from

Conversation

JVerwolf
Copy link
Contributor

@JVerwolf JVerwolf commented Mar 12, 2025 •
edited
Loading

This is still a work in progress.

This PR adds a new MetadataDeleteDataStreamService to delete system data streams prior to a restore operation.

closes #89261

@JVerwolf JVerwolf added >bug :Core/Infra/Core Core issues without another label labels Mar 12, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @JVerwolf, I've created a changelog YAML for you.

JVerwolf
JVerwolf commented Mar 12, 2025
View reviewed changes
server/src/internalClusterTest/java/org/elasticsearch/snapshots/SystemIndicesSnapshotIT.java Outdated
@@ -608,6 +711,69 @@ public void testSystemIndexAliasesAreAlwaysRestored() {

}

// TODO: This test is failing because the assertion `assertTrue(indexExists(systemDataStreamAlias));` trips.
@AwaitsFix(bugUrl = "assertTrue(indexExists(systemDataStreamAlias)); is broken")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is failing because the alias isn't restored. I'm supposing this is a bug, and not expected behaviour for system datastreams?

Copy link
Member

Choose a reason for hiding this comment

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

Data stream aliases are similar to index aliases, where they are metadata on the data stream itself. I would expect restoring a data stream (system or otherwise) to also restore the alias for that data stream.

I tested that with a non-system data stream, the alias is restored when the data stream is restored, so I would treat this as a bug.

rjernst
rjernst reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

A few minor initial comments. I suggest someone from data management review.

@JVerwolf JVerwolf added v9.0.1 v8.19.0 v8.18.1 auto-backport Automatically create backport pull requests when merged labels Mar 12, 2025
dakrone
dakrone reviewed Mar 12, 2025
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

I left a couple of comments from the Data Management side, but nothing major. I agree with Ryan about trying to combine with MetadataDataStreamsService instead of adding a new service if possible.

server/src/internalClusterTest/java/org/elasticsearch/snapshots/SystemIndicesSnapshotIT.java Outdated
@@ -608,6 +711,69 @@ public void testSystemIndexAliasesAreAlwaysRestored() {

}

// TODO: This test is failing because the assertion `assertTrue(indexExists(systemDataStreamAlias));` trips.
@AwaitsFix(bugUrl = "assertTrue(indexExists(systemDataStreamAlias)); is broken")
Copy link
Member

Choose a reason for hiding this comment

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

Data stream aliases are similar to index aliases, where they are metadata on the data stream itself. I would expect restoring a data stream (system or otherwise) to also restore the alias for that data stream.

I tested that with a non-system data stream, the alias is restored when the data stream is restored, so I would treat this as a bug.

@JVerwolf JVerwolf marked this pull request as ready for review March 13, 2025 05:26
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 13, 2025
@elasticsearchmachine
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakrone dakrone dakrone left review comments

@rjernst rjernst rjernst left review comments

@alexey-ivanov-es alexey-ivanov-es Awaiting requested review from alexey-ivanov-es

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.fleet-actions-results data stream cannot be restored via the fleet feature state
5 participants
@JVerwolf @elasticsearchmachine @dakrone @rjernst @alexey-ivanov-es