jwa: Improve how JWA exposes errors #6952

elenzio9 · 2023-02-14T13:28:22Z

Currently, JWA doesn't properly expose the error state when errors occur in the underlying resources that the app is showing. Our goal it to further improve the way the app exposes errors to better inform users what's going on. So, in this PR:

Backend

Extend the process_status function to consider the following cases:
- Check the .status.containerState.
- Check the .status.conditions, since they have the one-liner reason and a message.
- If none of the above exist, it will use the Events emitted for the notebook.
- In case it deduces the status from an Event and it's not available anymore, it will use a generic message.
Add a 10 second delay to the backend logic, where we display a spinner and a generic message to prevent a warning icon from appearing just after a notebook is being initialized.
Extend the getNotebook request to also include the processed status information in the Notebook details page.

Frontend

Add an admonition in the details page of each Notebook with a detailed message on the current status.
Don't show the popup when a notebook is being stopped.

Click here to see some screenshots of how it looks like

Related issue: #6999

kimwnasptd · 2023-02-15T10:52:37Z

Thanks for this effort @elenzio9! I'll also create a more generalized issue for this effort, since we'll also need to set some future goals for how the Notebooks Controller exposes the status right now.

Regarding this PR, could you also create some Cypress tests for the different cases of the status we expect? This way we'll be sure this expectation will be tested in future PRs

elenzio9 · 2023-02-16T09:40:49Z

Regarding this PR, could you also create some Cypress tests for the different cases of the status we expect? This way we'll be sure this expectation will be tested in future PRs

After looking at the Cypress tests I found this one, which checks the status icon considering any type of status. So I think that covers us at least for now.

Once the frontend receives the raw YAML of each notebook (rather than the processed status sent by the backend), we can go ahead and create more tests for the different cases of the status we expect, as then the preprocessing part will happen in the frontend. However, this requires improvements to be made on the Notebook Controller side in order to always expose the status in the Notebook's .status.conditions.

@kimwnasptd Wdyt?

tasos-ale

Thanks for the PR @elenzio9. Please take a look at my comments.

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-info/status-info.component.scss

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-icon/status-icon.component.html

elenzio9 · 2023-03-10T13:26:01Z

Thanks for the PR @elenzio9. Please take a look at my comments.

@tasos-ale Thanks for you comments! I just force pushed!

...ontend/kubeflow-common-lib/projects/kubeflow/src/lib/resource-table/resource-table.module.ts

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-icon/status-icon.component.scss

tasos-ale · 2023-03-14T14:01:27Z

/lgtm
cc @kimwnasptd

* Have an admonition in the details page of each Notebook with a detailed message on the current status. Signed-off-by: Elena Zioga <elena@arrikto.com>

The process_status parses the status by: - Checking the .status.containerState. - Checking the .status.conditions, since they have the one-liner reason and a message. - If none of the above exist, it will use the Events emitted for the notebook. - In case it deduces the status from an Event and it's not available anymore, it uses a generic message. Also, add a 10 second delay to the backend logic where we display a spinner and a generic message to prevent a warning icon from appearing immediately after a notebook is initialized. Signed-off-by: Elena Zioga <elena@arrikto.com>

* Extend the getNotebook request to also include the processed status information in the Notebook details page. Signed-off-by: Elena Zioga <elena@arrikto.com>

Extend the frontend by: - Adding an admonition with a detailed message on the current status bellow the notebook name. - Adding the processed_status field. Signed-off-by: Elena Zioga <elena@arrikto.com>

Fix unit tests accordingly. Signed-off-by: Elena Zioga <elena@arrikto.com>

* Use the waiting status, which also uses the spinner, when a notebook is being stopped. Signed-off-by: Elena Zioga <elena@arrikto.com>

Signed-off-by: Elena Zioga <elena@arrikto.com>

elenzio9 · 2023-03-23T12:18:44Z

Did a first pass and added some comments as I took a look at the backend changes. Will add more in the next days

