fix(jwa): Fix limits calculation when limitFactor is none (kubeflow#6058

) * jwa(front): Don't allow NaN values in limits The UI should always catch a NaN value and don't add it in the form. Currently this is the case for the cpu/memory limits. Signed-off-by: Kimonas Sotirchos <[email protected]> * jwa(front): Limits should not be changed if dirty If the user has manually edited the limits fields then the UI should not try to automatically calculate them again, using the limitFactors. Signed-off-by: Kimonas Sotirchos <[email protected]>
arrikto · Aug 13, 2021 · fe3f2f7 · fe3f2f7
1 parent 82f6228
commit fe3f2f7
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 38 deletions.
diff --git a/...s/jupyter/frontend/src/app/pages/form/form-default/form-cpu-ram/form-cpu-ram.component.ts b/...s/jupyter/frontend/src/app/pages/form/form-default/form-cpu-ram/form-cpu-ram.component.ts
@@ -1,5 +1,6 @@
 import { Component, OnInit, Input } from '@angular/core';
 import { FormGroup } from '@angular/forms';
+import { calculateLimits } from '../utils';
 
 @Component({
   selector: 'app-form-cpu-ram',
@@ -17,29 +18,27 @@ export class FormCpuRamComponent implements OnInit {
 
   ngOnInit() {
     this.parentForm.get('cpu').valueChanges.subscribe(val => {
-      //set cpu limit when value of the cpu request changes
-      if (this.cpuLimitFactor !== null) {
-        this.parentForm
-          .get('cpuLimit')
-          .setValue(
-            (
-              parseFloat(this.cpuLimitFactor) * this.parentForm.get('cpu').value
-            ).toFixed(1),
-          );
+      // set cpu limit when value of the cpu request changes
+      if (this.parentForm.get('cpuLimit').dirty) {
+        return;
       }
+
+      const cpu = this.parentForm.get('cpu').value;
+      this.parentForm
+        .get('cpuLimit')
+        .setValue(calculateLimits(cpu, this.cpuLimitFactor));
     });
+
     this.parentForm.get('memory').valueChanges.subscribe(val => {
-      //set memory limit when value of the memory request changes
-      if (this.memoryLimitFactor !== null) {
-        this.parentForm
-          .get('memoryLimit')
-          .setValue(
-            (
-              parseFloat(this.memoryLimitFactor) *
-              this.parentForm.get('memory').value
-            ).toFixed(1),
-          );
+      // set memory limit when value of the memory request changes
+      if (this.parentForm.get('memoryLimit').dirty) {
+        return;
       }
+
+      const memory = this.parentForm.get('memory').value;
+      this.parentForm
+        .get('memoryLimit')
+        .setValue(calculateLimits(memory, this.memoryLimitFactor));
     });
   }
 

diff --git a/components/crud-web-apps/jupyter/frontend/src/app/pages/form/form-default/utils.ts b/components/crud-web-apps/jupyter/frontend/src/app/pages/form/form-default/utils.ts
@@ -138,37 +138,56 @@ export function updateGPUControl(formCtrl: FormGroup, gpuConf: any) {
   }
 }
 
-export function initFormControls(formCtrl: FormGroup, config: Config) {
-  // Sets the values from our internal dict. This is an initialization step
-  // that should be only run once
-  formCtrl.controls.cpu.setValue(configSizeToNumber(config.cpu.value));
-  if (config.cpu.limitFactor !== 'none') {
-    formCtrl.controls.cpuLimit.setValue(
-      (
-        configSizeToNumber(config.cpu.value) *
-        configSizeToNumber(config.cpu.limitFactor)
-      ).toFixed(1),
-    );
+export function calculateLimits(
+  requests: number | string,
+  factor: number | string,
+): string | null {
+  const limit = configSizeToNumber(requests) * configSizeToNumber(factor);
+
+  if (isNaN(limit)) {
+    return null;
+  }
+
+  return limit.toFixed(1);
+}
+
+export function initCpuFormControls(formCtrl: FormGroup, config: Config) {
+  const cpu = Number(config.cpu.value);
+  if (!isNaN(cpu)) {
+    formCtrl.controls.cpu.setValue(cpu);
   }
+
   if (config.cpu.readOnly) {
     formCtrl.controls.cpu.disable();
     formCtrl.controls.cpuLimit.disable();
   }
 
-  formCtrl.controls.memory.setValue(configSizeToNumber(config.memory.value));
-  if (config.memory.limitFactor !== 'none') {
-    formCtrl.controls.memoryLimit.setValue(
-      (
-        configSizeToNumber(config.memory.value) *
-        configSizeToNumber(config.memory.limitFactor)
-      ).toFixed(1),
-    );
+  formCtrl.controls.cpuLimit.setValue(
+    calculateLimits(cpu, config.cpu.limitFactor),
+  );
+}
+
+export function initMemoryFormControls(formCtrl: FormGroup, config: Config) {
+  const memory = configSizeToNumber(config.memory.value);
+  if (!isNaN(memory)) {
+    formCtrl.controls.memory.setValue(memory);
   }
+
   if (config.memory.readOnly) {
     formCtrl.controls.memory.disable();
     formCtrl.controls.memoryLimit.disable();
   }
 
+  formCtrl.controls.memoryLimit.setValue(
+    calculateLimits(memory, config.memory.limitFactor),
+  );
+}
+
+export function initFormControls(formCtrl: FormGroup, config: Config) {
+  initCpuFormControls(formCtrl, config);
+
+  initMemoryFormControls(formCtrl, config);
+
   formCtrl.controls.image.setValue(config.image.value);
 
   formCtrl.controls.imageGroupOne.setValue(config.imageGroupOne.value);
@@ -225,6 +244,10 @@ export function initFormControls(formCtrl: FormGroup, config: Config) {
 }
 
 export function configSizeToNumber(size: string | number): number {
+  if (size == null) {
+    return NaN;
+  }
+
   if (typeof size === 'number') {
     return size;
   }