Added prometheus alert RRM #938

ganeshrvel · 2023-06-16T21:01:31Z

No description provided.

…ment' into feature/Robusta-Resources-Management

… sink alerts

pablos44

Hey, nice work, but some important things need to be completed.

You need to update only the resources that changed. Use timestamp(updated_at) to find it
You need to keep a local table mapping entity_id to the real resource location. To avoid loading all CRD objects

pablos44 · 2023-06-22T10:21:07Z

src/robusta/core/sinks/robusta/rrm/rrm.py

+        try:
+            account_resources = self.dal.get_account_resources()
+            prometheus_rules = []
+            for resource in account_resources:


Here on each iteration, you update all alerts whether they have changed or not. You have to do it only on the first iteration when Robusta starts.
After that, you need to update only alerts that have been changed. For that, you have to get only records with larger timestamp stored on the previous iteration

pablos44 · 2023-06-22T10:24:45Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

@@ -537,3 +541,43 @@ def publish_cluster_nodes(self, node_count: int):
            self.handle_supabase_error()

        logging.info(f"cluster nodes: {UPDATE_CLUSTER_NODE_COUNT} => {data}")
+
+    def get_account_resources(self) -> List[AccountResource]:


I suggest adding parameters to the function like resource_kind, and from_timestamp. This will make this function reusable for other resources + will allow to fetch only recently updated changes

pablos44 · 2023-06-22T10:26:38Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

+
+            resource_state: Optional[ResourceState] = None
+            if data["resource_kind"] == ResourceKind.PrometheusAlert:
+                if "*" in data["clusters_target_set"] or self.cluster in data["clusters_target_set"]:


The clusters_target_set be a part of the query. You don't have to fetch records for all clusters

pablos44 · 2023-06-22T10:42:17Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_management.py

+
+    def create_prometheus_alerts(self, active_alert_rules: List[PrometheusAlertRule]):
+        try:
+            local_k8_crd_object = self.__list_crd_objects()


Here you need to keep a table with mapping entity_id to alert(from rules.alert) and probably to a CRD object name (if we want to support multiple CRDs). Then when there is a change in particular entity_id detected you can quickly find which alert has to be updated

…e events.

# Conflicts: # src/robusta/core/sinks/robusta/dal/supabase_dal.py

fixed bugs

pablos44

Well done!
See my notes for slightly improving your code writing.
==> Only one fix is mandatory for my opinion: init_resources error handling

pablos44 · 2023-07-06T11:25:48Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+                    name=name,
+                    grace_period_seconds=60
+                )
+        except Exception as e:


If it throws what does it mean? What is a recovery strategy? Pay attention, it also terminates the whole rrm __thread_loop !!. If you have other resource types in RRM loop it will terminate them too.

What I have done here is added a check. If the API call fails, the initialization method will make additional attempts to list and delete the custom objects. If these attempts also fail, we will remove the failed resources from the __resource_managers list, rendering them inert.

pablos44 · 2023-07-06T12:07:55Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+        rule = resource.resource_state.get('rule')
+        if "labels" not in rule:
+            rule["labels"] = {}
+        rule["labels"]["entity_id"] = resource.entity_id


try to make this stuff shorter (here and in further places). You just want to assign the entity_id label. Examples:

rule = {**rule, {"labels":{"entityId": entity_id}}}

rule.labels = {**rule.labels, {"entityId": entity_id} }'

pablos44 · 2023-07-06T12:11:39Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+    def __append_rule(self, slot: int, resource: AccountResource):
+        name = PrometheusAlertResourceManager.__crd_name(slot)
+
+        if "rule" not in resource.resource_state:


Why if?
See next comment

pablos44 · 2023-07-06T12:14:14Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+        except Exception as e:
+            # If the crd file does not exist, a new crd file will be created.
+            # Only raise an error if it's not a 404 exception.
+            if not (isinstance(e, exceptions.ApiException) and e.status == 404):


I would anyway continue to next slot, not only for 404

Another question, can get_namespaced_custom_object return None without throwing? What can be the reason for that?

Even though get_namespaced_custom_object could be None, I believe an exception will always be thrown in the case of a non-existent CRD file. Nonetheless, I have added a check for None.

pablos44 · 2023-07-06T12:19:26Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+                )
+            self.__cdr_slots_len[slot] += 1
+        except Exception as e:
+            raise f"An error occured while replacing PrometheusRules CRD alert => entity: {resource}. Error: {e}"


done, thanks!:)

pablos44 · 2023-07-06T12:26:09Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+            )
+
+            index = index_of(crd_obj["spec"]["groups"][0]["rules"],
+                             lambda r: r["labels"]["entity_id"] == resource.entity_id)


I think it may fail if one of the rules doesn't have a label for some reason. I know it is a corner case. It can happen only if someone modifies a CRD. I think later or sooner we will need to handle it as well

