loadbalancer resource suffers from having 2 masters

[Angus Salkeld]

This bug: https://bugs.launchpad.net/heat/+bug/1377681 highlighted a problem with the way we drive the neutron loadbalancer.

Firstly it has a public property "members" that the user can set. Then autoscaling dynamically updates this property too.
So if a user creates a stack with members not set (fully expecting autoscaling to drive it)
then does an unrelated update the loadbalancer will update to the now empty list of members - oops.

This is an example of this: https://github.com/openstack/heat/blob/master/heat/tests/test_neutron_autoscaling.py

I am not sure how to worm our way out of this one:-(

[Angus Salkeld (asalkeld)]

Here is a bit of debug from that test case: http://paste.openstack.org/show/120038/

To clarify the situation:
stack create (lb.members not set)
 -> scaling group runs an update on the lb (lb.members = [something])
stack update (lb.members not set)
...
If it were to stop here the user would not have a functioning lb!

Relevant bits from that log:

 WARNING [heat.engine.resource] before_props {u'protocol_port': 8080, u'pool_id': u'987a5c57-fd6f-4615-9f78-f24aa1692da1', u'members': [u'phsl4no46pnh']}

# here ^ it remembered the properties from the automatic update

 WARNING [heat.engine.resource] after_props {u'protocol_port': 8080, u'pool_id': <heat.engine.cfn.functions.ResourceRef {Ref: u'myPool'} -> u'987a5c57-fd6f-4615-9f78-f24aa1692da1'>}

# this ^ is the new empty members from the user's template

    INFO [heat.engine.resource] updating LoadBalancer "ElasticLoadBalancer" Stack "update_test_stack" [b906dc3f-7b89-4236-9627-b218fd920e7f]
 WARNING [heat.engine.resource] _store_or_update {u'protocol_port': 8080, u'pool_id': u'987a5c57-fd6f-4615-9f78-f24aa1692da1', u'members': [u'phsl4no46pnh']}
 WARNING [**lb**] LoadBalancer members:set([])
 WARNING [**lb**] LoadBalancer old_members:set([u'phsl4no46pnh'])

# here the members are getting removed from the lb.

[Angus Salkeld (asalkeld)]

My current best solution to this is to not store the properties that are modified by dynamic mechanisms (autoscaling).
We could do this by moving the self._update_stored_properties() to outside the resource.update() - maybe engine/update.py
This would mean that all user driven changes would be stored as per normal, but dynamic stuff would be ignored.
One thing will still be messy tho', the updates will always be compared to the initial state, so if you start with members=[] then it gets dynamically changed to members=[inst], then later it needs to scale down to an empty list the update method will say "nothing to change here".

[Steven Hardy (shardy)]

Would a solution be to store the resource-generated stuff in resource_data, and not mess with the properties, then merge the two when we update the LB? IIRC that's a pattern we already use elsewhere (/me tries to remember exactly where..)

[Steven Hardy (shardy)]

Additional ideas on proposed solution - use handle_signal (asg calls lb.signal() with details=[members]) instead of update, so we remove any update interaction from the ASG and poke the ASG members into resource data independently of the properties

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: proposed/juno
Review: https://review.openstack.org/127708

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/127663
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=7506d2c22fcd3d6e825ee660a4b20c5947cc7e9a
Submitter: Jenkins
Branch: master

commit 7506d2c22fcd3d6e825ee660a4b20c5947cc7e9a
Author: Zane Bitter <email address hidden>
Date: Fri Oct 10 15:43:16 2014 -0400

    Don't update a LoadBalancer under autoscaling control

    Unfortunately, Autoscaling currently uses the update() method of a
    LoadBalancer resource to do updates, with the result that the current
    member list gets persisted (good) and used to compare in the event of
    future *stack* updates (bad).

    With this patch, we assume that LoadBalancers under the control of
    Autoscaling will never have a members list property supplied in the
    template. We then ignore any updates to Autoscaling LoadBalancers that
    don't actually modify the template.

    The test changes revert the changes made in order to be able to merge
    d32370233eaf2a5c32888f269bd1dc5e0e787467, before which LoadBalancers were
    behaving correctly.

    Change-Id: I9c02ab3d3dfbee0a8a90dd0ba345a5acdaf8a610
    Closes-Bug: #1379619

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/127708
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=c06772571a81383aa37336a78d72d455067b75e8
Submitter: Jenkins
Branch: proposed/juno

commit c06772571a81383aa37336a78d72d455067b75e8
Author: Zane Bitter <email address hidden>
Date: Fri Oct 10 15:43:16 2014 -0400

    Don't update a LoadBalancer under autoscaling control

    Unfortunately, Autoscaling currently uses the update() method of a
    LoadBalancer resource to do updates, with the result that the current
    member list gets persisted (good) and used to compare in the event of
    future *stack* updates (bad).

    With this patch, we assume that LoadBalancers under the control of
    Autoscaling will never have a members list property supplied in the
    template. We then ignore any updates to Autoscaling LoadBalancers that
    don't actually modify the template.

    The test changes revert the changes made in order to be able to merge
    d32370233eaf2a5c32888f269bd1dc5e0e787467, before which LoadBalancers were
    behaving correctly.

    Change-Id: I9c02ab3d3dfbee0a8a90dd0ba345a5acdaf8a610
    Closes-Bug: #1379619
    (cherry picked from commit 7506d2c22fcd3d6e825ee660a4b20c5947cc7e9a)

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/128908
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=91c0e2329982df57e9520c23cda44930fa1b2cf4
Submitter: Jenkins
Branch: master

commit c06772571a81383aa37336a78d72d455067b75e8
Author: Zane Bitter <email address hidden>
Date: Fri Oct 10 15:43:16 2014 -0400

    Don't update a LoadBalancer under autoscaling control

    Unfortunately, Autoscaling currently uses the update() method of a
    LoadBalancer resource to do updates, with the result that the current
    member list gets persisted (good) and used to compare in the event of
    future *stack* updates (bad).

    With this patch, we assume that LoadBalancers under the control of
    Autoscaling will never have a members list property supplied in the
    template. We then ignore any updates to Autoscaling LoadBalancers that
    don't actually modify the template.

    The test changes revert the changes made in order to be able to merge
    d32370233eaf2a5c32888f269bd1dc5e0e787467, before which LoadBalancers were
    behaving correctly.

    Change-Id: I9c02ab3d3dfbee0a8a90dd0ba345a5acdaf8a610
    Closes-Bug: #1379619
    (cherry picked from commit 7506d2c22fcd3d6e825ee660a4b20c5947cc7e9a)

commit 5aa66555209eb5a59abdb189b0a5d1224e73b566
Author: Angus Salkeld <email address hidden>
Date: Fri Oct 10 12:00:37 2014 +1000

    Make sure that the properties are stored on updates

    Previously properties_data was only stored on creation, now
    this is getting passed to the update mechanism too.
    Later we can look at reworking this into a single mechanism.

    Closes-bug: #1377681
    Change-Id: If3d476f34b9e61a3c99f63ba33734a875353c8fc
    (cherry picked from commit d32370233eaf2a5c32888f269bd1dc5e0e787467)

commit 6e1ad898d887514267e3a429c291b2a067ea7e03
Author: huangtianhua <email address hidden>
Date: Wed Sep 24 15:19:24 2014 +0800

    Do not attempt a stack update when it is deleting

    If a stack is in any delete state(inprogress, failed),
    don't attempt to update it.

    Closes-bug: #1379113
    Change-Id: I1de99702a385ac8ddeffc568270d2d3b51674323

commit 1e9b2cdd9004c5e1fa3bb1a27830d75ebaebe20c
Author: Brant Knudson <email address hidden>
Date: Wed Oct 8 20:15:54 2014 -0500

    Update references to auth_token middleware

    There's references to the auth_token middleware in keystoncelient.
    The auth_token middleware has been moved to keystonemiddleware and
    the version in keystoneclient shouldn't be used anymore.

    If these references aren't updated, then when options are changed in
    keystonemiddleware.auth_token the heat-api will fail to start because
    there's duplicate options in keystoneclient.middleware.auth_token.

    Change-Id: I04573aa5ff967afe3e00329f797fcc71b779e7b3
    Closes-Bug: #1379082

commit 170069d540342ced4dc3fdb3df6619395dd56c6f
Author: Angus Salkeld <email address hidden>
Date: Thu Oct 9 15:35:34 2014 +1000

    Add missing extra "greenthread" arg to remove_event()

    See: http://eventlet.net/doc/modules/greenthread.html
    the callback gets passed a "gt" arg.

    Change-Id: I9bd44857662e45a1da2d4287017e...

[OpenStack Infra (hudson-openstack)]

Change abandoned by Steven Hardy (<email address hidden>) on branch: master
Review: https://review.openstack.org/127509