Skip to content
This repository was archived by the owner on Aug 20, 2025. It is now read-only.

Conversation

@mmiklavc
Copy link
Contributor

@mmiklavc mmiklavc commented Nov 10, 2017

EDIT This is ready now (11/30/17)

This PR builds on the great work in #619

Contributor Comments

I wanted to get eyes on this despite some outstanding work left. It was no small undertaking moving 3 major versions, so there's quite a bit to review. In the future I would like to recommend we make feature branches for changes of this magnitude.

This work upgrades Elasticsearch and Kibana both to 5.6. I'm running a local full dev build now, but assuming no failures this will spin up full dev with everything, including the Kibana dashboard (had to be completely rewritten).

TODO

  • Re-create error dashboard - METRON-745: Create Error Dashboards #475
  • Fix outstanding test failures
  • Another merge with master
  • Update ES and Kibana docs
  • Document using Curator in place of the pruner tool
  • Docs for managing Kibana dashboards (no more pickle file - straight JSON using ES's bulk load API. This was not as easy as you would think it would be.)
  • Test with Kerberos

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines 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.
  • 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?

  • 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 is set up for your personal repository such that your branches are built there before submitting a pull request.

mmiklavc and others added 30 commits October 2, 2017 12:51
Day's work 10/19/2018 - 10/20/2018:)
@ottobackwards
Copy link
Contributor

So, this has me thinking.... It is a shame to have these test materials ( and others like @JonZeolla creates ) embedded in these pr's.

Maybe we should have someplace to put them in the code tree?

@mmiklavc
Copy link
Contributor Author

mmiklavc commented Dec 4, 2017

I don't think that's a bad idea. I definitely like having test scripts associated with the PR's because we can explicitly see what was done and/or recommended at the time the PR was being ushered into master. But we could also start to consolidate these into manual testing scripts that we reference ongoing. Then you could say something like:

Testing Plan
- es basic search test
- meta alerts search test
- kibana dashboard smoke test

That could be links, or we could copy-paste the current manual test plan from the source tree. I think the main thing I would want is to have the PR's test plan (here in the comments) be maintained statically for posterity based on the code at that time. For example, since we're removing the data pruner, the new test plan would not have a data pruner test, but I'd want to make sure any earlier PRs that did have a test plan for it can still be viewed statically. Basically, you'd either include the latest commit as part of the link or copy-paste the test plan.

@ottobackwards
Copy link
Contributor

Yeah, I'm trying to think of the right way to do it, without having to have it be "officially maintained".
more like an informal /testing_stuff , with some descriptions, when it was valid/written against etc.

Something like that. Then people could edit them and adapt them etc.

@mmiklavc
Copy link
Contributor Author

Just a status update on this. We're currently waiting for 0.4.2 to roll out before this gets committed. We definitely want more eyes and testing on this PR considering its breadth and size. We do not have any +1's yet, and I would prefer to have at least 2 for good measure, if at all possible.

Copy link
Contributor

@justinleet justinleet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple README comments added

* [Properties](#properties)
* [Upgrading to 5.6.2](#upgrading-to-562)
* [Type Mappings](#type-mappings)
* [Using Metron with Elasticsearch 5.x](#using-metron-with-elasticsearch-5x)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is broken, because the section name doesn't line up with the actual name

* [https://www.elastic.co/guide/en/elasticsearch/reference/5.6/breaking_50_mapping_changes.html](https://www.elastic.co/guide/en/elasticsearch/reference/5.6/breaking_50_mapping_changes.html)
* [https://www.elastic.co/blog/strings-are-dead-long-live-strings](https://www.elastic.co/blog/strings-are-dead-long-live-strings)

## Using Metron with Elasticsearch 5.6.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ES 2.x nested alert field still exists in this branch, right? I don't think that requirement got removed.

If not, this section should probably just say it's required in ES, and drop the obsolete portion of the statement..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set

@@ -1,3 +1,16 @@
# Metron Deployment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add the license header to this? #884 is close to going in and enforcing this, so I'm hoping to avoid impact to master.

<!--
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.
-->

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a heads up, #883 is in now, so this will have to be taken care of when you merge master to deconflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set

- Kibana:
* Set "kibana_es_url" to `http://<replace_with_elasticsearch_master_hostname>:9200`. "replace_with_elasticsearch_master_hostname" is the IP of the node where you assigned ElasticSearch Master on the Assign Master tab.
* Change kibana_default_application to "dashboard/Metron-Dashboard"
* Change kibana_default_application to "dashboard/AV-YpDmwdXwc6Ua9Muh9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to provide an easily identifiable name, or are we stuck with the "AV-..." ugliness?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They changed the links to point to index keys. I was bummed about this as well.

<configuration>
<!-- Skip the default running of this plug-in (or everything is run twice...see below) -->
<argLine>@{argLine} -Xmx2048m</argLine>
<argLine>-Xmx2048m</argLine>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in for code coverage via JaCoCo. It basically overrides the empty argline from above that got deleted. Was it causing problems with running things with it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build was failing with this argLine. Though, I can't recall the exact error now as it's been a while. What's the impact of leaving it off? No code coverage for this module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the top level pom, so no code coverage at all, iirc. I haven't looked at it in awhile. I'll play with it a bit and see what's going on

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with this a bit and I'm not convinced this works in master anymore, so if this was necessary to get this running, I'd rather push fixing coverage off to a follow on task.

@mmiklavc
Copy link
Contributor Author

mmiklavc commented Jan 4, 2018

Looking at the additional deployment readme merge conflicts now

@mmiklavc
Copy link
Contributor Author

mmiklavc commented Jan 4, 2018

This is going to take some time to resolve, but everything else in this branch is still unhindered. 01c26a7#diff-55e8119c8b8ae56260305e01c354d04b

@merrimanr
Copy link
Contributor

I spun this up in full dev and spent all day testing it. From a functional perspective, I can not find anything wrong with it. I ran through the test plan in this PR and everything worked as expected. I also tested this exhaustively with the Alerts UI and Swagger UI and everything works great.

The only issue I found are with the Alerts UI e2e tests. These no longer run at all and I suspect it's because the templates have changed with ES 5.6.2 so loading e2e test data and template no longer works. I'm not sure that this should hold up this PR since e2e tests are actively being worked on in #857 but I wanted everyone to be aware.

@mmiklavc
Copy link
Contributor Author

mmiklavc commented Jan 5, 2018

Worked with @merrimanr to fix the e2e test issue and just submitted a fix. This does not fix the full e2e test runs as this is being handled by 857, however it brings this PR back to parity with current functionality in master.

@merrimanr
Copy link
Contributor

I ran this up in full dev again and verified the e2e tests now work similar to how they do in master. I also manually tested several other areas including the Alerts UI, Kibana and Swagger. Everything works as expected. Assuming others are still testing but +1 from me.

@justinleet
Copy link
Contributor

At this point, I'm +1 since @merrimanr ran up the e2e tests. A couple people have put a fair amount of testing into this, and it seems like at this point we're at parity and not finding more issues.

@cestella
Copy link
Member

cestella commented Jan 8, 2018

I want to pile on and give this my (non-binding since I contributed PRs against this PR) +1. LGTM!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants