You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by justinleet <gi...@git.apache.org> on 2017/09/14 15:40:55 UTC
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
GitHub user justinleet opened a pull request:
https://github.com/apache/metron/pull/757
METRON-938: "service metron-rest start <password>" does not work on CentOS 7.
## Contributor Comments
Moved things from an init script to Ambari, since we manage pretty much everything through Ambari anyway. Hive does something similar. Now, the password comes from Ambari and we're independent of the `service` command and underlying CentOS management in this realm.
A couple properties are pushed to Ambari to make this easier. Notably, it's a pain in the butt (and maybe not possible at all), to get the version of our Metron install from interrogating Ambari. As such, it's added an invisible read-only property. This probably has to be revisited when we support upgrades (in order to make sure that the version increments appropriately). If this is objectionable, we may want to just hard code it and replace it when we do the overall release increments like we do the other instances.
As a sidenote, it would be really nice to have a version/profile/whatever of full-dev available on CentOS 7.
## 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:
- [x] 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).
- [x] 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.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [x] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified and tested manually?
- [x] 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
```
- [x] Have you written or updated unit tests and or integration tests to verify your changes?
- [x] 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)?
- [x] 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:
- [x] 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/justinleet/metron METRON-938
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/757.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 #757
----
commit 5701db6d9338b4a32f358aa5ec081e11b7618cdb
Author: justinjleet <ju...@gmail.com>
Date: 2017-09-13T00:06:19Z
WIP
commit 109591348fda5f7728721948ed9521453c0bbabb
Author: justinjleet <ju...@gmail.com>
Date: 2017-09-13T19:52:40Z
mostly working
commit cc7be848d3fb7b3285f288ff8283f77bc1e8f06d
Author: justinjleet <ju...@gmail.com>
Date: 2017-09-13T23:04:42Z
taking out password argument to avoid exposing in ps
commit b7387ee23a6a7bcb5214d1a8efcb2d69807243e5
Author: justinjleet <ju...@gmail.com>
Date: 2017-09-13T23:26:49Z
Fixing restart, updating documentation, and removing extraneous fields from template
commit 7a998f6116891a3b2c4ca40d849ac46a0d0fa48a
Author: justinjleet <ju...@gmail.com>
Date: 2017-09-14T15:20:44Z
removing extra comments
----
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139942579
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/rest_commands.py ---
@@ -58,17 +74,94 @@ def init_kafka_acls(self):
def start_rest_application(self):
Logger.info('Starting REST application')
- command = format("service metron-rest start {metron_jdbc_password!p}")
- Execute(command)
+
+ # Build the spring options
+ # the vagrant Spring profile provides configuration values, otherwise configuration is provided by rest_application.yml
+ metron_spring_options = format(" --server.port={metron_rest_port}")
--- End diff --
Ignore the things I say, it would have helped if I'd done stuff right. PR is updated to primarily run from a script, that's largely a renamed and slightly modified metron-rest (rename is to metron-rest.sh). Need to update docs to reflect it.
I also killed the metron version variable by doing some shell globbing. If you have multiple metron-rest jars in the lib directory it could be problematic, but I don't think it's unreasonable to not add random crap to the dirs.
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139813301
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/rest_commands.py ---
@@ -58,17 +74,94 @@ def init_kafka_acls(self):
def start_rest_application(self):
Logger.info('Starting REST application')
- command = format("service metron-rest start {metron_jdbc_password!p}")
- Execute(command)
+
+ # Build the spring options
+ # the vagrant Spring profile provides configuration values, otherwise configuration is provided by rest_application.yml
+ metron_spring_options = format(" --server.port={metron_rest_port}")
--- End diff --
@merrimanr So I tried doing this, but the problem is that if the Java command gets kicked up from a script, the Java command ends up showing up on in ps, which then goes and gives up spring.datasource.password. Which is incredibly annoying.
Unless you have any ideas on how to get around that, I think we're stuck in Ambari
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139246527
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/params/params_linux.py ---
@@ -76,6 +81,9 @@
global_json_template = config['configurations']['metron-env']['global-json']
global_properties_template = config['configurations']['metron-env']['elasticsearch-properties']
+# Java
+java64_home = config['hostLevelParams']['java_home']
--- End diff --
Nope, I dropped it and the reference to it is changed to just java_home
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139245252
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/params/params_linux.py ---
@@ -76,6 +81,9 @@
global_json_template = config['configurations']['metron-env']['global-json']
global_properties_template = config['configurations']['metron-env']['elasticsearch-properties']
+# Java
+java64_home = config['hostLevelParams']['java_home']
--- End diff --
Is this necessary? This property is also defined in params.py.
---
[GitHub] metron issue #757: METRON-938: "service metron-rest start " does n...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/757
Spun this up in full dev and tested several different cases including setting up a MySQL database for authentication. Worked great. I made a couple minor suggestions but I think this is pretty close to being ready. Nice job @justinleet.
---
[GitHub] metron issue #757: METRON-938: "service metron-rest start " does n...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/757
Merged in master for extremely minor deconflict of `rest_commands.py` for imports added on the same line.
---
[GitHub] metron issue #757: METRON-938: "service metron-rest start " does n...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/757
Spun it up again in full dev, setup MySQL and everything works as it should. Thanks so much @justinleet for fixing this! Turned out great. +1
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/757
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139957912
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/service_advisor.py ---
@@ -69,6 +69,10 @@ def getServiceComponentLayoutValidations(self, services, hosts):
message = "Metron REST must be colocated with an instance of STORM SUPERVISOR"
items.append({ "type": 'host-component', "level": 'WARN', "message": message, "component-name": 'METRON_REST', "host": metronRESTHost })
+ if metronRESTHost not in hbaseClientHosts:
--- End diff --
It's more a shorthand for the fact that REST requires requires basically everything else (HDFS, ZK, HBase, all the topologies, etc.). We could do all the requirements in specifics if we want (and I'm not necessarily opposed to doing it for REST while I'm already in here), but I'd argue that's a good candidate for a follow on task (since we do it on multiple items).
I'd prefer to do what the Hive service does and just automatically place every required item on the same node automatically (the selector on install automatically selects the same thing for all the components), which then means that this colocation check on something like parsers is more robust. Having said that, I never quite dug deep enough into how Ambari actually enforces that for Hive.
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139943087
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/service_advisor.py ---
@@ -69,6 +69,10 @@ def getServiceComponentLayoutValidations(self, services, hosts):
message = "Metron REST must be colocated with an instance of STORM SUPERVISOR"
items.append({ "type": 'host-component', "level": 'WARN', "message": message, "component-name": 'METRON_REST', "host": metronRESTHost })
+ if metronRESTHost not in hbaseClientHosts:
--- End diff --
Updated the service advisor to check on metronParsersHost
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139250975
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/service_advisor.py ---
@@ -69,6 +69,10 @@ def getServiceComponentLayoutValidations(self, services, hosts):
message = "Metron REST must be colocated with an instance of STORM SUPERVISOR"
items.append({ "type": 'host-component', "level": 'WARN', "message": message, "component-name": 'METRON_REST', "host": metronRESTHost })
+ if metronRESTHost not in hbaseClientHosts:
--- End diff --
Good catch here. This was a problem someone was going to run into eventually. Do we also need a check for hdfsClientHosts? zookeeperClientHosts? Now that I think about it we also need to ensure REST is installed on the same host as parsers, enrichment and indexing (for topologoy start/stop). Add a check for metronParsersHost might satisfy all those indirectly.
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139951556
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/service_advisor.py ---
@@ -69,6 +69,10 @@ def getServiceComponentLayoutValidations(self, services, hosts):
message = "Metron REST must be colocated with an instance of STORM SUPERVISOR"
items.append({ "type": 'host-component', "level": 'WARN', "message": message, "component-name": 'METRON_REST', "host": metronRESTHost })
+ if metronRESTHost not in hbaseClientHosts:
--- End diff --
isn't making everything require everything else on the same node kind of an anti pattern?
---
[GitHub] metron issue #757: METRON-938: "service metron-rest start " does n...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/757
As a note, the way I tested this was running it up in a CentOS 7 VM, along with a couple changes to get things working:
* Turning off firewalld during spinup.
* Downgrading python.x86_64 and python-libs.x86_64 to 2.7.5-48.el7 to avoid an SSL error. Apparently 58 or whatever latest epel has causes entirely unrelated issues with node registration in Ambari.
* Installing Metron + dependencies via Ambari.
I also ran in standard full dev to ensure that it still worked in 6.
---
[GitHub] metron pull request #757: METRON-938: "service metron-rest start "...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/757#discussion_r139249622
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/rest_commands.py ---
@@ -58,17 +74,94 @@ def init_kafka_acls(self):
def start_rest_application(self):
Logger.info('Starting REST application')
- command = format("service metron-rest start {metron_jdbc_password!p}")
- Execute(command)
+
+ # Build the spring options
+ # the vagrant Spring profile provides configuration values, otherwise configuration is provided by rest_application.yml
+ metron_spring_options = format(" --server.port={metron_rest_port}")
--- End diff --
This is probably just a style issue and not critically important but would consider moving some of this logic to a start script that lives inside the metron-rest module? MPack code is notoriously challenging to troubleshoot and having all this in here will make starting the REST app outside of Ambari very tedious. I have had to do this a lot when troubleshooting REST problems and helping others. Maybe REST is fairly stable at this point and we shouldn't have to mess with it much though.
This would also keep you from having to add a metron_version property to Ambari because the version would be substituted in the script at build time. This is how all our other scripts get the version.
---