Skip to content

HDDS-12608. Race condition in datanode version file creation #8093

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

Merged
adoroszlai merged 10 commits into from
Mar 17, 2025
Merged

HDDS-12608. Race condition in datanode version file creation #8093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
251b16a
HDDS-12608. Race condition in datanode version file creation
adoroszlai Mar 15, 2025
98699e3
Revert "HDDS-12608. Race condition in datanode version file creation"
adoroszlai Mar 17, 2025
4c9a1b6
HDDS-12608. Race condition in datanode version file creation
adoroszlai Mar 17, 2025
6c892f2
fix checkstyle
adoroszlai Mar 17, 2025
9d7cf17
fix findbugs
adoroszlai Mar 17, 2025
1a92ae2
Revert "fix findbugs"
adoroszlai Mar 17, 2025
db6dc2e
Revert "fix checkstyle"
adoroszlai Mar 17, 2025
cc8aeeb
Revert "HDDS-12608. Race condition in datanode version file creation"
adoroszlai Mar 17, 2025
db0b130
HDDS-12608. Race condition in datanode version file creation
adoroszlai Mar 17, 2025
381e0ed
fix OzoneContainer integration tests
adoroszlai Mar 17, 2025
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
6 changes: 0 additions & 6 deletions ...in/java/org/apache/hadoop/ozone/container/common/states/endpoint/VersionEndpointTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.apache.hadoop.hdds.conf.ConfigurationSource;
import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.SCMVersionResponseProto;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.container.common.DatanodeLayoutStorage;
import org.apache.hadoop.ozone.container.common.statemachine.EndpointStateMachine;
import org.apache.hadoop.ozone.container.common.utils.StorageVolumeUtil;
import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet;
Expand Down Expand Up @@ -88,11 +87,6 @@ public EndpointStateMachine.EndPointStates call() throws Exception {
// Check HddsVolumes
checkVolumeSet(ozoneContainer.getVolumeSet(), scmId, clusterId);

DatanodeLayoutStorage layoutStorage
= new DatanodeLayoutStorage(configuration);
layoutStorage.setClusterId(clusterId);
layoutStorage.persistCurrentState();

Comment on lines -91 to -95
Copy link
Contributor

@szetszwo szetszwo Mar 17, 2025

Choose a reason for hiding this comment

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

Good catch on the bug! There are multiple SCM endpoints so the persistCurrentState() is called multiple times for the same file.

// Start the container services after getting the version information
ozoneContainer.start(clusterId);
}
Expand Down
6 changes: 6 additions & 0 deletions ...ner-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.apache.hadoop.hdds.utils.db.Table;
import org.apache.hadoop.hdds.utils.db.TableIterator;
import org.apache.hadoop.ozone.HddsDatanodeService;
import org.apache.hadoop.ozone.container.common.DatanodeLayoutStorage;
import org.apache.hadoop.ozone.container.common.helpers.ContainerMetrics;
import org.apache.hadoop.ozone.container.common.impl.BlockDeletingService;
import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
Expand Down Expand Up @@ -484,6 +485,11 @@ public void start(String clusterId) throws IOException {
return;
}

DatanodeLayoutStorage layoutStorage
= new DatanodeLayoutStorage(config);
layoutStorage.setClusterId(clusterId);
layoutStorage.persistCurrentState();
Comment on lines +488 to +491
Copy link
Contributor

@szetszwo szetszwo Mar 17, 2025

Choose a reason for hiding this comment

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

The VERSION file will be overwritten for each restart. Unfortunately, it is the current behavior.

The correct behavior is that the VERSION file should

  • be created when storage is formatted, and
  • be overwritten when there is a layout change.

For the other cases, the VERSION file should not be changed, i.e. no overwriting the file is needed.

Let change it separately.


buildContainerSet();

// Start background volume checks, which will begin after the configured
Expand Down
11 changes: 11 additions & 0 deletions ...er-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package org.apache.hadoop.ozone.container.common;

