Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Service installs: Avoid creating duplicate tasks on device registration #1867

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+124 −43
Open

Service installs: Avoid creating duplicate tasks on device registration #1867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6d22cd
Unify the POST hooks for app, supervisor & hostApp Service Install cr…
thgreasi Nov 27, 2024
39fb0d9
Test that only one create_service_install task is created per device …
thgreasi Nov 27, 2024
ad0c14d
Service installs: Avoid creating duplicate tasks on device registration
thgreasi Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 39 additions & 42 deletions src/features/ci-cd/hooks/service-installs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,13 @@ hooks.addPureHook('POST', 'resin', 'device', {
const app = request.values.belongs_to__application;
if (app != null) {
await createAppServiceInstalls(rootApi, app, [deviceId], tx);
if (ASYNC_TASKS_ENABLED && ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED only ever equal true if ASYNC_TASKS_ENABLED is also true? Maybe there was a discussion about this at some point, but I feel like system init should fail if ASYNC_TASKS_ENABLED === false && ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED === true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I agree on that this was strange. I expected it to only have to check one of the two, maybe something like:

export let ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED = ASYNC_TASKS_ENABLED && boolVar(
	'ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED',
	false,
);

But since I don't have background on why it was done this way, I just copied it "as is" from above where we do the same check. I guess that it might be this way to make mocking in tests easier. I would prefer to PR such a change separately and fix all occurrences and whichever tests might break after having a short chat on it.

// When creating service installs via the async tasks, a single call to createServiceInstallsAsync()
// creates all service installs (target release, hostApp & SV release), so we don't need
// to run the extra code below to handle them separately, and doing so would create
// duplicate & unnecessary tasks.
return;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I expect that soon tasks will become the only supported way for creating service installs, I decided to fix this in the simplest way that required the less changes in the current code.

}
}

const release = request.values.is_pinned_on__release;
Expand All @@ -273,6 +280,23 @@ hooks.addPureHook('POST', 'resin', 'device', {
tx,
);
}

const svAndHostAppReleaseIds = [
request.values.should_be_managed_by__release,
request.values.should_be_operated_by__release,
].filter((releaseId) => releaseId != null);

if (svAndHostAppReleaseIds.length > 0) {
// Create supervisor/hostApp service installs when the supervisor/hostApp is pinned on device creation
await createReleaseServiceInstalls(
rootApi,
[deviceId],
{
id: { $in: svAndHostAppReleaseIds },
},
tx,
);
}
},
});

Expand Down Expand Up @@ -346,40 +370,20 @@ hooks.addPureHook('PATCH', 'resin', 'device', {
},
});

