Open Stack

On Thu, Jul 17, 2014 at 12:31:05AM -0400, Zane Bitter wrote:
> On 16/07/14 23:48, Manickam, Kanagaraj wrote:
> >I have gone thru the Heat Database and found the drawbacks in the
> >existing model as listed below.  Could you review and add anything
> >missing here. Thanks.
> >
> >Heat Database model is having following drawbacks:
> >
> >1.Duplicate information
> >
> >2.Incomplete naming of columns
> >
> >3.Inconsistency in the identifiers (id) and deleted_at columns across
> >the tables
> >
> >4.resource table is specific to nova and make it generic
> >
> >5.Pre-defined constants are not using enum.
> >
> >And the section provided below describes these problem on  table vice.
> >
> >*Stack*
> >
> >Duplicate info
> >
> >Tenant & stack_user_project_id
> 
> These are different things; "stack_user_project_id" is the project/tenant in
> which Heat creates users (in a different domain); "tenant" is the
> project/tenant in which the stack itself was created.

+1

See this blog post for further info on the stack user project:

http://hardysteven.blogspot.co.uk/2014/04/heat-auth-model-updates-part-2-stack.html

A better cleanup related to the "tenant" entry imo would be to migrate all
references to "project_id", but there are some issues with that:

- The code inconsistently stores the tenant ID and name in the "tenant"
  fields (in the stack and user_creds tables repectively)
- Some clients (the swift Connection class in particular) seem to need the
  tenant *name*, which seems wrong, and probably won't work for non-default
  domains, so we need to investigate this and figure out if we can migrate
  to just storing and using the project_id everywhere.

> >Credentials_id & username , owner_id.
> >
> >Tenant is also part of user_creds and Stack always has credentials_id,
> >so what is the need of having tenant info in stack table and in stack
> >table only the credentials_id is sufficient.
> 
> tenant is in the Stack table because we routinely query by tenant and we
> don't want to have to do a join.
> 
> There may be a legitimate reason for the UserCreds table to exist separately
> from the Stack table but I don't know what it is, so merging the two is an
> option.

Yes, there is IMO - the user_creds record is not necessarily unique to a
stack - we've been creating a new row for all stacks (including backup and
nested stacks), but really you only need one set of stored credentials for
the "top level" stack which is then inherited by all child stacks.

I posted two patches yesterday implementing this:

https://review.openstack.org/#/q/status:open+project:openstack/heat+branch:master+topic:bug/1342593_2,n,z

So when they land, we won't (easily) be able to merge the usre_creds and
stack tables.  Note I'm also working on a cleanup which abstracts the
user_creds data behind a class, similar to Stack/Template etc.

> 
> >Status & action should be enum of predefined status
> 
> +1. I assume it is still easy to add more actions later?

Well they're already defined via tuples in the respective classes, but OK.

> >*User_creads*
> >
> >correct the spelling in Truststore_id
> 
> "trustor_user_id" is correct.

Yeah, see the trust documentation and my trusts blog post:

https://github.com/openstack/identity-api/blob/master/v3/src/markdown/identity-api-v3-os-trust-ext.md

http://hardysteven.blogspot.co.uk/2014/04/heat-auth-model-updates-part-1-trusts.html

> >*Resource*
> >
> >Status & action should be enum of predefined status
> 
> +1
> 
> >Rsrc_metadata - make full name resource_metadata
> 
> -0. I don't see any benefit here.

Agreed

> >Why only nova_instance column how about for other services like cinder,
> >glance resource, could be renamed to be generic enough??
> 
> +1 this should have been called "physical_resource_id".

+1, the nova_instance thing is just a historical mistake which never got
cleaned up.

> >
> >Last_evaluated -> append _at
> 
> I really don't see the point.

Me neither, particularly since we plan to deprecate the WatchRule
implementation (possibly for Juno):

https://blueprints.launchpad.net/heat/+spec/deprecate-stack-watch

> 
> >State should be an enum
> 
> +1
> 
> >*Event*
> >
> >Why uuid and id both used?
> 
> I believe it's because you should always use an integer as the primary key.
> I'm not sure if it makes a difference even though we _never_ do a lookup by
> the (integer) id.
> 
> >Resource_action is being used in both event and resource table, so it
> >should be moved to common table
> 
> I'm not sure what this means. Do you mean a common base class?

The resource action/status in the event is a snapshot of the resource
action/status at the time of the event, ergo we can't reference some common
data (such as the resource table) as the data will change.

> >Resource_status should be any enum
> 
> +1

Ok, but note we're doing a weird thing in resource.Resource.signal which
may need to be fixed first (using "signal" as the action, which isn't a
real resource action and gives odd output combined with the status)

Steve