Integration test framework to test RCAs and decision Makers #301

yojs · 2020-07-24T04:26:37Z

fixes: #337

Description of changes:
This PR introduces a framework that can be used to write tests concerning
RCA graph and all the components that leverage it(RCAs, Deciders etc.).
Currently it supports adding custom metrics, running tests against
single node cluster, multi-node clusters with dedicated masters and
the ones without dedicated masters. It provides a way to query the
SQLite data-store file on a particular host or hit the rest endpoint
on a particular node. Currently, the nodes of the clusters work over
http only. For more details, I would point you here

Tests:
There is a POC test to check the working of the framework.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2020-08-03T18:38:26Z

Codecov Report

Merging #301 into master will increase coverage by 0.59%.
The diff coverage is 61.60%.

@@             Coverage Diff              @@
##             master     #301      +/-   ##
============================================
+ Coverage     67.17%   67.76%   +0.59%     
- Complexity     2086     2135      +49     
============================================
  Files           301      301              
  Lines         13326    13403      +77     
  Branches       1104     1115      +11     
============================================
+ Hits           8952     9083     +131     
+ Misses         3970     3923      -47     
+ Partials        404      397       -7

Impacted Files	Coverage Δ	Complexity Δ
...ch/performanceanalyzer/PerformanceAnalyzerApp.java	`58.40% <0.00%> (+18.22%)`	`14.00 <0.00> (+7.00)`
...formanceanalyzer/PerformanceAnalyzerWebServer.java	`72.97% <0.00%> (-1.00%)`	`9.00 <0.00> (ø)`
...rformanceanalyzer/rest/QueryRcaRequestHandler.java	`48.27% <0.00%> (ø)`	`16.00 <1.00> (ø)`
...manceanalyzer/rca/persistence/SQLitePersistor.java	`68.08% <11.11%> (-4.91%)`	`27.00 <2.00> (+4.00)`	⬇️
...erformanceanalyzer/rca/persistence/FileRotate.java	`53.84% <50.00%> (+0.90%)`	`10.00 <0.00> (ø)`
...csearch/performanceanalyzer/rca/RcaController.java	`83.98% <75.60%> (+3.00%)`	`46.00 <6.00> (+8.00)`
...erformanceanalyzer/rca/scheduler/RCAScheduler.java	`67.85% <80.95%> (-1.27%)`	`12.00 <1.00> (+3.00)`	⬇️
...ceanalyzer/rca/framework/core/ConfJsonWrapper.java	`94.28% <100.00%> (-0.16%)`	`16.00 <1.00> (-1.00)`
...nalyzer/rca/framework/core/ConnectedComponent.java	`100.00% <100.00%> (ø)`	`19.00 <4.00> (+2.00)`
...ormanceanalyzer/rca/persistence/PersistorBase.java	`83.69% <100.00%> (+1.08%)`	`22.00 <0.00> (+2.00)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e03a120...086cdb4. Read the comment docs.

...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/Persistable.java

rguo-aws · 2020-08-07T23:06:08Z

...a/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/PersistorBase.java

@@ -245,14 +249,14 @@ private synchronized void rotateRegisterGarbageThenCreateNewDB(RotationType type
  }

  /** recursively insert nested summary to sql tables */
-  private void writeSummary(
+  private synchronized void writeSummary(


Maybe I am wrong. but since we've already added synchronized keyword in the parent function which calls this private function, do we still need to add another synchronized keyword here ?

No you are 100% right. Its not required. Its just that some of the private methods in the class were marked as synchronized so I made them all be so. Just making the public methods should suffice.

rguo-aws · 2020-08-07T23:34:57Z

...com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/SQLitePersistor.java

+      if (!de.getMessage().contains("no such table")) {
+        // it is totally fine if we fail to read some certain tables.
+        LOG.warn("Fail to read RCA : {}.", rca, de);
+      }


should we print an error log if otherwise

You mean make it an error log instead of warning ?

rguo-aws · 2020-08-07T23:38:48Z

...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/rest/QueryRcaRequestHandler.java

-    //check if RCA is valid
-    if (!validParams(rcaList)) {
+      // rcaList = SQLiteQueryUtils.getClusterLevelRca();
+      rcaList = persistable.getAllPersistedRcas();


this will contain RCAs that are not cluster level RCAs. Do we also want to expose node level RCAs on local master node to user ?

I would like to. Eventually, We would want to expose the RCAs on the local node, if they are not elected master.

The only thing with RCAs are that they are nested so, we won't want one node to gather RCAs from all nodes but if the request hits a node, it should be able to get all local RCAs.

rguo-aws · 2020-08-07T23:39:34Z

...ava/com/amazon/opendistro/elasticsearch/performanceanalyzer/rest/QueryRcaRequestHandler.java

@@ -178,14 +185,6 @@ private void handleClusterRcaRequest(Map<String, String> params, HttpExchange ex
          HttpURLConnection.HTTP_BAD_REQUEST);
      return;
    }
-    //check if we are querying from elected master


same as above

we would want to

...a/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/integTests/framework/Host.java

This PR introduces a framework that can be used to write tests concerning RCA graph and all the components that leverage it(RCAs, Deciders etc.). Currently it supports adding custom metrics, running tests against single node cluster, multi-node clusters with dedicated masters and the ones without dedicated masters. It provies a way to query the SQLite datastore file on a particular host or hit the rest endpoint on a particular node. Currently, the nodes of the clsuters work over http only.

…e scheduler initializations.

sidheart

This is basically JUnit for RCAs. It's insane how cool this is. I just have a few minor nits, the logic in here is solid and it's pretty much ready to merge.

...com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/persistence/SQLitePersistor.java

...stro/elasticsearch/performanceanalyzer/rca/integTests/framework/runners/RcaItRunnerBase.java

...endistro/elasticsearch/performanceanalyzer/rca/integTests/tests/poc/SimpleAnalysisGraph.java

...pendistro/elasticsearch/performanceanalyzer/rca/integTests/tests/poc/RcaItPocSingleNode.java

...istro/elasticsearch/performanceanalyzer/rca/integTests/tests/poc/validator/PocValidator.java

yojs force-pushed the rca-it2 branch from 962031d to 9ad007c Compare August 3, 2020 18:25

yojs force-pushed the rca-it2 branch from 9ad007c to c43eab4 Compare August 4, 2020 20:04

yojs changed the title ~~rca it first cut, after removing the refactoring changes in the exist…~~ Integration test framework to test RCAs and decision Makers Aug 4, 2020

yojs requested review from ktkrg, rguo-aws, vigyasharma and khushbr and removed request for ktkrg August 4, 2020 22:55

yojs added the enhancement Enhancements to existing codebase label Aug 5, 2020

yojs requested a review from sidheart August 5, 2020 19:05

yojs force-pushed the rca-it2 branch 2 times, most recently from 8ac14bf to d5f98c1 Compare August 7, 2020 00:11

rguo-aws reviewed Aug 7, 2020

View reviewed changes

yojs added 4 commits August 10, 2020 16:00

Fixed a bug where the database was being corrupted because of multipl…

209e36f

…e scheduler initializations.

Fix the POC graph

ccf1ebd

Removing unused methods

0d43965

yojs force-pushed the rca-it2 branch from 547b999 to 0d43965 Compare August 10, 2020 23:00

Name the validation checks that failed

9caaaac

sidheart suggested changes Aug 11, 2020

View reviewed changes

rguo-aws approved these changes Aug 11, 2020

View reviewed changes

Addressing PR comments

086cdb4

sidheart approved these changes Aug 11, 2020

View reviewed changes

yojs merged commit e358f20 into master Aug 11, 2020

yojs deleted the rca-it2 branch August 11, 2020 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test framework to test RCAs and decision Makers #301

Integration test framework to test RCAs and decision Makers #301

yojs commented Jul 24, 2020 •

edited

codecov bot commented Aug 3, 2020 •

edited

rguo-aws Aug 7, 2020

yojs Aug 10, 2020

rguo-aws Aug 7, 2020

yojs Aug 10, 2020 •

edited

rguo-aws Aug 7, 2020

yojs Aug 10, 2020

rguo-aws Aug 7, 2020

yojs Aug 10, 2020

sidheart left a comment

Integration test framework to test RCAs and decision Makers #301

Integration test framework to test RCAs and decision Makers #301

Conversation

yojs commented Jul 24, 2020 • edited

codecov bot commented Aug 3, 2020 • edited

Codecov Report

rguo-aws Aug 7, 2020

Choose a reason for hiding this comment

yojs Aug 10, 2020

Choose a reason for hiding this comment

rguo-aws Aug 7, 2020

Choose a reason for hiding this comment

yojs Aug 10, 2020 • edited

Choose a reason for hiding this comment

rguo-aws Aug 7, 2020

Choose a reason for hiding this comment

yojs Aug 10, 2020

Choose a reason for hiding this comment

rguo-aws Aug 7, 2020

Choose a reason for hiding this comment

yojs Aug 10, 2020

Choose a reason for hiding this comment

sidheart left a comment

Choose a reason for hiding this comment

yojs commented Jul 24, 2020 •

edited

codecov bot commented Aug 3, 2020 •

edited

yojs Aug 10, 2020 •

edited