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.

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

BC-7561 - separate job for failed batch deletion requests #5448

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
ca7185d
BC-7561 - a bit of cleanup
virgilchiriac Jan 15, 2025
dba8d83
BC-7561 - improve logic for bulk deletion
virgilchiriac Jan 20, 2025
a364574
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 20, 2025
ee088b5
fix api tets and change sorting
virgilchiriac Jan 20, 2025
e3b952c
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 20, 2025
fc5ce93
Fix some tests
virgilchiriac Jan 21, 2025
da3d694
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 21, 2025
3772528
split pending and failed into a separate job
virgilchiriac Jan 22, 2025
42127e2
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 22, 2025
7b8eccf
implement type casting for command flags
virgilchiriac Jan 22, 2025
9745671
fix linter / remove tests
virgilchiriac Jan 22, 2025
26444f0
unset db debug mode
virgilchiriac Jan 22, 2025
d10659a
undo flag in console
virgilchiriac Jan 22, 2025
e8ab582
fix test
virgilchiriac Jan 23, 2025
eb96b3f
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
8b8e624
fix tests
virgilchiriac Jan 23, 2025
3cc6551
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
86bfdcf
add cronjob
virgilchiriac Jan 23, 2025
51ea68e
fix test
virgilchiriac Jan 23, 2025
3114233
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 23, 2025
ec23c1d
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 24, 2025
e80920a
improve test cov
virgilchiriac Jan 24, 2025
2ec2d7d
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 24, 2025
a42c73b
fixes from revviews
virgilchiriac Jan 24, 2025
e216fc9
fix query
virgilchiriac Jan 24, 2025
92dc292
hacky type casting for console args
virgilchiriac Jan 27, 2025
e73ff3a
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
d070847
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
88906bd
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 27, 2025
a7e13c7
undo package.json command
virgilchiriac Jan 27, 2025
3d25423
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 27, 2025
c40c290
throttle down max number of processing requests in batch
virgilchiriac Jan 28, 2025
e6e8051
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 28, 2025
6adc016
fix max concurency / limit and some tests
virgilchiriac Jan 29, 2025
90dc237
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Jan 29, 2025
39a1d67
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Jan 29, 2025
b2a607b
optimization
virgilchiriac Jan 29, 2025
b6d183a
Merge branch 'main' into BC-7561-batch-delete-fix
virgilchiriac Feb 3, 2025
490e35b
Merge branch 'BC-7561-batch-delete-fix' of https://github.com/hpi-sch…
virgilchiriac Feb 3, 2025
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
18 changes: 18 additions & 0 deletions ansible/roles/schulcloud-server-core/tasks/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,24 @@
tags:
- configmap

- name: Data deletion trigger failed CronJob
kubernetes.core.k8s:
kubeconfig: ~/.kube/config
namespace: "{{ NAMESPACE }}"
template: data-deletion-trigger-failed-cronjob.yml.j2
virgilchiriac marked this conversation as resolved.
Show resolved Hide resolved
when: WITH_API_ADMIN is defined and WITH_API_ADMIN|bool
tags:
- cronjob

- name: Data deletion trigger failed CronJob ConfigMap
kubernetes.core.k8s:
kubeconfig: ~/.kube/config
namespace: "{{ NAMESPACE }}"
template: data-deletion-trigger-failed-cronjob-configmap.yml.j2
when: WITH_API_ADMIN is defined and WITH_API_ADMIN|bool
tags:
- configmap

