Aggregated target health does not take into account checks selection

trento-project · Nov 30, 2023 · ba600e1 · ba600e1
1 parent 8eb6dc9
commit ba600e1
Show file tree

Hide file tree

Showing 4 changed files with 126 additions and 12 deletions.
diff --git a/lib/trento/clusters/cluster.ex b/lib/trento/clusters/cluster.ex
@@ -605,19 +605,20 @@ defmodule Trento.Clusters.Cluster do
 
   defp maybe_emit_cluster_deregistered_event(_, _), do: nil
 
-  defp maybe_add_checks_health(healths, _, []), do: healths
-  defp maybe_add_checks_health(healths, checks_health, _), do: [checks_health | healths]
+  defp maybe_add_checks_health(healths, checks_health) when checks_health != Health.unknown(),
+    do: [checks_health | healths]
+
+  defp maybe_add_checks_health(healths, _), do: healths
 
   defp maybe_emit_cluster_health_changed_event(%Cluster{
          cluster_id: cluster_id,
          discovered_health: discovered_health,
          checks_health: checks_health,
-         selected_checks: selected_checks,
          health: health
        }) do
     new_health =
       [discovered_health]
-      |> maybe_add_checks_health(checks_health, selected_checks)
+      |> maybe_add_checks_health(checks_health)
       |> Enum.filter(& &1)
       |> HealthService.compute_aggregated_health()
 

diff --git a/lib/trento/hosts/host.ex b/lib/trento/hosts/host.ex
@@ -730,15 +730,14 @@ defmodule Trento.Hosts.Host do
 
   defp maybe_emit_host_health_changed_event(%Host{
          host_id: host_id,
-         selected_checks: selected_checks,
          heartbeat: heartbeat,
          checks_health: checks_health,
          saptune_health: saptune_health,
          health: current_health
        }) do
     new_health =
       [heartbeat]
-      |> maybe_add_checks_health(checks_health, selected_checks)
+      |> maybe_add_checks_health(checks_health)
       |> maybe_add_saptune_health(saptune_health)
       |> Enum.filter(& &1)
       |> HealthService.compute_aggregated_health()
@@ -748,8 +747,10 @@ defmodule Trento.Hosts.Host do
     end
   end
 
-  defp maybe_add_checks_health(healths, _, []), do: healths
-  defp maybe_add_checks_health(healths, checks_health, _), do: [checks_health | healths]
+  defp maybe_add_checks_health(healths, checks_health) when checks_health != Health.unknown(),
+    do: [checks_health | healths]
+
+  defp maybe_add_checks_health(healths, _), do: healths
 
   defp maybe_add_saptune_health(healths, Health.unknown()), do: healths
   defp maybe_add_saptune_health(healths, saptune_health), do: [saptune_health | healths]

diff --git a/test/trento/clusters/cluster_test.exs b/test/trento/clusters/cluster_test.exs
@@ -474,6 +474,82 @@ defmodule Trento.ClusterTest do
       )
     end
 
+    test "checks health should not affect health when checks health is unknown" do
+      cluster_id = Faker.UUID.v4()
+      selected_checks = Enum.map(0..4, fn _ -> Faker.Cat.name() end)
+
+      assert_events_and_state(
+        [
+          build(:cluster_registered_event, cluster_id: cluster_id, health: Health.passing()),
+          %ClusterChecksHealthChanged{
+            cluster_id: cluster_id,
+            checks_health: Health.unknown()
+          },
+          %ClusterDiscoveredHealthChanged{
+            cluster_id: cluster_id,
+            discovered_health: Health.passing()
+          }
+        ],
+        SelectChecks.new!(%{
+          cluster_id: cluster_id,
+          checks: selected_checks
+        }),
+        [
+          %ChecksSelected{
+            cluster_id: cluster_id,
+            checks: selected_checks
+          }
+        ],
+        fn cluster ->
+          assert %Cluster{
+                   cluster_id: ^cluster_id,
+                   health: Health.passing(),
+                   checks_health: Health.unknown()
+                 } = cluster
+        end
+      )
+    end
+
+    test "should not update health when an empty checks selection is saved" do
+      cluster_id = Faker.UUID.v4()
+      selected_checks = Enum.map(0..4, fn _ -> Faker.Cat.name() end)
+
+      assert_events_and_state(
+        [
+          build(:cluster_registered_event, cluster_id: cluster_id, health: Health.critical()),
+          %ChecksSelected{
+            cluster_id: cluster_id,
+            checks: selected_checks
+          },
+          %ClusterChecksHealthChanged{
+            cluster_id: cluster_id,
+            checks_health: Health.warning()
+          },
+          %ClusterDiscoveredHealthChanged{
+            cluster_id: cluster_id,
+            discovered_health: Health.critical()
+          }
+        ],
+        SelectChecks.new!(%{
+          cluster_id: cluster_id,
+          checks: []
+        }),
+        [
+          %ChecksSelected{
+            cluster_id: cluster_id,
+            checks: []
+          }
+        ],
+        fn cluster ->
+          assert %Cluster{
+                   cluster_id: ^cluster_id,
+                   health: Health.critical(),
+                   checks_health: Health.warning()
+                 } = cluster
+        end
+      )
+    end
+
     test "should not change health if it is already critical" do
       cluster_id = Faker.UUID.v4()
       selected_checks = Enum.map(0..4, fn _ -> Faker.Cat.name() end)

diff --git a/test/trento/hosts/host_test.exs b/test/trento/hosts/host_test.exs
@@ -381,7 +381,44 @@ defmodule Trento.Hosts.HostTest do
       end
     end
 
-    test "should ignore checks health when an empty checks selection is saved" do
+    test "checks health should not affect health when checks health is unknown" do
+      host_id = Faker.UUID.v4()
+      selected_checks = Enum.map(0..4, fn _ -> Faker.Cat.name() end)
+      host_registered_event = build(:host_registered_event, host_id: host_id)
+      heartbeat_succeded_event = build(:heartbeat_succeded, host_id: host_id)
+
+      host_health_changed_event =
+        build(:host_health_changed_event, host_id: host_id, health: Health.warning())
+
+      assert_events_and_state(
+        [
+          host_registered_event,
+          heartbeat_succeded_event,
+          host_health_changed_event
+        ],
+        SelectHostChecks.new!(%{
+          host_id: host_id,
+          checks: selected_checks
+        }),
+        [
+          %HostChecksSelected{
+            host_id: host_id,
+            checks: selected_checks
+          },
+          %HostHealthChanged{host_id: host_id, health: Health.passing()}
+        ],
+        fn host ->
+          assert %Host{
+                   heartbeat: Health.passing(),
+                   selected_checks: ^selected_checks,
+                   checks_health: Health.unknown(),
+                   health: Health.passing()
+                 } = host
+        end
+      )
+    end
+
+    test "should not update health when an empty checks selection is saved" do
       host_id = Faker.UUID.v4()
       host_registered_event = build(:host_registered_event, host_id: host_id)
       heartbeat_succeded_event = build(:heartbeat_succeded, host_id: host_id)
@@ -416,15 +453,14 @@ defmodule Trento.Hosts.HostTest do
           %HostChecksSelected{
             host_id: host_id,
             checks: []
-          },
-          %HostHealthChanged{host_id: host_id, health: Health.passing()}
+          }
         ],
         fn host ->
           assert %Host{
                    heartbeat: Health.passing(),
                    selected_checks: [],
                    checks_health: Health.warning(),
-                   health: Health.passing()
+                   health: Health.warning()
                  } = host
         end
       )