Bug 1503038
| Summary: | No validation on disks' ids when taking a snapshot | ||
|---|---|---|---|
| Product: | [oVirt] ovirt-engine | Reporter: | shani <sleviim> |
| Component: | BLL.Storage | Assignee: | shani <sleviim> |
| Status: | CLOSED CURRENTRELEASE | QA Contact: | Kevin Alon Goldblatt <kgoldbla> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 4.2.0 | CC: | amureini, bugs, ebenahar, lveyde, sleviim, tnisan, ylavi |
| Target Milestone: | ovirt-4.2.2 | Flags: | rule-engine:
ovirt-4.2+
rule-engine: exception+ |
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | ovirt-engine-4.2.2.4 | Doc Type: | Bug Fix |
| Doc Text: |
Cause:
When previewing or restoring a snapshot using the REST API,
there was some missing validation regarding the disk parameters:
- No checking for the disk's existence on DB
- No validation for disk ids' uniqueness among the passing parameters.
Consequence:
- In case of a not exists disk, is has been silently ignored.
- In case of a duplicated disk, there's an NPE
Fix:
- As a part of the fix, I had to revert some older patches which used a Set<Guid> back to a Lisk<DiskImage>, and to add a new DB query which trying to retrieve the diskImages with the ids specified by the user(or shows a 404 error if such doesn't exist) as part of patch 88355.
- Also, there's a new validation of the disk ids' uniqueness, so no disk id will be supplied more than once as patched on 88646.
Result:
- In case of not exists disk: a 404 error
- In case of duplicated ids: an appropriate error message.
In both cases - operation should be stopped.
|
Story Points: | --- |
| Clone Of: | Environment: | ||
| Last Closed: | 2018-04-05 09:39:31 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | Storage | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
shani
2017-10-17 09:06:33 UTC
The proposed solution is based on a refactoring proposed in patch https://gerrit.ovirt.org/#/c/82017/. While this refactoring looks like a step in the right direction, it's in the very least incomplete, and will result in lost functionality (full details in gerrit). Properly refactoring the command/API will require quite a bit of effort, and considering the fact that this issue has been present since the partial snapshot feature [1] was introduced in 3.4, and was never reported from the field, I'm pushing it out to 4.3. [1] https://www.ovirt.org/develop/release-management/features/storage/single-disk-snapshot/ In addition to the original scenario of the problem, another missing validation is for supplying duplicated disk ids: When the user supplies the same disk image more than once, the following error message appears on the engine log: (Checked on ovirt-engine-4.2-7684911dad) 2018-03-12 10:45:21,102+02 ERROR [org.ovirt.engine.core.bll.snapshots.RestoreFromSnapshotCommand] (default task-38) [38c09a80-e7ee-481d-ba5e-167b32910362] Command 'org.ovirt.engine.core.bll.snapshots.RestoreFromSnapshotCommand' failed: Duplicate key org.ovirt.engine.core.common.businessentities.storage.DiskImage@c63003ec 2018-03-12 10:45:21,102+02 ERROR [org.ovirt.engine.core.bll.snapshots.RestoreFromSnapshotCommand] (default task-38) [38c09a80-e7ee-481d-ba5e-167b32910362] Exception: java.lang.IllegalStateException: Duplicate key org.ovirt.engine.core.common.businessentities.storage.DiskImage@c63003ec Therefore, although this bug is targeted for 4.3.0, backporting patch http://gerrit.ovirt.org/88646 is needed. (In reply to shani from comment #2) > Therefore, although this bug is targeted for 4.3.0, backporting patch > http://gerrit.ovirt.org/88646 is needed. Let's split the difference, and just retarget it to 4.2.z. Shani, can you add some text explaining what was added here? As a part of disks' validation, while trying to preview/restore a snapshot, I had to revert some older patches which used a Set<Guid> back to a Lisk<DiskImage>. The added validation uses a new DB query which trying to retrieve the diskImage with the disk id and image_id specified by the user on REST API (or a 404 error if such doesn't exist) as part of patch 88355. Also, there's a new validation of the disk ids' uniqueness, so no disk id will be supplied more than once as patched on 88646. To make it easier to validate, the diskImage field on DiskImageValidator was changed from an iterable to a collection. Verified with the following code: ---------------------------------------- ovirt-engine-4.2.2.6-0.1.el7.noarch vdsm-4.20.23-1.el7ev.x86_64 Verified with the following scenario: ------------------------------------------- Url https://storage-ge-10.scl.lab.tlv.redhat.com/ovirt-engine/api/vms/9f00362d-3712-44e5-b846-7e6298690ad7/snapshots/967d52c6-0aab-4947-8550-e586e541dfe2/restore Method POST Body <action> <disks> <disk id = "a2487ec9-53db-42d6-b112-687dd8ddd846"> </disk> </disks> </action> Response <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <fault> <detail>Entity not found: GetDiskImageByDiskAndImageIds: disk id=a2487ec9-53db-42d6-b112-687dd8ddd846, image_id=00000000-0000-0000-0000-000000000000</detail> <reason>Operation Failed</reason> </fault> Moving to VERIFIED This bugzilla is included in oVirt 4.2.2 release, published on March 28th 2018. Since the problem described in this bug report should be resolved in oVirt 4.2.2 release, it has been closed with a resolution of CURRENT RELEASE. If the solution does not work for you, please open a new bug report. |