Update logic of 'hasError' in 'provisioning.cattle.io.cluster' model (r…

…ancher#10366) * update logic of hasError in prov cluster model * working on unit tests * fix code logic and finish unit tests * fix lint issue * address pr comments --------- Co-authored-by: Alexandre Alves <[email protected]>
aalves08 · Feb 2, 2024 · 21327c5 · 21327c5
1 parent 3099aef
commit 21327c5
Show file tree

Hide file tree

Showing 2 changed files with 123 additions and 1 deletion.
diff --git a/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts b/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts
@@ -87,4 +87,106 @@ describe('class ProvCluster', () => {
     }
     );
   });
+
+  describe('hasError', () => {
+    const conditionsWithoutError = [
+      {
+        error:          false,
+        lastUpdateTime: '2022-10-17T23:09:15Z',
+        status:         'True',
+        transitioning:  false,
+        type:           'Ready'
+      },
+    ];
+
+    const conditionsWithoutReady = [
+      {
+        error:          true,
+        lastUpdateTime: '2022-10-17T23:09:15Z',
+        status:         'False',
+        message:        'some-error-message',
+        transitioning:  false,
+        type:           'Pending'
+      },
+    ];
+
+    const noConditions:[] = [];
+
+    const conditionsWithReadyLatest = [
+      {
+        error:          true,
+        lastUpdateTime: '2022-10-17T23:09:15Z',
+        status:         'False',
+        message:        'some-error-message',
+        transitioning:  false,
+        type:           'Pending'
+      },
+      {
+        error:          false,
+        lastUpdateTime: '2023-10-17T23:09:15Z',
+        status:         'True',
+        transitioning:  false,
+        type:           'Ready'
+      }
+    ];
+
+    const conditionsWithErrorLatest = [
+      {
+        error:          false,
+        lastUpdateTime: '2022-10-17T23:09:15Z',
+        status:         'True',
+        transitioning:  false,
+        type:           'Ready'
+      },
+      {
+        error:          true,
+        lastUpdateTime: '2023-10-17T23:09:15Z',
+        status:         'False',
+        message:        'some-error-message',
+        transitioning:  false,
+        type:           'Pending'
+      }
+    ];
+
+    const conditionsWithProblemInLastUpdateTimeProp = [
+      {
+        error:          true,
+        lastUpdateTime: '',
+        status:         'False',
+        message:        'some-error-message',
+        transitioning:  false,
+        type:           'Pending'
+      },
+      {
+        error:          false,
+        lastUpdateTime: '2023-10-17T23:09:15Z',
+        status:         'True',
+        transitioning:  false,
+        type:           'Ready'
+      }
+    ];
+
+    const testCases = [
+      ['conditionsWithoutError', conditionsWithoutError, false],
+      ['conditionsWithoutReady', conditionsWithoutReady, true],
+      ['noConditions', noConditions, false],
+      ['conditionsWithReadyLatest', conditionsWithReadyLatest, false],
+      ['conditionsWithErrorLatest', conditionsWithErrorLatest, true],
+      ['conditionsWithProblemInLastUpdateTimeProp', conditionsWithProblemInLastUpdateTimeProp, false],
+    ];
+
+    const resetMocks = () => {
+      // Clear all mock function calls
+      jest.clearAllMocks();
+    };
+
+    it.each(testCases)('should return the hasError value properly based on the "status.conditions" props data for testcase %p', (testName: string, conditions: Array, expected: Boolean) => {
+      const ctx = { rootGetters: { 'management/byId': jest.fn() } };
+      const cluster = new ProvCluster({ status: { conditions } }, ctx);
+
+      expect(cluster.hasError).toBe(expected);
+      resetMocks();
+    }
+    );
+  });
 });
diff --git a/shell/models/provisioning.cattle.io.cluster.js b/shell/models/provisioning.cattle.io.cluster.js
@@ -891,7 +891,27 @@ export default class ProvCluster extends SteveModel {
   }
 
   get hasError() {
-    return this.status?.conditions?.some((condition) => condition.error === true);
+    // Before we were just checking for this.status?.conditions?.some((condition) => condition.error === true)
+    // but this is wrong as an error might exist but it might not be meaningful in the context of readiness of a cluster
+    // which is what this 'hasError' is used for.
+    // We now check if there's a ready condition after an error, which helps dictate the readiness of a cluster
+    // Based on the findings in https://github.com/rancher/dashboard/issues/10043
+    if (this.status?.conditions && this.status?.conditions.length) {
+      // if there are errors, we compare with how recent the "Ready" condition is compared to that error, otherwise we just move on
+      if (this.status?.conditions.some((c) => c.error === true)) {
+        // there's no ready condition and has an error, mark it
+        if (!this.status?.conditions.some((c) => c.type === 'Ready')) {
+          return true;
+        }
+
+        const filteredConditions = this.status?.conditions.filter((c) => c.error === true || c.type === 'Ready');
+        const mostRecentCondition = filteredConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b));
+
+        return mostRecentCondition.error;
+      }
+    }
+
+    return false;
   }
 
   get namespaceLocation() {