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.

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

Make data directories work with symlinks again #85878

Merged
merged 13 commits into from
Apr 25, 2022

Conversation

grcevski
Copy link
Contributor

As per #85701, Elasticsearch fails to start if the configured data directory is a symlink. This PR fixes the call to createDirectories to support following through symlinks.

Closes #85701

@grcevski grcevski added >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team auto-backport-and-merge Automatically create backport pull requests and merge when ready v8.0.2 v8.3.0 v8.2.1 labels Apr 13, 2022
@elasticmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@grcevski
Copy link
Contributor Author

I added the backport levels because I assume we want to restore the symlink functionality all the way to 8.0.

@@ -597,6 +597,18 @@ public void testIndexCompatibilityChecks() throws IOException {
}
}

public void testSymlinkDataDirectory() throws Exception {
Path tempDir = createTempDir().toAbsolutePath();
Copy link
Member

@rjernst rjernst Apr 15, 2022

Choose a reason for hiding this comment

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

I suspect this test is going to be enough, since the test environment is not exactly like production. I suggest a packaging test in qa/os. There should be some other symlink related tests there I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll add a test there.

@grcevski grcevski added the test-windows Trigger CI checks on Windows label Apr 18, 2022
Copy link
Contributor

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

LGTM.

@grcevski grcevski added v8.1.4 and removed v8.0.2 labels Apr 19, 2022
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.

LGTM

@@ -701,4 +701,17 @@ public static void verifySecurityNotAutoConfigured(Installation es) throws Excep
}
}

public void updateConfig(Path confPath, String key, String replacement) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

There are some methods in ServerUtils for adding/removing settings. Perhaps this should go there? Or those existing methods could be used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I'll take a look to see if I can reuse something in there.

@grcevski
Copy link
Contributor Author

@elasticmachine update branch

@grcevski
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-2

@grcevski
Copy link
Contributor Author

@elasticmachine update branch

@grcevski grcevski merged commit 189fc68 into elastic:master Apr 25, 2022
@grcevski grcevski deleted the fix/symlink_data branch April 25, 2022 17:40
grcevski added a commit to grcevski/elasticsearch that referenced this pull request Apr 25, 2022
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.2
8.1 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 85878

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport-and-merge Automatically create backport pull requests and merge when ready >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team test-windows Trigger CI checks on Windows v8.1.4 v8.2.1 v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

path.data can no longer be a symlink
5 participants