Send and receive explicit restoration events #1673

jamie-suse · 2023-07-26T14:07:12Z

Description

Previously, when a restoration occurred on the backend, *_registered events were being sent. Now we need to explicitly send events indicating that a restoration (*_restored event) has occurred.

sap_system_restored and database_restored events also send database_instances (and additionally application_instances, in the case of sap_system_restored) data, which is required to update the UI.

Fixes TRNT-2355

How was this tested?

Updated projector tests to ensure they send the new events.

Added tests for new Sagas and new SAP systems reducer functions.

arbulu89

You still need to preload the database_instances in the database restoration and maybe change the view

arbulu89 · 2023-07-27T14:49:37Z

lib/trento/application/projectors/sap_system_projector.ex

@@ -341,11 +341,11 @@ defmodule Trento.SapSystemProjector do
    sap_system =
      SapSystemReadModel
      |> Repo.get!(sap_system_id)
-      |> Repo.preload([:tags])
+      |> Repo.preload([:tags, :database_instances, :application_instances])


Something I noticed here as that we could even do this:

@impl true def after_update( %SapSystemRestored{}, _, %{sap_system: %SapSystemReadModel{} = sap_system} ) do enriched_sap_system = Repo.preload(sap_system, [ :application_instances, :database_instances, :tags ]) ....

I mean, we don't need to query, as the read model already comes in the 3rd parameter of the after_update.
Not a big deal though.

arbulu89 · 2023-07-27T14:50:17Z

lib/trento/application/projectors/database_projector.ex

