custom constraints can break stack-list

Bug #1314401 reported by Steven Hardy
14
This bug affects 3 people
Affects Status Importance Assigned to Milestone
OpenStack Heat
Fix Released
High
Thomas Spatzier
Icehouse
Fix Released
High
Thomas Spatzier

Bug Description

If a custom constraint validates at stack-create time, then for some reason later fails to validate, you can no longer list any stacks:

$heat stack-create a123 -f shtmp.yaml -P "key_name=stack_key;image_id=fedora-20.x86_64"
$ heat stack-list
+--------------------------------------+------------+---------------+----------------------+
| id | stack_name | stack_status | creation_time |
+--------------------------------------+------------+---------------+----------------------+
| 91e79d75-e630-49e5-a473-46e76879a32c | a123 | CREATE_FAILED | 2014-04-29T21:08:53Z |
+--------------------------------------+------------+---------------+----------------------+
$ nova keypair-delete stack_key
$ heat stack-list
ERROR: "stack_key" does not validate nova.keypair

Revision history for this message
Steven Hardy (shardy) wrote :

Actually you can't do anything with the stack which fails validation, including delete it

summary: - custom constraints can break stack-list for all stacks
+ custom constraints can break stack-list
Revision history for this message
Victor HU (huruifeng) wrote :

I can't re-produce it. Are you using sample custom_constraint as in HOT specification? Can you elaborate the shtmp.yaml?

Revision history for this message
Steven Hardy (shardy) wrote :

shtmp.yaml:

heat_template_version: 2013-05-23

parameters:
  key_name:
    type: string
    description: keypair to enable SSH access to the instance.
    constraints:
      - custom_constraint: nova.keypair

  instance_type:
    type: string
    description: Type of the instance to be created.
    default: m1.small

  image_id:
    type: string
    description: ID of the image to use for the instance to be created.

resources:
  instance:
    type: OS::Nova::Server
    properties:
      image: { get_param: image_id }
      flavor: { get_param: instance_type }
      key_name: { get_param: key_name }

Revision history for this message
Victor HU (huruifeng) wrote :

I have re-produced it. And even when the stack is created successfully, all the heat cli commands still don't work. I'm trying to find out the cause.

Changed in heat:
status: New → Confirmed
status: Confirmed → New
Thomas Herve (therve)
Changed in heat:
status: New → Confirmed
assignee: nobody → Steven Hardy (shardy)
importance: Undecided → High
milestone: none → juno-1
Revision history for this message
Victor HU (huruifeng) wrote :

It looks like the cause is when calling parser.Stack.load fuction, the template will be validated. Since the key is removed, validation always raises exception.

It's resonable that the validation need to be done when actions like create_stack, update_stack is executed.

But when we query an exsiting stack, is it necessary to validate the template again?

I suggest remove the validation for actions like query, and keep unchanged for the rest.

What do you guys think?

Revision history for this message
Thomas Spatzier (thomas-spatzier) wrote :

I am working on a related fix for bug #1314240 .

What I am basically doing to fix that other bug is to move constraint validation out of the schema object constructors but into separate validate() methods. For #1314240 I need this to have more control over the context used for validation, which is important for custom constraints.
Once that is done, it should be easier to fix this bug here. So I suggest to wait for my fix for #1314240. It should be nearly done.

Revision history for this message
Steve Baker (steve-stevebaker) wrote :

As a medium term optimisation it would be good if a stack-list didn't need to do a Stack.load on every stack

Revision history for this message
Openstack Gerrit (openstack-gerrit) wrote : Related fix proposed to heat (master)

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

Changed in heat:
assignee: Steven Hardy (shardy) → Thomas Spatzier (thomas-spatzier)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to heat (master)

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (master)

Reviewed: https://review.openstack.org/91485
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=19f83e84269e1aec2d299ada616a4648e00f4333
Submitter: Jenkins
Branch: master

commit 19f83e84269e1aec2d299ada616a4648e00f4333
Author: Thomas Spatzier <email address hidden>
Date: Wed Apr 30 19:18:16 2014 +0200

    Do not validate constraints in schema constructor

    This patch moves validation of constraints out of the constructor of
    schema objects into a separate method that can be invoked on the
    schema objects after creation. This gives us more control over when
    validation shall be invoked.

    This patch fixes an issue with custom constraints that could not be
    validated when having a default value, since they require the RPC
    context to be present. However, the context was not present in the
    places where needed.
    The short-sighted fix would have been to change a lot of method
    signatures to pass the context properly, but that did not seem to be
    really clean. The better fix seemed to be to move validation into
    a separate method that can be invoked with the proper context. This
    will also give us more control on when we do validation, and which
    validation we do.

    This patch
    Closes-Bug: #1314240

    This patch will also enable to fix
    Partial-Bug: #1314401

    For that bug it will be necessary to switch off validation under
    certain conditions, which was not possible before when doing
    validation right in the constructor.

    Change-Id: I19e1fb520551eb42bf36bb380f1cc28c81bbaedd

