fix(resourceLimits): calculate limits correctly

this fixes a couple places that fomerly presented misleading information:

when deploying a single app and scaling it, we might run into the case that our
former calculations came to the conclusion to display some limits (because of
unintended side effects of default values).

when having a look at the tasks of either such an app or a scaled pod, we would
also see wrong information for the tasks. they would each claim to reserve / be
limited at their `own claim * scale` because the calculation did not distinguish
the `ServicesTable` from the `TasksTable`.

We now got some unit tests that you can look at to get a feeling for what values
we show!

Closes COPS-6555

plugins/services/src/js/columns/ServicesTableCPUColumn.tsx

@@ -51,6 +51,6 @@ export const cpuRenderer = (service: TreeItem): React.ReactNode => (
   <ServiceCPU
     id={service.getId()}
     resource={service.getResources().cpus}
-    limit={getResourceLimits(service).cpus}
+    limit={getResourceLimits(service, true).cpus}
   />
 );

plugins/services/src/js/columns/ServicesTableMemColumn.tsx

@@ -14,7 +14,7 @@ export const ServiceMem = React.memo(
     limit,
   }: {
     resource: string;
-    limit: number | string | null;
+    limit?: number | string;
     id: string;
   }) => {
     if (limit != null && limit !== 0 && limit !== resource) {
@@ -50,6 +50,6 @@ export const memRenderer = (service: TreeItem): React.ReactNode => (
   <ServiceMem
     id={service.getId()}
     resource={service.getResources().mem}
-    limit={getResourceLimits(service).mem}
+    limit={getResourceLimits(service, true).mem}
   />
 );

plugins/services/src/js/columns/__tests__/resourceLimits-test.ts

@@ -0,0 +1,81 @@
+import { getResourceLimits, ResourceLimits } from "../resourceLimits";
+import Application from "../../structs/Application";
+import Pod from "../../structs/Pod";
+
+const app = (
+  resourceLimits: ResourceLimits = { cpus: 1, mem: 256 },
+  instances = 2
+) => new Application({ instances, resourceLimits });
+
+const podContainer = (opts: any = {}) => ({
+  resourceLimits: { cpus: 1, mem: 256 },
+  ...opts,
+});
+
+// opts is passed to the second container
+const pod = (opts: any[] = []) =>
+  new Pod({
+    spec: {
+      containers: [podContainer(opts[0]), podContainer(opts[1])],
+      scaling: { instances: 2 },
+    },
+  });
+
+describe("getResourceLimits for apps", () => {
+  it("does not incorporate scale by default", () => {
+    expect(getResourceLimits(app())).toEqual({
+      cpus: 1,
+      mem: 256,
+    });
+  });
+  it("can compute scaled values", () => {
+    expect(getResourceLimits(app(), true)).toEqual({
+      cpus: 2,
+      mem: 512,
+    });
+  });
+  it("returns null when no resourceLimits are set", () => {
+    expect(
+      getResourceLimits(app({ cpus: undefined, mem: undefined }), true)
+    ).toEqual({
+      cpus: undefined,
+      mem: undefined,
+    });
+  });
+});
+
+describe("getResourceLimits for pods", () => {
+  it("computes something sensible ", () => {
+    expect(getResourceLimits(pod())).toEqual({
+      cpus: 2,
+      mem: 512,
+    });
+  });
+  it("can compute scaled values", () => {
+    expect(getResourceLimits(pod(), true)).toEqual({
+      cpus: 4,
+      mem: 1024,
+    });
+  });
+  it("incorporates resources when calculating limits", () => {
+    expect(
+      getResourceLimits(
+        pod([
+          { resources: { mem: 128 }, resourceLimits: { mem: 256 } },
+          { resources: { mem: 128 }, resourceLimits: { mem: undefined } },
+        ])
+      )
+    ).toEqual({ cpus: undefined, mem: 384 });
+  });
+  it("incorporates resources when calculating limits", () => {
+    expect(
+      getResourceLimits(
+        pod([
+          { resources: { mem: 128 }, resourceLimits: { mem: 256 } },
+          { resources: { mem: 128 }, resourceLimits: { mem: undefined } },
+        ]),
+        true
+      )
+    ).toEqual({ cpus: undefined, mem: 768 });
+  });
+});

plugins/services/src/js/columns/resourceLimits.ts

@@ -1,12 +1,16 @@
 import Pod from "../structs/Pod";
 import Service from "../structs/Service";
 import ServiceTree from "../structs/ServiceTree";
+import Application from "../structs/Application";
 
-type TreeItem = Service | Pod | ServiceTree;
-type ResourceLimit = number | "unlimited";
-type ResourceLimits = { cpus: ResourceLimit; mem: ResourceLimit };
+type TreeItem = Application | Service | Pod | ServiceTree;
+type ResourceLimit = number | "unlimited" | undefined;
+export type ResourceLimits = { cpus: ResourceLimit; mem: ResourceLimit };
 
-export const getResourceLimits = (treeItem: TreeItem): ResourceLimits => {
+export const getResourceLimits = (
+  treeItem: TreeItem,
+  multiplyScale = false
+): ResourceLimits => {
   if (treeItem instanceof ServiceTree) {
     return treeItem.reduceItems(
       (acc: ResourceLimits, item: TreeItem) => {
@@ -24,40 +28,42 @@ export const getResourceLimits = (treeItem: TreeItem): ResourceLimits => {
     const limits = treeItem.spec.containers.reduce(
       (acc: ResourceLimits, container) => {
         const { cpus, mem } = getLimits(container);
-        acc.cpus = addResourceLimit(acc.cpus, cpus);
-        acc.mem = addResourceLimit(acc.mem, mem);
+        const resources = container.resources;
+        acc.cpus = addResourceLimit(acc.cpus, cpus || resources?.cpus);
+        acc.mem = addResourceLimit(acc.mem, mem || resources?.mem);
         return acc;
       },
-      { cpus: 0, mem: 0, ...treeItem.spec.executorResources }
+      { cpus: undefined, mem: undefined, ...treeItem.spec.executorResources }
     );
-    return multInstances(limits, instances);
+    return multiplyScale ? multInstances(limits, instances) : limits;
   }
   if (treeItem instanceof Service) {
-    return multInstances(getLimits(treeItem), instances);
+    const limits = getLimits(treeItem);
+    return multiplyScale ? multInstances(limits, instances) : limits;
   }
   throw Error(
     `could not get resource limits for unexpected row in table: ${treeItem}`
   );
 };
 
-// TODO: type thing. it's currently either a Service or a Pod.spec.containers[]
-// we currently fall back to using the allocated resources as the limit in
-// case a limit is not set until marathon gives us the correct limits according to whether cGroups are shared or not.
 const getLimits = (thing) => {
-  const { cpus = 0, mem = 0 } = thing.getResources?.() || thing.resources;
-  return {
-    cpus: thing.resourceLimits?.cpus ?? cpus,
-    mem: thing.resourceLimits?.mem ?? mem,
-  };
+  return { cpus: thing.resourceLimits?.cpus, mem: thing.resourceLimits?.mem };
 };
 
 const multInstances = (limits, instances) => ({
   cpus: multResourceLimit(limits.cpus, instances),
   mem: multResourceLimit(limits.mem, instances),
 });
 
+// when one is "unlimited", return unlimited
+// when both are undefined, return undefined
+// else add with converting undefined to 0.
 const addResourceLimit = (a: ResourceLimit, b: ResourceLimit) =>
-  a === "unlimited" || b === "unlimited" ? "unlimited" : a + b;
+  a === "unlimited" || b === "unlimited"
+    ? "unlimited"
+    : a === undefined && b === undefined
+    ? undefined
+    : (a || 0) + (b || 0);
 
 const multResourceLimit = (rl: ResourceLimit, mult: number) =>
-  rl === "unlimited" ? rl : rl * mult;
+  rl === undefined || rl === "unlimited" ? rl : rl * mult;

plugins/services/src/js/containers/tasks/TaskTable.tsx

@@ -45,7 +45,8 @@ const tableColumnClasses = {
 const getService = (taskId) =>
   DCOSStore.serviceTree.getServiceFromTaskID(taskId);
 
-class TaskTable extends React.Component {
+export default class extends React.Component {
+  static contextTypes = { router: routerShape.isRequired };
   static defaultProps = {
     className:
       "table table-flush table-borderless-outer table-borderless-inner-columns flush-bottom",
@@ -403,9 +404,7 @@ class TaskTable extends React.Component {
     return (
       <span>
         {Units.formatResource("cpus", task.resources.cpus)}{" "}
-        {cpusLimit !== 0
-          ? ` / ${Units.formatResource("cpus", cpusLimit)}`
-          : null}
+        {cpusLimit ? ` / ${Units.formatResource("cpus", cpusLimit)}` : null}
       </span>
     );
   };
@@ -518,9 +517,3 @@ class TaskTable extends React.Component {
     );
   }
 }
-
-TaskTable.contextTypes = {
-  router: routerShape.isRequired,
-};
-
-export default TaskTable;

plugins/services/src/js/structs/Service.ts

@@ -106,30 +106,13 @@ export default class Service extends Item {
 
   getResources() {
     const instances = this.getInstancesCount();
-    const {
-      cpus = 0,
-      mem = 0,
-      gpus = 0,
-      disk = 0,
-    } = this.getSpec().getResources();
-    let executorCpus = 0;
-    let executorMem = 0;
-    let executorGpus = 0;
-    let executorDisk = 0;
-
-    if (this.getSpec().get("executorResources")) {
-      const executor = this.getSpec().get("executorResources");
-      executorCpus = executor.cpus ? executor.cpus : 0;
-      executorMem = executor.mem ? executor.mem : 0;
-      executorGpus = executor.gpus ? executor.gpus : 0;
-      executorDisk = executor.disk ? executor.disk : 0;
-    }
-
+    const resources = this.getSpec().getResources();
+    const executor = this.getSpec()?.get("executorResources");
     return {
-      cpus: (cpus + executorCpus) * instances,
-      mem: (mem + executorMem) * instances,
-      gpus: (gpus + executorGpus) * instances,
-      disk: (disk + executorDisk) * instances,
+      cpus: (resources.cpus + (executor?.cpus || 0)) * instances,
+      mem: (resources.mem + (executor?.mem || 0)) * instances,
+      gpus: (resources.gpus + (executor?.gpus || 0)) * instances,
+      disk: (resources.disk + (executor?.disk || 0)) * instances,
     };
   }
 

src/js/__tests__/__snapshots__/typecheck-test.ts.snap

@@ -2658,7 +2658,6 @@ plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2339: Property '
 plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2339: Property 'onCheckboxChange' does not exist on type *.
 plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2339: Property 'tasks' does not exist on type *.
 plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2769: No overload matches this call.
-plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2551: Property 'contextTypes' does not exist on type *. Did you mean 'contextType'?
 plugins/services/src/js/containers/tasks/TaskTable.tsx: error TS2339: Property 'i18n' does not exist on type *.
 plugins/services/src/js/containers/tasks/TasksContainer.tsx: error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'never'.
 plugins/services/src/js/containers/tasks/TasksContainer.tsx: error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'never'.