Thanks Alex for your detailed inspection of my work. Comments inline.. On 3 February 2015 at 21:32, Alexandre Levine <[email protected]> wrote:
> I'm writing this in regard to several reviews concering tagging > functionality for EC2 API in nova. > The list of the reviews concerned is here: > > https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:master+topic:bp/ec2-volume-and-snapshot-tags,n,z > > I don't think it's a good idea to merge these reviews. The analysis is > below: > > *Tagging in AWS* > > Main goal for the tagging functionality in AWS is to be able to > efficiently distinguish various resources based on user-defined criteria: > > "Tags enable you to categorize your AWS resources in different ways, for > example, by purpose, owner, or environment. > ... > You can search and filter the resources based on the tags you add." > > (quoted from here: > http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html) > > It means that one of the two main use-cases is to be able to use Tags as > filter when you describe something. Another one is to be able to get > information about particular tag with all of the resources tagged by it. > Also there is a constraint: > > "You can tag public or shared resources, but the tags you assign are > available only to your AWS account and not to the other accounts sharing > the resource." > The important part here is "shared resources" which are visible to > different users but tags are not shared - each user sees his own. > > > > *Existing implementation in nova *Existing implementation of tags in > nova's EC2 API covers only instances. But it does so in both areas: > 1. Tags management (create, delete, describe,...) > 2. Instances filtering (describe_instances with filtering by tags). > The implementation is based on storing tags in each instance's metadata. > And nova DB sqlalchemy level uses "tag:" in queries to allow instances > describing with tag filters. > > I see the following design flaws in existing implementation: > > 1. It uses instance's own metadata for storing information about assigned > tags. > Problems: > - it doesn't scale when you want to start using tags for other resources. > Following this design decision you'll have to store tags in other resources > metadata, which mean different services APIs and other databases. So > performance for searching for tags or tagged resources in main use cases > should suffer. You'll have to search through several remote APIs, querying > different metadatas to collect all info and then to compile the result. > - instances are not shared resources, but images are. It means that, when > developed, metadata for images will have to store different tags for > different users somehow. > > 2. EC2-specific code ("tag:" searching in novaDB sqlalchemy) leaked into > lower layers of nova. > - layering is violated. There should be no EC2-specifics below EC2 API > library in nova, ideally. > All of the Nova-EC2 mapping happens in Nova's DB currently. See InstanceIdMapping model in nova/db/sqlalchemy/models.py. EC2 API which resides in Nova will keep using the Nova database as long as it is functional. > - each other service will have to implement the same solution in its own > DB level to support tagging for EC2 API. > > *Proposed review changes* > > The review in question introduces tagging for volumes and snapshots. It > follows design decisions of existing instance tagging implementation, but > realizes only one of the two use cases. It provides "create", "delete", > "describe" for tags. But it doesn't provide describe_volumes or > describe_snapshots for filtering. > I honestly forgot about those two methods. I can implement them. > > It suffers from the design flaws I listed above. It has to query remote > API (cinder) for metadata. It didn't implement filtering by "tag:" in > cinder DB level so we don't see implementation of describe_volumes with > tags filtering. > Cinder do support filtering based on tags, and I marked the work as TODO in https://review.openstack.org/#/c/112325/23/nova/volume/cinder.py . This was not the reason why I didn't implement describe_volumes and describe_snapshots. Those two methods just missed my attention :) Nova's EC2 API's tag filtering is also done in-memory presently if I'm correct, as Nova's API doesn't support filtering only on the basis of tag names or tag values alone.. > > *Current stackforge/ec2-api tagging implementation* > > In comparison, the implementation of tagging in stackforge/ec2-api, stores > all of the tags and their links to resources and users in a separate place. > So we can efficiently list tags and its resources or filter by tags during > describing of some of the resources. Also user-specific tagging is > supported. > > > > *Conclusion *Keeping in mind all of the above, and seeing your discussion > about deprecation of EC2 API in nova, I don't feel it's a good time to add > such a half-baked code with some potential problems into nova. > I think it's better to concentrate on cleaning up, fixing, reviving and > making bullet-proof whatever functionality is currently present in nova for > EC2 and used by clients. > EC2 API already shares database with Nova's, so the tight coupling between EC2 API and Nova's database is not going to go away till the time EC2 API server/controller is present in Nova. Nova instance metadata is being used as EC2 instance tags, and what the above-referenced spec is doing is is very similar: Cinder volume metadata is being used as EC2 volume tags, and similarly for volume snapshots. I don't see a difference between instances and volumes and volume snapshots in the sense that they all are non-share-able (yet). I completely understand that these patches look like feature additions. I started working on them first in January 2014 ( https://review.openstack.org/#/c/64690/ ), and at that time it was just a sincere effort to improve EC2 API using the first possible way I could find out. Since we have not deprecated the in-Nova EC2 support yet, and we are yet to reach a concrete plan to move forward, I am tempted to ask for allowing this patch to be considered for review.. I am fine if people think these patches shouldn't be allowed to go in. I can only wish that the patches got more attention when it was possible to get them merged :) Regards, Rushi Agrawal > Best regards, > Alex Levine > > > __________________________________________________________________________ > OpenStack Development Mailing List (not for usage questions) > Unsubscribe: [email protected]?subject:unsubscribe > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > >
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: [email protected]?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
