Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregated target health does not take into account checks selection status #2050

Merged
merged 7 commits into from
Dec 1, 2023
Merged

Aggregated target health does not take into account checks selection status #2050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ba600e1
Aggregated target health does not take into account checks selection
jamie-suse Nov 30, 2023
86d9dc9
Make test descriptions clearer
jamie-suse Dec 1, 2023
b18f013
Make logic easier on the eyes (and brain)
jamie-suse Dec 1, 2023
a02a198
Use `:unknown` for default value of `checks_health` in cluster aggregate
jamie-suse Dec 1, 2023
a3e9624
Fix test description
jamie-suse Dec 1, 2023
a8a19df
Remove host health recalculation on checks selection
nelsonkopliku Dec 1, 2023
c02ec49
Remove cluster health recalculation on checks selection
jamie-suse Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 9 additions & 17 deletions lib/trento/clusters/cluster.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ defmodule Trento.Clusters.Cluster do
field :hosts_number, :integer
field :provider, Ecto.Enum, values: Provider.values()
field :discovered_health, Ecto.Enum, values: Health.values()
field :checks_health, Ecto.Enum, values: Health.values()
field :checks_health, Ecto.Enum, values: Health.values(), default: Health.unknown()
field :health, Ecto.Enum, values: Health.values(), default: Health.unknown()
field :hosts, {:array, :string}, default: []
field :selected_checks, {:array, :string}, default: []
Expand Down Expand Up @@ -294,22 +294,15 @@ defmodule Trento.Clusters.Cluster do
def execute(
%Cluster{
cluster_id: cluster_id
} = cluster,
},
%SelectChecks{
checks: selected_checks
}
) do
cluster
|> Multi.new()
|> Multi.execute(fn _ ->
[
%ChecksSelected{
cluster_id: cluster_id,
checks: selected_checks
}
]
end)
|> Multi.execute(fn cluster -> maybe_emit_cluster_health_changed_event(cluster) end)
%ChecksSelected{
cluster_id: cluster_id,
checks: selected_checks
}
end

def execute(
Expand Down Expand Up @@ -605,19 +598,18 @@ 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, Health.unknown()), do: healths
defp maybe_add_checks_health(healths, checks_health), do: [checks_health | 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()

Expand Down
22 changes: 8 additions & 14 deletions lib/trento/hosts/host.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,20 +392,15 @@ defmodule Trento.Hosts.Host do
end

def execute(
%Host{} = host,
%Host{host_id: host_id},
%SelectHostChecks{
checks: selected_checks
}
) do
host
|> Multi.new()
|> Multi.execute(
&%HostChecksSelected{
host_id: &1.host_id,
checks: selected_checks
}
)
|> Multi.execute(&maybe_emit_host_health_changed_event/1)
%HostChecksSelected{
host_id: host_id,
checks: selected_checks
}
end

def execute(
Expand Down Expand Up @@ -730,15 +725,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()
Expand All @@ -748,8 +742,8 @@ 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, Health.unknown()), do: healths
defp maybe_add_checks_health(healths, checks_health), do: [checks_health | healths]

defp maybe_add_saptune_health(healths, Health.unknown()), do: healths
defp maybe_add_saptune_health(healths, saptune_health), do: [saptune_health | healths]
Expand Down
86 changes: 79 additions & 7 deletions test/trento/clusters/cluster_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,78 @@ defmodule Trento.ClusterTest do
)
end

test "unknown checks health should not affect aggregated cluster's health" 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()),
%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)
Expand Down Expand Up @@ -523,7 +595,7 @@ defmodule Trento.ClusterTest do
host_added_to_cluster_event,
%ClusterChecksHealthChanged{
cluster_id: cluster_registered_event.cluster_id,
checks_health: :unknown
checks_health: Health.unknown()
},
%ChecksSelected{
cluster_id: cluster_registered_event.cluster_id,
Expand Down Expand Up @@ -557,7 +629,7 @@ defmodule Trento.ClusterTest do
fn cluster ->
%Cluster{
discovered_health: :warning,
checks_health: :unknown,
checks_health: Health.unknown(),
health: :warning
} = cluster
end
Expand Down Expand Up @@ -687,8 +759,8 @@ defmodule Trento.ClusterTest do
health: cluster_registered_event.health,
hosts: [],
selected_checks: [],
discovered_health: :passing,
checks_health: nil
discovered_health: Health.passing(),
checks_health: Health.unknown()
}
},
fn %Cluster{rolling_up: rolling_up} ->
Expand Down Expand Up @@ -718,8 +790,8 @@ defmodule Trento.ClusterTest do
health: cluster_registered_event.health,
hosts: [],
selected_checks: [],
discovered_health: :passing,
checks_health: nil
discovered_health: Health.passing(),
checks_health: Health.unknown()
}
}
],
Expand All @@ -736,7 +808,7 @@ defmodule Trento.ClusterTest do
assert cluster.health == cluster_registered_event.health
assert cluster.hosts == []
assert cluster.selected_checks == []
assert cluster.discovered_health == :passing
assert cluster.discovered_health == Health.passing()
end
)
end
Expand Down
43 changes: 39 additions & 4 deletions test/trento/hosts/host_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,43 @@ defmodule Trento.Hosts.HostTest do
end
end

test "should ignore checks health when an empty checks selection is saved" do
test "unknown checks health should not affect aggregated host's health" 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
}
],
fn host ->
assert %Host{
heartbeat: Health.passing(),
selected_checks: ^selected_checks,
checks_health: Health.unknown(),
health: Health.warning()
} = 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)
Expand Down Expand Up @@ -416,15 +452,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
)
Expand Down