NAS-128601 / 24.10.0 / Fixes the process to assign GPUs to VMs when c…

…reating or editing (by RehanY147) (#10849) * NAS-128601: Fix edit vm (cherry picked from commit 82b1d3e) * NAS-128601: Fixes the process to assign GPUs to VMs when creating or editing * NAS-128601: Fixes the process to assign GPUs to VMs when creating or editing --------- Co-authored-by: RehanY147 <[email protected]> Co-authored-by: undsoft <[email protected]>
truenas · Oct 11, 2024 · 8e83301 · 8e83301
1 parent fe8dd2f
commit 8e83301
Show file tree

Hide file tree

Showing 12 changed files with 107 additions and 72 deletions.
diff --git a/src/app/helpers/operators/options.operators.ts b/src/app/helpers/operators/options.operators.ts
@@ -10,7 +10,9 @@ import { MapOption, Option } from 'app/interfaces/option.interface';
  */
 export function choicesToOptions(): OperatorFunction<Choices, Option[]> {
   return map((choices) => {
-    return Object.entries(choices).map(([value, label]) => ({ label, value }));
+    return Object.entries(choices).map(
+      ([value, label]) => ({ label, value }),
+    );
   });
 }
 

diff --git a/src/app/interfaces/api/api-call-directory.interface.ts b/src/app/interfaces/api/api-call-directory.interface.ts
@@ -877,6 +877,7 @@ export interface ApiCallDirectory {
   'vm.device.delete': { params: [number, VmDeviceDelete?]; response: boolean };
   'vm.device.disk_choices': { params: void; response: Choices };
   'vm.device.get_pci_ids_for_gpu_isolation': { params: [string]; response: string[] };
+  'system.advanced.get_gpu_pci_choices': { params: void; response: Choices };
   'vm.device.nic_attach_choices': { params: void; response: Choices };
   'vm.device.passthrough_device_choices': { params: void; response: Record<string, VmPassthroughDeviceChoice> };
   'vm.device.query': { params: QueryParams<VmDevice>; response: VmDevice[] };

diff --git a/...ges/system/advanced/isolated-gpus/isolated-gpus-form/isolated-gpus-form.component.spec.ts b/...ges/system/advanced/isolated-gpus/isolated-gpus-form/isolated-gpus-form.component.spec.ts
@@ -45,6 +45,10 @@ describe('IsolatedGpuPcisFormComponent', () => {
       }),
       mockWebSocket([
         mockCall('system.advanced.update_gpu_pci_ids'),
+        mockCall('system.advanced.get_gpu_pci_choices', {
+          'Fake HD Graphics [0000:00:01.0]': '0000:00:01.0',
+          'Intel Corporation HD Graphics 510 [0000:00:02.0]': '0000:00:02.0',
+        }),
       ]),
       mockProvider(SystemGeneralService),
       mockProvider(IxChainedSlideInService, {
@@ -78,14 +82,14 @@ describe('IsolatedGpuPcisFormComponent', () => {
     const values = await form.getValues();
 
     expect(values).toEqual({
-      GPUs: ['Intel Corporation HD Graphics 510'],
+      GPUs: ['Intel Corporation HD Graphics 510 [0000:00:02.0]'],
     });
   });
 
   it('saves updated settings when Save is pressed', async () => {
     const form = await loader.getHarness(IxFormHarness);
     await form.fillForm({
-      GPUs: 'Fake HD Graphics',
+      GPUs: 'Fake HD Graphics [0000:00:01.0]',
     });
 
     const saveButton = await loader.getHarness(MatButtonHarness.with({ text: 'Save' }));

diff --git a/...pp/pages/system/advanced/isolated-gpus/isolated-gpus-form/isolated-gpus-form.component.ts b/...pp/pages/system/advanced/isolated-gpus/isolated-gpus-form/isolated-gpus-form.component.ts
@@ -5,12 +5,11 @@ import { FormControl, FormGroup } from '@angular/forms';
 import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
 import { Store } from '@ngrx/store';
 import { TranslateService } from '@ngx-translate/core';
-import { take } from 'rxjs';
+import { map, take } from 'rxjs';
 import { Role } from 'app/enums/role.enum';
 import { ChainedRef } from 'app/modules/forms/ix-forms/components/ix-slide-in/chained-component-ref';
 import { FormErrorHandlerService } from 'app/modules/forms/ix-forms/services/form-error-handler.service';
 import { SnackbarService } from 'app/modules/snackbar/services/snackbar.service';
-import { GpuService } from 'app/services/gpu/gpu.service';
 import { IsolatedGpuValidatorService } from 'app/services/gpu/isolated-gpu-validator.service';
 import { WebSocketService } from 'app/services/ws.service';
 import { AppState } from 'app/store';
@@ -33,7 +32,11 @@ export class IsolatedGpusFormComponent implements OnInit {
       asyncValidators: [this.gpuValidator.validateGpu],
     }),
   });
-  readonly options$ = this.gpuService.getGpuOptions();
+  readonly options$ = this.ws.call('system.advanced.get_gpu_pci_choices').pipe(map((choices) => {
+    return Object.entries(choices).map(
+      ([value, label]) => ({ value: label, label: value }),
+    );
+  }));
 
   constructor(
     protected ws: WebSocketService,
@@ -42,7 +45,6 @@ export class IsolatedGpusFormComponent implements OnInit {
     private cdr: ChangeDetectorRef,
     private store$: Store<AppState>,
     private gpuValidator: IsolatedGpuValidatorService,
-    private gpuService: GpuService,
     private snackbar: SnackbarService,
     private chainedRef: ChainedRef<unknown>,
   ) { }

diff --git a/src/app/pages/vm/vm-edit-form/vm-edit-form.component.spec.ts b/src/app/pages/vm/vm-edit-form/vm-edit-form.component.spec.ts
@@ -81,15 +81,24 @@ describe('VmEditFormComponent', () => {
         }),
         mockCall('vm.update'),
         mockCall('vm.device.get_pci_ids_for_gpu_isolation', ['10DE:1401']),
+        mockCall('system.advanced.update_gpu_pci_ids'),
+        mockCall('system.advanced.get_gpu_pci_choices', {
+          'GeForce [0000:02:00.0]': '0000:02:00.0',
+          'Intel Arc [0000:03:00.0]': '0000:03:00.0',
+        }),
       ]),
       mockAuth(),
       mockProvider(DialogService),
       mockProvider(GpuService, {
-        getGpuOptions: () => of([
-          { label: 'GeForce', value: '0000:02:00.0' },
-          { label: 'Intel Arc', value: '0000:03:00.0' },
-        ]),
-        getAllGpus: () => of([
+        getGpuOptions: jest.fn(() => of([
+          { label: 'GeForce [0000:02:00.0]', value: '0000:02:00.0' },
+          { label: 'Intel Arc [0000:03:00.0]', value: '0000:03:00.0' },
+        ])),
+        addIsolatedGpuPciIds: jest.fn(() => of({})),
+        getIsolatedGpuPciIds: jest.fn(() => of([
+          '0000:02:00.0',
+        ])),
+        getAllGpus: jest.fn(() => of([
           {
             addr: {
               pci_slot: '0000:02:00.0',
@@ -114,8 +123,7 @@ describe('VmEditFormComponent', () => {
               },
             ],
           },
-        ]),
-        addIsolatedGpuPciIds: jest.fn(() => of(undefined)),
+        ])),
       }),
       mockProvider(VmGpuService, {
         updateVmGpus: jest.fn(() => of(undefined)),
@@ -168,7 +176,7 @@ describe('VmEditFormComponent', () => {
 
       'Hide from MSR': false,
       'Ensure Display Device': true,
-      GPUs: ['GeForce'],
+      GPUs: ['GeForce [0000:02:00.0]'],
     });
   });
 
@@ -246,13 +254,15 @@ describe('VmEditFormComponent', () => {
 
   it('updates GPU devices when form is edited and saved', async () => {
     await form.fillForm({
-      GPUs: ['Intel Arc'],
+      GPUs: ['Intel Arc [0000:03:00.0]'],
     });
 
     const saveButton = await loader.getHarness(MatButtonHarness.with({ text: 'Save' }));
     await saveButton.click();
 
-    expect(spectator.inject(VmGpuService).updateVmGpus).toHaveBeenCalledWith(existingVm, ['0000:03:00.0', '10DE:1401']);
-    expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(['0000:03:00.0', '10DE:1401']);
+    expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(
+      ['0000:03:00.0'],
+    );
+    expect(spectator.inject(VmGpuService).updateVmGpus).toHaveBeenCalledWith(existingVm, ['0000:03:00.0']);
   });
 });
diff --git a/src/app/pages/vm/vm-edit-form/vm-edit-form.component.ts b/src/app/pages/vm/vm-edit-form/vm-edit-form.component.ts
@@ -1,11 +1,13 @@
 import {
   ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, OnInit,
+  signal,
 } from '@angular/core';
 import { FormBuilder, Validators } from '@angular/forms';
 import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
 import { TranslateService } from '@ngx-translate/core';
 import {
-  Observable, forkJoin, map, of, switchMap,
+  forkJoin, of, switchMap,
+  tap,
 } from 'rxjs';
 import { MiB } from 'app/constants/bytes.constant';
 import { Role } from 'app/enums/role.enum';
@@ -15,6 +17,7 @@ import {
 import { choicesToOptions } from 'app/helpers/operators/options.operators';
 import { mapToOptions } from 'app/helpers/options.helper';
 import { helptextVmWizard } from 'app/helptext/vm/vm-wizard/vm-wizard';
+import { Option } from 'app/interfaces/option.interface';
 import { VirtualMachine, VirtualMachineUpdate } from 'app/interfaces/virtual-machine.interface';
 import { VmPciPassthroughDevice } from 'app/interfaces/vm-device.interface';
 import { DialogService } from 'app/modules/dialog/dialog.service';
@@ -70,12 +73,13 @@ export class VmEditFormComponent implements OnInit {
     gpus: [[] as string[], [], [this.gpuValidator.validateGpu]],
   });
 
+  private readonly gpuOptions = signal<Option[]>([]);
   isLoading = false;
   timeOptions$ = of(mapToOptions(vmTimeNames, this.translate));
   bootloaderOptions$ = this.ws.call('vm.bootloader_options').pipe(choicesToOptions());
   cpuModeOptions$ = of(mapToOptions(vmCpuModeLabels, this.translate));
   cpuModelOptions$ = this.ws.call('vm.cpu_model_choices').pipe(choicesToOptions());
-  gpuOptions$ = this.gpuService.getGpuOptions();
+  gpuOptions$ = this.gpuService.getGpuOptions().pipe(tap((options) => this.gpuOptions.set(options)));
 
   readonly helptext = helptextVmWizard;
 
@@ -138,26 +142,13 @@ export class VmEditFormComponent implements OnInit {
 
     const gpusIds = this.form.value.gpus;
 
-    const pciIdsRequests$ = gpusIds.map((gpu) => {
-      return this.ws.call('vm.device.get_pci_ids_for_gpu_isolation', [gpu]);
-    });
-
-    let updateVmRequest$: Observable<unknown>;
-
-    if (pciIdsRequests$.length) {
-      updateVmRequest$ = forkJoin(pciIdsRequests$).pipe(
-        map((pciIds) => pciIds.flat()),
-        switchMap((pciIds) => forkJoin([
-          this.ws.call('vm.update', [this.existingVm.id, vmPayload as VirtualMachineUpdate]),
-          this.vmGpuService.updateVmGpus(this.existingVm, gpusIds.concat(pciIds)),
-          this.gpuService.addIsolatedGpuPciIds(gpusIds.concat(pciIds)),
-        ])),
-      );
-    } else {
-      updateVmRequest$ = this.ws.call('vm.update', [this.existingVm.id, vmPayload as VirtualMachineUpdate]);
-    }
-
-    updateVmRequest$.pipe(untilDestroyed(this)).subscribe({
+    this.gpuService.addIsolatedGpuPciIds(gpusIds).pipe(
+      switchMap(() => forkJoin([
+        this.ws.call('vm.update', [this.existingVm.id, vmPayload as VirtualMachineUpdate]),
+        this.vmGpuService.updateVmGpus(this.existingVm, gpusIds),
+      ])),
+      untilDestroyed(this),
+    ).subscribe({
       next: () => {
         this.isLoading = false;
         this.cdr.markForCheck();

diff --git a/src/app/pages/vm/vm-wizard/steps/6-gpu-step/gpu-step.component.spec.ts b/src/app/pages/vm/vm-wizard/steps/6-gpu-step/gpu-step.component.spec.ts
@@ -3,6 +3,7 @@ import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed';
 import { ReactiveFormsModule } from '@angular/forms';
 import { createComponentFactory, mockProvider, Spectator } from '@ngneat/spectator/jest';
 import { of } from 'rxjs';
+import { mockCall, mockWebSocket } from 'app/core/testing/utils/mock-websocket.utils';
 import { IxFormsModule } from 'app/modules/forms/ix-forms/ix-forms.module';
 import { IxFormHarness } from 'app/modules/forms/ix-forms/testing/ix-form.harness';
 import { GpuStepComponent } from 'app/pages/vm/vm-wizard/steps/6-gpu-step/gpu-step.component';
@@ -23,13 +24,19 @@ describe('GpuStepComponent', () => {
     providers: [
       mockProvider(GpuService, {
         getGpuOptions: () => of([
-          { label: 'GeForce GTX 1080', value: '0000:03:00.0' },
-          { label: 'GeForce GTX 1080 Ti', value: '0000:04:00.0' },
+          { label: 'GeForce GTX 1080 [0000:03:00.0]', value: '0000:03:00.0' },
+          { label: 'GeForce GTX 1080 Ti [0000:04:00.0]', value: '0000:04:00.0' },
         ]),
       }),
       mockProvider(IsolatedGpuValidatorService, {
         validateGpu: () => of(null),
       }),
+      mockWebSocket([
+        mockCall('system.advanced.get_gpu_pci_choices', {
+          'GeForce GTX 1080 [0000:03:00.0]': '0000:03:00.0',
+          'GeForce GTX 1080 Ti [0000:04:00.0]': '0000:04:00.0',
+        }),
+      ]),
     ],
   });
 
@@ -43,7 +50,7 @@ describe('GpuStepComponent', () => {
     await form.fillForm({
       'Hide from MSR': true,
       'Ensure Display Device': true,
-      GPUs: ['GeForce GTX 1080 Ti'],
+      GPUs: ['GeForce GTX 1080 Ti [0000:04:00.0]'],
     });
   }
 

diff --git a/src/app/pages/vm/vm-wizard/steps/6-gpu-step/gpu-step.component.ts b/src/app/pages/vm/vm-wizard/steps/6-gpu-step/gpu-step.component.ts
@@ -28,8 +28,8 @@ export class GpuStepComponent implements SummaryProvider {
   constructor(
     private formBuilder: FormBuilder,
     private gpuValidator: IsolatedGpuValidatorService,
-    private gpuService: GpuService,
     private translate: TranslateService,
+    private gpuService: GpuService,
   ) {}
 
   getSummary(): SummarySection {

diff --git a/src/app/pages/vm/vm-wizard/vm-wizard.component.spec.ts b/src/app/pages/vm/vm-wizard/vm-wizard.component.spec.ts
@@ -96,12 +96,21 @@ describe('VmWizardComponent', () => {
           eno2: 'eno2',
         }),
         mockCall('vm.device.get_pci_ids_for_gpu_isolation', ['10DE:1401']),
+        mockCall('system.advanced.update_gpu_pci_ids'),
+        mockCall('system.advanced.get_gpu_pci_choices', {
+          'GeForce GTX 1080 [0000:03:00.0]': '0000:03:00.0',
+          'GeForce GTX 1070 [0000:02:00.0]': '0000:02:00.0',
+        }),
       ]),
       mockProvider(GpuService, {
         getGpuOptions: () => of([
-          { label: 'GeForce GTX 1080', value: '0000:03:00.0' },
+          { label: 'GeForce GTX 1080 [0000:03:00.0]', value: '0000:03:00.0' },
+          { label: 'GeForce GTX 1070 [0000:02:00.0]', value: '0000:02:00.0' },
         ]),
-        addIsolatedGpuPciIds: jest.fn(() => of(undefined)),
+        addIsolatedGpuPciIds: jest.fn(() => of({})),
+        getIsolatedGpuPciIds: jest.fn(() => of([
+          '0000:02:00.0',
+        ])),
       }),
       mockProvider(FilesystemService, {
         getFilesystemNodeProvider: jest.fn(),
@@ -167,7 +176,7 @@ describe('VmWizardComponent', () => {
     await updateStepHarnesses();
 
     await form.fillForm({
-      GPUs: ['GeForce GTX 1080'],
+      GPUs: ['GeForce GTX 1080 [0000:03:00.0]'],
     });
     await nextButton.click();
   }
@@ -321,8 +330,10 @@ describe('VmWizardComponent', () => {
         web: true,
       },
     }]);
-    expect(spectator.inject(VmGpuService).updateVmGpus).toHaveBeenCalledWith({ id: 4 }, ['0000:03:00.0', '10DE:1401']);
-    expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(['0000:03:00.0', '10DE:1401']);
+    expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(
+      ['0000:03:00.0'],
+    );
+    expect(spectator.inject(VmGpuService).updateVmGpus).toHaveBeenCalledWith({ id: 4 }, ['0000:03:00.0']);
     expect(spectator.inject(IxSlideInRef).close).toHaveBeenCalled();
   });
 });
diff --git a/src/app/pages/vm/vm-wizard/vm-wizard.component.ts b/src/app/pages/vm/vm-wizard/vm-wizard.component.ts
@@ -7,7 +7,7 @@ import * as _ from 'lodash';
 import {
   forkJoin, Observable, of, switchMap,
 } from 'rxjs';
-import { catchError, defaultIfEmpty, map } from 'rxjs/operators';
+import { catchError, defaultIfEmpty } from 'rxjs/operators';
 import { GiB, MiB } from 'app/constants/bytes.constant';
 import { Role } from 'app/enums/role.enum';
 import { VmDeviceType, VmNicType, VmOs } from 'app/enums/vm.enum';
@@ -252,17 +252,9 @@ export class VmWizardComponent implements OnInit {
   private getGpuRequests(vm: VirtualMachine): Observable<unknown> {
     const gpusIds = this.gpuForm.gpus as unknown as string[];
 
-    const pciIdsRequests$ = gpusIds.map((gpu) => {
-      return this.ws.call('vm.device.get_pci_ids_for_gpu_isolation', [gpu]);
-    });
-
-    return forkJoin(pciIdsRequests$).pipe(
+    return this.gpuService.addIsolatedGpuPciIds(gpusIds).pipe(
       defaultIfEmpty([]),
-      map((pciIds) => pciIds.flat()),
-      switchMap((pciIds) => forkJoin([
-        this.vmGpuService.updateVmGpus(vm, gpusIds.concat(pciIds)),
-        this.gpuService.addIsolatedGpuPciIds(gpusIds.concat(pciIds)),
-      ])),
+      switchMap(() => this.vmGpuService.updateVmGpus(vm, gpusIds)),
     );
   }