pablos44 · 2023-07-06T12:31:18Z

src/robusta/core/sinks/robusta/rrm/types.py

+        return self._resource_kind
+
+    # create resources
+    def create_resource(self, resource: AccountResource) -> Union[ResourceEntry, None]:


Isn't it using docstring a more conventional way of documenting in python?
something like

def get_resource_kind(self) -> ResourceKind: """ create resources """ .... ....

…ment' into feature/Robusta-Resources-Management

…th managed_prometheus_alerts_enabled

# Conflicts: # src/robusta/core/sinks/robusta/dal/supabase_dal.py

Migrated to the newer supabase library

Added support for alert sync status to the db Added managedPrometheusAlerts value to the db

# Conflicts: # .gitignore # src/robusta/core/sinks/robusta/dal/supabase_dal.py # src/robusta/core/sinks/robusta/robusta_sink.py # src/robusta/utils/common.py

# Conflicts: # src/robusta/core/model/env_vars.py

…encing the alerts creation

arikalon1

Nice work
Left some comments

I think I'm missing some logic for prometheus rules helm chart.
If the use chooses to use the saas configuration, and is using Robusta bundled Prometheus, will he have a duplication of the rules? (once installed via the helm chart, and once via saas)
Also, in this case will he still have the recording rules?

arikalon1 · 2023-11-05T15:59:30Z

helm/robusta/templates/runner-service-account.yaml

@@ -44,6 +44,20 @@ rules:
      - watch
      - patch

+  {{- if .Values.monitorHelmReleases }}


Why is this related to helm releases?

arikalon1 · 2023-11-05T16:03:20Z

src/robusta/core/model/cluster_status.py

@@ -29,3 +32,11 @@ class ClusterStatus(BaseModel):
    ttl_hours: int
    stats: ClusterStats
    activity_stats: ActivityStats
+
+
+class Account(BaseModel):


I think this class is not in use, is it?

arikalon1 · 2023-11-18T10:44:55Z

src/robusta/utils/common.py

@@ -37,3 +37,10 @@ def duplicate_without_fields(obj: HikaruBase, omitted_fields: List[str]):
            pass  # in case the field doesn't exist on this object

    return duplication
+
+
+def index_of(array: list, predicate):


I think this is not used

arikalon1 · 2023-11-18T11:03:05Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

@@ -33,19 +37,23 @@
 UPDATE_CLUSTER_NODE_COUNT = "update_cluster_node_count"
 SCANS_RESULT_TABLE = "ScansResults"
 RESOURCE_EVENTS = "ResourceEvents"
+ACCOUNT_RESOURCE_TABLE = "AccountResource"
+ACCOUNT_RESOURCE_STATUS_TABLE = "AccountResourceStatus"
+ACCOUNTS_TABLE = "Accounts"


arikalon1 · 2023-11-18T11:15:38Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

+                self.client.table(ACCOUNT_RESOURCE_TABLE)
+                .select("entity_id", "resource_kind", "clusters_target_set", "resource_state", "deleted", "enabled",
+                        "updated_at")
+                .filter("resource_kind", "eq", resource_kind)


for equals, use .eq and not .filter("xx", "eq", ...)

arikalon1 · 2023-11-19T11:29:02Z

src/robusta/core/sinks/robusta/rrm/types.py

+    pass
+
+
+class PrometheusAlertAnnotations(BaseModel):


What is the types of annotations and labels on the DB?
Aren't these just dictionaries?

Why do we have a typed classes, with a subset of the fields?

arikalon1 · 2023-11-19T11:30:10Z

src/robusta/core/sinks/robusta/rrm/types.py

+    @staticmethod
+    def from_supabase_dict(data: dict, entity_id: str):
+        labels = data.get("labels", {})
+        labels["entity_id"] = entity_id


Why do we save the entity id to the alert labels?
What is the use of it?

arikalon1 · 2023-11-19T11:30:39Z

src/robusta/core/sinks/robusta/rrm/types.py

+        )
+
+    @staticmethod
+    def from_local_k8_cluster_dict(data: dict):


arikalon1 · 2023-11-19T11:34:07Z

helm/robusta/values.yaml

@@ -74,6 +74,7 @@ lightActions:

 # install prometheus, alert-manager, and grafana along with Robusta?
 enablePrometheusStack: false
+enableManagedPrometheusAlerts: false


I think this should be renamed to enabledManagedConfiguration
A single value to all types of managed configurations

arikalon1 · 2023-11-19T11:34:52Z

helm/robusta/templates/runner.yaml

@@ -52,6 +52,8 @@ spec:
            value: {{ .Release.Name | quote }}
          - name: PROMETHEUS_ENABLED
            value: {{ .Values.enablePrometheusStack | quote}}
