Skip to content

Commit

Permalink
Merge pull request #1857 from balena-io/fix-double-task-on-move
Browse files Browse the repository at this point in the history 
Service installs: Fix creating two tasks when moving a device
  • Loading branch information
@flowzone-app
flowzone-app[bot] authored Nov 26, 2024
2 parents fd428e5 + be81dab commit c2fdca5
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 10 deletions.
23 changes: 15 additions & 8 deletions src/features/ci-cd/hooks/service-installs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,18 +287,25 @@ hooks.addPureHook('PATCH', 'resin', 'device', {
const affectedIds = await sbvrUtils.getAffectedIds(args);
if (affectedIds.length !== 0) {
await deleteServiceInstallsForCurrentApp(args.api, newAppId, affectedIds);
await createAppServiceInstalls(args.api, newAppId, affectedIds, args.tx);
}
},
});

hooks.addPureHook('PATCH', 'resin', 'device', {
POSTRUN: async ({ api, request, tx }) => {
const affectedIds = request.affectedIds!;
if (
request.values.is_pinned_on__release !== undefined &&
affectedIds.length !== 0
) {
if (affectedIds.length === 0) {
return;
}
const newAppId = request.values.belongs_to__application;
if (newAppId != null) {
// We could also have an optimization for the case that `values.is_pinned_on__release != null`
// to make the part that finds the target release faster, but chose to keep this simpler since:
// a) We expect that in the majority of device move requests users will not be also be pinning
// the device at the same time.
// b) Only the sync approach would benefit from it, since creating the service installs via tasks
// (which is going to be the default), only accepts the deviceIds as a parameter,
await createAppServiceInstalls(api, newAppId, affectedIds, tx);
return;
}
if (request.values.is_pinned_on__release !== undefined) {
// If the device was preloaded, and then pinned, service_installs do not exist
// for this device+release combination. We need to create these
if (request.values.is_pinned_on__release != null) {
Expand Down
63 changes: 63 additions & 0 deletions test/25_service-installs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as fixtures from './test-lib/fixtures.js';
import { assertExists, expectToEventually } from './test-lib/common.js';
import * as config from '../src/lib/config.js';
import { supertest } from './test-lib/supertest.js';
import { expectNewTasks, resetLatestTaskIds } from './test-lib/api-helpers.js';

export default () => {
versions.test((version, pineTest) => {
Expand Down Expand Up @@ -45,6 +46,8 @@ export default () => {

config.TEST_MOCK_ONLY.ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED =
isServiceInstallEnabled;

await resetLatestTaskIds('create_service_installs');
});

after(async () => {
Expand Down Expand Up @@ -74,6 +77,19 @@ export default () => {
ctx.app1Service1.id,
);
});
await expectNewTasks(
'create_service_installs',
isServiceInstallEnabled
? [
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
},
status: 'succeeded',
},
]
: [],
);
});

it('for pinning an application to a release', async () => {
Expand Down Expand Up @@ -104,6 +120,19 @@ export default () => {
ctx.app1Service2.id,
);
});
await expectNewTasks(
'create_service_installs',
isServiceInstallEnabled
? [
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
},
status: 'succeeded',
},
]
: [],
);
});

it('when a device is pinned on a different release', async () => {
Expand Down Expand Up @@ -133,6 +162,19 @@ export default () => {
ctx.app1Service3.id,
);
});
await expectNewTasks(
'create_service_installs',
isServiceInstallEnabled
? [
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
},
status: 'succeeded',
},
]
: [],
);
});

it('when device is moved to different application', async () => {
Expand Down Expand Up @@ -170,6 +212,27 @@ export default () => {
ctx.app2Service1.id,
);
});
await expectNewTasks(
'create_service_installs',
isServiceInstallEnabled
? [
// the first one is from unpinning
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
},
status: 'succeeded',
},
// the second one is from moving application
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
},
status: 'succeeded',
},
]
: [],
);
});

it('should be able to use service_install to create a device_service_environment_variable', async () => {
Expand Down
89 changes: 87 additions & 2 deletions test/test-lib/api-helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ import type { UserObjectParam } from '../test-lib/supertest.js';
import { supertest } from '../test-lib/supertest.js';
import type { TokenUserPayload } from '../../src/index.js';
import type { RequiredField } from '@balena/pinejs/out/sbvr-api/common-types.js';
import { assertExists } from './common.js';
import { assertExists, expectToEventually } from './common.js';
import { sbvrUtils, permissions } from '@balena/pinejs';
import type { tasks } from '@balena/pinejs';

const version = 'resin';

Expand Down Expand Up @@ -208,7 +210,7 @@ export const thatIsDateStringAfter = (

const validJwtProps = ['id', 'jwt_secret', 'authTime', 'iat', 'exp'].sort();

export function expectJwt(tokenOrJwt: string | AnyObject) {
export function expectJwt(tokenOrJwt: string | object) {
const decoded = (
typeof tokenOrJwt === 'string'
? jsonwebtoken.decode(tokenOrJwt)
Expand All @@ -232,3 +234,86 @@ export function expectJwt(tokenOrJwt: string | AnyObject) {

return decoded;
}

export type TaskExpectation = Pick<
tasks.Task['Read'],
'is_executed_with__parameter_set' | 'status'
>;

const expectTasks = async (
handler: string,
expectedTasks: TaskExpectation[],
lastId = '0',
) => {
const tasks = await sbvrUtils.api.tasks.get({
resource: 'task',
passthrough: { req: permissions.rootRead },
options: {
$select: ['id', 'is_executed_with__parameter_set', 'status'],
$filter: {
id: { $gt: lastId },
is_executed_by__handler: handler,
},
$orderby: { id: 'asc' },
},
});
const actual = tasks.map((t) =>
_.pick(t, ['is_executed_with__parameter_set', 'status']),
);

expect(actual).to.deep.equal(expectedTasks);
return tasks;
};

const latestTaskIdByHandler: Dictionary<string | undefined> = {};

export const expectNewTasks = async (
handler: string,
expectedUsage: TaskExpectation[],
) => {
const tasks = await expectTasks(
handler,
expectedUsage,
latestTaskIdByHandler[handler],
);
const lastTaskId = tasks.at(-1)?.id;
if (lastTaskId != null) {
latestTaskIdByHandler[handler] = lastTaskId;
}
};

export const resetLatestTaskIds = async (handler: string) => {
await expectToEventually(async () => {
const runningTasks = await sbvrUtils.api.tasks.get({
resource: 'task',
passthrough: { req: permissions.rootRead },
options: {
$top: 1,
$select: 'id',
$filter: {
status: 'queued',
is_executed_by__handler: handler,
},
$orderby: { id: 'desc' },
},
});
expect(runningTasks).to.have.lengthOf(0);
});
const [latestTask] = await sbvrUtils.api.tasks.get({
resource: 'task',
passthrough: { req: permissions.rootRead },
options: {
$top: 1,
$select: 'id',
$filter: {
status: 'succeeded',
is_executed_by__handler: handler,
},
$orderby: { id: 'desc' },
},
});
if (latestTask == null) {
return;
}
latestTaskIdByHandler[handler] = latestTask?.id;
};

0 comments on commit c2fdca5

Please sign in to comment.