Reader changes for dynamic enable/disable of RCA graph components #325

ktkrg · 2020-07-31T13:27:12Z

Issue #:
Fixes #327

Description of changes:
This change adds functionality to read the overrides written by the NodeDetailsCollector and update the rca conf file accordingly. To that effect this change adds two new sections to the rca[_idle][_master].conf:
mutedActions: []
mutedDeciders: []

Updating RCA conf and the muted nodes work for RCAs and deciders. For actions, the list of muted actions are looked at by the collator and prunes the ones that are muted.

Tests:
Ran tests with different overrides and made sure that the rca conf was updated correctly and muted correctly. Verified same things for the actions as well.

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

…ides

codecov · 2020-07-31T13:33:21Z

Codecov Report

❗ No coverage uploaded for pull request base (master@7be6969). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #325   +/-   ##
=========================================
  Coverage          ?   67.15%           
  Complexity        ?     2074           
=========================================
  Files             ?      299           
  Lines             ?    13280           
  Branches          ?     1099           
=========================================
  Hits              ?     8918           
  Misses            ?     3958           
  Partials          ?      404

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 7be6969...556b527. Read the comment docs.

vigyasharma · 2020-08-03T16:52:59Z

General comment on the description message:

For actions, the list of muted actions are looked at by the collator and prunes the ones that are muted.

Should this be done at collator level or decider level? I was thinking that if an action is muted, it returns isActionable() == false, and decider picks the next best possible action. If we mute it at collator level, then decider will keep picking that same muted action.

ktkrg · 2020-08-03T16:59:31Z

General comment on the description message:

For actions, the list of muted actions are looked at by the collator and prunes the ones that are muted.

Should this be done at collator level or decider level? I was thinking that if an action is muted, it returns isActionable() == false, and decider picks the next best possible action. If we mute it at collator level, then decider will keep picking that same muted action.

Aah yes, I started this before I started on collator, so I missed the fact that deciders can suggest multiple actions based.

Will change this.

src/test/resources/rca/rca_elected_master.conf

src/test/resources/rca/rca.conf

src/test/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/net/GRPCTest.java

...on/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverridesWrapper.java

...istro/elasticsearch/performanceanalyzer/decisionmaker/actions/ModifyQueueCapacityAction.java

src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/RcaController.java

.../amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/ConfJsonWrapper.java

...on/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverridesApplier.java

yojs · 2020-08-07T00:53:53Z

...amazon/opendistro/elasticsearch/performanceanalyzer/reader/ClusterDetailsEventProcessor.java

@@ -75,15 +87,18 @@ public void processEvent(Event event) {

    // An example node_metrics data is something like this for a two node cluster:
    // {"current_time":1566414001749}
+    // 1566414001749


This file used to be a json object per line. It would be nice if we can keep that and push the override timestamp inside as follows:

{"overrides": {"enabled": {}, "disabled": {}}, ts:54674895}

This needs a change in the plugin as well. Will follow up with a separate PR for this.

cool. But let's change it.

...n/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/Stats.java

...amazon/opendistro/elasticsearch/performanceanalyzer/reader/ClusterDetailsEventProcessor.java

...on/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverridesApplier.java

...java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/RcaConf.java

yojs · 2020-08-07T01:39:24Z

...amazon/opendistro/elasticsearch/performanceanalyzer/reader/ClusterDetailsEventProcessor.java

+    this(new ConfigOverridesApplier());
+  }
+
+  public ClusterDetailsEventProcessor(final ConfigOverridesApplier overridesApplier) {


Maybe we don't need to pass ConfigOverridesApplier as a constructor. No state is kept in it. It reads the config and applies it to the rca.conf. Maybe we can just use it as a helper static class ?

It does keep the lastAppliedTimestamp as an instance variable and uses that to check if the config overrides string received from the ClusterDetailsEventProcessor. Making it a static helper will make all the consumers see the same value of last applied time(in an IT for example).

yojs · 2020-08-07T17:24:12Z

...on/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverridesApplier.java

+        LOG.debug("New set of muted rcas: {}", currentMutedRcaSet);
+        LOG.debug("New set of muted deciders: {}", currentMutedDeciderSet);
+        LOG.debug("New set of muted actions: {}", currentMutedActionSet);


We should be applying change when someone calls the API or at startup, which shouldn't be that many. Should we make it info logs ? Or add a metric.

yojs · 2020-08-07T17:51:12Z

...on/opendistro/elasticsearch/performanceanalyzer/config/overrides/ConfigOverridesApplier.java

+        LOG.debug("New set of muted rcas: {}", currentMutedRcaSet);
+        LOG.debug("New set of muted deciders: {}", currentMutedDeciderSet);
+        LOG.debug("New set of muted actions: {}", currentMutedActionSet);
+        rcaConf.updateAllRcaConfFiles(currentMutedRcaSet, currentMutedDeciderSet, currentMutedActionSet);


Now we are updating three rca.conf files. updateAllRcaConfFiles has a void return type. What happens if it succeeded on a subset of files but not all? We will still update lastAppliedTimestamp and never look back until someone makes a change again. So, should we make lastAppliedTimestamp return true when writes on all three files succeed and and then only update the lastAppliedTimestamp. If not, then we will retry again on the next run and eventually all three files will be updated or we likely have a disk corruption or disk full issues. thoughts ?

That's a good point! I'm checking all three updates before updating the lastAppliedTimestamp.

I'm also emitting a metric if we fail to update an rca conf file. If we end up in a state where we can't do these file system changes, I think we should be failing in more places than just this, so yea, it's probably ok to do the timestamp update only if all three succeed.

yojs · 2020-08-07T17:53:36Z

src/main/java/com/amazon/opendistro/elasticsearch/performanceanalyzer/AppContext.java

@@ -39,10 +40,12 @@
  // initiate a node config cache within each AppContext space
  // to store node config settings from ES
  private final NodeConfigCache nodeConfigCache;
+  private Set<String> mutedActions;


The RCAController thread updates this but it is read by the SchedulerTask threads. We should make this volatile

yojs · 2020-08-07T18:54:02Z

...java/com/amazon/opendistro/elasticsearch/performanceanalyzer/rca/framework/core/RcaConf.java

+        mutedActions);
+      if (!updateStatus) {
+        LOG.error("Failed to update the conf file at path: {}", confFilePath);
+        StatsCollector.instance().logMetric(RcaConsts.WRITE_UPDATED_RCA_CONF_ERROR);


You can use the new metric framework. There your metric can also tell you which file erred out ?

Changes for 1 package (OpenDistroPerformanceAnalyzerEngine), pushed in snapshot... https://code.amazon.com/snapshots/partsrut/2020-08-08T02-49-04 Changes for OpenDistroPerformanceAnalyzerEngine package: 0194f9f Fix failing reaction wheel tests bfdb93d Merge remote-tracking branch 'upstream/master' e03a120 Use StringUtils instead of NumberUtils to check timestamp string (#360) 79841f2 Expose queue rejection time period setting in rca.conf (#356) f9dfda5 Implement isMuted method in DummyAction (#355) 7ede0b9 Reader changes for dynamic enable/disable of RCA graph components (#325) ee58207 Fix bug in publisher to support cool off period on a per node basis (#351) cr https://code.amazon.com/reviews/CR-31344155

ktkrg added 5 commits July 22, 2020 10:26

Common changes needed to support dynamic en/disabling of config overr…

d787724

…ides

Add ability to apply the config overrides

59bfc48

Merge from master

23d833e

Refactor muting logic for actions and graph nodes

b0ed031

Add unit tests

771f1c9

ktkrg linked an issue Jul 31, 2020 that may be closed by this pull request

Create common structures needed to support dynamic config overrides. #290

Closed

ktkrg self-assigned this Jul 31, 2020

ktkrg added the enhancement Enhancements to existing codebase label Jul 31, 2020

ktkrg removed a link to an issue Jul 31, 2020

Create common structures needed to support dynamic config overrides. #290

Closed

ktkrg linked an issue Jul 31, 2020 that may be closed by this pull request

Support dynamic enable/disabling of RCA, decider nodes and actions. #327

Closed

ktkrg requested review from khushbr and vigyasharma July 31, 2020 18:44

ktkrg added 2 commits July 31, 2020 11:46

Remove merge conflict markers in comments

274009c

Add licence header to new files

3188ef2

ktkrg changed the title ~~[DRAFT] Reader changes for dynamic enable/disable of RCA graph components~~ Reader changes for dynamic enable/disable of RCA graph components Jul 31, 2020

ktkrg added 8 commits August 3, 2020 19:17

Filter at the decider instead of collator

e631aa8

Merge from master

eb77ae7

Delete collator test as it is not used to filter actions

27189e0

Merge from master

77473dc

Use the right log levels

b4c9f28

Add abstract SuppressibleAction class to handle muted actions

6282932

Remove unwanted import

a914d68

Merge from master

35c416d

ktkrg requested review from adityaj1107 and yojs August 6, 2020 03:17

khushbr reviewed Aug 6, 2020

View reviewed changes

Address PR comments

2b16890

khushbr approved these changes Aug 7, 2020

View reviewed changes

yojs reviewed Aug 7, 2020

View reviewed changes

Address PR comments

300dcc5

yojs reviewed Aug 7, 2020

View reviewed changes

Address PR comments

556b527

yojs approved these changes Aug 7, 2020

View reviewed changes

ktkrg merged commit 7ede0b9 into master Aug 7, 2020

ktkrg deleted the ktkrg-ed2 branch August 7, 2020 19:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader changes for dynamic enable/disable of RCA graph components #325

Reader changes for dynamic enable/disable of RCA graph components #325

ktkrg commented Jul 31, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020 •

edited

Loading

vigyasharma commented Aug 3, 2020

ktkrg commented Aug 3, 2020

yojs Aug 7, 2020

ktkrg Aug 7, 2020

yojs Aug 7, 2020

yojs Aug 7, 2020

ktkrg Aug 7, 2020

yojs Aug 7, 2020

yojs Aug 7, 2020

ktkrg Aug 7, 2020

yojs Aug 7, 2020

ktkrg Aug 7, 2020

yojs Aug 7, 2020

Reader changes for dynamic enable/disable of RCA graph components #325

Reader changes for dynamic enable/disable of RCA graph components #325

Conversation

ktkrg commented Jul 31, 2020 • edited Loading

codecov bot commented Jul 31, 2020 • edited Loading

Codecov Report

vigyasharma commented Aug 3, 2020

ktkrg commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktkrg commented Jul 31, 2020 •

edited

Loading

codecov bot commented Jul 31, 2020 •

edited

Loading