Add ability to mark instances as absent #1731

jamie-suse · 2023-08-22T13:54:24Z

Description

When an instance is no longer responding, the corresponding MarkDatabaseInstanceAbsent/MarkApplicationInstanceAbsent command is dispatched (instead of Deregister*Instance). This even then applies an absent marker on the corresponding read/write models.

When an instance is registered, the absent field should be cleared.

How was this tested?

Updated unit tests to suit behaviour.

arbulu89

@jamie-suse Looking better.
Some work still to be done yet hehe

arbulu89 · 2023-08-23T14:16:20Z

lib/trento/domain/sap_system/commands/mark_application_instance_absent.ex

@@ -0,0 +1,15 @@
+defmodule Trento.Domain.Commands.MarkApplicationInstanceAbsent do
+  @moduledoc """
+  Mark an application instance as absent from the monitoring system.


I don't really like this from the monitoring system. It is kind of implicit, isn't it?

arbulu89 · 2023-08-23T14:17:09Z

lib/trento/domain/sap_system/instance.ex

@@ -17,5 +17,6 @@ defmodule Trento.Domain.SapSystem.Instance do
    field :health, Ecto.Enum, values: Health.values()
    field :system_replication, :string
    field :system_replication_status, :string
+    field :absent_at, :utc_datetime_usec, default: nil


nil is the default value by default XD
You don't need to add it

lib/trento/domain/sap_system/sap_system.ex

arbulu89 · 2023-08-23T14:23:45Z

lib/trento/domain/sap_system/sap_system.ex

+         },
+         %RegisterDatabaseInstance{sap_system_id: sap_system_id}
+       )
+       when not is_nil(absent_at) do


Can we maybe revert the logic to use a better pattern matching:

defp maybe_emit_database_instance_marked_present_event( %Instance{absent_at: nil}, %RegisterDatabaseInstance{}), do: nil defp maybe_emit_database_instance_marked_present_event( %Instance{}, %RegisterDatabaseInstance{sap_system_id: sap_system_id, ....} do %DatabaseInstanceMarkedPresent{ instance_number: instance_number, host_id: host_id, sap_system_id: sap_system_id } end

The same goes for the application part.
Besides that, have a look on where this function should go checking the order of usage please.
I didn't have a proper look myself

lib/trento/domain/sap_system/sap_system.ex

arbulu89 · 2023-08-23T14:27:36Z

lib/trento/domain/sap_system/sap_system.ex

+          absent_at: absent_at
+        }
+      ) do
+    %DatabaseInstanceMarkedAbsent{


Here we need to check if the instance was already absent.
We don't want to emit the event if it was already that way.
I guess simply get the instance and with a case check if the current value of absent. You don't need a new maybe function. Just do it here.

The same for the app

arbulu89 · 2023-08-23T14:29:07Z

lib/trento/domain/sap_system/sap_system.ex

+          application: %Application{instances: instances},
+          deregistered_at: deregistered_at
+        } = sap_system,
+        %RegisterApplicationInstance{host_id: host_id, instance_number: instance_number} = command


I wouldn't rename this to command. It was ok as instance

I thought to rename it as I need to use instance = get_instance(...) further down

arbulu89 · 2023-08-23T14:30:47Z

lib/trento/domain/sap_system/sap_system.ex

    sap_system
    |> Multi.new()
+    |> Multi.execute(fn _ ->
+      maybe_emit_application_instance_marked_present_event(instance, command)


Let's put this after the maybe_emit_application_instance_registered_or_moved_event event.
Besides that, and to follow with the already existing logic, I would pass sap_system/instance to the function, and run the get_instance within that one.
It looks cleaner and you reduce a duplicated code a bit (as you do it twice now)

I was trying to make this in-line with the existing logic in execute(%SapSystem{}, %RegisterDatabaseInstance{}). But I've changed it 👍

Referring to the logic seen here:

web/lib/trento/domain/sap_system/sap_system.ex

Line 223 in bac7caa

def execute(

arbulu89 · 2023-08-24T11:59:44Z

lib/trento/domain/sap_system/sap_system.ex

+      ) do
+    %Instance{absent_at: instance_absent_at} = get_instance(instances, host_id, instance_number)
+
+    case instance_absent_at do


case get_instance(instances, host_id, instance_number) do %Instance{absent_at: nil} -> %DatabaseInstanceMarkedAbsent{ instance_number: instance_number, host_id: host_id, sap_system_id: sap_system_id, absent_at: absent_at } _ -> nil end

jamie-suse · 2023-08-29T11:29:48Z

FYI I had to rebase, so I squashed most commits down

arbulu89

Okey! Much better!
I think we need to work a bit more in the tests.
I would split the tests in smaller pieces and separate the databases and application flows, after all they have a totally independent flow

lib/trento/domain/sap_system/sap_system.ex

arbulu89 · 2023-08-29T13:21:24Z

test/trento/domain/sap_system/sap_system_test.exs

@@ -4202,6 +4208,361 @@ defmodule Trento.SapSystemTest do
    end
  end

+  describe "delta deregistration" do


I wouldn't call this delta deregistration.
Maybe instance marked absent/present or similar?

arbulu89 · 2023-08-29T13:23:28Z

test/trento/domain/sap_system/sap_system_test.exs

@@ -4202,6 +4208,361 @@ defmodule Trento.SapSystemTest do
    end
  end

+  describe "delta deregistration" do
+    test "newly registered instances should be marked as present" do


Do we need this 1st test?
We have plenty of tests where we have a regular registration. We could add the absent_at pattern matching in any of them.

If you still want to keep it, i think we should still split the database and application parts.
But the test must be fixed in any case.
The database_instance_registered must not go in the initial events, as this is the event which triggers the change, so I would suggest to use the command in the 2nd param of assert_events_and_state

test/trento/domain/sap_system/sap_system_test.exs

arbulu89 · 2023-08-29T13:26:47Z

test/trento/domain/sap_system/sap_system_test.exs

+      )
+    end
+
+    test "absent instances receiving 'mark as absent' command should mark as absent" do


I would put this test before the others.
The tests can tell the story of the flow as well, so we would set to absent before setting it back to present, right?

arbulu89 · 2023-08-29T13:27:53Z

test/trento/domain/sap_system/sap_system_test.exs

+      )
+    end
+
+    test "absent instances receiving 'mark as absent' command should mark as absent" do


I would rename this to should mark as absent a previously registered instance

test/trento/domain/sap_system/sap_system_test.exs

arbulu89 · 2023-08-29T13:30:18Z

lib/trento/domain/sap_system/sap_system.ex

+    Enum.map(
+      instances,
+      fn instance ->
+        if instance.instance_number == instance_number and instance.host_id == host_id do


Can we pattern match instead of this if ?

arbulu89 · 2023-08-29T13:32:07Z

lib/trento/domain/sap_system/sap_system.ex

@@ -1199,4 +1385,17 @@ defmodule Trento.Domain.SapSystem do
        false
    end)
  end
+
+  defp update_instance(instances, instance_number, host_id, key, value) do


I don't really like this key/value thing.
Could we send the whole new update and use maybe Map.merge?
https://hexdocs.pm/elixir/1.12/Map.html#merge/2

arbulu89

Some tiny things.
Ready to merge after them

arbulu89 · 2023-08-30T07:10:59Z

lib/trento/domain/sap_system/sap_system.ex

-
-    embeds_one :database, Database
-    embeds_one :application, Application
+    field(:sap_system_id, Ecto.UUID)


Formatting error. Remove parenthesis please

arbulu89 · 2023-08-30T07:14:16Z

lib/trento/domain/sap_system/sap_system.ex

+      instances,
+      fn
+        %Instance{instance_number: ^instance_number, host_id: ^host_id} = instance ->
+          struct(instance, fields)


Maybe I would put Kernel.struct to exactly know the module. Up to you

test/trento/domain/sap_system/sap_system_test.exs

arbulu89 · 2023-08-30T07:19:34Z

test/trento/domain/sap_system/sap_system_test.exs

@@ -4202,6 +4210,365 @@ defmodule Trento.SapSystemTest do
    end
  end

+  describe "instance marked absent/present" do
+    test "should mark as absent a previously registered database instance" do


I'm not sure if mixing 2 different flows is a good idea, as it "hides" them.
I'm talking about the scenario where you test here that an already marked instance is not re-marked.
No need for a change, the test is totally correct.
For the future

arbulu89 · 2023-08-30T07:20:51Z

test/trento/domain/sap_system/sap_system_test.exs

+          sap_system_id: sap_system_id,
+          sid: sid,
+          host_id: host_id,
+          instance_number: absent_app_instance_number,


This is not an app instance in reallity, as it is a MESSAGESERVER

arbulu89 · 2023-08-30T07:23:10Z

test/trento/domain/sap_system/sap_system_test.exs

+      )
+    end
+
+    test "receiving a 'register instance' command for an already-registered database instance sets instance as present if absent" do


I would rephrase this to:

should mark as present and already registered absent database instance

We use the should wording at the beginning of each test by default

arbulu89 · 2023-08-30T07:25:21Z

test/trento/domain/sap_system/sap_system_test.exs

+        ],
+        fn state ->
+          assert %SapSystem{
+                   database: %SapSystem.Database{


SapSystem.Database to Database (already aliased in top).
The same for Instance and Application in all tests

arbulu89 · 2023-08-30T07:26:21Z

test/trento/domain/sap_system/sap_system_test.exs

+          sap_system_id: sap_system_id,
+          host_id: host_id,
+          instance_number: absent_app_instance_number,
+          absent_at: DateTime.utc_now()


Shouldn't this absent_at be created by the factory?

arbulu89

Alright!
Green light from my side.
@rtorrero would you like to give a second look if I missed something?

jamie-suse added the elixir Pull requests that update Elixir code label Aug 22, 2023

jamie-suse marked this pull request as draft August 22, 2023 13:54

arbulu89 requested changes Aug 23, 2023

View reviewed changes

jamie-suse added the env Create an ephimeral environment for the pr branch label Aug 23, 2023

nelsonkopliku mentioned this pull request Aug 24, 2023

Instance deregistration endpoint #1730

Merged

arbulu89 reviewed Aug 24, 2023

View reviewed changes

jamie-suse added 3 commits August 29, 2023 13:23

Mark hosts as absent on backend

e6aa120

Revert changes to cluster

c345e99

Add tests and rework implementation

2b76bce

jamie-suse force-pushed the mark-absent branch from acf9125 to 2b76bce Compare August 29, 2023 11:27

Abstract repeated code into a private helper function

ba582d4

jamie-suse marked this pull request as ready for review August 29, 2023 13:21

arbulu89 requested changes Aug 29, 2023

View reviewed changes

jamie-suse added 4 commits August 29, 2023 18:02

Reordering tests

18b8113

Move test for initial conditions into pre-existing tests

1eccd96

Separate database/application instance tests

50c36f7

Refactor update_instance() to use pattern-matching

919f257

jamie-suse requested review from arbulu89, rtorrero and CDimonaco August 30, 2023 06:49

arbulu89 requested changes Aug 30, 2023

View reviewed changes

jamie-suse added 6 commits August 30, 2023 09:46

Revert formatting

f69ecd3

Use namespaced Kernel.struct()

9c5105f

Renaming

7cfc072

Remove SapSystem namespace

64a3e01

Remove redundant initialisation of default fields

f322059

Renaming

6a6f2f4

jamie-suse requested a review from arbulu89 August 30, 2023 09:26

arbulu89 approved these changes Aug 30, 2023

View reviewed changes

arbulu89 added the enhancement New feature or request label Aug 30, 2023

jamie-suse added env Create an ephimeral environment for the pr branch and removed env Create an ephimeral environment for the pr branch labels Aug 30, 2023

rtorrero approved these changes Aug 30, 2023

View reviewed changes

arbulu89 merged commit 8a4da21 into main Aug 30, 2023

arbulu89 deleted the mark-absent branch August 30, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to mark instances as absent #1731

Add ability to mark instances as absent #1731

jamie-suse commented Aug 22, 2023

arbulu89 left a comment

arbulu89 Aug 23, 2023

arbulu89 Aug 23, 2023

arbulu89 Aug 23, 2023

arbulu89 Aug 23, 2023

arbulu89 Aug 23, 2023

jamie-suse Aug 24, 2023

arbulu89 Aug 23, 2023

jamie-suse Aug 24, 2023

jamie-suse Aug 24, 2023

arbulu89 Aug 24, 2023 •

edited

Loading

jamie-suse commented Aug 29, 2023

arbulu89 left a comment

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 Aug 29, 2023

arbulu89 left a comment

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 Aug 30, 2023

arbulu89 left a comment

Add ability to mark instances as absent #1731

Add ability to mark instances as absent #1731

Conversation

jamie-suse commented Aug 22, 2023

Description

How was this tested?

arbulu89 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

arbulu89 Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

jamie-suse commented Aug 29, 2023

arbulu89 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

arbulu89 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

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Aug 24, 2023 •

edited

Loading