placement aggregate handler imports wrong exception module

Bug #1761295 reported by Chris Dent
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
Medium
Chris Dent

Bug Description

The placement/handlers/aggregate.py handler imports nova.exception and then tries to use exception.ConcurrentUpdateDetected. Since the move of exceptions to the placement namespace, that exception is no longer in the module.

However, we're not seeing an error because this piece of code is very very rarely called and we don't have tests (can) cover it.

I just happened to notice while doing a readthrough.

Tags: placement
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

Fix proposed to branch: master
Review: https://review.openstack.org/558916

Changed in nova:
status: Triaged → In Progress
Changed in nova:
assignee: Chris Dent (cdent) → Eric Fried (efried)
Changed in nova:
assignee: Eric Fried (efried) → Matt Riedemann (mriedem)
Matt Riedemann (mriedem)
Changed in nova:
assignee: Matt Riedemann (mriedem) → Chris Dent (cdent)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/558916
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=db6aa640623d6dbd5730b6cab359d0b470cb2dd5
Submitter: Zuul
Branch: master

commit db6aa640623d6dbd5730b6cab359d0b470cb2dd5
Author: Chris Dent <email address hidden>
Date: Wed Apr 4 21:08:02 2018 +0100

    [placement] Fix incorrect exception import

    In change I2b94945a0963d6a61af931505b69afe2d4733759 the exceptions that
    placement uses were moved into the placement hierarchy and most of the
    code that uses those exceptions was updated to reflect the change.

    I86416e35da1798cdf039b42c9ed7629f0f9c75fc merged slightly before,
    and had not made the change to the new style.

    As the old aggregate handler had no exceptions imported, there was no
    merge conflict when the exceptions change merged. At the same time
    the exception in question is not covered by tests, because it only
    happens if there is a concurrent update, which we don't cover in the
    gabbi tests. We do cover a generation conflict on a supplied
    generation, but that's not the same thing.

    So basically this went by without us noticing and I happened to notice
    while doing something else, so here's a fix for it.

    To test it, a new directory for unit tests for handlers is added and
    a new file, test_aggregate. Following the guidelines in
    https://docs.openstack.org/nova/latest/contributor/placement.html#testing
    the problematic code has been extracted to its own private method so that
    it can be tested as a unit with limited mocking. The overarching flow
    of setting aggregates continues to be tested by the gabbi functional
    tests.

    Note the added NOTE in the new _set_aggregates method: It appears that
    the DBDuplicateEntry exception maybe doesn't need to be there.

    Change-Id: I4b7b29270b2d9900b59c7dd74082688164430252
    Closes-Bug: #1761295

Changed in nova:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/nova 18.0.0.0b1

This issue was fixed in the openstack/nova 18.0.0.0b1 development milestone.

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.