Mailing-List: contact dev-help@brooklyn.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@brooklyn.apache.org
Subject: Re: [PROPOSAL] merging config keys
To: dev@brooklyn.apache.org
References: <e9db148a-cb7a-e6f1-9ae3-ec8e45195846@gmail.com>
 <F85A5C64-947C-44B1-A133-DDE6B6F2C85F@cloudsoftcorp.com>
 <F02C152A-2F80-4D0C-AA0B-9C473FCBA887@cloudsoftcorp.com>
 <5745CDDC.8070107@CloudsoftCorp.com>
From: Aled Sage <aled.sage@gmail.com>
Message-ID: <41379905-2bb5-f1ba-5da2-76ecdaf7ff97@gmail.com>
Date: Wed, 25 May 2016 21:28:16 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0)
 Gecko/20100101 Thunderbird/45.1.0
MIME-Version: 1.0
In-Reply-To: <5745CDDC.8070107@CloudsoftCorp.com>
Content-Type: multipart/alternative;
 boundary="------------5358E36062A148873A76A275"
archived-at: Wed, 25 May 2016 20:56:49 -0000

--------------5358E36062A148873A76A275
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Alex, all,

I wondered about (1) as well. I concluded that we should optimise for 
what I believe is the most common case for these config keys: being able 
to add additional values and override specific values. For example, if 
someone defines a Tomcat entity with default environment variables, then 
I want to be able to add to those and override specific values easily.

The alternative approach you mentioned (below) is technically elegant, 
but feels overly complicated YAML for the common case.

     shell.env:
       $brooklyn:merge:
       - overridden_key: value
         new_key: value

If folk agree that the common case to optimise for is the "merge" for 
config keys like shell.env, then we could limit the less-obvious 
approach for the overwrite use-case:

     shell.env:
       $brooklyn:overwrite:
       - new_key1: value
         new_key2: value

/(As an aside, at some point I'd like to completely revisit shell.env: 
we should better support supplying different environment variables to 
each of the install/launch/checkRunning scripts.)/

---
For (2), I lean towards treating templateOptions (within 
provisioningProperties) as a special case. Really we need a major 
overhaul of our JcloudsLocation code.

My long-term ideal would be that we don't need to put those key-values 
within a templateOptions sub-map. That is really a consequence of our 
implementation.

I think merging of templateOptions is the behaviour that a user 
(unfamiliar with the underlying implementation) would expect. Until one 
explains about how this maps to the jclouds TemplateOptions class, there 
is little logic for what is a top-level key-value and what goes inside 
templateOptions. So I think a user would want them both merged.

Aled


