From 75a53b838a78124e48ab982d5ac7b268e71b0ed1 Mon Sep 17 00:00:00 2001 From: ovalice <31350208+ovalice@users.noreply.github.com> Date: Fri, 22 Mar 2024 15:57:41 +0100 Subject: [PATCH 1/4] Keep log stream open when running a container with KubernetesContainerRunner Signed-off-by: ovalice <31350208+ovalice@users.noreply.github.com> --- .changeset/wise-moons-begin.md | 5 +++++ .../util/KubernetesContainerRunner.test.ts | 19 +++++++++++++++++++ .../src/util/KubernetesContainerRunner.ts | 9 +++++++-- 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 .changeset/wise-moons-begin.md diff --git a/.changeset/wise-moons-begin.md b/.changeset/wise-moons-begin.md new file mode 100644 index 0000000000..ba7a8bbed0 --- /dev/null +++ b/.changeset/wise-moons-begin.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-common': patch +--- + +KubernetesContainerRunner.runContainer no longer closes the `logStream` is receives as input. diff --git a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts index e218abf960..7b55f8cd42 100644 --- a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts +++ b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts @@ -187,6 +187,25 @@ describeIfKubernetes('KubernetesContainerRunner', () => { ); }); + it('should not close the original log stream', async () => { + const options: KubernetesContainerRunnerOptions = { + kubeConfig, + name, + namespace: 'default', + }; + const containerRunner = new KubernetesContainerRunner(options); + const logStream = new PassThrough(); + const runOptions: RunContainerOptions = { + imageName: 'alpine', + args: ['echo', 'hello world'], + logStream, + }; + + await containerRunner.runContainer(runOptions); + + expect(logStream.closed).toBe(false); + }); + describe('with namespace test', () => { let api: CoreV1Api; let authApi: RbacAuthorizationV1Api; diff --git a/packages/backend-common/src/util/KubernetesContainerRunner.ts b/packages/backend-common/src/util/KubernetesContainerRunner.ts index 4a9c447430..4b235caf2c 100644 --- a/packages/backend-common/src/util/KubernetesContainerRunner.ts +++ b/packages/backend-common/src/util/KubernetesContainerRunner.ts @@ -132,12 +132,17 @@ export class KubernetesContainerRunner implements ContainerRunner { imageName, command, args, - logStream = new PassThrough(), + logStream, mountDirs = {}, workingDir, envVars = {}, } = options; + const containerLogStream = new PassThrough(); + if (logStream) { + containerLogStream.pipe(logStream, { end: false }); + } + const commandArr = typeof command === 'string' ? [command] : command; const volumeMounts: V1VolumeMount[] = []; @@ -209,7 +214,7 @@ export class KubernetesContainerRunner implements ContainerRunner { }, }; - await this.runJob(jobSpec, taskId, logStream); + await this.runJob(jobSpec, taskId, containerLogStream); } private handleError(err: any, errorCallback: (reason: any) => void) { From 8ccbdb2dbf2b0c6ddd648e40648c198be8a3cfed Mon Sep 17 00:00:00 2001 From: ovalice <31350208+ovalice@users.noreply.github.com> Date: Fri, 22 Mar 2024 16:21:57 +0100 Subject: [PATCH 2/4] Fix typo in changeset Signed-off-by: ovalice <31350208+ovalice@users.noreply.github.com> --- .changeset/wise-moons-begin.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/wise-moons-begin.md b/.changeset/wise-moons-begin.md index ba7a8bbed0..b1d9c165af 100644 --- a/.changeset/wise-moons-begin.md +++ b/.changeset/wise-moons-begin.md @@ -2,4 +2,4 @@ '@backstage/backend-common': patch --- -KubernetesContainerRunner.runContainer no longer closes the `logStream` is receives as input. +KubernetesContainerRunner.runContainer no longer closes the `logStream` it receives as input. From bc4ab9fcf69073211e9256105fbff56f5bddaebd Mon Sep 17 00:00:00 2001 From: ovalice <31350208+ovalice@users.noreply.github.com> Date: Sun, 7 Apr 2024 00:29:27 +0200 Subject: [PATCH 3/4] Use writableEnded to determine whether logStream was closed during test When asserting whether the logStream is closed, the .closed prop is not sufficient. The prop only becomes true when all the data has been flushed, which probably happens after we want to do the assertion. By using .writableEnded, we can properly check whether the logStream was intentionally closed, as it becomes true as soon as .end() is called. Signed-off-by: ovalice <31350208+ovalice@users.noreply.github.com> --- .../backend-common/src/util/KubernetesContainerRunner.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts index 7b55f8cd42..2be2ec64ef 100644 --- a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts +++ b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts @@ -203,7 +203,7 @@ describeIfKubernetes('KubernetesContainerRunner', () => { await containerRunner.runContainer(runOptions); - expect(logStream.closed).toBe(false); + expect(logStream.writableEnded).toBe(false); }); describe('with namespace test', () => { From 8b08e98feeef8f153a7b290c8798675b2433e894 Mon Sep 17 00:00:00 2001 From: ovalice <31350208+ovalice@users.noreply.github.com> Date: Sun, 7 Apr 2024 00:40:12 +0200 Subject: [PATCH 4/4] Also check for logStream.destroyed during test in case .end() was not called to close the logStream Signed-off-by: ovalice <31350208+ovalice@users.noreply.github.com> --- .../backend-common/src/util/KubernetesContainerRunner.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts index 2be2ec64ef..706b78c4ae 100644 --- a/packages/backend-common/src/util/KubernetesContainerRunner.test.ts +++ b/packages/backend-common/src/util/KubernetesContainerRunner.test.ts @@ -204,6 +204,7 @@ describeIfKubernetes('KubernetesContainerRunner', () => { await containerRunner.runContainer(runOptions); expect(logStream.writableEnded).toBe(false); + expect(logStream.destroyed).toBe(false); }); describe('with namespace test', () => {