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.

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

Send and receive explicit restoration events #1673

Merged
merged 19 commits into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/js/state/channels.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const processChannelEvents = (reduxStore, socket) => {
'heartbeat_succeded',
'heartbeat_failed',
'host_deregistered',
'host_restored',
]);
registerEvents(reduxStore, socket, 'monitoring:clusters', [
'cluster_registered',
Expand All @@ -32,6 +33,7 @@ const processChannelEvents = (reduxStore, socket) => {
'cluster_health_changed',
'cluster_cib_last_written_updated',
'cluster_deregistered',
'cluster_restored',
]);
registerEvents(reduxStore, socket, 'monitoring:sap_systems', [
'sap_system_registered',
Expand All @@ -41,11 +43,13 @@ const processChannelEvents = (reduxStore, socket) => {
'application_instance_deregistered',
'application_instance_health_changed',
'sap_system_deregistered',
'sap_system_restored',
'sap_system_updated',
]);
registerEvents(reduxStore, socket, 'monitoring:databases', [
'database_registered',
'database_deregistered',
'database_restored',
'database_health_changed',
'database_instance_registered',
'database_instance_deregistered',
Expand Down
2 changes: 1 addition & 1 deletion assets/js/state/clusters.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export const clustersListSlice = createSlice({
});

export const CLUSTER_DEREGISTERED = 'CLUSTER_DEREGISTERED';

export const CLUSTER_RESTORED = 'CLUSTER_RESTORED';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a new line after this?

export const CLUSTER_CHECKS_SELECTED = 'CLUSTER_CHECKS_SELECTED';
export const checksSelected = createAction(CLUSTER_CHECKS_SELECTED);

Expand Down
12 changes: 8 additions & 4 deletions assets/js/state/databases.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createSlice } from '@reduxjs/toolkit';
import { maybeUpdateInstanceHealth } from './instances';
import { upsertInstances, maybeUpdateInstanceHealth } from './instances';

const initialState = {
loading: false,
Expand Down Expand Up @@ -27,8 +27,11 @@ export const databasesListSlice = createSlice({
appendDatabase: (state, action) => {
state.databases = [...state.databases, action.payload];
},
appendDatabaseInstance: (state, action) => {
state.databaseInstances = [...state.databaseInstances, action.payload];
upsertDatabaseInstances: (state, action) => {
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 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

state.databaseInstances = upsertInstances(
state.databaseInstances,
action.payload
);
},
removeDatabase: (state, { payload: { id } }) => {
state.databases = state.databases.filter(
Expand Down Expand Up @@ -101,6 +104,7 @@ export const databasesListSlice = createSlice({

export const DATABASE_REGISTERED = 'DATABASE_REGISTERED';
export const DATABASE_DEREGISTERED = 'DATABASE_DEREGISTERED';
export const DATABASE_RESTORED = 'DATABASE_RESTORED';
export const DATABASE_HEALTH_CHANGED = 'DATABASE_HEALTH_CHANGED';
export const DATABASE_INSTANCE_REGISTERED = 'DATABASE_INSTANCE_REGISTERED';
export const DATABASE_INSTANCE_DEREGISTERED = 'DATABASE_INSTANCE_DEREGISTERED';
Expand All @@ -116,7 +120,7 @@ export const {
appendDatabase,
removeDatabase,
removeDatabaseInstance,
appendDatabaseInstance,
upsertDatabaseInstances,
updateDatabaseHealth,
updateDatabaseInstanceHealth,
updateDatabaseInstanceSystemReplication,
Expand Down
24 changes: 24 additions & 0 deletions assets/js/state/databases.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import databaseReducer, {
removeDatabase,
removeDatabaseInstance,
upsertDatabaseInstances,
} from '@state/databases';
import {
databaseFactory,
Expand Down Expand Up @@ -47,4 +48,27 @@ describe('Databases reducer', () => {

expect(databaseReducer(initialState, action)).toEqual(expectedState);
});

it('should upsert database instances', () => {
const initialInstances = databaseInstanceFactory.buildList(2);

const initialState = {
databaseInstances: initialInstances,
};

const updatedInstance = {
...initialState.databaseInstances[0],
instance_hostname: 'my_name_has_changed',
};
const newInstance = databaseInstanceFactory.build();
const newInstances = [updatedInstance, newInstance];

const action = upsertDatabaseInstances(newInstances);

const expectedState = {
databaseInstances: [initialInstances[1], ...newInstances],
};

expect(databaseReducer(initialState, action)).toEqual(expectedState);
});
});
1 change: 1 addition & 0 deletions assets/js/state/hosts.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ export const CHECK_HOST_IS_DEREGISTERABLE = 'CHECK_HOST_IS_DEREGISTERABLE';
export const CANCEL_CHECK_HOST_IS_DEREGISTERABLE =
'CANCEL_CHECK_HOST_IS_DEREGISTERABLE';
export const HOST_DEREGISTERED = 'HOST_DEREGISTERED';
export const HOST_RESTORED = 'HOST_RESTORED';
export const DEREGISTER_HOST = 'DEREGISTER_HOST';

export const checkHostIsDeregisterable = createAction(
Expand Down
10 changes: 10 additions & 0 deletions assets/js/state/instances.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@ const payloadMatchesInstance = (payload, instance) =>
payload.host_id === instance.host_id &&
payload.instance_number === instance.instance_number;

const filterByInstances = (currentInstances, newInstances) =>
currentInstances.filter((currentInstance) =>
newInstances.every(
(newInstance) => !payloadMatchesInstance(newInstance, currentInstance)
)
);

export const upsertInstances = (currentInstances, newInstances) =>
filterByInstances(currentInstances, newInstances).concat(newInstances);

export const maybeUpdateInstanceHealth = (payload, instance) => {
if (payloadMatchesInstance(payload, instance)) {
instance.health = payload.health;
Expand Down
21 changes: 20 additions & 1 deletion assets/js/state/sagas/clusters.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { put, takeEvery } from 'redux-saga/effects';

import { notify } from '@state/actions/notifications';
import { CLUSTER_DEREGISTERED, removeCluster } from '@state/clusters';
import {
CLUSTER_DEREGISTERED,
CLUSTER_RESTORED,
appendCluster,
removeCluster,
} from '@state/clusters';

export function* clusterDeregistered({ payload: { name, id } }) {
yield put(removeCluster({ id }));
Expand All @@ -13,6 +18,20 @@ export function* clusterDeregistered({ payload: { name, id } }) {
);
}

export function* clusterRestored({ payload }) {
yield put(appendCluster(payload));
yield put(
notify({
text: `Cluster, ${payload.name}, has been restored.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@rtorrero rtorrero Aug 1, 2023

Choose a reason for hiding this comment

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

I'd remove the ,s

icon: 'ℹ️',
})
);
}

export function* watchClusterDeregistered() {
yield takeEvery(CLUSTER_DEREGISTERED, clusterDeregistered);
}

export function* watchClusterRestored() {
yield takeEvery(CLUSTER_RESTORED, clusterRestored);
}
19 changes: 17 additions & 2 deletions assets/js/state/sagas/clusters.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@ import { recordSaga } from '@lib/test-utils';

import { clusterFactory } from '@lib/test-utils/factories';

import { clusterDeregistered } from '@state/sagas/clusters';
import { removeCluster } from '@state/clusters';
import { clusterDeregistered, clusterRestored } from '@state/sagas/clusters';
import { removeCluster, appendCluster } from '@state/clusters';
import { notify } from '@state/actions/notifications';

describe('Clusters sagas', () => {
it('should remove the cluster', async () => {
Expand All @@ -15,4 +16,18 @@ describe('Clusters sagas', () => {

expect(dispatched).toContainEqual(removeCluster({ id }));
});

it('should restore the cluster', async () => {
const payload = clusterFactory.build();

const dispatched = await recordSaga(clusterRestored, { payload });

expect(dispatched).toEqual([
appendCluster(payload),
notify({
text: `Cluster, ${payload.name}, has been restored.`,
icon: 'ℹ️',
}),
]);
});
});
21 changes: 17 additions & 4 deletions assets/js/state/sagas/databases.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ import { put, select, takeEvery } from 'redux-saga/effects';
import {
DATABASE_REGISTERED,
DATABASE_DEREGISTERED,
DATABASE_RESTORED,
DATABASE_HEALTH_CHANGED,
DATABASE_INSTANCE_REGISTERED,
DATABASE_INSTANCE_DEREGISTERED,
DATABASE_INSTANCE_HEALTH_CHANGED,
DATABASE_INSTANCE_SYSTEM_REPLICATION_CHANGED,
appendDatabase,
appendDatabaseInstance,
upsertDatabaseInstances,
updateDatabaseHealth,
updateDatabaseInstanceHealth,
updateDatabaseInstanceSystemReplication,
Expand All @@ -17,7 +18,7 @@ import {
} from '@state/databases';

import {
appendDatabaseInstanceToSapSystem,
upsertDatabaseInstancesToSapSystem,
removeDatabaseInstanceFromSapSystem,
updateSAPSystemDatabaseInstanceHealth,
updateSAPSystemDatabaseInstanceSystemReplication,
Expand Down Expand Up @@ -50,8 +51,8 @@ function* databaseHealthChanged({ payload }) {
}

function* databaseInstanceRegistered({ payload }) {
yield put(appendDatabaseInstance(payload));
yield put(appendDatabaseInstanceToSapSystem(payload));
yield put(upsertDatabaseInstances([payload]));
yield put(upsertDatabaseInstancesToSapSystem([payload]));
yield put(
notify({
text: `A new Database instance, ${payload.sid}, has been discovered.`,
Expand All @@ -70,6 +71,17 @@ export function* databaseDeregistered({ payload }) {
);
}

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

Choose a reason for hiding this comment

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

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

yield put(
notify({
text: `The database ${payload.sid} has been restored.`,
icon: 'ℹ️',
})
);
}

export function* databaseInstanceDeregistered({ payload }) {
yield put(removeDatabaseInstance(payload));
yield put(removeDatabaseInstanceFromSapSystem(payload));
Expand All @@ -94,6 +106,7 @@ function* databaseInstanceSystemReplicationChanged({ payload }) {
export function* watchDatabase() {
yield takeEvery(DATABASE_REGISTERED, databaseRegistered);
yield takeEvery(DATABASE_DEREGISTERED, databaseDeregistered);
yield takeEvery(DATABASE_RESTORED, databaseRestored);
yield takeEvery(DATABASE_HEALTH_CHANGED, databaseHealthChanged);
yield takeEvery(DATABASE_INSTANCE_REGISTERED, databaseInstanceRegistered);
yield takeEvery(DATABASE_INSTANCE_DEREGISTERED, databaseInstanceDeregistered);
Expand Down
25 changes: 24 additions & 1 deletion assets/js/state/sagas/databases.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ import { recordSaga } from '@lib/test-utils';
import {
databaseDeregistered,
databaseInstanceDeregistered,
databaseRestored,
} from '@state/sagas/databases';
import { removeDatabase, removeDatabaseInstance } from '@state/databases';
import {
upsertDatabaseInstances,
removeDatabase,
removeDatabaseInstance,
appendDatabase,
} from '@state/databases';
import { removeDatabaseInstanceFromSapSystem } from '@state/sapSystems';
import {
databaseFactory,
Expand Down Expand Up @@ -55,4 +61,21 @@ describe('SAP Systems sagas', () => {
})
);
});

it('should restore the database', async () => {
const database = databaseFactory.build();

const dispatched = await recordSaga(databaseRestored, {
payload: database,
});

expect(dispatched).toEqual([
appendDatabase(database),
upsertDatabaseInstances(database.database_instances),
notify({
text: `The database ${database.sid} has been restored.`,
icon: 'ℹ️',
}),
]);
});
});
16 changes: 16 additions & 0 deletions assets/js/state/sagas/hosts.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import {
CANCEL_CHECK_HOST_IS_DEREGISTERABLE,
HOST_DEREGISTERED,
DEREGISTER_HOST,
HOST_RESTORED,
removeHost,
setHostListDeregisterable,
setHostDeregistering,
setHostNotDeregistering,
appendHost,
} from '@state/hosts';

import { notify } from '@state/actions/notifications';
Expand Down Expand Up @@ -69,6 +71,16 @@ export function* deregisterHost({
}
}

export function* hostRestored({ payload }) {
yield put(appendHost(payload));
yield put(
notify({
text: `Host ${payload.hostname} has been restored.`,
icon: 'ℹ️',
})
);
}

export function* watchHostDeregisterable() {
yield takeEvery(CHECK_HOST_IS_DEREGISTERABLE, checkHostDeregisterable);
}
Expand All @@ -80,3 +92,7 @@ export function* watchHostDeregistered() {
export function* watchDeregisterHost() {
yield takeEvery(DEREGISTER_HOST, deregisterHost);
}

export function* watchHostRestored() {
yield takeEvery(HOST_RESTORED, hostRestored);
}
16 changes: 16 additions & 0 deletions assets/js/state/sagas/hosts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
checkHostDeregisterable,
hostDeregistered,
deregisterHost,
hostRestored,
} from '@state/sagas/hosts';

import {
Expand All @@ -15,6 +16,7 @@ import {
removeHost,
setHostDeregistering,
setHostNotDeregistering,
appendHost,
} from '@state/hosts';

import { networkClient } from '@lib/network';
Expand Down Expand Up @@ -112,4 +114,18 @@ describe('Hosts sagas', () => {
setHostNotDeregistering(payload),
]);
});

it('should restore a host', async () => {
const host = hostFactory.build();

const dispatched = await recordSaga(hostRestored, { payload: host });

expect(dispatched).toEqual([
appendHost(host),
notify({
text: `Host ${host.hostname} has been restored.`,
icon: 'ℹ️',
}),
]);
});
});