X Tutup
Skip to content

Fix repository mount failure during restore due to path format handling #12778

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

Open
harikrishna-patnala wants to merge 2 commits into
base: 4.22
Choose a base branch
from
+47 −13
Open

Fix repository mount failure during restore due to path format handling #12778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffdf6bd
Fix repository mount failure during restore due to path format handling
harikrishna-patnala Mar 10, 2026
c7aa780
Unit test fixes
harikrishna-patnala Mar 10, 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
35 changes: 26 additions & 9 deletions ...in/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Objects;

@ResourceWrapper(handles = RestoreBackupCommand.class)
public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper<RestoreBackupCommand, Answer, LibvirtComputingResource> {
private static final String BACKUP_TEMP_FILE_PREFIX = "csbackup";
private static final String MOUNT_COMMAND = "sudo mount -t %s %s %s";
Copy link
Contributor

@sureshanaparti sureshanaparti Mar 10, 2026

Choose a reason for hiding this comment

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

any issues using this pattern? should consider for umount cmd as well?

Copy link
Member Author

@harikrishna-patnala harikrishna-patnala Mar 10, 2026

Choose a reason for hiding this comment

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

This is unused so removed it. Unmount command is straight forward which unmounts the backup directory.

private static final String UMOUNT_COMMAND = "sudo umount %s";
private static final String FILE_PATH_PLACEHOLDER = "%s/%s";
private static final String ATTACH_QCOW2_DISK_COMMAND = " virsh attach-disk %s %s %s --driver qemu --subdriver qcow2 --cache none";
Expand Down Expand Up @@ -182,7 +182,7 @@ private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPa

private String mountBackupDirectory(String backupRepoAddress, String backupRepoType, String mountOptions, Integer mountTimeout) {
String randomChars = RandomStringUtils.random(5, true, false);
String mountDirectory = String.format("%s.%s",BACKUP_TEMP_FILE_PREFIX , randomChars);
String mountDirectory = String.format("%s.%s", BACKUP_TEMP_FILE_PREFIX, randomChars);

try {
mountDirectory = Files.createTempDirectory(mountDirectory).toString();
Expand All @@ -191,24 +191,41 @@ private String mountBackupDirectory(String backupRepoAddress, String backupRepoT
throw new CloudRuntimeException("Failed to create the tmp mount directory for restore on the KVM host");
}

String mount = String.format(MOUNT_COMMAND, backupRepoType, backupRepoAddress, mountDirectory);
if ("cifs".equals(backupRepoType)) {
if ("cifs".equalsIgnoreCase(backupRepoType)) {
if (Objects.isNull(mountOptions) || mountOptions.trim().isEmpty()) {
mountOptions = "nobrl";
} else {
mountOptions += ",nobrl";
}
}

executeMount(backupRepoAddress, backupRepoType, mountOptions, mountDirectory, mountTimeout);

return mountDirectory;
}

private void executeMount(String backupRepoAddress, String backupRepoType, String mountOptions,
String mountDirectory, Integer mountTimeout) {
List<String[]> commands = new ArrayList<>();
List<String> cmd = new ArrayList<>();
cmd.add("sudo");
cmd.add("mount");
cmd.add("-t");
cmd.add(backupRepoType);
cmd.add(backupRepoAddress);
cmd.add(mountDirectory);
if (Objects.nonNull(mountOptions) && !mountOptions.trim().isEmpty()) {
mount += " -o " + mountOptions;
cmd.add("-o");
cmd.add(mountOptions);
}
commands.add(cmd.toArray(new String[0]));

int exitValue = Script.runSimpleBashScriptForExitValue(mount, mountTimeout, false);
if (exitValue != 0) {
logger.error("Failed to mount repository {} of type {} to the directory {}", backupRepoAddress, backupRepoType, mountDirectory);
Pair<Integer, String> result = Script.executePipedCommands(commands, mountTimeout);
if (result.first() != 0) {
logger.error("Failed to mount repository {} of type {} to the directory {}. Error: {}",
backupRepoAddress, backupRepoType, mountDirectory, result.second());
throw new CloudRuntimeException("Failed to mount the backup repository on the KVM host");
}
return mountDirectory;
}

private void unmountBackupDirectory(String backupDirectory) {
Expand Down
25 changes: 21 additions & 4 deletions ...ava/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.cloud.agent.api.Answer;
import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
import com.cloud.utils.Pair;
import com.cloud.utils.script.Script;
import com.cloud.vm.VirtualMachine;
import org.apache.cloudstack.backup.BackupAnswer;
Expand Down Expand Up @@ -79,6 +80,8 @@ public void testExecuteWithVmExistsNull() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -121,6 +124,8 @@ public void testExecuteWithVmExistsTrue() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -159,6 +164,8 @@ public void testExecuteWithVmExistsFalse() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -198,6 +205,8 @@ public void testExecuteWithCifsMountType() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -239,8 +248,8 @@ public void testExecuteWithMountFailure() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(1); // Mount failure
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(1, "Failed"));

Answer result = wrapper.execute(command, libvirtComputingResource);

Expand Down Expand Up @@ -275,6 +284,8 @@ public void testExecuteWithBackupFileNotFound() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -321,8 +332,8 @@ public void testExecuteWithCorruptBackupFile() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
.thenAnswer(invocation -> {
String command = invocation.getArgument(0);
Expand Down Expand Up @@ -369,6 +380,8 @@ public void testExecuteWithRsyncFailure() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -419,6 +432,8 @@ public void testExecuteWithAttachVolumeFailure() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down Expand Up @@ -510,6 +525,8 @@ public void testExecuteWithMultipleVolumes() throws Exception {
filesMock.when(() -> Files.createTempDirectory(anyString())).thenReturn(tempPath);

try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
Mockito.when(Script.executePipedCommands(Mockito.anyList(), Mockito.anyLong()))
.thenReturn(new Pair<>(0, "success"));
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString(), anyInt(), any(Boolean.class)))
.thenReturn(0); // Mount success
scriptMock.when(() -> Script.runSimpleBashScriptForExitValue(anyString()))
Expand Down
Loading
X Tutup