@kimwnasptd Thanks for the comments! Just pushed the changes in fixup commits.

kimwnasptd · 2023-03-23T15:22:45Z

/lgtm
/approve

google-oss-prow · 2023-03-23T15:22:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~components/crud-web-apps/OWNERS~~ [kimwnasptd]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* web-apps(front): Fix status case * Fix the status case to properly show the warning icon when the status phase is warning. Signed-off-by: Elena Zioga <elena@arrikto.com> * web-apps(front): Modify status-icon component * Modify the status-icon component to follow the status cases. Signed-off-by: Elena Zioga <elena@arrikto.com> * web-apps(front): Modify status component Signed-off-by: Elena Zioga <elena@arrikto.com> * web-apps(front): Introduce status-info component * Have an admonition in the details page of each Notebook with a detailed message on the current status. Signed-off-by: Elena Zioga <elena@arrikto.com> * jwa(back): Extend process_status function The process_status parses the status by: - Checking the .status.containerState. - Checking the .status.conditions, since they have the one-liner reason and a message. - If none of the above exist, it will use the Events emitted for the notebook. - In case it deduces the status from an Event and it's not available anymore, it uses a generic message. Also, add a 10 second delay to the backend logic where we display a spinner and a generic message to prevent a warning icon from appearing immediately after a notebook is initialized. Signed-off-by: Elena Zioga <elena@arrikto.com> * jwa(back): Extend getNotebook request * Extend the getNotebook request to also include the processed status information in the Notebook details page. Signed-off-by: Elena Zioga <elena@arrikto.com> * jwa(front): Extend the frontend Extend the frontend by: - Adding an admonition with a detailed message on the current status bellow the notebook name. - Adding the processed_status field. Signed-off-by: Elena Zioga <elena@arrikto.com> * jwa(front): Fix unit tests Fix unit tests accordingly. Signed-off-by: Elena Zioga <elena@arrikto.com> * jwa: Don't show the popup when a notebook is being stopped * Use the waiting status, which also uses the spinner, when a notebook is being stopped. Signed-off-by: Elena Zioga <elena@arrikto.com> * vwa(front): Update lib-status-icon Signed-off-by: Elena Zioga <elena@arrikto.com> * fixup! jwa(back): Extend getNotebook request * fixup! jwa(back): Extend process_status function --------- Signed-off-by: Elena Zioga <elena@arrikto.com>

google-oss-prow bot requested review from elikatsis, orfeas-k and tasos-ale February 14, 2023 13:28

google-oss-prow bot added the size/L label Feb 14, 2023

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch 2 times, most recently from 269f4aa to b8a20a2 Compare February 16, 2023 09:08

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch 6 times, most recently from a9861a0 to ad9b231 Compare February 17, 2023 11:19

elenzio9 changed the title ~~[jwa-exposing-errors] Improve how JWA exposes errors~~ jwa: Improve how JWA exposes errors Feb 24, 2023

kimwnasptd mentioned this pull request Feb 24, 2023

Controllers should always set a status #6999

Open

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from ad9b231 to ef3af1b Compare March 1, 2023 19:03

tasos-ale suggested changes Mar 10, 2023

View reviewed changes

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-info/status-info.component.scss Outdated Show resolved Hide resolved

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-icon/status-icon.component.html Outdated Show resolved Hide resolved

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from ef3af1b to bbe0233 Compare March 10, 2023 13:25

google-oss-prow bot added size/XL and removed size/L labels Mar 10, 2023

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from 07afc0a to 4d810cd Compare March 10, 2023 15:48

tasos-ale reviewed Mar 13, 2023

View reviewed changes

...ontend/kubeflow-common-lib/projects/kubeflow/src/lib/resource-table/resource-table.module.ts Outdated Show resolved Hide resolved

tasos-ale suggested changes Mar 13, 2023

View reviewed changes

