diff --git a/src/juju/api.test.ts b/src/juju/api.test.ts index e43a3a147..3944dec59 100644 --- a/src/juju/api.test.ts +++ b/src/juju/api.test.ts @@ -369,9 +369,9 @@ describe("Juju API", () => { () => state, ); jest.advanceTimersByTime(LOGIN_TIMEOUT); - await expect(response).resolves.toBeNull(); + await expect(response).rejects.toStrictEqual(new Error("timeout")); expect(console.error).toHaveBeenCalledWith( - "error connecting to model:", + "Error connecting to model:", "abc123", new Error("timeout"), ); @@ -456,15 +456,18 @@ describe("Juju API", () => { jest .spyOn(jujuLib, "connectAndLogin") .mockImplementation(async () => loginResponse); - const response = await fetchModelStatus( + const response = fetchModelStatus( "abc123", "wss://example.com/api", () => state, ); - expect(response).toBeUndefined(); + await expect(response).rejects.toStrictEqual( + new Error("Unable to fetch the status. Uh oh!"), + ); expect(console.error).toHaveBeenCalledWith( - "Unable to fetch the status.", - "Uh oh!", + "Error connecting to model:", + "abc123", + new Error("Unable to fetch the status. Uh oh!"), ); console.error = consoleError; }); @@ -535,12 +538,20 @@ describe("Juju API", () => { .spyOn(jujuLib, "connectAndLogin") .mockImplementation(async () => loginResponse); const dispatch = jest.fn(); - await fetchAndStoreModelStatus( + const response = fetchAndStoreModelStatus( "abc123", "wss://example.com/api", dispatch, () => state, ); + await expect(response).rejects.toStrictEqual( + new Error("Unable to fetch the status. "), + ); + expect(console.error).toHaveBeenCalledWith( + "Error connecting to model:", + "abc123", + new Error("Unable to fetch the status. "), + ); expect(dispatch).not.toHaveBeenCalled(); console.error = consoleError; }); @@ -713,6 +724,97 @@ describe("Juju API", () => { new Error("Error while trying to dispatch!"), ); }); + + it("should return a rejected promise when retrieving data for some models fails", async () => { + // The dispatch of updateModelInfo fails only for the first model. + // This would make 50% (1/2) of all models error out. + const dispatch = jest.fn().mockImplementationOnce(() => { + throw new Error("Error while dispatching updateModelInfo!"); + }); + const abc123 = modelInfoResultsFactory.build({ + results: [ + modelInfoResultFactory.build({ + result: modelInfoFactory.build({ + uuid: "abc123", + }), + }), + ], + }); + const def456 = modelInfoResultsFactory.build({ + results: [ + modelInfoResultFactory.build({ + result: modelInfoFactory.build({ + uuid: "def456", + }), + }), + ], + }); + const conn = { + facades: { + modelManager: { + modelInfo: jest + .fn() + .mockResolvedValueOnce(abc123) + .mockResolvedValueOnce(def456), + }, + }, + } as unknown as Connection; + const response = fetchAllModelStatuses( + "wss://example.com/api", + ["abc123", "def456"], + conn, + dispatch, + () => state, + ); + await expect(response).rejects.toStrictEqual( + new Error("Unable to load some models."), + ); + }); + + it("should return a rejected promise when retrieving data for all models", async () => { + // The dispatch of updateModelInfo fails for all models. + const dispatch = jest.fn().mockImplementation(() => { + throw new Error("Error while dispatching updateModelInfo!"); + }); + const abc123 = modelInfoResultsFactory.build({ + results: [ + modelInfoResultFactory.build({ + result: modelInfoFactory.build({ + uuid: "abc123", + }), + }), + ], + }); + const def456 = modelInfoResultsFactory.build({ + results: [ + modelInfoResultFactory.build({ + result: modelInfoFactory.build({ + uuid: "def456", + }), + }), + ], + }); + const conn = { + facades: { + modelManager: { + modelInfo: jest + .fn() + .mockResolvedValueOnce(abc123) + .mockResolvedValueOnce(def456), + }, + }, + } as unknown as Connection; + const response = fetchAllModelStatuses( + "wss://example.com/api", + ["abc123", "def456"], + conn, + dispatch, + () => state, + ); + await expect(response).rejects.toStrictEqual( + new Error("Unable to load models."), + ); + }); }); describe("fetchControllerList", () => { diff --git a/src/juju/api.ts b/src/juju/api.ts index 853eb3f80..649727dd5 100644 --- a/src/juju/api.ts +++ b/src/juju/api.ts @@ -42,6 +42,7 @@ import type { Credential } from "store/general/types"; import { actions as jujuActions } from "store/juju"; import { addControllerCloudRegion } from "store/juju/thunks"; import type { Controller as JujuController } from "store/juju/types"; +import { ModelsError } from "store/middleware/model-poller"; import type { RootState, Store } from "store/store"; import { getModelByUUID } from "../store/juju/selectors"; @@ -260,8 +261,7 @@ export async function fetchModelStatus( // a location to notify the user. In the new watcher model that's // being implemented we will be able to surface this error in the // model details page. - console.error("Unable to fetch the status.", status); - return; + throw new Error(`Unable to fetch the status. ${status ?? ""}`); } } @@ -284,7 +284,8 @@ export async function fetchModelStatus( } logout(); } catch (error) { - console.error("error connecting to model:", modelUUID, error); + console.error("Error connecting to model:", modelUUID, error); + throw error; } } return status; @@ -344,49 +345,64 @@ export async function fetchAllModelStatuses( getState: () => RootState, ) { const queue = new Limiter({ concurrency: 1 }); + let modelErrorCount = 0; modelUUIDList.forEach((modelUUID) => { queue.push(async (done) => { if (isLoggedIn(getState(), wsControllerURL)) { - const modelWsControllerURL = getModelByUUID( - getState(), - modelUUID, - )?.wsControllerURL; - if (modelWsControllerURL) { - await fetchAndStoreModelStatus( + try { + const modelWsControllerURL = getModelByUUID( + getState(), modelUUID, - modelWsControllerURL, - dispatch, - getState, - ); - } - const modelInfo = await fetchModelInfo(conn, modelUUID); - if (modelInfo) { - dispatch( - jujuActions.updateModelInfo({ - modelInfo, - wsControllerURL, - }), - ); - } - if (modelInfo?.results[0].result?.["is-controller"]) { - // If this is a controller model then update the - // controller data with this model data. - dispatch( - addControllerCloudRegion({ wsControllerURL, modelInfo }), - ).catch((error) => - console.error( - "Error when trying to add controller cloud and region data.", - error, - ), - ); + )?.wsControllerURL; + if (modelWsControllerURL) { + await fetchAndStoreModelStatus( + modelUUID, + modelWsControllerURL, + dispatch, + getState, + ); + } + const modelInfo = await fetchModelInfo(conn, modelUUID); + if (modelInfo) { + dispatch( + jujuActions.updateModelInfo({ + modelInfo, + wsControllerURL, + }), + ); + } + if (modelInfo?.results[0].result?.["is-controller"]) { + // If this is a controller model then update the + // controller data with this model data. + dispatch( + addControllerCloudRegion({ wsControllerURL, modelInfo }), + ).catch((error) => + console.error( + "Error when trying to add controller cloud and region data.", + error, + ), + ); + } + } catch (error) { + modelErrorCount++; } } done(); }); }); - return new Promise((resolve) => { + return new Promise((resolve, reject) => { queue.onDone(() => { - resolve(); + // If errors appear in more than 10% of models, the promise should be + // rejected and the error further handled in modelPollerMiddleware(). + modelErrorCount >= 0.1 * modelUUIDList.length + ? reject( + new Error( + modelErrorCount === modelUUIDList.length + ? ModelsError.LOAD_ALL_MODELS + : ModelsError.LOAD_SOME_MODELS, + ), + ) + : resolve(); }); }); } diff --git a/src/pages/ModelsIndex/ModelsIndex.test.tsx b/src/pages/ModelsIndex/ModelsIndex.test.tsx index 250348710..5c6a228f0 100644 --- a/src/pages/ModelsIndex/ModelsIndex.test.tsx +++ b/src/pages/ModelsIndex/ModelsIndex.test.tsx @@ -173,4 +173,26 @@ describe("Models Index page", () => { }); expect(window.location.search).toBe(`?${params.toString()}`); }); + + it("should display the error notification", async () => { + state.juju.modelsError = "Oops!"; + renderComponent(, { state }); + expect(screen.getByText(new RegExp(/Oops!/))).toBeInTheDocument(); + }); + + it("should refresh the window when pressing the button in error notification", async () => { + const location = window.location; + Object.defineProperty(window, "location", { + value: { ...location, reload: jest.fn() }, + }); + + state.juju.modelsError = "Oops!"; + renderComponent(, { state }); + await userEvent.click(screen.getByRole("button", { name: "refreshing" })); + expect(window.location.reload).toHaveBeenCalled(); + + Object.defineProperty(window, "location", { + value: location, + }); + }); }); diff --git a/src/pages/ModelsIndex/ModelsIndex.tsx b/src/pages/ModelsIndex/ModelsIndex.tsx index ba1e57075..a7de8744a 100644 --- a/src/pages/ModelsIndex/ModelsIndex.tsx +++ b/src/pages/ModelsIndex/ModelsIndex.tsx @@ -1,4 +1,8 @@ -import { SearchAndFilter } from "@canonical/react-components"; +import { + Button, + Notification, + SearchAndFilter, +} from "@canonical/react-components"; import type { SearchAndFilterChip } from "@canonical/react-components/dist/components/SearchAndFilter/types"; import type { ReactNode } from "react"; import { useSelector } from "react-redux"; @@ -16,6 +20,7 @@ import { getGroupedModelStatusCounts, getModelData, getModelListLoaded, + getModelsError, hasModels, } from "store/juju/selectors"; import { pluralize } from "store/juju/utils/models"; @@ -55,6 +60,7 @@ export default function Models() { custom: queryParams.custom, }; + const modelsError = useAppSelector(getModelsError); const modelsLoaded = useAppSelector(getModelListLoaded); const hasSomeModels = useSelector(hasModels); // loop model data and pull out filter panel data @@ -196,6 +202,15 @@ export default function Models() { titleComponent="div" titleClassName="u-no-max-width u-full-width" > + {modelsError ? ( + + {modelsError} Try{" "} + {" "} + the page. + + ) : null} {content} ); diff --git a/src/store/juju/actions.test.ts b/src/store/juju/actions.test.ts index 5d82b6a88..b31aafa21 100644 --- a/src/store/juju/actions.test.ts +++ b/src/store/juju/actions.test.ts @@ -95,6 +95,21 @@ describe("actions", () => { }); }); + it("updateModelsError", () => { + expect( + actions.updateModelsError({ + modelsError: null, + wsControllerURL: "wss://test.example.com", + }), + ).toStrictEqual({ + type: "juju/updateModelsError", + payload: { + modelsError: null, + wsControllerURL: "wss://test.example.com", + }, + }); + }); + it("clearModelData", () => { expect(actions.clearModelData()).toStrictEqual({ type: "juju/clearModelData", diff --git a/src/store/juju/reducers.test.ts b/src/store/juju/reducers.test.ts index 8c4622792..4938fff12 100644 --- a/src/store/juju/reducers.test.ts +++ b/src/store/juju/reducers.test.ts @@ -160,6 +160,26 @@ describe("reducers", () => { }); }); + it("updateModelsError", () => { + const state = jujuStateFactory.build({ + modelData: { + abc123: model, + }, + }); + expect( + reducer( + state, + actions.updateModelsError({ + modelsError: null, + wsControllerURL: "wss://example.com", + }), + ), + ).toStrictEqual({ + ...state, + modelsError: null, + }); + }); + it("clearModelData", () => { const state = jujuStateFactory.build({ modelData: { diff --git a/src/store/juju/selectors.ts b/src/store/juju/selectors.ts index 9ce3576db..f0d63f0b9 100644 --- a/src/store/juju/selectors.ts +++ b/src/store/juju/selectors.ts @@ -176,6 +176,11 @@ export const getModelData = createSelector( (sliceState) => sliceState.modelData ?? null, ); +export const getModelsError = createSelector( + [slice], + (sliceState) => sliceState.modelsError, +); + /** Fetches the controller data from state. @param state The application state. diff --git a/src/store/juju/slice.ts b/src/store/juju/slice.ts index d83078d5e..5b69021c6 100644 --- a/src/store/juju/slice.ts +++ b/src/store/juju/slice.ts @@ -46,6 +46,7 @@ const slice = createSlice({ }, controllers: null, models: {}, + modelsError: null, modelsLoaded: false, modelData: {}, modelWatcherData: {}, @@ -128,9 +129,20 @@ const slice = createSlice({ state.modelData[modelInfo.uuid].info = modelInfo; } }, + updateModelsError: ( + state, + action: PayloadAction< + { + modelsError: string | null; + } & WsControllerURLParam + >, + ) => { + state.modelsError = action.payload.modelsError; + }, clearModelData: (state) => { state.modelData = {}; state.models = {}; + state.modelsError = null; state.modelsLoaded = false; }, clearControllerData: (state) => { diff --git a/src/store/juju/types.ts b/src/store/juju/types.ts index 0fd12c063..45a3acb88 100644 --- a/src/store/juju/types.ts +++ b/src/store/juju/types.ts @@ -76,6 +76,7 @@ export type JujuState = { crossModelQuery: CrossModelQueryState; controllers: Controllers | null; models: ModelsList; + modelsError: string | null; modelsLoaded: boolean; modelData: ModelDataList; modelWatcherData?: ModelWatcherData; diff --git a/src/store/middleware/model-poller.test.ts b/src/store/middleware/model-poller.test.ts index 8a2716305..193bda80f 100644 --- a/src/store/middleware/model-poller.test.ts +++ b/src/store/middleware/model-poller.test.ts @@ -15,7 +15,7 @@ import { jujuStateFactory, } from "testing/factories/juju/juju"; -import { LoginError, modelPollerMiddleware } from "./model-poller"; +import { LoginError, ModelsError, modelPollerMiddleware } from "./model-poller"; jest.mock("juju/api", () => ({ disableControllerUUIDMasking: jest @@ -69,12 +69,15 @@ describe("model poller", () => { }, }, }); + const consoleError = console.error; beforeEach(() => { jest.useFakeTimers(); next = jest.fn(); fakeStore = { - getState: jest.fn(() => ({})), + getState: jest.fn(() => ({ + juju: jujuStateFactory.build(), + })), dispatch: jest.fn(), }; conn = { @@ -96,6 +99,11 @@ describe("model poller", () => { juju = { logout: jest.fn(), } as unknown as Client; + console.error = jest.fn(); + }); + + afterEach(() => { + console.error = consoleError; }); const runMiddleware = async (actionOverrides?: Partial) => { @@ -441,6 +449,138 @@ describe("model poller", () => { ); }); + it("should remove models error from state if no error occurs", async () => { + jest.spyOn(fakeStore, "getState").mockReturnValue({ + ...storeState, + juju: jujuStateFactory.build({ + controllers: { + [wsControllerURL]: [controllerFactory.build()], + }, + modelsError: "Oops!", + }), + }); + jest.spyOn(jujuModule, "loginWithBakery").mockImplementation(async () => ({ + conn, + intervalId, + juju, + })); + await runMiddleware(); + expect(fakeStore.dispatch).toHaveBeenCalledWith( + jujuActions.updateModelsError({ + modelsError: null, + wsControllerURL, + }), + ); + }); + + it("should display error when unable to load all models", async () => { + jest.spyOn(fakeStore, "getState").mockReturnValue(storeState); + jest.spyOn(jujuModule, "loginWithBakery").mockImplementation(async () => ({ + conn, + intervalId, + juju, + })); + jest + .spyOn(jujuModule, "fetchAllModelStatuses") + .mockRejectedValue(new Error(ModelsError.LOAD_ALL_MODELS)); + await runMiddleware(); + expect(console.error).toHaveBeenCalledWith( + ModelsError.LOAD_ALL_MODELS, + new Error(ModelsError.LOAD_ALL_MODELS), + ); + expect(fakeStore.dispatch).toHaveBeenCalledWith( + jujuActions.updateModelsError({ + modelsError: ModelsError.LOAD_ALL_MODELS, + wsControllerURL, + }), + ); + }); + + it("should display error when unable to load some models", async () => { + jest.spyOn(fakeStore, "getState").mockReturnValue(storeState); + jest.spyOn(jujuModule, "loginWithBakery").mockImplementation(async () => ({ + conn, + intervalId, + juju, + })); + jest + .spyOn(jujuModule, "fetchAllModelStatuses") + .mockRejectedValue(new Error(ModelsError.LOAD_SOME_MODELS)); + await runMiddleware(); + expect(console.error).toHaveBeenCalledWith( + ModelsError.LOAD_SOME_MODELS, + new Error(ModelsError.LOAD_SOME_MODELS), + ); + expect(fakeStore.dispatch).toHaveBeenCalledWith( + jujuActions.updateModelsError({ + modelsError: ModelsError.LOAD_SOME_MODELS, + wsControllerURL, + }), + ); + }); + + it("should display error when unable to load latest models", async () => { + jest.spyOn(fakeStore, "getState").mockReturnValue(storeState); + jest.spyOn(jujuModule, "loginWithBakery").mockImplementation(async () => ({ + conn, + intervalId, + juju, + })); + jest + .spyOn(jujuModule, "fetchAllModelStatuses") + .mockImplementationOnce( + async () => new Promise((resolve) => resolve()), + ) + .mockImplementationOnce( + async () => + new Promise((resolve, reject) => + reject(new Error(ModelsError.LOAD_SOME_MODELS)), + ), + ); + runMiddleware({ + payload: { controllers, isJuju: true, poll: 1 }, + }).catch(console.error); + await new Promise(jest.requireActual("timers").setImmediate); + jest.advanceTimersByTime(30000); + // Resolve the async calls again. + await new Promise(jest.requireActual("timers").setImmediate); + expect(console.error).toHaveBeenCalledWith( + ModelsError.LOAD_LATEST_MODELS, + new Error(ModelsError.LOAD_SOME_MODELS), + ); + expect(fakeStore.dispatch).toHaveBeenCalledWith( + jujuActions.updateModelsError({ + modelsError: ModelsError.LOAD_LATEST_MODELS, + wsControllerURL, + }), + ); + }); + + it("should display error when unable to update model list", async () => { + jest.spyOn(fakeStore, "getState").mockReturnValue(storeState); + jest.spyOn(jujuModule, "loginWithBakery").mockImplementation(async () => ({ + conn, + intervalId, + juju, + })); + jest.spyOn(jujuActions, "updateModelList").mockImplementation( + jest.fn(() => { + throw new Error(ModelsError.LIST_OR_UPDATE_MODELS); + }), + ); + await runMiddleware(); + expect(console.error).toHaveBeenCalledWith( + ModelsError.LIST_OR_UPDATE_MODELS, + new Error(ModelsError.LIST_OR_UPDATE_MODELS), + ); + expect(fakeStore.dispatch).toHaveBeenCalledWith( + jujuActions.updateModelsError({ + modelsError: ModelsError.LIST_OR_UPDATE_MODELS, + wsControllerURL, + }), + ); + }); + it("updates models every 30 seconds", async () => { jest.spyOn(fakeStore, "getState").mockReturnValue(storeState); const fetchAllModelStatuses = jest.spyOn( @@ -452,11 +592,11 @@ describe("model poller", () => { intervalId, juju, })); - runMiddleware({ payload: { controllers, isJuju: true, poll: 2 } }).catch( + runMiddleware({ payload: { controllers, isJuju: true, poll: 1 } }).catch( console.error, ); await new Promise(jest.requireActual("timers").setImmediate); - jest.advanceTimersByTime(3000000); + jest.advanceTimersByTime(30000); // Resolve the async calls again. await new Promise(jest.requireActual("timers").setImmediate); expect(next).not.toHaveBeenCalled(); diff --git a/src/store/middleware/model-poller.ts b/src/store/middleware/model-poller.ts index 22ab2d6ab..9cd9d8984 100644 --- a/src/store/middleware/model-poller.ts +++ b/src/store/middleware/model-poller.ts @@ -26,6 +26,13 @@ export enum LoginError { NO_INFO = "Unable to retrieve controller details", } +export enum ModelsError { + LOAD_ALL_MODELS = "Unable to load models.", + LOAD_SOME_MODELS = "Unable to load some models.", + LOAD_LATEST_MODELS = "Unable to load latest model data.", + LIST_OR_UPDATE_MODELS = "Unable to list or update models.", +} + const checkJIMMRelation = async ( conn: ConnectionWithFacades, identity: string, @@ -207,8 +214,36 @@ export const modelPollerMiddleware: Middleware< reduxStore.dispatch, reduxStore.getState, ); - } catch (e) { - console.log(e); + // If the code execution arrives here, then the model statuses + // have been successfully updated. Models error should be removed. + if (reduxStore.getState().juju.modelsError) { + reduxStore.dispatch( + jujuActions.updateModelsError({ + modelsError: null, + wsControllerURL, + }), + ); + } + } catch (error) { + let errorMessage; + if ( + error instanceof Error && + (error.message === ModelsError.LOAD_ALL_MODELS || + error.message === ModelsError.LOAD_SOME_MODELS) + ) { + errorMessage = pollCount + ? ModelsError.LOAD_LATEST_MODELS + : error.message; + } else { + errorMessage = ModelsError.LIST_OR_UPDATE_MODELS; + } + console.error(errorMessage, error); + reduxStore.dispatch( + jujuActions.updateModelsError({ + modelsError: errorMessage, + wsControllerURL, + }), + ); } } @@ -217,8 +252,8 @@ export const modelPollerMiddleware: Middleware< if (pollCount === action.payload.poll) { break; } - pollCount++; } + pollCount++; // Wait 30s then start again. await new Promise((resolve) => { setTimeout(() => { diff --git a/src/testing/factories/juju/juju.ts b/src/testing/factories/juju/juju.ts index 7e76d496c..74801a42a 100644 --- a/src/testing/factories/juju/juju.ts +++ b/src/testing/factories/juju/juju.ts @@ -183,6 +183,7 @@ export const jujuStateFactory = Factory.define(() => ({ controllers: null, models: {}, modelsLoaded: false, + modelsError: null, modelData: {}, modelWatcherData: {}, charms: [],