brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (BROOKLYN-286) merge config keys map values, where appropriate
Date Thu, 02 Jun 2016 13:04:59 GMT

    [ https://issues.apache.org/jira/browse/BROOKLYN-286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15312264#comment-15312264
] 

ASF GitHub Bot commented on BROOKLYN-286:
-----------------------------------------

Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/173#discussion_r65534619
  
    --- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/ConfigInheritanceYamlTest.java
---
    @@ -300,14 +338,212 @@ public void testOverridesParentConfig() throws Exception {
                     ImmutableMap.of("mykey2", "myval2", "templateOptions", ImmutableMap.of("myOptionsKey2",
"myOptionsVal2")));
         }
         
    +    @Test
    +    public void testEntityTypeInheritanceOptions() throws Exception {
    +        addCatalogItems(
    +                "brooklyn.catalog:",
    +                "  itemType: entity",
    +                "  items:",
    +                "  - id: entity-with-keys",
    +                "    item:",
    +                "      type: org.apache.brooklyn.core.test.entity.TestEntity",
    +                "      brooklyn.parameters:",
    +                "      - name: map.type-merged",
    +                "        type: java.util.Map",
    +                "        inheritance.type: merge",
    +                "        default: {myDefaultKey: myDefaultVal}",
    +                "      - name: map.type-always",
    +                "        type: java.util.Map",
    +                "        inheritance.type: always",
    +                "        default: {myDefaultKey: myDefaultVal}",
    +                "      - name: map.type-never",
    +                "        type: java.util.Map",
    +                "        inheritance.type: none",
    +                "        default: {myDefaultKey: myDefaultVal}",
    +                "  - id: entity-with-conf",
    +                "    item:",
    +                "      type: entity-with-keys",
    +                "      brooklyn.config:",
    +                "        map.type-merged:",
    +                "          mykey: myval",
    +                "        map.type-always:",
    +                "          mykey: myval",
    +                "        map.type-never:",
    +                "          mykey: myval");
    +        
    +        // Test retrieval of defaults
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: entity-with-keys");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +        }
    +
    +        // Test override defaults
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: entity-with-keys",
    +                    "  brooklyn.config:",
    +                    "    map.type-merged:",
    +                    "      mykey2: myval2",
    +                    "    map.type-always:",
    +                    "      mykey2: myval2",
    +                    "    map.type-never:",
    +                    "      mykey2: myval2");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("mykey2", "myval2"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("mykey2", "myval2"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("mykey2", "myval2"));
    +        }
    +
    +        // Test retrieval of explicit config
    +        // TODO what should "never" mean here? Should we get the default value?
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: entity-with-conf");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("mykey", "myval"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("mykey", "myval"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +        }
    +        
    +        // Test merging/overriding of explicit config
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: entity-with-conf",
    +                    "  brooklyn.config:",
    +                    "    map.type-merged:",
    +                    "      mykey2: myval2",
    +                    "    map.type-always:",
    +                    "      mykey2: myval2",
    +                    "    map.type-never:",
    +                    "      mykey2: myval2");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("mykey", "myval", "mykey2", "myval2"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("mykey2", "myval2"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("mykey2", "myval2"));
    +        }
    +    }
    +    
    +    @Test
    +    public void testParentInheritanceOptions() throws Exception {
    +        addCatalogItems(
    +                "brooklyn.catalog:",
    +                "  itemType: entity",
    +                "  items:",
    +                "  - id: entity-with-keys",
    +                "    item:",
    +                "      type: org.apache.brooklyn.core.test.entity.TestEntity",
    +                "      brooklyn.parameters:",
    +                "      - name: map.type-merged",
    +                "        type: java.util.Map",
    +                "        inheritance.parent: merge",
    +                "        default: {myDefaultKey: myDefaultVal}",
    +                "      - name: map.type-always",
    +                "        type: java.util.Map",
    +                "        inheritance.parent: always",
    +                "        default: {myDefaultKey: myDefaultVal}",
    +                "      - name: map.type-never",
    +                "        type: java.util.Map",
    +                "        inheritance.parent: none",
    +                "        default: {myDefaultKey: myDefaultVal}");
    +        
    +        // Test retrieval of defaults
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: org.apache.brooklyn.entity.stock.BasicApplication",
    +                    "  brooklyn.children:",
    +                    "  - type: entity-with-keys");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +        }
    +
    +        // Test override defaults in parent entity
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: org.apache.brooklyn.entity.stock.BasicApplication",
    +                    "  brooklyn.config:",
    +                    "    map.type-merged:",
    +                    "      mykey: myval",
    +                    "    map.type-always:",
    +                    "      mykey: myval",
    +                    "    map.type-never:",
    +                    "      mykey: myval",
    +                    "  brooklyn.children:",
    +                    "  - type: entity-with-keys");
    +            
    +            Entity app = createStartWaitAndLogApplication(new StringReader(yaml));
    +            TestEntity entity = (TestEntity) Iterables.getOnlyElement(app.getChildren());
    +    
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-merged")),
ImmutableMap.of("mykey", "myval"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-always")),
ImmutableMap.of("mykey", "myval"));
    +            assertEquals(entity.config().get(entity.getEntityType().getConfigKey("map.type-never")),
ImmutableMap.of("myDefaultKey", "myDefaultVal"));
    +        }
    +
    +        // Test merging/overriding of explicit config
    +        {
    +            String yaml = Joiner.on("\n").join(
    +                    "services:",
    +                    "- type: org.apache.brooklyn.entity.stock.BasicApplication",
    +                    "  brooklyn.config:",
    +                    "    map.type-merged:",
    +                    "      mykey: myval",
    +                    "    map.type-always:",
    +                    "      mykey: myval",
    +                    "    map.type-never:",
    +                    "      mykey: myval",
    +                    "  brooklyn.children:",
    +                    "  - type: entity-with-keys",
    +                    "    brooklyn.config:",
    +                    "      map.type-merged:",
    +                    "        mykey2: myval2",
    +                    "      map.type-always:",
    +                    "        mykey2: myval2",
    +                    "      map.type-never:",
    +                    "        mykey2: myval2");
    --- End diff --
    
    If 3 levels within a map, then if merging it will do a "deep merge", as per `CollectionMerger`.
I agree it's not clear what a user would expect. I suspect some users would think it will
be shallow, and other users will think deep (depending on the particularly values in the config
map, presumably).
    
    Or do you mean 3 levels of entity (e.g. A extends B, and C extends B)? Then the rules
apply at each level: B will merge or override the values of A, and then C will merge or override
those values of B (based on the config key definition).


> merge config keys map values, where appropriate
> -----------------------------------------------
>
>                 Key: BROOKLYN-286
>                 URL: https://issues.apache.org/jira/browse/BROOKLYN-286
>             Project: Brooklyn
>          Issue Type: New Feature
>    Affects Versions: 0.9.0
>            Reporter: Aled Sage
>
> See the email discussion on dev@brooklyn.apache.org, subject "[PROPOSAL] merging config
keys", initially kicked off 25/05/2016, 12:12.
> Below is a copy of that initial proposal, which has then been further discussed on the
mailing list.
> 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.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message