import static org.apache.hadoop.ozone.common.Storage.StorageState.INITIALIZED;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -46,6 +47,7 @@
import org.apache.hadoop.ipc.ProtobufRpcEngine;
import org.apache.hadoop.ipc.RPC;
import org.apache.hadoop.net.NetUtils;
import org.apache.hadoop.ozone.HddsDatanodeService;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.container.ContainerTestHelper;
import org.apache.hadoop.ozone.container.common.impl.ContainerData;
Expand Down Expand Up @@ -350,4 +352,13 @@ public static XceiverServerRatis newXceiverServerRatis(
getNoopContainerDispatcher(), getEmptyContainerController(),
null, null);
}

/** Initialize {@link DatanodeLayoutStorage}. Normally this is done during {@link HddsDatanodeService} start,
* have to do the same for tests that create {@link OzoneContainer} manually. */
public static void initializeDatanodeLayout(ConfigurationSource conf, DatanodeDetails dn) throws IOException {
DatanodeLayoutStorage layoutStorage = new DatanodeLayoutStorage(conf, dn.getUuidString());
if (layoutStorage.getState() != INITIALIZED) {
layoutStorage.initialize();
}
}
}
8 changes: 8 additions & 0 deletions ...-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import org.apache.hadoop.ipc.RPC;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.common.Storage.StorageState;
import org.apache.hadoop.ozone.container.common.statemachine.DatanodeStateMachine;
import org.apache.hadoop.ozone.container.common.statemachine.EndpointStateMachine;
import org.apache.hadoop.ozone.container.common.statemachine.StateContext;
Expand Down Expand Up @@ -310,6 +311,13 @@ public void testDnLayoutVersionFile() throws Exception {

assertEquals("different_cluster_id", layout1.getClusterID());
assertNotEquals(scmServerImpl.getClusterId(), layout1.getClusterID());

// another call() with OzoneContainer already started should not write the file
FileUtils.forceDelete(layout1.getVersionFile());
rpcEndPoint.setState(EndpointStateMachine.EndPointStates.GETVERSION);
versionTask.call();
assertEquals(StorageState.NOT_INITIALIZED, new DatanodeLayoutStorage(ozoneConf, "any").getState());

FileUtils.forceDelete(storageDir);
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...on-test/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public void testCreateOzoneContainer(
.getOzoneContainer(datanodeDetails, conf);
StorageVolumeUtil.getHddsVolumesList(container.getVolumeSet().getVolumesList())
.forEach(hddsVolume -> hddsVolume.setDbParentDir(tempDir.toFile()));
ContainerTestUtils.initializeDatanodeLayout(conf, datanodeDetails);
//Set clusterId and manually start ozone container.
container.start(UUID.randomUUID().toString());

Expand Down Expand Up @@ -112,6 +113,7 @@ void testOzoneContainerStart(
DatanodeDetails datanodeDetails = randomDatanodeDetails();
container = ContainerTestUtils
.getOzoneContainer(datanodeDetails, conf);
ContainerTestUtils.initializeDatanodeLayout(conf, datanodeDetails);

String clusterId = UUID.randomUUID().toString();
container.start(clusterId);
Expand Down
1 change: 1 addition & 0 deletions .../src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainerWithTLS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ private OzoneContainer createAndStartOzoneContainerInstance() {
MutableVolumeSet volumeSet = container.getVolumeSet();
StorageVolumeUtil.getHddsVolumesList(volumeSet.getVolumesList())
.forEach(hddsVolume -> hddsVolume.setDbParentDir(tempFolder.toFile()));
ContainerTestUtils.initializeDatanodeLayout(conf, dn);
container.start(clusterID);
} catch (Throwable e) {
if (container != null) {
Expand Down
1 change: 1 addition & 0 deletions ...t/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestSecureOzoneContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ void testCreateOzoneContainer(boolean requireToken, boolean hasToken,
MutableVolumeSet volumeSet = container.getVolumeSet();
StorageVolumeUtil.getHddsVolumesList(volumeSet.getVolumesList())
.forEach(hddsVolume -> hddsVolume.setDbParentDir(tempFolder.toFile()));
ContainerTestUtils.initializeDatanodeLayout(conf, dn);
//Set scmId and manually start ozone container.
container.start(UUID.randomUUID().toString());

Expand Down