From 734d11a226d30e0fa39a8bf0b09332814158acd2 Mon Sep 17 00:00:00 2001 From: axel7083 <42176370+axel7083@users.noreply.github.com> Date: Mon, 27 May 2024 10:45:24 +0200 Subject: [PATCH] refactor(ApplicationManager): remove unused interfaces (#1114) * refactor: remove unused interfaces Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com> * fix: restart not starting Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com> --------- Signed-off-by: axel7083 <42176370+axel7083@users.noreply.github.com> --- .../src/managers/applicationManager.spec.ts | 74 +++--- .../src/managers/applicationManager.ts | 214 +++++++++--------- .../src/managers/recipes/PodManager.spec.ts | 38 ++++ .../src/managers/recipes/PodManager.ts | 7 + 4 files changed, 182 insertions(+), 151 deletions(-) diff --git a/packages/backend/src/managers/applicationManager.spec.ts b/packages/backend/src/managers/applicationManager.spec.ts index 63c8f5dd5..da615a1f7 100644 --- a/packages/backend/src/managers/applicationManager.spec.ts +++ b/packages/backend/src/managers/applicationManager.spec.ts @@ -16,13 +16,7 @@ * SPDX-License-Identifier: Apache-2.0 ***********************************************************************/ import { describe, expect, test, vi, beforeEach } from 'vitest'; -import { - type ContainerAttachedInfo, - type ImageInfo, - type ApplicationPodInfo, - ApplicationManager, - CONFIG_FILENAME, -} from './applicationManager'; +import { type ImageInfo, ApplicationManager, CONFIG_FILENAME } from './applicationManager'; import type { GitManager } from './gitManager'; import os from 'os'; import fs, { type PathLike } from 'node:fs'; @@ -151,6 +145,7 @@ const podManager = { findPodByLabelsValues: vi.fn(), getPodsWithLabels: vi.fn(), getHealth: vi.fn(), + getPod: vi.fn(), } as unknown as PodManager; const localRepositoryRegistry = { @@ -283,6 +278,10 @@ describe('pullApplication', () => { recipeFolderExists: false, }); vi.mocked(podManager.getAllPods).mockResolvedValue([]); + vi.mocked(podManager.getPod).mockResolvedValue({ + engineId: 'dummyEngineId', + Id: 'dummyPodId', + } as unknown as PodInfo); vi.spyOn(podman, 'isQEMUMachine').mockResolvedValue(false); vi.spyOn(modelsManager, 'isModelOnDisk').mockReturnValue(false); vi.spyOn(modelsManager, 'uploadModelToPodmanMachine').mockResolvedValue('path'); @@ -428,6 +427,10 @@ describe('pullApplication', () => { recipeFolderExists: true, }); vi.mocked(podManager.getAllPods).mockResolvedValue([]); + vi.mocked(podManager.getPod).mockResolvedValue({ + engineId: 'dummyEngineId', + Id: 'dummyPodId', + } as unknown as PodInfo); vi.spyOn(modelsManager, 'isModelOnDisk').mockReturnValue(true); vi.spyOn(modelsManager, 'uploadModelToPodmanMachine').mockResolvedValue('path'); vi.spyOn(modelsManager, 'getLocalModelPath').mockReturnValue('path'); @@ -801,17 +804,15 @@ describe('createApplicationPod', () => { }); }); test('call createAndAddContainersToPod after pod is created', async () => { - const pod: ApplicationPodInfo = { + const pod: PodInfo = { engineId: 'engine', Id: 'id', - portmappings: [], - }; + } as unknown as PodInfo; vi.spyOn(manager, 'createPod').mockResolvedValue(pod); + // mock createContainerAndAttachToPod const createAndAddContainersToPodMock = vi - .spyOn(manager, 'createAndAddContainersToPod') - .mockImplementation((_pod: ApplicationPodInfo, _images: ImageInfo[], _modelInfo: ModelInfo, _modelPath: string) => - Promise.resolve([]), - ); + .spyOn(manager, 'createContainerAndAttachToPod') + .mockResolvedValue(undefined); await manager.createApplicationPod({ id: 'recipe-id' } as Recipe, { id: 'model-id' } as ModelInfo, images, 'path'); expect(createAndAddContainersToPodMock).toBeCalledWith(pod, images, { id: 'model-id' }, 'path'); expect(mocks.updateTaskMock).toBeCalledWith({ @@ -824,13 +825,12 @@ describe('createApplicationPod', () => { }); }); test('throw if createAndAddContainersToPod fails', async () => { - const pod: ApplicationPodInfo = { + const pod: PodInfo = { engineId: 'engine', Id: 'id', - portmappings: [], - }; + } as unknown as PodInfo; vi.spyOn(manager, 'createPod').mockResolvedValue(pod); - vi.spyOn(manager, 'createAndAddContainersToPod').mockRejectedValue('error'); + vi.spyOn(manager, 'createContainerAndAttachToPod').mockRejectedValue('error'); await expect(() => manager.createApplicationPod({ id: 'recipe-id' } as Recipe, { id: 'model-id' } as ModelInfo, images, 'path'), ).rejects.toThrowError('error'); @@ -860,42 +860,22 @@ describe('runApplication', () => { builderManager, podManager, ); - const pod: ApplicationPodInfo = { + const pod: PodInfo = { engineId: 'engine', Id: 'id', - containers: [ - { - name: 'first', - modelService: false, - ports: ['8080', '8081'], - }, - { - name: 'second', - modelService: true, - ports: ['9000'], - }, - ], - portmappings: [ + Containers: [ { - container_port: 9000, - host_port: 9001, - host_ip: '', - protocol: '', - range: -1, + Id: 'dummyContainerId', }, ], - }; + } as unknown as PodInfo; test('check startPod is called and also waitContainerIsRunning for sample app', async () => { - const waitContainerIsRunningMock = vi - .spyOn(manager, 'waitContainerIsRunning') - .mockImplementation((_engineId: string, _container: ContainerAttachedInfo) => Promise.resolve()); + const waitContainerIsRunningMock = vi.spyOn(manager, 'waitContainerIsRunning').mockResolvedValue(undefined); vi.spyOn(utils, 'timeout').mockResolvedValue(); await manager.runApplication(pod); expect(mocks.startPod).toBeCalledWith(pod.engineId, pod.Id); expect(waitContainerIsRunningMock).toBeCalledWith(pod.engineId, { - name: 'first', - modelService: false, - ports: ['8080', '8081'], + Id: 'dummyContainerId', }); }); }); @@ -914,11 +894,11 @@ describe('createAndAddContainersToPod', () => { builderManager, podManager, ); - const pod: ApplicationPodInfo = { + const pod: PodInfo = { engineId: 'engine', Id: 'id', portmappings: [], - }; + } as unknown as PodInfo; const imageInfo1: ImageInfo = { id: 'id', appName: 'appName', @@ -936,7 +916,7 @@ describe('createAndAddContainersToPod', () => { id: 'container-1', }); vi.spyOn(podman, 'isQEMUMachine').mockResolvedValue(false); - await manager.createAndAddContainersToPod(pod, [imageInfo1, imageInfo2], modelInfo, 'path'); + await manager.createContainerAndAttachToPod(pod, [imageInfo1, imageInfo2], modelInfo, 'path'); expect(mocks.createContainerMock).toHaveBeenNthCalledWith(1, 'engine', { Image: 'id', Detach: true, diff --git a/packages/backend/src/managers/applicationManager.ts b/packages/backend/src/managers/applicationManager.ts index 22cbc2773..0d46f2a45 100644 --- a/packages/backend/src/managers/applicationManager.ts +++ b/packages/backend/src/managers/applicationManager.ts @@ -28,6 +28,7 @@ import type { Webview, HostConfig, HealthConfig, + PodContainerInfo, } from '@podman-desktop/api'; import type { AIConfig, AIConfigFile, ContainerConfig } from '../models/AIConfig'; import { parseYamlFile } from '../models/AIConfig'; @@ -65,19 +66,6 @@ interface AIContainers { containers: ContainerConfig[]; } -export interface ContainerAttachedInfo { - name: string; - modelService: boolean; - ports: string[]; -} - -export interface ApplicationPodInfo { - engineId: string; - Id: string; - containers?: ContainerAttachedInfo[]; - portmappings: PodCreatePortOptions[]; -} - export interface ImageInfo { id: string; modelService: boolean; @@ -114,76 +102,18 @@ export class ApplicationManager extends Publisher implements 'recipe-id': recipe.id, 'model-id': model.id, }); - return this.startApplication(recipe, model); - } - private async startApplication(recipe: Recipe, model: ModelInfo): Promise { - // const recipeStatus = this.recipeStatusRegistry. const startTime = performance.now(); try { - const localFolder = path.join(this.appUserDirectory, recipe.id); - - // clone the recipe repository on the local folder - const gitCloneInfo: GitCloneInfo = { - repository: recipe.repository, - ref: recipe.ref, - targetDirectory: localFolder, - }; - await this.doCheckout(gitCloneInfo, { - 'recipe-id': recipe.id, - 'model-id': model.id, - }); - - this.localRepositories.register({ - path: gitCloneInfo.targetDirectory, - sourcePath: path.join(gitCloneInfo.targetDirectory, recipe.basedir ?? ''), - labels: { - 'recipe-id': recipe.id, - }, - }); - - // load and parse the recipe configuration file and filter containers based on architecture, gpu accelerator - // and backend (that define which model supports) - const configAndFilteredContainers = this.getConfigAndFilterContainers(recipe.basedir, localFolder); - - // get model by downloading it or retrieving locally - await this.modelsManager.requestDownloadModel(model, { - 'recipe-id': recipe.id, - 'model-id': model.id, - }); - - // upload model to podman machine if user system is supported - const modelPath = await this.modelsManager.uploadModelToPodmanMachine(model, { - 'recipe-id': recipe.id, - 'model-id': model.id, - }); - - // build all images, one per container (for a basic sample we should have 2 containers = sample app + model service) - const images = await this.builderManager.build( - recipe, - configAndFilteredContainers.containers, - configAndFilteredContainers.aiConfigFile.path, - { - 'recipe-id': recipe.id, - 'model-id': model.id, - }, - ); - - // first delete any existing pod with matching labels - if (await this.hasApplicationPod(recipe.id, model.id)) { - await this.deleteApplication(recipe.id, model.id); - } - - // create a pod containing all the containers to run the application - const podInfo = await this.createApplicationPod(recipe, model, images, modelPath, { - 'recipe-id': recipe.id, - 'model-id': model.id, - }); - + // init application (git clone, models download etc.) + const podInfo: PodInfo = await this.initApplication(recipe, model); + // start the pod await this.runApplication(podInfo, { 'recipe-id': recipe.id, 'model-id': model.id, }); + + // measure init + start time const durationSeconds = getDurationSecondsSince(startTime); this.telemetry.logUsage('recipe.pull', { 'recipe.id': recipe.id, 'recipe.name': recipe.name, durationSeconds }); } catch (err: unknown) { @@ -199,14 +129,94 @@ export class ApplicationManager extends Publisher implements } } - async runApplication(podInfo: ApplicationPodInfo, labels?: { [key: string]: string }): Promise { + /** + * This method will execute the following tasks + * - git clone + * - git checkout + * - register local repository + * - download models + * - upload models + * - build containers + * - create pod + * + * @param recipe + * @param model + * @private + */ + private async initApplication(recipe: Recipe, model: ModelInfo): Promise { + const localFolder = path.join(this.appUserDirectory, recipe.id); + + // clone the recipe repository on the local folder + const gitCloneInfo: GitCloneInfo = { + repository: recipe.repository, + ref: recipe.ref, + targetDirectory: localFolder, + }; + await this.doCheckout(gitCloneInfo, { + 'recipe-id': recipe.id, + 'model-id': model.id, + }); + + this.localRepositories.register({ + path: gitCloneInfo.targetDirectory, + sourcePath: path.join(gitCloneInfo.targetDirectory, recipe.basedir ?? ''), + labels: { + 'recipe-id': recipe.id, + }, + }); + + // load and parse the recipe configuration file and filter containers based on architecture, gpu accelerator + // and backend (that define which model supports) + const configAndFilteredContainers = this.getConfigAndFilterContainers(recipe.basedir, localFolder); + + // get model by downloading it or retrieving locally + await this.modelsManager.requestDownloadModel(model, { + 'recipe-id': recipe.id, + 'model-id': model.id, + }); + + // upload model to podman machine if user system is supported + const modelPath = await this.modelsManager.uploadModelToPodmanMachine(model, { + 'recipe-id': recipe.id, + 'model-id': model.id, + }); + + // build all images, one per container (for a basic sample we should have 2 containers = sample app + model service) + const images = await this.builderManager.build( + recipe, + configAndFilteredContainers.containers, + configAndFilteredContainers.aiConfigFile.path, + { + 'recipe-id': recipe.id, + 'model-id': model.id, + }, + ); + + // first delete any existing pod with matching labels + if (await this.hasApplicationPod(recipe.id, model.id)) { + await this.deleteApplication(recipe.id, model.id); + } + + // create a pod containing all the containers to run the application + return this.createApplicationPod(recipe, model, images, modelPath, { + 'recipe-id': recipe.id, + 'model-id': model.id, + }); + } + + /** + * Given an ApplicationPodInfo, start the corresponding pod + * @param podInfo + * @param labels + */ + async runApplication(podInfo: PodInfo, labels?: { [key: string]: string }): Promise { const task = this.taskRegistry.createTask('Starting AI App', 'loading', labels); // it starts the pod await containerEngine.startPod(podInfo.engineId, podInfo.Id); // check if all containers have started successfully - for (const container of podInfo.containers ?? []) { + for (const container of podInfo.Containers ?? []) { await this.waitContainerIsRunning(podInfo.engineId, container); } @@ -218,17 +228,17 @@ export class ApplicationManager extends Publisher implements }); } - async waitContainerIsRunning(engineId: string, container: ContainerAttachedInfo): Promise { + async waitContainerIsRunning(engineId: string, container: PodContainerInfo): Promise { const TIME_FRAME_MS = 5000; const MAX_ATTEMPTS = 60 * (60000 / TIME_FRAME_MS); // try for 1 hour for (let i = 0; i < MAX_ATTEMPTS; i++) { - const sampleAppContainerInspectInfo = await containerEngine.inspectContainer(engineId, container.name); + const sampleAppContainerInspectInfo = await containerEngine.inspectContainer(engineId, container.Id); if (sampleAppContainerInspectInfo.State.Running) { return; } await timeout(TIME_FRAME_MS); } - throw new Error(`Container ${container.name} not started in time`); + throw new Error(`Container ${container.Id} not started in time`); } async createApplicationPod( @@ -237,11 +247,11 @@ export class ApplicationManager extends Publisher implements images: ImageInfo[], modelPath: string, labels?: { [key: string]: string }, - ): Promise { + ): Promise { const task = this.taskRegistry.createTask('Creating AI App', 'loading', labels); // create empty pod - let podInfo: ApplicationPodInfo; + let podInfo: PodInfo; try { podInfo = await this.createPod(recipe, model, images); task.labels = { @@ -257,9 +267,8 @@ export class ApplicationManager extends Publisher implements this.taskRegistry.updateTask(task); } - let attachedContainers: ContainerAttachedInfo[]; try { - attachedContainers = await this.createAndAddContainersToPod(podInfo, images, model, modelPath); + await this.createContainerAndAttachToPod(podInfo, images, model, modelPath); task.state = 'success'; } catch (e) { console.error(`error when creating pod ${podInfo.Id}`, e); @@ -270,17 +279,15 @@ export class ApplicationManager extends Publisher implements this.taskRegistry.updateTask(task); } - podInfo.containers = attachedContainers; return podInfo; } - async createAndAddContainersToPod( - podInfo: ApplicationPodInfo, + async createContainerAndAttachToPod( + podInfo: PodInfo, images: ImageInfo[], modelInfo: ModelInfo, modelPath: string, - ): Promise { - const containers: ContainerAttachedInfo[] = []; + ): Promise { // temporary check to set Z flag or not - to be removed when switching to podman 5 const isQEMUVM = await isQEMUMachine(); await Promise.all( @@ -332,17 +339,11 @@ export class ApplicationManager extends Publisher implements pod: podInfo.Id, HealthCheck: healthcheck, }); - containers.push({ - name: podifiedName, - modelService: image.modelService, - ports: image.ports, - }); }), ); - return containers; } - async createPod(recipe: Recipe, model: ModelInfo, images: ImageInfo[]): Promise { + async createPod(recipe: Recipe, model: ModelInfo, images: ImageInfo[]): Promise { // find the exposed port of the sample app so we can open its ports on the new pod const sampleAppImageInfo = images.find(image => !image.modelService); if (!sampleAppImageInfo) { @@ -386,16 +387,13 @@ export class ApplicationManager extends Publisher implements if (appPorts.length) { labels[LABEL_APP_PORTS] = appPorts.join(','); } - const pod = await containerEngine.createPod({ + const { engineId, Id } = await containerEngine.createPod({ name: getRandomName(`pod-${sampleAppImageInfo.appName}`), portmappings: portmappings, labels, }); - return { - Id: pod.Id, - engineId: pod.engineId, - portmappings: portmappings, - }; + + return this.podManager.getPod(engineId, Id); } private getConfigAndFilterContainers( @@ -716,7 +714,15 @@ export class ApplicationManager extends Publisher implements await this.deleteApplication(recipeId, modelId); const recipe = this.catalogManager.getRecipeById(recipeId); const model = this.catalogManager.getModelById(appPod.Labels[LABEL_MODEL_ID]); - await this.startApplication(recipe, model); + + // init the recipe + const podInfo = await this.initApplication(recipe, model); + + // start the pod + return this.runApplication(podInfo, { + 'recipe-id': recipe.id, + 'model-id': model.id, + }); } async getApplicationPorts(recipeId: string, modelId: string): Promise { diff --git a/packages/backend/src/managers/recipes/PodManager.spec.ts b/packages/backend/src/managers/recipes/PodManager.spec.ts index af2d7c5bb..433328108 100644 --- a/packages/backend/src/managers/recipes/PodManager.spec.ts +++ b/packages/backend/src/managers/recipes/PodManager.spec.ts @@ -168,3 +168,41 @@ describe('getHealth', () => { expect(health).toBe('starting'); }); }); + +describe('getPod', () => { + test('getPod should throw an error if none is matching', async () => { + vi.mocked(containerEngine.listPods).mockResolvedValue([]); + await expect(async () => { + await new PodManager().getPod('fakeEngineId', 'fakePodId'); + }).rejects.toThrowError('pod with engineId fakeEngineId and Id fakePodId cannot be found.'); + }); + + test('getPod should return matching pod', async () => { + vi.mocked(containerEngine.listPods).mockResolvedValue([ + { + engineId: 'engine-1', + Id: 'pod-id-1', + Labels: { + 'dummy-key': 'dummy-value', + hello: 'world', + }, + }, + { + engineId: 'engine-2', + Id: 'pod-id-2', + Labels: { + hello: 'world', + 'dummy-key': 'dummy-value', + }, + }, + { + engineId: 'engine-3', + Id: 'pod-id-3', + }, + ] as unknown as PodInfo[]); + const pod = await new PodManager().getPod('engine-3', 'pod-id-3'); + expect(pod).toBeDefined(); + expect(pod.engineId).toBe('engine-3'); + expect(pod.Id).toBe('pod-id-3'); + }); +}); diff --git a/packages/backend/src/managers/recipes/PodManager.ts b/packages/backend/src/managers/recipes/PodManager.ts index 0aed25280..16a8baf6d 100644 --- a/packages/backend/src/managers/recipes/PodManager.ts +++ b/packages/backend/src/managers/recipes/PodManager.ts @@ -74,4 +74,11 @@ export class PodManager implements Disposable { return getPodHealth(containerStates); } + + async getPod(engineId: string, Id: string): Promise { + const pods = await this.getAllPods(); + const result = pods.find(pod => pod.engineId === engineId && pod.Id === Id); + if (!result) throw new Error(`pod with engineId ${engineId} and Id ${Id} cannot be found.`); + return result; + } }