Status set multiple times in OpenStack charms with optional relations

Bug #1588462 reported by David Ames
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
ceph-radosgw (Juju Charms Collection)
Undecided
Alex Kavanagh
cinder (Juju Charms Collection)
Undecided
Alex Kavanagh
glance (Juju Charms Collection)
Undecided
Alex Kavanagh
keystone (Juju Charms Collection)
Undecided
Alex Kavanagh
neutron-api (Juju Charms Collection)
Undecided
Alex Kavanagh
neutron-gateway (Juju Charms Collection)
Undecided
Alex Kavanagh
nova-cloud-controller (Juju Charms Collection)
Undecided
Unassigned
rabbitmq-server (Juju Charms Collection)
Undecided
Alex Kavanagh

Bug Description

In charms that call assess_status() on every hook execution and that have
optional relations there are two places where status_set() is called in
set_os_workload_status() and make_assess_status_func()'s _assess_status_func()
leading to a race condition in which a status may be overwritten.

For charms like nova-cloud-controller where strangely we call
set_os_workload_status() on each hook execution instead of assess_status() we
only see this during pause and resume actions which do call assess_status().

Regardless of __if__ you hit the race condition you can see the problem simply
using print statements and an update-status hook run:
'STATUS SET IN set_os_workload_status', 'active', 'Unit is ready'
'STATUS SET IN _assess_status_func()', 'active', 'Unit is ready'

If the first happened to be different the second would clobber it.

In the set_os_workload_status() stack a comparison is made against the current
status before setting status. The assess_status() stack needs to do the same
possibly by calling set_os_workload_status() itself.

The goal should be to have a single canonical source for status_set.

This will affect all OpenStack charms that have optional relations.

Revision history for this message
Alex Kavanagh (ajkavanagh) wrote :

I guess we're trying to minimise the changes to the full set of charms, but I think that this might not be avoidable.

The main issue, as David has investigated, is that the optional charm_func= function passed to assess_status call 'set_os_workload_status()' which sets the status, followed by 'assess_status()' doing the same thing after calling the charm_func.

The reason the optional charm_func sets the status is to discover what it will be prior to returning it to assess_status() which then sets it.

Obviously, this is a design failure. In charm_helpers, the consequence of the optional relations charm_func function, is that _determine_os_workload_status() is called TWICE, once before the optional relations, and then afterwards as part of the optional relations.

The fix is to change optional_relations such that it returns a dictionary of the possible optional interfaces that can be dynamically added to the required_interfaces, which is then passed to assess_status() so that _determine_os_workload_status() [in charmhelpers] is only called once. I'll do this on glance and propose it for review.

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

Related fix proposed to branch: master
Review: https://review.openstack.org/325650

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

Reviewed: https://review.openstack.org/325650
Committed: https://git.openstack.org/cgit/openstack/charm-glance/commit/?id=1ab2c809ad7251f6d8a30c053d8c7818ac051547
Submitter: Jenkins
Branch: master

commit 1ab2c809ad7251f6d8a30c053d8c7818ac051547
Author: Alex Kavanagh <email address hidden>
Date: Sun Jun 5 12:57:53 2016 +0000

    Fix for status-set race - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: I06efbcaade8f0c99b5931104e6887d24cb10e35a
    Related-Bug:#1588462