- name: amqp files storage Deployment
kubernetes.core.k8s:
kubeconfig: ~/.kube/config
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
apiVersion: v1
kind: ConfigMap
metadata:
namespace: {{ NAMESPACE }}
name: data-deletion-trigger-failed-cronjob-configmap
labels:
app: data-deletion-trigger-failed-cronjob
data:
NODE_OPTIONS: "--max-old-space-size=3072"
NEST_LOG_LEVEL: "error"
EXIT_ON_ERROR: "true"
SC_DOMAIN: "{{ DOMAIN }}"
FEATURE_PROMETHEUS_METRICS_ENABLED: "true"
ETHERPAD__PAD_URI: "https://{{ DOMAIN }}/etherpad/p"

Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
apiVersion: batch/v1
kind: CronJob
metadata:
namespace: {{ NAMESPACE }}
labels:
app: data-deletion-trigger-failed
app.kubernetes.io/part-of: schulcloud-verbund
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
app.kubernetes.io/name: data-deletion-trigger-failed
app.kubernetes.io/component: data-deletion
app.kubernetes.io/managed-by: ansible
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }}
name: data-deletion-trigger-failed-cronjob
spec:
concurrencyPolicy: Forbid
schedule: "{{ SERVER_DATA_DELETION_TRIGGER_FAILED_CRONJOB_SCHEDULE|default("@daily", true) }}"
jobTemplate:
metadata:
labels:
app: data-deletion-trigger-failed
app.kubernetes.io/part-of: schulcloud-verbund
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
app.kubernetes.io/name: data-deletion-trigger-failed
app.kubernetes.io/component: data-deletion
app.kubernetes.io/managed-by: ansible
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }}
spec:
template:
spec:
securityContext:
runAsUser: 1000
runAsGroup: 1000
fsGroup: 1000
runAsNonRoot: true
containers:
- name: data-deletion-trigger-failed-cronjob
image: {{ SCHULCLOUD_SERVER_IMAGE }}:{{ SCHULCLOUD_SERVER_IMAGE_TAG }}
envFrom:
- configMapRef:
name: data-deletion-trigger-failed-cronjob-configmap
- secretRef:
name: admin-api-client-secret
- secretRef:
name: api-files-secret
command: ['/bin/sh', '-c']
args: ['npm run nest:start:deletion-console -- execution trigger -f true']
resources:
limits:
cpu: {{ API_CPU_LIMITS|default("2000m", true) }}
memory: {{ API_MEMORY_LIMITS|default("2Gi", true) }}
requests:
cpu: {{ API_CPU_REQUESTS|default("100m", true) }}
memory: {{ API_MEMORY_REQUESTS|default("150Mi", true) }}
restartPolicy: OnFailure
{% if AFFINITY_ENABLE is defined and AFFINITY_ENABLE|bool %}
affinity:
podAffinity:
preferredDuringSchedulingIgnoredDuringExecution:
- weight: 20
podAffinityTerm:
labelSelector:
matchExpressions:
- key: app.kubernetes.io/part-of
operator: In
values:
- schulcloud-verbund
topologyKey: "kubernetes.io/hostname"
namespaceSelector: {}
- weight: 10
podAffinityTerm:
labelSelector:
matchExpressions:
- key: git.repo
operator: In
values:
- {{ SCHULCLOUD_SERVER_REPO_NAME }}
topologyKey: "kubernetes.io/hostname"
namespaceSelector: {}
- weight: 10
podAffinityTerm:
labelSelector:
matchExpressions:
- key: git.branch
operator: In
values:
- {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
topologyKey: "kubernetes.io/hostname"
namespaceSelector: {}
- weight: 10
podAffinityTerm:
labelSelector:
matchExpressions:
- key: app.kubernetes.io/version
operator: In
values:
- {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
topologyKey: "kubernetes.io/hostname"
namespaceSelector: {}
{% endif %}
metadata:
labels:
app: data-deletion-trigger-failed
app.kubernetes.io/part-of: schulcloud-verbund
app.kubernetes.io/version: {{ SCHULCLOUD_SERVER_IMAGE_TAG }}
app.kubernetes.io/name: data-deletion-trigger-failed
app.kubernetes.io/component: data-deletion
app.kubernetes.io/managed-by: ansible
git.branch: {{ SCHULCLOUD_SERVER_BRANCH_NAME }}
git.repo: {{ SCHULCLOUD_SERVER_REPO_NAME }}
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
import { TriggerDeletionExecutionOptions } from '../interface';
import { TriggerDeletionExecutionOptionsBuilder } from './trigger-deletion-execution-options.builder';

describe(TriggerDeletionExecutionOptionsBuilder.name, () => {
describe(TriggerDeletionExecutionOptionsBuilder.build.name, () => {
describe('when called with proper arguments', () => {
const setup = () => {
const limit = 1000;
const runFailed = false;

const expectedOutput: TriggerDeletionExecutionOptions = { limit };

return { limit, expectedOutput };
return { limit, runFailed };
};

it('should return valid object with expected values', () => {
const { limit, expectedOutput } = setup();
const { limit, runFailed } = setup();

const output = TriggerDeletionExecutionOptionsBuilder.build(limit);
const output = TriggerDeletionExecutionOptionsBuilder.build(limit, runFailed);

expect(output).toStrictEqual(expectedOutput);
expect(output).toStrictEqual({ limit, runFailed });
});
});
});
Expand Down
virgilchiriac marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { TriggerDeletionExecutionOptions } from '../interface';

export class TriggerDeletionExecutionOptionsBuilder {
static build(limit: number): TriggerDeletionExecutionOptions {
return { limit };
static build(limit: number, runFailed: boolean): TriggerDeletionExecutionOptions {
return { limit, runFailed };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -249,5 +249,28 @@ describe(DeletionClient.name, () => {
await expect(client.executeDeletions()).rejects.toThrow(Error);
});
});

describe('when runFailed is true', () => {
const setup = () => {
setupConfig();

const response: AxiosResponse<DeletionRequestOutput> = axiosResponseFactory.build({
status: 204,
});

httpService.post.mockReturnValueOnce(of(response));

const fullUrl = 'http://api-admin:4030/admin/api/v1/deletionExecutions?runFailed=true';

return { fullUrl };
};
it('should call endpoint with runFailed param', async () => {
const { fullUrl } = setup();

await client.executeDeletions(undefined, true);

expect(httpService.post).toHaveBeenCalledWith(fullUrl, null, expect.any(Object));
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ export class DeletionClient {
}
}

public async executeDeletions(limit?: number): Promise<void> {
public async executeDeletions(limit?: number, runFailed?: boolean): Promise<void> {
Copy link
Contributor

@uidp uidp Jan 27, 2025

Choose a reason for hiding this comment

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

Would it make sense to use an options object type instead of sereate parameters? Would better show the independence of limit and runFailed IMHO. (maybe also a matter of taste)

try {
const response = await this.postDeletionExecutionRequest(limit);
const response = await this.postDeletionExecutionRequest(limit, runFailed);
this.checkResponseStatusCode(response, HttpStatusCode.NoContent);
} catch (err) {
throw this.createError(err, 'executeDeletions');
Expand All @@ -46,13 +46,25 @@ export class DeletionClient {
return response;
}

private async postDeletionExecutionRequest(limit?: number): Promise<AxiosResponse<any, any>> {
private async postDeletionExecutionRequest(limit?: number, runFailed?: boolean): Promise<AxiosResponse<any, any>> {
const defaultHeaders = this.createDefaultHeaders();
const headers = this.isLimitGeaterZero(limit) ? { ...defaultHeaders, params: { limit } } : defaultHeaders;

const baseUrl = this.configService.get('ADMIN_API_CLIENT_BASE_URL', { infer: true });
const postDeletionExecutionsEndpoint = new URL('/admin/api/v1/deletionExecutions', baseUrl).toString();
const endpoint = '/admin/api/v1/deletionExecutions';

const postDeletionExecutionsEndpoint = new URL(endpoint, baseUrl);

const params = new URLSearchParams(postDeletionExecutionsEndpoint.search);
if (limit && this.isLimitGreaterZero(limit)) {
params.append('limit', limit.toString());
}
if (runFailed) {
params.append('runFailed', runFailed.toString());
}

const fullUrl = `${postDeletionExecutionsEndpoint.toString()}?${params.toString()}`;

const request = this.httpService.post(postDeletionExecutionsEndpoint, null, headers);
const request = this.httpService.post(fullUrl, null, defaultHeaders);
const response = await firstValueFrom(request);

return response;
Expand Down Expand Up @@ -87,7 +99,7 @@ export class DeletionClient {
}
}

private isLimitGeaterZero(limit?: number): boolean {
private isLimitGreaterZero(limit?: number): boolean {
return typeof limit === 'number' && limit > 0;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { DeletionExecutionConsole } from './deletion-execution.console';
import { DeletionExecutionTriggerResultBuilder, TriggerDeletionExecutionOptionsBuilder } from './builder';
import { DeletionExecutionUc } from './uc';
import { DeletionConsoleModule } from './deletion-console.app.module';
import { TriggerDeletionExecutionOptions } from './interface';

describe(DeletionExecutionConsole.name, () => {
let module: TestingModule;
Expand Down Expand Up @@ -38,7 +39,7 @@ describe(DeletionExecutionConsole.name, () => {
const setup = () => {
const limit = 1000;

const options = TriggerDeletionExecutionOptionsBuilder.build(1000);
const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false);

return { limit, options };
};
Expand All @@ -50,13 +51,13 @@ describe(DeletionExecutionConsole.name, () => {

await console.triggerDeletionExecution(options);

expect(spy).toBeCalledWith(limit);
expect(spy).toBeCalledWith(limit, false);
});
});

describe(`when ${DeletionExecutionUc.name}'s triggerDeletionExecution() method doesn't throw an exception`, () => {
const setup = () => {
const options = TriggerDeletionExecutionOptionsBuilder.build(1000);
const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false);

deletionExecutionUc.triggerDeletionExecution.mockResolvedValueOnce(undefined);
const spy = jest.spyOn(DeletionExecutionTriggerResultBuilder, 'buildSuccess');
Expand All @@ -75,7 +76,7 @@ describe(DeletionExecutionConsole.name, () => {

describe(`when ${DeletionExecutionUc.name}'s triggerDeletionExecution() method throws an exception`, () => {
const setup = () => {
const options = TriggerDeletionExecutionOptionsBuilder.build(1000);
const options = TriggerDeletionExecutionOptionsBuilder.build(1000, false);
const err = new Error('some error occurred...');

deletionExecutionUc.triggerDeletionExecution.mockRejectedValueOnce(err);
Expand All @@ -92,5 +93,29 @@ describe(DeletionExecutionConsole.name, () => {
expect(spy).toHaveBeenCalledWith(err);
});
});

describe('when runFailed is given as string "true"', () => {
it('should convert runFailed to boolean', async () => {
const options = { limit: 1000, runFailed: 'true' } as unknown as TriggerDeletionExecutionOptions;

const spy = jest.spyOn(deletionExecutionUc, 'triggerDeletionExecution').mockResolvedValueOnce(undefined);

await console.triggerDeletionExecution(options);

expect(spy).toHaveBeenCalledWith(1000, true);
});
});

describe('when limit is a string "5"', () => {
it('should convert limit to number', async () => {
const options = { limit: '5', runFailed: false } as unknown as TriggerDeletionExecutionOptions;

const spy = jest.spyOn(deletionExecutionUc, 'triggerDeletionExecution').mockResolvedValueOnce(undefined);

await console.triggerDeletionExecution(options);

expect(spy).toHaveBeenCalledWith(5, false);
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,36 @@ export class DeletionExecutionConsole {
options: [
{
flags: '-l, --limit <value>',
// TODO implemnt fn with proper command testing
// fn: (value: string) => (value ? Number(value) : undefined),
description: 'Limit of the requested deletion executions that should be performed.',
required: false,
},
{
flags: '-f, --runFailed <value>',
// TODO implemnt fn with proper command testing
// fn: (value: string) => /^(true|yes|1)$/i.test(value),
description: 'Limit of the requested deletion executions that should be performed.',
required: false,
},
],
})
async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise<void> {
public async triggerDeletionExecution(options: TriggerDeletionExecutionOptions): Promise<void> {
// Try to trigger the deletion execution(s) via Deletion API client,
// return successful status in case of a success, otherwise return
// a result with a failure status and a proper error message.
let result: DeletionExecutionTriggerResult;

try {
await this.deletionExecutionUc.triggerDeletionExecution(options.limit ? Number(options.limit) : undefined);
if (typeof options.limit === 'string') {
options.limit = Number(options.limit);
}

if (typeof options.runFailed === 'string') {
options.runFailed = /^(true|yes|1)$/i.test(options.runFailed);
}

try {
await this.deletionExecutionUc.triggerDeletionExecution(options.limit, options.runFailed);
result = DeletionExecutionTriggerResultBuilder.buildSuccess();
} catch (err) {
result = DeletionExecutionTriggerResultBuilder.buildFailure(err as Error);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export interface TriggerDeletionExecutionOptions {
limit: number;
limit: string | number;
runFailed: string | boolean;
}
Loading
Loading