X Tutup
Skip to content

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

Open
harikrishna-patnala wants to merge 2 commits intoapache:4.22from
shapeblue:FixMountFormatBackupAndRestore
Open

Fix repository mount failure during restore due to path format handling#12778
harikrishna-patnala wants to merge 2 commits intoapache:4.22from
shapeblue:FixMountFormatBackupAndRestore

Conversation

@harikrishna-patnala
Copy link
Member

@harikrishna-patnala harikrishna-patnala commented Mar 10, 2026 •
edited
Loading

Description

This PR addresses an issue mentioned here #12669

Here we are addressing the mount operation to be same as it does during backup

https://github.com/shapeblue/cloudstack/blob/b7a11cb203aeb9458460e3ce06090172b3a814d1/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java#L71-L84

        List<String[]> commands = new ArrayList<>();
        commands.add(new String[]{
                libvirtComputingResource.getNasBackupPath(),
                "-o", "backup",
                "-v", vmName,
                "-t", backupRepoType,
                "-s", backupRepoAddress,
                "-m", Objects.nonNull(mountOptions) ? mountOptions : "",
                "-p", backupPath,
                "-q", command.getQuiesce() != null && command.getQuiesce() ? "true" : "false",
                "-d", diskPaths.isEmpty() ? "" : String.join(",", diskPaths)
        });

        Pair<Integer, String> result = Script.executePipedCommands(commands, libvirtComputingResource.getCmdsTimeout());

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@harikrishna-patnala
Fix repository mount failure during restore due to path format handling
ffdf6bd 
Restore could fail when the backup repository address was specified in
formats such as \\server\share. The restore logic built a raw shell
command which caused backslashes to be interpreted as escape characters,
resulting in an invalid mount path.

Execute the mount command using Script.executePipedCommands() so the
repository path is passed as an argument instead of being embedded in a
shell command string.
@harikrishna-patnala
Copy link
Member Author

harikrishna-patnala commented Mar 10, 2026

@blueorangutan package

@harikrishna-patnala harikrishna-patnala linked an issue Mar 10, 2026 that may be closed by this pull request
Restore Instance backup: Failed to mount repository, wrong cifs nas path #12669
Open
@harikrishna-patnala harikrishna-patnala added this to the 4.22.1 milestone Mar 10, 2026
@codecov
Copy link

codecov bot commented Mar 10, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.61%. Comparing base (58916eb) to head (c7aa780).
⚠️ Report is 3 commits behind head on 4.22.

Additional details and impacted files 
@@            Coverage Diff            @@
##               4.22   #12778   +/-   ##
=========================================
  Coverage     17.61%   17.61%           
+ Complexity    15666    15665    -1     
=========================================
  Files          5917     5917           
  Lines        531400   531415   +15     
  Branches      64970    64971    +1     
=========================================
+ Hits          93602    93606    +4     
- Misses       427244   427254   +10     
- Partials      10554    10555    +1
Flag Coverage Δ
uitests 3.70% <ø> (ø)
unittests 18.68% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@weizhouapache
Copy link
Member

weizhouapache commented Mar 10, 2026

did you mean that the issue is because executePipedCommands and runSimpleBashScriptForExitValue work differently ? @harikrishna-patnala

@harikrishna-patnala
Copy link
Member Author

harikrishna-patnala commented Mar 10, 2026 •
edited
Loading

did you mean that the issue is because executePipedCommands and runSimpleBashScriptForExitValue work differently ? @harikrishna-patnala

Yes @weizhouapache I did not tested them separately but based on the issue reported, backup operation was successful with the repository address having back slashes (\). So I followed the same execution method in restore operation as well.

sureshanaparti
sureshanaparti reviewed Mar 10, 2026
View reviewed changes
.../main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
@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.

@harikrishna-patnala
Copy link
Member Author

harikrishna-patnala commented Mar 10, 2026

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Mar 10, 2026

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Mar 10, 2026

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17064

@weizhouapache
Copy link
Member

weizhouapache commented Mar 10, 2026

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Mar 10, 2026

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

@RosiKyu RosiKyu self-assigned this Mar 10, 2026
@blueorangutan
Copy link

blueorangutan commented Mar 10, 2026

[SF] Trillian Build Failed (tid-15592)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abh1sar abh1sar Awaiting requested review from abh1sar

@sureshanaparti sureshanaparti Awaiting requested review from sureshanaparti

Assignees

@RosiKyu RosiKyu

Labels

component:kvm status:needs-testing

Projects

None yet

Milestone

4.22.1

Development

Successfully merging this pull request may close these issues.

Restore Instance backup: Failed to mount repository, wrong cifs nas path

5 participants

@harikrishna-patnala @weizhouapache @blueorangutan @sureshanaparti @RosiKyu
X Tutup