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

Prevent downgrades from 8.x to 7.x #78586

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.elasticsearch.core.PathUtils;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.gateway.PersistedClusterStateService;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.test.ESIntegTestCase;
Expand Down Expand Up @@ -138,8 +139,11 @@ public void testUpgradeDataFolder() throws IOException, InterruptedException {
// simulate older data path layout by moving data under "nodes/0" folder
final List<Path> dataPaths = List.of(PathUtils.get(Environment.PATH_DATA_SETTING.get(dataPathSettings)));
dataPaths.forEach(path -> {
final Path targetPath = path.resolve("nodes").resolve("0");
final Path nodesPath = path.resolve("nodes");
final Path targetPath = nodesPath.resolve("0");
try {
assertTrue(Files.isRegularFile(nodesPath));
Files.delete(nodesPath);
Files.createDirectories(targetPath);

try (DirectoryStream<Path> stream = Files.newDirectoryStream(path)) {
Expand Down Expand Up @@ -192,9 +196,9 @@ public void testUpgradeDataFolder() throws IOException, InterruptedException {
}

// check that upgrade works
dataPaths.forEach(path -> assertTrue(Files.exists(path.resolve("nodes"))));
dataPaths.forEach(path -> assertTrue(Files.isDirectory(path.resolve("nodes"))));
internalCluster().startNode(dataPathSettings);
dataPaths.forEach(path -> assertFalse(Files.exists(path.resolve("nodes"))));
dataPaths.forEach(path -> assertTrue(Files.isRegularFile(path.resolve("nodes"))));
assertEquals(nodeId, client().admin().cluster().prepareState().get().getState().nodes().getMasterNodeId());
assertTrue(indexExists("test"));
ensureYellow("test");
Expand Down
35 changes: 19 additions & 16 deletions server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,12 @@
import java.io.Closeable;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.DirectoryStream;
import java.nio.file.FileStore;
import java.nio.file.Files;
import java.nio.file.OpenOption;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.ArrayList;
Expand Down Expand Up @@ -269,6 +271,16 @@ public NodeEnvironment(Settings settings, Environment environment) throws IOExce
assertCanWrite();
}

// versions 7.x and earlier put their data under ${path.data}/nodes/; leave a file at that location to prevent downgrades
final Path legacyNodesPath = environment.dataFile().resolve("nodes");
if (Files.isRegularFile(legacyNodesPath) == false) {
final String content = "written by Elasticsearch v" + Version.CURRENT +
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should follow-up with a 7.16 change to log the first 256 bytes of the file if nodes is a file and not a directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heh yeah I suppose we could at least help 7.16 users here :)

" to prevent a downgrade to a version prior to v8.0.0 which would result in data loss";
Files.write(legacyNodesPath, content.getBytes(StandardCharsets.UTF_8));
IOUtils.fsync(legacyNodesPath, false);
IOUtils.fsync(environment.dataFile(), true);
}

if (DiscoveryNode.canContainData(settings) == false) {
if (DiscoveryNode.isMasterNode(settings) == false) {
ensureNoIndexMetadata(nodePath);
Expand Down Expand Up @@ -347,7 +359,6 @@ private static boolean upgradeLegacyNodeFolders(Logger logger, Settings settings

// move contents from legacy path to new path
try {
final List<CheckedRunnable<IOException>> upgradeActions = new ArrayList<>();
final NodePath legacyNodePath = legacyNodeLock.getNodePath();
final NodePath nodePath = nodeLock.getNodePath();

Expand Down Expand Up @@ -398,22 +409,14 @@ private static boolean upgradeLegacyNodeFolders(Logger logger, Settings settings
assert Sets.difference(folderNames, expectedFolderNames).isEmpty() :
"expected indices and/or state dir folder but was " + folderNames;

upgradeActions.add(() -> {
for (String folderName : folderNames) {
final Path sourceSubFolderPath = legacyNodePath.path.resolve(folderName);
final Path targetSubFolderPath = nodePath.path.resolve(folderName);
Files.move(sourceSubFolderPath, targetSubFolderPath, StandardCopyOption.ATOMIC_MOVE);
logger.info("data folder upgrade: moved from [{}] to [{}]", sourceSubFolderPath, targetSubFolderPath);
}
IOUtils.fsync(nodePath.path, true);
});

// now do the actual upgrade. start by upgrading the node metadata file before moving anything, since a downgrade in an
// intermediate state would be pretty disastrous
loadNodeMetadata(settings, logger, legacyNodeLock.getNodePath());
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 doesn't actually do anything any more, the node metadata is in the persisted cluster state which doesn't get written until much later.

for (CheckedRunnable<IOException> upgradeAction : upgradeActions) {
upgradeAction.run();
// now do the actual upgrade
for (String folderName : folderNames) {
final Path sourceSubFolderPath = legacyNodePath.path.resolve(folderName);
final Path targetSubFolderPath = nodePath.path.resolve(folderName);
Files.move(sourceSubFolderPath, targetSubFolderPath, StandardCopyOption.ATOMIC_MOVE);
logger.info("data folder upgrade: moved from [{}] to [{}]", sourceSubFolderPath, targetSubFolderPath);
}
IOUtils.fsync(nodePath.path, true);
} finally {
legacyNodeLock.close();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.test.NodeRoles;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
Expand All @@ -41,6 +42,7 @@
import static org.elasticsearch.test.NodeRoles.nonDataNode;
import static org.elasticsearch.test.NodeRoles.nonMasterNode;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.startsWith;
Expand Down Expand Up @@ -439,6 +441,23 @@ public void testEnsureNoShardDataOrIndexMetadata() throws IOException {
verifyFailsOnShardData(noDataNoMasterSettings, indexPath, shardDataDirName);
}

public void testBlocksDowngradeToVersionWithMultipleNodesInDataPath() throws IOException {
final Settings settings = buildEnvSettings(Settings.EMPTY);
for (int i = 0; i < 2; i++) { // ensure the file gets created again if missing
try (NodeEnvironment env = newNodeEnvironment(settings)) {
final Path nodesPath = env.nodeDataPath().resolve("nodes");
assertTrue(Files.isRegularFile(nodesPath));
assertThat(
Files.readString(nodesPath, StandardCharsets.UTF_8),
allOf(
containsString("written by Elasticsearch"),
containsString("prevent a downgrade"),
containsString("data loss")));
Files.delete(nodesPath);
}
}
}

private void verifyFailsOnShardData(Settings settings, Path indexPath, String shardDataDirName) {
IllegalStateException ex = expectThrows(IllegalStateException.class,
"Must fail creating NodeEnvironment on a data path that has shard data if node does not have data role",
Expand All @@ -459,17 +478,6 @@ private void verifyFailsOnMetadata(Settings settings, Path indexPath) {
assertThat(ex.getMessage(), startsWith("node does not have the data and master roles but has index metadata"));
}

/**
* Converts an array of Strings to an array of Paths, adding an additional child if specified
*/
private Path[] stringsToPaths(String[] strings, String additional) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, but unused.

Path[] locations = new Path[strings.length];
for (int i = 0; i < strings.length; i++) {
locations[i] = PathUtils.get(strings[i], additional);
}
return locations;
}

@Override
public NodeEnvironment newNodeEnvironment() throws IOException {
return newNodeEnvironment(Settings.EMPTY);
Expand Down