Skip to content

Fix NAS backup filename for legacy LINSTOR path - #13538

Merged
rp- merged 1 commit into
apache:4.22from
WanzenBug:fix-linstor-nas-backup-legacy-path
Aug 19, 2026
Merged

Fix NAS backup filename for legacy LINSTOR path#13538
rp- merged 1 commit into
apache:4.22from
WanzenBug:fix-linstor-nas-backup-legacy-path

Conversation

@WanzenBug

Copy link
Copy Markdown

Description

When backing up a running VM, nasbackup.sh derives the backup filename from the disk source path reported by libvirt. VMs started before the switch to /dev/drbd/by-res/ paths still carry the raw DRBD device node (e.g. /dev/drbd1098) in their live domain XML, which fell into the basename fallback and produced root.drbd1098.qcow2 instead of root..qcow2. Restore always expects the UUID-based name, so these backups could not be restored until the VM was stopped and started.

Resolve raw /dev/drbdNNNN device nodes back to the volume UUID by asking udev for the device's /dev/drbd/by-res/cs- symlink. Apply the same handling in all three backup loops (running-VM XML build, sparsify, and stopped-VM convert).

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

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?

@boring-cyborg

boring-cyborg Bot commented Jul 3, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

Copilot AI left a comment

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.

Pull request overview

Fixes CloudStack KVM NAS backup filename generation for legacy LINSTOR VMs whose live libvirt XML still references raw /dev/drbdNNNN device nodes, ensuring backups use the expected UUID-based naming so restores can locate the correct qcow2 files.

Changes:

  • Add get_linstor_uuid_from_device() to map raw DRBD device nodes back to a LINSTOR volume UUID via udev symlinks.
  • Apply the raw-DRBD handling consistently in the running-VM XML generation loop, the post-backup sparsify loop, and the stopped-VM convert loop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh
Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Outdated
Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Outdated
Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Outdated
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.67%. Comparing base (a951ac6) to head (5c5ac0a).
⚠️ Report is 27 commits behind head on 4.22.

Additional details and impacted files
@@             Coverage Diff              @@
##               4.22   #13538      +/-   ##
============================================
- Coverage     17.67%   17.67%   -0.01%     
+ Complexity    15797    15793       -4     
============================================
  Files          5923     5923              
  Lines        533349   533349              
  Branches      65248    65248              
============================================
- Hits          94253    94252       -1     
  Misses       428437   428437              
- Partials      10659    10660       +1     
Flag Coverage Δ
uitests 3.69% <ø> (ø)
unittests 18.74% <ø> (-0.01%) ⬇️

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

☔ View full report in Codecov by Harness.
📢 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.

@rp- rp- left a comment

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.

yes, I agree to the copilot suggestions, they make sense

When backing up a running VM, nasbackup.sh derives the backup filename
from the disk source path reported by libvirt. VMs started before the
switch to /dev/drbd/by-res/ paths still carry the raw DRBD device node
(e.g. /dev/drbd1098) in their live domain XML, which fell into the
basename fallback and produced root.drbd1098.qcow2 instead of
root.<volume-uuid>.qcow2. Restore always expects the UUID-based name, so
these backups could not be restored until the VM was stopped and started.

Resolve raw /dev/drbdNNNN device nodes back to the volume UUID by asking
udev for the device's /dev/drbd/by-res/cs-<uuid> symlink. Apply the same
handling in all three backup loops (running-VM XML build, sparsify, and
stopped-VM convert).

Signed-off-by: Moritz Tanner <moritz.tanner@linbit.com>
@WanzenBug
WanzenBug force-pushed the fix-linstor-nas-backup-legacy-path branch from 4e6ee49 to 5c5ac0a Compare July 8, 2026 13:16
@weizhouapache weizhouapache added this to the 4.22.2 milestone Jul 12, 2026

@abh1sar abh1sar left a comment

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.

Code LGTM

@DaanHoogland

Copy link
Copy Markdown
Contributor

@rp- @abh1sar what kind of testing do you suggest?

@rp-

rp- commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

@rp- @abh1sar what kind of testing do you suggest?

Maybe @sbrueseke can help with testing, as they have been the initial starter for this change.

@sbrueseke

Copy link
Copy Markdown

It is not possible for me to test this at the moment, because we do not have any stage system which has instances/backups in this state.

Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh
@rp-
rp- merged commit 85bcdb1 into apache:4.22 Aug 19, 2026
26 checks passed
@boring-cyborg

boring-cyborg Bot commented Aug 19, 2026

Copy link
Copy Markdown

Awesome work, congrats on your first merged pull request!

exit 1
fi
else
name="datadisk"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

although this PR has been merged, I am curious why line 209 is added.

      name="datadisk"

@WanzenBug
could you explain ?

@rp- @abh1sar
could you please review this line of code ?

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.

This fixes a pre-existing filename mismatch, if the first disk would be a non Linstor disk, it would keep root for the next Linstor disk instead of datadisk.

Anyway, a prepared PR, will change this again and the qemu-img convert will not be necessary anymore, but I have to wait until #13877 is merged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rp-

I am merging 4.22 branch to main branch, and will create 4.23.0.0-RC3

I would suggest to revert this PR. otherwise, 4.23.0.0 will have incomplete changes

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 isn't incomplete and fixes the transition from old volumes to new naming.

But if it helps with the 4.23.0.0 release, go ahead, I wasn't aware my merge would intervene with that...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks @rp-

since you think it is not incomplete, let's leave as it is

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants