Add support for intra.broker.goals in anomaly detection / self healing #1721

ecojan · 2021-10-26T11:21:06Z

This PR resolves #1264 by enabling the self-healing for intra-broker goals. This however does not account for race conditions between Cluster level Goals and will only trigger after said goals are finished.
The scope of this PR is to allow self-healing on IntraBroker goals for now (as future work might make this obsolete as intra-broker goals and regular goals might be made to work together).

New configuration:

anomaly.detection.intra.broker.goals= List of intra-broker goals for the Anomaly detector to look for
self.healing.intra.broker.goals= List of Intra broker Goals for self-healing
self.healing.intra.broker.goal.violation.enabled= If the self-healing should be enabled for intra-broker-goals

New Anomaly Type: INTRA_BROKER_GOAL_VIOLATION

A new type of Anomaly that needs to be treated differently from the regular GOAL_VIOLATIONS.
When configuring for the rebalance parameters for this violation, we are skipping hard goals check and ignoring proposalsCache.
We are skipping hard goals check because intra-broker goals may not be hard goals (at the current given time).
We are ignoring proposalsCache because the proposals model takes into account all given default goals. If we add the intraBrokersGoals this will not work properly as currently, disk-granularity goals do not work with broker-granularity goals.

New Anomaly detectors: IntraBrokerGoalViolationDetector

This anomaly detector is only looking for the anomaly.detection.intra.broker.goal and unlike the GoalViolationDetector, it's using a cluster model that is generating replica placement on disks as well.
This anomaly detector mainly functions for INTRA_BROKER_GOAL_VIOLATIONS.

What is missing from this feature

missing representation on INTRA_BROKER_GOAL_VIOLATIONS in CruiseControl UI when an anomaly is detected (as this is a structure in cruise-control-ui project)

alexander-falca · 2021-10-26T14:32:29Z

.../linkedin/kafka/cruisecontrol/servlet/handler/async/runnable/GoalBasedOperationRunnable.java

+   */
+  public GoalBasedOperationRunnable(KafkaCruiseControl kafkaCruiseControl,
+                                    OperationFuture future,
+                                    List<String> goals,


I see dryRun boolean in other version of constructor. Do we need to support dryRun here too ?

We do not, that one is used for a user request (ex. request coming from UI for example)

alexander-falca · 2021-10-26T14:34:48Z

.../linkedin/kafka/cruisecontrol/servlet/handler/async/runnable/GoalBasedOperationRunnable.java

+  /**
+   * Constructor to be used for creating a runnable for self-healing.
+   */
+  public GoalBasedOperationRunnable(KafkaCruiseControl kafkaCruiseControl,


Would be great to have same order of parameters as in other constructor.

alexander-falca · 2021-10-26T15:02:12Z

.../main/java/com/linkedin/kafka/cruisecontrol/detector/notifier/AlertaSelfHealingNotifier.java

@@ -252,6 +257,20 @@ private void alertGoalViolation(AnomalyType anomalyType, final String localHostn
    }
  }

+  private void alertIntraBrokerGoalViolation(AnomalyType anomalyType, final String localHostname,
+                                             List<AlertaMessage> alertaMessages, IntraBrokerGoalViolations goalViolations) {
+    Map<Boolean, List<String>> violations = goalViolations.violatedGoalsByFixability();


there is some duplication in logic for this and method above. WDYT about extracting logic into private method and calling with required params ?

alexander-falca · 2021-10-27T04:17:10Z

...rc/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolationDetector.java

+ * This class will be scheduled to run periodically to check if the given goals are violated or not. An alert will be
+ * triggered if one of the goals is not met.
+ */
+public class IntraBrokerGoalViolationDetector extends AbstractAnomalyDetector implements Runnable {


There is a lot of logic in common with GoalViolationDetector class. wdyt about extracting into a common(abstract) class and then override only necessary pieces ?

Extracted this into a BaseGoalViolationDetector class, please have a look at let me know what you think.

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/GoalOptimizer.java

ecojan · 2021-11-22T13:49:15Z

Hi @efeg can you do a review of this PR please?
I would also like to bring up the #1724 as the integrationTests task runs for 3-5 minutes per test, if the build goes beyond 10 minutes without outputting, it will fail (see last build). Could please let me know if there was a different error here?
I tried building both this and the default branch locally, both took ~ 38minutes (+/- a few seconds) and finished successfully.

zornhsu

Thanks for the contribution, @ecojan! I haven't finished reviewing all the PR, but do let me know what you think about refactoring AbstractGoalViolations.

zornhsu · 2022-01-13T18:08:37Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

+   * @throws TimeoutException If broker capacity resolver is unable to resolve broker capacity in time.
+   * @throws BrokerCapacityResolutionException If broker capacity resolver fails to resolve broker capacity.
+   */
+  public ClusterModel clusterModel(long now,


Suggested change

public ClusterModel clusterModel(long now,

public ClusterModel clusterModel(long nowMs,

zornhsu · 2022-01-13T18:11:47Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

@@ -443,6 +443,37 @@ public ClusterModel clusterModel(long nowMs,
    return clusterModel;


Suggested change

return clusterModel;

return clusterModel(nowMs, requirements, allowCapacityEstimation, false, operationProgress);

Would something go wrong if we dedup this method?

Not 100% sure on this one,
We have already generated an initial cluster model in this Constructor. (see line 435)

Sorry I wasn't clear. Github doesn't allow me to drag L435-442.

How about we delete L435-442, and replace L443 with my suggested change?

Got it! Updated this bit

zornhsu · 2022-01-13T18:14:24Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

+                                   boolean allowCapacityEstimation,
+                                   boolean populateReplicaPlacementInfo,


Let's keep the order align with line 540.

Suggested change

boolean allowCapacityEstimation,

boolean populateReplicaPlacementInfo,

boolean populateReplicaPlacementInfo,

boolean allowCapacityEstimation,

zornhsu · 2022-01-13T18:14:35Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

+   * @param allowCapacityEstimation whether allow capacity estimation in cluster model if the underlying live broker capacity is unavailable.
+   * @param populateReplicaPlacementInfo whether populate replica placement information.


Let's keep the order align with line 540.

zornhsu · 2022-01-13T18:15:11Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

+                                   boolean allowCapacityEstimation,
+                                   OperationProgress operationProgress,
+                                   boolean populateReplicaPlacementInfo)


Let's keep the order align with line 540.

Ended up removing this Constructor since we have one already with the same signature.

zornhsu · 2022-01-13T21:59:49Z

.../java/com/linkedin/kafka/cruisecontrol/servlet/handler/async/runnable/RebalanceRunnable.java

+    _ignoreProposalCache = ignoreProposalCache;
+    _destinationBrokerIds = SELF_HEALING_DESTINATION_BROKER_IDS;
+    _isRebalanceDiskMode = isRebalanceDiskMode;
+


nit: extra whitespace

zornhsu · 2022-01-13T22:06:01Z

.../java/com/linkedin/kafka/cruisecontrol/servlet/handler/async/runnable/RebalanceRunnable.java

+                           boolean isRebalanceDiskMode,
+                           boolean ignoreProposalCache,
+                           boolean skipHardGoalCheck,
+                           boolean stopOngoingExecution) {


Suggested change

boolean isRebalanceDiskMode,

boolean ignoreProposalCache,

boolean skipHardGoalCheck,

boolean stopOngoingExecution) {

boolean stopOngoingExecution,

boolean skipHardGoalCheck,

boolean ignoreProposalCache,

boolean isRebalanceDiskMode) {

Good to keep same parameter order as GoalBasedOperationRunnable.java line 103.
Also adding new parameters to the end, according to existing assigning order.

zornhsu · 2022-01-13T22:09:06Z

...ntrol/src/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolations.java

+
+
+/**
+ * A class that holds all the goal violations.


Suggested change

* A class that holds all the goal violations.

* A class that holds all the intra broker goal violations.

zornhsu · 2022-01-13T22:26:08Z

...ntrol/src/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolations.java

+/**
+ * A class that holds all the goal violations.
+ */
+public class IntraBrokerGoalViolations extends KafkaAnomaly {


I have the same comment as there a lot of dup between this class and GoalViolations. How about making another abstract class?

Agreed, will create an abstract class.

zornhsu · 2022-01-13T23:05:06Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/AnomalyDetails.java

+      case INTRA_BROKER_GOAL_VIOLATION:
+        IntraBrokerGoalViolations intraBrokerGoalViolations = (IntraBrokerGoalViolations) _anomalyState.anomaly();


Maybe we can combine both cases once we have the abstract class?

Suggested change

case INTRA_BROKER_GOAL_VIOLATION:

IntraBrokerGoalViolations intraBrokerGoalViolations = (IntraBrokerGoalViolations) _anomalyState.anomaly();

case GOAL_VIOLATION:

case INTRA_BROKER_GOAL_VIOLATION:

IntraBrokerGoalViolations intraBrokerGoalViolations = (AbstractGoalViolations) _anomalyState.anomaly();

Hey @zornhsu , returned a bit to this.
My small concern on this is that it's used in other parts of the application and I would avoid obfuscating the difference between the two, maybe reuse the logic but keep them separate. WDYT?

Hi @ecojan, I think what zorn suggested makes more sense. Do you have an example that merging them would cause confusions?

ecojan · 2022-01-20T10:06:39Z

Hey @zornhsu thanks for the review, will try and get around to it the following days

zornhsu

Thanks for the new commits, @ecojan! What do you think about my previous comment to abstract BrokerGoalViolations for IntraBrokerGoalViolations to extend?

#1721

zornhsu · 2022-02-11T23:14:36Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/LoadMonitor.java

@@ -443,6 +443,37 @@ public ClusterModel clusterModel(long nowMs,
    return clusterModel;


Sorry I wasn't clear. Github doesn't allow me to drag L435-442.

How about we delete L435-442, and replace L443 with my suggested change?

ecojan · 2022-02-12T09:59:37Z

Thanks for the new commits, @ecojan! What do you think about my previous comment to abstract BrokerGoalViolations for IntraBrokerGoalViolations to extend?

#1721

Hey @zornhsu sorry for not replying to the initial question, yes I makes perfect sense to refactor AbstractGoalViolations.

Have a few changes locally already.

chen-anders · 2022-04-14T04:53:20Z

Just stumbled across this and saw that this would be really useful with some of the JBOD Kafka clusters our team is currently managing. Is there anything else (besides cruise-control UI stuff) preventing this from being merged in (vs ppl who want to use it having to maintain an ongoing fork)?

ecojan · 2022-05-04T11:19:29Z

@chen-anders all that's left is perhaps some small modifications and cleanup + another round of reviews

…r reuse

…r intra broker goals

…rokerGoalViolations to extend it

ecojan · 2022-05-04T11:36:57Z

Did a rebase with the latest changes from the default branch.

amuraru · 2022-08-14T09:24:56Z

@CCisGG I see you are an active maintainer lately - is this something you wanna help us review please? thanks!

CCisGG · 2022-08-14T17:34:20Z

@amuraru Sure, this seems to be a request from multiple cc users. I'll take some time to get the context and review the latest update. Meanwhile, @wyuka please feel free to take a pass.

CCisGG · 2022-08-14T17:39:35Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/GoalOptimizer.java

@@ -553,7 +574,11 @@ public void run() {
        // We compute the proposal even if there is not enough modeled partitions.
        ModelCompletenessRequirements requirements = _loadMonitor.meetCompletenessRequirements(_defaultModelCompletenessRequirements)
                                                     ? _defaultModelCompletenessRequirements : _requirementsWithAvailableValidWindows;
-        ClusterModel clusterModel = _loadMonitor.clusterModel(_time.milliseconds(), requirements, _allowCapacityEstimation, operationProgress);
+
+        // We check for Intra broker goals among Default goals - if we have intra broker goals, set replicaPlacementInfo to true


nit: "set replicaPlacementInfo to true" -> "set populateReplicaPlacementInfo to true".

CCisGG · 2022-08-14T18:00:17Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/AnomalyDetails.java

@@ -15,6 +15,8 @@

 import static com.linkedin.cruisecontrol.CruiseControlUtils.utcDateFor;
 import static com.linkedin.kafka.cruisecontrol.detector.notifier.KafkaAnomalyType.GOAL_VIOLATION;
+import static com.linkedin.kafka.cruisecontrol.detector.notifier.KafkaAnomalyType.INTRA_BROKER_GOAL_VIOLATION;
+


nit: redundant new lines. Just curious did you enable code style check? The code style should force to avoid 2 consecutive blank lines.

I did, checkstyle passed (running the lastest version that exists in the default branch as well). Also, build process has a checkStyle task in it as well and I noticed this is a recurring thing in other java files as well (just following the status quo).
Let me know if it should be different.

CCisGG · 2022-08-14T18:04:41Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/AnomalyDetails.java

+      case INTRA_BROKER_GOAL_VIOLATION:
+        IntraBrokerGoalViolations intraBrokerGoalViolations = (IntraBrokerGoalViolations) _anomalyState.anomaly();


Hi @ecojan, I think what zorn suggested makes more sense. Do you have an example that merging them would cause confusions?

CCisGG · 2022-08-14T18:12:00Z

...se-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/AnomalyDetectorUtils.java

+   * @param goalViolations Goal violations to check whether there are unfixable goals.
+   * @return True if the given goal violations contain unfixable goals, false otherwise.
+   */
+  public static boolean hasUnfixableGoals(IntraBrokerGoalViolations goalViolations) {


Is that better to change the input param to type AbstractGoalViolations? That way this method can help to check both interBrokerGoals and intraBrokerGoals

CCisGG · 2022-08-14T18:19:52Z

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/GoalViolationDetector.java

@@ -177,10 +64,10 @@ public void run() {
      }

      Set<Integer> excludedBrokersForLeadership = _excludeRecentlyDemotedBrokers ? executorState.recentlyDemotedBrokers()
-                                                                                 : Collections.emptySet();


nit: unnecessary change

CCisGG · 2022-08-14T18:21:13Z

...e-control/src/main/java/com/linkedin/kafka/cruisecontrol/detector/GoalViolationDetector.java

@@ -209,7 +96,7 @@ public void run() {
              _lastCheckedModelGeneration = clusterModel.generation();
            }
            newModelNeeded = optimizeForGoal(clusterModel, goal, goalViolations, excludedBrokersForLeadership, excludedBrokersForReplicaMove,
-                                             checkPartitionsWithRFGreaterThanNumRacks);


nit: unnecessary change.

CCisGG · 2022-08-14T18:24:07Z

...rc/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolationDetector.java

+  }
+
+  @Override
+  public void run() {


This method looks like highly duplicated with the GoalViolationDetector. I think we should avoid it. E.g. maybe extract the common logic into the parent class.

CCisGG · 2022-08-14T18:26:26Z

...rc/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolationDetector.java

+                                    boolean checkPartitionsWithRFGreaterThanNumRacks)
+          throws KafkaCruiseControlException {
+    if (clusterModel.topics().isEmpty()) {
+      LOG.info("Skipping goal violation detection because the cluster model does not have any topic.");


nit: skip intra broker goal violation detector.

CCisGG · 2022-08-14T18:27:52Z

...rc/main/java/com/linkedin/kafka/cruisecontrol/detector/IntraBrokerGoalViolationDetector.java

+    }
+  }
+
+  protected boolean optimizeForGoal(ClusterModel clusterModel,


Why are we not able to use the method in BaseGoalViolationDetector?

CCisGG · 2022-08-14T18:29:01Z

...ntrol/src/main/java/com/linkedin/kafka/cruisecontrol/detector/BaseGoalViolationDetector.java

+
+  protected boolean optimizeForGoal(ClusterModel clusterModel,
+                                    Goal goal,
+                                    GoalViolations goalViolations,


If we change the type to AbstractGoalViolations, maybe we can reuse this method for both GoalViolations and IntraBrokerGoalViolations?

CCisGG · 2022-08-14T18:39:03Z

Took a first pass and left some comments. Besides, I have some general comments for the commits so far:

From my understand, the only difference between the intraBrokerGoalViolation and (InterBroker)GoalViolation, is "when configuring for the rebalance parameters for this intraBrokerGoalViolation, we are skipping hard goals check and ignoring proposalsCache". In that case, I think we can re-use a lot of code in GoalViolation and GoalViolationDetector. However, I can still see lots of duplicate lines in current commits. To me, it seems like the AbstractGoalViolation and BaseGoalViolationDetector class are not fully utilized yet.
We need to make it more clear that "GoalViolations"/"GoalViolationDetector" are only for inter broker goals. For people who have enough background for cruise-control, they may be familiar with "goals only means interBrokerGoals", but for people don't have much background, this can be pretty confusing. I'd recommend to explicitly clarify that as much as we can. E.g. in JavaDocs, Configuration.md, and cruisecontrol.properties file.

ecojan · 2022-08-16T12:54:56Z

Thank you for the review @CCisGG , will go through the proposed changes and come back this week.

tkornai · 2023-02-16T08:48:29Z

Hi @ecojan, thanks for your work on this very useful feature. Do you think that you'll have the opportunity to come back to this PR in the future?

ecojan · 2023-02-16T10:16:20Z

Hi @tkornai
Unfortunately I had some priority changes in my schedule and did not get around to finish this one.
I can try in the near future, but I can't promise anything. If anybody else is open to continue the effort, I can help a bit.

alexander-falca reviewed Oct 27, 2021

View reviewed changes

alexander-falca mentioned this pull request Nov 8, 2021

[INTERNAL] Allow intra-broker goals to be used as default adobe/cruise-control#2

Closed

ecojan force-pushed the 1264-intra-broker-self-healing branch 2 times, most recently from 70ce833 to 0278353 Compare November 19, 2021 10:18

amuraru reviewed Nov 19, 2021

View reviewed changes

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/analyzer/GoalOptimizer.java Outdated Show resolved Hide resolved

ecojan force-pushed the 1264-intra-broker-self-healing branch from e47231b to 6516586 Compare November 22, 2021 11:44

efeg requested a review from zornhsu December 16, 2021 21:42

zornhsu reviewed Jan 13, 2022

View reviewed changes

ecojan force-pushed the 1264-intra-broker-self-healing branch from 6516586 to 37d010b Compare February 7, 2022 17:20

zornhsu reviewed Feb 11, 2022

View reviewed changes

ecojan added 8 commits May 4, 2022 14:23

Add support for intra.broker.goals in anomaly detection / self healing

d516eaf

Updated order of parameters for GoalBasedOperationRunnable constructor

8082503

Extracted alerting logic for goal violation in a separate function fo…

922a78e

…r reuse

Extracted GoalViolationDetector into a base class that can be used fo…

767ede0

…r intra broker goals

Added suggestion from review

6073d72

Fixed project build

6c95dee

Addressed PR comments

bddb2ef

Added AbstractGoalViolations class and made GoalViolations and IntraB…

05ff397

…rokerGoalViolations to extend it

ecojan force-pushed the 1264-intra-broker-self-healing branch from 27e433b to 05ff397 Compare May 4, 2022 11:36

CCisGG requested review from CCisGG and wyuka August 14, 2022 17:33

CCisGG requested changes Aug 14, 2022

View reviewed changes

	public ClusterModel clusterModel(long now,
	public ClusterModel clusterModel(long nowMs,

		@@ -443,6 +443,37 @@ public ClusterModel clusterModel(long nowMs,
		return clusterModel;

	return clusterModel;
	return clusterModel(nowMs, requirements, allowCapacityEstimation, false, operationProgress);

		boolean allowCapacityEstimation,
		boolean populateReplicaPlacementInfo,

		* @param allowCapacityEstimation whether allow capacity estimation in cluster model if the underlying live broker capacity is unavailable.
		* @param populateReplicaPlacementInfo whether populate replica placement information.

	* A class that holds all the goal violations.
	* A class that holds all the intra broker goal violations.

		case INTRA_BROKER_GOAL_VIOLATION:
		IntraBrokerGoalViolations intraBrokerGoalViolations = (IntraBrokerGoalViolations) _anomalyState.anomaly();

Add support for intra.broker.goals in anomaly detection / self healing #1721

Are you sure you want to change the base?

Add support for intra.broker.goals in anomaly detection / self healing #1721

Conversation

ecojan commented Oct 26, 2021

New configuration:

New Anomaly Type: INTRA_BROKER_GOAL_VIOLATION

New Anomaly detectors: IntraBrokerGoalViolationDetector

What is missing from this feature

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

ecojan commented Nov 22, 2021

zornhsu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zornhsu Jan 13, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zornhsu Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecojan commented Jan 20, 2022

zornhsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecojan commented Feb 12, 2022

chen-anders commented Apr 14, 2022

ecojan commented May 4, 2022

ecojan commented May 4, 2022

amuraru commented Aug 14, 2022

CCisGG commented Aug 14, 2022

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

CCisGG commented Aug 14, 2022 • edited Loading

ecojan commented Aug 16, 2022

tkornai commented Feb 16, 2023

ecojan commented Feb 16, 2023

zornhsu left a comment •

edited

Loading

zornhsu Jan 13, 2022 •

edited

Loading

zornhsu Jan 13, 2022 •

edited

Loading

CCisGG commented Aug 14, 2022 •

edited

Loading