From 9ae82c1b0476ea07ca9329dec0c0f8dd6315042c Mon Sep 17 00:00:00 2001 From: Alexandre Alves Date: Wed, 31 Jan 2024 15:55:48 +0000 Subject: [PATCH 1/5] update logic of hasError in prov cluster model --- .../models/provisioning.cattle.io.cluster.js | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/shell/models/provisioning.cattle.io.cluster.js b/shell/models/provisioning.cattle.io.cluster.js index 08b88236c01..bf7743f39d8 100644 --- a/shell/models/provisioning.cattle.io.cluster.js +++ b/shell/models/provisioning.cattle.io.cluster.js @@ -891,7 +891,37 @@ 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 (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 errorConditions = this.status?.condition.filter((c) => c.error === true); + const readyConditions = this.status?.condition.filter((c) => c.type === 'Ready'); + + const lastErrorCondition = errorConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); + const lastReadyCondition = readyConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); + + const lastErrorConditionDate = new Date(lastErrorCondition.lastUpdateTime) || null; + const lastReadyConditionDate = new Date(lastReadyCondition.lastUpdateTime) || null; + + // fallback is false because something might be wrong with the condition 'lastUpdateTime' prop + return lastErrorConditionDate && lastReadyConditionDate ? lastReadyConditionDate.getTime() > lastErrorConditionDate.getTime() : false; + } + + // here we just fallback to the previous original logic + return false; + } + + // no conditions, no assumptions here + return false; } get namespaceLocation() { From eeb6467ce39d9e813b27448b301193433f8157d3 Mon Sep 17 00:00:00 2001 From: Alexandre Alves Date: Wed, 31 Jan 2024 17:03:12 +0000 Subject: [PATCH 2/5] working on unit tests --- .../provisioning.cattle.io.cluster.test.ts | 102 ++++++++++++++++++ .../models/provisioning.cattle.io.cluster.js | 8 +- 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts b/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts index 8c785666f75..ec6e9e93b0d 100644 --- 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: Bool) => { + 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 index bf7743f39d8..c71387f98e8 100644 --- a/shell/models/provisioning.cattle.io.cluster.js +++ b/shell/models/provisioning.cattle.io.cluster.js @@ -903,8 +903,8 @@ export default class ProvCluster extends SteveModel { return true; } - const errorConditions = this.status?.condition.filter((c) => c.error === true); - const readyConditions = this.status?.condition.filter((c) => c.type === 'Ready'); + const errorConditions = this.status?.conditions.filter((c) => c.error === true); + const readyConditions = this.status?.conditions.filter((c) => c.type === 'Ready'); const lastErrorCondition = errorConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); const lastReadyCondition = readyConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); @@ -912,6 +912,10 @@ export default class ProvCluster extends SteveModel { const lastErrorConditionDate = new Date(lastErrorCondition.lastUpdateTime) || null; const lastReadyConditionDate = new Date(lastReadyCondition.lastUpdateTime) || null; + console.log('this.status?.conditions', lastReadyConditionDate.getTime()); + console.log('lastReadyConditionDate.getTime()', lastReadyConditionDate.getTime()); + console.log('lastErrorConditionDate.getTime()', lastErrorConditionDate.getTime()); + // fallback is false because something might be wrong with the condition 'lastUpdateTime' prop return lastErrorConditionDate && lastReadyConditionDate ? lastReadyConditionDate.getTime() > lastErrorConditionDate.getTime() : false; } From 7273c36a039e51ebb4d72bc2068597cbfa046c96 Mon Sep 17 00:00:00 2001 From: Alexandre Alves Date: Wed, 31 Jan 2024 17:35:56 +0000 Subject: [PATCH 3/5] fix code logic and finish unit tests --- shell/models/provisioning.cattle.io.cluster.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/shell/models/provisioning.cattle.io.cluster.js b/shell/models/provisioning.cattle.io.cluster.js index c71387f98e8..29d7d751358 100644 --- a/shell/models/provisioning.cattle.io.cluster.js +++ b/shell/models/provisioning.cattle.io.cluster.js @@ -912,12 +912,8 @@ export default class ProvCluster extends SteveModel { const lastErrorConditionDate = new Date(lastErrorCondition.lastUpdateTime) || null; const lastReadyConditionDate = new Date(lastReadyCondition.lastUpdateTime) || null; - console.log('this.status?.conditions', lastReadyConditionDate.getTime()); - console.log('lastReadyConditionDate.getTime()', lastReadyConditionDate.getTime()); - console.log('lastErrorConditionDate.getTime()', lastErrorConditionDate.getTime()); - // fallback is false because something might be wrong with the condition 'lastUpdateTime' prop - return lastErrorConditionDate && lastReadyConditionDate ? lastReadyConditionDate.getTime() > lastErrorConditionDate.getTime() : false; + return lastErrorConditionDate && lastReadyConditionDate ? lastErrorConditionDate.getTime() > lastReadyConditionDate.getTime() : false; } // here we just fallback to the previous original logic From 6af2ab5b196414cd0b8c52c10f90f7626681a450 Mon Sep 17 00:00:00 2001 From: Alexandre Alves Date: Wed, 31 Jan 2024 17:38:46 +0000 Subject: [PATCH 4/5] fix lint issue --- shell/models/__tests__/provisioning.cattle.io.cluster.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts b/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts index ec6e9e93b0d..8d75daf9dd4 100644 --- a/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts +++ b/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts @@ -180,7 +180,7 @@ describe('class ProvCluster', () => { 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: Bool) => { + 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); From 4164071176bd32e78bb54bc53284b7e476d2fdc1 Mon Sep 17 00:00:00 2001 From: Alexandre Alves Date: Thu, 1 Feb 2024 09:49:22 +0000 Subject: [PATCH 5/5] address pr comments --- shell/models/provisioning.cattle.io.cluster.js | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/shell/models/provisioning.cattle.io.cluster.js b/shell/models/provisioning.cattle.io.cluster.js index 29d7d751358..756118561a3 100644 --- a/shell/models/provisioning.cattle.io.cluster.js +++ b/shell/models/provisioning.cattle.io.cluster.js @@ -897,30 +897,20 @@ export default class ProvCluster extends SteveModel { // 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 errorConditions = this.status?.conditions.filter((c) => c.error === true); - const readyConditions = this.status?.conditions.filter((c) => c.type === 'Ready'); + 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)); - const lastErrorCondition = errorConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); - const lastReadyCondition = readyConditions.reduce((a, b) => ((a.lastUpdateTime > b.lastUpdateTime) ? a : b)); - - const lastErrorConditionDate = new Date(lastErrorCondition.lastUpdateTime) || null; - const lastReadyConditionDate = new Date(lastReadyCondition.lastUpdateTime) || null; - - // fallback is false because something might be wrong with the condition 'lastUpdateTime' prop - return lastErrorConditionDate && lastReadyConditionDate ? lastErrorConditionDate.getTime() > lastReadyConditionDate.getTime() : false; + return mostRecentCondition.error; } - - // here we just fallback to the previous original logic - return false; } - // no conditions, no assumptions here return false; }