Open Stack

On 09/30/2015 11:43 AM, Sofer Athlan-Guyot wrote:
> Gilles Dubreuil <gilles at redhat.com> writes:
>
>> On 30/09/15 03:43, Rich Megginson wrote:
>>> On 09/28/2015 10:18 PM, Gilles Dubreuil wrote:
>>>> On 15/09/15 19:55, Sofer Athlan-Guyot wrote:
>>>>> Gilles Dubreuil <gilles at redhat.com> writes:
>>>>>
>>>>>> On 15/09/15 06:53, Rich Megginson wrote:
>>>>>>> On 09/14/2015 02:30 PM, Sofer Athlan-Guyot wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Gilles Dubreuil <gilles at redhat.com> writes:
>>>>>>>>
>>>>>>>>> A. The 'composite namevar' approach:
>>>>>>>>>
>>>>>>>>>       keystone_tenant {'projectX::domainY': ... }
>>>>>>>>>     B. The 'meaningless name' approach:
>>>>>>>>>
>>>>>>>>>      keystone_tenant {'myproject': name='projectX',
>>>>>>>>> domain=>'domainY',
>>>>>>>>> ...}
>>>>>>>>>
>>>>>>>>> Notes:
>>>>>>>>>     - Actually using both combined should work too with the domain
>>>>>>>>> supposedly overriding the name part of the domain.
>>>>>>>>>     - Please look at [1] this for some background between the two
>>>>>>>>> approaches:
>>>>>>>>>
>>>>>>>>> The question
>>>>>>>>> -------------
>>>>>>>>> Decide between the two approaches, the one we would like to
>>>>>>>>> retain for
>>>>>>>>> puppet-keystone.
>>>>>>>>>
>>>>>>>>> Why it matters?
>>>>>>>>> ---------------
>>>>>>>>> 1. Domain names are mandatory in every user, group or project.
>>>>>>>>> Besides
>>>>>>>>> the backward compatibility period mentioned earlier, where no domain
>>>>>>>>> means using the default one.
>>>>>>>>> 2. Long term impact
>>>>>>>>> 3. Both approaches are not completely equivalent which different
>>>>>>>>> consequences on the future usage.
>>>>>>>> I can't see why they couldn't be equivalent, but I may be missing
>>>>>>>> something here.
>>>>>>> I think we could support both.  I don't see it as an either/or
>>>>>>> situation.
>>>>>>>
>>>>>>>>> 4. Being consistent
>>>>>>>>> 5. Therefore the community to decide
>>>>>>>>>
>>>>>>>>> Pros/Cons
>>>>>>>>> ----------
>>>>>>>>> A.
>>>>>>>> I think it's the B: meaningless approach here.
>>>>>>>>
>>>>>>>>>      Pros
>>>>>>>>>        - Easier names
>>>>>>>> That's subjective, creating unique and meaningful name don't look
>>>>>>>> easy
>>>>>>>> to me.
>>>>>>> The point is that this allows choice - maybe the user already has some
>>>>>>> naming scheme, or wants to use a more "natural" meaningful name -
>>>>>>> rather
>>>>>>> than being forced into a possibly "awkward" naming scheme with "::"
>>>>>>>
>>>>>>>     keystone_user { 'heat domain admin user':
>>>>>>>       name => 'admin',
>>>>>>>       domain => 'HeatDomain',
>>>>>>>       ...
>>>>>>>     }
>>>>>>>
>>>>>>>     keystone_user_role {'heat domain admin user@::HeatDomain':
>>>>>>>       roles => ['admin']
>>>>>>>       ...
>>>>>>>     }
>>>>>>>
>>>>>>>>>      Cons
>>>>>>>>>        - Titles have no meaning!
>>>>>>> They have meaning to the user, not necessarily to Puppet.
>>>>>>>
>>>>>>>>>        - Cases where 2 or more resources could exists
>>>>>>> This seems to be the hardest part - I still cannot figure out how
>>>>>>> to use
>>>>>>> "compound" names with Puppet.
>>>>>>>
>>>>>>>>>        - More difficult to debug
>>>>>>> More difficult than it is already? :P
>>>>>>>
>>>>>>>>>        - Titles mismatch when listing the resources (self.instances)
>>>>>>>>>
>>>>>>>>> B.
>>>>>>>>>      Pros
>>>>>>>>>        - Unique titles guaranteed
>>>>>>>>>        - No ambiguity between resource found and their title
>>>>>>>>>      Cons
>>>>>>>>>        - More complicated titles
>>>>>>>>> My vote
>>>>>>>>> --------
>>>>>>>>> I would love to have the approach A for easier name.
>>>>>>>>> But I've seen the challenge of maintaining the providers behind the
>>>>>>>>> curtains and the confusion it creates with name/titles and when
>>>>>>>>> not sure
>>>>>>>>> about the domain we're dealing with.
>>>>>>>>> Also I believe that supporting self.instances consistently with
>>>>>>>>> meaningful name is saner.
>>>>>>>>> Therefore I vote B
>>>>>>>> +1 for B.
>>>>>>>>
>>>>>>>> My view is that this should be the advertised way, but the other
>>>>>>>> method
>>>>>>>> (meaningless) should be there if the user need it.
>>>>>>>>
>>>>>>>> So as far as I'm concerned the two idioms should co-exist.  This
>>>>>>>> would
>>>>>>>> mimic what is possible with all puppet resources.  For instance
>>>>>>>> you can:
>>>>>>>>
>>>>>>>>      file { '/tmp/foo.bar': ensure => present }
>>>>>>>>
>>>>>>>> and you can
>>>>>>>>
>>>>>>>>      file { 'meaningless_id': name => '/tmp/foo.bar', ensure =>
>>>>>>>> present }
>>>>>>>>
>>>>>>>> The two refer to the same resource.
>>>>>>> Right.
>>>>>>>
>>>>>> I disagree, using the name for the title is not creating a composite
>>>>>> name. The latter requires adding at least another parameter to be part
>>>>>> of the title.
>>>>>>
>>>>>> Also in the case of the file resource, a path/filename is a unique
>>>>>> name,
>>>>>> which is not the case of an Openstack user which might exist in several
>>>>>> domains.
>>>>>>
>>>>>> I actually added the meaningful name case in:
>>>>>> http://lists.openstack.org/pipermail/openstack-dev/2015-September/074325.html
>>>>>>
>>>>>>
>>>>>> But that doesn't work very well because without adding the domain to
>>>>>> the
>>>>>> name, the following fails:
>>>>>>
>>>>>> keystone_tenant {'project_1': domain => 'domain_A', ...}
>>>>>> keystone_tenant {'project_1': domain => 'domain_B', ...}
>>>>>>
>>>>>> And adding the domain makes it a de-facto 'composite name'.
>>>>> I agree that my example is not similar to what the keystone provider has
>>>>> to do.  What I wanted to point out is that user in puppet should be used
>>>>> to have this kind of *interface*, one where your put something
>>>>> meaningful in the title and one where you put something meaningless.
>>>>> The fact that the meaningful one is a compound one shouldn't matter to
>>>>> the user.
>>>>>
>>>> There is a big blocker of making use of domain name as parameter.
>>>> The issue is the limitation of autorequire.
>>>>
>>>> Because autorequire doesn't support any parameter other than the
>>>> resource type and expects the resource title (or a list of) [1].
>>>>
>>>> So for instance, keystone_user requires the tenant project1 from
>>>> domain1, then the resource name must be 'project1::domain1' because
>>>> otherwise there is no way to specify 'domain1':
>>>>
>> Yeah, I kept forgetting this is only about resource relationship/order
>> within a given catalog.
>> And therefore this is *not* about guaranteeing referred resources exist,
>>   for instance when created (or not) in a different puppet run/catalog.
>>
>> This might be obvious but it's easy (at least for me) to forget that
>> when thinking of the resources list, in terms of openstack IDs for
>> example inside self.instances!
>>
>>>> autorequire(:keystone_tenant) do
>>>>     self[:tenant]
>>>> end
>>> Not exactly.  See https://review.openstack.org/#/c/226919/
>>>
>> That's nice and makes the implementation easier.
>> Thanks.
>>
>>> For example::
>>>
>>>      keystone_tenant {'some random tenant':
>>>        name   => 'project1',
>>>        domain => 'domain1'
>>>      }
>>>      keystone_user {'some random user':
>>>        name   => 'user1',
>>>        domain => 'domain1'
>>>      }
>>>
>>> How does keystone_user_role need to be declared such that the
>>> autorequire for keystone_user and keystone_tenant work?
>>>
>>>      keystone_user_role {'some random user at some random tenant': ...}
>>>
>>> In this case, I'm assuming this will work
>>>
>>>    autorequire(:keystone_user) do
>>>      self[:name].rpartition('@').first
>>>    end
>>>    autorequire(:keystone_user) do
>>>      self[:name].rpartition('@').last
>>>    end
>>>
>>> The keystone_user require will be on 'some random user' and the
>>> keystone_tenant require will be on 'some random tenant'.
>>>
>>> So it should work, but _you have to be absolutely consistent in using
>>> the title everywhere_.
> Ok, so it seems I found a puppet pattern that could enable us to not
> depend on the particular syntax used on the title to retrieve the
> resource.  If one use "isnamevar" on multiple parameters, then using
> "uniqueness_key" on the resource enable us to retrieve the resource in
> the catalog, whatever the title of the resource is.
>
> I have a working example in this change
> https://review.openstack.org/#/c/226919/ for keystone_tenant with name,
> and domain as the keys.  All of the following work and can be easily
> retrieved using [domain, keys]
>
>    keystone_domain { 'domain_one': ensure => present }
>    keystone_domain { 'domain_two': ensure => present }
>    keystone_tenant { 'project_one::domain_one': ensure => present }
>    keystone_tenant { 'project_one::domain_two': ensure => present }
>    keystone_tenant { 'meaningless_title_one': name => 'project_less', domain => 'domain_one', ensure => present }
>
> This will raise a error:
>
>    keystone_tenant { 'project_one::domain_two': ensure => present }
>    keystone_tenant { 'meaningless_title_one': name => 'project_one', domain => 'domain_two', ensure => present }
>
> As puppet will correctly find that they are the same resource.

