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

Handle software updates discovery errors #2532

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Handle software updates discovery errors #2532

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa22775
Add testing scenarios for host's health transition to unknown when so…
nelsonkopliku Apr 18, 2024
0e1005b
Handle software updates discovery errors by dispatching an unknown he…
nelsonkopliku Apr 18, 2024
0307ce6
Adjust software updates event handler tests
nelsonkopliku Apr 18, 2024
e67c703
Simplify dispatching on discovery error
nelsonkopliku Apr 18, 2024
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
54 changes: 38 additions & 16 deletions lib/trento/software_updates/discovery.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,32 +80,54 @@ defmodule Trento.SoftwareUpdates.Discovery do
end

def discover_host_software_updates(host_id, fully_qualified_domain_name) do
with {:ok, system_id} <- get_system_id(fully_qualified_domain_name),
{:ok, relevant_patches} <- get_relevant_patches(system_id),
:ok <-
host_id
|> build_discovery_completion_command(relevant_patches)
|> commanded().dispatch() do
{:ok, host_id, system_id, relevant_patches}
else
error ->
case discover_relevant_patches(fully_qualified_domain_name) do
{:ok, system_id, relevant_patches} ->
determine_health_and_dispatch_completion_command(host_id, system_id, relevant_patches)

{:error, discovery_error} = error ->
Logger.error(
"An error occurred during software updates discovery for host #{host_id}: #{inspect(error)}"
)

{:error, error}
dispatch_completion_command_on_discovery_error(host_id, discovery_error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand a thing 😅
Wasn't as simple as putting the the dispatch with the unknown state in the else side?

commanded().dispatch(
      CompleteSoftwareUpdatesDiscovery.new!(%{
        host_id: host_id,
        health: SoftwareUpdatesHealth.unknown()
    })
)

error

Copy link
Member Author

@nelsonkopliku nelsonkopliku Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's where I started from, honestly.

I wanted to avoid dispatching a CompleteSoftwareUpdatesDiscovery command with unknown when the failure was not a discovery failure, but an error in dispatching CompleteSoftwareUpdatesDiscovery after a successful discovery, as mentioned in the description.

I'd be fine also with that dispatch in the else side 🙈

end
end

defp build_discovery_completion_command(host_id, relevant_patches),
do:
defp discover_relevant_patches(fully_qualified_domain_name) do
with {:ok, system_id} <- get_system_id(fully_qualified_domain_name),
{:ok, relevant_patches} <- get_relevant_patches(system_id) do
{:ok, system_id, relevant_patches}
end
end

defp determine_health_and_dispatch_completion_command(host_id, system_id, relevant_patches) do
with :ok <-
relevant_patches
|> track_relevant_patches
|> compute_software_updates_discovery_health
|> dispatch_completion_command(host_id) do
{:ok, host_id, system_id, relevant_patches}
end
end

defp dispatch_completion_command(discovered_software_updates_health, host_id) do
commanded().dispatch(
CompleteSoftwareUpdatesDiscovery.new!(%{
host_id: host_id,
health:
relevant_patches
|> track_relevant_patches
|> compute_software_updates_discovery_health
health: discovered_software_updates_health
})
)
end

defp dispatch_completion_command_on_discovery_error(host_id, discovery_error) do
case dispatch_completion_command(SoftwareUpdatesHealth.unknown(), host_id) do
:ok ->
{:error, discovery_error}

{:error, dispatching_error} ->
{:error, [discovery_error, dispatching_error]}
end
end

defp track_relevant_patches(relevant_patches),
do:
Expand Down
18 changes: 18 additions & 0 deletions test/trento/hosts/host_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1736,6 +1736,24 @@ defmodule Trento.Hosts.HostTest do
initial_host_health: Health.critical(),
software_updates_discovery_health: SoftwareUpdatesHealth.passing(),
expected_host_health: Health.passing()
},
%{
initial_host_health: Health.passing(),
software_updates_discovery_health: SoftwareUpdatesHealth.unknown(),
expected_host_health: Health.unknown()
},
%{
initial_host_health: Health.critical(),
initial_heartbeat: :heartbeat_failed,
software_updates_discovery_health: SoftwareUpdatesHealth.unknown(),
expected_host_health: Health.unknown()
},
%{
initial_host_health: Health.unknown(),
initial_heartbeat: :heartbeat_failed,
software_updates_discovery_health: SoftwareUpdatesHealth.unknown(),
expect_host_health_changed: false,
expected_host_health: Health.unknown()
}
]