David Ames (thedac)
Changed in glance (Juju Charms Collection):
status: New → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-nova-cloud-controller (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/329219

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to charm-nova-cloud-controller (master)

Reviewed: https://review.openstack.org/329219
Committed: https://git.openstack.org/cgit/openstack/charm-nova-cloud-controller/commit/?id=b570e36cba93affe67594ef820afae731b746ad7
Submitter: Jenkins
Branch: master

commit b570e36cba93affe67594ef820afae731b746ad7
Author: David Ames <email address hidden>
Date: Mon Jun 13 15:55:40 2016 -0700

    Fix for status-set race - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: Ie37a4d98de9c5e7bd26304e096796ce6287ea52b
    Related-Bug:#1588462

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

Related fix proposed to branch: master
Review: https://review.openstack.org/329879

Changed in ceph-radosgw (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
assignee: Alex Kavanagh (ajkavanagh) → nobody
status: New → In Progress
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in nova-cloud-controller (Juju Charms Collection):
status: New → Fix Committed
Changed in cinder (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in cinder (Juju Charms Collection):
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-cinder (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/329916

Changed in glance (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
Changed in keystone (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
summary: - Status set race condition in OpenStack charms with optional relations
+ Status set multiple times in OpenStack charms with optional relations
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-keystone (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/329959

Changed in keystone (Juju Charms Collection):
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to charm-cinder (master)

Reviewed: https://review.openstack.org/329916
Committed: https://git.openstack.org/cgit/openstack/charm-cinder/commit/?id=734fb80b08e3fba6249ce911baa068a8eda65a7a
Submitter: Jenkins
Branch: master

commit 734fb80b08e3fba6249ce911baa068a8eda65a7a
Author: Alex Kavanagh <email address hidden>
Date: Wed Jun 15 13:10:31 2016 +0000

    Fix for multiple status-set - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: I3604ed3b36af91afb95d43e0e5e24483c9919fd1
    Related-Bug:#1588462

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

Reviewed: https://review.openstack.org/329959
Committed: https://git.openstack.org/cgit/openstack/charm-keystone/commit/?id=61047ac055fa79d1d8142f51612088bac042155f
Submitter: Jenkins
Branch: master

commit 61047ac055fa79d1d8142f51612088bac042155f
Author: Alex Kavanagh <email address hidden>
Date: Wed Jun 15 14:05:01 2016 +0000

    Fix for multiple status-set - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: I928f60967e4a7588df2b25136525391c283cda14
    Related-Bug:#1588462

Changed in keystone (Juju Charms Collection):
status: In Progress → Fix Committed
Changed in cinder (Juju Charms Collection):
status: In Progress → Fix Committed
Changed in neutron-api (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-neutron-api (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/330446

Changed in neutron-gateway (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-neutron-gateway (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/330461

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

Reviewed: https://review.openstack.org/330446
Committed: https://git.openstack.org/cgit/openstack/charm-neutron-api/commit/?id=638e06de085ea156cbed060b1c965d617fe9fdc6
Submitter: Jenkins
Branch: master

commit 638e06de085ea156cbed060b1c965d617fe9fdc6
Author: Alex Kavanagh <email address hidden>
Date: Thu Jun 16 10:35:21 2016 +0000

    Fix for multiple status-set - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: Ic5d0be6e1f7a2283e4dd0594c6465a99497dbbec
    Related-Bug:#1588462

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

Reviewed: https://review.openstack.org/329879
Committed: https://git.openstack.org/cgit/openstack/charm-ceph-radosgw/commit/?id=36ee1afc8dde730b719382ee1331bffac6f58c37
Submitter: Jenkins
Branch: master

commit 36ee1afc8dde730b719382ee1331bffac6f58c37
Author: Alex Kavanagh <email address hidden>
Date: Wed Jun 15 11:27:31 2016 +0000

    Fix for multiple status-set - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: Idb11019cec20061622b5f36911e49adfb9bac14e
    Related-Bug:#1588462

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

Reviewed: https://review.openstack.org/330461
Committed: https://git.openstack.org/cgit/openstack/charm-neutron-gateway/commit/?id=0322be6cc64f9d2c108095c77f5fd18df9c88fc0
Submitter: Jenkins
Branch: master

commit 0322be6cc64f9d2c108095c77f5fd18df9c88fc0
Author: Alex Kavanagh <email address hidden>
Date: Thu Jun 16 10:55:05 2016 +0000

    Fix for multiple status-set - related to bug 1588462

    This change fixes the obvious race for a status_set() between
    check_optional_interfaces() and assess_status() as the later calls the former
    which calls status_set(), returns the status, which is then potentially set
    again by the assess_status() function. This cleans up the code so that only a
    single status_set() is performed when calling assess_status().

    Change-Id: I05e071d635819c516ab76684842a25a9c2f81d83
    Related-Bug:#1588462

Changed in neutron-gateway (Juju Charms Collection):
status: In Progress → Fix Committed
Changed in neutron-api (Juju Charms Collection):
status: In Progress → Fix Committed
Changed in ceph-radosgw (Juju Charms Collection):
status: In Progress → Fix Committed
Revision history for this message
Alex Kavanagh (ajkavanagh) wrote :

The initially listed charms have been checked, and the rest of the charms reviewed for the multiple status_set issue:

charm-ceilometer - OK
charm-ceilometer-agent - OK
charm-ceph - different implementation - OK
charm-ceph-mon - different implementation - OK
charm-ceph-osd - different implementation - OK
charm-ceph-radosgw - DONE
charm-cinder - DONE
charm-cinder-backup - no implementation of assess_status - OK
charm-cinder-ceph - no implementation of assess_status - OK
charm-glance - DONE
charm-hacluster - no implementation of assess_status - OK
charm-heat - no implementation of assess_status - OK
charm-keystone - DONE
charm-lxd - different implementation of assess_status - OK
charm-neutron-api - DONE
charm-neutron-api-odl - no implementation of assess_status - OK
charm-neutron-gateway - DONE
charm-neutron-openvswitch - has a slightly different implementation - OK
charm-nova-cloud-controller - DONE
charm-nova-compute - has a slightly different implementation - OK
charm-odl-controller - no implementation of assess_status - OK
charm-openstack-dashboard - no optional interfaces - OK
charm-openvswitch-odl - reactive charm with no assess_status implementation - OK
charm-percona-cluster - has different implementation - not affected - OK
charm-rabbitmq-server - Not OK - has different multiple-status-set problem !!!
charm-swift-proxy - OK - but needs an edit to fix bad function names
charm-swift-storage - has simple implementation of assess_status - OK
charm-tempest - new, so has no assess_status() implementation yet. - OK

This pulls out that the rabbitmq-server charm needs to be resolved as well.

no longer affects: rabbitmq (Juju Charms Collection)
Changed in rabbitmq-server (Juju Charms Collection):
assignee: nobody → Alex Kavanagh (ajkavanagh)
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to charm-rabbitmq-server (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/331150

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

Reviewed: https://review.openstack.org/331150
Committed: https://git.openstack.org/cgit/openstack/charm-rabbitmq-server/commit/?id=406c8b69a3d71f2088bb9918831b5fe88c78c376
Submitter: Jenkins
Branch: master

commit 406c8b69a3d71f2088bb9918831b5fe88c78c376
Author: Alex Kavanagh <email address hidden>
Date: Fri Jun 17 13:53:34 2016 +0000

    Fix for multiple status_set() in assess_status()

    This fixes a multiple status_set() bug in the implementation of assess_status()
    on the charm, when it is determining whether it is active AND clustered.
    The change hooks into the _determine_os_workload_status() directly in
    charmhelpers to ensure that status_set() is only called once when
    assess_status() is called.

    Change-Id: Idfd93126c3edb41f83897c82ee7f20fe5f366559
    Related-Bug:#1588462

David Ames (thedac)
Changed in rabbitmq-server (Juju Charms Collection):
status: In Progress → Fix Committed
James Page (james-page)
Changed in ceph-radosgw (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in neutron-gateway (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in rabbitmq-server (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in neutron-api (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in keystone (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in glance (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in nova-cloud-controller (Juju Charms Collection):
status: Fix Committed → Fix Released
Changed in cinder (Juju Charms Collection):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers