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

NAS-128601 / 25.04 / Fixes the process to assign GPUs to VMs when creating or editing #10838

Merged
merged 15 commits into from
Oct 11, 2024
6 changes: 4 additions & 2 deletions src/app/helpers/operators/options.operators.ts
Original file line number Diff line number Diff line change
@@ -8,9 +8,11 @@ import { MapOption, Option } from 'app/interfaces/option.interface';
* Convert choices to options
* @returns Option[]
*/
export function choicesToOptions(): OperatorFunction<Choices, Option[]> {
export function choicesToOptions(reverseValueAndLabels = false): OperatorFunction<Choices, Option[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is obscure. It's better to create a new function for this local to vms module, like in GpuService.

return map((choices) => {
return Object.entries(choices).map(([value, label]) => ({ label, value }));
return Object.entries(choices).map(
([value, label]) => (reverseValueAndLabels ? ({ label: value, value: label }) : ({ label, value })),
);
});
}

1 change: 1 addition & 0 deletions src/app/interfaces/api/api-call-directory.interface.ts
Original file line number Diff line number Diff line change
@@ -879,6 +879,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[] };
Original file line number Diff line number Diff line change
@@ -43,6 +43,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, {
@@ -76,14 +80,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' }));
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@ import { TranslateService, TranslateModule } from '@ngx-translate/core';
import { take } from 'rxjs';
import { RequiresRolesDirective } from 'app/directives/requires-roles/requires-roles.directive';
import { Role } from 'app/enums/role.enum';
import { choicesToOptions } from 'app/helpers/operators/options.operators';
import { FormActionsComponent } from 'app/modules/forms/ix-forms/components/form-actions/form-actions.component';
import { IxFieldsetComponent } from 'app/modules/forms/ix-forms/components/ix-fieldset/ix-fieldset.component';
import { IxSelectComponent } from 'app/modules/forms/ix-forms/components/ix-select/ix-select.component';
@@ -55,7 +56,7 @@ 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(choicesToOptions(true));

constructor(
protected ws: WebSocketService,
13 changes: 9 additions & 4 deletions src/app/pages/vm/vm-edit-form/vm-edit-form.component.spec.ts
Original file line number Diff line number Diff line change
@@ -79,6 +79,11 @@ 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),
@@ -166,7 +171,7 @@ describe('VmEditFormComponent', () => {

'Hide from MSR': false,
'Ensure Display Device': true,
GPUs: ['GeForce'],
GPUs: ['GeForce [0000:02:00.0]'],
});
});

@@ -244,13 +249,13 @@ 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(VmGpuService).updateVmGpus).toHaveBeenCalledWith(existingVm, ['0000:03:00.0']);
expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(['0000:03:00.0']);
});
});
27 changes: 14 additions & 13 deletions src/app/pages/vm/vm-edit-form/vm-edit-form.component.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import {
ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, OnInit,
signal,
} from '@angular/core';
import { FormBuilder, Validators, ReactiveFormsModule } from '@angular/forms';
import { MatButton } from '@angular/material/button';
import { MatCard, MatCardContent } from '@angular/material/card';
import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy';
import { TranslateService, TranslateModule } from '@ngx-translate/core';
import {
Observable, forkJoin, map, of, switchMap,
Observable, forkJoin, of, switchMap,
tap,
} from 'rxjs';
import { MiB } from 'app/constants/bytes.constant';
import { RequiresRolesDirective } from 'app/directives/requires-roles/requires-roles.directive';
@@ -18,6 +20,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';
@@ -96,12 +99,16 @@ 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.ws.call('system.advanced.get_gpu_pci_choices').pipe(
Copy link
Collaborator

Choose a reason for hiding this comment

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

getGpuOptions in GpuService is now unused and could hold this code.

choicesToOptions(true),
tap((options) => this.gpuOptions.set(options)),
);

readonly helptext = helptextVmWizard;

@@ -163,20 +170,14 @@ 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([
if (gpusIds.length) {
updateVmRequest$ = this.ws.call('system.advanced.update_gpu_pci_ids', [gpusIds]).pipe(
switchMap(() => 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)),
this.vmGpuService.updateVmGpus(this.existingVm, gpusIds),
this.gpuService.addIsolatedGpuPciIds(gpusIds),
])),
);
} else {
Original file line number Diff line number Diff line change
@@ -4,6 +4,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 { 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';
import { GpuService } from 'app/services/gpu/gpu.service';
@@ -30,6 +31,12 @@ describe('GpuStepComponent', () => {
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]'],
});
}

Original file line number Diff line number Diff line change
@@ -5,14 +5,15 @@ import { FormBuilder, ReactiveFormsModule } from '@angular/forms';
import { MatButton } from '@angular/material/button';
import { MatStepperPrevious, MatStepperNext } from '@angular/material/stepper';
import { TranslateService, TranslateModule } from '@ngx-translate/core';
import { choicesToOptions } from 'app/helpers/operators/options.operators';
import { helptextVmWizard } from 'app/helptext/vm/vm-wizard/vm-wizard';
import { FormActionsComponent } from 'app/modules/forms/ix-forms/components/form-actions/form-actions.component';
import { IxCheckboxComponent } from 'app/modules/forms/ix-forms/components/ix-checkbox/ix-checkbox.component';
import { IxSelectComponent } from 'app/modules/forms/ix-forms/components/ix-select/ix-select.component';
import { SummaryProvider, SummarySection } from 'app/modules/summary/summary.interface';
import { TestDirective } from 'app/modules/test-id/test.directive';
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';