Expand Down
9 changes: 7 additions & 2 deletions ...infrastructure/commanded/event_handlers/software_updates_discovery_event_handler_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ defmodule Trento.Infrastructure.Commanded.EventHandlers.SoftwareUpdatesDiscovery

alias Trento.Infrastructure.Commanded.EventHandlers.SoftwareUpdatesDiscoveryEventHandler

require Trento.SoftwareUpdates.Enums.SoftwareUpdatesHealth, as: SoftwareUpdatesHealth

setup [:set_mox_from_context, :verify_on_exit!]

test "should discover software updates when a SoftwareUpdatesDiscoveryRequested is emitted" do
Expand Down Expand Up @@ -63,8 +65,11 @@ defmodule Trento.Infrastructure.Commanded.EventHandlers.SoftwareUpdatesDiscovery
expect(
Trento.Commanded.Mock,
:dispatch,
0,
fn _ -> :ok end
fn %CompleteSoftwareUpdatesDiscovery{
health: SoftwareUpdatesHealth.unknown()
} ->
:ok
end
)

assert :ok = SoftwareUpdatesDiscoveryEventHandler.handle(event, %{})
Expand Down
131 changes: 106 additions & 25 deletions test/trento/software_updates/discovery_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
host_id = Faker.UUID.v4()
fully_qualified_domain_name = Faker.Internet.domain_name()

discovery_error = {:error, :some_error_while_getting_system_id}
discovery_error = :some_error_while_getting_system_id

fail_on_getting_system_id(fully_qualified_domain_name, discovery_error)
fail_on_getting_system_id(host_id, fully_qualified_domain_name, discovery_error)

{:error, ^discovery_error} =
Discovery.discover_host_software_updates(host_id, fully_qualified_domain_name)
Expand All @@ -43,9 +43,14 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
host_id = Faker.UUID.v4()
fully_qualified_domain_name = Faker.Internet.domain_name()
system_id = 100
discovery_error = {:error, :some_error_while_getting_relevant_patches}
discovery_error = :some_error_while_getting_relevant_patches

fail_on_getting_relevant_patches(fully_qualified_domain_name, system_id, discovery_error)
fail_on_getting_relevant_patches(
host_id,
fully_qualified_domain_name,
system_id,
discovery_error
)

{:error, ^discovery_error} =
Discovery.discover_host_software_updates(host_id, fully_qualified_domain_name)
Expand All @@ -55,7 +60,7 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
host_id = Faker.UUID.v4()
fully_qualified_domain_name = Faker.Internet.domain_name()
system_id = 100
dispatching_error = {:error, :error_while_dispatching_completion_command}
dispatching_error = :error_while_dispatching_completion_command