...rontend/kubeflow-common-lib/projects/kubeflow/src/lib/status-icon/status-icon.component.scss Outdated Show resolved Hide resolved

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch 3 times, most recently from b84f73b to 3cce67c Compare March 14, 2023 11:09

google-oss-prow bot assigned tasos-ale Mar 14, 2023

elenzio9 added 8 commits March 23, 2023 14:06

web-apps(front): Introduce status-info component

7951726

* Have an admonition in the details page of each Notebook with a detailed message on the current status. Signed-off-by: Elena Zioga <elena@arrikto.com>

jwa(back): Extend getNotebook request

b89f627

* Extend the getNotebook request to also include the processed status information in the Notebook details page. Signed-off-by: Elena Zioga <elena@arrikto.com>

jwa(front): Extend the frontend

8cbe307

Extend the frontend by: - Adding an admonition with a detailed message on the current status bellow the notebook name. - Adding the processed_status field. Signed-off-by: Elena Zioga <elena@arrikto.com>

jwa(front): Fix unit tests

b0dca32

Fix unit tests accordingly. Signed-off-by: Elena Zioga <elena@arrikto.com>

jwa: Don't show the popup when a notebook is being stopped

8fa7521

* Use the waiting status, which also uses the spinner, when a notebook is being stopped. Signed-off-by: Elena Zioga <elena@arrikto.com>

vwa(front): Update lib-status-icon

60c057c

Signed-off-by: Elena Zioga <elena@arrikto.com>

fixup! jwa(back): Extend getNotebook request

f4e20f7

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from 3cce67c to ee15024 Compare March 23, 2023 12:07

google-oss-prow bot removed the lgtm label Mar 23, 2023

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from ee15024 to 991c6e4 Compare March 23, 2023 12:17

fixup! jwa(back): Extend process_status function

6bebb7f

elenzio9 force-pushed the feature-elena-jwa-exposing-errors branch from 991c6e4 to 6bebb7f Compare March 23, 2023 12:26

google-oss-prow bot assigned kimwnasptd Mar 23, 2023

google-oss-prow bot added the lgtm label Mar 23, 2023

google-oss-prow bot added the approved label Mar 23, 2023

google-oss-prow bot merged commit 3649e7e into kubeflow:master Mar 23, 2023

kimwnasptd deleted the feature-elena-jwa-exposing-errors branch March 23, 2023 15:27

elenzio9 mentioned this pull request Mar 24, 2023

mwa: Improve how MWA exposes errors kserve/models-web-app#69

Merged

thesuperzapper mentioned this pull request Jan 27, 2024

[TRACKING] Kubeflow 1.9.0 (Notebooks WG Components) #7453

Open

thesuperzapper modified the milestone: v1.9.0 May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jwa: Improve how JWA exposes errors #6952

jwa: Improve how JWA exposes errors #6952

elenzio9 commented Feb 14, 2023 •

edited

Loading

kimwnasptd commented Feb 15, 2023

elenzio9 commented Feb 16, 2023

tasos-ale left a comment

elenzio9 commented Mar 10, 2023

tasos-ale commented Mar 14, 2023

elenzio9 commented Mar 23, 2023

kimwnasptd commented Mar 23, 2023

google-oss-prow bot commented Mar 23, 2023

jwa: Improve how JWA exposes errors #6952

jwa: Improve how JWA exposes errors #6952

Conversation

elenzio9 commented Feb 14, 2023 • edited Loading

Backend

Frontend

kimwnasptd commented Feb 15, 2023

elenzio9 commented Feb 16, 2023

tasos-ale left a comment

Choose a reason for hiding this comment

elenzio9 commented Mar 10, 2023

tasos-ale commented Mar 14, 2023

elenzio9 commented Mar 23, 2023

kimwnasptd commented Mar 23, 2023

google-oss-prow bot commented Mar 23, 2023

elenzio9 commented Feb 14, 2023 •

edited

Loading