On 25/05/2016 17:07, Alex Heneveld wrote:
>
> two difficulties i see:
>
> 1) how do i clear inherited map values on a MERGEd config?
> 2) how do we specify that "templateOptions" in a 
> "provisioningProperties" is to be merged?
>
> (followed by conclusion -- outlining an alternative but overall unsure)
>
>
> *1) how do i clear inherited map values on a MERGEd config?*
>
> e.g. say we have
>
> parent with
>
>     shell.env: { X: 1 }
>
> and child wants to ensure shell.env has *nothing* for X. previously 
> child could just say
>
>     shell.env: {}
>
> however with this proposal i think the child now requires:
>
>     shell.env: { X: null }
>
> listing every key it inherits and hoping that the shell to-string 
> excludes nulls.
>
>
> *2) how do we specify that "templateOptions" in a 
> "provisioningProperties" is to be merged?*
>
> for this proposal we'll write code to understand ConfigInheritance at 
> entities and locations.  i don't think we yet have discussed any way 
> to do this one level deeper.  specifically if i've got
>
>     # parent
>     provisioningProperties:
>       templateOptions:
>         floatingIpPoolNames: pool
>
>     # child
>     type: parent
>     provisioningProperties:
>       templateOptions:
>         networks: xyz
>
> we know from the definition of PROVISIONING_PROPERTIES on 
> SoftwareProcess that that map should be merged with its super. however 
> when merging the actual provisioning properties we have no way to 
> understand the semantics, do we?  specifically there is no indication 
> that PROVISIONING_PROPERTIES is:
> * a map of config
> * usually containing config keys from JcloudsLocationConfig.
> without that knowledge we can't do the "depth 2" merge illustrated in 
> the example, can we?  in other words we'll lose "floatingIpPoolNames: 
> pool".
>
>
> *conclusion*
>
> these two issues aren't showstoppers but they are a little bit 
> smelly.  apart from them the proposal is very good:  it solves an 
> irritation around maps in a fairly simple elegant way and only 
> impacting opt-in config-keys in a cleanly defined way.
>
> using $brooklyn:super() with a proposed $brooklyn:merge is an 
> alternative solution which lets us solve (2) and avoids both of these 
> issues:
>
>     shell.env:
>       $brooklyn:merge:
>       - $brooklyn:super()
>       - overridden_key: value
>         new_key: value
>
> this could also work for lists.  however it requires the user to 
> explicitly write this, it's uglier, and it might be hard to implement.
>
> if we introduced a `$brooklyn:overwrite` we could combine aled's 
> proposal with dsl solutions to problems (1) and (2) described here.  
> but it makes behaviours more complicated.
>
> in short not yet sure what is best...
>
> --a
>
>
>
> On 25/05/2016 07:36, Geoff Macartney wrote:
>> +1
>>
>> This sounds like a good proposal.  At the same time it’s fairly 
>> complex, so I think an important part of the change for this would be
>>
>> 1. to test it comprehensively, so each of the scenarios below would 
>> require at least one test case, and then
>> 2. to document it equally comprehensively - a new subsection could be 
>> added in the User Manual under YAML blueprints, with content taken 
>> from the email below and beefed up for general readership
>>
>> At the moment I don’t think the documentation is comprehensive enough 
>> about all these details (as they work today), this could be a good 
>> opportunity to improve it.
>>
>> cheers
>> Geoff
>>
>>
>> ————————————————————
>> Gnu PGP key - http://is.gd/TTTTuI
>>
>>
>>> On 25 May 2016, at 12:44, Svetoslav Neykov 
>>> <svetoslav.neykov@cloudsoftcorp.com> wrote:
>>>
>>> +1 for the proposal, definitely makes sense.
>>>
>>> One thing that's not clear to me is how deep the merge should be. 
>>> Having templateOptions as an example I think it should be a shallow 
>>> merge. Can't think of deep complex structures passed in yaml that 
>>> would favour deep merge.
>>>
>>> Re generalizing "$brooklyn:super()" - could have it as a string key 
>>> in maps that we want to merge. That is the owner of the map that's 
>>> doing the override can define whether he prefers merge or override. 
>>> It makes sense when developing blueprints because you know what the 
>>> catalog items being inherited are and can decide which way to go.
>>>
>>> Svet.
>>>
>>>
>>>
>>>
>>>> On 25.05.2016 г., at 14:12, Aled Sage <aled.sage@gmail.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> TL;DR: we should merge config when overriding entities/locations, 
>>>> where it's obvious that such behaviour is desired. For example, 
>>>> where an entity type defines shell.env, then a new entity extending 
>>>> this type should inherit and add to those values.
>>>>
>>>>
>>>> _*REQUIREMENTS*_
>>>>
>>>> _*shell.env in entities*_
>>>>
>>>> When extending an existing entity type in YAML, it is not possible 
>>>> to extend the set of environment variables. Instead, if the 
>>>> sub-type declares shell.env it will override the inherited values.
>>>>
>>>> For example, consider the catalog items below:
>>>>
>>>>   # Catalog
>>>>   brooklyn.catalog:
>>>>      items:
>>>>      - id: machine-with-env
>>>>        item:
>>>>          type:
>>>> org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess
>>>>          brooklyn.config:
>>>>            shell.env:
>>>>              ENV1: myEnv1
>>>>
>>>>
>>>>   # Blueprint
>>>>   location: ...
>>>>   services:
>>>>   - type: machine-with-env
>>>>      brooklyn.config:
>>>>        shell.env:
>>>>          ENV2: myEnv2
>>>>        launch.command: echo "ENV1=$ENV1, ENV2=$ENV2"
>>>>
>>>> A user might well expect the launch.command to have myEnv1 and 
>>>> myEnv2. However, it does not get the ENV1 environment variable. 
>>>> This is a real pain when trying to customize stock blueprints.
>>>>
>>>> We propose that the shell.env map should be *merged*.
>>>>
>>>>
>>>> _*provisioning.properties*_
>>>>
>>>> An entity can be configured with provisioning.properties. These are 
>>>> passed to the location when obtaining a new machine. They 
>>>> supplement and override the values configured on the location. 
>>>> However, for templateOptions the expected/desired behaviour would 
>>>> be to merge the options.
>>>>
>>>> Consider the blueprint below:_*
>>>> *_
>>>>
>>>>   location:
>>>>      minCores: 1
>>>>      templateOptions:
>>>>        networks: myNetwork
>>>>   services:
>>>>   - type: org.apache.brooklyn.entity.machine.MachineEntity
>>>>      brooklyn.config:
>>>>        provisioning.properties:
>>>>          minRam: 2G
>>>>          templateOptions:
>>>>            tags: myTag
>>>>
>>>> A user might well expect the VM to be created with the given 
>>>> networks and tags. However, currently the templateOptions in 
>>>> provisoining.properties will override the existing value, rather 
>>>> than being merged with it.
>>>>
>>>> We propose that the templateOptions map should be *merged*.
>>>>
>>>> Valentin made a start to fix this in 
>>>> https://github.com/apache/brooklyn-server/pull/151.
>>>>
>>>>
>>>> _*_*provisioning.properties in sub-entities*_
>>>> *_
>>>>
>>>> A similar argument holds for when extending an entity-type in YAML.
>>>>
>>>> If the super-type declares template options, then any additional 
>>>> provisioning.properties declared on the entity sub-type should be 
>>>> *merged* (including merging the templateOptions map contained 
>>>> within it).
>>>>
>>>>
>>>> _*files.preinstall, templates.preinstall, etc*_
>>>>
>>>> The same applies for the map config for: files.preinstall, 
>>>> templates.preinstall, files.install, templates.install, 
>>>> files.runtime and templates.runtime.
>>>>
>>>> We propose that these maps get *merged* with the value defined in 
>>>> the super-type.
>>>>
>>>>
>>>> _*Overriding default values*_
>>>>
>>>> For default values in the super-type, we propose that this value 
>>>> *does* get overridden, rather than merged.
>>>>
>>>> For example, in the blueprint below we suggest that the 
>>>> launch-command in the sub-type should have ENV2 but not 
>>>> ENV_IN_DEFAULT.
>>>>
>>>>   brooklyn.catalog:
>>>>      items:
>>>>      - id: machine-with-env
>>>>        version: 1.0.0
>>>>        item:
>>>>          type:
>>>> org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess
>>>>          brooklyn.parameters:
>>>>          - name: shell.env
>>>>            default:
>>>>              ENV_IN_DEFAULT: myEnvInDefault
>>>>      - id: machine-with-env-2
>>>>        version: 1.0.0
>>>>        item:
>>>>          type: machine-with-env
>>>>          brooklyn.config:
>>>>            shell.env:
>>>>              ENV2: myEnv2
>>>>            launch.command: echo "ENV_IN_DEFAULT=$ENV_IN_DEFAULT,
>>>>   ENV2=$ENV2"
>>>>
>>>> (Interestingly, the current behaviour of machine-with-env is that 
>>>> it gets the value for ENV_IN_DEFAULT but not for ENV2, so sometime 
>>>> strange is going on with re-defining the shell.env config key!)
>>>>
>>>>
>>>> _*Extending commands: deferred*_
>>>>
>>>> Another scenario is where a super-type declares a value for 
>>>> `install.command`, and the sub-type wants to augment this by adding 
>>>> additional commands. Currently that is not possible. Instead the 
>>>> sub-type needs to use pre.install.command and/or 
>>>> post.install.command. But that leads to the same problem if a 
>>>> super-type also has a value defined for that key.
>>>>
>>>> Svet suggested we could perhaps introduce something like 
>>>> $brooklyn:super().
>>>>
>>>> Unless we can generalise that approach to also solve the merging of 
>>>> `shell.env` etc, then I suggest we defer the `install.command` 
>>>> use-case. That can be proposed and discussed in a different thread.
>>>>
>>>> However, if we can solve these problems with clever explicit use of 
>>>> $brooklyn:super(), then that could provide an elegant solution to 
>>>> all of these problems!
>>>>
>>>>
>>>> _*Inheritance from parent entities*_
>>>>
>>>> Things are made yet more complicated by the fact we inherit config 
>>>> from parent entities, in the entity hierarchy.
>>>>
>>>> We propose that this behaviour is also configurable for the config 
>>>> key, but that the defaults stay as they are. The existing logic is 
>>>> applied to find the config value that applies to the given entity. 
>>>> That value is then merged with its super-type, as appropriate.
>>>>
>>>> For example, in the blueprint below... machine1 would get ENV1 and 
>>>> ENV2 (i.e. the ENV1 definition overrides the ENV_IN_APP 
>>>> definition). However, machine2 would get ENV1 and ENV_IN_APP (i.e. 
>>>> it inherits ENV_IN_APP from the parent, and this is meged with the 
>>>> super-type).
>>>>
>>>>   services:
>>>>   - type: org.apache.brooklyn.entity.stock.BasicApplication
>>>>      brooklyn.config:
>>>>        shell.env:
>>>>          ENV_IN_APP: myEnvInApp
>>>>      brooklyn.children:
>>>>      - type: machine-with-env
>>>>        id: machine1
>>>>        brooklyn.config:
>>>>          shell.env:
>>>>            ENV2: myEnv2
>>>>      - type: machine-with-env
>>>>        id: machine2
>>>>
>>>> The reasoning behind this is to figure out the inheritance/override 
>>>> rules incrementally. We leave the parent-inheritance as-is, and 
>>>> just focus on the sub-typing inheritance.
>>>>
>>>> Note that there is already a ConfigInheritance defined on ConfigKey 
>>>> for controlling this kind of inheritance from the parent. The legal 
>>>> values for ConfigInheritance are currently just ALWAYS and NONE.
>>>>
>>>>
>>>> _*IMPLEMENTATION*_
>>>>
>>>> Clearly we do not want to implement this piecemeal. We'll add a way 
>>>> to declare that a config key should be merged with that value from 
>>>> the super-type.
>>>>
>>>> We'll change the Java ConfigKey code to be:
>>>>
>>>>   public interface ConfigKey {
>>>>      /**
>>>>       * @since 0.10.0
>>>>       */
>>>>      @Nullable ConfigInheritance getParentInheritance();
>>>>
>>>>      /**
>>>>       * @since 0.10.0
>>>>       */
>>>>   @Nullable ConfigInheritance getTypeInheritance();
>>>>
>>>>      /**
>>>>       * @deprecated since 0.10.0; instead use {@link
>>>>   #getParentInheritance()}
>>>>       */
>>>>      @Nullable ConfigInheritance getInheritance();
>>>>   }
>>>>
>>>> We'll add to ConfigInheritance support for MERGE. We'll change the 
>>>> name "ALWAYS" to OVERRIDE (deprecating the old value).
>>>>
>>>> We'll change EntityConfigMap.getConfig to handle this new merge 
>>>> behaviour. And same for locations, policies and enrichers.
>>>>
>>>> Aled
>>>>
>>>>
>>
>
>


--------------5358E36062A148873A76A275--