You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by mmiklavc <gi...@git.apache.org> on 2017/09/15 01:30:32 UTC
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
GitHub user mmiklavc opened a pull request:
https://github.com/apache/metron/pull/760
METRON-1188: Ambari global configuration management broken
## Contributor Comments
This PR addresses https://issues.apache.org/jira/browse/METRON-1188
There are a number of issues with Ambari configuration management with the Metron configuration stored in Zookeeper. I wanted to get this PR out for discussion while I finish testing/tweaking docs around this because this is a fairly major defect. This is one part new features, one part defect fixes. Here's the quick gist:
- Added ability to patch JSON configs per RFC-6902
- Added ability to patch/push/pull/dump individual config types, e.g. GLOBAL, PARSERS, ENRICHMENTS, etc.
- Added ability to patch/push/pull/dump individual configuration names given a specific config type, e.g. PARSERS + 'bro'
- Fixed the order of operations around configuration refresh
- On first service start, these steps are performed
- push zookeeper config (initialize zookeeper configs)
- run the start/restart commands
- set "configured" flag so this initial setup doesn't run again
- On service start or restart
- create a patch file for the global config in /tmp
- read global config from zookeeper, apply patch via new CLI zk_load_configs.sh option (note: I am pretty print formatting the JSON on writing to Zookeeper. ConfigurationManager is a bit of a monstrosity right now, and we should probably clean it up at some point. There are still some modes (PUSH) that miss the pretty print code and I'll work to fix those up also)
- pull zookeeper config locally
- Added timestamps in the "configured" flag files. Yes, I know you can stat a file, but I like explicitly stating in the file when Ambari suggests it wrote to disk :)
- Ambari now patches configuration in an additive way. Note: Any specific configs managed by Ambari will overwrite user changes to those parameters. Here's the patch excerpt from metron_service.py
```
patch_template = """
[
{
"op": "add",
"path": "/es.clustername",
"value": "{{ es_cluster_name }}"
},
{
"op": "add",
"path": "/es.ip",
"value": "{{ es_url }}"
},
{
"op": "add",
"path": "/es.date.format",
"value": "{{es_date_format}}"
},
{
"op": "add",
"path": "/parser.error.topic",
"value": "{{parser_error_topic}}"
},
{
"op": "add",
"path": "/update.hbase.table",
"value": "{{update_hbase_table}}"
},
{
"op": "add",
"path": "/update.hbase.cf",
"value": "{{update_hbase_cf}}"
},
{
"op": "add",
"path": "/profiler.client.period.duration",
"value": "{{profiler_period_duration}}"
},
{
"op": "add",
"path": "/profiler.client.period.duration.units",
"value": "{{profiler_period_units}}"
}
]
"""
```
- removed the global.json.j2 template file and moved a default global.json to metron-common. Modified the assembly.xml and the RPM metron.spec to included this accordingly also.
- Fixed global config not updating based on Ambari parameters - look at the Metron status_params.py file to see the changes. Also updated the parameter names to be consistent and modified metron-env.xml and metron-enrichment.xml to properly include these params as well.
A friendlier, detailed testing plan for this is coming very soon. Minimally, you should be able to spin up full dev without it failing. A green deployment is a major indicator we're looking good. I'm currently working through Kerberos testing and various restart combinations. Any of the parameters in that patch list above are good candidates to mess around with in Ambari to ensure they are cascaded to both Zookeeper and the local file system. Also check that any new parameters pushed from a local global.json change to Zookeeper, e.g. "blah" : "bloop", are not blown away by restarting Metron services.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [ ] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mmiklavc/metron METRON-1188
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/760.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #760
----
commit 3d9b49b63e03d34d1fea230647036671ea88129b
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-01T01:46:35Z
Add ability to patch Zookeeper config
commit a64c59363ec49bdd073a5e1bb171da95712205b5
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-12T00:23:28Z
Configuration management working with JSON configuration patching. Modify ConfigurationManager to handle
commit 012c37ee8c7029d290dbf994c81c0400b133963e
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-12T21:45:46Z
Merge with master
commit d4d7d4917db7e40b8e2a6010f2c9f4753980392e
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T01:04:58Z
METRON-1188: Ambari global configuration management broken
----
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
GitHub user mmiklavc reopened a pull request:
https://github.com/apache/metron/pull/760
METRON-1188: Ambari global configuration management broken
## Contributor Comments
This PR addresses https://issues.apache.org/jira/browse/METRON-1188
It also references, but does not address all, the concerns/issues brought up in this discuss thread - http://mail-archives.apache.org/mod_mbox/metron-dev/201701.mbox/%3CCAPpQHK1svDAx7R-s7x2Q0kJR6=5d+KDmR6a+ZmSo-5=WCmofyQ@mail.gmail.com%3E
There are a number of issues with Ambari configuration management with the Metron configuration stored in Zookeeper. I wanted to get this PR out for discussion while I finish testing/tweaking docs around this because this is a fairly major defect. This is one part new features, one part defect fixes. Here's the quick gist:
- Added ability to patch JSON configs per RFC-6902
- Added ability to patch/push/pull/dump individual config types, e.g. GLOBAL, PARSERS, ENRICHMENTS, etc.
- Added ability to patch/push/pull/dump individual configuration names given a specific config type, e.g. PARSERS + 'bro'
- Fixed the order of operations around configuration refresh
- On first service start, these steps are performed
- push zookeeper config (initialize zookeeper configs)
- run the start/restart commands
- set "configured" flag so this initial setup doesn't run again
- On service start or restart
- create a patch file for the global config in /tmp
- read global config from zookeeper, apply patch via new CLI zk_load_configs.sh option (note: I am pretty print formatting the JSON on writing to Zookeeper. ConfigurationManager is a bit of a monstrosity right now, and we should probably clean it up at some point. There are still some modes (PUSH) that miss the pretty print code and I'll work to fix those up also)
- pull zookeeper config locally
- Added timestamps in the "configured" flag files. Yes, I know you can stat a file, but I like explicitly stating in the file when Ambari suggests it wrote to disk :)
- Ambari now patches configuration in an additive way. Note: Any specific configs managed by Ambari will overwrite user changes to those parameters. Here's the patch excerpt from metron_service.py
```
patch_template = """
[
{
"op": "add",
"path": "/es.clustername",
"value": "{{ es_cluster_name }}"
},
{
"op": "add",
"path": "/es.ip",
"value": "{{ es_url }}"
},
{
"op": "add",
"path": "/es.date.format",
"value": "{{es_date_format}}"
},
{
"op": "add",
"path": "/parser.error.topic",
"value": "{{parser_error_topic}}"
},
{
"op": "add",
"path": "/update.hbase.table",
"value": "{{update_hbase_table}}"
},
{
"op": "add",
"path": "/update.hbase.cf",
"value": "{{update_hbase_cf}}"
},
{
"op": "add",
"path": "/profiler.client.period.duration",
"value": "{{profiler_period_duration}}"
},
{
"op": "add",
"path": "/profiler.client.period.duration.units",
"value": "{{profiler_period_units}}"
}
]
"""
```
- removed the global.json.j2 template file and moved a default global.json to metron-common. Modified the assembly.xml and the RPM metron.spec to included this accordingly also.
- Fixed global config not updating based on Ambari parameters - look at the Metron status_params.py file to see the changes. Also updated the parameter names to be consistent and modified metron-env.xml and metron-enrichment.xml to properly include these params as well.
A friendlier, detailed testing plan for this is coming very soon. Minimally, you should be able to spin up full dev without it failing. A green deployment is a major indicator we're looking good. I'm currently working through Kerberos testing and various restart combinations. Any of the parameters in that patch list above are good candidates to mess around with in Ambari to ensure they are cascaded to both Zookeeper and the local file system. Also check that any new parameters pushed from a local global.json change to Zookeeper, e.g. "blah" : "bloop", are not blown away by restarting Metron services.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [ ] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mmiklavc/metron METRON-1188
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/760.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #760
----
commit 3d9b49b63e03d34d1fea230647036671ea88129b
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-01T01:46:35Z
Add ability to patch Zookeeper config
commit a64c59363ec49bdd073a5e1bb171da95712205b5
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-12T00:23:28Z
Configuration management working with JSON configuration patching. Modify ConfigurationManager to handle
commit 012c37ee8c7029d290dbf994c81c0400b133963e
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-12T21:45:46Z
Merge with master
commit d4d7d4917db7e40b8e2a6010f2c9f4753980392e
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T01:04:58Z
METRON-1188: Ambari global configuration management broken
commit 4538a560d9bd4a7465e3696b8e409ad4824d36b8
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T02:28:23Z
Merge branch 'master' into patch-config
commit 774252fbbf7ae44f0831828682fd3851724f4721
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T16:36:14Z
Fix unit test
commit 91b8953e85c2cde9e77173aee1cd2635446b59d1
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T17:23:20Z
Address changed lib version for zjsonpatch. Address one of the coding style comments from justinleet
commit f9348d08d0de6c03d97a06eff57dc0eeb955b940
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-15T22:43:30Z
Address review comments
commit e11cec4643a4405e4708e49c428c1ac543bef119
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-16T01:07:00Z
Resolve merge conflicts with master
commit abda8da3e1903fc20d89c9d0b92ed58350398cd9
Author: Michael Miklavcic <mi...@gmail.com>
Date: 2017-09-16T06:02:16Z
Fix enrichment template in metron-enrichment, not the mpack itself
----
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139172458
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -401,7 +530,16 @@ else if (configType.equals(PARSER) || configType.equals(ENRICHMENT) || configTyp
public static void dumpConfigs(PrintStream out, CuratorFramework client) throws Exception {
ConfigurationsUtils.visitConfigs(client, (type, name, data) -> {
type.deserialize(data);
- out.println(type + " Config: " + name + "\n" + data);
+ out.println(type + " Config: " + name + System.lineSeparator() + data);
});
}
+
+ public static void dumpConfigs(PrintStream out, CuratorFramework client,
--- End diff --
Javadocs while you're already in here, if you don't mind.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/760
Couple notes on the test instructions for anyone trying this out.
* Missing the input dir flag on the PUSH instructions, just need -i:
`$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c GLOBAL -i $METRON_HOME/config/zookeeper`
`$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c PARSER -n bro -i $METRON_HOME/config/zookeeper`
* The flag for files isn't `-pf`, it's `-pp` for patch path
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/760
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/760
@mmiklavc I will spin this up and get cozy with it.
> A friendlier, detailed testing plan for this is coming very soon.
Have you been able to get a test plan together? That would help me understand what I should expect from the PR.
As you know tackling configuration is a big thing and will take a few iterations. We know this is good step in the right direction, but I want to make sure that I understand exactly what this PR does and (more importantly) what it does not do.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
> Can you reference : http://mail-archives.apache.org/mod_mbox/metron-dev/201701.mbox/%3cCAPpQHK1svDAx7R-s7x2Q0kJR6=5d+KDmR6a+ZmSo-5=WCmofyQ@mail.gmail.com%3e
> Which I think is the discuss list?
Not sure I follow - you want me to reference it in the docs? This doesn't change the current paradigm of Zookeeper being the source of truth at all times.
> Is this a standard way of working between ambari and zookeeper or a new take?
There isn't really a standard way of managing configs in Zookeeper in Ambari that I'm aware of. This is mostly fixing a gap in how we handle materializing configs from the properties that are managed via the Ambari interface.
> I have not found it this morning, but my recollection is we did some documentation around ambari and what we do there, do you plan on putting in something about this for maintainers etc?
Yes, completely agreed. Per my initial PR comments this workflow should definitely be explained so the implications of making config changes from local disk, Ambari, Zookeeper, and the management UI are well understood. I have to look for where we put that. While I'm addressing some other code review comments, if anyone happens to know where off the top of their head, that would be helpful.
> Do you plan on integrating this into the feature branch yourself? Or were you planning on me doing it?
This is backwards compatible with existing config management. The only major change would be around Ambari:
1. actually writing the global config from properties held in the Ambari UI (as opposed to the hard-coded values) and
2. using a patching (additive in nature, overwrite in the event of property conflict) mechanism
3. doing a zk config pull to local disk after any service starts/restarts.
What problems with integration are you expecting? There aren't any new classes introduced.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139086639
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -76,7 +80,7 @@ public static void writeProfilerConfigToZookeeper(byte[] config, CuratorFramewor
}
public static void writeSensorParserConfigToZookeeper(String sensorType, SensorParserConfig sensorParserConfig, String zookeeperUrl) throws Exception {
- writeSensorParserConfigToZookeeper(sensorType, JSONUtils.INSTANCE.toJSON(sensorParserConfig), zookeeperUrl);
+ writeSensorParserConfigToZookeeper(sensorType, JSONUtils.INSTANCE.toJSONPretty(sensorParserConfig), zookeeperUrl);
--- End diff --
So, I think the test is failing because you are pretty printing here. `GlobalConfigServiceImplTest` functions are expecting to write out `{}` and retrieve back literally `{}` and, yet, you are pretty printing, which would yield `{ }`. I think what you're doing is fine, but you probably want to adjust the failing `GlobalConfigServiceImplTest` cases to expect `{ }` rather than `{}`.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139172028
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -346,19 +434,56 @@ public static void setupStellarStatically(CuratorFramework client, Optional<Stri
}
public static Map<String, byte[]> readSensorConfigsFromFile(String rootPath, ConfigurationType configType) throws IOException {
+ return readSensorConfigsFromFile(rootPath, configType, Optional.empty());
+ }
+
+ public static Map<String, byte[]> readSensorConfigsFromFile(String rootPath,
+ ConfigurationType configType, Optional<String> configName) throws IOException {
Map<String, byte[]> sensorConfigs = new HashMap<>();
File configPath = new File(rootPath, configType.getDirectory());
- if (configPath.exists()) {
+ if (configPath.exists() && configPath.isDirectory()) {
File[] children = configPath.listFiles();
- if (children != null) {
+ if (!configName.isPresent()) {
for (File file : children) {
- sensorConfigs.put(FilenameUtils.removeExtension(file.getName()), Files.readAllBytes(file.toPath()));
+ sensorConfigs.put(FilenameUtils.removeExtension(file.getName()),
+ Files.readAllBytes(file.toPath()));
+ }
+ } else {
+ for (File file : children) {
+ if (FilenameUtils.removeExtension(file.getName()).equals(configName.get())) {
+ sensorConfigs.put(FilenameUtils.removeExtension(file.getName()),
+ Files.readAllBytes(file.toPath()));
+ }
+ }
+ if (sensorConfigs.isEmpty()) {
+ throw new RuntimeException("Unable to find configuration for " + configName.get());
}
}
}
return sensorConfigs;
}
+ public static void applyConfigPatchToZookeeper(ConfigurationType configurationType, byte[] patchData, String zookeeperUrl) throws Exception {
--- End diff --
Javadocs again.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
I also ran this through testing with Kerberos enabled in full dev.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139160248
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/enrichment_commands.py ---
@@ -55,14 +56,14 @@ def is_kafka_acl_configured(self):
def set_kafka_configured(self):
Logger.info("Setting Kafka Configured to True")
File(self.__params.enrichment_kafka_configured_flag_file,
- content="",
+ content="This file created on: " + datetime.now().strftime('%Y-%m-%d %H:%M:%S'),
--- End diff --
Mostly thinking out loud about a follow on, would it be worth it to just pull these file creations (and probably a variant of the Logger statement) into a `createConfiguredFlagFile` function in the common area, and just pass it the flag file? They all have the same content, owner, and perms, right?
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139171388
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type) throws Exception {
+ uploadConfigsToZookeeper(rootFilePath, client, type, Optional.empty());
+ }
+
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type, Optional<String> configName) throws Exception {
+ switch (type) {
+ case GLOBAL:
+ final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath);
+ if (globalConfig.length > 0) {
+ setupStellarStatically(client, Optional.of(new String(globalConfig)));
+ writeGlobalConfigToZookeeper(readGlobalConfigFromFile(rootFilePath), client);
+ }
+ break;
+ case PARSER:
+ Map<String, byte[]> sensorParserConfigs = readSensorConfigsFromFile(rootFilePath, PARSER, configName);
+ for (String sensorType : sensorParserConfigs.keySet()) {
+ writeConfigToZookeeper(type, configName, sensorParserConfigs.get(sensorType), client);
+ }
+ break;
+ case ENRICHMENT:
+ Map<String, byte[]> sensorEnrichmentConfigs = readSensorConfigsFromFile(rootFilePath, ENRICHMENT, configName);
--- End diff --
Incredibly nitpicky, but the `readSensorConfigsFromFile` are over the line limit.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139170907
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type) throws Exception {
+ uploadConfigsToZookeeper(rootFilePath, client, type, Optional.empty());
+ }
+
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type, Optional<String> configName) throws Exception {
+ switch (type) {
+ case GLOBAL:
+ final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath);
+ if (globalConfig.length > 0) {
+ setupStellarStatically(client, Optional.of(new String(globalConfig)));
+ writeGlobalConfigToZookeeper(readGlobalConfigFromFile(rootFilePath), client);
+ }
+ break;
+ case PARSER:
--- End diff --
Couldn't this just be:
```
case PARSER:
case ENRICHMENT:
case INDEXING:
Map<String, byte[]> sensorConfigs = readSensorConfigsFromFile(rootFilePath, type, configName);
for (String sensorType : sensorConfigs.keySet()) {
writeConfigToZookeeper(type, configName, sensorConfigs.get(sensorType), client);
}
break;
```
Although I would ask that if we do that, that a comment indicating fallthrough is intended is added.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Kick?
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/760
Why would those .j2 files be in gitignore?
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139194237
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
--- End diff --
Yes
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
@justinleet One of my earlier commits erroneously triggered the Github "outdated comment" feature on some of your requests for javadocs. I've gone back through and added them.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/metron/pull/760
Spun up fine now
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139526927
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/cli/ConfigurationManager.java ---
@@ -130,29 +201,39 @@ public void pull(CuratorFramework client, String outFileStr, final boolean force
public void visit(ConfigurationType configurationType, String name, String data) {
File out = getFile(outputDir, configurationType, name);
if (!out.exists() || force) {
- if(!out.exists()) {
+ if (!out.exists()) {
out.getParentFile().mkdirs();
}
try {
Files.write(data, out, Charset.defaultCharset());
} catch (IOException e) {
throw new RuntimeException("Sorry, something bad happened writing the config to " + out.getAbsolutePath() + ": " + e.getMessage(), e);
}
- }
- else if(out.exists() && !force) {
+ } else if (out.exists() && !force) {
throw new IllegalStateException("Unable to overwrite existing file (" + out.getAbsolutePath() + ") without the force flag (-f or --force) being set.");
}
}
});
}
public void push(String inputDirStr, CuratorFramework client) throws Exception {
- final File inputDir = new File(inputDirStr);
+ final File inputDir = new File(inputDirStr);
--- End diff --
The examples in the test plan don't work directly because they NPE here. I assume it's because inputDirStr is null here and we only check for validity after wrapping it in a `File`. While you're in here, could you add a check (and meaningful exception) for the case where inputDirStr is null?
Error is:
```
[root@node1 zookeeper]# $METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c GLOBAL
Exception in thread "main" java.lang.NullPointerException
at java.io.File.<init>(File.java:277)
at org.apache.metron.common.cli.ConfigurationManager.push(ConfigurationManager.java:230)
at org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:256)
at org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:242)
at org.apache.metron.common.cli.ConfigurationManager.main(ConfigurationManager.java:350)
```
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139194942
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type) throws Exception {
+ uploadConfigsToZookeeper(rootFilePath, client, type, Optional.empty());
+ }
+
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type, Optional<String> configName) throws Exception {
+ switch (type) {
+ case GLOBAL:
+ final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath);
+ if (globalConfig.length > 0) {
+ setupStellarStatically(client, Optional.of(new String(globalConfig)));
+ writeGlobalConfigToZookeeper(readGlobalConfigFromFile(rootFilePath), client);
+ }
+ break;
+ case PARSER:
+ Map<String, byte[]> sensorParserConfigs = readSensorConfigsFromFile(rootFilePath, PARSER, configName);
+ for (String sensorType : sensorParserConfigs.keySet()) {
+ writeConfigToZookeeper(type, configName, sensorParserConfigs.get(sensorType), client);
+ }
+ break;
+ case ENRICHMENT:
+ Map<String, byte[]> sensorEnrichmentConfigs = readSensorConfigsFromFile(rootFilePath, ENRICHMENT, configName);
--- End diff --
Will fix this also.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/760
I spun this up in full dev and it worked really well. I think it's a great step towards cleaning up our config management. I'm still looking over some of the review updates and discussion, but this is a great.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
You're never going to believe this, but I found our Ambari docs under, wait for it, the ambari project in packaging. https://github.com/apache/metron/tree/master/metron-deployment/packaging/ambari. Thanks @justinleet and @merrimanr for writing that doc.
---
Re: [GitHub] metron issue #760: METRON-1188: Ambari global
configuration management broke...
Posted by "Zeolla@GMail.com" <ze...@gmail.com>.
Spun up fine now, thanks.
On Tue, Sep 19, 2017, 14:09 mmiklavc <gi...@git.apache.org> wrote:
> Github user mmiklavc commented on the issue:
>
> https://github.com/apache/metron/pull/760
>
> Ahhhh @JonZeolla fixing it now. Sorry about that - I missed one of the
> "patch_path -> patch_file" arg changes in the mpack.
>
>
> ---
>
--
Jon
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Ahhhh @JonZeolla fixing it now. Sorry about that - I missed one of the "patch_path -> patch_file" arg changes in the mpack.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Found a possible problem. I was wondering why my changes aren't making it into my commits for the enrichment properties. There's this
https://github.com/apache/metron/blob/master/metron-deployment/packaging/ambari/.gitignore
but also this
https://github.com/apache/metron/blob/master/metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/enrichment_master.py#L41
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139550653
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/cli/ConfigurationManager.java ---
@@ -130,29 +201,39 @@ public void pull(CuratorFramework client, String outFileStr, final boolean force
public void visit(ConfigurationType configurationType, String name, String data) {
File out = getFile(outputDir, configurationType, name);
if (!out.exists() || force) {
- if(!out.exists()) {
+ if (!out.exists()) {
out.getParentFile().mkdirs();
}
try {
Files.write(data, out, Charset.defaultCharset());
} catch (IOException e) {
throw new RuntimeException("Sorry, something bad happened writing the config to " + out.getAbsolutePath() + ": " + e.getMessage(), e);
}
- }
- else if(out.exists() && !force) {
+ } else if (out.exists() && !force) {
throw new IllegalStateException("Unable to overwrite existing file (" + out.getAbsolutePath() + ") without the force flag (-f or --force) being set.");
}
}
});
}
public void push(String inputDirStr, CuratorFramework client) throws Exception {
- final File inputDir = new File(inputDirStr);
+ final File inputDir = new File(inputDirStr);
--- End diff --
Latest commit addresses the input dir null check.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/760
A bit on the second point: I would argue this is a new (more correct) take if, for no other reason, that there isn't to my my knowledge other patterns to emulate here (other projects don't let ambari have control over their zk configs). The problem was letting ambari own the whole global config on disk in the first place via the templating system. This moves to a more rational approach of just having ambari apply patches to the config within the global config that it owns, rather than controlling the whole file/global config.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139190738
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -76,7 +80,7 @@ public static void writeProfilerConfigToZookeeper(byte[] config, CuratorFramewor
}
public static void writeSensorParserConfigToZookeeper(String sensorType, SensorParserConfig sensorParserConfig, String zookeeperUrl) throws Exception {
- writeSensorParserConfigToZookeeper(sensorType, JSONUtils.INSTANCE.toJSON(sensorParserConfig), zookeeperUrl);
+ writeSensorParserConfigToZookeeper(sensorType, JSONUtils.INSTANCE.toJSONPretty(sensorParserConfig), zookeeperUrl);
--- End diff --
Yeah, I hastily read that stacktrace. This was indeed the problem.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Just pushed out a commit to address comments. I added documentation around the config management lifecycle to the Ambari README. I'm continuing with testing and will verify Kerberos works as expected also. More testing details to follow. @justinleet I'm looking also at options for added logging per your comments above.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by JonZeolla <gi...@git.apache.org>.
Github user JonZeolla commented on the issue:
https://github.com/apache/metron/pull/760
This is failing to spin up for me. As a part of Metron Enrichment Start, during:
```
2017-09-19 18:05:54,844 - Execute['/usr/metron/0.4.1/bin/zk_load_configs.sh --zk_quorum node1:2181 --mode PATCH --config_type GLOBAL --patch_path /tmp/metron-global-config-patch.json'] {'path': ['/usr/jdk64/jdk1.8.0_77/bin']}
```
I get
```
Traceback (most recent call last):
File "/var/lib/ambari-agent/cache/common-services/METRON/0.4.1/package/scripts/enrichment_master.py", line 117, in <module>
Enrichment().execute()
File "/usr/lib/python2.6/site-packages/resource_management/libraries/script/script.py", line 280, in execute
method(env)
File "/var/lib/ambari-agent/cache/common-services/METRON/0.4.1/package/scripts/enrichment_master.py", line 58, in start
self.configure(env)
File "/var/lib/ambari-agent/cache/common-services/METRON/0.4.1/package/scripts/enrichment_master.py", line 50, in configure
metron_service.refresh_configs(params)
File "/var/lib/ambari-agent/cache/common-services/METRON/0.4.1/package/scripts/metron_service.py", line 129, in refresh_configs
patch_global_config(params)
File "/var/lib/ambari-agent/cache/common-services/METRON/0.4.1/package/scripts/metron_service.py", line 111, in patch_global_config
path=ambari_format("{java_home}/bin")
File "/usr/lib/python2.6/site-packages/resource_management/core/base.py", line 155, in __init__
self.env.run()
File "/usr/lib/python2.6/site-packages/resource_management/core/environment.py", line 160, in run
self.run_action(resource, action)
File "/usr/lib/python2.6/site-packages/resource_management/core/environment.py", line 124, in run_action
provider_action()
File "/usr/lib/python2.6/site-packages/resource_management/core/providers/system.py", line 273, in action_run
tries=self.resource.tries, try_sleep=self.resource.try_sleep)
File "/usr/lib/python2.6/site-packages/resource_management/core/shell.py", line 70, in inner
result = function(command, **kwargs)
File "/usr/lib/python2.6/site-packages/resource_management/core/shell.py", line 92, in checked_call
tries=tries, try_sleep=try_sleep)
File "/usr/lib/python2.6/site-packages/resource_management/core/shell.py", line 140, in _call_wrapper
result = _call(command, **kwargs_copy)
File "/usr/lib/python2.6/site-packages/resource_management/core/shell.py", line 293, in _call
raise ExecutionFailed(err_msg, code, out, err)
resource_management.core.exceptions.ExecutionFailed: Execution of '/usr/metron/0.4.1/bin/zk_load_configs.sh --zk_quorum node1:2181 --mode PATCH --config_type GLOBAL --patch_path /tmp/metron-global-config-patch.json' returned 255. Unable to parse args: --zk_quorum node1:2181 --mode PATCH --config_type GLOBAL --patch_path /tmp/metron-global-config-patch.json
org.apache.commons.cli.UnrecognizedOptionException: Unrecognized option: --patch_path
at org.apache.commons.cli.Parser.processOption(Parser.java:363)
at org.apache.commons.cli.Parser.parse(Parser.java:199)
at org.apache.commons.cli.Parser.parse(Parser.java:85)
at org.apache.metron.common.cli.ConfigurationManager$ConfigurationOptions.parse(ConfigurationManager.java:140)
at org.apache.metron.common.cli.ConfigurationManager.main(ConfigurationManager.java:352)
usage: configuration_manager
-c,--config_type <CONFIG_TYPE> The configuration type: GLOBAL,
PARSER, ENRICHMENT, INDEXING,
PROFILER
-f,--force Force operation
-h,--help Generate Help screen
-i,--input_dir <DIR> The input directory containing
the configuration files named
like "$source.json"
-m,--mode <MODE> The mode of operation: DUMP,
PULL, PUSH, PATCH
-n,--config_name <CONFIG_NAME> The configuration name: bro,
yaf, snort, squid, etc.
-o,--output_dir <DIR> The output directory which will
store the JSON configuration
from Zookeeper
-pf,--patch_file <PATCH_FILE> Path to the patch file.
-pk,--patch_key <PATCH_KEY> The key to modify
-pm,--patch_mode <PATCH_MODE> One of: ADD, REMOVE - relevant
only for key/value patches,
i.e. when a patch file is not
used.
-pv,--patch_value <PATCH_VALUE> Value to use in the patch.
-z,--zk_quorum <host:port,[host:port]*> Zookeeper Quorum URL
(zk1:port,zk2:port,...)
```
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139547949
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/cli/ConfigurationManager.java ---
@@ -130,29 +201,39 @@ public void pull(CuratorFramework client, String outFileStr, final boolean force
public void visit(ConfigurationType configurationType, String name, String data) {
File out = getFile(outputDir, configurationType, name);
if (!out.exists() || force) {
- if(!out.exists()) {
+ if (!out.exists()) {
out.getParentFile().mkdirs();
}
try {
Files.write(data, out, Charset.defaultCharset());
} catch (IOException e) {
throw new RuntimeException("Sorry, something bad happened writing the config to " + out.getAbsolutePath() + ": " + e.getMessage(), e);
}
- }
- else if(out.exists() && !force) {
+ } else if (out.exists() && !force) {
throw new IllegalStateException("Unable to overwrite existing file (" + out.getAbsolutePath() + ") without the force flag (-f or --force) being set.");
}
}
});
}
public void push(String inputDirStr, CuratorFramework client) throws Exception {
- final File inputDir = new File(inputDirStr);
+ final File inputDir = new File(inputDirStr);
--- End diff --
Yeah, I think we could definitely handle the scripts better in general.
I'm not opposed to just leaving this as-is, but it seems like a decent / small opportunity to make it more usable. Up to you.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
@justinleet Looks like you found the missing -i option. Also, it's funny that I confirmed "pp" and still ended up typing "pf." While I'm in here, what do you think is better, pp or pf? I'm happy to change it.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/760
So, it is not uncommon for a PR that has a discussion thread backing up it's approach to reference that thread to point out the public discussion when it has occurred. Since this thread exists, it should be referenced in the jira or the PR.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/760
Thanks for making the adjustments, @mmiklavc. I'm +1 on this. It's a great step forward and gives a good foundation for future improvements we want to make.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139170122
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
--- End diff --
Can you add Javadocs to these methods?
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
@ottobackwards Added the mailing list ref
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139194807
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -226,6 +276,44 @@ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramewor
uploadConfigsToZookeeper(rootFilePath, rootFilePath, rootFilePath, rootFilePath, rootFilePath, client);
}
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type) throws Exception {
+ uploadConfigsToZookeeper(rootFilePath, client, type, Optional.empty());
+ }
+
+ public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
+ ConfigurationType type, Optional<String> configName) throws Exception {
+ switch (type) {
+ case GLOBAL:
+ final byte[] globalConfig = readGlobalConfigFromFile(rootFilePath);
+ if (globalConfig.length > 0) {
+ setupStellarStatically(client, Optional.of(new String(globalConfig)));
+ writeGlobalConfigToZookeeper(readGlobalConfigFromFile(rootFilePath), client);
+ }
+ break;
+ case PARSER:
--- End diff --
Ha, yes. Good catch, I missed refactoring that piece. Red, green, refactor Mike! Red, green, refactor!
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139171727
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java ---
@@ -346,19 +434,56 @@ public static void setupStellarStatically(CuratorFramework client, Optional<Stri
}
public static Map<String, byte[]> readSensorConfigsFromFile(String rootPath, ConfigurationType configType) throws IOException {
+ return readSensorConfigsFromFile(rootPath, configType, Optional.empty());
+ }
+
+ public static Map<String, byte[]> readSensorConfigsFromFile(String rootPath,
--- End diff --
Would you mind adding Javadocs while you're already in the functions?
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Hm, I'll have to look into that build failure. I don't think I touched anything in the profiler, but that's where the test failures are.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139532867
--- Diff: metron-platform/metron-common/src/main/java/org/apache/metron/common/cli/ConfigurationManager.java ---
@@ -130,29 +201,39 @@ public void pull(CuratorFramework client, String outFileStr, final boolean force
public void visit(ConfigurationType configurationType, String name, String data) {
File out = getFile(outputDir, configurationType, name);
if (!out.exists() || force) {
- if(!out.exists()) {
+ if (!out.exists()) {
out.getParentFile().mkdirs();
}
try {
Files.write(data, out, Charset.defaultCharset());
} catch (IOException e) {
throw new RuntimeException("Sorry, something bad happened writing the config to " + out.getAbsolutePath() + ": " + e.getMessage(), e);
}
- }
- else if(out.exists() && !force) {
+ } else if (out.exists() && !force) {
throw new IllegalStateException("Unable to overwrite existing file (" + out.getAbsolutePath() + ") without the force flag (-f or --force) being set.");
}
}
});
}
public void push(String inputDirStr, CuratorFramework client) throws Exception {
- final File inputDir = new File(inputDirStr);
+ final File inputDir = new File(inputDirStr);
--- End diff --
That was actually me missing the "-i" parameter in the test script. I've modified the test accordingly. In general we should be better guarding user inputs, e.g. some may have noticed that help never actually worked without throwing an exception. I could add a check there also. I'm inclined to rewrite our configuration classes altogether as I think they can be greatly simplified.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Just pushed a unit test fix to get the build working again. The other code changes and doc requests from @ottobackwards and @justinleet soon to follow. Thanks for the feedback guys! @ottobackwards let me know how I can help wrt the feature branch and these changes.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/760
Thanks for the contribution! A couple of questions before I try to review:
- Can you reference : http://mail-archives.apache.org/mod_mbox/metron-dev/201701.mbox/%3cCAPpQHK1svDAx7R-s7x2Q0kJR6=5d+KDmR6a+ZmSo-5=WCmofyQ@mail.gmail.com%3e
Which I think is the discuss list?
- Is this a standard way of working between ambari and zookeeper or a new take?
- I have not found it this morning, but my recollection is we did some documentation around ambari and what we do there, do you plan on putting in something about this for maintainers etc?
- Do you plan on integrating this into the feature branch yourself? Or were you planning on me doing it?
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
Merge conflicts, of course. Unfortunately, I have an appointment and will be back in a couple hours to fix.
---
[GitHub] metron issue #760: METRON-1188: Ambari global configuration management broke...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/760
### Test Plan
1. Run up full dev and verify you have data flowing through to the indexes
2. Change global config options.
1. Go into the Ambari management UI and modify any of the following config options. e.g. set the es_clustername property to "ivebeenchanged". These are values that end up in global.json and will prompt for a component restart. Restart the components.
- es.clustername
- es.ip
- es.date.format
- parser.error.topic
- update.hbase.table
- update.hbase.cf
- profiler.client.period.duration
- profiler.client.period.duration.units
2. Verify $METRON_HOME/config/zookeeper/global.json now has the new value. For the example above, you would expect to see es.clustername=ivebeenchanged.
3. Verify Zookeeper contains the new config.
`$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c GLOBAL`
3. Test changing configs from the command line. The zk config utils has been updated with new backwards compatible features, so we'll test that here.
1. Change one of the zookeeper configs locally and then try a vanilla config PUSH using the original method. Verify the values made it into zookeeper.
`${METRON_HOME}/bin/zk_load_configs.sh -i ${METRON_HOME}/config/zookeeper -m PUSH -z $ZOOKEEPER`
2. Change a config locally and push JUST that config to Zookeeper by using one of the new config loader options. You can then perform a DUMP in a similar manner.
```
# global config only
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c GLOBAL
# bro config only
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c PARSER -n bro
# take a dump of config
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c GLOBAL
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PARSER -n bro
```
3. Try the new JSON patch mechanism with key/value from the command line. Below are some examples but please feel free to explore other combinations. **Note** - commons cli strips the first outermost pair of quotes from all arguments, so you have to provide escaped quotes twice. Neither \""bar"\" nor "\"bar\"" will work. You must provide both escaped quotes for string values. \"\"bar\"\". Complex objects are a bit easier, but you still have to escape quotes inside the string to get them to parse properly.
```
# global config only - add a new key "foo" with simple string value "bar"
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c GLOBAL -pm ADD -pk foo -pv \"\"bar\"\"
# global config only - add a new key "foo" to the JSON doc root with complex value "{ \"bar\" : { \"baz\" : [ \"bazval1\", \"bazval2\" ] } }"
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c GLOBAL -pm ADD -pk "/foo" -pv "{ \"bar\" : { \"baz\" : [ \"bazval1\", \"bazval2\" ] } }"
# global config only - remove a key altogether
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c GLOBAL -pm REMOVE -pk "/foo"
```
4. Try the new JSON patch mechanism with a patch file. Below are some examples but please feel free to explore other combinations. Note that a patch file is a JSON array of individual patches.
1. Try a single patch from file
```
# Create a single patch that adds a new node. Create file "/tmp/mypatch.txt" and add the following JSON array.
[
{
"op": "add",
"path": "/you",
"value": { "cannot" : { "handle" : [ "the", "flow" ] } }
}
]
# Perform the patch
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c GLOBAL -pf /tmp/mypatch.txt
# Should result in adding the following to global.json
"you" : {
"cannot" : {
"handle" : [ "the", "flow" ]
}
}
```
2. Try multiple patches in a patch file
```
Create a single patch file that adds multiple nodes. Create file "/tmp/mygrimespatch.txt" and add the following JSON array.
# 2 patches in a single patch file
[
{
"op": "add",
"path": "/la",
"value": { "jiggy" : { "jar" : { "jar" : "do" } } }
},
{
"op": "add",
"path": "/carl",
"value": [ "poppa", "rhymes" ]
}
]
# Perform the patch
$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c GLOBAL -pf /tmp/mygrimespatch.txt
# Should result in adding the following to global.json
"la" : {
"jiggy" : {
"jar" : {
"jar" : "do"
}
}
},
"carl" : [ "poppa", "rhymes" ]
```
4. Enable Kerberos to verify config changes still work on service restarts.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/760#discussion_r139194190
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/enrichment_commands.py ---
@@ -55,14 +56,14 @@ def is_kafka_acl_configured(self):
def set_kafka_configured(self):
Logger.info("Setting Kafka Configured to True")
File(self.__params.enrichment_kafka_configured_flag_file,
- content="",
+ content="This file created on: " + datetime.now().strftime('%Y-%m-%d %H:%M:%S'),
--- End diff --
Yeah, probably worthwhile.
---
[GitHub] metron pull request #760: METRON-1188: Ambari global configuration managemen...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc closed the pull request at:
https://github.com/apache/metron/pull/760
---