You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by anandsubbu <gi...@git.apache.org> on 2018/07/25 19:34:50 UTC
[GitHub] metron pull request #1132: METRON-1695: Expose pcap properties through Ambar...
GitHub user anandsubbu opened a pull request:
https://github.com/apache/metron/pull/1132
METRON-1695: Expose pcap properties through Ambari
## Contributor Comments
This change add a new jinja template for the pcap properties and adds a new tab in Ambari Metron config for the PCAP properties. Here are the screenshots:
<img width="853" alt="pcap-config-1" src="https://user-images.githubusercontent.com/20395490/43222746-a9e0026c-906e-11e8-9353-0ea34dbb4224.png">
<img width="988" alt="pcap-config-2" src="https://user-images.githubusercontent.com/20395490/43222747-aa1ba704-906e-11e8-82e8-71d4a9731aba.png">
*Testing Done*
Built mpack out of these changes, deployed a multi-node cluster and validated that the pcap.properties is being updated during Parser startup scripts per the screenshot below:
<img width="882" alt="parser-startup-screenshot" src="https://user-images.githubusercontent.com/20395490/43222878-fe5fcc50-906e-11e8-80a7-45c73e173638.png">
Also validated that the pcap properties is updated with config changes (ZK quorum, kafka broker etc.)
```
[root@metron-12 config]# cat pcap.properties
##### Storm #####
topology.worker.childopts=
topology.auto-credentials=[]
topology.workers=1
##### Kafka #####
spout.kafka.topic.pcap=pcap
kafka.zk=metron-10:2181,metron-11:2181,metron-1:2181
hdfs.sync.every=1
hdfs.replication.factor=--1
kafka.security.protocol=PLAINTEXT
# One of EARLIEST, LATEST, UNCOMMITTED_EARLIEST, UNCOMMITTED_LATEST
kafka.pcap.start=UNCOMMITTED_EARLIEST
kafka.pcap.numPackets=1000
kafka.pcap.maxTimeMS=300000
kafka.pcap.ts_scheme=FROM_KEY
kafka.pcap.out=/apps/metron/pcap/input
##### Parallelism #####
kafka.pcap.ts_granularity=MICROSECONDS
kafka.spout.parallelism=1
```
*Verification Steps*
* Spin up full dev (or a multi-node cluster)
* Ensure that the PCAP tab in Ambari config allows for values to be entered.
* Deploy cluster and validate that $METRON_HOME/config/pcap.properties file has the proper config values set.
## 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:
- [ ] 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?
- [ ] 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 && dev-utilities/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/anandsubbu/incubator-metron METRON-1695
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1132.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 #1132
----
commit 6d5cab2eb5ebbea56faa8855115533cb7eff52ab
Author: Anand Subramanian <as...@...>
Date: 2018-07-25T19:23:01Z
METRON-1695: Expose pcap properties through Ambari
----
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
This PR lays the foundation for exposing the properties to begin with. My thought was that it reduces one error prone manual step of hand-editing the `pcap.properties` file. I agree with your point about how users might be concerned about restarting all parsers after modifying PCAP config.
I did not create this earlier under the feature branch since this was not related specifically to the PCAP query panel, it was a more generic change.
I am fine to move this under the feature branch or I could wait until the fix for METRON-1709 (making the PCAP service its own) is available and then submit a fresh PR. Let me know which one works better.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by simonellistonball <gi...@git.apache.org>.
Github user simonellistonball commented on the issue:
https://github.com/apache/metron/pull/1132
+1 to that, PCAP should definitely get its own service, but agreed with @anandsubbu that should probably be a follow on item.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
Yup, @mmiklavc I see what you are saying. Make sense to me. Let me know if the latest README update conveys the message.
I also added a fix to prompt a service restart upon changes to the PCAP config settings. Thanks @MohanDV for the pointer!
---
[GitHub] metron pull request #1132: METRON-1695: Expose pcap properties through Ambar...
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on a diff in the pull request:
https://github.com/apache/metron/pull/1132#discussion_r205237418
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/parser_master.py ---
@@ -54,6 +57,12 @@ def configure(self, env, upgrade_type=None, config_dir=None):
commands.init_kafka_acls()
commands.set_acl_configured()
+ File(format("{metron_config_path}/pcap.properties"),
--- End diff --
I had earlier place this under the [indexing_master.py ](https://github.com/apache/metron/blob/master/metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/indexing_master.py#L62)script, but felt that it is more related to parser than with indexing. I am happy to move this block to any other place that is deemed fit.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
@anandsubbu, considering the changes in this PR:
1. Let's say I manually make changes to `pcap.properties`. I restart the parser topologies. Later, I manually restart pcap with the shell command. What happens to my `pcap.properties` changes?
2. Similarly, I make changes via Ambari's new PCAP panel. Now I go start pcap using `$METRON_HOME/bin/start_pcap_topology.sh`. What does pcap.properties look like?
In 1, your changes are overwritten by the parsers' restart. In 2, you've made changes in Ambari but they don't get materialized in the properties file because you haven't restarted the parsers - I may be mistaken, but I'm pretty sure clicking "save" in Ambari does not deploy the new properties, and this is the reason I ask for a doc change. Previously, configuring pcap was all manual steps. Now it will be a combo of manual steps for start/stop, automated steps for the config updates. And more specifically for the config updates, you will *only* see them if you make them in Ambari and restart the parsers. Also, if you choose not to use Ambari for this config handling and instead make your config changes to pcap.properties locally, you will have to remember that restarting the parsers will overwrite your pcap config - something you're only likely to realize the next time you start/restart pcap. This is the behavior that I think we should document if we accept this. Does that make s
ense?
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1132
I don't see any good reason to put off making pcap it's own Ambari component. Having to restart the parsers component (which is unrelated to pcap) to propagate pcap changes is a deal breaker for me.
Had this been submitted against the pcap feature branch I would be fine with it as an incremental change but I don't feel like this puts master in a good state and I definitely would not want it in a release. Why not just do it here?
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
@mmiklavc thanks for the review. I concur with your observations. PCAP topology does deserve its own place in Ambari and the Management UI.
As far as the scope of this PR goes, this IMHO, would be a first cut towards it. This PR addresses the immediate need for exposing the parameters in `pcap.properties` via Ambari. It also addresses auto-populating the ZK quorum in `pcap.properties` thus simplifying the manual steps that would be otherwise required in a multi-node deployment.
Would you be okay if we create a separate JIRA to cover the broader change? Let me know your thoughts.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1132
If you want to do the work in a separate PR (METRON-1709) then that's fine. As long as they are tested and committed together that works for me. If it were me, I would just do the work in this PR and save the trouble of managing two different PRs. We have several different components already that you can use as a template. I don't think this is that much work.
Since this was developed against master I wouldn't switch to the feature branch.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/1132
@anandsubbu I am conflicted on this one. :) Can you de-conflict?
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
I'm +1 via inspection pending the requested documentation. Also, please create the follow-on Jira, have it linked to this Jira, and also link it here for reference in the comments on this PR. Both this Jira/PR and https://github.com/apache/metron/pull/1134#issuecomment-409658527 should link up.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
I think I'm ok with this as a first step. Please document the lifecycle for how pcap.properties is updated in the README. It should be clear to users that they will need to restart parsers for the changes to take effect after the initial install is performed. Can you confirm, is that the way the properties are/will be deployed with this PR @anandsubbu?
https://github.com/apache/metron/pull/1134 should be included in the refactoring once we make PCAP a proper component.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
@anandsubbu Thanks for contributing this! The only thing I'm unsure of after reviewing this is your comment regarding this belonging in parser_master.py or not. On one hand, you could make a case that this is a parser like any other with the exception that it is spout-only. On the other hand, we don't currently provide a mechanism (iirc) to start/stop/manage the PCAP topology from Ambari. Starting parsers doesn't start PCAP, does it?
It may be that PCAP deserves its own "master", e.g. pcap_master.py, and lifecycle as a proper first class citizen (topology) within Ambari. Below is the current list of components, and PCAP is not in it afaik, even as a parser (there's a start_pcap_topology.sh script that is NOT the same as start_parser_topology.sh). It seems like if we're going to add this to Ambari we should also provide the ability to manage it. What do you think of that approach?
---
[GitHub] metron pull request #1132: METRON-1695: Expose pcap properties through Ambar...
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu closed the pull request at:
https://github.com/apache/metron/pull/1132
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
Sure @merrimanr , sounds good.
@MohanDV has already begun work on METRON-1709. I will wait for him to complete and then submit this pull request so it would be a natural fit.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
@anandsubbu thanks for the updates, I'm going merge your PR.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1132
@anandsubbu, I hate to give you whiplash, but @merrimanr may have a point here. There benefit of exposing the pcap properties via Ambari is lessened by now requiring the user to do 3 things instead of 2 to get the properties set for Pcap.
**Old**
1. Update properties
2. Restart pcap from command line
**New**
1. Update properties
2. Restart parsers to deploy the properties (users may not like this requirement)
3. Restart pcap from command line
The work is by no means throw away in its current form, but if we're going to do this it's probably worth going the whole way.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
Btw, I created [METRON-1719](https://issues.apache.org/jira/browse/METRON-1719) to track PCAP sensor being an independent service entity.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
> Please document the lifecycle for how pcap.properties is updated in the README. It should be clear to users that they will need to restart parsers for the changes to take effect after the initial install is performed. Can you confirm, is that the way the properties are/will be deployed with this PR @anandsubbu?
Hi @mmiklavc - there is no change in behavior from earlier to now. Nothing changes from a usage perspective. Let me clarify more with an example for a multi-node deployment...
**Earlier**
* User deploys metron
* `pcap.properties` file created with defaults under `$METRON_HOME/config`
* User hand edits the properties file (sets ZK quorum and other parameters as required)
* Starts the topology per the instructions [here](https://github.com/apache/metron/tree/master/metron-platform/metron-pcap-backend#starting-the-topology).
**Now**
* During deploy time, user is presented with a separate config tab in Ambari to configure/reconfigure PCAP properties. If user chooses to leave them untouched, the pcap.properties is initialized with appropriate defaults. ZK quorum is auto-populated as well.
* `pcap.properties` file is created during metron-parsers startup step.
* Starts the topology per the instructions [here](https://github.com/apache/metron/tree/master/metron-platform/metron-pcap-backend#starting-the-topology).
The steps in the [README](https://github.com/apache/metron/tree/master/metron-platform/metron-pcap-backend) still holds.
Now, we can add a note in the README to indicate that the PCAP properties can be configured via the 'PCAP' tab in Ambari Metron config. But I noticed this was not explicitly mentioned for other components (e.g. [REST](https://github.com/apache/metron/tree/master/metron-interface/metron-rest#configuration), [elasticsearch](https://github.com/apache/metron/tree/master/metron-platform/metron-elasticsearch#properties)). Let me know if you prefer to have this added.
---
[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on the issue:
https://github.com/apache/metron/pull/1132
Hey @nickwallen ... Let me close this PR. Will create one afresh based on the latest changes from #1201
---
[GitHub] metron pull request #1132: METRON-1695: Expose pcap properties through Ambar...
Posted by anandsubbu <gi...@git.apache.org>.
Github user anandsubbu commented on a diff in the pull request:
https://github.com/apache/metron/pull/1132#discussion_r205241710
--- Diff: metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-pcap-env.xml ---
@@ -0,0 +1,141 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/xsl" href="configuration.xsl"?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+<configuration supports_final="true">
+ <property>
+ <name>pcap_topology_worker_childopts</name>
+ <description>PCAP Topology JVM Options</description>
+ <value/>
+ <display-name>PCAP Topology childopts</display-name>
+ <value-attributes>
+ <empty-value-valid>true</empty-value-valid>
+ </value-attributes>
+ </property>
+ <property>
+ <name>pcap_topology_workers</name>
+ <description>Number of PCAP Topology Workers</description>
+ <value>1</value>
+ <display-name>Workers for PCAP Topology</display-name>
+ </property>
+ <property>
+ <name>spout_kafka_topic_pcap</name>
+ <description>PCAP Input Topic</description>
+ <value>pcap</value>
+ <display-name>PCAP Input Topic</display-name>
+ </property>
+ <property>
+ <name>hdfs_sync_every</name>
--- End diff --
The PCAP [config readme](https://github.com/apache/metron/tree/master/metron-platform/metron-pcap-backend#configuration) does not talk more about the units for this parameter (whether seconds, or minutes or something else). I looked through the code as well, but was not able to find more info. If someone could clarify about this, I can add the units so it is clearer.
---