const addSystemAppServiceInstallHooks = (
fieldName: 'should_be_managed_by__release' | 'should_be_operated_by__release',
) => {
hooks.addPureHook('POST', 'resin', 'device', {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this higher, in the hooks.addPureHook('POST', 'resin', 'device') that we have for user apps.

POSTRUN: async ({ request, api, tx, result: deviceId }) => {
// Don't try to add service installs if the device wasn't created
if (typeof deviceId !== 'number') {
return;
}

const releaseId = request.values[fieldName];
// Create supervisor/hostApp service installs when the supervisor/hostApp is pinned on device creation
if (releaseId != null) {
const rootApi = api.clone({
passthrough: { tx, req: permissions.root },
});
await createReleaseServiceInstalls(
rootApi,
[deviceId],
{
id: releaseId,
},
tx,
);
}
},
});

hooks.addPureHook('PATCH', 'resin', 'device', {
POSTRUN: async ({ api, request, tx }) => {
const affectedIds = request.affectedIds!;
hooks.addPureHook('PATCH', 'resin', 'device', {
POSTRUN: async ({ api, request, tx }) => {
const affectedIds = request.affectedIds!;
if (affectedIds.length === 0) {
return;
}
// add system app service install PATCH hooks
for (const fieldName of [
'should_be_managed_by__release',
'should_be_operated_by__release',
] as const) {
const releaseId = request.values[fieldName];
// Create supervisor/hostApp service installs when the supervisor/hostApp is pinned on device update
if (releaseId != null && affectedIds.length !== 0) {
if (releaseId != null) {
await createReleaseServiceInstalls(
api,
affectedIds,
Expand All @@ -389,13 +393,6 @@ const addSystemAppServiceInstallHooks = (
tx,
);
}
},
});
};

for (const fieldName of [
'should_be_managed_by__release',
'should_be_operated_by__release',
] as const) {
addSystemAppServiceInstallHooks(fieldName);
}
}
},
});
59 changes: 58 additions & 1 deletion test/25_service-installs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ export default () => {
ctx.app1Service3 = fx.services.app1Service3;
ctx.app2Service1 = fx.services.app2Service1;
ctx.app2Service2 = fx.services.app2Service2;
ctx.intelNucHostAppService1 =
fx.services['intel-nuc-host-appService1'];

config.TEST_MOCK_ONLY.ASYNC_TASK_CREATE_SERVICE_INSTALLS_ENABLED =
isServiceInstallEnabled;
Expand Down Expand Up @@ -92,6 +94,61 @@ export default () => {
);
});

it('when a device is created with an os release specified', async () => {
const { body: provisioningKey } = await supertest(ctx.admin)
.post(`/api-key/application/${ctx.app1.id}/provisioning`)
.expect(200);
const uuid = fakeDevice.generateDeviceUuid();
const { body: deviceWithOsVersion } = await supertest()
.post(`/device/register?apikey=${provisioningKey}`)
.send({
user: ctx.admin.id,
application: ctx.app1.id,
device_type: 'intel-nuc',
uuid,
os_version: 'balenaOS 2.88.4',
os_variant: 'prod',
})
.expect(201);
ctx.deviceWithOsVersion = deviceWithOsVersion;

await expectToEventually(async () => {
const { body: serviceInstalls } = await pineUser
.get({
resource: 'service_install',
options: {
$select: ['device', 'installs__service'],
$filter: {
device: deviceWithOsVersion.id,
},
$orderby: { installs__service: 'asc' },
},
})
.expect(200);
expect(serviceInstalls).to.deep.equal(
[ctx.app1Service1.id, ctx.intelNucHostAppService1.id]
.sort((a, b) => a - b)
.map((serviceInstallId) => ({
device: { __id: deviceWithOsVersion.id },
installs__service: { __id: serviceInstallId },
})),
);
});
await expectNewTasks(
'create_service_installs',
isServiceInstallEnabled
? [
{
is_executed_with__parameter_set: {
devices: [deviceWithOsVersion.id],
},
status: 'succeeded',
},
]
: [],
);
});

it('for pinning an application to a release', async () => {
await pineUser
.patch({
Expand Down Expand Up @@ -126,7 +183,7 @@ export default () => {
? [
{
is_executed_with__parameter_set: {
devices: [ctx.device.id],
devices: [ctx.device.id, ctx.deviceWithOsVersion.id],
},
status: 'succeeded',
},
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/25-service-installs/applications.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
{
"intel-nuc-host-app": {
"user": "admin",
"is_host": true,
"is_public": true,
"device_type": "intel-nuc",
"app_name": "intel-nuc"
},
"app1": {
"user": "admin",
"device_type": "intel-nuc",
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/25-service-installs/images.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
{
"intel-nuc-host-app-release2-88-4Image": {
"user": "admin",
"service": "intel-nuc-host-appService1",
"releases": [ "intel-nuc-host-app-release2-88-4" ],
"project_type": "Dockerfile.template",
"status": "success"
},
"image1": {
"user": "admin",
"service": "app1Service1",
Expand Down
8 changes: 8 additions & 0 deletions test/fixtures/25-service-installs/releases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
{
"intel-nuc-host-app-release2-88-4": {
"application": "intel-nuc-host-app",
"composition": {},
"source": "cloud",
"status": "success",
"semver": "2.88.4",
"user": "admin"
},
"release1": {
"application": "app1",
"commit": "de2dc0de",
Expand Down
5 changes: 5 additions & 0 deletions test/fixtures/25-service-installs/services.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
{
"intel-nuc-host-appService1": {
"service_name": "main",
"application": "intel-nuc-host-app",
"user": "admin"
},
"app1Service1": {
"user": "admin",
"application": "app1",
Expand Down
Loading