-
Notifications
You must be signed in to change notification settings - Fork 647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(server): Server should periodically delete old builds #471
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,67 @@ | ||||||||
'use strict'; | ||||||||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
const {CronJob} = require('cron'); | ||||||||
const {normalizeCronSchedule} = require('./utils'); | ||||||||
|
||||||||
/** | ||||||||
* @param {LHCI.ServerCommand.StorageMethod} storageMethod | ||||||||
* @param {number} range | ||||||||
* @param {Date} now | ||||||||
* @return {Promise<void>} | ||||||||
*/ | ||||||||
async function deleteOldBuilds(storageMethod, range, now = new Date()) { | ||||||||
if (!range || !Number.isInteger(range) || range <= 0) { | ||||||||
throw new Error('Invalid range'); | ||||||||
} | ||||||||
|
||||||||
const runAt = new Date(now.setDate(now.getDate() + range)); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WDYT about...
Suggested change
or something like that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with your comment.
Your code looks to be easier than my code to understand. Thanks for the good suggestions. I will change it. |
||||||||
const oldBuilds = await storageMethod.findOldBuilds(runAt); | ||||||||
for (const {projectId, id} of oldBuilds) { | ||||||||
await storageMethod.deleteBuild(projectId, id); | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* @param {LHCI.ServerCommand.StorageMethod} storageMethod | ||||||||
* @param {LHCI.ServerCommand.Options} options | ||||||||
* @return {void} | ||||||||
*/ | ||||||||
function startDeletingOldBuildsCron(storageMethod, options) { | ||||||||
if (options.storage.storageMethod !== 'sql' || !options.storage.deleteOldBuilds) { | ||||||||
return; | ||||||||
} | ||||||||
|
||||||||
if (!options.storage.deleteOldBuilds.schedule || !options.storage.deleteOldBuilds.dateRange) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of |
||||||||
throw new Error('Cannot configure schedule'); | ||||||||
} | ||||||||
|
||||||||
const log = | ||||||||
options.logLevel === 'silent' | ||||||||
? () => {} | ||||||||
: msg => process.stdout.write(`${new Date().toISOString()} - ${msg}\n`); | ||||||||
|
||||||||
let inProgress = false; | ||||||||
|
||||||||
const {schedule, dateRange} = options.storage.deleteOldBuilds; | ||||||||
|
||||||||
const cron = new CronJob(normalizeCronSchedule(schedule), () => { | ||||||||
if (inProgress) { | ||||||||
log(`Deleting old builds still in progress. Skipping...`); | ||||||||
return; | ||||||||
} | ||||||||
inProgress = true; | ||||||||
log(`Starting delete old builds`); | ||||||||
deleteOldBuilds(storageMethod, dateRange, new Date()) | ||||||||
.then(() => { | ||||||||
log(`Successfully delete old builds`); | ||||||||
}) | ||||||||
.catch(err => { | ||||||||
log(`Delete old builds failure: ${err.message}`); | ||||||||
}) | ||||||||
.finally(() => { | ||||||||
inProgress = false; | ||||||||
}); | ||||||||
}); | ||||||||
cron.start(); | ||||||||
} | ||||||||
module.exports = {startDeletingOldBuildsCron, deleteOldBuilds}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
'use strict'; | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* @param {string} schedule | ||
* @return {string} | ||
*/ | ||
function normalizeCronSchedule(schedule) { | ||
if (typeof schedule !== 'string') { | ||
throw new Error(`Schedule must be provided`); | ||
} | ||
|
||
if (process.env.OVERRIDE_SCHEDULE_FOR_TEST) { | ||
return process.env.OVERRIDE_SCHEDULE_FOR_TEST; | ||
} | ||
|
||
const parts = schedule.split(/\s+/).filter(Boolean); | ||
if (parts.length !== 5) { | ||
throw new Error(`Invalid cron format, expected <minutes> <hours> <day> <month> <day of week>`); | ||
} | ||
|
||
if (parts[0] === '*') { | ||
throw new Error(`Cron schedule "${schedule}" is too frequent`); | ||
} | ||
|
||
return ['0', ...parts].join(' '); | ||
} | ||
module.exports = {normalizeCronSchedule}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ const createProjectsRouter = require('./api/routes/projects.js'); | |
const StorageMethod = require('./api/storage/storage-method.js'); | ||
const {errorMiddleware, createBasicAuthMiddleware} = require('./api/express-utils.js'); | ||
const {startPsiCollectCron} = require('./cron/psi-collect.js'); | ||
const {startDeletingOldBuildsCron} = require('./cron/delete-old-builds'); | ||
const version = require('../package.json').version; | ||
|
||
const DIST_FOLDER = path.join(__dirname, '../dist'); | ||
|
@@ -55,6 +56,7 @@ async function createApp(options) { | |
app.use(errorMiddleware); | ||
|
||
startPsiCollectCron(storageMethod, options); | ||
startDeletingOldBuildsCron(storageMethod, options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for consistency with option proposed name |
||
|
||
return {app, storageMethod}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,133 @@ | ||
'use strict'; | ||
patrickhulce marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/* eslint-env jest */ | ||
|
||
/** @type {jest.MockInstance} */ | ||
let cronJob = jest.fn().mockReturnValue({start: () => {}}); | ||
jest.mock('cron', () => ({ | ||
CronJob: function(...args) { | ||
// use this indirection because we have to invoke it with `new` and it's harder to mock assertions | ||
return cronJob(...args); | ||
}, | ||
})); | ||
|
||
const { | ||
startDeletingOldBuildsCron, | ||
deleteOldBuilds, | ||
} = require('../../src/cron/delete-old-builds.js'); | ||
|
||
describe('cron/delete-old-builds', () => { | ||
/** @type {{ findOldBuilds: jest.MockInstance, deleteBuild: jest.MockInstance}} */ | ||
let storageMethod; | ||
beforeEach(() => { | ||
storageMethod = { | ||
findOldBuilds: jest.fn().mockResolvedValue([]), | ||
deleteBuild: jest.fn().mockResolvedValue({}), | ||
}; | ||
|
||
cronJob = jest.fn().mockReturnValue({ | ||
start: () => {}, | ||
}); | ||
}); | ||
describe('.deleteOldBuilds', () => { | ||
it.each([undefined, null, -1, new Date()])( | ||
'should throw for invalid range (%s)', | ||
async range => { | ||
await expect(deleteOldBuilds(storageMethod, range)).rejects.toMatchObject({ | ||
message: 'Invalid range', | ||
}); | ||
} | ||
); | ||
|
||
it('should collect', async () => { | ||
storageMethod.deleteBuild.mockClear(); | ||
const deleteObjects = [{id: 'id-1', projectId: 'pid-1'}, {id: 'id-2', projectId: 'pid-2'}]; | ||
storageMethod.findOldBuilds.mockResolvedValue(deleteObjects); | ||
await deleteOldBuilds(storageMethod, 30, new Date('2019-01-01')); | ||
expect(storageMethod.deleteBuild).toHaveBeenCalledTimes(deleteObjects.length); | ||
|
||
expect(storageMethod.deleteBuild.mock.calls).toMatchObject([ | ||
['pid-1', 'id-1'], | ||
['pid-2', 'id-2'], | ||
]); | ||
}); | ||
}); | ||
|
||
describe('.startDeletingOldBuildsCron', () => { | ||
it.each([ | ||
[ | ||
'storageMethod is not sql', | ||
{ | ||
storage: { | ||
storageMethod: 'notsql', | ||
deleteOldBuilds: { | ||
schedule: '0 * * * *', | ||
dateRange: 30, | ||
}, | ||
}, | ||
}, | ||
], | ||
[ | ||
'no deleteOldBuilds options', | ||
{ | ||
storage: {storageMethod: 'sql'}, | ||
}, | ||
], | ||
])('should not schedule a cron job (%s)', (_, options) => { | ||
startDeletingOldBuildsCron(storageMethod, options); | ||
expect(cronJob).toHaveBeenCalledTimes(0); | ||
}); | ||
it.each([ | ||
[ | ||
'no schedule', | ||
{ | ||
storage: { | ||
storageMethod: 'sql', | ||
deleteOldBuilds: { | ||
dateRange: 30, | ||
}, | ||
}, | ||
}, | ||
], | ||
[ | ||
'no dateRagne', | ||
{ | ||
storage: { | ||
storageMethod: 'sql', | ||
deleteOldBuilds: { | ||
schedule: '0 * * * *', | ||
}, | ||
}, | ||
}, | ||
], | ||
])('should throw for invalid options (%s)', (_, options) => { | ||
expect(() => startDeletingOldBuildsCron(storageMethod, options)).toThrow(/Cannot configure/); | ||
}); | ||
it('should throw for invalid schedule', () => { | ||
const options = { | ||
storage: { | ||
storageMethod: 'sql', | ||
deleteOldBuilds: { | ||
schedule: '* * *', | ||
dateRange: 30, | ||
}, | ||
}, | ||
}; | ||
expect(() => startDeletingOldBuildsCron(storageMethod, options)).toThrow( | ||
/Invalid cron format/ | ||
); | ||
}); | ||
it('should schedule a cron job', () => { | ||
startDeletingOldBuildsCron(storageMethod, { | ||
storage: { | ||
storageMethod: 'sql', | ||
deleteOldBuilds: { | ||
schedule: '0 * * * *', | ||
dateRange: 30, | ||
}, | ||
}, | ||
}); | ||
expect(cronJob).toHaveBeenCalledTimes(1); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
'use strict'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add license comment |
||
|
||
/* eslint-env jest */ | ||
|
||
const {normalizeCronSchedule} = require('../../src/cron/utils.js'); | ||
|
||
describe('cron/utils', () => { | ||
describe('.normalizeCronSchedule()', () => { | ||
it('should validate string', () => { | ||
expect(() => normalizeCronSchedule(1)).toThrow(/Schedule must be provided/); | ||
}); | ||
|
||
it('should validate cron job', () => { | ||
expect(() => normalizeCronSchedule('* * * * *')).toThrow(/too frequent/); | ||
}); | ||
|
||
it('should validate invalid format', () => { | ||
expect(() => normalizeCronSchedule('* * *')).toThrow(/Invalid cron format/); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,6 @@ declare global { | |
|
||
export interface Options { | ||
url?: string | string[]; | ||
autodiscoverUrlBlocklist?: string | string[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert this please :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is my mistake! I overlooked that. Thanks. |
||
psiApiKey?: string; | ||
psiApiEndpoint?: string; | ||
staticDistDir?: string; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,10 @@ declare global { | |
sqlConnectionUrl?: string; | ||
sqlDangerouslyResetDatabase?: boolean; | ||
sequelizeOptions?: import('sequelize').Options; | ||
deleteOldBuilds?: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure this belongs at the can we move up to WDYT about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function only works when the storage option is 'sql', so I put it in |
||
schedule: string; | ||
dateRange: number; | ||
} | ||
} | ||
|
||
export interface Options { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findBuildsBeforeTimestamp
maybe?