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 1 commit
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
9 changes: 5 additions & 4 deletions lib/trento/clusters/cluster.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
nelsonkopliku marked this conversation as resolved.
Show resolved Hide resolved

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
9 changes: 5 additions & 4 deletions lib/trento/hosts/host.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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]
Expand Down
76 changes: 76 additions & 0 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,82 @@ defmodule Trento.ClusterTest do
)
end

test "checks health should not affect health when checks health is unknown" do
nelsonkopliku marked this conversation as resolved.
Show resolved Hide resolved
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{
nelsonkopliku marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
44 changes: 40 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,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)
Expand Down Expand Up @@ -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
)
Expand Down