From 39601473c063d63feea6388d38500f33b7135829 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 5 May 2021 12:34:48 +0900 Subject: [PATCH 1/4] Fix undervoltage regex, add undervoltage tests, move sysinfo suite to test/src Signed-off-by: Christina Wang --- src/lib/system-info.ts | 4 +- test/data/foo.json | 1 - test/data/serial-number | Bin 17 -> 0 bytes .../lib/system-info.spec.ts} | 120 ++++++++++-------- 4 files changed, 69 insertions(+), 56 deletions(-) delete mode 100644 test/data/foo.json delete mode 100644 test/data/serial-number rename test/{38-sys-info.spec.ts => src/lib/system-info.spec.ts} (73%) diff --git a/src/lib/system-info.ts b/src/lib/system-info.ts index b9635e07..b9fef9d0 100644 --- a/src/lib/system-info.ts +++ b/src/lib/system-info.ts @@ -77,7 +77,7 @@ export async function getCpuId(): Promise { } } -const undervoltageRegex = /under.*voltage/; +const undervoltageRegex = /under.*voltage/i; export async function undervoltageDetected(): Promise { try { const { stdout: dmesgStdout } = await exec('dmesg'); @@ -138,6 +138,6 @@ export function isSignificantChange( return Math.floor(current / bucketSize) !== Math.floor(past / bucketSize); } -function bytesToMb(bytes: number) { +export function bytesToMb(bytes: number) { return Math.floor(bytes / 1024 / 1024); } diff --git a/test/data/foo.json b/test/data/foo.json deleted file mode 100644 index 6d959030..00000000 --- a/test/data/foo.json +++ /dev/null @@ -1 +0,0 @@ -{"foo": "bar"} diff --git a/test/data/serial-number b/test/data/serial-number deleted file mode 100644 index b41850c542eee12a2979d26871564dfb6877fd47..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 17 ScmXpsfC9rLOXD=-GzI`4ngg%^ diff --git a/test/38-sys-info.spec.ts b/test/src/lib/system-info.spec.ts similarity index 73% rename from test/38-sys-info.spec.ts rename to test/src/lib/system-info.spec.ts index 3836c9c9..96fe0836 100644 --- a/test/38-sys-info.spec.ts +++ b/test/src/lib/system-info.spec.ts @@ -1,28 +1,32 @@ import { expect } from 'chai'; -import { stub } from 'sinon'; -import * as systeminformation from 'systeminformation'; +import { SinonStub, stub } from 'sinon'; import { promises as fs } from 'fs'; +import * as systeminformation from 'systeminformation'; -import * as sysInfo from '../src/lib/system-info'; +import * as fsUtils from '../../../src/lib/fs-utils'; +import * as sysInfo from '../../../src/lib/system-info'; -function toMb(bytes: number) { - return Math.floor(bytes / 1024 / 1024); -} - -describe('System information', async () => { - const fsSizeStub = stub(systeminformation, 'fsSize'); - const memStub = stub(systeminformation, 'mem'); - const currentLoadStub = stub(systeminformation, 'currentLoad'); - const cpuTempStub = stub(systeminformation, 'cpuTemperature'); - - after(() => { - fsSizeStub.restore(); - memStub.restore(); - currentLoadStub.restore(); - cpuTempStub.restore(); +describe('System information', () => { + before(() => { + stub(systeminformation, 'fsSize'); + stub(systeminformation, 'mem').resolves(mockMemory); + stub(systeminformation, 'currentLoad').resolves(mockCPU.load); + stub(systeminformation, 'cpuTemperature').resolves(mockCPU.temp); + // @ts-ignore TS thinks we can't return a buffer... + stub(fs, 'readFile').resolves(mockCPU.idBuffer); + stub(fsUtils, 'exec'); }); - describe('Delta-based filtering', () => { + after(() => { + (systeminformation.fsSize as SinonStub).restore(); + (systeminformation.mem as SinonStub).restore(); + (systeminformation.currentLoad as SinonStub).restore(); + (systeminformation.cpuTemperature as SinonStub).restore(); + (fs.readFile as SinonStub).restore(); + (fsUtils.exec as SinonStub).restore(); + }); + + describe('filterNonSignificantChanges', () => { it('should correctly filter cpu usage', () => { expect(sysInfo.isSignificantChange('cpu_usage', 21, 20)).to.equal(false); @@ -45,7 +49,7 @@ describe('System information', async () => { ); }); - it('should not filter if we didnt have a past value', () => { + it("should not filter if we didn't have a past value", () => { expect(sysInfo.isSignificantChange('cpu_usage', undefined, 22)).to.equal( true, ); @@ -60,9 +64,8 @@ describe('System information', async () => { }); }); - describe('CPU information', async () => { + describe('CPU information', () => { it('gets CPU usage', async () => { - currentLoadStub.resolves(mockCPU.load); const cpuUsage = await sysInfo.getCpuUsage(); // Make sure it is a whole number expect(cpuUsage % 1).to.equal(0); @@ -71,7 +74,6 @@ describe('System information', async () => { }); it('gets CPU temp', async () => { - cpuTempStub.resolves(mockCPU.temp); const tempInfo = await sysInfo.getCpuTemp(); // Make sure it is a whole number expect(tempInfo % 1).to.equal(0); @@ -80,41 +82,17 @@ describe('System information', async () => { }); it('gets CPU ID', async () => { - const fsStub = stub(fs, 'readFile').resolves( - // @ts-ignore TS thinks we can't return a buffer... - Buffer.from([ - 0x31, - 0x30, - 0x30, - 0x30, - 0x30, - 0x30, - 0x30, - 0x30, - 0x30, - 0x31, - 0x62, - 0x39, - 0x33, - 0x66, - 0x33, - 0x66, - 0x00, - ]), - ); const cpuId = await sysInfo.getCpuId(); expect(cpuId).to.equal('1000000001b93f3f'); - fsStub.restore(); }); }); - describe('Memory information', async () => { + describe('getMemoryInformation', async () => { it('should return the correct value for memory usage', async () => { - memStub.resolves(mockMemory); const memoryInfo = await sysInfo.getMemoryInformation(); expect(memoryInfo).to.deep.equal({ - total: toMb(mockMemory.total), - used: toMb( + total: sysInfo.bytesToMb(mockMemory.total), + used: sysInfo.bytesToMb( mockMemory.total - mockMemory.free - (mockMemory.cached + mockMemory.buffers), @@ -123,9 +101,9 @@ describe('System information', async () => { }); }); - describe('Storage information', async () => { + describe('getStorageInfo', async () => { it('should return info on /data mount', async () => { - fsSizeStub.resolves(mockFS); + (systeminformation.fsSize as SinonStub).resolves(mockFS); const storageInfo = await sysInfo.getStorageInfo(); expect(storageInfo).to.deep.equal({ blockDevice: '/dev/mmcblk0p6', @@ -135,7 +113,7 @@ describe('System information', async () => { }); it('should handle no /data mount', async () => { - fsSizeStub.resolves([]); + (systeminformation.fsSize as SinonStub).resolves([]); const storageInfo = await sysInfo.getStorageInfo(); expect(storageInfo).to.deep.equal({ blockDevice: '', @@ -144,6 +122,23 @@ describe('System information', async () => { }); }); }); + + describe('undervoltageDetected', () => { + it('should detect undervoltage', async () => { + (fsUtils.exec as SinonStub).resolves({ + stdout: Buffer.from( + '[58611.126996] Under-voltage detected! (0x00050005)', + ), + stderr: Buffer.from(''), + }); + expect(await sysInfo.undervoltageDetected()).to.be.true; + (fsUtils.exec as SinonStub).resolves({ + stdout: Buffer.from('[569378.450066] eth0: renamed from veth3aa11ca'), + stderr: Buffer.from(''), + }); + expect(await sysInfo.undervoltageDetected()).to.be.false; + }); + }); }); const mockCPU = { @@ -226,6 +221,25 @@ const mockCPU = { }, ], }, + idBuffer: Buffer.from([ + 0x31, + 0x30, + 0x30, + 0x30, + 0x30, + 0x30, + 0x30, + 0x30, + 0x30, + 0x31, + 0x62, + 0x39, + 0x33, + 0x66, + 0x33, + 0x66, + 0x00, + ]), }; const mockFS = [ { From ea3e50e96ef0bddd653b898ec0e4c356159b2a2b Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 5 May 2021 22:12:51 +0900 Subject: [PATCH 2/4] Create & unify src/device-state/current-state tests Signed-off-by: Christina Wang --- src/device-state/current-state.ts | 62 ++- src/lib/errors.ts | 6 + test/05-device-state.spec.ts | 2 - test/10-api-binder.spec.ts | 47 -- test/src/device-state/current-state.spec.ts | 473 ++++++++++++++++++++ 5 files changed, 509 insertions(+), 81 deletions(-) create mode 100644 test/src/device-state/current-state.spec.ts diff --git a/src/device-state/current-state.ts b/src/device-state/current-state.ts index 143fd26d..06005412 100644 --- a/src/device-state/current-state.ts +++ b/src/device-state/current-state.ts @@ -1,17 +1,19 @@ import Bluebird = require('bluebird'); -import constants = require('../lib/constants'); -import log from '../lib/supervisor-console'; import * as _ from 'lodash'; -import { InternalInconsistencyError } from '../lib/errors'; -import { DeviceStatus } from '../types/state'; +import * as url from 'url'; +import { CoreOptions } from 'request'; + +import * as constants from '../lib/constants'; +import { log } from '../lib/supervisor-console'; +import { InternalInconsistencyError, StatusError } from '../lib/errors'; import { getRequestInstance } from '../lib/request'; +import * as sysInfo from '../lib/system-info'; + +import { DeviceStatus } from '../types/state'; import * as config from '../config'; +import { SchemaTypeKey, SchemaReturn } from '../config/schema-type'; import * as eventTracker from '../event-tracker'; import * as deviceState from '../device-state'; -import { CoreOptions } from 'request'; -import * as url from 'url'; - -import * as sysInfo from '../lib/system-info'; // The exponential backoff starts at 15s const MINIMUM_BACKOFF_DELAY = 15000; @@ -33,21 +35,23 @@ const stateForReport: DeviceStatus = { }; let reportPending = false; -class StatusError extends Error { - constructor(public statusCode: number, public statusMessage?: string) { - super(statusMessage); - } -} +type CurrentStateReportConf = { + [key in keyof Pick< + config.ConfigMap, + | 'uuid' + | 'apiEndpoint' + | 'apiTimeout' + | 'deviceApiKey' + | 'deviceId' + | 'localMode' + >]: SchemaReturn; +}; /** * Returns an object that contains only status fields relevant for the local mode. * It basically removes information about applications state. - * - * Exported for tests */ -export const stripDeviceStateInLocalMode = ( - state: DeviceStatus, -): DeviceStatus => { +const stripDeviceStateInLocalMode = (state: DeviceStatus): DeviceStatus => { return { local: _.cloneDeep( _.omit(state.local, 'apps', 'is_on__commit', 'logs_channel'), @@ -57,10 +61,11 @@ export const stripDeviceStateInLocalMode = ( const sendReportPatch = async ( stateDiff: DeviceStatus, - conf: { apiEndpoint: string; uuid: string; localMode: boolean }, + conf: Omit, ) => { let body = stateDiff; - if (conf.localMode) { + const { apiEndpoint, apiTimeout, deviceApiKey, localMode, uuid } = conf; + if (localMode) { body = stripDeviceStateInLocalMode(stateDiff); // In local mode, check if it still makes sense to send any updates after data strip. if (_.isEmpty(body.local)) { @@ -68,12 +73,6 @@ const sendReportPatch = async ( return; } } - const { uuid, apiEndpoint, apiTimeout, deviceApiKey } = await config.getMany([ - 'uuid', - 'apiEndpoint', - 'apiTimeout', - 'deviceApiKey', - ]); const endpoint = url.resolve(apiEndpoint, `/device/v2/${uuid}/state`); const request = await getRequestInstance(); @@ -101,7 +100,7 @@ const getStateDiff = (): DeviceStatus => { const lastReportedDependent = lastReportedState.dependent; if (lastReportedLocal == null || lastReportedDependent == null) { throw new InternalInconsistencyError( - `No local or dependent component of lastReportedLocal in ApiBinder.getStateDiff: ${JSON.stringify( + `No local or dependent component of lastReportedLocal in CurrentState.getStateDiff: ${JSON.stringify( lastReportedState, )}`, ); @@ -132,7 +131,7 @@ const getStateDiff = (): DeviceStatus => { const report = _.throttle(async () => { const conf = await config.getMany([ - 'deviceId', + 'deviceApiKey', 'apiTimeout', 'apiEndpoint', 'uuid', @@ -144,15 +143,14 @@ const report = _.throttle(async () => { return 0; } - const { apiEndpoint, uuid, localMode } = conf; - if (uuid == null || apiEndpoint == null) { + if (conf.uuid == null || conf.apiEndpoint == null) { throw new InternalInconsistencyError( - 'No uuid or apiEndpoint provided to ApiBinder.report', + 'No uuid or apiEndpoint provided to CurrentState.report', ); } try { - await sendReportPatch(stateDiff, { apiEndpoint, uuid, localMode }); + await sendReportPatch(stateDiff, conf); stateReportErrors = 0; _.assign(lastReportedState.local, stateDiff.local); diff --git a/src/lib/errors.ts b/src/lib/errors.ts index d1cf01e8..8ede7aa1 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -9,6 +9,12 @@ export interface StatusCodeError extends Error { statusCode?: string | number; } +export class StatusError extends Error { + constructor(public statusCode: number, public statusMessage?: string) { + super(statusMessage); + } +} + interface CodedSysError extends Error { code?: string; } diff --git a/test/05-device-state.spec.ts b/test/05-device-state.spec.ts index 681f96fa..079262b9 100644 --- a/test/05-device-state.spec.ts +++ b/test/05-device-state.spec.ts @@ -298,8 +298,6 @@ describe('deviceState', () => { deviceState.reportCurrentState({ someStateDiff: 'someValue' } as any); }); - it('returns the current state'); - it.skip('writes the target state to the db with some extra defaults', async () => { const testTarget = _.cloneDeep(testTargetWithDefaults2); diff --git a/test/10-api-binder.spec.ts b/test/10-api-binder.spec.ts index 6696f258..74fffa88 100644 --- a/test/10-api-binder.spec.ts +++ b/test/10-api-binder.spec.ts @@ -12,8 +12,6 @@ import balenaAPI = require('./lib/mocked-balena-api'); import { schema } from '../src/config/schema'; import ConfigJsonConfigBackend from '../src/config/configJson'; import * as TargetState from '../src/device-state/target-state'; -import { DeviceStatus } from '../src/types/state'; -import * as CurrentState from '../src/device-state/current-state'; import * as ApiHelper from '../src/lib/api-helper'; import supervisorVersion = require('../src/lib/supervisor-version'); import * as eventTracker from '../src/event-tracker'; @@ -319,51 +317,6 @@ describe('ApiBinder', () => { }); }); - describe('local mode', () => { - const components: Dictionary = {}; - - before(() => { - return initModels(components, '/config-apibinder.json'); - }); - - after(async () => { - // @ts-expect-error setting read-only property - config.configJsonBackend = defaultConfigBackend; - await config.generateRequiredFields(); - }); - - const sampleState = { - local: { - ip_address: '192.168.1.42 192.168.1.99', - api_port: 48484, - api_secret: - '20ffbd6e15aba827dca6381912d6aeb6c3a7a7c7206d4dfadf0d2f0a9e1136', - os_version: 'balenaOS 2.32.0+rev4', - os_variant: 'dev', - supervisor_version: '9.16.3', - provisioning_progress: null, - provisioning_state: '', - status: 'Idle', - logs_channel: null, - apps: {}, - is_on__commit: 'whatever', - }, - dependent: { apps: {} }, - } as DeviceStatus; - - it('should strip applications data', () => { - const result = CurrentState.stripDeviceStateInLocalMode( - sampleState, - ) as Dictionary; - expect(result).to.not.have.property('dependent'); - - const local = result['local']; - expect(local).to.not.have.property('apps'); - expect(local).to.not.have.property('is_on__commit'); - expect(local).to.not.have.property('logs_channel'); - }); - }); - describe('healthchecks', () => { const components: Dictionary = {}; let configStub: SinonStub; diff --git a/test/src/device-state/current-state.spec.ts b/test/src/device-state/current-state.spec.ts new file mode 100644 index 00000000..69927e6c --- /dev/null +++ b/test/src/device-state/current-state.spec.ts @@ -0,0 +1,473 @@ +import { expect } from 'chai'; +import { stub, SinonStub, spy, SinonSpy } from 'sinon'; +import rewire = require('rewire'); +import * as Bluebird from 'bluebird'; +import * as _ from 'lodash'; + +import * as config from '../../../src/config'; +import * as deviceState from '../../../src/device-state'; +import { stateReportErrors } from '../../../src/device-state/current-state'; + +import * as request from '../../../src/lib/request'; +import log from '../../../src/lib/supervisor-console'; +import { + InternalInconsistencyError, + StatusError, +} from '../../../src/lib/errors'; +import * as sysInfo from '../../../src/lib/system-info'; + +describe('device-state/current-state', () => { + // Set up rewire to track private methods and variables + const currentState = rewire('../../../src/device-state/current-state'); + const stripDeviceStateInLocalMode = currentState.__get__( + 'stripDeviceStateInLocalMode', + ); + const sendReportPatch = currentState.__get__('sendReportPatch'); + const getStateDiff = currentState.__get__('getStateDiff'); + const lastReportedState = currentState.__get__('lastReportedState'); + const stateForReport = currentState.__get__('stateForReport'); + + const resetGlobalStateObjects = () => { + lastReportedState.local = {}; + lastReportedState.dependent = {}; + stateForReport.local = {}; + stateForReport.dependent = {}; + }; + + // Global spies & stubs + let stubbedGetReqInstance: SinonStub; + // Mock the patchAsync method from src/lib/request.ts + let patchAsyncSpy: SinonSpy; + const requestInstance = { + patchAsync: () => Bluebird.resolve([{ statusCode: 200 }]), + }; + + // Suite-level hooks + before(() => { + stubbedGetReqInstance = stub(request, 'getRequestInstance'); + stubbedGetReqInstance.resolves(requestInstance); + }); + + after(() => { + stubbedGetReqInstance.restore(); + }); + + beforeEach(() => { + resetGlobalStateObjects(); + patchAsyncSpy = spy(requestInstance, 'patchAsync'); + }); + + afterEach(() => { + patchAsyncSpy.restore(); + }); + + // Test fixtures + const testDeviceConf = { + uuid: 'testuuid', + apiEndpoint: 'http://127.0.0.1', + apiTimeout: 100, + deviceApiKey: 'testapikey', + deviceId: 1337, + localMode: false, + disableHardwareMetrics: true, + }; + + const testDeviceReportFields = { + api_port: 48484, + api_secret: + '20ffbd6e15aba827dca6381912d6aeb6c3a7a7c7206d4dfadf0d2f0a9e1136', + ip_address: '192.168.1.42 192.168.1.99', + os_version: 'balenaOS 2.32.0+rev4', + os_variant: 'dev', + supervisor_version: '9.16.3', + provisioning_progress: null, + provisioning_state: '', + status: 'Idle', + update_failed: false, + update_pending: false, + update_downloaded: false, + is_on__commit: 'whatever', + logs_channel: null, + mac_addresss: '1C:69:7A:6E:B2:FE D8:3B:BF:51:F1:E4', + }; + + const testStateConfig = { + ENV_VAR_1: '1', + ENV_VAR_2: '1', + }; + + const testStateApps = { + '123': { + services: { + '456': { + status: 'Downloaded', + releaseId: '12345', + download_progress: null, + }, + '789': { + status: 'Downloaded', + releaseId: '12346', + download_progress: null, + }, + }, + }, + }; + + const testStateApps2 = { + '321': { + services: { + '654': { + status: 'Downloaded', + releaseId: '12347', + download_progress: null, + }, + }, + }, + }; + + const testCurrentState = { + local: { + ...testDeviceReportFields, + config: testStateConfig, + apps: testStateApps, + }, + dependent: { apps: testStateApps2 }, + commit: 'whatever', + }; + + describe('stripDeviceStateInLocalMode', () => { + it('should strip applications data', () => { + const result = stripDeviceStateInLocalMode(testCurrentState); + + expect(result).to.not.have.property('dependent'); + + const local = result['local']; + expect(local).to.not.have.property('apps'); + expect(local).to.not.have.property('is_on__commit'); + expect(local).to.not.have.property('logs_channel'); + }); + + it('should not mutate the input state', () => { + const result = stripDeviceStateInLocalMode(testCurrentState); + expect(result).to.not.deep.equal(testCurrentState); + }); + }); + + describe('sendReportPatch', () => { + it('should only strip app state in local mode', async () => { + // Strip state in local mode + await sendReportPatch(testCurrentState, { + ...testDeviceConf, + localMode: true, + }); + // Request body's stripped state should be different than input state + expect(patchAsyncSpy.lastCall.lastArg.body).to.not.deep.equal( + testCurrentState, + ); + + // Don't strip state out of local mode + await sendReportPatch(testCurrentState, testDeviceConf); + expect(patchAsyncSpy.lastCall.lastArg.body).to.deep.equal( + testCurrentState, + ); + }); + + it('should patch state with empty local objects depending on local mode config', async () => { + // Don't patch state with empty state.local in local mode + await sendReportPatch( + { ...testCurrentState, local: {} }, + { ...testDeviceConf, localMode: true }, + ); + expect(patchAsyncSpy).to.not.have.been.called; + + // Patch state with empty state.local out of local mode + await sendReportPatch({ ...testCurrentState, local: {} }, testDeviceConf); + expect(patchAsyncSpy).to.have.been.called; + }); + + it('should patch with specified endpoint and params', async () => { + await sendReportPatch(testCurrentState, testDeviceConf); + const [endpoint, params] = patchAsyncSpy.lastCall.args; + expect(endpoint).to.equal( + `${testDeviceConf.apiEndpoint}/device/v2/${testDeviceConf.uuid}/state`, + ); + expect(params).to.deep.equal({ + json: true, + headers: { Authorization: `Bearer ${testDeviceConf.deviceApiKey}` }, + body: testCurrentState, + }); + }); + + it('should timeout patch request after apiTimeout milliseconds', async () => { + // Overwrite mock patchAsync to delay past apiTimeout ms + patchAsyncSpy.restore(); + requestInstance.patchAsync = () => + Bluebird.delay( + testDeviceConf.apiTimeout + 100, + Bluebird.resolve([{ statusCode: 200 }]), + ); + patchAsyncSpy = spy(requestInstance, 'patchAsync'); + + await expect( + sendReportPatch(testCurrentState, testDeviceConf), + ).to.be.rejectedWith(Bluebird.TimeoutError); + + // Reset to default patchAsync + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 200 }]); + }); + + it('should communicate string error messages from the API', async () => { + // Overwrite mock patchAsync to reject patch request + patchAsyncSpy.restore(); + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 400, body: 'string error message' }]); + patchAsyncSpy = spy(requestInstance, 'patchAsync'); + + stub(log, 'error'); + + await expect( + sendReportPatch(testCurrentState, testDeviceConf), + ).to.be.rejected.then((err) => { + expect(err).to.be.instanceOf(StatusError); + expect(err).to.have.property('statusMessage', '"string error message"'); + }); + expect(log.error as SinonStub).to.have.been.calledWith( + `Error from the API: ${400}`, + ); + + // Reset to default patchAsync + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 200 }]); + + (log.error as SinonStub).restore(); + }); + + it('should communicate multiline object error messages from the API', async () => { + const objectErrorMessage = { + testKey: 'testErrorVal', + testChild: { testNestedKey: 'testNestedVal' }, + }; + // Overwrite mock patchAsync to reject patch request + patchAsyncSpy.restore(); + requestInstance.patchAsync = () => + Bluebird.resolve([ + { + statusCode: 400, + body: objectErrorMessage, + }, + ]); + patchAsyncSpy = spy(requestInstance, 'patchAsync'); + + stub(log, 'error'); + + await expect( + sendReportPatch(testCurrentState, testDeviceConf), + ).to.be.rejected.then((err) => { + expect(err).to.be.instanceOf(StatusError); + expect(err).to.have.property( + 'statusMessage', + JSON.stringify(objectErrorMessage, null, 2), + ); + }); + expect(log.error as SinonStub).to.have.been.calledWith( + `Error from the API: ${400}`, + ); + + // Reset to default patchAsync + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 200 }]); + + (log.error as SinonStub).restore(); + }); + }); + + describe('getStateDiff', () => { + it('should error if last reported state is missing local or dependent properties', () => { + lastReportedState.local = null; + lastReportedState.dependent = null; + + expect(() => getStateDiff()).to.throw(InternalInconsistencyError); + }); + + it('should not modify global stateForReport or lastReportedState after call', async () => { + lastReportedState.local = { + status: 'Downloading', + config: testStateConfig, + }; + stateForReport.local = { + status: 'Idle', + config: { ...testStateConfig, ENV_VAR_3: '1' }, + }; + getStateDiff(); + expect(lastReportedState.local).to.deep.equal({ + status: 'Downloading', + config: { + ENV_VAR_1: '1', + ENV_VAR_2: '1', + }, + }); + expect(stateForReport.local).to.deep.equal({ + status: 'Idle', + config: { + ENV_VAR_1: '1', + ENV_VAR_2: '1', + ENV_VAR_3: '1', + }, + }); + }); + + it('should return any changed fields', async () => { + // No diffs when lastReportedState and stateForReport are the same + expect(getStateDiff()).to.deep.equal({}); + + // Changed config fields + lastReportedState.local = { + config: { ENV_VAR_3: '1' }, + }; + stateForReport.local = { + config: { ENV_VAR_3: '0' }, + }; + expect(getStateDiff()).to.deep.equal({ + local: { config: { ENV_VAR_3: '0' } }, + }); + resetGlobalStateObjects(); + + // Changed apps fields + lastReportedState.local = { apps: testStateApps }; + stateForReport.local = { apps: testStateApps2 }; + expect(getStateDiff()).to.deep.equal({ local: { apps: testStateApps2 } }); + resetGlobalStateObjects(); + + // Changed dependent fields + lastReportedState.dependent = { apps: testStateApps2 }; + stateForReport.dependent = { apps: testStateApps }; + expect(getStateDiff()).to.deep.equal({ + dependent: { apps: testStateApps }, + }); + resetGlobalStateObjects(); + + // Changed sys info fields + lastReportedState.local = { cpu_temp: 10 }; + stateForReport.local = { cpu_temp: 16 }; + expect(getStateDiff()).to.deep.equal({ local: { cpu_temp: 16 } }); + resetGlobalStateObjects(); + }); + + it('should omit internal state keys and report DeviceReportField (type) info', async () => { + // INTERNAL_STATE_KEYS are: ['update_pending', 'update_downloaded', 'update_failed'] + stateForReport.local = _.pick(testDeviceReportFields, [ + 'update_pending', + 'update_downloaded', + 'update_failed', + 'os_version', + ]); + stateForReport.dependent = _.pick(testDeviceReportFields, [ + 'update_pending', + 'update_downloaded', + 'update_failed', + 'status', + ]); + expect(getStateDiff()).to.deep.equal({ + local: _.pick(testDeviceReportFields, ['os_version']), + dependent: _.pick(testDeviceReportFields, ['status']), + }); + }); + }); + + describe('throttled report', () => { + const report = currentState.__get__('report'); + + let configGetManyStub: SinonStub; + + before(() => { + configGetManyStub = stub(config, 'getMany'); + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 200 }]); + }); + + after(() => { + configGetManyStub.restore(); + }); + + beforeEach(() => { + configGetManyStub.resolves( + _.omit(testDeviceConf, ['deviceId', 'disableHardwareMetrics']) as any, + ); + }); + + afterEach(() => { + // Clear the throttle time limit between tests + report.cancel(); + }); + + it("doesn't report if current state hasn't changed", async () => { + // A beforeEach hook resets the global state objects to all be empty, so + // by default, report() will return 0 in this test env + expect(await report()).to.equal(0); + }); + + it('errors when provided invalid uuid or apiEndpoint', async () => { + configGetManyStub.resolves( + _.omit(testDeviceConf, [ + 'deviceId', + 'disableHardwareMetrics', + 'uuid', + 'apiEndpoint', + ]) as any, + ); + stateForReport.local = { ...testDeviceReportFields }; + await expect(report()).to.be.rejectedWith(InternalInconsistencyError); + }); + + it('resets stateReportErrors to 0 on patch success', async () => { + spy(_, 'assign'); + + stateForReport.local = { ...testDeviceReportFields }; + await report(); + + expect(stateReportErrors).to.equal(0); + expect(_.assign as SinonSpy).to.have.been.calledTwice; + + (_.assign as SinonSpy).restore(); + }); + + it('handles errors on state patch failure', async () => { + // Overwrite default patchAsync response to return erroring statusCode + patchAsyncSpy.restore(); + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 400 }]); + patchAsyncSpy = spy(requestInstance, 'patchAsync'); + + stub(log, 'error'); + + stateForReport.local = { ...testDeviceReportFields }; + await report(); + + expect((log.error as SinonStub).lastCall.args[0]).to.equal( + 'Non-200 response from the API! Status code: 400 - message:', + ); + (log.error as SinonStub).restore(); + + // Reset to default patchAsync + requestInstance.patchAsync = () => + Bluebird.resolve([{ statusCode: 200 }]); + }); + }); + + describe.skip('reportCurrentState', () => { + const reportPending = currentState.__get__('reportPending'); + + before(() => { + stub(deviceState, 'getStatus').resolves(testCurrentState as any); + stub(config, 'get').resolves(true); + }); + + after(() => { + (deviceState.getStatus as SinonStub).restore(); + (config.get as SinonStub).restore(); + }); + + it('does not report if current state has not changed'); + }); +}); From dcd863eed8b58cc7eae866c941597c7e0a9bfcb1 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Wed, 5 May 2021 23:16:58 +0900 Subject: [PATCH 3/4] Add toggleable `SUPERVISOR_HARDWARE_METRICS` config On devices with bandwidth sensitivity, this config var disables sending system information such as memory usage or cpu temp as current state. Closes: #1645 Change-type: minor Signed-off-by: Christina Wang --- src/config/schema-type.ts | 4 + src/config/schema.ts | 5 + src/device-config.ts | 5 + src/device-state/current-state.ts | 21 +++- src/lib/system-info.ts | 37 ++++-- test/12-device-config.spec.ts | 1 + test/lib/helpers.ts | 3 + test/src/device-state/current-state.spec.ts | 130 ++++++++++++++++++-- test/src/lib/system-info.spec.ts | 14 +++ 9 files changed, 202 insertions(+), 18 deletions(-) diff --git a/src/config/schema-type.ts b/src/config/schema-type.ts index 308681f8..9660d29f 100644 --- a/src/config/schema-type.ts +++ b/src/config/schema-type.ts @@ -166,6 +166,10 @@ export const schemaTypes = { type: PermissiveBoolean, default: true, }, + hardwareMetrics: { + type: PermissiveBoolean, + default: true, + }, // Function schema types // The type should be the value that the promise resolves diff --git a/src/config/schema.ts b/src/config/schema.ts index 588660cf..90b687bc 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -180,6 +180,11 @@ export const schema = { mutable: true, removeIfNull: false, }, + hardwareMetrics: { + source: 'db', + mutable: true, + removeIfNull: false, + }, }; export type Schema = typeof schema; diff --git a/src/device-config.ts b/src/device-config.ts index f520b35c..579265db 100644 --- a/src/device-config.ts +++ b/src/device-config.ts @@ -191,6 +191,11 @@ const configKeys: Dictionary = { varType: 'bool', defaultValue: 'true', }, + hardwareMetrics: { + envVarName: 'SUPERVISOR_HARDWARE_METRICS', + varType: 'bool', + defaultValue: 'true', + }, }; export const validKeys = [ diff --git a/src/device-state/current-state.ts b/src/device-state/current-state.ts index 06005412..e3ac7b4f 100644 --- a/src/device-state/current-state.ts +++ b/src/device-state/current-state.ts @@ -44,6 +44,7 @@ type CurrentStateReportConf = { | 'deviceApiKey' | 'deviceId' | 'localMode' + | 'hardwareMetrics' >]: SchemaReturn; }; @@ -61,7 +62,7 @@ const stripDeviceStateInLocalMode = (state: DeviceStatus): DeviceStatus => { const sendReportPatch = async ( stateDiff: DeviceStatus, - conf: Omit, + conf: Omit, ) => { let body = stateDiff; const { apiEndpoint, apiTimeout, deviceApiKey, localMode, uuid } = conf; @@ -175,7 +176,23 @@ const reportCurrentState = (): null => { reportPending = true; try { const currentDeviceState = await deviceState.getStatus(); - const info = await sysInfo.getSysInfoToReport(); + // If hardwareMetrics is false, send null patch for system metrics to cloud API + const info = { + ...((await config.get('hardwareMetrics')) + ? await sysInfo.getSystemMetrics() + : { + cpu_usage: null, + memory_usage: null, + memory_total: null, + storage_usage: null, + storage_total: null, + storage_block_device: null, + cpu_temp: null, + cpu_id: null, + }), + ...(await sysInfo.getSystemChecks()), + }; + stateForReport.local = { ...stateForReport.local, ...currentDeviceState.local, diff --git a/src/lib/system-info.ts b/src/lib/system-info.ts index b9fef9d0..b5808a74 100644 --- a/src/lib/system-info.ts +++ b/src/lib/system-info.ts @@ -87,14 +87,31 @@ export async function undervoltageDetected(): Promise { } } -export async function getSysInfoToReport() { - const [cpu, mem, temp, cpuid, storage, undervoltage] = await Promise.all([ +/** + * System metrics that are always reported in current state + * due to their importance, regardless of HARDWARE_METRICS + */ +export async function getSystemChecks() { + // TODO: feature - add more eager diagnostic style metrics here, + // such as fs corruption checks, network issues, etc. + const undervoltage = await undervoltageDetected(); + + return { + is_undervolted: undervoltage, + }; +} + +/** + * Metrics that would be reported in current state only + * when HARDWARE_METRICS config var is true. + */ +export async function getSystemMetrics() { + const [cpu, mem, temp, cpuid, storage] = await Promise.all([ getCpuUsage(), getMemoryInformation(), getCpuTemp(), getCpuId(), getStorageInfo(), - undervoltageDetected(), ]); return { @@ -106,13 +123,14 @@ export async function getSysInfoToReport() { storage_block_device: storage.blockDevice, cpu_temp: temp, cpu_id: cpuid, - is_undervolted: undervoltage, }; } -export type SystemInfo = UnwrappedPromise< - ReturnType ->; +type SystemChecks = UnwrappedPromise>; + +type SystemMetrics = UnwrappedPromise>; + +export type SystemInfo = SystemChecks & SystemMetrics; const significantChange: { [key in keyof SystemInfo]?: number } = { cpu_usage: 20, @@ -134,6 +152,11 @@ export function isSignificantChange( if (bucketSize == null) { return true; } + // If the `current` parameter is null, HARDWARE_METRICS has been toggled false + // and we should include this value for the null metrics patch to the API + if (current === null) { + return true; + } return Math.floor(current / bucketSize) !== Math.floor(past / bucketSize); } diff --git a/test/12-device-config.spec.ts b/test/12-device-config.spec.ts index 39120d16..ffcd6547 100644 --- a/test/12-device-config.spec.ts +++ b/test/12-device-config.spec.ts @@ -263,6 +263,7 @@ describe('Device Backend Config', () => { SUPERVISOR_INSTANT_UPDATE_TRIGGER: 'true', SUPERVISOR_OVERRIDE_LOCK: 'false', SUPERVISOR_PERSISTENT_LOGGING: 'false', + SUPERVISOR_HARDWARE_METRICS: 'true', }); }); diff --git a/test/lib/helpers.ts b/test/lib/helpers.ts index 638fc019..19afe385 100644 --- a/test/lib/helpers.ts +++ b/test/lib/helpers.ts @@ -1,3 +1,6 @@ +/** + * Delays for input ms before resolving + */ export async function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } diff --git a/test/src/device-state/current-state.spec.ts b/test/src/device-state/current-state.spec.ts index 69927e6c..b8ada63d 100644 --- a/test/src/device-state/current-state.spec.ts +++ b/test/src/device-state/current-state.spec.ts @@ -4,9 +4,12 @@ import rewire = require('rewire'); import * as Bluebird from 'bluebird'; import * as _ from 'lodash'; +import { sleep } from '../../lib/helpers'; + import * as config from '../../../src/config'; import * as deviceState from '../../../src/device-state'; import { stateReportErrors } from '../../../src/device-state/current-state'; +import * as eventTracker from '../../../src/event-tracker'; import * as request from '../../../src/lib/request'; import log from '../../../src/lib/supervisor-console'; @@ -42,7 +45,7 @@ describe('device-state/current-state', () => { patchAsync: () => Bluebird.resolve([{ statusCode: 200 }]), }; - // Suite-level hooks + // Parent-level hooks to prevent any requests from actually happening in any suites below before(() => { stubbedGetReqInstance = stub(request, 'getRequestInstance'); stubbedGetReqInstance.resolves(requestInstance); @@ -69,7 +72,7 @@ describe('device-state/current-state', () => { deviceApiKey: 'testapikey', deviceId: 1337, localMode: false, - disableHardwareMetrics: true, + hardwareMetrics: true, }; const testDeviceReportFields = { @@ -392,7 +395,7 @@ describe('device-state/current-state', () => { beforeEach(() => { configGetManyStub.resolves( - _.omit(testDeviceConf, ['deviceId', 'disableHardwareMetrics']) as any, + _.omit(testDeviceConf, ['deviceId', 'hardwareMetrics']) as any, ); }); @@ -411,7 +414,7 @@ describe('device-state/current-state', () => { configGetManyStub.resolves( _.omit(testDeviceConf, [ 'deviceId', - 'disableHardwareMetrics', + 'hardwareMetrics', 'uuid', 'apiEndpoint', ]) as any, @@ -455,19 +458,128 @@ describe('device-state/current-state', () => { }); }); - describe.skip('reportCurrentState', () => { - const reportPending = currentState.__get__('reportPending'); + describe('reportCurrentState', () => { + const reportCurrentState = currentState.__get__('reportCurrentState'); + const report = currentState.__get__('report'); + let testHardwareMetrics = true; + const testAppUpdatePollInterval = 1000; + + const unhandledRejectionHandler = () => { + /* noop */ + }; before(() => { - stub(deviceState, 'getStatus').resolves(testCurrentState as any); - stub(config, 'get').resolves(true); + stub(deviceState, 'getStatus').resolves({}); + stub(sysInfo, 'getSystemMetrics').resolves(); + stub(sysInfo, 'getSystemChecks').resolves(); + stub(eventTracker, 'track'); + stub(config, 'get'); + (config.get as SinonStub).callsFake((conf) => + conf === 'hardwareMetrics' + ? Promise.resolve(testHardwareMetrics) + : Promise.resolve(testAppUpdatePollInterval), + ); + // We need to stub this rejection so that reportCurrentState doesn't keep calling itself + stub(config, 'getMany').rejects(); + // We also need to stub this rejection because it's called right before + // reportCurrentState in the catch, to prevent more calls of reportCurrentState + stub(Bluebird, 'delay').rejects(); }); after(() => { (deviceState.getStatus as SinonStub).restore(); + (sysInfo.getSystemMetrics as SinonStub).restore(); + (sysInfo.getSystemChecks as SinonStub).restore(); + (eventTracker.track as SinonStub).restore(); (config.get as SinonStub).restore(); + (config.getMany as SinonStub).restore(); + (Bluebird.delay as SinonStub).restore(); }); - it('does not report if current state has not changed'); + beforeEach(() => { + resetGlobalStateObjects(); + }); + + afterEach(() => { + // Clear the throttle time limit between tests + report.cancel(); + }); + + it('does not report if current state has not changed', async () => { + // Use a temporary unhandledRejectionHandler to catch the promise + // rejection from the Bluebird.delay stub + process.on('unhandledRejection', unhandledRejectionHandler); + spy(_, 'size'); + + reportCurrentState(); + + // Wait 200ms for anonymous async IIFE inside reportCurrentState to finish executing + // TODO: is there a better way to test this? Possible race condition + await sleep(200); + + expect(stateForReport).to.deep.equal({ local: {}, dependent: {} }); + expect(_.size as SinonSpy).to.have.returned(0); + + (_.size as SinonSpy).restore(); + process.removeListener('unhandledRejection', unhandledRejectionHandler); + }); + + it('sends a null patch for system metrics when HARDWARE_METRICS is false', async () => { + // Use a temporary unhandledRejectionHandler to catch the promise + // rejection from the Bluebird.delay stub + process.on('unhandledRejection', unhandledRejectionHandler); + + testHardwareMetrics = false; + + (sysInfo.getSystemMetrics as SinonStub).resolves({ cpu_usage: 20 }); + (sysInfo.getSystemChecks as SinonStub).resolves({ + is_undervolted: false, + }); + + reportCurrentState(); + + await sleep(200); + + expect(stateForReport).to.deep.equal({ + local: { + is_undervolted: false, + cpu_usage: null, + memory_usage: null, + memory_total: null, + storage_usage: null, + storage_total: null, + storage_block_device: null, + cpu_temp: null, + cpu_id: null, + }, + dependent: {}, + }); + + process.removeListener('unhandledRejection', unhandledRejectionHandler); + }); + + it('reports both system metrics and system checks when HARDWARE_METRICS is true', async () => { + // Use a temporary unhandledRejectionHandler to catch the promise + // rejection from the Bluebird.delay stub + process.on('unhandledRejection', unhandledRejectionHandler); + + testHardwareMetrics = true; + + (sysInfo.getSystemMetrics as SinonStub).resolves({ cpu_usage: 20 }); + (sysInfo.getSystemChecks as SinonStub).resolves({ + is_undervolted: false, + }); + + reportCurrentState(); + + await sleep(200); + + expect(stateForReport).to.deep.equal({ + local: { is_undervolted: false, cpu_usage: 20 }, + dependent: {}, + }); + + process.removeListener('unhandledRejection', unhandledRejectionHandler); + }); }); }); diff --git a/test/src/lib/system-info.spec.ts b/test/src/lib/system-info.spec.ts index 96fe0836..6f3f6384 100644 --- a/test/src/lib/system-info.spec.ts +++ b/test/src/lib/system-info.spec.ts @@ -62,6 +62,20 @@ describe('System information', () => { sysInfo.isSignificantChange('memory_usage', undefined, 5), ).to.equal(true); }); + + it('should not filter if the current value is null', () => { + // When the current value is null, we're sending a null patch to the + // API in response to setting HARDWARE_METRICS to false, so + // we need to include null for all values. None of the individual metrics + // in systemMetrics return null (only number/undefined), so the only + // reason for current to be null is when a null patch is happening. + expect(sysInfo.isSignificantChange('cpu_usage', 15, null as any)).to.be + .true; + expect(sysInfo.isSignificantChange('cpu_temp', 55, null as any)).to.be + .true; + expect(sysInfo.isSignificantChange('memory_usage', 760, null as any)).to + .be.true; + }); }); describe('CPU information', () => { From 5004cc5fd29267e39e02f2748d14013a07bc6be4 Mon Sep 17 00:00:00 2001 From: Christina Wang Date: Mon, 10 May 2021 17:44:39 +0900 Subject: [PATCH 4/4] Add `SUPERVISOR_HARDWARE_METRICS` to config documentation Signed-off-by: Christina Wang --- docs/configurations.md | 1 + test/src/lib/system-info.spec.ts | 49 ++++++++++++++++---------------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/docs/configurations.md b/docs/configurations.md index 587ed669..b459756a 100644 --- a/docs/configurations.md +++ b/docs/configurations.md @@ -26,6 +26,7 @@ This list contains configuration variables that can be used with all balena devi | BALENA_HOST_FIREWALL_MODE | string | false | off | Toggle firewall modes between on, off, and auto. | v11.9.1 | | BALENA_HOST_DISCOVERABILITY | boolean | false | true | Enable / Disable Avahi to run which will allow the device to respond to requests such as network scans. | v11.9.2 | | BALENA_HOST_SPLASH_IMAGE | integer | true | /boot/splash/balena-logo-default.png | Allows changing splash screen on boot to user defined file from userspace. | v12.3.0 | +| BALENA_SUPERVISOR_HARDWARE_METRICS | boolean | false | true | Toggle hardware metrics reporting to the cloud, which occurs every 10 seconds when there are changes. Metrics include CPU utilization, CPU temperature, memory usage, and disk space. Useful for devices with bandwith sensitivity. | v12.8.0 | --- diff --git a/test/src/lib/system-info.spec.ts b/test/src/lib/system-info.spec.ts index 6f3f6384..c01e3054 100644 --- a/test/src/lib/system-info.spec.ts +++ b/test/src/lib/system-info.spec.ts @@ -26,41 +26,42 @@ describe('System information', () => { (fsUtils.exec as SinonStub).restore(); }); - describe('filterNonSignificantChanges', () => { + describe('isSignificantChange', () => { it('should correctly filter cpu usage', () => { - expect(sysInfo.isSignificantChange('cpu_usage', 21, 20)).to.equal(false); - - expect(sysInfo.isSignificantChange('cpu_usage', 10, 20)).to.equal(true); + expect(sysInfo.isSignificantChange('cpu_usage', 21, 20)).to.be.false; + expect(sysInfo.isSignificantChange('cpu_usage', 10, 20)).to.be.true; }); it('should correctly filter cpu temperature', () => { - expect(sysInfo.isSignificantChange('cpu_temp', 21, 22)).to.equal(false); - - expect(sysInfo.isSignificantChange('cpu_temp', 10, 20)).to.equal(true); + expect(sysInfo.isSignificantChange('cpu_temp', 21, 22)).to.be.false; + expect(sysInfo.isSignificantChange('cpu_temp', 10, 20)).to.be.true; }); it('should correctly filter memory usage', () => { - expect(sysInfo.isSignificantChange('memory_usage', 21, 22)).to.equal( - false, - ); - - expect(sysInfo.isSignificantChange('memory_usage', 10, 20)).to.equal( - true, - ); + expect(sysInfo.isSignificantChange('memory_usage', 21, 22)).to.be.false; + expect(sysInfo.isSignificantChange('memory_usage', 10, 20)).to.be.true; }); it("should not filter if we didn't have a past value", () => { - expect(sysInfo.isSignificantChange('cpu_usage', undefined, 22)).to.equal( - true, - ); + expect(sysInfo.isSignificantChange('cpu_usage', undefined, 22)).to.be + .true; + expect(sysInfo.isSignificantChange('cpu_temp', undefined, 10)).to.be.true; + expect(sysInfo.isSignificantChange('memory_usage', undefined, 5)).to.be + .true; + }); - expect(sysInfo.isSignificantChange('cpu_temp', undefined, 10)).to.equal( - true, - ); - - expect( - sysInfo.isSignificantChange('memory_usage', undefined, 5), - ).to.equal(true); + it('should not filter if the current value is null', () => { + // When the current value is null, we're sending a null patch to the + // API in response to setting DISABLE_HARDWARE_METRICS to true, so + // we need to include null for all values. None of the individual metrics + // in systemMetrics return null (only number/undefined), so the only + // reason for current to be null is when a null patch is happening. + expect(sysInfo.isSignificantChange('cpu_usage', 15, null as any)).to.be + .true; + expect(sysInfo.isSignificantChange('cpu_temp', 55, null as any)).to.be + .true; + expect(sysInfo.isSignificantChange('memory_usage', 760, null as any)).to + .be.true; }); it('should not filter if the current value is null', () => {