+          - name: MANAGED_PROMETHEUS_ALERTS_ENABLED


Rename to MANAGED_CONFIGURATION_ENABLED

Simplified the Alerts config logic

arikalon1

Looks good
Left few comments

arikalon1 · 2023-12-17T16:38:52Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

+
+            account_resources_map[resource.resource_kind].append(resource)
+
+        return dict(account_resources_map)


Isn't it already of type Dict?
Why do we need to wrap it with dict?

arikalon1 · 2023-12-17T16:42:32Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

+    def __to_db_account_resource_status(
+            self,
+            status_type: AccountResourceStatusType,
+            last_updated_at: datetime,


I think would call is latest_revision
I think it's not an update date.
It's a revision, which has a date value

arikalon1 · 2023-12-17T16:54:01Z

src/robusta/core/sinks/robusta/dal/supabase_dal.py

+            if resource_kind:
+                query_builder.eq("resource_kind", resource_kind)
+
+            if updated_at:


It will be much easier to understand if this is called latest_revision or something similar

arikalon1 · 2023-12-17T17:05:20Z

src/robusta/core/sinks/robusta/rrm/base_resource_manager.py

+from robusta.core.sinks.robusta.rrm.types import ResourceKind, AccountResourceStatusType, AccountResourceStatusInfo
+
+
+class BaseResourceManager:


I think this class is redundant

I think last_updated_at, cluster and dal should all be part of the RRM class
(there's only one of those, and not one per resource_kind)

arikalon1 · 2023-12-17T17:09:12Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+    AccountResource, ResourceKind, AccountResourceStatusType, AccountResourceStatusInfo
+
+
+class PrometheusAlertResourceManager(BaseResourceManager):


I would call this PrometheusAlertResourceHandler
I would make a base class, BaseResourceHandler, with one method:
def handle_resources(resources: List[AccountResource]) -> str

arikalon1 · 2023-12-17T18:23:40Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+                if in_cluster:
+                    self.__alerts_config_supabase_cache[res.entity_id] = rule
+
+        self.prepare_syncing_rules()


If there are no account_resources maybe we can skip this?

arikalon1 · 2023-12-17T18:25:00Z

src/robusta/core/sinks/robusta/rrm/prometheus_alert_resource_manager.py

+                self.__delete_crd_file(name=existing_cr_name)
+
+        except Exception as e:
+            if isinstance(e, ApiException):


I think weee should return an error here, or None
So the calling function, can handle errors from multiple resource types

arikalon1 · 2023-12-17T23:36:45Z

src/robusta/core/sinks/robusta/rrm/rrm.py

+                    logging.error("RRM Alerts config threw an error while executing `start_syncing_alerts`",
+                                  exc_info=True)
+
+    def __thread_start(self):


I would name this run_rrm or some logic name

arikalon1 · 2023-12-17T23:38:35Z

src/robusta/core/sinks/robusta/rrm/rrm.py

+
+    def __periodic_loop(self):
+        account_resources = self.dal \
+            .get_account_resources(updated_at=self.__last_updated_at)


I think this should also be under the same try/except
If it fails, also update the resource status

arikalon1 · 2023-12-17T23:43:07Z

src/robusta/core/sinks/robusta/rrm/rrm.py

+        account_resources = self.dal \
+            .get_account_resources(updated_at=self.__last_updated_at)
+
+        for resource_kind, resources in account_resources.items():


I would try to make this a little more generic:

errors: List[str] = [] for resource_kind, resources in account_resources.items(): handler = resource_handlers.get(resource_kind) if handler: error = handler.handle_resources(resources) if error: errors.append(error) else: logger.warning(f"Could not handle resources from kind {resource_kind}") # then report sync status based on the collected errors

# Conflicts: # src/robusta/core/sinks/robusta/robusta_sink.py

arikalon1

Nice work!

ganeshrvel and others added 9 commits June 17, 2023 02:31

Added prometheus alert RRM

17f27b3

Merge branch 'master' into feature/Robusta-Resources-Management

17dc4ff

fixed MAX_ALLOWED_RULES_PER_CRD

6149da7

Reorganized the code

d82dfdb

Improved the RRM alert creation

438f34a

Merge branch 'master' into feature/Robusta-Resources-Management

10cb2c3

fixed the ModuleNotFoundError: No module named 'postgrest_py' error.

6ff3de0

Merge remote-tracking branch 'origin/feature/Robusta-Resources-Manage…

68ccc67

…ment' into feature/Robusta-Resources-Management

instead of delete crd object, replace has been added to prevent noisy…

7b37675

… sink alerts

pablos44 suggested changes Jun 22, 2023

View reviewed changes

ganeshrvel and others added 9 commits June 26, 2023 20:29

refactored the alert config protocols to only accept the latest chang…

4ba43b6

…e events.

Merge branch 'master' into feature/Robusta-Resources-Management

d28d7bd

# Conflicts: # src/robusta/core/sinks/robusta/dal/supabase_dal.py

optimized the alerts config to use fewer resources than before.

e56c2b5

improved the alerts config

d8d445f

fixed bugs

Merge branch 'master' into feature/Robusta-Resources-Management

47976b7

removed postgrest package

7d53b7b

Fixing SupabaseDal dependencies

a602ceb

work in progress

3e17354

cleaned and reorganized the code

4f13655

ganeshrvel requested a review from pablos44 July 5, 2023 17:38

Added better try catch errors

891d9c1

pablos44 suggested changes Jul 6, 2023

View reviewed changes

ganeshrvel and others added 8 commits July 6, 2023 22:17

Added some improvements to error handling

2296ad7

Added docs improvements

aed984e

Testing module error - 1

1a75387

add account resource fecther interface

943f024

add account resource fecther interface

a80e9ab

add account resource fecther interface

a302cd1

add account resource fecther interface

b8005be

Fixed TypeError: 'type' object is not subscriptable

91febe6

ganeshrvel added 16 commits July 22, 2023 01:27

Merge remote-tracking branch 'origin/feature/Robusta-Resources-Manage…

5507f78

…ment' into feature/Robusta-Resources-Management

replaced enable_prometheus_stack params in the create/accounts api wi…

693c65f

…th managed_prometheus_alerts_enabled

Merge branch 'master' into feature/Robusta-Resources-Management

d5591c0

# Conflicts: # src/robusta/core/sinks/robusta/dal/supabase_dal.py

Merge branch 'master' into feature/Robusta-Resources-Management

9f25d84

Added service account permissions

5b1b6d2

Migrated to the newer supabase library

tiny cleanup

5bf829a

Modified the rrm to suite the new data structure

d2a13c8

Optimized the error handling and cache invalidation of alerts config.

f4afa2d

Added support for alert sync status to the db Added managedPrometheusAlerts value to the db

fixed labels and annotations

d9a4f08

fixed bug related to clearing residual rules

206620d

improved max_iterations logic

e08375c

Merge branch 'master' into feature/Robusta-Resources-Management

c1031cd

# Conflicts: # .gitignore # src/robusta/core/sinks/robusta/dal/supabase_dal.py # src/robusta/core/sinks/robusta/robusta_sink.py # src/robusta/utils/common.py

Merge branch 'master' into feature/Robusta-Resources-Management

c3c6c1e

# Conflicts: # src/robusta/core/model/env_vars.py

Merge branch 'master' into feature/Robusta-Resources-Management

01171c0

Fixed a bug where enable managed prometheus alerts flag was not influ…

0b15e04

…encing the alerts creation

Merge branch 'master' into feature/Robusta-Resources-Management

7143c40

arikalon1 reviewed Nov 19, 2023

View reviewed changes

ganeshrvel added 3 commits November 20, 2023 11:47

Merge branch 'master' into feature/Robusta-Resources-Management

08c4978

Added improvements to the RRM and alerts config.

780e93a

Simplified the Alerts config logic

Added MANAGED_CONFIGURATION_ENABLED checks

11d1f1e

arikalon1 reviewed Dec 17, 2023

View reviewed changes

ganeshrvel and others added 6 commits December 18, 2023 19:10

Merge branch 'master' into feature/Robusta-Resources-Management

45c4c9b

# Conflicts: # src/robusta/core/sinks/robusta/robusta_sink.py

Merge branch 'master' into feature/Robusta-Resources-Management

1656c35

Improvements to Alerts config.

36e597c

Merge branch 'master' into feature/Robusta-Resources-Management

7a78bde

CR fixes

b7c7b1d

Merge branch 'master' into feature/Robusta-Resources-Management

978416b

arikalon1 approved these changes Dec 22, 2023

View reviewed changes

arikalon1 merged commit fa5bbd9 into master Dec 22, 2023
16 checks passed

arikalon1 deleted the feature/Robusta-Resources-Management branch December 22, 2023 18:21


		account_resources_map[resource.resource_kind].append(resource)

		return dict(account_resources_map)

		from robusta.core.sinks.robusta.rrm.types import ResourceKind, AccountResourceStatusType, AccountResourceStatusInfo


		class BaseResourceManager:

		AccountResource, ResourceKind, AccountResourceStatusType, AccountResourceStatusInfo


		class PrometheusAlertResourceManager(BaseResourceManager):

Added prometheus alert RRM #938

Added prometheus alert RRM #938

Conversation

ganeshrvel commented Jun 16, 2023

pablos44 left a 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

pablos44 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arikalon1 left a 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

arikalon1 left a 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

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

arikalon1 left a comment

Choose a reason for hiding this comment