diff --git a/app-shell/src/__mocks__/log.js b/app-shell/src/__mocks__/log.js index e11b4d8b1f3..dd99c691435 100644 --- a/app-shell/src/__mocks__/log.js +++ b/app-shell/src/__mocks__/log.js @@ -1,4 +1,4 @@ // mock logger // NOTE: importing mock to avoid copy-paste // eslint-disable-next-line jest/no-mocks-import -module.exports = require('../../../app/src/__mocks__/logger') +export * from '../../../app/src/__mocks__/logger' diff --git a/app-shell/src/__tests__/discovery.test.js b/app-shell/src/__tests__/discovery.test.js index 4308cb3169f..60142c1339a 100644 --- a/app-shell/src/__tests__/discovery.test.js +++ b/app-shell/src/__tests__/discovery.test.js @@ -1,43 +1,44 @@ // tests for the app-shell's discovery module -import EventEmitter from 'events' import { app } from 'electron' import Store from 'electron-store' -import noop from 'lodash/noop' +import { noop, last } from 'lodash' import { createDiscoveryClient } from '@opentrons/discovery-client' import { startDiscovery, finishDiscovery } from '@opentrons/app/src/discovery' import { registerDiscovery } from '../discovery' -import { getConfig, getOverrides, handleConfigChange } from '../config' +import { getFullConfig, getOverrides, handleConfigChange } from '../config' import { createNetworkInterfaceMonitor } from '../system-info' jest.mock('electron') jest.mock('electron-store') jest.mock('@opentrons/discovery-client') -jest.mock('../log') jest.mock('../config') jest.mock('../system-info') describe('app-shell/discovery', () => { - let dispatch - let mockClient + const dispatch = jest.fn() + const mockClient = { + start: jest.fn(), + stop: jest.fn(), + getRobots: jest.fn(), + removeRobot: jest.fn(), + } + + const emitListChange = () => { + const lastCall = last(createDiscoveryClient.mock.calls) + const { onListChange } = lastCall[0] + onListChange() + } beforeEach(() => { - mockClient = Object.assign(new EventEmitter(), { - services: [], - candidates: [], - start: jest.fn().mockReturnThis(), - stop: jest.fn().mockReturnThis(), - add: jest.fn().mockReturnThis(), - remove: jest.fn().mockReturnThis(), - setPollInterval: jest.fn().mockReturnThis(), + getFullConfig.mockReturnValue({ + discovery: { disableCache: false, candidates: [] }, }) - getConfig.mockReturnValue({ enabled: true, candidates: [] }) getOverrides.mockReturnValue({}) createNetworkInterfaceMonitor.mockReturnValue({ stop: noop }) - - dispatch = jest.fn() createDiscoveryClient.mockReturnValue(mockClient) + Store.__mockReset() Store.__store.get.mockImplementation(key => { if (key === 'services') return [] @@ -54,17 +55,21 @@ describe('app-shell/discovery', () => { expect(createDiscoveryClient).toHaveBeenCalledWith( expect.objectContaining({ - pollInterval: expect.any(Number), - // support for legacy IPv6 wired robots - candidates: ['[fd00:0:cafe:fefe::1]'], - services: [], + onListChange: expect.any(Function), }) ) }) it('calls client.start on discovery registration', () => { registerDiscovery(dispatch) + expect(mockClient.start).toHaveBeenCalledTimes(1) + expect(mockClient.start).toHaveBeenCalledWith({ + healthPollInterval: 15000, + initialRobots: [], + // support for legacy (pre-v3.3.0) IPv6 wired robots + manualAddresses: [{ ip: 'fd00:0:cafe:fefe::1', port: 31950 }], + }) }) it('calls client.stop when electron app emits "will-quit"', () => { @@ -87,44 +92,31 @@ describe('app-shell/discovery', () => { const handleAction = registerDiscovery(dispatch) handleAction({ type: 'discovery:START' }) - expect(mockClient.setPollInterval).toHaveBeenLastCalledWith( - expect.any(Number) - ) - handleAction({ type: 'discovery:FINISH' }) - expect(mockClient.setPollInterval).toHaveBeenLastCalledWith( - expect.any(Number) - ) + expect(mockClient.start).toHaveBeenLastCalledWith({ + healthPollInterval: 3000, + }) - expect(mockClient.setPollInterval).toHaveBeenCalledTimes(2) - const fastPoll = mockClient.setPollInterval.mock.calls[0][0] - const slowPoll = mockClient.setPollInterval.mock.calls[1][0] - expect(fastPoll).toBeLessThan(slowPoll) + handleAction({ type: 'discovery:FINISH' }) + expect(mockClient.start).toHaveBeenLastCalledWith({ + healthPollInterval: 15000, + }) }) it('sets poll speed on "shell:UI_INTIALIZED"', () => { const handleAction = registerDiscovery(dispatch) - handleAction({ type: 'discovery:START' }) - expect(mockClient.setPollInterval).toHaveBeenLastCalledWith( - expect.any(Number) - ) handleAction({ type: 'shell:UI_INITIALIZED' }) - expect(mockClient.setPollInterval).toHaveBeenLastCalledWith( - expect.any(Number) - ) - - expect(mockClient.setPollInterval).toHaveBeenCalledTimes(2) - const startPollSpeed = mockClient.setPollInterval.mock.calls[0][0] - const uiInitializedPollSpeed = mockClient.setPollInterval.mock.calls[1][0] - expect(startPollSpeed).toEqual(uiInitializedPollSpeed) + expect(mockClient.start).toHaveBeenLastCalledWith({ + healthPollInterval: 3000, + }) }) it('always sends "discovery:UPDATE_LIST" on "discovery:START"', () => { const expected = [ - { name: 'opentrons-dev', ip: '192.168.1.42', port: 31950, ok: true }, + { name: 'opentrons', health: null, serverHealth: null, addresses: [] }, ] - mockClient.services = expected + mockClient.getRobots.mockReturnValue(expected) registerDiscovery(dispatch)({ type: 'discovery:START' }) expect(dispatch).toHaveBeenCalledWith({ type: 'discovery:UPDATE_LIST', @@ -132,202 +124,283 @@ describe('app-shell/discovery', () => { }) }) - describe('"service" event handling', () => { - beforeEach(() => registerDiscovery(dispatch)) - - const SPECS = [ - { - name: 'dispatches discovery:UPDATE_LIST on "service" event', - services: [ - { name: 'opentrons-dev', ip: '192.168.1.42', port: 31950, ok: true }, - ], - }, - ] - - SPECS.forEach(spec => - it(spec.name, () => { - mockClient.services = spec.services - - mockClient.emit('service') - expect(dispatch).toHaveBeenCalledWith({ - type: 'discovery:UPDATE_LIST', - payload: { robots: spec.services }, - }) - }) - ) - }) - - it('stores services to file on service events', () => { - registerDiscovery(dispatch) - expect(Store).toHaveBeenCalledWith({ - name: 'discovery', - defaults: { services: [] }, + it('calls client.removeRobot on discovery:REMOVE', () => { + const handleAction = registerDiscovery(dispatch) + handleAction({ + type: 'discovery:REMOVE', + payload: { robotName: 'robot-name' }, }) - mockClient.services = [{ name: 'foo' }, { name: 'bar' }] - mockClient.emit('service') - expect(Store.__store.set).toHaveBeenLastCalledWith('services', [ - { name: 'foo' }, - { name: 'bar' }, - ]) + expect(mockClient.removeRobot).toHaveBeenCalledWith('robot-name') }) - it('stores services to file on serviceRemoved events', () => { - registerDiscovery(dispatch) + describe('robot list caching', () => { + it('stores services to when onListUpdate is called', () => { + registerDiscovery(dispatch) + expect(Store).toHaveBeenCalledWith({ + name: 'discovery', + defaults: { robots: [] }, + }) - mockClient.services = [{ name: 'foo' }] - mockClient.emit('serviceRemoved') - expect(Store.__store.set).toHaveBeenLastCalledWith('services', [ - { name: 'foo' }, - ]) - }) + mockClient.getRobots.mockReturnValue([{ name: 'foo' }, { name: 'bar' }]) + emitListChange() - it('loads services from file on client initialization', () => { - Store.__store.get.mockImplementation(key => { - if (key === 'services') return [{ name: 'foo' }] - return null + expect(Store.__store.set).toHaveBeenLastCalledWith('robots', [ + { name: 'foo' }, + { name: 'bar' }, + ]) }) - registerDiscovery(dispatch) - expect(createDiscoveryClient).toHaveBeenCalledWith( - expect.objectContaining({ - services: [{ name: 'foo' }], - }) - ) - }) - - it('loads candidates from config on client initialization', () => { - getConfig.mockReturnValue({ enabled: true, candidates: ['1.2.3.4'] }) - registerDiscovery(dispatch) + it('loads robots from cache on client initialization', () => { + const mockRobot = { name: 'foo' } - expect(createDiscoveryClient).toHaveBeenCalledWith( - expect.objectContaining({ - candidates: expect.arrayContaining(['1.2.3.4']), + Store.__store.get.mockImplementation(key => { + if (key === 'robots') return [mockRobot] + return null }) - ) - }) - // ensures config override works with only one candidate specified - it('candidates in config can be single value', () => { - getConfig.mockReturnValue({ enabled: true, candidates: '1.2.3.4' }) - registerDiscovery(dispatch) + registerDiscovery(dispatch) + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + initialRobots: [mockRobot], + }) + ) + }) - expect(createDiscoveryClient).toHaveBeenCalledWith( - expect.objectContaining({ - candidates: expect.arrayContaining(['1.2.3.4']), + it('loads legacy services from cache on client initialization', () => { + const services = [ + { + name: 'opentrons', + ip: '192.168.1.1', + port: 31950, + local: false, + ok: false, + serverOk: true, + advertising: true, + health: { + name: 'opentrons', + api_version: '3.19.0', + fw_version: 'v1.0.8-1f0a3d7', + system_version: 'v1.3.7-2-g9e23b93f41', + }, + serverHealth: { + name: 'opentrons', + apiServerVersion: '3.19.0', + updateServerVersion: '3.19.0', + smoothieVersion: 'unimplemented', + systemVersion: 'v1.3.7-2-g9e23b93f41', + capabilities: {}, + }, + }, + { + name: 'opentrons', + ip: '169.254.92.130', + port: 31950, + local: false, + ok: false, + serverOk: true, + advertising: true, + health: { + name: 'opentrons', + api_version: '3.19.0', + fw_version: 'v1.0.8-1f0a3d7', + system_version: 'v1.3.7-2-g9e23b93f41', + }, + serverHealth: { + name: 'opentrons', + apiServerVersion: '3.19.0', + updateServerVersion: '3.19.0', + smoothieVersion: 'unimplemented', + systemVersion: 'v1.3.7-2-g9e23b93f41', + capabilities: {}, + }, + }, + { + name: 'opentrons', + ip: '169.254.239.127', + port: 31950, + local: true, + ok: false, + serverOk: false, + advertising: false, + health: null, + serverHealth: null, + }, + { + name: 'unknown', + ip: null, + port: 31950, + local: true, + ok: false, + serverOk: false, + advertising: false, + health: null, + serverHealth: null, + }, + ] + + Store.__store.get.mockImplementation(key => { + if (key === 'services') return services + return null }) - ) - }) - it('services from overridden candidates are not persisted', () => { - getConfig.mockReturnValue({ enabled: true, candidates: 'localhost' }) - getOverrides.mockImplementation(key => { - if (key === 'discovery.candidates') return ['1.2.3.4', '5.6.7.8'] - return null + registerDiscovery(dispatch) + expect(Store.__store.delete).toHaveBeenCalledWith('services') + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + initialRobots: [ + { + name: 'opentrons', + health: null, + serverHealth: null, + addresses: [ + { + ip: '192.168.1.1', + port: 31950, + seen: false, + healthStatus: null, + serverHealthStatus: null, + healthError: null, + serverHealthError: null, + }, + { + ip: '169.254.92.130', + port: 31950, + seen: false, + healthStatus: null, + serverHealthStatus: null, + healthError: null, + serverHealthError: null, + }, + { + ip: '169.254.239.127', + port: 31950, + seen: false, + healthStatus: null, + serverHealthStatus: null, + healthError: null, + serverHealthError: null, + }, + ], + }, + { + name: 'unknown', + health: null, + serverHealth: null, + addresses: [], + }, + ], + }) + ) }) - registerDiscovery(dispatch) + it('can delete cached robots', () => { + const handleAction = registerDiscovery(dispatch) + mockClient.start.mockReset() - mockClient.services = [{ name: 'foo', ip: '5.6.7.8' }, { name: 'bar' }] - mockClient.emit('service') - expect(Store.__store.set).toHaveBeenCalledWith('services', [ - { name: 'bar' }, - ]) - }) + handleAction({ + type: 'discovery:CLEAR_CACHE', + meta: { shell: true }, + }) - it('service from overridden single candidate is not persisted', () => { - getConfig.mockReturnValue({ enabled: true, candidates: 'localhost' }) - getOverrides.mockImplementation(key => { - if (key === 'discovery.candidates') return '1.2.3.4' - return null + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + initialRobots: [], + }) + ) }) - registerDiscovery(dispatch) + it('does not update services from store when caching disabled', () => { + // cache has been disabled + getFullConfig.mockReturnValue({ + discovery: { + candidates: [], + disableCache: true, + }, + }) - mockClient.services = [{ name: 'foo', ip: '1.2.3.4' }, { name: 'bar' }] - mockClient.emit('service') - expect(Store.__store.set).toHaveBeenCalledWith('services', [ - { name: 'bar' }, - ]) - }) + // discovery.json contains 1 entry + Store.__store.get.mockImplementation(key => { + if (key === 'robots') return [{ name: 'foo' }] + return null + }) - it('calls client.remove on discovery:REMOVE', () => { - const handleAction = registerDiscovery(dispatch) - handleAction({ - type: 'discovery:REMOVE', - payload: { robotName: 'robot-name' }, + registerDiscovery(dispatch) + + // should not contain above entry + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + initialRobots: [], + }) + ) }) - expect(mockClient.remove).toHaveBeenCalledWith('robot-name') - }) + it('should clear cache and suspend caching when caching becomes disabled', () => { + // Cache enabled initially + getFullConfig.mockReturnValue({ + discovery: { + candidates: [], + disableCache: false, + }, + }) + // discovery.json contains 1 entry + Store.__store.get.mockImplementation(key => { + if (key === 'robots') return [{ name: 'foo' }] + return null + }) - it('deletes cached robots', () => { - const handleAction = registerDiscovery(dispatch) - mockClient.start.mockClear() - mockClient.services = [{ name: 'foo', ip: '5.6.7.8' }, { name: 'bar' }] - handleAction({ - type: 'discovery:CLEAR_CACHE', - meta: { shell: true }, - }) + registerDiscovery(dispatch) - expect(mockClient.stop).toHaveBeenCalled() - expect(mockClient.services).toEqual([]) - expect(mockClient.start).toHaveBeenCalled() - expect(dispatch).toHaveBeenCalledWith({ - type: 'discovery:UPDATE_LIST', - payload: { robots: [] }, - }) - }) + // the 'discovery.disableCache' change handler + const changeHandler = handleConfigChange.mock.calls[1][1] + const disableCache = true + changeHandler(disableCache) - it('does not update services from store when caching disabled', () => { - // cache has been disabled - getConfig.mockReturnValue({ - candidates: [], - disableCache: true, - }) - // discovery.json contains 1 entry - Store.__store.get.mockImplementation(key => { - if (key === 'services') return [{ name: 'foo' }] - return null - }) + expect(Store.__store.set).toHaveBeenCalledWith('robots', []) - registerDiscovery(dispatch) + // new services discovered + Store.__store.set.mockClear() + mockClient.getRobots.mockReturnValue([{ name: 'foo' }, { name: 'bar' }]) + emitListChange() - // should not contain above entry - expect(createDiscoveryClient).toHaveBeenCalledWith( - expect.objectContaining({ - services: [], - }) - ) + // but discovery.json should not update + expect(Store.__store.set).toHaveBeenCalledTimes(0) + }) }) - it('clears cache & suspends caching when caching changes to disabled', () => { - // Cache enabled initially - getConfig.mockReturnValue({ - candidates: [], - disableCache: false, - }) - // discovery.json contains 1 entry - Store.__store.get.mockImplementation(key => { - if (key === 'services') return [{ name: 'foo' }] - return null - }) + describe('manual addresses', () => { + it('loads candidates from config on client initialization', () => { + getFullConfig.mockReturnValue({ + discovery: { cacheDisabled: false, candidates: ['1.2.3.4'] }, + }) - registerDiscovery(dispatch) + registerDiscovery(dispatch) - // the 'discovery.disableCache' change handler - const changeHandler = handleConfigChange.mock.calls[1][1] - const disableCache = true - changeHandler(disableCache) + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + manualAddresses: expect.arrayContaining([ + { ip: '1.2.3.4', port: 31950 }, + ]), + }) + ) + }) - expect(Store.__store.set).toHaveBeenCalledWith('services', []) + // ensures config override works with only one candidate specified + it('candidates in config can be single string value', () => { + getFullConfig.mockReturnValue({ + discovery: { cacheDisabled: false, candidates: '1.2.3.4' }, + }) - // new services discovered - mockClient.services = [{ name: 'foo' }, { name: 'bar' }] - mockClient.emit('service') + registerDiscovery(dispatch) - // but discovery.json should not update - expect(Store.__store.set).toHaveBeenLastCalledWith('services', []) + expect(mockClient.start).toHaveBeenCalledWith( + expect.objectContaining({ + manualAddresses: expect.arrayContaining([ + { ip: '1.2.3.4', port: 31950 }, + ]), + }) + ) + }) }) // TODO(mc, 2020-06-16): move this functionality into discovery-client diff --git a/app-shell/src/discovery.js b/app-shell/src/discovery.js index acbe0ac1769..a6e87439de8 100644 --- a/app-shell/src/discovery.js +++ b/app-shell/src/discovery.js @@ -2,12 +2,12 @@ // app shell discovery module import { app } from 'electron' import Store from 'electron-store' +import groupBy from 'lodash/groupBy' import throttle from 'lodash/throttle' import { createDiscoveryClient, - SERVICE_EVENT, - SERVICE_REMOVED_EVENT, + DEFAULT_PORT, } from '@opentrons/discovery-client' import { UI_INITIALIZED } from '@opentrons/app/src/shell/actions' @@ -18,12 +18,14 @@ import { CLEAR_CACHE, } from '@opentrons/app/src/discovery/actions' -import { getConfig, getOverrides, handleConfigChange } from './config' +import { getFullConfig, handleConfigChange } from './config' import { createLogger } from './log' import { createNetworkInterfaceMonitor } from './system-info' -import type { Service } from '@opentrons/discovery-client' - +import type { + DiscoveryClientRobot, + LegacyService, +} from '@opentrons/discovery-client' import type { Action, Dispatch } from './types' const log = createLogger('discovery') @@ -39,35 +41,87 @@ let config let store let client +const makeManualAddresses = (addrs: string | Array) => { + return ['fd00:0:cafe:fefe::1'] + .concat(addrs) + .map(ip => ({ ip, port: DEFAULT_PORT })) +} + +const migrateLegacyServices = ( + legacyServices: Array +): Array => { + const servicesByName = groupBy(legacyServices, 'name') + + return Object.keys(servicesByName).map((name: string) => { + const services = servicesByName[name] + const addresses = services.flatMap((service: LegacyService) => { + const { ip, port } = service + return ip != null + ? [ + { + ip, + port, + seen: false, + healthStatus: null, + serverHealthStatus: null, + healthError: null, + serverHealthError: null, + }, + ] + : [] + }) + + return { name, health: null, serverHealth: null, addresses } + }) +} + export function registerDiscovery(dispatch: Dispatch): Action => mixed { - const onServiceUpdate = throttle(handleServices, UPDATE_THROTTLE_MS) + const handleRobotListChange = throttle(handleRobots, UPDATE_THROTTLE_MS) + + config = getFullConfig().discovery + store = new Store({ name: 'discovery', defaults: { robots: [] } }) - config = getConfig('discovery') - store = new Store({ name: 'discovery', defaults: { services: [] } }) let disableCache = config.disableCache + let initialRobots: Array = [] + + if (!disableCache) { + const legacyCachedServices: Array | null = store.get( + 'services', + null + ) + + if (legacyCachedServices) { + initialRobots = migrateLegacyServices(legacyCachedServices) + store.delete('services') + } else { + initialRobots = store.get('robots', []) + } + } client = createDiscoveryClient({ - pollInterval: SLOW_POLL_INTERVAL_MS, + onListChange: handleRobotListChange, logger: log, - candidates: ['[fd00:0:cafe:fefe::1]'].concat(config.candidates), - services: disableCache ? [] : store.get('services'), }) - client - .on(SERVICE_EVENT, onServiceUpdate) - .on(SERVICE_REMOVED_EVENT, onServiceUpdate) - .on('error', error => log.error('discovery error', { error })) - .start() + client.start({ + initialRobots, + healthPollInterval: SLOW_POLL_INTERVAL_MS, + manualAddresses: makeManualAddresses(config.candidates), + }) - handleConfigChange('discovery.candidates', value => - client.setCandidates(['[fd00:0:cafe:fefe::1]'].concat(value)) + handleConfigChange( + 'discovery.candidates', + (value: string | Array) => { + client.start({ manualAddresses: makeManualAddresses(value) }) + } ) - handleConfigChange('discovery.disableCache', value => { + handleConfigChange('discovery.disableCache', (value: boolean) => { if (value === true) { + disableCache = value + store.set('robots', []) clearCache() } - disableCache = value }) let ifaceMonitor @@ -75,7 +129,7 @@ export function registerDiscovery(dispatch: Dispatch): Action => mixed { ifaceMonitor && ifaceMonitor.stop() ifaceMonitor = createNetworkInterfaceMonitor({ pollInterval, - onInterfaceChange: () => client.start(), + onInterfaceChange: () => client.start({}), }) } @@ -92,44 +146,34 @@ export function registerDiscovery(dispatch: Dispatch): Action => mixed { switch (action.type) { case UI_INITIALIZED: case DISCOVERY_START: - handleServices() + handleRobots() startIfaceMonitor(IFACE_MONITOR_FAST_INTERVAL_MS) - return client.setPollInterval(FAST_POLL_INTERVAL_MS) + return client.start({ healthPollInterval: FAST_POLL_INTERVAL_MS }) case DISCOVERY_FINISH: startIfaceMonitor(IFACE_MONITOR_SLOW_INTERVAL_MS) - return client.setPollInterval(SLOW_POLL_INTERVAL_MS) + return client.start({ healthPollInterval: SLOW_POLL_INTERVAL_MS }) case DISCOVERY_REMOVE: - return client.remove(action.payload.robotName) + return client.removeRobot(action.payload.robotName) case CLEAR_CACHE: return clearCache() } } - function handleServices() { - if (!disableCache) { - store.set('services', filterServicesToPersist(client.services)) - } + function handleRobots() { + const robots = client.getRobots() + + if (!disableCache) store.set('robots', robots) + dispatch({ type: 'discovery:UPDATE_LIST', - payload: { robots: client.services }, + payload: { robots }, }) } function clearCache() { - client.stop() - client.services = [] - handleServices() - client.start() + client.start({ initialRobots: [] }) } } - -function filterServicesToPersist(services: Array) { - const candidateOverrides = getOverrides('discovery.candidates') - if (!candidateOverrides) return client.services - - const blocklist = [].concat(candidateOverrides) - return client.services.filter(s => blocklist.every(ip => ip !== s.ip)) -} diff --git a/app/package.json b/app/package.json index 5b9f95b469e..d80c9364e3e 100644 --- a/app/package.json +++ b/app/package.json @@ -29,6 +29,7 @@ "file-saver": "2.0.1", "formik": "2.1.4", "history": "4.7.2", + "is-ip": "3.1.0", "lodash": "4.17.15", "mixpanel-browser": "2.22.1", "netmask": "1.0.6", diff --git a/app/src/buildroot/epic.js b/app/src/buildroot/epic.js index f01a74a05a9..d163d31377e 100644 --- a/app/src/buildroot/epic.js +++ b/app/src/buildroot/epic.js @@ -14,7 +14,7 @@ import { } from 'rxjs/operators' // imported directly to avoid circular dependencies between discovery and shell -import { getAllRobots, getRobotApiVersion } from '../discovery/selectors' +import { getAllRobots, getRobotApiVersion, CONNECTABLE } from '../discovery' import { startDiscovery, finishDiscovery, @@ -324,7 +324,11 @@ export const watchForOfflineAfterRestartEpic: Epic = (_, state$) => { const session = getBuildrootSession(state) const robot = getBuildrootRobot(state) - return !robot?.ok && !session?.error && session?.step === RESTART + return ( + robot?.status !== CONNECTABLE && + !session?.error && + session?.step === RESTART + ) }), mapTo(setBuildrootSessionStep(RESTARTING)) ) @@ -337,7 +341,9 @@ export const watchForOnlineAfterRestartEpic: Epic = (_, state$) => { const robot = getBuildrootRobot(state) return ( - Boolean(robot?.ok) && !session?.error && session?.step === RESTARTING + robot?.status === CONNECTABLE && + !session?.error && + session?.step === RESTARTING ) }), switchMap(stateWithRobot => { diff --git a/app/src/components/ConnectPanel/RobotItem.js b/app/src/components/ConnectPanel/RobotItem.js index 4e56f9aeb82..c01b5c74355 100644 --- a/app/src/components/ConnectPanel/RobotItem.js +++ b/app/src/components/ConnectPanel/RobotItem.js @@ -57,7 +57,7 @@ export function RobotItemComponent(props: RobotItemProps): React.Node { isConnectable, isUpgradable, isSelected, - isLocal, + isLocal: Boolean(isLocal), isConnected, onToggleConnect: handleToggleConnect, }} diff --git a/app/src/components/FileInfo/__tests__/FileInfo.test.js b/app/src/components/FileInfo/__tests__/FileInfo.test.js index f3a150b10ec..7efa9f2d619 100644 --- a/app/src/components/FileInfo/__tests__/FileInfo.test.js +++ b/app/src/components/FileInfo/__tests__/FileInfo.test.js @@ -30,7 +30,9 @@ describe('File info Component', () => { expect(wrapper.find(ProtocolPipettesCard).exists()).toEqual(true) expect(wrapper.find(ProtocolModulesCard).exists()).toEqual(true) expect(wrapper.find(Continue).exists()).toEqual(true) - expect(labwareCard.prop('robotName')).toEqual('robot-name') + expect(labwareCard.prop('robotName')).toEqual( + Fixtures.mockConnectedRobot.name + ) }) it('An error renders when an upload error is given', () => { diff --git a/app/src/components/LostConnectionAlert/index.js b/app/src/components/LostConnectionAlert/index.js index 5294e9567be..4ca83bf73fb 100644 --- a/app/src/components/LostConnectionAlert/index.js +++ b/app/src/components/LostConnectionAlert/index.js @@ -3,11 +3,8 @@ import * as React from 'react' import { useDispatch, useSelector } from 'react-redux' import { useHistory } from 'react-router-dom' -import { - actions as robotActions, - selectors as robotSelectors, -} from '../../robot' -import { getAllRobots } from '../../discovery' +import { actions as robotActions, selectors as robotSel } from '../../robot' +import { getConnectedRobot, CONNECTABLE } from '../../discovery' import { AlertModal } from '@opentrons/components' import { Portal } from '../portal' import { ModalCopy } from './ModalCopy' @@ -20,13 +17,14 @@ export function LostConnectionAlert(): React.Node { // TODO(mc, 2020-05-07): move LostConnectionAlert into `state.alerts` const showAlert = useSelector((state: State) => { - // search _all_ robots, not just connectable ones, in case we were connected - // and then robot became not connectable - const connectedName = robotSelectors.getConnectedRobotName(state) - const robot = getAllRobots(state).find(r => r.name === connectedName) + const connectedName = robotSel.getConnectedRobotName(state) + const connectedRobot = getConnectedRobot(state) + const robotDown = connectedRobot?.status !== CONNECTABLE const unexpectedDisconnect = state.robot.connection.unexpectedDisconnect - return Boolean((connectedName && !robot?.ok) || unexpectedDisconnect) + // trigger an alert if we're supposed to be connected but the robot is down + // or if the WebSocket closed unexpectedly + return Boolean((connectedName && robotDown) || unexpectedDisconnect) }) const disconnect = () => { diff --git a/app/src/components/RobotSettings/InformationCard.js b/app/src/components/RobotSettings/InformationCard.js index 546f9a47e38..2918a7e972c 100644 --- a/app/src/components/RobotSettings/InformationCard.js +++ b/app/src/components/RobotSettings/InformationCard.js @@ -8,6 +8,7 @@ import { getRobotApiVersion, getRobotFirmwareVersion, getRobotProtocolApiVersion, + HEALTH_STATUS_OK, } from '../../discovery' import { getBuildrootRobot, getBuildrootUpdateAvailable } from '../../buildroot' @@ -59,14 +60,14 @@ export function InformationCard(props: InformationCardProps): React.Node { dispatch, ]) - const { displayName, serverOk } = robot + const { displayName, serverHealthStatus } = robot const buildrootRobot = useSelector(getBuildrootRobot) const version = getRobotApiVersion(robot) const firmwareVersion = getRobotFirmwareVersion(robot) const maxApiVersion = getRobotProtocolApiVersion(robot) const updateFilesUnavailable = updateType === null - const updateServerUnavailable = !serverOk + const updateServerUnavailable = serverHealthStatus !== HEALTH_STATUS_OK const otherRobotUpdating = Boolean(buildrootRobot && buildrootRobot !== robot) const updateDisabled = updateFilesUnavailable || updateServerUnavailable || otherRobotUpdating diff --git a/app/src/components/RobotSettings/ReachableRobotBanner.js b/app/src/components/RobotSettings/ReachableRobotBanner.js index 9adc7248f51..6406e109586 100644 --- a/app/src/components/RobotSettings/ReachableRobotBanner.js +++ b/app/src/components/RobotSettings/ReachableRobotBanner.js @@ -3,32 +3,47 @@ import * as React from 'react' import { AlertItem } from '@opentrons/components' import styles from './styles.css' -import type { ReachableRobot } from '../../discovery/types' +import { + HEALTH_STATUS_OK, + HEALTH_STATUS_NOT_OK, + HEALTH_STATUS_UNREACHABLE, +} from '../../discovery' -type State = { dismissed: boolean } +import type { ReachableRobot, HealthStatus } from '../../discovery/types' + +type State = {| dismissed: boolean |} const UNRESPONSIVE_TITLE = 'Unable to establish connection with robot' +const STATUS_DESCRIPTION = { + [HEALTH_STATUS_OK]: 'responding to requests', + [HEALTH_STATUS_NOT_OK]: 'not responding correctly to requests', + [HEALTH_STATUS_UNREACHABLE]: 'not reachable', +} + const LAST_RESORT = (

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

) -const NO_SERVER_MESSAGE = ( +const NO_SERVER_MESSAGE = (serverStatus: HealthStatus, ip: string) => (

- Your robot is advertising its IP address but is not responding to - requests. + This OT-2 has been seen recently, but it is currently{' '} + {STATUS_DESCRIPTION[serverStatus]} at IP address {ip}.

We recommend power-cycling your robot.

{LAST_RESORT}
) -const SERVER_MESSAGE = ( +const SERVER_MESSAGE = (status: HealthStatus, ip: string) => (
-

Your robot's API server is not responding.

+

+ Your {"OT-2's"} API server is {STATUS_DESCRIPTION[status]} at IP address{' '} + {ip}. +

We recommend the following troubleshooting steps:

  1. @@ -36,10 +51,10 @@ const SERVER_MESSAGE = (
  2. - If power-cycling does not work, please update your robot's + If power-cycling does not work, please update your {"robot's"} software
    - (Note: your robot's update server is still responding and should + (Note: your {"robot's"} update server is still responding and should accept an update.)

  3. @@ -58,9 +73,12 @@ export class ReachableRobotBanner extends React.Component< } render(): React.Node { - const { serverOk } = this.props + const { ip, healthStatus, serverHealthStatus } = this.props const isVisible = !this.state.dismissed - const message = serverOk ? SERVER_MESSAGE : NO_SERVER_MESSAGE + const message = + serverHealthStatus === HEALTH_STATUS_OK + ? SERVER_MESSAGE(healthStatus, ip) + : NO_SERVER_MESSAGE(serverHealthStatus, ip) if (!isVisible) return null diff --git a/app/src/discovery/__fixtures__/index.js b/app/src/discovery/__fixtures__/index.js index 320154f033f..702ec746651 100644 --- a/app/src/discovery/__fixtures__/index.js +++ b/app/src/discovery/__fixtures__/index.js @@ -1,8 +1,13 @@ // @flow -import { CONNECTABLE, REACHABLE } from '../selectors' +import { + CONNECTABLE, + REACHABLE, + HEALTH_STATUS_OK, + HEALTH_STATUS_NOT_OK, +} from '../constants' -import type { Service, ResolvedRobot, Robot, ReachableRobot } from '../types' +import type { BaseRobot, Robot, ReachableRobot } from '../types' export const mockHealthResponse = { name: 'robot-name', @@ -10,7 +15,7 @@ export const mockHealthResponse = { fw_version: '0.0.0-mock', system_version: '0.0.0-mock', logs: ([]: Array), - protocol_api_version: [2, 0], + protocol_api_version: ([2, 0]: [number, number]), } export const mockUpdateServerHealthResponse = { @@ -22,34 +27,43 @@ export const mockUpdateServerHealthResponse = { capabilities: ({}: { ... }), } -export const mockService: $Exact = { +export const mockDiscoveryClientRobot = { name: 'robot-name', - ip: null, - port: 31950, - local: true, - ok: null, - serverOk: null, - advertising: null, - health: null, - serverHealth: null, + health: mockHealthResponse, + serverHealth: mockUpdateServerHealthResponse, + addresses: [ + { + ip: '127.0.0.1', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + healthError: null, + serverHealthError: null, + }, + ], } -export const mockResolvedRobot: ResolvedRobot = { - ...mockService, - ip: '127.0.0.1', - local: true, - ok: true, - serverOk: true, +export const mockBaseRobot: BaseRobot = { + name: 'opentrons-robot-name', displayName: 'robot-name', + connected: false, + seen: false, + local: null, + health: null, + serverHealth: null, } export const mockConnectableRobot: Robot = { - ...mockResolvedRobot, - ok: true, + ...mockBaseRobot, + status: CONNECTABLE, health: mockHealthResponse, serverHealth: mockUpdateServerHealthResponse, - status: CONNECTABLE, - connected: false, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + ip: '127.0.0.1', + port: 31950, + seen: true, } export const mockConnectedRobot: Robot = { @@ -58,7 +72,13 @@ export const mockConnectedRobot: Robot = { } export const mockReachableRobot: ReachableRobot = { - ...mockResolvedRobot, - ok: false, + ...mockBaseRobot, status: REACHABLE, + health: mockHealthResponse, + serverHealth: mockUpdateServerHealthResponse, + healthStatus: HEALTH_STATUS_NOT_OK, + serverHealthStatus: HEALTH_STATUS_OK, + ip: '127.0.0.1', + port: 31950, + seen: true, } diff --git a/app/src/discovery/__tests__/reducer.test.js b/app/src/discovery/__tests__/reducer.test.js index d55b8080c63..09d27739654 100644 --- a/app/src/discovery/__tests__/reducer.test.js +++ b/app/src/discovery/__tests__/reducer.test.js @@ -40,16 +40,16 @@ describe('discoveryReducer', () => { type: 'discovery:UPDATE_LIST', payload: { robots: [ - { name: 'foo', ip: '192.168.1.1', port: 31950 }, - { name: 'bar', ip: '192.168.1.2', port: 31950 }, + { name: 'foo', health: null, serverHealth: null, addresses: [] }, + { name: 'bar', health: null, serverHealth: null, addresses: [] }, ], }, }, initialState: { robotsByName: {} }, expectedState: { robotsByName: { - foo: [{ name: 'foo', ip: '192.168.1.1', port: 31950 }], - bar: [{ name: 'bar', ip: '192.168.1.2', port: 31950 }], + foo: { name: 'foo', health: null, serverHealth: null, addresses: [] }, + bar: { name: 'bar', health: null, serverHealth: null, addresses: [] }, }, }, }, diff --git a/app/src/discovery/__tests__/selectors.test.js b/app/src/discovery/__tests__/selectors.test.js index 3c006396521..49e67c19e4c 100644 --- a/app/src/discovery/__tests__/selectors.test.js +++ b/app/src/discovery/__tests__/selectors.test.js @@ -1,83 +1,216 @@ // discovery selectors tests +import { + mockHealthResponse, + mockServerHealthResponse, + mockHealthErrorStringResponse, + mockHealthFetchErrorResponse, +} from '@opentrons/discovery-client/src/__fixtures__' + +import { + HEALTH_STATUS_OK, + HEALTH_STATUS_NOT_OK, + HEALTH_STATUS_UNREACHABLE, + CONNECTABLE, + REACHABLE, + UNREACHABLE, +} from '../constants' + import * as discovery from '../selectors' -const makeFullyUp = ( - name, - ip, - status = null, - connected = null, - displayName = null -) => ({ - name, - ip, +const MOCK_STATE = { + robot: { connection: { connectedTo: 'bar' } }, + discovery: { + robotsByName: { + foo: { + name: 'foo', + health: mockHealthResponse, + serverHealth: null, + addresses: [ + { + ip: '10.0.0.1', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_NOT_OK, + healthError: null, + serverHealthError: mockHealthErrorStringResponse, + }, + ], + }, + bar: { + name: 'bar', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [ + { + ip: '10.0.0.2', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + healthError: null, + serverHealthError: null, + }, + ], + }, + baz: { + name: 'baz', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [ + { + ip: '10.0.0.3', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_NOT_OK, + serverHealthStatus: HEALTH_STATUS_OK, + healthError: mockHealthErrorStringResponse, + serverHealthError: null, + }, + ], + }, + qux: { + name: 'qux', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [ + { + ip: '10.0.0.4', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_UNREACHABLE, + serverHealthStatus: HEALTH_STATUS_UNREACHABLE, + healthError: mockHealthFetchErrorResponse, + serverHealthError: mockHealthFetchErrorResponse, + }, + ], + }, + fizz: { + name: 'fizz', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [ + { + ip: '10.0.0.5', + port: 31950, + seen: false, + healthStatus: HEALTH_STATUS_UNREACHABLE, + serverHealthStatus: HEALTH_STATUS_UNREACHABLE, + healthError: mockHealthFetchErrorResponse, + serverHealthError: mockHealthFetchErrorResponse, + }, + ], + }, + buzz: { + name: 'buzz', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [], + }, + }, + }, +} + +// foo is connectable because health is defined and healthStatus of primary +// address is "ok" +const EXPECTED_FOO = { + name: 'foo', + displayName: 'foo', + status: CONNECTABLE, + connected: false, local: false, - ok: true, - serverOk: true, - advertising: true, - health: {}, - serverHealth: {}, - status, - connected, - displayName, -}) + seen: true, + health: mockHealthResponse, + serverHealth: null, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_NOT_OK, + ip: '10.0.0.1', + port: 31950, +} -const makeConnectable = ( - name, - ip, - status = null, - connected = null, - displayName = null -) => ({ - name, - ip, +// bar is connectable because health is defined and healthStatus of primary +// address is "ok", and bar is connected because of connectedTo state +const EXPECTED_BAR = { + name: 'bar', + displayName: 'bar', + status: CONNECTABLE, + connected: true, local: false, - ok: true, - serverOk: false, - health: {}, - status, - connected, - displayName, -}) + seen: true, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + ip: '10.0.0.2', + port: 31950, +} -const makeAdvertising = (name, ip, status = null, displayName = null) => ({ - name, - ip, +// baz is reachable because healthStatus is "notOk", which means it responded +// with an error code +const EXPECTED_BAZ = { + name: 'baz', + displayName: 'baz', + status: REACHABLE, + connected: false, local: false, - ok: false, - serverOk: false, - advertising: true, - status, - displayName, -}) + seen: true, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthStatus: HEALTH_STATUS_NOT_OK, + serverHealthStatus: HEALTH_STATUS_OK, + ip: '10.0.0.3', + port: 31950, +} -const makeServerUp = ( - name, - ip, - advertising, - status = null, - displayName = null -) => ({ - name, - ip, - advertising, +// qux is reachable because it was recently seen, even though primary IP is +// not currently responding in any way +const EXPECTED_QUX = { + name: 'qux', + displayName: 'qux', + status: REACHABLE, + connected: false, local: false, - ok: false, - serverOk: true, - serverHealth: {}, - status, - displayName, -}) + seen: true, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthStatus: HEALTH_STATUS_UNREACHABLE, + serverHealthStatus: HEALTH_STATUS_UNREACHABLE, + ip: '10.0.0.4', + port: 31950, +} -const makeUnreachable = (name, ip, status = null, displayName = null) => ({ - name, - ip, +// fizz is unreachable because IP is unreachable and we haven't seen any of +// this robot's IP addresses recently +const EXPECTED_FIZZ = { + name: 'fizz', + displayName: 'fizz', + status: UNREACHABLE, + connected: false, local: false, - ok: false, - serverOk: false, - advertising: false, - status, - displayName, -}) + seen: false, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthStatus: HEALTH_STATUS_UNREACHABLE, + serverHealthStatus: HEALTH_STATUS_UNREACHABLE, + ip: '10.0.0.5', + port: 31950, +} + +// buzz is unreachable because we don't have any IP addresses for it +const EXPECTED_BUZZ = { + name: 'buzz', + displayName: 'buzz', + status: UNREACHABLE, + connected: false, + local: null, + seen: false, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthStatus: null, + serverHealthStatus: null, + ip: null, + port: null, +} describe('discovery selectors', () => { const SPECS = [ @@ -94,269 +227,109 @@ describe('discovery selectors', () => { expected: false, }, { - name: 'getConnectableRobots grabs robots with ok: true and health', - selector: discovery.getConnectableRobots, - state: { - discovery: { - robotsByName: { - foo: [makeConnectable('foo', '10.0.0.1')], - bar: [makeFullyUp('bar', '10.0.0.2')], - }, - }, - robot: { connection: { connectedTo: 'bar' } }, - }, + name: + 'getDiscoveredRobots assigns status based on healthStatus and serverHealthStatus', + selector: discovery.getDiscoveredRobots, + state: MOCK_STATE, expected: [ - makeConnectable('foo', '10.0.0.1', 'connectable', false, 'foo'), - makeFullyUp('bar', '10.0.0.2', 'connectable', true, 'bar'), + EXPECTED_FOO, + EXPECTED_BAR, + EXPECTED_BAZ, + EXPECTED_QUX, + EXPECTED_FIZZ, + EXPECTED_BUZZ, ], }, { - name: 'getConnectableRobots grabs correct service', + name: 'getConnectableRobots grabs robots with connectable status', selector: discovery.getConnectableRobots, - state: { - discovery: { - robotsByName: { - foo: [ - makeConnectable('foo', '10.0.0.1'), - makeConnectable('foo', '10.0.0.2'), - makeServerUp('foo', '10.0.0.3', false), - makeAdvertising('foo', '10.0.0.4', false), - ], - }, - }, - robot: { connection: { connectedTo: 'foo' } }, - }, - expected: [ - makeConnectable('foo', '10.0.0.1', 'connectable', true, 'foo'), - ], + state: MOCK_STATE, + expected: [EXPECTED_FOO, EXPECTED_BAR], }, { - name: 'getReachableRobots grabs robots with serverUp or advertising', + name: 'getReachableRobots grabs robots with reachable status', selector: discovery.getReachableRobots, - state: { - discovery: { - robotsByName: { - foo: [makeServerUp('foo', '10.0.0.1', false)], - bar: [makeAdvertising('bar', '10.0.0.2')], - }, - }, - }, - expected: [ - makeServerUp('foo', '10.0.0.1', false, 'reachable', 'foo'), - makeAdvertising('bar', '10.0.0.2', 'reachable', 'bar'), - ], - }, - { - name: 'getReachableRobots grabs correct service', - selector: discovery.getReachableRobots, - state: { - discovery: { - robotsByName: { - foo: [ - makeServerUp('foo', '10.0.0.1', true), - makeServerUp('foo', '10.0.0.1', false), - makeAdvertising('foo', '10.0.0.2'), - ], - }, - }, - }, - expected: [makeServerUp('foo', '10.0.0.1', true, 'reachable', 'foo')], + state: MOCK_STATE, + expected: [EXPECTED_BAZ, EXPECTED_QUX], }, { - name: 'getReachableRobots does not grab connectable robots', - selector: discovery.getReachableRobots, - state: { - discovery: { - robotsByName: { - foo: [ - makeConnectable('foo', '10.0.0.1'), - makeServerUp('foo', '10.0.0.2', true), - ], - bar: [ - makeConnectable('bar', '10.0.0.3'), - makeServerUp('bar', '10.0.0.4', false), - ], - baz: [ - makeConnectable('baz', '10.0.0.5'), - makeAdvertising('baz', '10.0.0.6'), - ], - qux: [makeFullyUp('qux', '10.0.0.7')], - }, - }, - }, - expected: [], - }, - { - name: 'getUnreachableRobots grabs robots with no ip', + name: 'getUnreachableRobots grabs robots with unreachable status', selector: discovery.getUnreachableRobots, - state: { - discovery: { - robotsByName: { foo: [{ name: 'foo', ip: null }] }, - }, - }, - expected: [ - { - name: 'foo', - ip: null, - status: 'unreachable', - displayName: 'foo', - }, - ], - }, - { - name: 'getUnreachableRobots grabs robots with IP but no responses', - selector: discovery.getUnreachableRobots, - state: { - discovery: { - robotsByName: { - foo: [ - makeUnreachable('foo', '10.0.0.1'), - makeUnreachable('foo', '10.0.0.2'), - ], - }, - }, - }, - expected: [makeUnreachable('foo', '10.0.0.1', 'unreachable', 'foo')], - }, - { - name: "getUnreachableRobots won't grab connectable/reachable robots", - selector: discovery.getUnreachableRobots, - state: { - discovery: { - robotsByName: { - foo: [ - makeServerUp('foo', '10.0.0.1', true), - makeUnreachable('foo', '10.0.0.2'), - ], - bar: [ - makeServerUp('bar', '10.0.0.3', false), - makeUnreachable('bar', '10.0.0.4'), - ], - baz: [ - makeAdvertising('bar', '10.0.0.5'), - makeUnreachable('baz', '10.0.0.6'), - ], - qux: [makeConnectable('qux', '10.0.0.7')], - }, - }, - }, - expected: [], + state: MOCK_STATE, + expected: [EXPECTED_FIZZ, EXPECTED_BUZZ], }, { name: 'display name removes opentrons- from connectable robot names', - selector: discovery.getConnectableRobots, + selector: discovery.getDiscoveredRobots, state: { discovery: { robotsByName: { - 'opentrons-foo': [makeConnectable('opentrons-foo', '10.0.0.1')], - 'opentrons-bar': [makeFullyUp('opentrons-bar', '10.0.0.2')], + 'opentrons-foo': { + name: 'opentrons-foo', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [], + }, }, }, - robot: { connection: { connectedTo: 'opentrons-bar' } }, + robot: { connection: { connectedTo: '' } }, }, expected: [ - makeConnectable( - 'opentrons-foo', - '10.0.0.1', - 'connectable', - false, - 'foo' - ), - makeFullyUp('opentrons-bar', '10.0.0.2', 'connectable', true, 'bar'), + expect.objectContaining({ name: 'opentrons-foo', displayName: 'foo' }), ], }, { - name: 'display name removes opentrons- from reachable robot names', - selector: discovery.getReachableRobots, - state: { - discovery: { - robotsByName: { - 'opentrons-foo': [makeServerUp('opentrons-foo', '10.0.0.1', false)], - 'opentrons-bar': [makeAdvertising('opentrons-bar', '10.0.0.2')], - }, - }, - }, - expected: [ - makeServerUp('opentrons-foo', '10.0.0.1', false, 'reachable', 'foo'), - makeAdvertising('opentrons-bar', '10.0.0.2', 'reachable', 'bar'), - ], - }, - { - name: 'display name removes opentrons- from unreachable robot names', - selector: discovery.getUnreachableRobots, - state: { - discovery: { - robotsByName: { - 'opentrons-foo': [makeUnreachable('opentrons-foo', null)], - }, - }, - }, - expected: [makeUnreachable('opentrons-foo', null, 'unreachable', 'foo')], - }, - { - name: 'getAllRobots returns all robots', - selector: discovery.getAllRobots, + name: + 'handles legacy IPv6 robots by wrapping IP in [] and setting as local', + selector: discovery.getDiscoveredRobots, state: { - robot: { connection: { connectedTo: 'qux' } }, discovery: { robotsByName: { - foo: [ - makeConnectable('foo', '10.0.0.1'), - makeUnreachable('foo', '10.0.0.2'), - ], - bar: [ - makeServerUp('bar', '10.0.0.3', false), - makeUnreachable('bar', '10.0.0.4'), - ], - baz: [ - makeAdvertising('baz', '10.0.0.5'), - makeUnreachable('baz', '10.0.0.6'), - ], - qux: [makeFullyUp('qux', '10.0.0.7')], + 'opentrons-foo': { + name: 'opentrons-foo', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + addresses: [ + { + ip: 'fd00:0:cafe:fefe::1', + port: 31950, + seen: true, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_UNREACHABLE, + healthError: null, + serverHealthError: mockHealthFetchErrorResponse, + }, + ], + }, }, }, + robot: { connection: { connectedTo: '' } }, }, expected: [ - makeConnectable('foo', '10.0.0.1', 'connectable', false, 'foo'), - makeFullyUp('qux', '10.0.0.7', 'connectable', true, 'qux'), - makeServerUp('bar', '10.0.0.3', false, 'reachable', 'bar'), - makeAdvertising('baz', '10.0.0.5', 'reachable', 'baz'), + expect.objectContaining({ + name: 'opentrons-foo', + ip: '[fd00:0:cafe:fefe::1]', + local: true, + }), ], }, { name: 'getViewableRobots returns connectable and reachable robots', selector: discovery.getViewableRobots, - state: { - discovery: { - robotsByName: { - foo: [makeConnectable('foo', '10.0.0.1')], - bar: [makeFullyUp('bar', '10.0.0.2')], - baz: [makeServerUp('baz', '10.0.0.3', false)], - qux: [makeAdvertising('qux', '10.0.0.4')], - }, - }, - robot: { connection: { connectedTo: 'bar' } }, - }, - expected: [ - makeConnectable('foo', '10.0.0.1', 'connectable', false, 'foo'), - makeFullyUp('bar', '10.0.0.2', 'connectable', true, 'bar'), - makeServerUp('baz', '10.0.0.3', false, 'reachable', 'baz'), - makeAdvertising('qux', '10.0.0.4', 'reachable', 'qux'), - ], + state: MOCK_STATE, + expected: [EXPECTED_FOO, EXPECTED_BAR, EXPECTED_BAZ, EXPECTED_QUX], }, { - name: 'getConnectedRobot returns connected robot', + name: 'getConnectedRobot returns connected robot if connectable', selector: discovery.getConnectedRobot, - state: { - discovery: { - robotsByName: { - foo: [makeConnectable('foo', '10.0.0.1')], - bar: [makeFullyUp('bar', '10.0.0.2')], - }, - }, - robot: { connection: { connectedTo: 'bar' } }, - }, - expected: makeFullyUp('bar', '10.0.0.2', 'connectable', true, 'bar'), + state: MOCK_STATE, + expected: EXPECTED_BAR, + }, + { + name: 'getConnectedRobot returns null if not connectable', + selector: discovery.getConnectedRobot, + state: { ...MOCK_STATE, robot: { connection: { connectedTo: 'fizz' } } }, + expected: null, }, { name: 'getRobotApiVersion returns health.apiServerVersion', @@ -451,74 +424,37 @@ describe('discovery selectors', () => { { name: 'getRobotByName returns connectable robot by name', selector: discovery.getRobotByName, - state: { - discovery: { - robotsByName: { - foo: [makeConnectable('foo', '10.0.0.1')], - bar: [makeFullyUp('bar', '10.0.0.2')], - baz: [makeServerUp('baz', '10.0.0.3', false)], - qux: [makeAdvertising('qux', '10.0.0.4')], - }, - }, - robot: { connection: { connectedTo: null } }, - }, + state: MOCK_STATE, args: ['foo'], - expected: makeConnectable('foo', '10.0.0.1', 'connectable', false, 'foo'), + expected: EXPECTED_FOO, }, { name: 'getRobotByName returns reachable robot by name', selector: discovery.getRobotByName, - state: { - discovery: { - robotsByName: { - foo: [makeConnectable('foo', '10.0.0.1')], - bar: [makeFullyUp('bar', '10.0.0.2')], - baz: [makeServerUp('baz', '10.0.0.3', false)], - qux: [makeAdvertising('qux', '10.0.0.4')], - }, - }, - robot: { connection: { connectedTo: null } }, - }, + state: MOCK_STATE, args: ['baz'], - expected: makeServerUp('baz', '10.0.0.3', false, 'reachable', 'baz'), + expected: EXPECTED_BAZ, + }, + { + name: 'getRobotByName returns null if robot is not connectable', + selector: discovery.getRobotByName, + state: MOCK_STATE, + args: ['fizz'], + expected: null, }, { name: 'getRobotApiVersionByName returns API version of connectable robot', selector: discovery.getRobotApiVersionByName, - state: { - discovery: { - robotsByName: { - foo: [ - { - ...makeConnectable('foo', '10.0.0.1'), - health: { api_version: '4.5.6' }, - }, - ], - }, - }, - robot: { connection: { connectedTo: null } }, - }, + state: MOCK_STATE, args: ['foo'], - expected: '4.5.6', + expected: EXPECTED_FOO.health.api_version, }, { name: 'getRobotApiVersionByName returns API version of reachable robot', selector: discovery.getRobotApiVersionByName, - state: { - discovery: { - robotsByName: { - baz: [ - { - ...makeServerUp('baz', '10.0.0.3', false), - serverHealth: { apiServerVersion: '1.2.3' }, - }, - ], - }, - }, - robot: { connection: { connectedTo: null } }, - }, + state: MOCK_STATE, args: ['baz'], - expected: '1.2.3', + expected: EXPECTED_BAZ.health.api_version, }, ] diff --git a/app/src/discovery/constants.js b/app/src/discovery/constants.js new file mode 100644 index 00000000000..7cc854481e5 --- /dev/null +++ b/app/src/discovery/constants.js @@ -0,0 +1,15 @@ +// @flow + +export { + HEALTH_STATUS_OK, + HEALTH_STATUS_NOT_OK, + HEALTH_STATUS_UNREACHABLE, + RE_HOSTNAME_IPV6_LL, + RE_HOSTNAME_IPV4_LL, + RE_HOSTNAME_LOCALHOST, + RE_HOSTNAME_LOOPBACK, +} from '@opentrons/discovery-client/src/constants' + +export const CONNECTABLE: 'connectable' = 'connectable' +export const REACHABLE: 'reachable' = 'reachable' +export const UNREACHABLE: 'unreachable' = 'unreachable' diff --git a/app/src/discovery/index.js b/app/src/discovery/index.js index 6ab110f1888..f0538522f5b 100644 --- a/app/src/discovery/index.js +++ b/app/src/discovery/index.js @@ -1,3 +1,4 @@ // @flow export * from './actions' +export * from './constants' export * from './selectors' diff --git a/app/src/discovery/reducer.js b/app/src/discovery/reducer.js index 5c36f68f7b0..15ea00027fb 100644 --- a/app/src/discovery/reducer.js +++ b/app/src/discovery/reducer.js @@ -1,15 +1,11 @@ // @flow // robot discovery state -import groupBy from 'lodash/groupBy' +import keyBy from 'lodash/keyBy' import { UI_INITIALIZED } from '../shell' import * as actions from './actions' import type { Action } from '../types' -import type { Service, RobotsMap, DiscoveryState } from './types' - -export const normalizeRobots = (robots: Array = []): RobotsMap => { - return groupBy(robots, 'name') -} +import type { DiscoveryState } from './types' export const INITIAL_STATE: DiscoveryState = { scanning: false, @@ -29,7 +25,7 @@ export function discoveryReducer( return { ...state, scanning: false } case actions.DISCOVERY_UPDATE_LIST: { - return { ...state, robotsByName: normalizeRobots(action.payload.robots) } + return { ...state, robotsByName: keyBy(action.payload.robots, 'name') } } } diff --git a/app/src/discovery/selectors.js b/app/src/discovery/selectors.js index fc6a401cc7f..035d9da1975 100644 --- a/app/src/discovery/selectors.js +++ b/app/src/discovery/selectors.js @@ -1,121 +1,131 @@ // @flow +import isIp from 'is-ip' import concat from 'lodash/concat' -import filter from 'lodash/filter' import find from 'lodash/find' -import groupBy from 'lodash/groupBy' import head from 'lodash/head' -import map from 'lodash/map' -import mapValues from 'lodash/mapValues' -import pickBy from 'lodash/pickBy' import { createSelector } from 'reselect' import semver from 'semver' +import { + HEALTH_STATUS_OK, + HEALTH_STATUS_UNREACHABLE, + RE_HOSTNAME_IPV6_LL, + RE_HOSTNAME_IPV4_LL, + RE_HOSTNAME_LOCALHOST, + RE_HOSTNAME_LOOPBACK, + CONNECTABLE, + REACHABLE, + UNREACHABLE, +} from './constants' + // TODO(mc, 2018-10-10): fix circular dependency with RPC API client // that requires us to bypass the robot entry point here import { getConnectedRobotName } from '../robot/selectors' -import type { Service } from '@opentrons/discovery-client' import type { State } from '../types' import type { - ResolvedRobot, + DiscoveredRobot, Robot, ReachableRobot, UnreachableRobot, ViewableRobot, - AnyRobot, - ConnectableStatus, - ReachableStatus, - UnreachableStatus, } from './types' -type GroupedRobotsMap = { - [name: string]: { - connectable: Array, - reachable: Array, - unreachable: Array, - }, -} - -type GetGroupedRobotsMap = State => GroupedRobotsMap type GetConnectableRobots = State => Array type GetReachableRobots = State => Array type GetUnreachableRobots = State => Array -type GetAllRobots = State => Array +type GetAllRobots = State => Array type GetViewableRobots = State => Array -type GetConnectedRobot = State => ?Robot - -export const CONNECTABLE: ConnectableStatus = 'connectable' -export const REACHABLE: ReachableStatus = 'reachable' -export const UNREACHABLE: UnreachableStatus = 'unreachable' - -const isResolved = (s: Service) => - s.ip != null && s.local != null && s.ok != null && s.serverOk != null +type GetConnectedRobot = State => Robot | null -const isConnectable = (s: ResolvedRobot) => s.ok === true && s.health != null +const makeDisplayName = (name: string): string => name.replace('opentrons-', '') -const isReachable = (s: ResolvedRobot) => - s.advertising === true || s.serverOk === true +const isLocal = (ip: string) => { + return ( + RE_HOSTNAME_IPV6_LL.test(ip) || + RE_HOSTNAME_IPV4_LL.test(ip) || + RE_HOSTNAME_LOCALHOST.test(ip) || + RE_HOSTNAME_LOOPBACK.test(ip) + ) +} -const maybeGetResolved = (service: Service): ?ResolvedRobot => - isResolved(service) ? (service: any) : null +const ipToHostname = (ip: string) => (isIp.v6(ip) ? `[${ip}]` : ip) -const makeDisplayName = (service: Service): string => - service.name.replace('opentrons-', '') +export function getScanning(state: State): boolean { + return state.discovery.scanning +} -// group services of each robot into connectable, reachable, and unconnectable -// sort order will be preserved from state (and therefore discovery-client), -// so the head of each group will be the most "desirable" option for that group -const getGroupedRobotsMap: GetGroupedRobotsMap = createSelector( +export const getDiscoveredRobots: State => Array = createSelector( state => state.discovery.robotsByName, - robotsMap => - mapValues(robotsMap, services => { - const servicesWithStatus = services.map(s => { - const resolved = maybeGetResolved(s) - const service = { ...s, displayName: makeDisplayName(s) } - - if (resolved) { - if (isConnectable(resolved)) { - return { ...service, status: CONNECTABLE } + getConnectedRobotName, + (robotsMap, connectedRobotName) => { + return Object.keys(robotsMap).map((robotName: string) => { + const robot = robotsMap[robotName] + const { addresses, ...robotState } = robot + const { health } = robotState + const addr = head(addresses) + const ip = addr?.ip ? ipToHostname(addr.ip) : null + const port = addr?.port ?? null + const healthStatus = addr?.healthStatus ?? null + const serverHealthStatus = addr?.serverHealthStatus ?? null + const baseRobot = { + ...robotState, + displayName: makeDisplayName(robotName), + connected: robotName === connectedRobotName, + local: ip !== null ? isLocal(ip) : null, + seen: addr?.seen === true, + } + + if (ip !== null && port !== null && healthStatus && serverHealthStatus) { + if (health && healthStatus === HEALTH_STATUS_OK) { + return { + ...baseRobot, + ip, + port, + health, + serverHealthStatus, + healthStatus: HEALTH_STATUS_OK, + status: CONNECTABLE, } - if (isReachable(resolved)) return { ...service, status: REACHABLE } } - return { ...service, status: UNREACHABLE } - }) - return groupBy(servicesWithStatus, 'status') + if (healthStatus !== HEALTH_STATUS_UNREACHABLE || addr?.seen) { + return { + ...baseRobot, + ip, + port, + healthStatus, + serverHealthStatus, + status: REACHABLE, + } + } + } + + return { + ...baseRobot, + ip, + port, + healthStatus, + serverHealthStatus, + status: UNREACHABLE, + } }) + } ) -export function getScanning(state: State): boolean { - return state.discovery.scanning -} - export const getConnectableRobots: GetConnectableRobots = createSelector( - getGroupedRobotsMap, - getConnectedRobotName, - (robotsMap, connectedName) => - map(robotsMap, g => head(g.connectable)) - .filter(Boolean) - .map(r => ({ ...r, connected: r.name === connectedName })) + getDiscoveredRobots, + robots => robots.flatMap(r => (r.status === CONNECTABLE ? [r] : [])) ) export const getReachableRobots: GetReachableRobots = createSelector( - getGroupedRobotsMap, - robotsMap => - filter(robotsMap, g => !g.connectable) - .map(g => head(g.reachable)) - .filter(Boolean) + getDiscoveredRobots, + robots => robots.flatMap(r => (r.status === REACHABLE ? [r] : [])) ) export const getUnreachableRobots: GetUnreachableRobots = createSelector( - getGroupedRobotsMap, - robotsMap => { - const unreachableMap = pickBy( - robotsMap, - g => !g.connectable && !g.reachable - ) - return map(unreachableMap, g => head(g.unreachable)).filter(Boolean) - } + getDiscoveredRobots, + robots => robots.flatMap(r => (r.status === UNREACHABLE ? [r] : [])) ) export const getAllRobots: GetAllRobots = createSelector( @@ -133,7 +143,7 @@ export const getViewableRobots: GetViewableRobots = createSelector( export const getConnectedRobot: GetConnectedRobot = createSelector( getConnectableRobots, - robots => find(robots, 'connected') + robots => find(robots, 'connected') ?? null ) export const getRobotByName = ( @@ -143,17 +153,21 @@ export const getRobotByName = ( return getViewableRobots(state).find(r => r.name === robotName) || null } -export const getRobotApiVersion = (robot: AnyRobot): string | null => +export const getRobotApiVersion = (robot: DiscoveredRobot): string | null => (robot.health && semver.valid(robot.health.api_version)) ?? (robot.serverHealth && semver.valid(robot.serverHealth.apiServerVersion)) ?? null -export const getRobotFirmwareVersion = (robot: AnyRobot): string | null => +export const getRobotFirmwareVersion = ( + robot: DiscoveredRobot +): string | null => (robot.health && robot.health.fw_version) ?? (robot.serverHealth && robot.serverHealth.smoothieVersion) ?? null -export const getRobotProtocolApiVersion = (robot: AnyRobot): string | null => { +export const getRobotProtocolApiVersion = ( + robot: DiscoveredRobot +): string | null => { const maxApiVersion = robot.health?.protocol_api_version return maxApiVersion ? maxApiVersion.join('.') : null } diff --git a/app/src/discovery/types.js b/app/src/discovery/types.js index 780168283cc..b6e2c65cbef 100644 --- a/app/src/discovery/types.js +++ b/app/src/discovery/types.js @@ -1,56 +1,69 @@ // @flow -import type { Service } from '@opentrons/discovery-client' +import type { + DiscoveryClientRobot, + HealthResponse, + HealthStatus, +} from '@opentrons/discovery-client' -export type { Service } +import typeof { + HEALTH_STATUS_OK, + CONNECTABLE, + REACHABLE, + UNREACHABLE, +} from './constants' -export type RobotsMap = $Shape<{| [name: string]: Array |}> +export type { DiscoveryClientRobot, HealthStatus } + +export type RobotsMap = $Shape<{| [name: string]: DiscoveryClientRobot |}> export type DiscoveryState = {| scanning: boolean, robotsByName: RobotsMap, |} -export type ConnectableStatus = 'connectable' -export type ReachableStatus = 'reachable' -export type UnreachableStatus = 'unreachable' - -// service with a known IP address -export type ResolvedRobot = {| - ...$Exact, - ip: $NonMaybeType<$PropertyType>, - local: $NonMaybeType<$PropertyType>, - ok: $NonMaybeType<$PropertyType>, - serverOk: $NonMaybeType<$PropertyType>, +export type BaseRobot = {| + ...$Rest, displayName: string, + connected: boolean, + local: boolean | null, + seen: boolean, |} // fully connectable robot export type Robot = {| - ...ResolvedRobot, - ok: true, - health: $NonMaybeType<$PropertyType>, - status: ConnectableStatus, - connected: boolean, + ...BaseRobot, + status: CONNECTABLE, + health: HealthResponse, + ip: string, + port: number, + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HealthStatus, |} -// robot with a known IP (i.e. advertising over mDNS) but unconnectable +// robot with a seen, but not connecteable IP export type ReachableRobot = {| - ...ResolvedRobot, - ok: false, - status: ReachableStatus, + ...BaseRobot, + status: REACHABLE, + ip: string, + port: number, + healthStatus: HealthStatus, + serverHealthStatus: HealthStatus, |} -// robot with an unknown IP +// robot with no reachable IP export type UnreachableRobot = {| - ...$Exact, - displayName: string, - status: UnreachableStatus, + ...BaseRobot, + status: UNREACHABLE, + ip: string | null, + port: number | null, + healthStatus: HealthStatus | null, + serverHealthStatus: HealthStatus | null, |} export type ViewableRobot = Robot | ReachableRobot -export type AnyRobot = Robot | ReachableRobot | UnreachableRobot +export type DiscoveredRobot = Robot | ReachableRobot | UnreachableRobot export type StartDiscoveryAction = {| type: 'discovery:START', @@ -65,7 +78,7 @@ export type FinishDiscoveryAction = {| export type UpdateListAction = {| type: 'discovery:UPDATE_LIST', - payload: {| robots: Array |}, + payload: {| robots: Array |}, |} export type RemoveRobotAction = {| diff --git a/app/src/robot-admin/__tests__/reducer.test.js b/app/src/robot-admin/__tests__/reducer.test.js index 245b440de3d..e172054698d 100644 --- a/app/src/robot-admin/__tests__/reducer.test.js +++ b/app/src/robot-admin/__tests__/reducer.test.js @@ -1,5 +1,7 @@ // @flow +import { mockDiscoveryClientRobot } from '../../discovery/__fixtures__' +import { HEALTH_STATUS_NOT_OK } from '../../discovery' import { robotAdminReducer } from '../reducer' import type { Action } from '../../types' @@ -35,13 +37,11 @@ describe('robotAdminReducer', () => { expected: { robotName: { status: 'restart-failed' } }, }, { - name: 'discovery:UPDATE_LIST sets status to up if ok: true', + name: 'discovery:UPDATE_LIST sets status to up if health status is ok', action: { type: 'discovery:UPDATE_LIST', payload: { - robots: [ - ({ name: 'a', ip: '192.168.1.1', port: 31950, ok: true }: any), - ], + robots: [{ ...mockDiscoveryClientRobot, name: 'a' }], }, }, state: { @@ -50,13 +50,12 @@ describe('robotAdminReducer', () => { expected: { a: { status: 'up' } }, }, { - name: 'discovery:UPDATE_LIST leaves restart pending alone if ok: true', + name: + 'discovery:UPDATE_LIST leaves restart pending alone if health status is ok', action: { type: 'discovery:UPDATE_LIST', payload: { - robots: [ - ({ name: 'a', ip: '192.168.1.1', port: 31950, ok: true }: any), - ], + robots: [{ ...mockDiscoveryClientRobot, name: 'a' }], }, }, state: { @@ -66,12 +65,21 @@ describe('robotAdminReducer', () => { }, { name: - 'discovery:UPDATE_LIST sets restarting if restart pending and ok: false', + 'discovery:UPDATE_LIST sets restarting if restart pending health status not ok', action: { type: 'discovery:UPDATE_LIST', payload: { robots: [ - ({ name: 'a', ip: '192.168.1.1', port: 31950, ok: false }: any), + { + ...mockDiscoveryClientRobot, + name: 'a', + addresses: [ + { + ...mockDiscoveryClientRobot.addresses[0], + healthStatus: HEALTH_STATUS_NOT_OK, + }, + ], + }, ], }, }, @@ -87,7 +95,16 @@ describe('robotAdminReducer', () => { type: 'discovery:UPDATE_LIST', payload: { robots: [ - ({ name: 'a', ip: '192.168.1.1', port: 31950, ok: false }: any), + { + ...mockDiscoveryClientRobot, + name: 'a', + addresses: [ + { + ...mockDiscoveryClientRobot.addresses[0], + healthStatus: HEALTH_STATUS_NOT_OK, + }, + ], + }, ], }, }, diff --git a/app/src/robot-admin/reducer.js b/app/src/robot-admin/reducer.js index 27d484445ef..add73c1b25b 100644 --- a/app/src/robot-admin/reducer.js +++ b/app/src/robot-admin/reducer.js @@ -1,6 +1,7 @@ // @flow +import fromPairs from 'lodash/fromPairs' import mapValues from 'lodash/mapValues' -import { DISCOVERY_UPDATE_LIST } from '../discovery/actions' +import { DISCOVERY_UPDATE_LIST, HEALTH_STATUS_OK } from '../discovery' import * as Constants from './constants' import type { Action } from '../types' @@ -51,17 +52,19 @@ export function robotAdminReducer( case DISCOVERY_UPDATE_LIST: { const { robots } = action.payload - const upByName = robots.reduce<$Shape<{| [string]: boolean | void |}>>( - (result, service) => ({ - ...result, - [service.name]: result[service.name] || service.ok, - }), - {} + const upByName = fromPairs( + robots.map(robot => [ + robot.name, + robot.addresses.some(a => a.healthStatus === HEALTH_STATUS_OK), + ]) ) return mapValues( state, - (robotState: PerRobotAdminState, robotName: string) => { + ( + robotState: PerRobotAdminState, + robotName: string + ): PerRobotAdminState => { let { status } = robotState const up = upByName[robotName] diff --git a/app/src/robot-api/types.js b/app/src/robot-api/types.js index 841c68c6e80..a55d55a4e09 100644 --- a/app/src/robot-api/types.js +++ b/app/src/robot-api/types.js @@ -8,6 +8,7 @@ export type RobotHost = { name: string, ip: string, port: number, + ... } export type RobotApiRequestOptions = {| diff --git a/app/src/robot/test/api-client.test.js b/app/src/robot/test/api-client.test.js index 8a49475e8cc..12404259b08 100755 --- a/app/src/robot/test/api-client.test.js +++ b/app/src/robot/test/api-client.test.js @@ -14,11 +14,14 @@ import { MockSessionNoDoorInfo, } from './__fixtures__/session' import { MockCalibrationManager } from './__fixtures__/calibration-manager' +import { mockConnectableRobot } from '../../discovery/__fixtures__' +import { getConnectableRobots } from '../../discovery/selectors' import { getLabwareDefBySlot } from '../../protocol/selectors' import { getCustomLabwareDefinitions } from '../../custom-labware/selectors' jest.mock('../../rpc/client') +jest.mock('../../discovery/selectors') jest.mock('../../protocol/selectors') jest.mock('../../custom-labware/selectors') @@ -58,9 +61,6 @@ describe('api client', () => { create_from_bundle: jest.fn(), } rpcClient = { - // TODO(mc, 2017-09-22): these jest promise mocks are causing promise - // rejection warnings. These warnings are Jest's fault for nextTick stuff - // http://clarkdave.net/2016/09/node-v6-6-and-asynchronously-handled-promise-rejections/ on: jest.fn(() => rpcClient), removeAllListeners: jest.fn(() => rpcClient), close: jest.fn(), @@ -75,6 +75,7 @@ describe('api client', () => { getLabwareDefBySlot.mockReturnValue({}) getCustomLabwareDefinitions.mockReturnValue([]) + getConnectableRobots.mockReturnValue([mockConnectableRobot]) const _receive = client(dispatch) @@ -89,8 +90,8 @@ describe('api client', () => { jest.resetAllMocks() }) - const ROBOT_NAME = 'ot' - const ROBOT_IP = '127.0.0.1' + const ROBOT_NAME = mockConnectableRobot.name + const ROBOT_IP = mockConnectableRobot.ip const STATE = { robot: { connection: { @@ -98,22 +99,6 @@ describe('api client', () => { connectRequest: { inProgress: false }, }, }, - discovery: { - robotsByName: { - [ROBOT_NAME]: [ - { - name: ROBOT_NAME, - ip: ROBOT_IP, - local: true, - port: 31950, - ok: true, - serverOk: true, - health: {}, - serverHealth: {}, - }, - ], - }, - }, } const sendConnect = () => sendToClient(STATE, actions.connect(ROBOT_NAME)) diff --git a/discovery-client/src/__fixtures__/index.js b/discovery-client/src/__fixtures__/index.js new file mode 100644 index 00000000000..c2d0ff5039e --- /dev/null +++ b/discovery-client/src/__fixtures__/index.js @@ -0,0 +1,4 @@ +// @flow + +export * from './health' +export * from './mdns-browser-service' diff --git a/discovery-client/src/__fixtures__/mdns-browser-service.js b/discovery-client/src/__fixtures__/mdns-browser-service.js index 1dedd032c98..dca545f14bb 100644 --- a/discovery-client/src/__fixtures__/mdns-browser-service.js +++ b/discovery-client/src/__fixtures__/mdns-browser-service.js @@ -1,7 +1,7 @@ // @flow import type { BrowserService, MdnsServiceType } from 'mdns-js' -export const MOCK_BROWSER_SERVICE: BrowserService = { +export const mockBrowserService: BrowserService = { addresses: ['192.168.1.42'], query: ['_http._tcp.local'], type: [ diff --git a/discovery-client/src/__fixtures__/service.js b/discovery-client/src/__fixtures__/service.js deleted file mode 100644 index 370ed5fa2ff..00000000000 --- a/discovery-client/src/__fixtures__/service.js +++ /dev/null @@ -1,14 +0,0 @@ -// @flow -import type { Service } from '../types' - -export const MOCK_SERVICE: Service = { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: null, - serverOk: null, - advertising: null, - health: null, - serverHealth: null, -} diff --git a/discovery-client/src/__tests__/client.test.js b/discovery-client/src/__tests__/client.test.js deleted file mode 100644 index 374855b3ee1..00000000000 --- a/discovery-client/src/__tests__/client.test.js +++ /dev/null @@ -1,562 +0,0 @@ -import mdns from 'mdns-js' - -import { createDiscoveryClient } from '..' -import * as poller from '../poller' -import * as service from '../service' -import * as serviceList from '../service-list' -import { MOCK_BROWSER_SERVICE } from '../__fixtures__/mdns-browser-service' -import { MOCK_SERVICE } from '../__fixtures__/service' - -jest.mock('mdns-js') -jest.mock('../poller') - -describe('discovery client', () => { - beforeAll(() => { - // spies - service.fromMdnsBrowser = jest.fn(service.fromMdnsBrowser) - serviceList.createServiceList = jest.fn(serviceList.createServiceList) - }) - - beforeEach(() => { - mdns.__mockReset() - jest.clearAllMocks() - }) - - it('start creates mdns browser searching for http', () => { - const client = createDiscoveryClient() - const result = client.start() - - expect(result).toBe(client) - expect(mdns.createBrowser).toHaveBeenCalledWith(mdns.tcp('http')) - expect(mdns.__mockBrowser.discover).not.toHaveBeenCalled() - }) - - it('mdns browser started on ready', () => { - const client = createDiscoveryClient() - - client.start() - expect(mdns.__mockBrowser.discover).toHaveBeenCalledTimes(0) - mdns.__mockBrowser.emit('ready') - expect(mdns.__mockBrowser.discover).toHaveBeenCalledTimes(1) - }) - - it('stops browser on client.stop', () => { - const client = createDiscoveryClient() - - client.start() - const result = client.stop() - expect(result).toBe(client) - expect(mdns.__mockBrowser.stop).toHaveBeenCalled() - }) - - it('stops browser and creates new one on repeated client.start', () => { - const client = createDiscoveryClient() - - client.start() - expect(mdns.createBrowser).toHaveBeenCalledTimes(1) - expect(mdns.__mockBrowser.stop).toHaveBeenCalledTimes(0) - client.start() - expect(mdns.createBrowser).toHaveBeenCalledTimes(2) - expect(mdns.__mockBrowser.stop).toHaveBeenCalledTimes(1) - }) - - it('emits "service" if browser finds a service', done => { - const client = createDiscoveryClient() - - client.start() - client.once('service', results => { - expect(service.fromMdnsBrowser).toHaveBeenCalledWith(MOCK_BROWSER_SERVICE) - expect(results).toHaveLength(1) - expect(results[0]).toEqual({ ...MOCK_SERVICE, advertising: true }) - done() - }) - - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - }, 10) - - it('adds robot to client.services if browser finds a service', () => { - const client = createDiscoveryClient() - - client.start() - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - expect(client.services).toEqual([{ ...MOCK_SERVICE, advertising: true }]) - expect(client.candidates).toEqual([]) - }) - - it('new mdns service does not null out ok of existing service', () => { - const client = createDiscoveryClient() - - client.services = [ - { - ...MOCK_SERVICE, - ok: true, - serverOk: true, - advertising: true, - }, - ] - - client.start() - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - - expect(client.services).toEqual([ - { - ...MOCK_SERVICE, - ok: true, - serverOk: true, - advertising: true, - }, - ]) - }) - - it('services and candidates can be prepopulated', () => { - const cachedServices = [MOCK_SERVICE] - const client = createDiscoveryClient({ - services: cachedServices, - candidates: [{ ip: '192.168.1.43', port: 31950 }], - }) - - expect(serviceList.createServiceList).toHaveBeenCalledWith(cachedServices) - expect(client.services).toEqual(cachedServices) - expect(client.candidates).toEqual([{ ip: '192.168.1.43', port: 31950 }]) - }) - - it('candidates should be deduped by services', () => { - const client = createDiscoveryClient({ - services: [MOCK_SERVICE], - candidates: [{ ip: MOCK_SERVICE.ip, port: 31950 }], - }) - - expect(client.candidates).toEqual([]) - }) - - it('client.start should start polling with default interval 5000', () => { - const client = createDiscoveryClient({ - services: [ - { - ...MOCK_SERVICE, - ip: 'foo', - port: 1, - }, - ], - candidates: [{ ip: 'bar', port: 2 }, { ip: 'baz', port: 3 }], - }) - - client.start() - expect(poller.poll).toHaveBeenCalledWith( - [{ ip: 'foo', port: 1 }, { ip: 'bar', port: 2 }, { ip: 'baz', port: 3 }], - 5000, - expect.any(Function), - client._logger - ) - }) - - it('client should have configurable poll interval', () => { - const client = createDiscoveryClient({ - pollInterval: 1000, - candidates: [{ ip: 'foo', port: 31950 }], - }) - - client.start() - expect(poller.poll).toHaveBeenCalledWith( - expect.anything(), - 1000, - expect.anything(), - client._logger - ) - }) - - it('client.stop should stop polling', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - poller.poll.mockReturnValueOnce({ id: 'foobar' }) - client.start() - client.stop() - expect(poller.stop).toHaveBeenCalledWith({ id: 'foobar' }, client._logger) - }) - - it('if polls come back good, oks should be flagged true from null', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'opentrons-dev' }, - { name: 'opentrons-dev' } - ) - - expect(client.services).toHaveLength(1) - expect(client.services[0].ok).toBe(true) - expect(client.services[0].serverOk).toBe(true) - }) - - it('if polls come back good, oks should be flagged true from false', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.services[0].ok = false - client.services[0].serverOk = false - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'opentrons-dev' }, - { name: 'opentrons-dev' } - ) - - expect(client.services[0].ok).toBe(true) - expect(client.services[0].serverOk).toBe(true) - }) - - it('if API health comes back bad, ok should be flagged false from null', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth({ ip: '192.168.1.42', port: 31950 }, null, { - name: 'opentrons-dev', - }) - expect(client.services[0].ok).toBe(false) - expect(client.services[0].serverOk).toBe(true) - }) - - it('if API health comes back bad, ok should be flagged false from true', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.services[0].ok = true - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth({ ip: '192.168.1.42', port: 31950 }, null, { - name: 'opentrons-dev', - }) - expect(client.services[0].ok).toBe(false) - expect(client.services[0].serverOk).toBe(true) - }) - - it('if /server health comes back bad, serverOk should be flagged false from null', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'opentrons-dev' }, - null - ) - - expect(client.services[0].ok).toBe(true) - expect(client.services[0].serverOk).toBe(false) - }) - - it('if /server health comes back bad, serverOk should be flagged false from true', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.services[0].serverOk = true - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'opentrons-dev' }, - null - ) - - expect(client.services[0].ok).toBe(true) - expect(client.services[0].serverOk).toBe(false) - }) - - it('if both polls comes back bad, oks should be flagged false from null', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth({ ip: '192.168.1.42', port: 31950 }, null, null) - - expect(client.services).toHaveLength(1) - expect(client.services[0].ok).toBe(false) - expect(client.services[0].serverOk).toBe(false) - }) - - it('if both polls comes back bad, oks should be flagged false from true', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.services[0].ok = true - client.services[0].serverOk = true - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth({ ip: '192.168.1.42', port: 31950 }, null, null) - - expect(client.services).toHaveLength(1) - expect(client.services[0].ok).toBe(false) - expect(client.services[0].serverOk).toBe(false) - }) - - it('if names come back conflicting, prefer /server and set ok to false', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'something-else' }, - { name: 'opentrons-dev' } - ) - - expect(client.services[0].ok).toBe(false) - expect(client.services[0].serverOk).toBe(true) - }) - - it('if health comes back for a candidate, it should be promoted', () => { - const client = createDiscoveryClient({ - candidates: [{ ip: '192.168.1.42', port: 31950 }], - }) - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: '192.168.1.42', port: 31950 }, - { name: 'opentrons-dev' }, - { name: 'opentrons-dev' } - ) - - expect(client.candidates).toEqual([]) - expect(client.services).toEqual([ - { - ...MOCK_SERVICE, - ok: true, - serverOk: true, - health: { name: 'opentrons-dev' }, - serverHealth: { name: 'opentrons-dev' }, - }, - ]) - }) - - it('if health comes back with IP conflict, null out old services', () => { - const client = createDiscoveryClient() - - client.services = [ - { ...MOCK_SERVICE, name: 'bar', ok: true, serverOk: true }, - { ...MOCK_SERVICE, name: 'baz', ok: true, serverOk: true }, - ] - - client.start() - const onHealth = poller.poll.mock.calls[0][2] - onHealth( - { ip: MOCK_SERVICE.ip, port: 31950 }, - { name: 'opentrons-dev' }, - { name: 'opentrons-dev' } - ) - - expect(client.services).toEqual([ - { - ...MOCK_SERVICE, - ok: true, - serverOk: true, - health: { name: 'opentrons-dev' }, - serverHealth: { name: 'opentrons-dev' }, - }, - { ...MOCK_SERVICE, name: 'bar', ip: null, local: null }, - { ...MOCK_SERVICE, name: 'baz', ip: null, local: null }, - ]) - }) - - it('if new service is added, poller is restarted', () => { - const client = createDiscoveryClient({ - candidates: [{ ip: '192.168.1.1', port: 31950 }], - }) - - poller.poll.mockReturnValueOnce({ id: 1234 }) - - client.start() - expect(poller.poll).toHaveBeenLastCalledWith( - [{ ip: '192.168.1.1', port: 31950 }], - expect.anything(), - expect.anything(), - client._logger - ) - - client.once('service', robot => { - expect(poller.stop).toHaveBeenLastCalledWith({ id: 1234 }, client._logger) - expect(poller.poll).toHaveBeenLastCalledWith( - [ - { ip: '192.168.1.42', port: 31950 }, - { ip: '192.168.1.1', port: 31950 }, - ], - expect.anything(), - expect.anything(), - client._logger - ) - }) - - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - }) - - it('services may be removed and removes candidates', () => { - const client = createDiscoveryClient({ - services: [ - { - ...MOCK_SERVICE, - ip: '192.168.1.42', - port: 31950, - }, - { - ...MOCK_SERVICE, - ip: '[fd00:0:cafe:fefe::1]', - port: 31950, - }, - ], - }) - - client.start() - const result = client.remove('opentrons-dev') - expect(result).toBe(client) - expect(client.services).toEqual([]) - expect(client.candidates).toEqual([]) - }) - - it('candidate removal restarts poll', () => { - const client = createDiscoveryClient({ services: [MOCK_SERVICE] }) - - poller.poll.mockReturnValueOnce({ id: 1234 }) - client.start() - client.remove('opentrons-dev') - expect(poller.stop).toHaveBeenLastCalledWith({ id: 1234 }, client._logger) - expect(poller.poll).toHaveBeenLastCalledWith( - [], - expect.anything(), - expect.anything(), - client._logger - ) - }) - - it('candidate removal emits removal events', done => { - const services = [ - { - ...MOCK_SERVICE, - ip: '[fd00:0:cafe:fefe::1]', - port: 31950, - local: true, - }, - MOCK_SERVICE, - ] - - const client = createDiscoveryClient({ services }) - - client.on('serviceRemoved', results => { - expect(results).toEqual(services) - done() - }) - - client.start() - client.remove('opentrons-dev') - }, 10) - - it('passes along mdns errors', () => { - return new Promise(resolve => { - const mockError = new Error('AH') - const client = createDiscoveryClient().once('error', error => { - expect(error).toEqual(mockError) - resolve() - }) - - client.start() - mdns.__mockBrowser.emit('error', mockError) - }) - }) - - it('can filter services by name', () => { - const client = createDiscoveryClient({ nameFilter: ['OPENTRONS'] }) - - client.start() - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - addresses: ['192.168.1.1'], - fullname: 'Opentrons-2._http._tcp.local', - }) - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - addresses: ['192.168.1.2'], - fullname: 'apentrons._http._tcp.local', - }) - - expect(client.services.map(s => s.name)).toEqual([ - 'opentrons-dev', - 'Opentrons-2', - ]) - }) - - it('can filter services by ip', () => { - const client = createDiscoveryClient({ ipFilter: ['169.254'] }) - - client.start() - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - addresses: ['169.254.1.2'], - }) - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - addresses: ['192.168.3.4'], - }) - - expect(client.services.map(s => s.ip)).toEqual(['169.254.1.2']) - }) - - it('can filter services by port', () => { - const client = createDiscoveryClient({ portFilter: [31950, 31951] }) - - client.start() - mdns.__mockBrowser.emit('update', MOCK_BROWSER_SERVICE) - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - fullname: '2._http._tcp.local', - addresses: ['192.168.1.1'], - port: 31951, - }) - mdns.__mockBrowser.emit('update', { - ...MOCK_BROWSER_SERVICE, - fullname: '3._http._tcp.local', - addresses: ['192.168.1.2'], - port: 22, - }) - - expect(client.services.map(s => s.port)).toEqual([31950, 31951]) - }) - - it('can add a candidate manually (with deduping)', () => { - const client = createDiscoveryClient() - const result = client.add('localhost').add('localhost') - - const expectedCandidates = [{ ip: 'localhost', port: 31950 }] - expect(result).toBe(client) - expect(client.candidates).toEqual(expectedCandidates) - expect(poller.poll).toHaveBeenLastCalledWith( - expectedCandidates, - expect.anything(), - expect.anything(), - client._logger - ) - }) - - it('can change polling interval on the fly', () => { - const client = createDiscoveryClient({ candidates: ['localhost'] }) - const expectedCandidates = [{ ip: 'localhost', port: 31950 }] - - const result = client.setPollInterval(1000) - expect(result).toBe(client) - expect(poller.poll).toHaveBeenLastCalledWith( - expectedCandidates, - 1000, - expect.anything(), - client._logger - ) - - client.setPollInterval(0) - expect(poller.poll).toHaveBeenLastCalledWith( - expectedCandidates, - 5000, - expect.anything(), - client._logger - ) - }) - - it.todo('periodically refreshes mDNS discovery') -}) diff --git a/discovery-client/src/__tests__/discovery-client.test.js b/discovery-client/src/__tests__/discovery-client.test.js index a1a0c30d773..aa048d0c792 100644 --- a/discovery-client/src/__tests__/discovery-client.test.js +++ b/discovery-client/src/__tests__/discovery-client.test.js @@ -1,15 +1,11 @@ // @flow import { last } from 'lodash' -import { - mockHealthResponse, - mockServerHealthResponse, -} from '../__fixtures__/health' - +import { mockHealthResponse, mockServerHealthResponse } from '../__fixtures__' import { HEALTH_STATUS_OK } from '../constants' import * as HealthPollerModule from '../health-poller' import * as MdnsBrowserModule from '../mdns-browser' -import { createDiscoveryClient } from '../discovery-client' +import { createDiscoveryClient } from '..' import type { HealthPoller, diff --git a/discovery-client/src/__tests__/health-poller.test.js b/discovery-client/src/__tests__/health-poller.test.js index 4670adc3f95..ce442575b78 100644 --- a/discovery-client/src/__tests__/health-poller.test.js +++ b/discovery-client/src/__tests__/health-poller.test.js @@ -2,7 +2,7 @@ import nodeFetch from 'node-fetch' import { isError } from 'lodash' -import * as Fixtures from '../__fixtures__/health' +import * as Fixtures from '../__fixtures__' import { createHealthPoller } from '../health-poller' import type { Request, RequestInit, Response } from 'node-fetch' diff --git a/discovery-client/src/__tests__/mdns-browser.test.js b/discovery-client/src/__tests__/mdns-browser.test.js index 971079030b2..5bab5f5fe86 100644 --- a/discovery-client/src/__tests__/mdns-browser.test.js +++ b/discovery-client/src/__tests__/mdns-browser.test.js @@ -2,7 +2,7 @@ import EventEmitter from 'events' import Mdns from 'mdns-js' -import { MOCK_BROWSER_SERVICE } from '../__fixtures__/mdns-browser-service' +import { mockBrowserService } from '../__fixtures__' import { createMdnsBrowser } from '../mdns-browser' import type { MdnsServiceType, Browser } from 'mdns-js' @@ -96,7 +96,7 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', MOCK_BROWSER_SERVICE) + baseBrowser.emit('update', mockBrowserService) expect(onService).toHaveBeenCalledWith({ name: 'opentrons-dev', @@ -110,7 +110,7 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, fullname: undefined }) + baseBrowser.emit('update', { ...mockBrowserService, fullname: undefined }) expect(onService).toHaveBeenCalledTimes(0) }) @@ -120,7 +120,7 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, port: undefined }) + baseBrowser.emit('update', { ...mockBrowserService, port: undefined }) expect(onService).toHaveBeenCalledTimes(0) }) @@ -130,7 +130,7 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, addresses: [] }) + baseBrowser.emit('update', { ...mockBrowserService, addresses: [] }) expect(onService).toHaveBeenCalledTimes(0) }) @@ -141,7 +141,7 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, addresses }) + baseBrowser.emit('update', { ...mockBrowserService, addresses }) expect(onService).toHaveBeenCalledWith({ name: 'opentrons-dev', @@ -155,8 +155,8 @@ describe('mdns browser', () => { browser.start() baseBrowser.emit('ready') - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE }) - baseBrowser.emit('update', { ...MOCK_BROWSER_SERVICE, port: 12345 }) + baseBrowser.emit('update', { ...mockBrowserService }) + baseBrowser.emit('update', { ...mockBrowserService, port: 12345 }) expect(onService).toHaveBeenCalledTimes(1) expect(onService).toHaveBeenCalledWith({ diff --git a/discovery-client/src/__tests__/poller.test.js b/discovery-client/src/__tests__/poller.test.js deleted file mode 100644 index f1e5a558ff7..00000000000 --- a/discovery-client/src/__tests__/poller.test.js +++ /dev/null @@ -1,172 +0,0 @@ -import fetch from 'node-fetch' -import { poll, stop } from '../poller' - -jest.mock('node-fetch') - -describe('discovery poller', () => { - beforeEach(() => { - jest.useFakeTimers() - fetch.__setMockResponse({ ok: false }) - }) - - afterEach(() => { - jest.clearAllTimers() - jest.useRealTimers() - fetch.__mockReset() - }) - - it('returns empty poll request if no candidates', () => { - const request = poll([], 5000, jest.fn()) - expect(request.id).toBe(null) - }) - - it('sets an interval to poll candidates evenly', () => { - poll( - [{ ip: 'foo', port: 31950 }, { ip: 'bar', port: 31950 }], - 6000, - jest.fn() - ) - - expect(setInterval).toHaveBeenCalledWith(expect.any(Function), 3000) - setInterval.mockClear() - - poll( - [ - { ip: 'foo', port: 31950 }, - { ip: 'bar', port: 31950 }, - { ip: 'baz', port: 31950 }, - ], - 6000, - jest.fn() - ) - - expect(setInterval).toHaveBeenCalledWith(expect.any(Function), 2000) - }) - - it('will not set a subinterval smaller than 100ms', () => { - poll( - [{ ip: 'foo', port: 31950 }, { ip: 'bar', port: 31950 }], - 42, - jest.fn() - ) - - expect(setInterval).toHaveBeenCalledWith(expect.any(Function), 100) - }) - - it('returns interval ID in request object', () => { - const intervalId = 1234 - setInterval.mockReturnValueOnce(intervalId) - - const request = poll( - [{ ip: 'foo', port: 31950 }, { ip: 'bar', port: 31950 }], - 6000, - jest.fn() - ) - - expect(request.id).toEqual(intervalId) - }) - - it('can stop polling', () => { - const request = { id: 1234 } - - stop(request) - expect(clearInterval).toHaveBeenCalledWith(1234) - }) - - it('calls fetch health for all candidates in an interval', () => { - poll( - [ - { ip: 'foo', port: 31950 }, - { ip: 'bar', port: 31950 }, - { ip: 'baz', port: 31950 }, - ], - 6000, - jest.fn() - ) - - jest.runTimersToTime(6000) - expect(fetch).toHaveBeenCalledTimes(6) - expect(fetch).toHaveBeenCalledWith(`http://foo:31950/health`, { - timeout: 30000, - }) - expect(fetch).toHaveBeenCalledWith(`http://bar:31950/health`, { - timeout: 30000, - }) - expect(fetch).toHaveBeenCalledWith(`http://baz:31950/health`, { - timeout: 30000, - }) - expect(fetch).toHaveBeenCalledWith( - `http://foo:31950/server/update/health`, - { timeout: 30000 } - ) - expect(fetch).toHaveBeenCalledWith( - `http://bar:31950/server/update/health`, - { timeout: 30000 } - ) - expect(fetch).toHaveBeenCalledWith( - `http://baz:31950/server/update/health`, - { timeout: 30000 } - ) - }) - - it('calls onHealth with health response if successful', done => { - fetch.__setMockResponse({ - ok: true, - json: () => Promise.resolve({ name: 'foo' }), - }) - - poll([{ ip: 'foo', port: 31950 }], 1000, (candidate, apiRes, serverRes) => { - expect(candidate).toEqual({ ip: 'foo', port: 31950 }) - expect(apiRes).toEqual({ name: 'foo' }) - expect(serverRes).toEqual({ name: 'foo' }) - done() - }) - - jest.runTimersToTime(1000) - }, 10) - - it('calls onHealth with null response if fetch not ok', done => { - fetch.__setMockResponse({ - ok: false, - json: () => Promise.resolve({ message: 'oh no!' }), - }) - - poll([{ ip: 'foo', port: 31950 }], 1000, (candidate, apiRes, serverRes) => { - expect(candidate).toEqual({ ip: 'foo', port: 31950 }) - expect(apiRes).toEqual(null) - expect(serverRes).toEqual(null) - done() - }) - - jest.runTimersToTime(1000) - }, 10) - - it('calls onHealth with null response if fetch rejects', done => { - fetch.__setMockError(new Error('failed to fetch')) - - poll([{ ip: 'foo', port: 31950 }], 1000, (candidate, apiRes, serverRes) => { - expect(candidate).toEqual({ ip: 'foo', port: 31950 }) - expect(apiRes).toEqual(null) - expect(serverRes).toEqual(null) - done() - }) - - jest.runTimersToTime(1000) - }, 10) - - it('calls onHealth with null response if JSON parse rejects', done => { - fetch.__setMockResponse({ - ok: true, - json: () => Promise.reject(new Error('oh no!')), - }) - - poll([{ ip: 'foo', port: 31950 }], 1000, (candidate, apiRes, serverRes) => { - expect(candidate).toEqual({ ip: 'foo', port: 31950 }) - expect(apiRes).toEqual(null) - expect(serverRes).toEqual(null) - done() - }) - - jest.runTimersToTime(1000) - }, 10) -}) diff --git a/discovery-client/src/__tests__/service-list.test.js b/discovery-client/src/__tests__/service-list.test.js deleted file mode 100644 index 83091f9f71e..00000000000 --- a/discovery-client/src/__tests__/service-list.test.js +++ /dev/null @@ -1,153 +0,0 @@ -import * as serviceList from '../service-list' -import { MOCK_SERVICE } from '../__fixtures__/service' - -describe('serviceList', () => { - describe('createServiceList', () => { - const SPECS = [ - { - name: 'returns empty array by default', - input: [], - expected: [], - }, - { - name: 'cleans up input list', - input: [ - { - ...MOCK_SERVICE, - name: 'foo', - ok: true, - serverOk: false, - health: { api_version: '1.0.0' }, - serverHealth: null, - }, - { - ...MOCK_SERVICE, - name: 'bar', - ok: false, - serverOk: true, - health: null, - serverHealth: { apiServerVersion: '1.0.0' }, - }, - { - ...MOCK_SERVICE, - name: 'baz', - ok: true, - serverOk: true, - health: { api_version: '1.0.0' }, - serverHealth: { apiServerVersion: '1.0.0' }, - }, - ], - expected: [ - { - ...MOCK_SERVICE, - name: 'foo', - health: { api_version: '1.0.0' }, - serverHealth: null, - }, - { - ...MOCK_SERVICE, - name: 'bar', - ip: null, - local: null, - health: null, - serverHealth: { apiServerVersion: '1.0.0' }, - }, - { - ...MOCK_SERVICE, - name: 'baz', - ip: null, - local: null, - health: { api_version: '1.0.0' }, - serverHealth: { apiServerVersion: '1.0.0' }, - }, - ], - }, - { - name: 'removes unknown IP entries if known IP entry exists', - input: [MOCK_SERVICE, { ...MOCK_SERVICE, ip: null }], - expected: [MOCK_SERVICE], - }, - { - name: 'collapses multiple IP unknown duplicates into one', - input: [ - { ...MOCK_SERVICE, ip: null, local: null }, - MOCK_SERVICE, - { ...MOCK_SERVICE, name: 'bar', ip: null, local: null }, - { ...MOCK_SERVICE, name: 'bar', ip: null, local: null }, - ], - expected: [ - MOCK_SERVICE, - { ...MOCK_SERVICE, name: 'bar', ip: null, local: null }, - ], - }, - ] - - SPECS.forEach(spec => { - const { name, input, expected } = spec - const result = serviceList.createServiceList.call(null, input) - it(name, () => expect(result).toEqual(expected)) - }) - }) - - describe('upsertServiceList', () => { - const SPECS = [ - { - name: 'adds service if new', - list: [], - upsert: MOCK_SERVICE, - expected: [MOCK_SERVICE], - }, - { - name: 'does not add existing service', - list: [MOCK_SERVICE], - upsert: MOCK_SERVICE, - expected: [MOCK_SERVICE], - }, - { - name: 'updates existing service', - list: [MOCK_SERVICE], - upsert: { ...MOCK_SERVICE, ok: true, serverOk: false }, - expected: [{ ...MOCK_SERVICE, ok: true, serverOk: false }], - }, - { - name: 'does not null out ok flags of existing service', - list: [{ ...MOCK_SERVICE, ok: true, serverOk: false }], - upsert: MOCK_SERVICE, - expected: [{ ...MOCK_SERVICE, ok: true, serverOk: false }], - }, - { - name: 'nulls out IP duplicates', - list: [ - { ...MOCK_SERVICE, name: 'bar', ok: false, serverOk: true }, - { ...MOCK_SERVICE, name: 'baz', ok: false, serverOk: true }, - ], - upsert: MOCK_SERVICE, - expected: [ - MOCK_SERVICE, - { - ...MOCK_SERVICE, - name: 'bar', - ip: null, - local: null, - ok: null, - serverOk: null, - }, - { - ...MOCK_SERVICE, - name: 'baz', - ip: null, - local: null, - ok: null, - serverOk: null, - }, - ], - }, - ] - - SPECS.forEach(spec => { - const { name, list, upsert, expected } = spec - const result = serviceList.upsertServiceList.call(null, list, upsert) - it(name, () => expect(result).toEqual(expected)) - }) - }) -}) diff --git a/discovery-client/src/__tests__/service.test.js b/discovery-client/src/__tests__/service.test.js deleted file mode 100644 index e76ed114f16..00000000000 --- a/discovery-client/src/__tests__/service.test.js +++ /dev/null @@ -1,250 +0,0 @@ -// tests for service and candidate creation utils -import { setIn } from '@thi.ng/paths' - -import * as service from '../service' -import { MOCK_BROWSER_SERVICE } from '../__fixtures__/mdns-browser-service' - -describe('service utils', () => { - describe('makeService', () => { - const SPECS = [ - { - name: 'all info specified', - args: [ - 'name', - 'ip', - 1234, - false, - false, - false, - { name: 'name' }, - { name: 'name' }, - ], - expected: { - name: 'name', - ip: 'ip', - port: 1234, - local: false, - ok: false, - serverOk: false, - advertising: false, - health: { name: 'name' }, - serverHealth: { name: 'name' }, - }, - }, - { - name: 'defaults', - args: ['name'], - expected: { - name: 'name', - ip: null, - port: 31950, - local: null, - ok: null, - serverOk: null, - advertising: null, - health: null, - serverHealth: null, - }, - }, - { - name: 'link-local IPv4', - args: ['name', '169.254.1.2'], - expected: expect.objectContaining({ local: true }), - }, - ] - - SPECS.forEach(spec => { - const { name, args, expected } = spec - it(name, () => - expect(service.makeService.apply(null, args)).toEqual(expected) - ) - }) - }) - - describe('makeCandidate', () => { - const SPECS = [ - { - name: 'all info specified', - args: ['ip', 1234], - expected: { ip: 'ip', port: 1234 }, - }, - { - name: 'defaults', - args: ['ip'], - expected: { ip: 'ip', port: 31950 }, - }, - ] - - SPECS.forEach(spec => { - const { name, args, expected } = spec - it(name, () => - expect(service.makeCandidate.apply(null, args)).toEqual(expected) - ) - }) - }) - - describe('fromMdnsBrowser', () => { - const SPECS = [ - { - name: 'returns null if no IP', - args: [setIn(MOCK_BROWSER_SERVICE, 'addresses', [])], - expected: null, - }, - { - name: 'gets name from fqdn and populates IP, port, and advertising', - args: [MOCK_BROWSER_SERVICE], - expected: { - name: 'opentrons-dev', - ip: MOCK_BROWSER_SERVICE.addresses[0], - port: MOCK_BROWSER_SERVICE.port, - local: false, - ok: null, - serverOk: null, - advertising: true, - health: null, - serverHealth: null, - }, - }, - { - name: 'prefers IPv4', - args: [ - setIn(MOCK_BROWSER_SERVICE, 'addresses', [ - 'fe80::caf4:6db4:4652:e975', - ...MOCK_BROWSER_SERVICE.addresses, - ]), - ], - expected: { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: null, - serverOk: null, - advertising: true, - health: null, - serverHealth: null, - }, - }, - { - name: 'adds brackets to IPv6', - args: [ - setIn(MOCK_BROWSER_SERVICE, 'addresses', [ - 'fe80::caf4:6db4:4652:e975', - ]), - ], - expected: { - name: 'opentrons-dev', - ip: '[fe80::caf4:6db4:4652:e975]', - port: 31950, - local: true, - ok: null, - serverOk: null, - advertising: true, - health: null, - serverHealth: null, - }, - }, - ] - - SPECS.forEach(spec => { - const { name, args, expected } = spec - it(name, () => - expect(service.fromMdnsBrowser.apply(null, args)).toEqual(expected) - ) - }) - }) - - describe('fromResponse', () => { - const MOCK_CANDIDATE = { ip: '192.168.1.42', port: 31950 } - const SPECS = [ - { - name: 'returns null if no responses', - args: [MOCK_CANDIDATE, null, null], - expected: null, - }, - { - name: 'gets ip from candidate and name from responses', - args: [ - MOCK_CANDIDATE, - { name: 'opentrons-dev', api_version: '1.0.0' }, - { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - ], - expected: { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: true, - serverOk: true, - advertising: null, - health: { name: 'opentrons-dev', api_version: '1.0.0' }, - serverHealth: { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - }, - }, - { - name: 'flags ok false if no /health response', - args: [ - MOCK_CANDIDATE, - null, - { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - ], - expected: { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: false, - serverOk: true, - advertising: null, - health: null, - serverHealth: { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - }, - }, - { - name: 'flags serverOk false if no /server/update/health response', - args: [ - MOCK_CANDIDATE, - { name: 'opentrons-dev', api_version: '1.0.0' }, - null, - ], - expected: { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: true, - serverOk: false, - advertising: null, - health: { name: 'opentrons-dev', api_version: '1.0.0' }, - serverHealth: null, - }, - }, - { - name: 'flags ok false if name mismatch in responses', - args: [ - MOCK_CANDIDATE, - { name: 'opentrons-wrong', api_version: '1.0.0' }, - { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - ], - expected: { - name: 'opentrons-dev', - ip: '192.168.1.42', - port: 31950, - local: false, - ok: false, - serverOk: true, - advertising: null, - health: { name: 'opentrons-wrong', api_version: '1.0.0' }, - serverHealth: { name: 'opentrons-dev', apiServerVersion: '1.0.0' }, - }, - }, - ] - - SPECS.forEach(spec => { - const { name, args, expected } = spec - it(name, () => - expect(service.fromResponse.apply(null, args)).toEqual(expected) - ) - }) - }) -}) diff --git a/discovery-client/src/cli.js b/discovery-client/src/cli.js index 258f72a07fc..3354ea8fb38 100755 --- a/discovery-client/src/cli.js +++ b/discovery-client/src/cli.js @@ -1,13 +1,13 @@ // @flow import Yargs from 'yargs' import noop from 'lodash/noop' -import { createDiscoveryClient, DEFAULT_PORT } from './discovery-client' +import { createDiscoveryClient, DEFAULT_PORT } from '.' import { version } from '../package.json' import type { Argv as YargsArgv } from 'yargs' import type { - DiscoveryClientNext, + DiscoveryClient, DiscoveryClientRobot, DiscoveryClientRobotAddress, LogLevel, @@ -73,7 +73,7 @@ const passesFilters = (argv: Argv) => (robot: DiscoveryClientRobot) => { const createClient = ( argv: Argv, onListChange: (robots: $ReadOnlyArray) => mixed -): DiscoveryClientNext => { +): DiscoveryClient => { const logger = createLogger(argv) const { pollInterval, candidates } = argv const client = createDiscoveryClient({ diff --git a/discovery-client/src/constants.js b/discovery-client/src/constants.js index d5c8c7d969c..d0a35064bc1 100644 --- a/discovery-client/src/constants.js +++ b/discovery-client/src/constants.js @@ -11,3 +11,10 @@ export const UPDATE_SERVER_HEALTH_PATH = '/server/update/health' // mdns service filters export const DEFAULT_PORT = 31950 + +// hostname matchers +// ipv6 matcher includes square bracket for backwards compatibility +export const RE_HOSTNAME_IPV6_LL: RegExp = /^\[?(?:fd00|fe80)/ +export const RE_HOSTNAME_IPV4_LL: RegExp = /^169\.254\.\d+\.\d+$/ +export const RE_HOSTNAME_LOCALHOST: RegExp = /^localhost$/ +export const RE_HOSTNAME_LOOPBACK: RegExp = /^127\.0\.0\.1$/ diff --git a/discovery-client/src/discovery-client.js b/discovery-client/src/discovery-client.js index b4966efc52a..14af8c49950 100644 --- a/discovery-client/src/discovery-client.js +++ b/discovery-client/src/discovery-client.js @@ -6,16 +6,14 @@ import { createMdnsBrowser } from './mdns-browser' import * as Store from './store' import type { - DiscoveryClientNext, + DiscoveryClient, DiscoveryClientConfig, DiscoveryClientOptions, } from './types' -export { DEFAULT_PORT } - export function createDiscoveryClient( options: DiscoveryClientOptions -): DiscoveryClientNext { +): DiscoveryClient { const { onListChange, logger } = options const { getState, dispatch, subscribe } = Store.createStore() const getAddresses = () => Store.getAddresses(getState()) diff --git a/discovery-client/src/index.js b/discovery-client/src/index.js index 8383446cd57..9c82ddde8c4 100644 --- a/discovery-client/src/index.js +++ b/discovery-client/src/index.js @@ -1,290 +1,6 @@ // @flow -// opentrons robot service discovery client -// finds robots on the network via mdns -import EventEmitter from 'events' -import escape from 'escape-string-regexp' -import toRegex from 'to-regex' -import differenceBy from 'lodash/differenceBy' -import xorBy from 'lodash/xorBy' +export { createDiscoveryClient } from './discovery-client' +export * from './constants' -import { createMdnsBrowserLegacy, getKnownIps } from './mdns-browser' -import { poll, stop, type PollRequest } from './poller' -import { - createServiceList, - upsertServiceList, - updateServiceListByIp, -} from './service-list' - -import { - DEFAULT_PORT, - updateService, - fromMdnsBrowser, - fromResponse, - makeCandidate, - toCandidate, -} from './service' - -import type { Browser, BrowserService } from 'mdns-js' - -import type { - Candidate, - Service, - ServiceList, - HealthResponse, - ServerHealthResponse, - LogLevel, - Logger, -} from './types' - -export * from './types' - -type Options = { - pollInterval?: number, - services?: Array, - candidates?: Array, - nameFilter?: Array, - ipFilter?: Array, - portFilter?: Array, - logger?: Logger, -} - -const log = (logger: ?Logger, level: LogLevel, msg: string, meta?: {}) => - logger && typeof logger[level] === 'function' && logger[level](msg, meta) - -const santizeRe = (patterns: ?Array) => { - if (!patterns) return [] - return patterns.map(p => (typeof p === 'string' ? escape(p) : p)) -} - -export const SERVICE_EVENT: 'service' = 'service' -export const SERVICE_REMOVED_EVENT: 'serviceRemoved' = 'serviceRemoved' -export const DEFAULT_POLL_INTERVAL = 5000 -export const DEFAULT_DISCOVERY_INTERVAL = 90000 -export { DEFAULT_PORT } - -const TO_REGEX_OPTS = { contains: true, nocase: true, safe: true } - -export class DiscoveryClient extends EventEmitter { - services: ServiceList - candidates: Array - _browser: ?Browser - _discoveryInterval: IntervalID - _pollList: Array - _pollInterval: number - _pollRequest: ?PollRequest - _nameFilter: RegExp - _ipFilter: RegExp - _portFilter: Array - _logger: ?Logger - - constructor(options: Options) { - super() - - // null out ok flag for pre-populated services - this.services = createServiceList(options.services || []) - - // allow strings instead of full {ip: string, port: ?number} object - this.candidates = (options.candidates || []) - .map(c => (typeof c === 'string' ? makeCandidate(c) : c)) - .filter(c => this.services.every(s => s.ip !== c.ip)) - - this._browser = null - this._pollList = [] - this._pollInterval = options.pollInterval || DEFAULT_POLL_INTERVAL - this._pollRequest = null - this._nameFilter = toRegex(santizeRe(options.nameFilter), TO_REGEX_OPTS) - this._ipFilter = toRegex(santizeRe(options.ipFilter), TO_REGEX_OPTS) - this._portFilter = [DEFAULT_PORT].concat(options.portFilter || []) - this._logger = options.logger - - log(this._logger, 'silly', 'Created', this) - } - - start(): DiscoveryClient { - log(this._logger, 'debug', 'starting discovery client', {}) - this._startBrowser() - this._poll() - this._discoveryInterval = setInterval( - this._rediscover.bind(this), - DEFAULT_DISCOVERY_INTERVAL - ) - - return this - } - - stop(): DiscoveryClient { - log(this._logger, 'debug', 'stopping discovery client', {}) - this._stopBrowser() - this._stopPoll() - clearInterval(this._discoveryInterval) - - return this - } - - add(ip: string, port?: number): DiscoveryClient { - if (!this.candidates.some(c => c.ip === ip)) { - const candidate = makeCandidate(ip, port) - log(this._logger, 'debug', 'adding new unique candidate', { candidate }) - this.candidates = this.candidates.concat(candidate) - this._poll() - } - - return this - } - - remove(name: string): DiscoveryClient { - const removals = this.services.filter(s => s.name === name) - - this.services = this.services.filter(s => s.name !== name) - this.candidates = this.candidates.filter(c => - removals.every(s => s.ip !== c.ip) - ) - - log(this._logger, 'debug', 'removed services from discovery', { removals }) - this._poll() - this.emit(SERVICE_REMOVED_EVENT, removals) - - return this - } - - setCandidates(candidates: Array): DiscoveryClient { - this.candidates = candidates.map(c => { - if (typeof c === 'string') { - return makeCandidate(c) - } else { - return c - } - }) - this._poll(true) - return this - } - - setPollInterval(interval: number): DiscoveryClient { - this._pollInterval = interval || DEFAULT_POLL_INTERVAL - this._poll(true) - - return this - } - - _poll(forceRestart?: boolean): void { - const nextPollList = this.services - .map(toCandidate) - .filter(Boolean) - .concat(this.candidates) - - // only poll if needed - if (forceRestart || xorBy(this._pollList, nextPollList, 'ip').length) { - log(this._logger, 'debug', '(re)starting polling', {}) - - this._pollList = nextPollList - stop(this._pollRequest, this._logger) - this._pollRequest = poll( - nextPollList, - this._pollInterval, - this._handleHealth.bind(this), - this._logger - ) - } - } - - _stopPoll(): void { - stop(this._pollRequest, this._logger) - this._pollRequest = null - } - - _startBrowser(): void { - this._stopBrowser() - - const browser = createMdnsBrowserLegacy() - .once('ready', () => browser.discover()) - .on('update', service => this._handleUp(service)) - .on('error', error => this.emit('error', error)) - - this._browser = browser - } - - _stopBrowser(): void { - if (this._browser) { - this._browser - .removeAllListeners('ready') - .removeAllListeners('update') - .removeAllListeners('error') - .stop() - - this._browser = null - } - } - - _rediscover(): void { - const knownIps = getKnownIps(this._browser) - log(this._logger, 'silly', 'refreshing advertising flags', { knownIps }) - - const nextServices = this.services.map(s => - updateService(s, { - advertising: knownIps.includes(s.ip), - }) - ) - - this._updateLists(nextServices) - this._stopBrowser() - this._startBrowser() - } - - _handleUp(browserService: BrowserService): void { - log(this._logger, 'silly', 'mdns service detected', { browserService }) - const service = fromMdnsBrowser(browserService) - - if (service) this._handleService(service) - } - - _handleHealth( - candidate: Candidate, - apiResponse: ?HealthResponse, - serverResponse: ?ServerHealthResponse - ): mixed { - const service = fromResponse(candidate, apiResponse, serverResponse) - - if (service) return this._handleService(service) - - // else, response was not ok, so unset ok flag in all matching ips - this._updateLists( - updateServiceListByIp(this.services, candidate.ip, { - ok: false, - serverOk: false, - }) - ) - } - - _handleService(service: Service): mixed { - if ( - !this._nameFilter.test(service.name) || - !this._ipFilter.test(service.ip || '') || - !this._portFilter.includes(service.port) - ) { - log(this._logger, 'debug', 'Ignoring service', service) - return - } - - this._updateLists(upsertServiceList(this.services, service)) - } - - // update this.services, emit if necessary, re-poll if necessary - _updateLists(nextServices: ServiceList): void { - const updated = differenceBy(nextServices, this.services) - - if (updated.length) { - // $FlowFixMe: flow doesn't type differenceBy properly, but this works - this.candidates = differenceBy(this.candidates, nextServices, 'ip') - this.services = nextServices - this._poll() - - log(this._logger, 'silly', 'updated services', { updated }) - this.emit(SERVICE_EVENT, updated) - } - } -} - -export function createDiscoveryClient(options?: Options): DiscoveryClient { - return new DiscoveryClient(options || {}) -} +export type * from './types' diff --git a/discovery-client/src/mdns-browser.js b/discovery-client/src/mdns-browser.js index c6cf73218f4..a8dfad20322 100644 --- a/discovery-client/src/mdns-browser.js +++ b/discovery-client/src/mdns-browser.js @@ -54,7 +54,7 @@ export function createMdnsBrowser(options: MdnsBrowserOptions): MdnsBrowser { const name = fullname.replace(/\._http\._tcp.local$/, '') onService({ name, ip, port }) } else { - log('debug', 'Ignoring mDNS service', { service }) + log('silly', 'Ignoring mDNS service', { service }) } }) .on('error', (e: Error) => { diff --git a/discovery-client/src/poller.js b/discovery-client/src/poller.js deleted file mode 100644 index 41e7453abe6..00000000000 --- a/discovery-client/src/poller.js +++ /dev/null @@ -1,95 +0,0 @@ -// @flow -// poller that continuously hits /health of a list of IP ipAddresses - -import fetch from 'node-fetch' - -import type { - Candidate, - Logger, - HealthResponse, - ServerHealthResponse, -} from './types' - -type HealthHandler = ( - candidate: Candidate, - apiResponse: ?HealthResponse, - serverResponse: ?ServerHealthResponse -) => mixed - -export type PollRequest = { - id: ?IntervalID, -} - -const MIN_SUBINTERVAL_MS = 100 -const MIN_TIMEOUT_MS = 30000 - -export function poll( - candidates: Array, - interval: number, - onHealth: HealthHandler, - log: ?Logger -): PollRequest { - if (!candidates.length) return { id: null } - - log && log.debug('poller start', { interval, candidates: candidates.length }) - - const subInterval = Math.max(interval / candidates.length, MIN_SUBINTERVAL_MS) - const timeout = Math.max(subInterval * candidates.length, MIN_TIMEOUT_MS) - - const id = setInterval(pollIp, subInterval) - const request = { id } - let current = -1 - - return request - - function pollIp() { - const next = getNextCandidate() - - fetchHealth(next, timeout, log).then(([apiRes, serverRes]) => - onHealth(next, apiRes, serverRes) - ) - } - - function getNextCandidate() { - current += 1 - - if (current > candidates.length - 1) { - current = 0 - } - - return candidates[current] - } -} - -export function stop(request: ?PollRequest, log: ?Logger) { - const id = request && request.id - - if (id) { - clearInterval(id) - log && log.debug('poller stop', { id }) - } -} - -function fetchHealth(cand: Candidate, timeout: number, log: ?Logger) { - const apiHealthUrl = `http://${cand.ip}:${cand.port}/health` - const serverHealthUrl = `http://${cand.ip}:${cand.port}/server/update/health` - - return Promise.all([ - fetchAndParseBody(apiHealthUrl, timeout, log), - fetchAndParseBody(serverHealthUrl, timeout, log), - ]) -} - -function fetchAndParseBody(url, timeout, log: ?Logger) { - return fetch(url, { timeout }) - .then(response => (response.ok ? response.json() : null)) - .then(body => { - log && log.silly('GET', { url, body }) - return body - }) - .catch(error => { - const { message, type, code } = error - log && log.silly('GET failed', { url, message, type, code }) - return null - }) -} diff --git a/discovery-client/src/service-list.js b/discovery-client/src/service-list.js deleted file mode 100644 index b015bcafa26..00000000000 --- a/discovery-client/src/service-list.js +++ /dev/null @@ -1,114 +0,0 @@ -// @flow -import differenceBy from 'lodash/differenceBy' -import partition from 'lodash/partition' -import uniqBy from 'lodash/uniqBy' -import stableSort from 'stable' - -import { - makeService, - updateService, - clearServiceIfConflict, - matchService, -} from './service' - -import type { Service, ServiceList, ServiceUpdate } from './types' - -export function createServiceList(list: ServiceList = []): ServiceList { - // strip health flags from input list - const nextList = list.map(s => - makeService( - s.name, - s.ip, - s.port, - null, - null, - null, - s.health, - s.serverHealth - ) - ) - - return dedupeServices(nextList) -} - -export function upsertServiceList( - list: ServiceList, - upsert: Service -): ServiceList { - const previous: ?Service = list.find(matchService(upsert)) - let nextList = list - if (!previous) nextList = nextList.concat(upsert) - - nextList = nextList.map(service => { - // don't do anything if this is the added entry - if (service === upsert) return service - // else update previous entry if it exists - if (service === previous) return updateService(service, upsert) - // else return the service, clearing flags if it conflicts with the update - return clearServiceIfConflict(service, upsert) - }) - - return dedupeServices(nextList) -} - -export function updateServiceListByIp( - list: ServiceList, - ip: string, - update: ServiceUpdate -): ServiceList { - const nextList = list.map(service => - service.ip === ip ? updateService(service, update) : service - ) - - return dedupeServices(nextList) -} - -// ensure there aren't multiple entries with the same IP and there aren't -// multiple entries with the same name and ip: null -function dedupeServices(list: ServiceList) { - const [listWithIp, listWithoutIp] = partition(list, 'ip') - const sanitizedWithIp = listWithIp.reduce( - (result, service) => { - // we know IP exists here thanks to our partition above - const ip: string = (service.ip: any) - const cleanedService = result.seenIps[ip] - ? clearServiceIfConflict(service, { ip }) - : service - - result.seenIps[ip] = true - result.unique.push(cleanedService) - - return result - }, - { unique: [], seenIps: {} } - ).unique - - const dedupedWithoutIp = differenceBy( - uniqBy(listWithoutIp, 'name'), - sanitizedWithIp, - 'name' - ) - - // use a stable sort because core-js polyfills can mess with Array.sort order - return stableSort(sanitizedWithIp.concat(dedupedWithoutIp), compareServices) -} - -// sort service list by: -// 1. ip exists, -// 2. update server healthy -// 3. API healthy -// 4. link-local address -// 5. advertising -function compareServices(a: Service, b: Service) { - if (a.ip && !b.ip) return -1 - if (!a.ip && b.ip) return 1 - if (a.serverOk && !b.serverOk) return -1 - if (!a.serverOk && b.serverOk) return 1 - if (a.ok && !b.ok) return -1 - if (!a.ok && b.ok) return 1 - if (a.local && !b.local) return -1 - if (!a.local && b.local) return 1 - if (a.advertising && !b.advertising) return -1 - if (!a.advertising && b.advertising) return 1 - return 0 -} diff --git a/discovery-client/src/service.js b/discovery-client/src/service.js deleted file mode 100644 index 9935c643c91..00000000000 --- a/discovery-client/src/service.js +++ /dev/null @@ -1,136 +0,0 @@ -// @flow -// create Services from different sources -import net from 'net' -import defaultTo from 'lodash/defaultTo' -import isEqual from 'lodash/isEqual' - -import type { BrowserService, ServiceType } from 'mdns-js' -import type { - Service, - ServiceUpdate, - Candidate, - HealthResponse, - ServerHealthResponse, -} from './types' - -const nameExtractor = (st: ServiceType) => - new RegExp(`^(.+)\\._${st.name}\\._${st.protocol}`) - -const isLocal = (ip: ?string) => { - if (ip == null) return null - return ( - ip.startsWith('169.254') || - ip.startsWith('[fe80') || - ip.startsWith('[fd00') || - ip === 'localhost' - ) -} -export const DEFAULT_PORT = 31950 - -export function makeService( - name: string, - ip: ?string, - port: ?number, - ok: ?boolean, - serverOk: ?boolean, - advertising: ?boolean, - health: ?HealthResponse, - serverHealth: ?ServerHealthResponse -): Service { - return { - name, - ip: defaultTo(ip, null), - port: defaultTo(port, DEFAULT_PORT), - local: isLocal(ip), - ok: defaultTo(ok, null), - serverOk: defaultTo(serverOk, null), - advertising: defaultTo(advertising, null), - health: health || null, - serverHealth: serverHealth || null, - } -} - -// apply known value updates (not null or undefined) to a service, returning -// original service reference if nothing to update -export function updateService( - service: Service, - update: ServiceUpdate -): Service { - const next: Service | ServiceUpdate = update - - return Object.keys(update).reduce((result: Service, key: string) => { - const prevVal = result[key] - const nextVal = defaultTo(next[key], prevVal) - // use isEqual to deep compare response objects - return isEqual(nextVal, prevVal) ? result : { ...result, [key]: nextVal } - }, service) -} - -// null out conflicting fields -export function clearServiceIfConflict( - service: Service, - update: ?ServiceUpdate -): Service { - return update && service.ip === update.ip - ? { ...service, ip: null, local: null, ok: null, serverOk: null } - : service -} - -export function makeCandidate(ip: string, port: ?number): Candidate { - return { ip, port: port || DEFAULT_PORT } -} - -export function fromMdnsBrowser(browserService: BrowserService): ?Service { - const { addresses, type, port, fullname } = browserService - - if (!type || !fullname) return null - - let ip = addresses.find(address => net.isIPv4(address)) - if (!ip) ip = addresses.find(address => net.isIP(address)) - if (!ip) return null - - if (net.isIPv6(ip)) ip = `[${ip}]` - - const nameMatch = type[0] && fullname.match(nameExtractor(type[0])) - const name = (nameMatch && nameMatch[1]) || fullname - - return makeService(name, ip, port, null, null, true) -} - -export function fromResponse( - candidate: Candidate, - healthResponse: ?HealthResponse, - serverHealthResponse: ?ServerHealthResponse -): ?Service { - const apiName = healthResponse && healthResponse.name - const serverName = serverHealthResponse && serverHealthResponse.name - const name = defaultTo(serverName, apiName) - let apiOk = !!healthResponse - - if (!name) return null - - // in case of name mismatch, prefer /server/update/health name and flag not ok - if (apiName != null && serverName != null && apiName !== serverName) { - apiOk = false - } - - return makeService( - name, - candidate.ip, - candidate.port, - apiOk, - !!serverHealthResponse, - null, - healthResponse, - serverHealthResponse - ) -} - -export function toCandidate(service: Service): ?Candidate { - if (!service.ip) return null - - return makeCandidate(service.ip, service.port) -} - -export const matchService: Service => Service => boolean = source => target => - source.name === target.name && source.ip === target.ip diff --git a/discovery-client/src/store/__tests__/robotsByNameReducer.test.js b/discovery-client/src/store/__tests__/robotsByNameReducer.test.js index fb2d312adc2..56531bcd76e 100644 --- a/discovery-client/src/store/__tests__/robotsByNameReducer.test.js +++ b/discovery-client/src/store/__tests__/robotsByNameReducer.test.js @@ -139,7 +139,7 @@ describe('robotsByName reducer', () => { }) }) - it('should handle a good "http:HEALTH_POLLED action for an existing robot', () => { + it('should handle a good "http:HEALTH_POLLED action for an existing robot with unknown health', () => { const action = Actions.healthPolled({ ip: '127.0.0.1', port: 31950, @@ -243,6 +243,36 @@ describe('robotsByName reducer', () => { }) }) + it('should update health with a good health poll', () => { + const action = Actions.healthPolled({ + ip: '127.0.0.1', + port: 31950, + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + healthError: null, + serverHealthError: null, + }) + const initialState = { + 'opentrons-dev': { + name: 'opentrons-dev', + health: { ...mockHealthResponse, api_version: '0.0.0' }, + serverHealth: { + ...mockServerHealthResponse, + apiServerVersion: '0.0.0', + }, + }, + } + const nextState = robotsByNameReducer(initialState, action) + + expect(nextState).toEqual({ + 'opentrons-dev': { + name: 'opentrons-dev', + health: mockHealthResponse, + serverHealth: mockServerHealthResponse, + }, + }) + }) + it('should not update state if new poll results are deep equal', () => { const action = Actions.healthPolled({ ip: '127.0.0.1', diff --git a/discovery-client/src/store/__tests__/selectors.test.js b/discovery-client/src/store/__tests__/selectors.test.js index 0a8da8cc39f..2b28b3495df 100644 --- a/discovery-client/src/store/__tests__/selectors.test.js +++ b/discovery-client/src/store/__tests__/selectors.test.js @@ -248,7 +248,7 @@ describe('discovery client state selectors', () => { expect(result).toEqual([ok, notOk, unreachable, unknown]) }) - it('prefer more local "ip" addresses', () => { + it('should prefer more local "ip" addresses', () => { const home: $Shape = { ip: '127.0.0.1', healthStatus: HEALTH_STATUS_OK, @@ -261,20 +261,51 @@ describe('discovery client state selectors', () => { serverHealthStatus: HEALTH_STATUS_OK, } - const linkLocal: $Shape = { + const linkLocalV4: $Shape = { ip: '169.254.24.42', healthStatus: HEALTH_STATUS_OK, serverHealthStatus: HEALTH_STATUS_OK, } + const linkLocalV6: $Shape = { + ip: 'fd00:0:cafe:fefe::1', + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + } + const regular: $Shape = { ip: '192.168.1.100', healthStatus: HEALTH_STATUS_OK, serverHealthStatus: HEALTH_STATUS_OK, } - const result = sort([regular, linkLocal, localhost, home]) - expect(result).toEqual([home, localhost, linkLocal, regular]) + const result = sort([regular, linkLocalV6, linkLocalV4, localhost, home]) + expect(result).toEqual([ + home, + localhost, + linkLocalV4, + linkLocalV6, + regular, + ]) + }) + + it('should prefer more seen "ip" addresses', () => { + const unseen: $Shape = { + ip: '192.168.1.1', + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + seen: false, + } + + const seen: $Shape = { + ip: '192.168.1.2', + healthStatus: HEALTH_STATUS_OK, + serverHealthStatus: HEALTH_STATUS_OK, + seen: true, + } + + const result = sort([unseen, seen]) + expect(result).toEqual([seen, unseen]) }) }) }) diff --git a/discovery-client/src/store/reducer.js b/discovery-client/src/store/reducer.js index 757869ad511..3a0a2384c1e 100644 --- a/discovery-client/src/store/reducer.js +++ b/discovery-client/src/store/reducer.js @@ -83,12 +83,10 @@ export const robotsByNameReducer = ( const name = serverHealth?.name ?? health?.name ?? null if (!name) return state - const nextHealth = state[name]?.health ?? health - const nextServerHealth = state[name]?.serverHealth ?? serverHealth const nextRobotState = { name, - health: nextHealth, - serverHealth: nextServerHealth, + health: health ?? state[name]?.health ?? null, + serverHealth: serverHealth ?? state[name]?.serverHealth ?? null, } return isEqual(state[name], nextRobotState) diff --git a/discovery-client/src/store/selectors.js b/discovery-client/src/store/selectors.js index 110eb349ffa..ae9a19bb5d3 100644 --- a/discovery-client/src/store/selectors.js +++ b/discovery-client/src/store/selectors.js @@ -5,6 +5,10 @@ import { HEALTH_STATUS_OK, HEALTH_STATUS_NOT_OK, HEALTH_STATUS_UNREACHABLE, + RE_HOSTNAME_IPV6_LL, + RE_HOSTNAME_IPV4_LL, + RE_HOSTNAME_LOCALHOST, + RE_HOSTNAME_LOOPBACK, } from '../constants' import type { DiscoveryClientRobot } from '../types' @@ -50,13 +54,17 @@ const HEALTH_PRIORITY = [ HEALTH_STATUS_OK, ] +const SEEN_PRIORITY = [false, true] + // accending priority order, where no match is lowest priority const IP_PRIORITY_MATCH = [ - /^169\.254\.\d+\.\d+$/, - /^localhost$/, - /^127\.0\.0\.1$/, + RE_HOSTNAME_IPV6_LL, + RE_HOSTNAME_IPV4_LL, + RE_HOSTNAME_LOCALHOST, + RE_HOSTNAME_LOOPBACK, ] +// compare hosts in decending priority order export const compareHostsByConnectability = ( a: HostState, b: HostState @@ -73,6 +81,10 @@ export const compareHostsByConnectability = ( if (serverHealthSort !== 0) return serverHealthSort + const seenSort = SEEN_PRIORITY.indexOf(b.seen) - SEEN_PRIORITY.indexOf(a.seen) + + if (seenSort !== 0) return seenSort + const aIpPriority = IP_PRIORITY_MATCH.findIndex(re => re.test(a.ip)) const bIpPriority = IP_PRIORITY_MATCH.findIndex(re => re.test(b.ip)) const ipSort = bIpPriority - aIpPriority diff --git a/discovery-client/src/types.js b/discovery-client/src/types.js index 3034e8a8131..24d192a389d 100644 --- a/discovery-client/src/types.js +++ b/discovery-client/src/types.js @@ -1,6 +1,13 @@ // @flow -import type { RobotState, HostState, Address } from './store/types' +import type { + RobotState, + HostState, + HealthStatus, + Address, +} from './store/types' + +export type { RobotState, HostState, HealthStatus } // TODO(mc, 2018-10-03): figure out what to do with duplicate type in app export type HealthResponse = { @@ -40,35 +47,6 @@ export type HealthErrorResponse = {| body: string | { [string]: mixed, ... }, |} -export type Candidate = { - ip: string, - port: number, - ... -} - -export type Service = { - name: string, - ip: ?string, - port: number, - // IP address (if known) is a link-local address - local: ?boolean, - // GET /health response.ok === true - ok: ?boolean, - // GET /server/update/health response.ok === true - serverOk: ?boolean, - // is advertising on MDNS - advertising: ?boolean, - // last good /health response - health: ?HealthResponse, - // last good /server/update/health response - serverHealth: ?ServerHealthResponse, - ... -} - -export type ServiceUpdate = $Shape - -export type ServiceList = Array - // TODO(mc, 2018-07-26): grab common logger type from app and app-shell export type LogLevel = | 'error' @@ -225,9 +203,31 @@ export type DiscoveryClientOptions = $ReadOnly<{| logger?: Logger, |}> -export type DiscoveryClientNext = $ReadOnly<{| +export type DiscoveryClient = $ReadOnly<{| getRobots: () => $ReadOnlyArray, removeRobot: (robotName: string) => void, start: (config: DiscoveryClientConfig) => void, stop: () => void, |}> + +/** + * Legacy type used in previous version of Discovery Client for robot state + */ +export type LegacyService = { + name: string, + ip: ?string, + port: number, + // IP address (if known) is a link-local address + local: ?boolean, + // GET /health response.ok === true + ok: ?boolean, + // GET /server/update/health response.ok === true + serverOk: ?boolean, + // is advertising on MDNS + advertising: ?boolean, + // last good /health response + health: ?HealthResponse, + // last good /server/update/health response + serverHealth: ?ServerHealthResponse, + ... +} diff --git a/yarn.lock b/yarn.lock index f97bd3a0395..6ca2c3028a4 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10128,6 +10128,11 @@ ip-regex@^2.1.0: version "2.1.0" resolved "https://registry.yarnpkg.com/ip-regex/-/ip-regex-2.1.0.tgz#fa78bf5d2e6913c911ce9f819ee5146bb6d844e9" +ip-regex@^4.0.0: + version "4.1.0" + resolved "https://registry.yarnpkg.com/ip-regex/-/ip-regex-4.1.0.tgz#5ad62f685a14edb421abebc2fff8db94df67b455" + integrity sha512-pKnZpbgCTfH/1NLIlOduP/V+WRXzC2MOz3Qo8xmxk8C5GudJLgK5QyLVXOSWy3ParAH7Eemurl3xjv/WXYFvMA== + ip@^1.1.0, ip@^1.1.5: version "1.1.5" resolved "https://registry.yarnpkg.com/ip/-/ip-1.1.5.tgz#bdded70114290828c0a039e72ef25f5aaec4354a" @@ -10388,6 +10393,13 @@ is-interactive@^1.0.0: resolved "https://registry.yarnpkg.com/is-interactive/-/is-interactive-1.0.0.tgz#cea6e6ae5c870a7b0a0004070b7b587e0252912e" integrity sha512-2HvIEKRoqS62guEC+qBjpvRubdX910WCMuJTZ+I9yvqKU2/12eSL549HMwtabb4oupdj2sMP50k+XJfB/8JE6w== +is-ip@3.1.0: + version "3.1.0" + resolved "https://registry.yarnpkg.com/is-ip/-/is-ip-3.1.0.tgz#2ae5ddfafaf05cb8008a62093cf29734f657c5d8" + integrity sha512-35vd5necO7IitFPjd/YBeqwWnyDWbuLH9ZXQdMfDA8TEo7pv5X8yfrvVO3xbJbLUlERCMvf6X0hTUamQxCYJ9Q== + dependencies: + ip-regex "^4.0.0" + is-nan@^1.2.1: version "1.2.1" resolved "https://registry.yarnpkg.com/is-nan/-/is-nan-1.2.1.tgz#9faf65b6fb6db24b7f5c0628475ea71f988401e2"