[GitHub] metron issue #1132: METRON-1695: Expose pcap properties through Ambari
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 issue #1132: METRON-1695: Expose pcap properties through Ambari
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
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
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
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
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
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
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
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 issue #1132: METRON-1695: Expose pcap properties through Ambari
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
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
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
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
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
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
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
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 ---
