Skip image creation if the image already exists#829
Conversation
WalkthroughModified error handling in RBD image creation to treat duplicate image scenarios as non-fatal. When Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/controllers/snapshot_controller.go`:
- Around line 318-326: When CreateImage returns librbd.ErrExist, don't log
"Created rbd image" and make the subsequent steps idempotent: update
createSnapshot to treat a duplicate-snapshot error as success (or check for
snapshot existence before creating) so repeated reconciliations don't fail;
before calling prepareSnapshotContent, verify the existing image's size equals
roundedSize and skip re-population if it matches (or error if it doesn't); and
only emit the "Created rbd image" log from the success path of
librbd.CreateImage (leave it out when ErrExist is true). Reference symbols:
librbd.CreateImage, librbd.ErrExist, roundedSize, prepareSnapshotContent,
createSnapshot.
|
|
||
| if err = librbd.CreateImage(ioCtx, imageName, roundedSize, options); err != nil { | ||
| return fmt.Errorf("failed to create os rbd image: %w", err) | ||
| if errors.Is(err, librbd.ErrExist) { | ||
| log.V(2).Info("RBD image already exists, skipping creation", "ImageID", imageName) | ||
| } else { | ||
| return fmt.Errorf("failed to create os rbd image: %w", err) | ||
| } | ||
| } | ||
| log.V(2).Info("Created rbd image", "bytes", roundedSize) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'func createSnapshot' --type go -A 25Repository: ironcore-dev/ceph-provider
Length of output: 1740
🏁 Script executed:
rg -n 'prepareSnapshotContent' --type go -B 3 -A 20Repository: ironcore-dev/ceph-provider
Length of output: 4037
🏁 Script executed:
rg -n 'reconcileIroncoreImageSnapshot' --type go -A 5Repository: ironcore-dev/ceph-provider
Length of output: 1258
When ErrExist is hit, the createSnapshot call at line 333 will fail if a snapshot already exists from a prior attempt, making the reconciliation non-idempotent.
The change skips image creation when it already exists but does not account for the snapshot creation step (line 333), which also needs idempotency. The createSnapshot function (common.go:107-128) does not check for duplicate snapshot errors — if a prior reconciliation reached line 333 but failed afterward, the retry will fail when attempting to create the same snapshot.
Additionally:
prepareSnapshotContent(line 328) re-populates the image unconditionally. If the image already exists from a prior partial failure, re-writing its content is wasteful. More importantly, there's no verification that the existing image's size matchesroundedSize; if the image was created with a different size previously, the re-population could produce incorrect results.- The log message "Created rbd image" (line 326) is misleading when the image already existed — it should only appear in the success case.
For true idempotency, consider either:
- Checking if the snapshot already exists in
createSnapshotbefore attempting creation, or - Verifying the image size before proceeding to
prepareSnapshotContent, or - Guarding both the population and snapshot steps with existence checks.
At minimum, move or adjust the log message on line 326 to avoid confusion.
🤖 Prompt for AI Agents
In `@internal/controllers/snapshot_controller.go` around lines 318 - 326, When
CreateImage returns librbd.ErrExist, don't log "Created rbd image" and make the
subsequent steps idempotent: update createSnapshot to treat a duplicate-snapshot
error as success (or check for snapshot existence before creating) so repeated
reconciliations don't fail; before calling prepareSnapshotContent, verify the
existing image's size equals roundedSize and skip re-population if it matches
(or error if it doesn't); and only emit the "Created rbd image" log from the
success path of librbd.CreateImage (leave it out when ErrExist is true).
Reference symbols: librbd.CreateImage, librbd.ErrExist, roundedSize,
prepareSnapshotContent, createSnapshot.
2 participants
Summary by CodeRabbit