Steven Hardy (shardy)
tags: added: icehouse-backport-potential
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/94329
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=349a6f04b53309f7fa8f3225f98d41670888d20c
Submitter: Jenkins
Branch: master

commit 349a6f04b53309f7fa8f3225f98d41670888d20c
Author: Thomas Spatzier <email address hidden>
Date: Tue May 20 11:08:59 2014 +0200

    Do no re-validate parameters for existing stacks

    This patch disables validation of stack parameters values when
    loading existing stacks from the database. The old behavior was
    to always validate stack parameter values when initializing a
    Stack object. However, this causes problems especially with
    custom constraints when a constraint could be fulfilled during
    stack creation but cannot be fulfilled at a later point (e.g.
    when a flavor or keypair has been deleted). In such a case,
    the existing stack became completely unusable.

    This patch changes the behavior to not do parameter validation
    for existing stacks. The assumption is that validation at stack
    creation time should be sufficient to make sure the stack is valid,
    so only valid stacks end up in the database. Therefore, validating
    parameters again when loading stacks is not really required.

    Change-Id: I0947c2dcfb9db4c81b07de582aab86f262d0c008
    Closes-Bug: #1314401

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

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/98785

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/98792

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/99011

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: stable/icehouse
Review: https://review.openstack.org/99012

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on heat (stable/icehouse)

Change abandoned by Thomas Spatzier (<email address hidden>) on branch: stable/icehouse
Review: https://review.openstack.org/98785

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Thomas Spatzier (<email address hidden>) on branch: stable/icehouse
Review: https://review.openstack.org/98792
Reason: I will abandon this change after handling Steve's comment. I uploaded a new change, this time with the ChangeId of the original patch:

https://review.openstack.org/#/c/99012

Thierry Carrez (ttx)
Changed in heat:
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to heat (stable/icehouse)

Reviewed: https://review.openstack.org/99011
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=aa7c0306bcc77320b670e0e15895b4b6b3944250
Submitter: Jenkins
Branch: stable/icehouse

commit aa7c0306bcc77320b670e0e15895b4b6b3944250
Author: Thomas Spatzier <email address hidden>
Date: Wed Apr 30 19:18:16 2014 +0200

    Do not validate constraints in schema constructor

    This patch moves validation of constraints out of the constructor of
    schema objects into a separate method that can be invoked on the
    schema objects after creation. This gives us more control over when
    validation shall be invoked.

    This patch fixes an issue with custom constraints that could not be
    validated when having a default value, since they require the RPC
    context to be present. However, the context was not present in the
    places where needed.
    The short-sighted fix would have been to change a lot of method
    signatures to pass the context properly, but that did not seem to be
    really clean. The better fix seemed to be to move validation into
    a separate method that can be invoked with the proper context. This
    will also give us more control on when we do validation, and which
    validation we do.

    This patch
    Closes-Bug: #1314240

    This patch will also enable to fix
    Partial-Bug: #1314401

    For that bug it will be necessary to switch off validation under
    certain conditions, which was not possible before when doing
    validation right in the constructor.

    Cherry-pick from review https://review.openstack.org/#/c/91485
    (cherry picked from commit 19f83e84269e1aec2d299ada616a4648e00f4333)

    Conflicts:

            heat/engine/parameters.py
            heat/engine/service.py
            heat/tests/test_hot.py

    Change-Id: I19e1fb520551eb42bf36bb380f1cc28c81bbaedd

tags: added: in-stable-icehouse
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/99012
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=a39ee7c87562eab3dbf176eb265866f61f5a5af7
Submitter: Jenkins
Branch: stable/icehouse

commit a39ee7c87562eab3dbf176eb265866f61f5a5af7
Author: Thomas Spatzier <email address hidden>
Date: Tue May 20 11:08:59 2014 +0200

    Do no re-validate parameters for existing stacks

    This patch disables validation of stack parameters values when
    loading existing stacks from the database. The old behavior was
    to always validate stack parameter values when initializing a
    Stack object. However, this causes problems especially with
    custom constraints when a constraint could be fulfilled during
    stack creation but cannot be fulfilled at a later point (e.g.
    when a flavor or keypair has been deleted). In such a case,
    the existing stack became completely unusable.

    This patch changes the behavior to not do parameter validation
    for existing stacks. The assumption is that validation at stack
    creation time should be sufficient to make sure the stack is valid,
    so only valid stacks end up in the database. Therefore, validating
    parameters again when loading stacks is not really required.

    Closes-Bug: #1314401

    Cherry-pick from review https://review.openstack.org/#/c/94329
    (cherry picked from commit 349a6f04b53309f7fa8f3225f98d41670888d20c)

    Conflicts:

            heat/tests/test_parser.py

    Change-Id: I0947c2dcfb9db4c81b07de582aab86f262d0c008

Thierry Carrez (ttx)
Changed in heat:
milestone: juno-1 → 2014.2
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.