Reject creating a snapshot with a duplicate name for the same volume by rootnomad · Pull Request #14089 · apache/cloudstack

rootnomad · 2026-09-09T03:03:19Z

Description

CloudStack lets you create more than one snapshot of the same volume with the same name. Since a snapshot's name is used to build its file name on the store, two snapshots of a volume with the same name map to the same file. Creating the second overwrites the first, and later deleting either one removes the shared file and leaves the other snapshot record pointing at a file that no longer exists.

This rejects creating a snapshot when an active (non-destroyed) snapshot with the same name already exists for the volume. Auto-generated snapshot names already include a timestamp, so they are not affected.

Fixes: #13051

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

Screenshots (if appropriate):

N/A

How Has This Been Tested?

Added a unit test that creates a snapshot with a name that already exists for the volume and checks it is rejected with an InvalidParameterValueException. Before this change that test fails because the create proceeds; after it, it is rejected. The existing snapshot manager tests still pass.

Also verified on a live 4.23 environment: before the change, two snapshots of the same volume with the same name both complete and end up BackedUp; after it, the second same-name snapshot is rejected while a differently named snapshot on the same volume still completes.

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

The check only compares against active snapshots (not Destroyed or Error), so re-using the name of a deleted snapshot still works. Auto-generated names carry a timestamp, so scheduled and default-named snapshots are not blocked.

DaanHoogland · 2026-09-09T16:42:51Z

thanks @nagaboinaramgopal makes sense, but I think it can go on branch 4.20

Two snapshots of the same volume with the same name map to the same file on the snapshot store, so creating the second overwrites the first, and later deleting either one removes the shared file and leaves the other snapshot pointing at nothing. Reject creating a snapshot when an active (non-destroyed) snapshot with the same name already exists for the volume. Auto-generated names already carry a timestamp so they are unaffected. Fixes: apache#13051

rootnomad · 2026-09-09T16:56:18Z

thanks @nagaboinaramgopal makes sense, but I think it can go on branch 4.20

Thanks. Moved it to 4.20.

codecov · 2026-09-10T07:12:50Z

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.42%. Comparing base (3a79799) to head (e62a34e).
⚠️ Report is 17 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...in/java/com/cloud/storage/dao/SnapshotDaoImpl.java	58.33%	5 Missing ⚠️
...om/cloud/storage/snapshot/SnapshotManagerImpl.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14089      +/-   ##
============================================
+ Coverage     16.34%   16.42%   +0.07%     
- Complexity    13576    13669      +93     
============================================
  Files          5669     5669              
  Lines        501405   501643     +238     
  Branches      60907    60944      +37     
============================================
+ Hits          81975    82374     +399     
+ Misses       410246   410042     -204     
- Partials       9184     9227      +43

Flag	Coverage Δ
uitests	`4.16% <ø> (+0.02%)`	⬆️
unittests	`17.28% <60.00%> (+0.07%)`	⬆️

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.

Query for an active snapshot with the given name on the volume instead of listing every snapshot of the volume and comparing names, as asked in review.

rootnomad · 2026-09-10T21:34:23Z

@DaanHoogland @sureshanaparti I've also verified on a live 4.23 KVM environment (NFS primary storage), creating snapshots of the same ROOT volume through the API.

Before the change, both snapshots with the same name are accepted and end up BackedUp:

