Attempt to import duplicate backup record deletes existing record

Bug #1965847 reported by Lars Kellogg-Stedman
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
High
Brian Rosmaita

Bug Description

If I have an existing backup record in Cinder:

  $ openstack volume backup list
  +--------------------------------------+----------------+-------------+-----------+------+
  | ID | Name | Description | Status | Size |
  +--------------------------------------+----------------+-------------+-----------+------+
  | fa571535-d075-49dc-b467-e2b91cb0554b | example-backup | None | available | 10 |
  +--------------------------------------+----------------+-------------+-----------+------+

And I attempt to import a duplicate record:

  $ openstack volume backup record export -f value fa571535-d075-49dc-467-e2b91cb0554b > record.txt
  $ openstack volume backup record import $(cat record.txt)

The import will fail as expected:

  Invalid backup: Backup already exists in database. (HTTP 400) (Request-ID: req-c3a84538-dd96-4225-845e-55c956dc0113)

But it will unexpectedly delete the existing backup:

  $ openstack volume backup list
  <empty response>

description: updated
Changed in cinder:
importance: Undecided → Low
tags: added: backup-service openstack-cli record
Changed in cinder:
status: New → Confirmed
importance: Low → Medium
Changed in cinder:
status: Confirmed → Triaged
Revision history for this message
Brian Rosmaita (brian-rosmaita) wrote :

Here's the code:

https://opendev.org/openstack/cinder/src/commit/41e315d63a3ba382ba6e95f55b27760ca4e7a592/cinder/backup/api.py

We check for this condition in a double nested try loop (line 576), raise an InvalidBackup excption, and then catch it ourselves at line 586 and proceed to destroy the backup.

Looks like the reason for the nested try is so that we can roll back a quota reservation made earlier at line 541. Would probably be better to check for whether the backup already exists (and raise) *before* we make the reservation.

tags: added: low-hanging-fruit
Changed in cinder:
importance: Medium → High
tags: removed: record
Revision history for this message
ZhangFan (zhang-fan) wrote :

I'm sorry I accidentally clicked on the Confirmed.

Changed in cinder:
status: Triaged → Confirmed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/839451

Changed in cinder:
status: Confirmed → In Progress
Changed in cinder:
assignee: nobody → Brian Rosmaita (brian-rosmaita)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/839451
Committed: https://opendev.org/openstack/cinder/commit/6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e
Submitter: "Zuul (22348)"
Branch: master

commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5

Changed in cinder:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/yoga)

Fix proposed to branch: stable/yoga
Review: https://review.opendev.org/c/openstack/cinder/+/841745

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to cinder (master)

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/cinder/+/842000

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to cinder (master)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/842000
Committed: https://opendev.org/openstack/cinder/commit/52c8fd58b326f7ee7678710ad1fa0b0cc7ea8a26
Submitter: "Zuul (22348)"
Branch: master

commit 52c8fd58b326f7ee7678710ad1fa0b0cc7ea8a26
Author: Brian Rosmaita <email address hidden>
Date: Mon May 16 17:21:46 2022 -0400

    Remove single-use test function

    Follow-up from change I3c0e365f5dc3, where Rajat noticed that
    the patch used a static test method that was used nowhere else.
    Refactored the code to remove the static function, which makes
    the test more readable, because now it is obvious that the test
    is checking a refreshed backup object.

    Change-Id: I16bdc81df4b1b01c7918a3da4024081079bcc969
    Related-bug: #1965847

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/yoga)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/841745
Committed: https://opendev.org/openstack/cinder/commit/753c0224631e218c860e48722520e9ad384ca2f4
Submitter: "Zuul (22348)"
Branch: stable/yoga

commit 753c0224631e218c860e48722520e9ad384ca2f4
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)

tags: added: in-stable-yoga
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/xena)

Fix proposed to branch: stable/xena
Review: https://review.opendev.org/c/openstack/cinder/+/843209

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/xena)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/843209
Committed: https://opendev.org/openstack/cinder/commit/332aa33404f53b39392df3fc37dddb85b5a2890c
Submitter: "Zuul (22348)"
Branch: stable/xena

commit 332aa33404f53b39392df3fc37dddb85b5a2890c
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)
    (cherry picked from commit 753c0224631e218c860e48722520e9ad384ca2f4)

tags: added: in-stable-xena
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/wallaby)

Fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/cinder/+/844474

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/wallaby)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/844474
Committed: https://opendev.org/openstack/cinder/commit/741d0ce546d3221329751fd024cade09088d6422
Submitter: "Zuul (22348)"
Branch: stable/wallaby

commit 741d0ce546d3221329751fd024cade09088d6422
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)
    (cherry picked from commit 753c0224631e218c860e48722520e9ad384ca2f4)
    (cherry picked from commit 332aa33404f53b39392df3fc37dddb85b5a2890c)

tags: added: in-stable-wallaby
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/victoria)

Fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/cinder/+/848726

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder 20.0.1

This issue was fixed in the openstack/cinder 20.0.1 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder 19.1.1

This issue was fixed in the openstack/cinder 19.1.1 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/victoria)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/848726
Committed: https://opendev.org/openstack/cinder/commit/70001254f6e9e9644e68f1c4bcd0465dfda5dedd
Submitter: "Zuul (22348)"
Branch: stable/victoria

commit 70001254f6e9e9644e68f1c4bcd0465dfda5dedd
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)
    (cherry picked from commit 753c0224631e218c860e48722520e9ad384ca2f4)
    (cherry picked from commit 332aa33404f53b39392df3fc37dddb85b5a2890c)
    (cherry picked from commit 741d0ce546d3221329751fd024cade09088d6422)

tags: added: in-stable-victoria
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/ussuri)

Fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/c/openstack/cinder/+/854133

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder 21.0.0.0rc1

This issue was fixed in the openstack/cinder 21.0.0.0rc1 release candidate.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (stable/train)

Fix proposed to branch: stable/train
Review: https://review.opendev.org/c/openstack/cinder/+/860036

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/train)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/860036
Committed: https://opendev.org/openstack/cinder/commit/933f8fde11203226689ca44f82b0db659047e8aa
Submitter: "Zuul (22348)"
Branch: stable/train

commit 933f8fde11203226689ca44f82b0db659047e8aa
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)
    (cherry picked from commit 753c0224631e218c860e48722520e9ad384ca2f4)
    (cherry picked from commit 332aa33404f53b39392df3fc37dddb85b5a2890c)
    (cherry picked from commit 741d0ce546d3221329751fd024cade09088d6422)
    (cherry picked from commit 70001254f6e9e9644e68f1c4bcd0465dfda5dedd)
    (cherry picked from commit 407e24b5c85377eb952d73491ff3e06ddd2c6994)

tags: added: in-stable-train
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder 18.2.1

This issue was fixed in the openstack/cinder 18.2.1 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (stable/ussuri)

Reviewed: https://review.opendev.org/c/openstack/cinder/+/854133
Committed: https://opendev.org/openstack/cinder/commit/ea54727fd2600337e1a9f91a443feb3993a117f8
Submitter: "Zuul (22348)"
Branch: stable/ussuri

commit ea54727fd2600337e1a9f91a443feb3993a117f8
Author: Brian Rosmaita <email address hidden>
Date: Tue Apr 26 18:57:36 2022 -0400

    Don't destroy existing backup by mistake on import

    We're supposed to fail an import request for an already existing
    backup, but the current code confuses an existing backup record with
    one we created for the import, and during quota rollback, will
    destroy either one. Simplify the logic so that we check whether
    there's an already existing backup first and bail without even
    bothering to make an unnecessary reservation, hence leaving us nothing
    to roll back or delete by mistake.

    Also revise and add tests.

    Closes-bug: #1965847
    Change-Id: I3c0e365f5dc3c32975343f538c6029f02ac7c2b5
    (cherry picked from commit 6386cbb0a0b6cbf05ce625c5b1b680f4259bde2e)
    (cherry picked from commit 753c0224631e218c860e48722520e9ad384ca2f4)
    (cherry picked from commit 332aa33404f53b39392df3fc37dddb85b5a2890c)
    (cherry picked from commit 741d0ce546d3221329751fd024cade09088d6422)
    (cherry picked from commit 70001254f6e9e9644e68f1c4bcd0465dfda5dedd)

tags: added: in-stable-ussuri
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder train-eol

This issue was fixed in the openstack/cinder train-eol release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder ussuri-eol

This issue was fixed in the openstack/cinder ussuri-eol release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/cinder victoria-eom

This issue was fixed in the openstack/cinder victoria-eom release.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.