From b4ba600998d2764e6acbf36c021d4cb63afed05e Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Thu, 8 Nov 2018 14:51:54 -0500 Subject: [PATCH] feat(app): Track restart status in discovery state for better alerts (#2639) Closes #2516 --- .../components/RobotSettings/ConnectBanner.js | 7 +- .../RobotSettings/ReachableRobotBanner.js | 38 ++++++-- app/src/discovery/__tests__/actions.test.js | 11 +++ app/src/discovery/__tests__/reducer.test.js | 49 +++++++++- app/src/discovery/__tests__/selectors.test.js | 91 ++++++++++++++++--- app/src/discovery/index.js | 57 ++++++++++-- app/src/discovery/selectors.js | 21 +++-- app/src/discovery/types.js | 5 +- .../http-api-client/__tests__/server.test.js | 3 + app/src/http-api-client/server.js | 2 + app/src/index.js | 3 +- app/src/robot/api-client/client.js | 9 ++ 12 files changed, 247 insertions(+), 49 deletions(-) diff --git a/app/src/components/RobotSettings/ConnectBanner.js b/app/src/components/RobotSettings/ConnectBanner.js index dc08c6c4061..1a61a12820b 100644 --- a/app/src/components/RobotSettings/ConnectBanner.js +++ b/app/src/components/RobotSettings/ConnectBanner.js @@ -13,16 +13,15 @@ export default class ConnectBanner extends React.Component { } render () { - const {name, connected} = this.props + const {displayName, connected} = this.props const isVisible = connected && !this.state.dismissed - const TITLE = `${name} successfully connected` - if (!isVisible) return null + return ( this.setState({dismissed: true})} - title={TITLE} + title={`${displayName} successfully connected`} /> ) } diff --git a/app/src/components/RobotSettings/ReachableRobotBanner.js b/app/src/components/RobotSettings/ReachableRobotBanner.js index 3e81bd9975e..3e15b03b35e 100644 --- a/app/src/components/RobotSettings/ReachableRobotBanner.js +++ b/app/src/components/RobotSettings/ReachableRobotBanner.js @@ -7,7 +7,21 @@ import type {ReachableRobot} from '../../discovery' type State = {dismissed: boolean} -const TITLE = 'Unable to establish connection with robot' +const UNRESPONSIVE_TITLE = 'Unable to establish connection with robot' +const RESTARTING_TITLE = 'Robot restarting' + +const LAST_RESORT = ( +

+ If your robot remains unresponsive, please reach out to our Support team. +

+) + +const RESTARTING_MESSAGE = ( +
+

Your robot is restarting, and should be back online shortly.

+ {LAST_RESORT} +
+) const NO_SERVER_MESSAGE = (
@@ -16,9 +30,7 @@ const NO_SERVER_MESSAGE = ( requests.

We recommend power-cycling your robot.

-

- If your robot remains unresponsive, please reach out to our Support team. -

+ {LAST_RESORT}
) @@ -40,9 +52,7 @@ const SERVER_MESSAGE = (

-

- If your robot remains unresponsive, please reach out to our Support team. -

+ {LAST_RESORT} ) @@ -56,9 +66,17 @@ export default class ReachableRobotBanner extends React.Component< } render () { - const {serverOk} = this.props + const {serverOk, restartStatus} = this.props const isVisible = !this.state.dismissed - const message = serverOk ? SERVER_MESSAGE : NO_SERVER_MESSAGE + let title = '' + let message = '' + if (restartStatus) { + title = RESTARTING_TITLE + message = RESTARTING_MESSAGE + } else { + title = UNRESPONSIVE_TITLE + message = serverOk ? SERVER_MESSAGE : NO_SERVER_MESSAGE + } if (!isVisible) return null @@ -66,7 +84,7 @@ export default class ReachableRobotBanner extends React.Component< this.setState({dismissed: true})} - title={TITLE} + title={title} > {message} diff --git a/app/src/discovery/__tests__/actions.test.js b/app/src/discovery/__tests__/actions.test.js index cc6286e65e7..6fd5bf8ba85 100644 --- a/app/src/discovery/__tests__/actions.test.js +++ b/app/src/discovery/__tests__/actions.test.js @@ -30,4 +30,15 @@ describe('discovery actions', () => { jest.runTimersToTime(expectedTimeout) expect(store.getActions()).toEqual([expectedStart, expectedFinish]) }) + + test('startDiscovery with timeout', () => { + const expectedTimeout = 60000 + const expectedStart = {type: 'discovery:START', meta: {shell: true}} + const expectedFinish = {type: 'discovery:FINISH', meta: {shell: true}} + + store.dispatch(startDiscovery(60000)) + expect(store.getActions()).toEqual([expectedStart]) + jest.runTimersToTime(expectedTimeout) + expect(store.getActions()).toEqual([expectedStart, expectedFinish]) + }) }) diff --git a/app/src/discovery/__tests__/reducer.test.js b/app/src/discovery/__tests__/reducer.test.js index ee7f145430f..30b979d451e 100644 --- a/app/src/discovery/__tests__/reducer.test.js +++ b/app/src/discovery/__tests__/reducer.test.js @@ -2,10 +2,10 @@ import {discoveryReducer} from '..' jest.mock('../../shell', () => ({ - getShellRobots: () => ([ + getShellRobots: () => [ {name: 'foo', ip: '192.168.1.1', port: 31950}, {name: 'bar', ip: '192.168.1.2', port: 31950}, - ]), + ], })) describe('discoveryReducer', () => { @@ -20,6 +20,7 @@ describe('discoveryReducer', () => { initialState: undefined, expectedState: { scanning: false, + restartsByName: {}, robotsByName: { foo: [{name: 'foo', ip: '192.168.1.1', port: 31950}], bar: [{name: 'bar', ip: '192.168.1.2', port: 31950}], @@ -49,12 +50,54 @@ describe('discoveryReducer', () => { ], }, }, - initialState: {robotsByName: {}}, + initialState: {robotsByName: {}, restartsByName: {}}, expectedState: { robotsByName: { foo: [{name: 'foo', ip: '192.168.1.1', port: 31950}], bar: [{name: 'bar', ip: '192.168.1.2', port: 31950}], }, + restartsByName: {}, + }, + }, + { + name: 'api:SERVER_SUCCESS sets restart pending', + action: { + type: 'api:SERVER_SUCCESS', + payload: {path: 'restart', robot: {name: 'name'}}, + }, + initialState: {restartsByName: {}}, + expectedState: {restartsByName: {name: 'pending'}}, + }, + { + name: 'discovery:UPDATE_LIST sets restart down if pending robot not ok', + action: { + type: 'discovery:UPDATE_LIST', + payload: { + robots: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: false}], + }, + }, + initialState: {restartsByName: {name: 'pending'}}, + expectedState: { + robotsByName: { + name: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: false}], + }, + restartsByName: {name: 'down'}, + }, + }, + { + name: 'discovery:UPDATE_LIST clears restart if down robot ok', + action: { + type: 'discovery:UPDATE_LIST', + payload: { + robots: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: true}], + }, + }, + initialState: {restartsByName: {name: 'down'}}, + expectedState: { + robotsByName: { + name: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: true}], + }, + restartsByName: {name: null}, }, }, ] diff --git a/app/src/discovery/__tests__/selectors.test.js b/app/src/discovery/__tests__/selectors.test.js index 4044dd8813f..dc40cfc751b 100644 --- a/app/src/discovery/__tests__/selectors.test.js +++ b/app/src/discovery/__tests__/selectors.test.js @@ -6,7 +6,8 @@ const makeFullyUp = ( ip, status = null, connected = null, - displayName = null + displayName = null, + restartStatus = null ) => ({ name, ip, @@ -19,6 +20,7 @@ const makeFullyUp = ( status, connected, displayName, + restartStatus, }) const makeConnectable = ( @@ -26,7 +28,8 @@ const makeConnectable = ( ip, status = null, connected = null, - displayName = null + displayName = null, + restartStatus = null ) => ({ name, ip, @@ -37,9 +40,16 @@ const makeConnectable = ( status, connected, displayName, + restartStatus, }) -const makeAdvertising = (name, ip, status = null, displayName = null) => ({ +const makeAdvertising = ( + name, + ip, + status = null, + displayName = null, + restartStatus = null +) => ({ name, ip, local: false, @@ -48,6 +58,7 @@ const makeAdvertising = (name, ip, status = null, displayName = null) => ({ advertising: true, status, displayName, + restartStatus, }) const makeServerUp = ( @@ -55,7 +66,8 @@ const makeServerUp = ( ip, advertising, status = null, - displayName = null + displayName = null, + restartStatus = null ) => ({ name, ip, @@ -66,9 +78,16 @@ const makeServerUp = ( serverHealth: {}, status, displayName, + restartStatus, }) -const makeUnreachable = (name, ip, status = null, displayName = null) => ({ +const makeUnreachable = ( + name, + ip, + status = null, + displayName = null, + restartStatus = null +) => ({ name, ip, local: false, @@ -77,6 +96,7 @@ const makeUnreachable = (name, ip, status = null, displayName = null) => ({ advertising: false, status, displayName, + restartStatus, }) describe('discovery selectors', () => { @@ -130,6 +150,20 @@ describe('discovery selectors', () => { makeConnectable('foo', '10.0.0.1', 'connectable', true, 'foo'), ], }, + { + name: 'getConnectableRobots adds restartStatus if it exists', + state: { + discovery: { + robotsByName: {foo: [makeFullyUp('foo', '10.0.0.2')]}, + restartsByName: {foo: 'pending'}, + }, + robot: {connection: {connectedTo: 'foo'}}, + }, + selector: discovery.getConnectableRobots, + expected: [ + makeFullyUp('foo', '10.0.0.2', 'connectable', true, 'foo', 'pending'), + ], + }, { name: 'getReachableRobots grabs robots with serverUp or advertising', selector: discovery.getReachableRobots, @@ -186,6 +220,19 @@ describe('discovery selectors', () => { }, expected: [], }, + { + name: 'getReachableRobots adds restartStatus if it exists', + state: { + discovery: { + robotsByName: {foo: [makeServerUp('foo', '10.0.0.1', false)]}, + restartsByName: {foo: 'down'}, + }, + }, + selector: discovery.getReachableRobots, + expected: [ + makeServerUp('foo', '10.0.0.1', false, 'reachable', 'foo', 'down'), + ], + }, { name: 'getUnreachableRobots grabs robots with no ip', selector: discovery.getUnreachableRobots, @@ -195,7 +242,13 @@ describe('discovery selectors', () => { }, }, expected: [ - {name: 'foo', ip: null, status: 'unreachable', displayName: 'foo'}, + { + name: 'foo', + ip: null, + status: 'unreachable', + displayName: 'foo', + restartStatus: null, + }, ], }, { @@ -237,6 +290,19 @@ describe('discovery selectors', () => { }, expected: [], }, + { + name: 'getUnreachableRobots adds restartStatus if it exists', + state: { + discovery: { + robotsByName: {foo: [makeUnreachable('foo', '10.0.0.1')]}, + restartsByName: {foo: 'down'}, + }, + }, + selector: discovery.getUnreachableRobots, + expected: [ + makeUnreachable('foo', '10.0.0.1', 'unreachable', 'foo', 'down'), + ], + }, { name: 'display name removes opentrons- from connectable robot names', selector: discovery.getConnectableRobots, @@ -281,17 +347,12 @@ describe('discovery selectors', () => { selector: discovery.getUnreachableRobots, state: { discovery: { - robotsByName: {'opentrons-foo': [{name: 'opentrons-foo', ip: null}]}, + robotsByName: { + 'opentrons-foo': [makeUnreachable('opentrons-foo', null)], + }, }, }, - expected: [ - { - name: 'opentrons-foo', - ip: null, - status: 'unreachable', - displayName: 'foo', - }, - ], + expected: [makeUnreachable('opentrons-foo', null, 'unreachable', 'foo')], }, { name: 'getAllRobots returns all robots', diff --git a/app/src/discovery/index.js b/app/src/discovery/index.js index 54e6e655fad..8235e8400ec 100644 --- a/app/src/discovery/index.js +++ b/app/src/discovery/index.js @@ -1,19 +1,25 @@ // @flow // robot discovery state import groupBy from 'lodash/groupBy' +import mapValues from 'lodash/mapValues' +import some from 'lodash/some' import {getShellRobots} from '../shell' import type {Service} from '@opentrons/discovery-client' -import type {Action, ThunkAction} from '../types' +import type {Action, ThunkAction, Middleware} from '../types' +import type {RestartStatus} from './types' export * from './types' export * from './selectors' type RobotsMap = {[name: string]: Array} +type RestartsMap = {[name: string]: ?RestartStatus} + type DiscoveryState = { scanning: boolean, robotsByName: RobotsMap, + restartsByName: RestartsMap, } type StartAction = {| @@ -33,14 +39,18 @@ type UpdateListAction = {| export type DiscoveryAction = StartAction | FinishAction | UpdateListAction -const DISCOVERY_TIMEOUT = 20000 +const DISCOVERY_TIMEOUT_MS = 30000 +const RESTART_DISCOVERY_TIMEOUT_MS = 60000 + +export const RESTART_PENDING: RestartStatus = 'pending' +export const RESTART_DOWN: RestartStatus = 'down' -export function startDiscovery (): ThunkAction { +export function startDiscovery (timeout = DISCOVERY_TIMEOUT_MS): ThunkAction { const start: StartAction = {type: 'discovery:START', meta: {shell: true}} const finish: FinishAction = {type: 'discovery:FINISH', meta: {shell: true}} return dispatch => { - setTimeout(() => dispatch(finish), DISCOVERY_TIMEOUT) + setTimeout(() => dispatch(finish), timeout) return dispatch(start) } } @@ -57,6 +67,7 @@ export function startDiscovery (): ThunkAction { const initialState: DiscoveryState = { scanning: false, robotsByName: normalizeRobots(getShellRobots()), + restartsByName: {}, } export function discoveryReducer ( @@ -70,16 +81,50 @@ export function discoveryReducer ( case 'discovery:FINISH': return {...state, scanning: false} - case 'discovery:UPDATE_LIST': + case 'discovery:UPDATE_LIST': { + const robotsByName = normalizeRobots(action.payload.robots) + const restartsByName = mapValues(state.restartsByName, (status, name) => { + // TODO(mc, 2018-11-07): once POST /restart sets the status to PENDING, + // we need the robot to be health checked by the discovery client at + // some point while it's down. This is _probably_ going to happen + // because we flip into fast polling when a POST /restart 200s, but + // there's a potential gap here that could lead to a latched PENDING + const up = some(robotsByName[name], 'ok') + if (status === RESTART_PENDING && !up) return RESTART_DOWN + if (status === RESTART_DOWN && up) return null + return status + }) + + return {...state, robotsByName, restartsByName} + } + + case 'api:SERVER_SUCCESS': { + const {path, robot} = action.payload + if (path !== 'restart') return state return { ...state, - robotsByName: normalizeRobots(action.payload.robots), + restartsByName: { + ...state.restartsByName, + [robot.name]: RESTART_PENDING, + }, } + } } return state } +export const discoveryMiddleware: Middleware = store => next => action => { + switch (action.type) { + case 'api:SERVER_SUCCESS': + if (action.payload.path === 'restart') { + store.dispatch(startDiscovery(RESTART_DISCOVERY_TIMEOUT_MS)) + } + } + + return next(action) +} + export function normalizeRobots (robots: Array = []): RobotsMap { return groupBy(robots, 'name') } diff --git a/app/src/discovery/selectors.js b/app/src/discovery/selectors.js index da3ed754488..8ca6123ab9e 100644 --- a/app/src/discovery/selectors.js +++ b/app/src/discovery/selectors.js @@ -2,6 +2,7 @@ import concat from 'lodash/concat' import filter from 'lodash/filter' import find from 'lodash/find' +import get from 'lodash/get' import groupBy from 'lodash/groupBy' import head from 'lodash/head' import map from 'lodash/map' @@ -66,20 +67,22 @@ const makeDisplayName = (service: Service): string => // so the head of each group will be the most "desirable" option for that group const getGroupedRobotsMap: GetGroupedRobotsMap = createSelector( state => state.discovery.robotsByName, - robotsMap => + state => state.discovery.restartsByName, + (robotsMap, restartsMap) => mapValues(robotsMap, services => { const servicesWithStatus = services.map(s => { const resolved = maybeGetResolved(s) - const displayName = makeDisplayName(s) + const service = { + ...s, + displayName: makeDisplayName(s), + restartStatus: get(restartsMap, s.name, null), + } + if (resolved) { - if (isConnectable(resolved)) { - return {...s, status: CONNECTABLE, displayName} - } - if (isReachable(resolved)) { - return {...s, status: REACHABLE, displayName} - } + if (isConnectable(resolved)) return {...service, status: CONNECTABLE} + if (isReachable(resolved)) return {...service, status: REACHABLE} } - return {...s, status: UNREACHABLE, displayName} + return {...service, status: UNREACHABLE} }) return groupBy(servicesWithStatus, 'status') diff --git a/app/src/discovery/types.js b/app/src/discovery/types.js index 55caa3aa54b..84a64e3b9a5 100644 --- a/app/src/discovery/types.js +++ b/app/src/discovery/types.js @@ -6,14 +6,17 @@ export type ConnectableStatus = 'connectable' export type ReachableStatus = 'reachable' export type UnreachableStatus = 'unreachable' +export type RestartStatus = 'pending' | 'down' + // service with a known IP address export type ResolvedRobot = { ...$Exact, - displayName: string, ip: $NonMaybeType<$PropertyType>, local: $NonMaybeType<$PropertyType>, ok: $NonMaybeType<$PropertyType>, serverOk: $NonMaybeType<$PropertyType>, + displayName: string, + restartStatus: ?RestartStatus, } // fully connectable robot diff --git a/app/src/http-api-client/__tests__/server.test.js b/app/src/http-api-client/__tests__/server.test.js index 562bab7541a..d2199a3e07b 100644 --- a/app/src/http-api-client/__tests__/server.test.js +++ b/app/src/http-api-client/__tests__/server.test.js @@ -167,6 +167,7 @@ describe('server API client', () => { { type: 'api:SERVER_SUCCESS', payload: {robot, response, path: 'restart'}, + meta: {robot: true}, }, ] @@ -217,6 +218,7 @@ describe('server API client', () => { { type: 'api:SERVER_SUCCESS', payload: {robot, response, path: 'update/ignore'}, + meta: {robot: true}, }, ] @@ -269,6 +271,7 @@ describe('server API client', () => { { type: 'api:SERVER_SUCCESS', payload: {robot, response, path: 'update/ignore'}, + meta: {robot: true}, }, ] diff --git a/app/src/http-api-client/server.js b/app/src/http-api-client/server.js index 1e5e004a2da..4a8e5286275 100644 --- a/app/src/http-api-client/server.js +++ b/app/src/http-api-client/server.js @@ -54,6 +54,7 @@ export type ServerSuccessAction = {| path: RequestPath, response: ServerResponse, |}, + meta: {|robot: true|}, |} export type ServerFailureAction = {| @@ -349,6 +350,7 @@ function serverSuccess ( return { type: 'api:SERVER_SUCCESS', payload: {robot, path, response}, + meta: {robot: true}, } } diff --git a/app/src/index.js b/app/src/index.js index eea400270dd..f95bbf217b1 100644 --- a/app/src/index.js +++ b/app/src/index.js @@ -14,7 +14,7 @@ import {checkShellUpdate, shellMiddleware} from './shell' import {apiClientMiddleware as robotApiMiddleware} from './robot' import {initializeAnalytics, analyticsMiddleware} from './analytics' import {initializeSupport, supportMiddleware} from './support' -import {startDiscovery} from './discovery' +import {startDiscovery, discoveryMiddleware} from './discovery' import reducer from './reducer' @@ -31,6 +31,7 @@ const middleware = applyMiddleware( shellMiddleware, analyticsMiddleware, supportMiddleware, + discoveryMiddleware, routerMiddleware(history) ) diff --git a/app/src/robot/api-client/client.js b/app/src/robot/api-client/client.js index 5dce17e104c..a60579ff1be 100755 --- a/app/src/robot/api-client/client.js +++ b/app/src/robot/api-client/client.js @@ -68,6 +68,15 @@ export default function client (dispatch) { return cancel(state, action) case 'robot:REFRESH_SESSION': return refreshSession(state, action) + + // disconnect RPC prior to robot restart + // TODO(mc, 2018-11-06): switch to api:RESPONSE when server.js switches + case 'api:SERVER_SUCCESS': { + const connectedName = selectors.getConnectedRobotName(state) + const {path, robot} = action.payload + if (path === 'restart' && connectedName === robot.name) disconnect() + break + } } }