20:46:48 GET command=createSnapshot&volumeid=79f03a4d-...&name=dup14089  HTTP/1.1 200 {"createsnapshotresponse":{"id":"f9df84d7-...
20:46:51 Complete async job-1519, jobStatus: SUCCEEDED ... SnapshotResponse/snapshot/{"id":"f9df84d7-20ab-48e5-a843-b5c45e16ca5e", ...
20:46:54 GET command=createSnapshot&volumeid=79f03a4d-...&name=dup14089  HTTP/1.1 200 {"createsnapshotresponse":{"id":"999155e6-...
20:46:55 Complete async job-1521, jobStatus: SUCCEEDED ... SnapshotResponse/snapshot/{"id":"999155e6-f9a4-4879-88fc-feb42a620678", ...

With the change, the second one is rejected, a different name still works, and the name can be reused once the first snapshot is deleted:

20:49:36 GET command=createSnapshot&volumeid=79f03a4d-...&name=dup14089    HTTP/1.1 200 {"createsnapshotresponse":{"id":"90e59767-...
20:49:38 Complete async job-1525, jobStatus: SUCCEEDED
20:49:41 GET command=createSnapshot&volumeid=79f03a4d-...&name=dup14089    HTTP/1.1 431 A snapshot with name [dup14089] already exists for volume ...
20:49:41 GET command=createSnapshot&volumeid=79f03a4d-...&name=other14089  HTTP/1.1 200 {"createsnapshotresponse":{"id":"7193286b-...
20:49:42 Complete async job-1527, jobStatus: SUCCEEDED
20:49:47 GET command=deleteSnapshot&id=90e59767-7aaa-470d-9077-26920547b523  HTTP/1.1 200
20:49:52 GET command=createSnapshot&volumeid=79f03a4d-...&name=dup14089    HTTP/1.1 200 {"createsnapshotresponse":{"id":"d531ec5b-...
20:49:54 Complete async job-1530, jobStatus: SUCCEEDED

No SQL errors or exceptions in the management server log during the run.

rootnomad · 2026-09-15T15:00:13Z

@sureshanaparti updated as suggested: the duplicate-name check now looks up by volume and snapshot name (ca6075e), verified on a live KVM environment. Could you take another look?

DaanHoogland · 2026-10-01T14:26:47Z

@sureshanaparti , can you look again?

Covers the finder added for duplicate snapshot name rejection: it returns the matching snapshot when one exists and null when none does, and queries by volume id and name.

The duplicate-name check added in this PR also ran for snapshots created without a caller-supplied name. Recurring snapshots are created that way and their name is auto-generated from the volume name and a timestamp, so an hourly and a daily policy firing on the same volume in the same second produce the same name and one of the scheduled snapshots was rejected. Only enforce the check when the caller supplied the name; the auto-generated path is left as it was before this PR.

rootnomad · 2026-10-13T16:25:21Z

Good catch, you're right. A scheduled snapshot carries no name, so allocSnapshot auto-generates vmname_volume_yyyyMMddHHmmss. An hourly and a daily policy firing on the same volume in the same second get the same name, and the new check rejected one of them, which were both allowed before this PR. Fixed in e62a34e: the check now only runs when the caller supplied a name, so auto-generated names are left alone. The duplicate explicit name from the original report is still refused and recurring snapshots are unaffected. Verified on a KVM lab and added a unit test for the auto-generated path.

Damans227 · 2026-10-13T17:00:31Z

Good catch, you're right. A scheduled snapshot carries no name, so allocSnapshot auto-generates vmname_volume_yyyyMMddHHmmss. An hourly and a daily policy firing on the same volume in the same second get the same name, and the new check rejected one of them, which were both allowed before this PR. Fixed in e62a34e: the check now only runs when the caller supplied a name, so auto-generated names are left alone. The duplicate explicit name from the original report is still refused and recurring snapshots are unaffected. Verified on a KVM lab and added a unit test for the auto-generated path.

that covers it, thx!

Damans227

LGTM overall..

DaanHoogland · 2026-10-20T09:00:41Z

@blueorangutan package

blueorangutan · 2026-10-20T09:02:06Z

@DaanHoogland 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 · 2026-10-20T11:19:45Z

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

DaanHoogland · 2026-10-21T14:30:33Z

@blueorangutan test

blueorangutan · 2026-10-21T14:32:04Z

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

Damans227 · 2026-10-21T19:20:34Z

Tested on a 4.20 KVM lab (NFS primary storage, one host) with the SL-JID 19624 packages.

Test:

Deploy a VM and snapshot its ROOT disk twice with the same name. Then use the same name on a data disk, again after deleting the first snapshot, and take snapshots with no name, two of them in the same second.

Before: plain 4.20 (the PR's classes swapped out). Both snapshots are accepted:

dupbefore  8331294e-cb1f-4774-ac9d-e1ea8ee5081e  BackedUp
dupbefore  f20b4bc0-a018-42ce-a28c-5039ab387388  BackedUp

On KVM with NFS each one still got its own file, so the overwrite from the issue didnt show up here.

After: the second one is refused:

2026-10-21 19:05:07,203 INFO  [c.c.a.ApiServer] (logid:6893d4e1) A snapshot with name [dup14089] already exists for volume VolumeObject {..."name":"ROOT-3"...}

The same name on another volume works. The name can be used again after the first snapshot is deleted. Snapshots with no name still work, including two in the same second that got the same auto name (vm14089_data14089_20261021190721).

@rootnomad can the error show just the volume name? right now it prints the whole volume object.

blueorangutan · 2026-10-22T07:29:55Z

[SF] Trillian test result (tid-17184)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 57235 seconds
Marvin logs: [archive removed]
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti

clgtm

boring-cyborg Bot added the component:storage label Sep 9, 2026

rootnomad force-pushed the pr/fix-duplicate-snapshot-name branch from c0df0d6 to b229715 Compare September 9, 2026 16:50

rootnomad changed the base branch from main to 4.20 September 9, 2026 16:50

sureshanaparti reviewed Sep 10, 2026

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java Outdated

sureshanaparti requested changes Sep 10, 2026

View reviewed changes

snapshot: look up a duplicate snapshot name by volume and name

ca6075e

Query for an active snapshot with the given name on the volume instead of listing every snapshot of the volume and comparing names, as asked in review.

engine-schema: unit test SnapshotDao.findByVolumeIdAndNameNotInStatus

dcd6739

Covers the finder added for duplicate snapshot name rejection: it returns the matching snapshot when one exists and null when none does, and queries by volume id and name.

DaanHoogland requested a review from sureshanaparti October 12, 2026 15:16

Damans227 reviewed Oct 12, 2026

View reviewed changes

Comment thread server/src/main/java/com/cloud/storage/snapshot/SnapshotManagerImpl.java Outdated

Damans227 approved these changes Oct 13, 2026

View reviewed changes

sureshanaparti approved these changes Oct 22, 2026

View reviewed changes

DaanHoogland merged commit e742aad into apache:4.20 Oct 22, 2026
26 checks passed

DaanHoogland added this to the 4.20.4 milestone Oct 22, 2026

DaanHoogland mentioned this pull request Oct 22, 2026

Cloudstack allows you to create snapshots with the same name for the same volume #13051

Closed

Conversation

rootnomad commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

DaanHoogland commented Sep 9, 2026

Uh oh!

rootnomad commented Sep 9, 2026

Uh oh!

Uh oh!

codecov Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rootnomad commented Sep 10, 2026

Uh oh!

rootnomad commented Sep 15, 2026

Uh oh!

DaanHoogland commented Oct 1, 2026

Uh oh!

Uh oh!

rootnomad commented Oct 13, 2026

Uh oh!

Damans227 commented Oct 13, 2026

Uh oh!

Damans227 left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 20, 2026

Uh oh!

blueorangutan commented Oct 20, 2026

Uh oh!

blueorangutan commented Oct 20, 2026

Uh oh!

DaanHoogland commented Oct 21, 2026

Uh oh!

blueorangutan commented Oct 21, 2026

Uh oh!

Damans227 commented Oct 21, 2026

Test:

Uh oh!

blueorangutan commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rootnomad commented Sep 9, 2026 •

edited

Loading

codecov Bot commented Sep 10, 2026 •

edited

Loading

blueorangutan commented Oct 22, 2026 •

edited

Loading