@@ -306,7 +306,7 @@ defmodule Trento.DatabaseProjector do

    TrentoWeb.Endpoint.broadcast(
      @databases_topic,
-      "database_registered",
+      "database_restored",


Here you need to preload the database_instances

jamie-suse · 2023-07-31T15:24:12Z

test/e2e/cypress/e2e/databases_overview.cy.js

@@ -36,7 +36,7 @@ context('Databases Overview', () => {
      cy.contains('div', hdqDatabase.instances[1].name).should('exist');
    });

-    it('should show the ACTIVE pill in the right host', () => {
+    it.skip('should show the ACTIVE pill in the right host', () => {


Note: this test is for bug TRNT-2357.
This skip needs to be removed is the PR fixing that bug.

mamma mia...
How we have ended like this now? 😭

arbulu89

Many comments...
The PR is pretty huge, so I don't know if I missed something..

arbulu89 · 2023-08-01T06:45:04Z

assets/js/state/clusters.js

@@ -102,6 +102,7 @@ export const clustersListSlice = createSlice({
 });

 export const CLUSTER_DEREGISTERED = 'CLUSTER_DEREGISTERED';
+export const CLUSTER_RESTORED = 'CLUSTER_RESTORED';


Why a new line after this?

arbulu89 · 2023-08-01T06:47:37Z

assets/js/state/databases.js

@@ -30,6 +30,12 @@ export const databasesListSlice = createSlice({
    appendDatabaseInstance: (state, action) => {
      state.databaseInstances = [...state.databaseInstances, action.payload];
    },
+    upsertDatabaseInstances: (state, action) => {


I don't really like this now.
I think that the filterByInstances should be changed to be upsertInstances and include the concat there.
In fact, I think we should remove the upsertDatabaseInstance and only leave upsertDatabaseInstances, because the we have the risk for appending already existing instances with the previous function (I would even leave appendDatabaseInstances as it is easier to understand)
So I would replace the plural usage everywhere

arbulu89 · 2023-08-01T06:53:05Z

assets/js/state/sagas/clusters.js

+  yield put(appendCluster(payload));
+  yield put(
+    notify({
+      text: `Cluster, ${payload.name}, has been restored.`,


@abravosuse Do we want to use this notifications with the restoration wording, or should we still use registered?

I'd remove the ,s

arbulu89 · 2023-08-01T06:54:54Z

assets/js/state/sagas/index.js

  yield debounce(
    debounceDuration,
    HOST_DEREGISTERED,
    loadSapSystemsHealthSummary
  );
+  yield debounce(debounceDuration, HOST_RESTORED, loadSapSystemsHealthSummary);


Is this formatted?
Anyway, the order of things looks unsorted.
Could we maybe have all deregistered first and all restored afterwards?

Yeah, the formatter keeps one-line-ing it 🤦‍♂️.

Moved all the restored events below the deregistered events 👍.

I notice there is not a listener here for DATABASE_DEREGISTERED, should this be added in?

arbulu89 · 2023-08-01T07:01:01Z

assets/js/state/sapSystems.test.js

+    const action = upsertDatabaseInstances(newInstances);
+
+    const expectedState = {
+      databaseInstances: initialState.databaseInstances


Really complex...
Cannot we have a fixed expected thing so everyone can understand in a first glance what's going on?

arbulu89 · 2023-08-01T07:02:32Z

lib/trento/application/projectors/database_projector.ex

+      |> Repo.preload([
+        :tags,
+        :database_instances,
+        database_instances: [host: :tags],


Why do we need this host and tags preloaded instances?
Why do we even need to have them twice?

I just included them otherwise the JSON serialiser was complaining.

Why do we even need to have them twice?

One is the :tags for the DatabaseReadModel, the other is :tags for HostReadModel.

I can see how to remove the HostReadModel tags if it's not desired.

So we need to fix the view them, not add database_instances twice loading hosts that we don't need, right?

So you don't want to include the hosts data from database_instances?

Is there any reason why we need it?
We don't use it in the frontend as far as I know

arbulu89 · 2023-08-01T07:03:57Z

test/e2e/cypress/e2e/databases_overview.cy.js

@@ -36,7 +36,7 @@ context('Databases Overview', () => {
      cy.contains('div', hdqDatabase.instances[1].name).should('exist');
    });

-    it('should show the ACTIVE pill in the right host', () => {
+    it.skip('should show the ACTIVE pill in the right host', () => {


mamma mia...
How we have ended like this now? 😭

arbulu89 · 2023-08-01T07:56:15Z

lib/trento_web/views/v1/sap_system_view.ex

@@ -40,7 +40,6 @@ defmodule TrentoWeb.V1.SapSystemView do
    database
    |> Map.from_struct()
    |> Map.delete(:__meta__)
-    |> Map.delete(:database_instances)


By the way, here we might want to use add_system_replication_status_to_secondary_instance so we put the pills properly.
We need to see how it behaves

arbulu89

Looks good!
Just a couple of last comments, but you don't need a new review.
I hope you tested how it works manually as well 😉

arbulu89 · 2023-08-01T13:02:01Z

assets/js/state/sagas/databases.js

@@ -70,6 +71,17 @@ export function* databaseDeregistered({ payload }) {
  );
 }

+export function* databaseRestored({ payload }) {
+  yield put(appendDatabase(payload));
+  yield put(upsertDatabaseInstances(payload.database_instances));


Don't we need yield put(upsertDatabaseInstancesToSapSystem([payload])); here?

arbulu89 · 2023-08-01T13:04:21Z

lib/trento/application/projectors/database_projector.ex

@@ -302,11 +302,11 @@ defmodule Trento.DatabaseProjector do
    database =
      DatabaseReadModel
      |> Repo.get!(sap_system_id)
-      |> Repo.preload([:tags])
+      |> Repo.preload([:tags, :database_instances, [database_instances: :host]])


Can we remove this [database_instances: :host]?

rtorrero

LGTM, thanks @jamie-suse

jamie-suse added the elixir Pull requests that update Elixir code label Jul 26, 2023

jamie-suse marked this pull request as draft July 26, 2023 14:07

jamie-suse force-pushed the restoration-bug-projectors branch 2 times, most recently from fb8e403 to c50ee6b Compare July 27, 2023 14:06

jamie-suse changed the title ~~Send cluster_restored & sap_system_restored websocket events~~ Send and receive explicit restoration events Jul 27, 2023

arbulu89 reviewed Jul 27, 2023

View reviewed changes

jamie-suse marked this pull request as ready for review July 27, 2023 16:42

jamie-suse force-pushed the restoration-bug-projectors branch from 160f81b to 0d5d2e2 Compare July 31, 2023 12:17

jamie-suse added the javascript Pull requests that update Javascript code label Jul 31, 2023

jamie-suse requested review from arbulu89, rtorrero and CDimonaco July 31, 2023 15:12

jamie-suse commented Jul 31, 2023

View reviewed changes

arbulu89 requested changes Aug 1, 2023

View reviewed changes

arbulu89 reviewed Aug 1, 2023

View reviewed changes

arbulu89 mentioned this pull request Aug 1, 2023

Fix SR status broadcast on database instance registration #1687

Merged

jamie-suse added 14 commits August 1, 2023 14:39

Send explicit restoration events from backend

7386060

Listen to restoration events

12dc38a

Add hosts Sagas

ee4a77e

Add databases Sagas

218a020

Add clusters Sagas

0fe29d1

Add SAP systems Sagas

9f5e046

Respond to restoration events with Sagas

eeec28c

Avoid unnecessary query

be64fab

Preload database instances during database restoration

2fea19d

Preload host of each DatabaseInstanceReadModel

b192c0a

Preload everything

eb877a3

Reordering/reorganising

fe41e7b

Use upserts instead of appending SAP instances

e55d06a

Don't load host data in database_restored.json view

37b6681

jamie-suse force-pushed the restoration-bug-projectors branch from 475fe2f to 37b6681 Compare August 1, 2023 12:40

Enable end-to-end tests

02990a7

jamie-suse requested a review from arbulu89 August 1, 2023 12:48

arbulu89 approved these changes Aug 1, 2023

View reviewed changes

jamie-suse added 3 commits August 1, 2023 15:09

Update database projector test

44bea21

Don't load host data for DatabaseRestored event

3abf8ea

Add database instances to SAP system when database restored

0afb270

rtorrero approved these changes Aug 1, 2023

View reviewed changes

Remove ,'s from toast notifications

0a89342

arbulu89 merged commit be3813c into main Aug 1, 2023
18 checks passed

arbulu89 deleted the restoration-bug-projectors branch August 1, 2023 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send and receive explicit restoration events #1673

Send and receive explicit restoration events #1673

jamie-suse commented Jul 26, 2023 •

edited

arbulu89 left a comment

arbulu89 Jul 27, 2023

arbulu89 Jul 27, 2023

jamie-suse Jul 31, 2023

arbulu89 Aug 1, 2023

arbulu89 left a comment

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023

rtorrero Aug 1, 2023 •

edited

arbulu89 Aug 1, 2023

jamie-suse Aug 1, 2023

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023

jamie-suse Aug 1, 2023

arbulu89 Aug 1, 2023

jamie-suse Aug 1, 2023

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023 •

edited

arbulu89 left a comment

arbulu89 Aug 1, 2023

arbulu89 Aug 1, 2023

rtorrero left a comment

Send and receive explicit restoration events #1673

Send and receive explicit restoration events #1673

Conversation

jamie-suse commented Jul 26, 2023 • edited

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

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

rtorrero Aug 1, 2023 • edited

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 1, 2023 • edited

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

rtorrero left a comment

Choose a reason for hiding this comment

jamie-suse commented Jul 26, 2023 •

edited

rtorrero Aug 1, 2023 •

edited

arbulu89 Aug 1, 2023 •

edited