@Component({
selector: 'ix-gpu-step',
@@ -41,13 +42,13 @@ export class GpuStepComponent implements SummaryProvider {
});

readonly helptext = helptextVmWizard;
readonly gpuOptions$ = this.gpuService.getGpuOptions();
readonly gpuOptions$ = this.ws.call('system.advanced.get_gpu_pci_choices').pipe(choicesToOptions(true));

constructor(
private formBuilder: FormBuilder,
private gpuValidator: IsolatedGpuValidatorService,
private gpuService: GpuService,
private translate: TranslateService,
private ws: WebSocketService,
) {}

getSummary(): SummarySection {
10 changes: 7 additions & 3 deletions src/app/pages/vm/vm-wizard/vm-wizard.component.spec.ts
Original file line number Diff line number Diff line change
@@ -94,6 +94,10 @@ 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',
}),
]),
mockProvider(GpuService, {
getGpuOptions: () => of([
@@ -165,7 +169,7 @@ describe('VmWizardComponent', () => {
await updateStepHarnesses();

await form.fillForm({
GPUs: ['GeForce GTX 1080'],
GPUs: ['GeForce GTX 1080 [0000:03:00.0]'],
});
await nextButton.click();
}
@@ -319,8 +323,8 @@ 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(VmGpuService).updateVmGpus).toHaveBeenCalledWith({ id: 4 }, ['0000:03:00.0']);
expect(spectator.inject(GpuService).addIsolatedGpuPciIds).toHaveBeenCalledWith(['0000:03:00.0']);
expect(spectator.inject(IxSlideInRef).close).toHaveBeenCalled();
});
});
19 changes: 8 additions & 11 deletions src/app/pages/vm/vm-wizard/vm-wizard.component.ts
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ import { pick } from 'lodash-es';
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 { RequiresRolesDirective } from 'app/directives/requires-roles/requires-roles.directive';
import { Role } from 'app/enums/role.enum';
@@ -285,17 +285,14 @@ 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.ws.call('system.advanced.update_gpu_pci_ids', [gpusIds]).pipe().pipe(
defaultIfEmpty([]),
map((pciIds) => pciIds.flat()),
switchMap((pciIds) => forkJoin([
this.vmGpuService.updateVmGpus(vm, gpusIds.concat(pciIds)),
this.gpuService.addIsolatedGpuPciIds(gpusIds.concat(pciIds)),
])),
switchMap(() => {
return forkJoin([
this.vmGpuService.updateVmGpus(vm, gpusIds),
this.gpuService.addIsolatedGpuPciIds(gpusIds),
Copy link
Collaborator

Choose a reason for hiding this comment

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

So you call system.advanced.update_gpu_pci_ids and then you call addIsolatedGpuPciIds which also calls the same endpoint.

]);
}),
);
}


Unchanged files with check annotations Beta

});
// TODO: Broken after package update. Actually functionality is working.
it.skip('inserts a suggestion when Enter is pressed', async () => {

Check warning on line 225 in src/app/modules/forms/search-input/components/advanced-search/tests/autocomplete.spec.ts

GitHub Actions / Validate code style

Disabled test
await searchHarness.setValue('User');
await (await searchHarness.getInputArea()).sendKeys(TestKey.ENTER);
});
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
it.skip('shows a list of previous versions for an installed app to roll back to', async () => {

Check warning on line 51 in src/app/pages/apps/components/installed-apps/app-rollback-modal/app-rollback-modal.component.spec.ts

GitHub Actions / Validate code style

Disabled test
const versionSelect = await loader.getHarness(IxSelectHarness);
const options = await versionSelect.getOptionLabels();
});
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
it.skip('rolls back app when form is submitted', async () => {

Check warning on line 59 in src/app/pages/apps/components/installed-apps/app-rollback-modal/app-rollback-modal.component.spec.ts

GitHub Actions / Validate code style

Disabled test
const form = await loader.getHarness(IxFormHarness);
await form.fillForm({
Version: '0.9.8',
expect(spectator.query('span')).toHaveText('Deploying');
});
it.skip('checks state for stopping app', () => {

Check warning on line 45 in src/app/pages/apps/components/installed-apps/app-state-cell/app-state-cell.component.spec.ts

GitHub Actions / Validate code style

Disabled test
setupTest(
{ state: AppState.Running } as App,
);
import { ShellDetailsType } from 'app/pages/apps/enum/shell-details-type.enum';
// TODO:
describe.skip('ShellDetailsDialogComponent', () => {

Check warning on line 14 in src/app/pages/apps/components/shell-details-dialog/shell-details-dialog.component.spec.ts

GitHub Actions / Validate code style

Disabled test suite
let spectator: Spectator<ShellDetailsDialogComponent>;
let loader: HarnessLoader;
let form: IxFormHarness;