Great!

>
>>> That is, once you have chosen to give something
>>> a title, you must use that title everywhere: in autorequires (as
>>> described above), in resource references (e.g. Keystone_user['some
>>> random user'] ~> Service['myservice']), and anywhere the resource will
>>> be referenced by its title.
>>>
>> Yes the title must the same everywhere it's used but only within a given
>> catalog.
>>
>> No matter how the dependent resources are named/titled as long as they
>> provide the necessary resources.
>>
>> For instance, given the following resources:
>>
>> keystone_user {'first user': name => 'user1', domain => 'domain_A', ...}
>> keystone_user {'user1::domain_B': ...}
>> keystone_user {'user1': ...} # Default domain
>> keystone_project {'project1::domain_A': ...}
>> keystone_project {'project1': ...} # Default domain
>>
>> And their respective titles:
>> 'first user'
>> 'user1::domain_B'
>> 'user1'
>> 'project1::domain_A'
>> 'project1'
>>
>> Then another resource to use them, let's say keystone_user_role.
>> Using those unique titles one should be able to do things like these:
>>
>> keystone_user_role {'first user at project1::domain_A':
>>    roles => ['role1]
>> }
>>
>> keystone_user_role {'admin role for user1':
>>    user    => 'user1'
>>    project => 'project1'
>>    roles   => ['admin'] }
>>
>> That's look cool but the drawback is the names are different when
>> listing. That's expected since we're allowing meaningless titles.
>>
>> $ puppet resource keystone_user
>>
>> keystone_user { 'user1::Default':
>>    ensure    => 'present',
>>    domain_id => 'default',
>>    email     => 'test at Default.com',
>>    enabled   => 'true',
>>    id        => 'fb56d86a21f54b09aa435b96fd321eee',
>> }
>> keystone_user { 'user1::domain_B':
>>    ensure    => 'present',
>>    domain_id => '79beff022efd4011b9a036155f450af8',
>>    email     => 'user1 at domain_B.com',
>>    enabled   => 'true',
>>    id        => '2174faac46f949fca44e2edab3d53675',
>> }
>> keystone_user { 'user1::domain_A':
>>    ensure    => 'present',
>>    domain_id => '9387210938a0ef1b3c843feee8a00a34',
>>    email     => 'user1 at domain_A.com',
>>    enabled   => 'true',
>>    id        => '1bfadcff825e4c188e8e4eb6ce9a2ff5',
>> }
>>
>> Note: I changed the domain field to domain_id because it makes more
>> sense here
>>
>> This is fine as long as when running any catalog, a same resource with a
>> different name but same parameters means the same resource.
>>
>> If everyone agrees with such behavior, then we might be good to go.
>>
>> The exceptions must be addressed on a per case basis.
>> Effectively, there are cases in Openstack where several objects with the
>> exact same parameters can co-exist, for instance with the trust (See
>> commit message in [1] for examples). In the trust case running the same
>> catalog over and over will keep adding the resource (not really
>> idempotent!). I've actually re-raised the issue with Keystone developers
>> [2].
>>
>> [1] https://review.openstack.org/200996
>> [2] https://bugs.launchpad.net/keystone/+bug/1475091
>>
> For the keystone_tenant resource name, and domain are isnamevar
> parameters.  Using "uniqueness_key" method we get the always unique,
> always the same, couple [<domain>, <name>], then, when we have found the
> resource we can associate it in prefetch[10] and in autorequire without
> any problem.  So if we create a unique key by using isnamevar on the
> required parameters for each resource that need it then we get rid of
> the dependence on the title to retrieve the resource.
>
> Example of resource that should have a composite key:
>   - keystone_user: name and domain should be isnamevar. Then all the
>     question about the parsing of title would go away with robust key
>     finding.
>
>   - user_role with username, user_domain_name, project_name, project_domain_name, domain as its elements.
>
> When any of the keys are not filled they default to nil. Nil for domain
> would be associated to default_domain.
>
> The point is to go away from "title parsing" to "composite key
> matching". I'm quite sure it would simplify the code in a lot of
> places and solve the concerns raised here.

How does resource naming work at the Puppet manifest level?  For example:

   keystone_user {'some user':
     name => 'someuser',
     domain => 'domain',
   }

Would I use

   some_resource { 'name':
     requires => Keystone_user['some user'],
   }

or ???

>
>>>> Alternatively, as Sofer suggested (in a discussion we had), we could
>>>> poke the catalog to retrieve the corresponding resource(s).
>>> That is another question I posed in
>>> https://review.openstack.org/#/c/226919/:
>>>
>>> I guess we can look up the user resource and tenant resource from the
>>> catalog based on the title?  e.g.
>>>
>>>      user = puppet.catalog.resource.find(:keystone_user, 'some random user')
>>>      userid = user[:id]
>>>
>>>> Unfortunately, unless there is a way around, that doesn't work because
>>>> no matter what autorequire wants a title.
>>> Which I think we can provide.
>>>
>>> The other tricky parts will be self.instances and self.prefetch.
>>>
>>> I think self.instances can continue to use the 'name::domain' naming
>>> convention, since it needs some way to create a unique title for all
>>> resources.
>>>
>>> The real work will be in self.prefetch, which will need to compare all
>>> of the parameters/properties to see if a resource declared in a manifest
>>> matches exactly a resource found in Keystone. In this case, we may have
>>> to 'rename' the resource returned by self.instances to make it match the
>>> one from the manifest so that autorequires and resource references
>>> continue to work.
>>>
>>>>
>>>> So it seems for the scoped domain resources, we have to stick together
>>>> the name and domain: '<name>::<domain>'.
>>>>
>>>> [1]
>>>> https://github.com/puppetlabs/puppet/blob/master/lib/puppet/type.rb#L2003
>>>>
>>>>>>>> But, If that's indeed not possible to have them both,
>>>>>> There are cases where having both won't be possible like the trusts,
>>>>>> but
>>>>>> why not for the resources supporting it.
>>>>>>
>>>>>> That said, I think we need to make a choice, at least to get
>>>>>> started, to
>>>>>> have something working, consistently, besides exceptions. Other options
>>>>>> to be added later.
>>>>> So we should go we the meaningful one first for consistency, I think.
>>>>>
>>>>>>>> then I would keep only the meaningful name.
>>>>>>>>
>>>>>>>>
>>>>>>>> As a side note, someone raised an issue about the delimiter being
>>>>>>>> hardcoded to "::".  This could be a property of the resource.  This
>>>>>>>> would enable the user to use weird name with "::" in it and assign
>>>>>>>> a "/"
>>>>>>>> (for instance) to the delimiter property:
>>>>>>>>
>>>>>>>>      Keystone_tenant { 'foo::blah/bar::is::cool': delimiter => "/",
>>>>>>>> ... }
>>>>>>>>
>>>>>>>> bar::is::cool is the name of the domain and foo::blah is the project.
>>>>>>> That's a good idea.  Please file a bug for that.
>>>>>>>
>>>>>>>>> Finally
>>>>>>>>> ------
>>>>>>>>> Thanks for reading that far!
>>>>>>>>> To choose, please provide feedback with more pros/cons, examples and
>>>>>>>>> your vote.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Gilles
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> PS:
>>>>>>>>> [1] https://groups.google.com/forum/#!topic/puppet-dev/CVYwvHnPSMc
>>>>>>>>>
>>>> __________________________________________________________________________
>>>>
>>>> OpenStack Development Mailing List (not for usage questions)
>>>> Unsubscribe:
>>>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>>> __________________________________________________________________________
>>> OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> [10]: there is a problem in puppet with the way it handle prefetch and
> composite namevar.  I still have to open the bug though.  In the
> meantime you will find in
> https://review.openstack.org/#/c/226919/5/lib/puppet/provider/keystone_tenant/openstack.rb
> a idiom that works.
>