> On May 12, 2015, 2:45 p.m., John Speidel wrote: > > Looks good although I would prefer that you moved this validation logic to > > BlueprintFactory which is where all of the current validation logic exists. > > Costel Radulescu wrote: > Thanks John, I chose not to put the structure validation logic into the > BlueprintFactory because it would require that the Factory know about either > the requestInfoProperties or the Blueprint in JSON format before hand.
That's fine. - John ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33848/#review83399 ----------------------------------------------------------- On May 14, 2015, 8:53 a.m., Emil Anca wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33848/ > ----------------------------------------------------------- > > (Updated May 14, 2015, 8:53 a.m.) > > > Review request for Ambari, Costel Radulescu, John Speidel, Robert Levas, and > Robert Nettleton. > > > Bugs: AMBARI-10931 > https://issues.apache.org/jira/browse/AMBARI-10931 > > > Repository: ambari > > > Description > ------- > > The Blueprints processor needs to have stronger validation during the process > of handling a POST for a newly-submitted Blueprint from a REST client. > The Blueprints structure requires that the "configurations" element be an > array of Maps, with each Map representing a given configuration type > ("core-site", "storm-site", etc). > Typically, the configurations structure should look like: > ""configurations": [ > { > "mapred-env": { > "properties": > { "mapreduce_log_dir_prefix": "/grid/0/log/hadoop-mapreduce" } > } > }, > { "mapred-site": { > "properties": > { "install-test-mapred-site": "install-test-mapred-site-VALUE" } > } > }, > { "yarn-env": { > "properties": > { "yarn_log_dir_prefix": "/grid/0/log/hadoop" } > } > } > ]" > The snippet above is just an example to illustrate the expected format. > Recent changes to the Blueprint's structure to handle "properties_attributes" > (such as "final") have modified the structure of a Blueprint, so that > configuration properties are now contained in a "properties" map, internal to > each configuration type. Creating new Blueprints using this newer format (the > older format is still acceptable to the processor) require that the > configuration types are still enclosed in a map, even though an internal map > is also present, which may be a source of confusion. > An example of an incorrect format would be: > ""configurations": [ > { > "mapred-env": { > "properties": > { "mapreduce_log_dir_prefix": "/grid/0/log/hadoop-mapreduce" } > }, > "mapred-site": { > "properties": > { "install-test-mapred-site": "install-test-mapred-site-VALUE" } > }, > "yarn-env": { > "properties": > { "yarn_log_dir_prefix": "/grid/0/log/hadoop" } > } > ] > " > Please note that in the example above a single map is created, with all > configuration elements being added to that entry's configuration type. This > causes a corrupted Blueprint to be stored and used for deployments, since all > configuration elements added in the original Blueprint will be registered > under one of the config types used in the original Blueprint. This will cause > many types of cluster startup failures, as the configuration elements will be > added to incorrect locations. > When the Blueprint processor receives a POST submission with a new Blueprint > that includes this incorrect format for "configurations", the processor must > reject this submission, and send back a relevant error code/message, so that > the user can resolve this problem prior to attempting a cluster deployment. > This issue will not affect the older format for Blueprints, and so any > backwards-compatible Blueprints used should work fine. This issue will only > occur when new Blueprints are submitted that use the newer syntax/format for > defining configuration properties (with support for properties_attributes) as > well. > This JIRA is being created to track this issue, as it could potentially be a > user experience issue going forward, and should probably be resolved in > Ambari 2.1, if possible. > > > Problem: Blueprint "configurations" element structure is not validated which > leads to cluster creations error > Solution: The "configuration" element is checked for the correct structure as > a List of Maps containing a single configuration type > > > Diffs > ----- > > > ambari-server/src/main/java/org/apache/ambari/server/controller/internal/BlueprintResourceProvider.java > aab5395 > > ambari-server/src/test/java/org/apache/ambari/server/controller/internal/BlueprintResourceProviderTest.java > 118a7be > > Diff: https://reviews.apache.org/r/33848/diff/ > > > Testing > ------- > > mvn clean test > > > Performed the following functional tests: > > 1) tested that the server returns an error when saving a blueprint with > invalid structure for the "configurations" element > 2) tested that the server saves several correctly structured blueprints: > a) blueprint having an empty "configurations" element > b) blueprint having "configurations" with a single "configuration-type" > element > c) blueprint having "configurations" with multiple "configuration-type" > elements properly structured > > Added new unit tests to account for success scenarions (a) and (b). > > > Thanks, > > Emil Anca > >