fail_on_dispatching_completion_command(
host_id,
Expand All @@ -68,6 +73,57 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
Discovery.discover_host_software_updates(host_id, fully_qualified_domain_name)
end

test "should handle failure when dispatching discovery completion command on discovery error" do
for discovery_error <- [:failing_get_system_id, :failing_get_relevant_patches] do
host_id = Faker.UUID.v4()
system_id = 100
fully_qualified_domain_name = Faker.Internet.domain_name()
dispatching_error = :dispatching_error

expect(
SoftwareUpdatesDiscoveryMock,
:get_system_id,
fn ^fully_qualified_domain_name ->
case discovery_error do
:failing_get_system_id -> {:error, discovery_error}
_ -> {:ok, system_id}
end
end
)

expected_get_relevant_patches_calls =
case discovery_error do
:failing_get_system_id -> 0
_ -> 1
end

expect(
SoftwareUpdatesDiscoveryMock,
:get_relevant_patches,
expected_get_relevant_patches_calls,
fn _ ->
if discovery_error == :failing_get_relevant_patches do
{:error, discovery_error}
end
end
)

expect(
Trento.Commanded.Mock,
:dispatch,
fn %CompleteSoftwareUpdatesDiscovery{
host_id: ^host_id,
health: SoftwareUpdatesHealth.unknown()
} ->
{:error, dispatching_error}
end
)

{:error, [^discovery_error, ^dispatching_error]} =
Discovery.discover_host_software_updates(host_id, fully_qualified_domain_name)
end
end

test "should complete discovery" do
scenarios = [
%{
Expand Down Expand Up @@ -160,9 +216,9 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
test "should handle errors when getting a system id" do
%{id: host_id, fully_qualified_domain_name: fully_qualified_domain_name} = insert(:host)

discovery_error = {:error, :some_error_while_getting_system_id}
discovery_error = :some_error_while_getting_system_id

fail_on_getting_system_id(fully_qualified_domain_name, discovery_error)
fail_on_getting_system_id(host_id, fully_qualified_domain_name, discovery_error)

{:ok, {[], errored_discoveries}} = Discovery.discover_software_updates()

Expand All @@ -175,7 +231,12 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
system_id = 100
discovery_error = {:error, :some_error_while_getting_relevant_patches}

fail_on_getting_relevant_patches(fully_qualified_domain_name, system_id, discovery_error)
fail_on_getting_relevant_patches(
host_id,
fully_qualified_domain_name,
system_id,
discovery_error
)

{:ok, {[], errored_discoveries}} = Discovery.discover_software_updates()

Expand All @@ -187,7 +248,7 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do

system_id = 100

dispatching_error = {:error, :error_while_dispatching_completion_command}
dispatching_error = :error_while_dispatching_completion_command

fail_on_dispatching_completion_command(
host_id,
Expand Down Expand Up @@ -236,16 +297,18 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
system_id3 => {:ok, discovered_relevant_patches}
}

expected_software_updates_health = SoftwareUpdatesHealth.critical()

expected_commands = %{
host_id1 => %CompleteSoftwareUpdatesDiscovery{
host_id: host_id1,
health: expected_software_updates_health
health: SoftwareUpdatesHealth.critical()
},
host_id2 => %CompleteSoftwareUpdatesDiscovery{
host_id: host_id2,
health: SoftwareUpdatesHealth.unknown()
},
host_id3 => %CompleteSoftwareUpdatesDiscovery{
host_id: host_id3,
health: expected_software_updates_health
health: SoftwareUpdatesHealth.critical()
}
}

Expand All @@ -272,8 +335,8 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
expect(
Trento.Commanded.Mock,
:dispatch,
2,
fn %{host_id: host_id} = command ->
3,
fn %CompleteSoftwareUpdatesDiscovery{host_id: host_id} = command ->
assert Map.get(expected_commands, host_id) == command

:ok
Expand All @@ -292,7 +355,7 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
] ==
successful_discoveries

assert {:error, host_id2, {:error, :some_error}} in errored_discoveries
assert {:error, host_id2, :some_error} in errored_discoveries
assert {:error, host_id4, :host_without_fqdn} in errored_discoveries
end
end
Expand Down Expand Up @@ -329,11 +392,11 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
end
end

defp fail_on_getting_system_id(fully_qualified_domain_name, discovery_error) do
defp fail_on_getting_system_id(host_id, fully_qualified_domain_name, discovery_error) do
expect(
SoftwareUpdatesDiscoveryMock,
:get_system_id,
fn ^fully_qualified_domain_name -> discovery_error end
fn ^fully_qualified_domain_name -> {:error, discovery_error} end
)

expect(
Expand All @@ -346,12 +409,21 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
expect(
Trento.Commanded.Mock,
:dispatch,
0,
fn _ -> :ok end
fn %CompleteSoftwareUpdatesDiscovery{
host_id: ^host_id,
health: SoftwareUpdatesHealth.unknown()
} ->
:ok
end
)
end

defp fail_on_getting_relevant_patches(fully_qualified_domain_name, system_id, discovery_error) do
defp fail_on_getting_relevant_patches(
host_id,
fully_qualified_domain_name,
system_id,
discovery_error
) do
expect(
SoftwareUpdatesDiscoveryMock,
:get_system_id,
Expand All @@ -361,14 +433,18 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
expect(
SoftwareUpdatesDiscoveryMock,
:get_relevant_patches,
fn ^system_id -> discovery_error end
fn ^system_id -> {:error, discovery_error} end
)

expect(
Trento.Commanded.Mock,
:dispatch,
0,
fn _ -> :ok end
fn %CompleteSoftwareUpdatesDiscovery{
host_id: ^host_id,
health: SoftwareUpdatesHealth.unknown()
} ->
:ok
end
)
end

Expand All @@ -393,7 +469,12 @@ defmodule Trento.SoftwareUpdates.DiscoveryTest do
expect(
Trento.Commanded.Mock,
:dispatch,
fn %CompleteSoftwareUpdatesDiscovery{host_id: ^host_id} -> dispatching_error end
fn %CompleteSoftwareUpdatesDiscovery{
host_id: ^host_id,
health: SoftwareUpdatesHealth.critical()
} ->
{:error, dispatching_error}
end
)
end
end
Loading