Skip to content

HDDS-15623. Disk check may not completely read test file #10552

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 3 commits into from
Jun 23, 2026
+20 −6
Merged

HDDS-15623. Disk check may not completely read test file #10552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87e8d48
HDDS-15623. Disk check may not completely read test file
adoroszlai Jun 19, 2026
bb5921f
clean up test file on failure
adoroszlai Jun 19, 2026
f27e18d
reduce file.toPath() calls
adoroszlai Jun 19, 2026
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
16 changes: 13 additions & 3 deletions ...r-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/DiskCheckUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@
import java.io.SyncFailedException;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Random;
import java.util.UUID;
import org.apache.commons.io.IOUtils;
import org.apache.ratis.util.FileUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -141,9 +143,10 @@ public boolean checkPermissions(File storageDir) {
public boolean checkReadWrite(File storageDir,
File testFileDir, int numBytesToWrite) {
File testFile = new File(testFileDir, "disk-check-" + UUID.randomUUID());
Path testPath = testFile.toPath();
byte[] writtenBytes = new byte[numBytesToWrite];
RANDOM.nextBytes(writtenBytes);
try (OutputStream fos = FileUtils.newOutputStreamForceAtClose(testFile, CREATE, TRUNCATE_EXISTING, WRITE)) {
try (OutputStream fos = FileUtils.newOutputStreamForceAtClose(testPath, CREATE, TRUNCATE_EXISTING, WRITE)) {
fos.write(writtenBytes);
} catch (FileNotFoundException | NoSuchFileException notFoundEx) {
logError(storageDir, String.format("Could not find file %s for " +
Expand All @@ -152,35 +155,41 @@ public boolean checkReadWrite(File storageDir,
} catch (SyncFailedException syncEx) {
logError(storageDir, String.format("Could not sync file %s to disk.",
testFile.getAbsolutePath()), syncEx);
FileUtils.deletePathQuietly(testPath);
return false;
} catch (IOException ioEx) {
String msg = ioEx.getMessage();
if (msg != null && msg.contains(LINUX_DISK_FULL_MESSAGE)) {
LOG.warn("Could not write file {} for volume check", testFile.getAbsolutePath(), ioEx);
FileUtils.deletePathQuietly(testPath);
return true;
}
logError(storageDir, String.format("Could not write file %s " +
"for volume check.", testFile.getAbsolutePath()), ioEx);
FileUtils.deletePathQuietly(testPath);
return false;
}

// Read data back from the test file.
byte[] readBytes = new byte[numBytesToWrite];
try (InputStream fis = Files.newInputStream(testFile.toPath())) {
int numBytesRead = fis.read(readBytes);
try (InputStream fis = Files.newInputStream(testPath)) {
int numBytesRead = IOUtils.read(fis, readBytes);
if (numBytesRead != numBytesToWrite) {
logError(storageDir, String.format("%d bytes written to file %s " +
"but %d bytes were read back.", numBytesToWrite,
testFile.getAbsolutePath(), numBytesRead));
FileUtils.deletePathQuietly(testPath);
return false;
}
} catch (FileNotFoundException | NoSuchFileException notFoundEx) {
logError(storageDir, String.format("Could not find file %s " +
"for volume check.", testFile.getAbsolutePath()), notFoundEx);
FileUtils.deletePathQuietly(testPath);

@ashishkumar50 ashishkumar50 Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be better to delete in finally, instead in each catch exception.

@adoroszlai adoroszlai Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The file is deleted as part of the check in normal case:

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/DiskCheckUtil.java

Lines 205 to 213 in f27e18d

// Delete the file.
if (!testFile.delete()) {
logError(storageDir, String.format("Could not delete file %s " +
"for volume check.", testFile.getAbsolutePath()));
return false;
}
// If all checks passed, the volume is healthy.
return true;

So we cannot delete in finally of read check.

@ashishkumar50 ashishkumar50 Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense, in normal scenario delete is verified.

return false;
} catch (IOException ioEx) {
logError(storageDir, String.format("Could not read file %s " +
"for volume check.", testFile.getAbsolutePath()), ioEx);
FileUtils.deletePathQuietly(testPath);
return false;
}

Expand All @@ -189,6 +198,7 @@ public boolean checkReadWrite(File storageDir,
logError(storageDir, String.format("%d Bytes read from file " +
"%s do not match the %d bytes that were written.",
writtenBytes.length, testFile.getAbsolutePath(), readBytes.length));
FileUtils.deletePathQuietly(testPath);
return false;
}

Expand Down
10 changes: 7 additions & 3 deletions ...rvice/src/test/java/org/apache/hadoop/ozone/container/common/utils/TestDiskCheckUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.io.File;
import java.nio.file.FileSystemException;
import java.nio.file.OpenOption;
import java.nio.file.Path;
import org.apache.ratis.util.FileUtils;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
Expand Down Expand Up @@ -84,7 +85,10 @@ public void testExistence() {
@Test
public void testReadWrite() {
assertTrue(DiskCheckUtil.checkReadWrite(testDir, testDir, 10));
assertTestFileDeleted();
}

private void assertTestFileDeleted() {
// Test file should have been deleted.
File[] children = testDir.listFiles();
assertNotNull(children);
Expand All @@ -95,16 +99,16 @@ public void testReadWrite() {
public void testCheckReadWriteDiskFull() {
try (MockedStatic<FileUtils> mockService = mockStatic(FileUtils.class)) {
// fos.write(writtenBytes) also through FileSystemException with the message
mockService.when(() -> FileUtils.newOutputStreamForceAtClose(any(File.class), any(OpenOption[].class)))
mockService.when(() -> FileUtils.newOutputStreamForceAtClose(any(Path.class), any(OpenOption[].class)))
.thenThrow(new FileSystemException("No space left on device"));

String path = testDir.getAbsolutePath();
assertThrows(FileSystemException.class,
() -> FileUtils.newOutputStreamForceAtClose(new File(path), new OpenOption[2]));
() -> FileUtils.newOutputStreamForceAtClose(testDir.toPath(), new OpenOption[2]));

// Test that checkReadWrite returns true for the disk full case
boolean result = DiskCheckUtil.checkReadWrite(testDir, testDir, 1024);
assertTrue(result, "checkReadWrite should return true when disk is full");
assertTestFileDeleted();
}
}
}