Add endpoint to deregister a host #1450

jamie-suse · 2023-05-24T07:54:07Z

Description

Adds endpoint DELETE /api/v1/hosts/{host_id} to request the server to deregister a host.

Response behaviour:

If last heartbeat is after timeout period: 204
If last heartbeat is within timeout period: 422
If host with ID host_id doesn't exist: 404
If attempting to deregister a host that is already deregistered: 404

How was this tested?

By adding unit tests

config/config.exs

test/trento/application/usecases/hosts_test.exs

lib/trento/application/usecases/hosts/hosts.ex

lib/trento_web/controllers/v1/host_controller.ex

lib/trento_web/router.ex

lib/trento/application/usecases/hosts/hosts.ex

arbulu89

@jamie-suse @rtorrero
Some comments from my side.
Please have a look on the tests.

lib/trento/application/usecases/hosts/hosts.ex

config/config.exs

lib/trento/application/usecases/hosts/hosts.ex

arbulu89 · 2023-05-29T07:35:37Z

lib/trento/application/usecases/hosts/hosts.ex

+      from(h in Heartbeat,
+        where:
+          h.timestamp >
+            ^DateTime.add(now, -total_deregistration_debounce, :millisecond) and


Going back in time with the - here hehe
I find this "negative" logic a bit confusing, but well, I guess it works.

I found the h.timestamp + total_debounce < now and sending the command in true easier.
But not a hard opinion, leave it as you best think

lib/trento/application/usecases/hosts/hosts.ex

arbulu89 · 2023-05-29T07:40:23Z

lib/trento_web/openapi/schema/host.ex

+          nullable: true,
+          format: :"date-time"
+        },
+        heartbeat_timestamp: %Schema{


Is this correct?
I though we were sending a struct here with agent_id/timestamp combo

arbulu89 · 2023-05-29T07:46:30Z

test/trento/application/usecases/hosts_test.exs

@@ -38,4 +41,70 @@ defmodule Trento.HostsTest do
      refute deregistered_host.id in hosts_ids
    end
  end
+
+  describe "deregister_host" do
+    test "should deregister a host with unknown health" do


I'm totally disoriented with these tests.
As far as I see the code, I don't see any place where the heartbeat state is even used.
How does this work?
Please, have a look on the tests. I don't find any logic on this.

And maybe add setup :verify_on_exit! on top of the test file. I think almost all (if not all) the expects are wrong.

CDimonaco

Guys just some comments,

Rename the association to hearbeat, updating all the preloading etc..
When we deal with controllers and controller tests, use the openapi spex test helper when needed, the inconsistency between what the controller returns and the schema could be spotted easily
Rephrase the tests, we are not dealing explicitely with health, so clarify it or change the phrasing

config/config.exs

config/test.exs

CDimonaco · 2023-06-05T11:27:59Z

lib/trento/application/integration/discovery/protocol/enrich_request_host_deregistration.ex

+          | {:error, :host_alive}
+          | {:error, :host_not_registered}
+  def enrich(%RequestHostDeregistration{host_id: host_id} = command, _) do
+    host_exists = Repo.exists?(HostReadModel, id: host_id)


Please short circuit here, we don't need to perform the query below, if the host does not exists

You can use different functions, or maybe a with - else block, but it's better if we avoid to perform useless query, not perfomance wise obvs, the query is really simple, but for general correctness

lib/trento/application/read_models/host_read_model.ex

lib/trento/application/usecases/hosts/hosts.ex

CDimonaco · 2023-06-05T11:38:02Z

...rento/application/integration/discovery/protocol/enrich_request_host_deregistration_test.exs

+               Enrichable.enrich(command, %{})
+    end
+
+    test "should return an error if a host with critical health is requested to be deregistered before debounce timer" do


same as before

CDimonaco · 2023-06-05T11:38:32Z

...rento/application/integration/discovery/protocol/enrich_request_host_deregistration_test.exs

+      assert {:error, :host_alive} == Enrichable.enrich(command, %{})
+    end
+
+    test "should return an error if a host with passing health is requested to be deregistered" do


CDimonaco · 2023-06-05T11:44:01Z

lib/trento_web/controllers/v1/host_controller.ex

+  def delete(conn, %{id: host_id}) do
+    case Hosts.deregister_host(host_id) do
+      :ok -> send_resp(conn, 204, "")
+      {:error, :host_alive} -> send_resp(conn, 422, "")


We are seding a no content for 422 without a reason? We should have the appropriate response for the 422 as described in OpenApiSpex.JsonErrorResponse.response(), please check the schema, you are describing it

CDimonaco · 2023-06-05T11:44:41Z

test/trento_web/controllers/v1/host_controller_test.exs

+
+      conn
+      |> delete("/api/v1/hosts/#{host_id}")
+      |> response(422)


Same as the controller comment, we should use the correct schema for 422 as described in open api and we should check here with openapi spex test helper that the schema is respected

CDimonaco · 2023-06-05T11:45:25Z

test/trento_web/controllers/v1/host_controller_test.exs

+
+      conn
+      |> delete("/api/v1/hosts/#{host_id}")
+      |> response(:not_found)


Not found is a json schema object, described by our openapi spec for that controller, please use the appropriate test helper from the openapi library

arbulu89

It looks good in general!
Once @CDimonaco comments are addresses feel free to merge from my side

config/config.exs

config/test.exs

lib/trento_web/controllers/v1/host_controller.ex

arbulu89 · 2023-06-05T14:30:34Z

...rento/application/integration/discovery/protocol/enrich_request_host_deregistration_test.exs

+      now = DateTime.utc_now()
+
+      %{id: id} = insert(:host)
+      insert(:heartbeat, agent_id: id, timestamp: DateTime.add(DateTime.utc_now(), -6, :second))


Is this value related to the values you put in the test.exs file.
Maybe you could fetch the value from the config file, so if you change it, the test doesn't break

arbulu89

Ready to merge once you address @CDimonaco comments

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

* Host deregistration events (#1245) * Host deregistration commands * Host deregistration events * Fixing module attributes in host deregistration commands * Fix deregistration commands namespace * Deregistration side effects (#1252) * Add deregistered_at to read model Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Handle side effects from HostDeregistered event Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Filter deregistered hosts from get_all_hosts Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Fix tests Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add test for filtering unregistered hosts Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Host Deregistration process manager and aggregate handling (#1249) * Host aggregate handling of deregistration event * Wip Deregistration process manager * Process Managers supervisor * Add Deregistration process manager test * Fix credo and mispells * Squashed Process manager state into process manager * Routing deregistration commands to Host aggregate * Fix deregistration process manager state with deftype * Format router file * Deregistration timestamp carried from Deregistration requested command * Fix spell in process manager docstring * Fix typo in test description * Cluster and cluster host deregistration events/commands (#1274) * Cluster and cluster host deregistration events/commands * Address comments * Remove leading space * Fix Hosts query without the soft deleted hosts exclusion (#1282) * Cluster registration also for nodes not DC (#1275) * Cluster registration process happens also when the discovered node is not a DC * Address docs and testing review feedbacks * Removed useless tests from cluster_tests * Add host to cluster when a message from a dc host arrives for a cluster Co-Authored-By: Fabrizio Sestito <fabrizio.sestito@suse.com> * Add register cluster host test when the host is and is not a DC --------- Co-authored-by: Fabrizio Sestito <fabrizio.sestito@suse.com> * Cluster deregistration process manager & aggregate changes (#1278) * Make Process Manager aware of cluster deregistration Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Handle cluster host deregistration in the cluster aggregate Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Route the DeregisterClusterHost to the cluster aggregate --------- Co-authored-by: Jamie Rodriguez <jamie.rodriguez@suse.com> * Cluster deregistered side effects (#1280) * Add deregistered_at field to Cluster read model * ClusterDeregistered event projection * Wip on clusters module, get clusters that are not soft deleted * Clusters entrypoint tests * Test broadcast of cluster deregistration in cluster projector * Removed first from request_check_execution query * Fix query parenthesis in request_clusters_checks_execution * get_all_clusters, tested for deregistered and soft deleted clusters * Handle HostRemovedFromCluster in host projection * Cluster projector reorder of project clauses * Host projector set the cluster to null when HostRemovedFromCluster * Correct typos (#1315) * Database registration changes (#1326) * Database sap system registration change The database istance is register only if the sr is disabled or with sr enabled but only primary instances * Sap systems moduledoc updated * Changed error when registering a non primary database for a sap system * Updated moduledoc of sap system domain * Refuse database registration changes The registration is refused if the sap system does not exists and the database has a secondary role * Fix sap system domain tests * Fix typos * Format * Add SAP System deregistration commands & events (#1314) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Fix Deregistration process manager state validation * Add application instance deregistration logic (#1357) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> * Application instances registration changes (#1358) * Add SAP System de/registration logic to Process Manager (#1356) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add database instance deregistration logic (#1372) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add SAP System deregistration side effects (#1387) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Detect ascs/ers cluster type (#1392) * Detect ascs/ers cluster type * Update functions order to improve pattern matching * Ascs ers type frontend (#1398) * Add ascs/ers cluster type to the frontend * Improve e2e testing of the clusters view * Discover ASCS/ERS cluster SIDs (#1400) * Discover ascs/ers cluster sids * Add additional_sids to domain and side effects * Add additional_sids to cluster api schema * Handle `:database_not_registered` errors (#1405) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add polymorphic embed usage to deftype macro (#1411) * Properly handle multiple errors returned (#1413) * Display ASCS/ERS clusters SID in clusters view (#1403) * Show additional sids in the clusters view * Adapt e2e tests to check for sids * Order get_all_clusters output by id * Revert HeartbeatSucceeded to old name (#1429) * Add deregistration websocket events (#1424) Co-authored-by: Jamie Rodríguez <jamie.rodriguez@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Discover ASCS/ERS clusters health and initial details (#1422) * Add nodes entry to the crm payload * Add ascs/ers cluster details value objects * Implement ascs/ers cluster health discovery * Update cluster details to the polymorphic embed * Add tests to the ascs/ers cluster health discovery * Fix validate_required usage * Set default value to additional_sids in cluster events (#1441) * Cluster delta deregistration (#1386) * Add get_cluster_id_by_host_id/1 * Add delta deregistration logic * Add additional ascs/ers cluster details discovery (#1442) * Add additional ascs/ers cluster details discovery * Add comment to parse_ascs_ers_cluster_nodes * Wrap long function in do/end * Set false instead of nil in Enum.filter function * Deregistration process manager retry the rollup errors from aggregates (#1473) * Fix HostRemovedFromCluster concurrency issues (#1474) * Basic deregistration leftovers (#1461) * test * Add deregistered_at field to Database * Fix update of Application and Database when deregistering instances * Put deregistered_at timestamp instead of nil sid on database deregistration * Remove sid to nil when Sap system is deregistered * Emit sap system deregistered when the database is deregistered * Emit database deregistered event when no instances left and db is not already deregistered * Sap systems domain deregistration tests refactored * format sap system domain * Emit database deregistered event when they are instances only when the db is not registered * Removed redundant nil default to deregistered_at in database * Fix mispell in sap system tests * Add fake_sid util to sap system testing suite * Sap system does not get deregistered a second time * Changed test titles in sap system test, to remove "a second time" misleading phrase * Deregistration tombstoning rollup (#1425) * Host tombstoning on host deregistration * Add host tombstone rollup handling in stream event handler * Cluster tombstoned event and lifespan when cluster is deregistered * Cluster rollup triggered on Cluster tombstone event * Move cluster tombstoned event to cluster domain folder * Sap system aggregate emits tombstone event * Sap system aggregate tombstoning test * SapSystem tombstoned event triggers sap system rollup * Removed tombstoned events from lifespan stop mechanism in all aggregates * Formatted Cluster/Host tombstoned events * Fix logging string in stream rollup event handler * Addressing review feedbacks * Credo format * Change redirect plug to use a list of available versions (#1487) * Change redirect plug to use a list of available versions * Add sanity check for empty available api versions list * Use Keyword.get to get the opts * Enable openapi schema versioning (#1488) * Convert the open api plug into a versinable macro * Move V1 schemas to its own folder * Update controllers to used versioned openapi * Update router to pipethrough specific open api version * Disable open api cache on dev * Update controller tests to use new openapi versions * Temporarily set openapi V1 version in CI docs generation * Test paths generation in open api spec * Add endpoint to deregister a host (#1450) Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> * Add V2 scope to /api/clusters endpoint (#1492) * Add openapi v2 schemas * Add clusters v2 endpoint controller and view * Update pheonix router to add v2 endpoints * Adapt clusters v1 schema and view to be backward compatible * Use clusters v2 version on projector * Remove deprecated code * Use /api/v2/clusters endpoint in frontend * Improve and fix some new schema implementations * Aggregate clauses reorder (#1495) * Reorder host aggregate clauses, host_id: nil clauses compacted * Sap system clauses sap_system_id: nil, compacted * Sap system delta deregistration (#1491) * Fix cluster delta computation * Add get_application_instances_by_host_id/1 and get_database_instances_by_host_id/1 * Add delta deregistration logic * Identify sap systems ensa version (#1496) * Identify sap systems ensa version * Replace nil by no_ensa atom in EnsaVersion enum * DatabaseDeregistered side effects (#1504) * Add deregistered_at field to databases read model * Handle database deregistered side effects * Filtered deregistered databases in the usecases * Make `HostReadModel.last_heartbeat_timestamp` a virtual field (#1505) * Project removal of db/app instances to the read models (#1507) * Project removal of db/app instances to the read models * Reject commands when aggregate deregistered (#1513) * Host aggregate reject commands when the host is deregistered * Cluster aggregate reject all commands except registration when deregistered * Sap system aggregate rejects commands when deregistered * Host aggregate rejects updates when deregistered * Cluster aggregate rejects all commands when deregistered * Fix host tests * Ensa version domain (#1512) * Add ensa_version to SapSystemReadModel * Add SapSystemUpdated event and domain logic * Add and update sap system domain tests * Add SapSystemUpdated projection * Add new ensa_version field in openapi schema * Add test to check updated event is not emitted * Update migration to set default value afterwards * Create AscsErsClusterRole enum (#1520) * Host restoration (#1517) * Host restoration in Host aggregate * Host restoration projector * Addressing typo feedbacks * Move host details update after dergistered_at guard clause * Broadcast sap system updated (#1516) * Broadcast SapSystemUpdated event * Update sap systems in redux state upon update event * Cluster restoration (#1523) * Extract cluster update procedure to separate function * Cluster restoration * Cluster restored projection * Addressing review feedbacks * Addressing review feedbacks * Move sagas functions to separate files according to adr 0009 (#1531) * Update health summary endpoint (#1530) * Update health summary service and view * Update openapi schema and deprecate old fields * Polish the code and handle unclustered systems * Fix function specs * Remove fixed sid usage from db instance creation in test * Frontend instance removal on broadcast (#1522) * Add sid to instance deregistration broadcast * Remove instances from upon instance deregistration broadcast * Apply various cleanups to the database state * Use Repo.get! where possible * Improve notification message * Fix wrong test placement * Add test for instances removal notify * Fix sap system domain apply ordering issues (#1550) * Move application instances (#1554) * Add Deregistration modal (#1537) * Allow rollup on deregistered aggregates (#1551) * Complete tests on command acceptance/rejection (#1563) * Sap system restoration (#1545) * Database restoration in sap system aggregate * Database restore projection * Sap system restore domain * Sap system restore projection * Emit application instance registered or moved in restore * Removed health multi change on sap system restoration * Remove sap_system_id setting in database restored apply * Removed sid from DatabaseRestored event * Removed sid field from SapSytemRestored event --------- Co-authored-by: Carmine Di Monaco <carmine.dimonaco@suse.com> Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com> Co-authored-by: Fabrizio Sestito <fabrizio.sestito@suse.com> Co-authored-by: Jamie Rodriguez <jamie.rodriguez@suse.com> Co-authored-by: Carmine Di Monaco <carmine.dimonaco@gmail.com> Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com> Co-authored-by: Eugen Maksymenko <eugen.maksymenko@suse.com> Co-authored-by: Jurgen Goldschmidt <jurgen.goldschmidt@suse.com>

jamie-suse added enhancement New feature or request elixir Pull requests that update Elixir code labels May 24, 2023

jamie-suse force-pushed the deregistration-api-controller branch 3 times, most recently from 9237393 to afc9b99 Compare May 26, 2023 08:46

jamie-suse commented May 26, 2023

View reviewed changes

config/config.exs Outdated Show resolved Hide resolved

jamie-suse commented May 26, 2023

View reviewed changes

test/trento/application/usecases/hosts_test.exs Outdated Show resolved Hide resolved

jamie-suse marked this pull request as ready for review May 26, 2023 11:13

jamie-suse requested review from fabriziosestito, arbulu89, CDimonaco and rtorrero May 26, 2023 11:13

CDimonaco requested changes May 26, 2023

View reviewed changes

arbulu89 requested changes May 29, 2023

View reviewed changes

jamie-suse requested review from arbulu89 and CDimonaco June 5, 2023 07:49

CDimonaco requested changes Jun 5, 2023

View reviewed changes

arbulu89 reviewed Jun 5, 2023

View reviewed changes

arbulu89 approved these changes Jun 6, 2023

View reviewed changes

jamie-suse requested a review from CDimonaco June 6, 2023 11:24

jamie-suse and others added 7 commits June 6, 2023 15:54

Add endpoint to deregister a host

d8ca6ad

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

Address initial feedback

31bee9e

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

Rename middleware helper function

e22aca7

Make middleware tests use config debounce values

53a6590

Preload heartbeat instead of extra database query in middleware

6e5c106

Cover cases when deregistering an already deregistered host

e48ae21

Assert 422 and 404 response schemas

6f0bd91

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

jamie-suse force-pushed the deregistration-api-controller branch from 24292bc to 6f0bd91 Compare June 6, 2023 14:35

CDimonaco approved these changes Jun 7, 2023

View reviewed changes

jamie-suse merged commit 51fd837 into deregistration Jun 7, 2023

jamie-suse deleted the deregistration-api-controller branch June 7, 2023 07:54

rtorrero added a commit that referenced this pull request Jun 16, 2023

Add endpoint to deregister a host (#1450)

851d1dd

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

rtorrero added a commit that referenced this pull request Jun 16, 2023

Add endpoint to deregister a host (#1450)

bada74c

Co-authored-by: Rubén Torrero Marijnissen <rtorreromarijnissen@suse.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint to deregister a host #1450

Add endpoint to deregister a host #1450

jamie-suse commented May 24, 2023 •

edited

Loading

arbulu89 left a comment

arbulu89 May 29, 2023

arbulu89 May 29, 2023

arbulu89 May 29, 2023

CDimonaco left a comment

CDimonaco Jun 5, 2023

CDimonaco Jun 5, 2023

CDimonaco Jun 5, 2023

CDimonaco Jun 5, 2023 •

edited

Loading

CDimonaco Jun 5, 2023

CDimonaco Jun 5, 2023

arbulu89 left a comment

arbulu89 Jun 5, 2023

arbulu89 left a comment

Add endpoint to deregister a host #1450

Add endpoint to deregister a host #1450

Conversation

jamie-suse commented May 24, 2023 • edited Loading

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

CDimonaco 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

CDimonaco Jun 5, 2023 • edited Loading

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

arbulu89 left a comment

Choose a reason for hiding this comment

jamie-suse commented May 24, 2023 •

edited

Loading

CDimonaco Jun 5, 2023 •

edited

Loading