Fix/log unavailable warning #3848

jonasdebeukelaer · 2020-05-26T11:53:36Z

only allow troubleshoot link on error banner
edit msg slightly when logs have gone away from cluster

kubeflow-bot · 2020-05-26T11:53:42Z

This change is

k8s-ci-robot · 2020-05-26T11:53:46Z

Hi @jonasdebeukelaer. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jonasdebeukelaer · 2020-05-26T12:08:53Z

feels like this failing build has nothing to do with ui? 🤔

broken build shows

============================== 71 passed in 1.21s ==============================
___________________________________ summary ____________________________________
  py35: commands succeeded
  congratulations :)
The command "./run_test.sh" exited with 0.
0.00s$ cd $TRAVIS_BUILD_DIR/test/sample-test/unittests
The command "cd $TRAVIS_BUILD_DIR/test/sample-test/unittests" exited with 0.
0.21s$ python3 -m unittest utils_tests.py
.
----------------------------------------------------------------------
Ran 1 test in 0.006s
OK
The command "python3 -m unittest utils_tests.py" exited with 0.

Bobgy · 2020-05-27T00:52:30Z

@jonasdebeukelaer Thanks a lot for your contribution! This is absolutely awesome for UX.
/lgtm
/approve
/ok-to-test

k8s-ci-robot · 2020-05-27T00:52:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

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:

~~frontend/OWNERS~~ [Bobgy]

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

Bobgy · 2020-05-27T00:52:48Z

Yes, the travis unit tests are flaky. I'll help you retry them.

Bobgy · 2020-05-27T01:06:15Z

/retest

Bobgy · 2020-05-27T01:12:30Z

@jonasdebeukelaer The error message for frontend unit test: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubeflow_pipelines/3848/kubeflow-pipeline-frontend-test/1265449238280540160#1:build-log.txt%3A60

frontend/src/pages/RunDetails.test.tsx

Ark-kun · 2020-05-27T20:43:41Z

/test kubeflow-pipeline-frontend-test

jonasdebeukelaer · 2020-05-30T17:13:32Z

changes:

added info type banner
use it if it's a case of pod not found by doing a get pod if errors, and seeing if it's a 404
updated other banners in the side panel to use info type banner where appropriate
added some missing unit tests

node: I've made namespace required for Apis.getPodLogs, not sure why it was optional before?

jonasdebeukelaer · 2020-05-31T15:42:13Z

/retest

Bobgy

Awesome! I've left some minor comments and nitpickings.

frontend/src/components/Banner.test.tsx

frontend/src/lib/Apis.ts

Bobgy · 2020-06-03T04:05:58Z

frontend/src/pages/RunDetails.tsx

+      logsBannerMessage = 'Failed to retrieve pod logs.';
+
+      // if pod can't be found, then assume it has gone from the server
+      if (await this.podNotFound(selectedNodeDetails.id, namespace)) {


nit: just confirm the error message returned from Apis.getPodLogs has no hint whether the pod is not found?
If no, this seems a reasonable workaround.

it does allude to workflow not being found, or something along those lines, but the error code is 500, so would have to do a msg contains text type check. Feels a bit brittle but happy to do that instead

I'm okay with text check in this PR, if you feel you have capacity, we can also detect and change the error code in

pipelines/frontend/server/handlers/pod-logs.ts

Line 76 in 57f415d

stream.on('error', err => res.status(500).send('Could not get main container logs: ' + err));

ohh I thought this was interfacing straight with argo 🤦 . This makes sense then yeah , will do here

so I've updated this but don't have time to stand up the whole backend to test it. is there some kind of short cut way to do it?

Do you have a running KFP cluster?
If yes, you can follow https://github.com/kubeflow/pipelines/blob/master/frontend/README.md#proxy-to-a-real-cluster, you will need to develop "Client UI + Node server". It will send api requests to the cluster for anything else.

unfortunately, we haven't added integration tests for pod logs api method.

pipelines/frontend/server/app.test.ts

Lines 1041 to 1042 in d6920ca

// TODO: Add integration tests for k8s helper related endpoints

// describe('/k8s/pod/logs', () => {});

You can also use integration test as a way for testing it too, I think there are plenty of example test cases next to it. Feel free to ask me if you need any help.

frontend/src/pages/RunDetails.tsx

Bobgy · 2020-06-03T04:11:36Z

frontend/src/pages/RunDetails.tsx

+            sidepanelBannerMode = 'error';
+            break;
+          case NodePhase.FAILED:
+            sidepanelBannerMode = 'warning';


Sorry I don't have much context here, can you help me understand the difference between ERROR and FAILED?

I'll be honest I'm not 100% of this either, but I would understand this to be

ERROR if there was a error trying to set up the container or something, or network error, or issue in argo, and

FAILED means the execution of the container failed

Thanks for the explanation, I'm not sure I understand why you want to show warning for failed node.
For users, it seems to me they should also perceive it as an error.

It's to try to separate pipelines failure from system failure, but happy to change to error type too

Understood. IMO, can we add some wording in the error message to indicate difference between fail and error.
e.g. error -> Pipelines System Error: original message; while fail -> Failed execution: ...

what do you think?

This might be a little controversial, we can also revert this part keeping current behavior, so that we can get this PR merged. Opening a new PR for this specific issue.

Well I'll make em all Error type, and we can add a separate issue to add those prefixes?

Sounds good! Thank you!

frontend/src/pages/RunDetails.tsx

jonasdebeukelaer · 2020-06-08T09:08:52Z

Awesome! I've left some minor comments and nitpickings.

@Bobgy oops totally missed your update! Should have some time to fix this evening 👍

jonasdebeukelaer · 2020-06-10T08:03:40Z

/retest

jonasdebeukelaer · 2020-06-11T09:02:26Z

/retest

Bobgy

@jonasdebeukelaer Great work!
Left some comments

frontend/server/handlers/pod-logs.ts

frontend/src/pages/RunDetails.tsx

Bobgy · 2020-06-12T01:09:20Z

frontend/src/pages/RunDetails.tsx

+            sidepanelBannerMode = 'error';
+            break;
+          case NodePhase.FAILED:
+            sidepanelBannerMode = 'warning';


Thanks for the explanation, I'm not sure I understand why you want to show warning for failed node.
For users, it seems to me they should also perceive it as an error.

frontend/src/pages/RunDetails.test.tsx

frontend/src/components/Banner.test.tsx

Bobgy · 2020-06-15T01:48:39Z

frontend/src/pages/RunDetails.tsx

+            sidepanelBannerMode = 'error';
+            break;
+          case NodePhase.FAILED:
+            sidepanelBannerMode = 'warning';


Understood. IMO, can we add some wording in the error message to indicate difference between fail and error.
e.g. error -> Pipelines System Error: original message; while fail -> Failed execution: ...

what do you think?

This might be a little controversial, we can also revert this part keeping current behavior, so that we can get this PR merged. Opening a new PR for this specific issue.

frontend/src/pages/RunDetails.test.tsx

Bobgy · 2020-06-15T01:55:54Z

frontend/src/components/Banner.test.tsx

+        }}
+      />,
+    );
+    expect(tree.findWhere(el => el.text() === 'Refresh')).toEqual({});


nit: I don't quite understand, why toEqual({})?
Let me take a look.

Verified, toEqual({}) will succeed no matter nodes found or not, because the wrapper helper object has no direct properties all the time.

Please use this way to verify not found:

pipelines/frontend/src/components/viewers/Tensorboard.test.tsx

Lines 323 to 330 in fcd2559

expect(

tree

.findWhere(

el =>

el.text() === `Tensorboard is starting, and you may need to wait for a few minutes.`,

)

.exists(),

).toEqual(false);

oh thanks for that, sorry I'm quite new to ts tests

No worries, you are doing great

Bobgy

Awesome work, thanks a lot for your continued efforts!

I'm okay with no additional test for pod logs for now, because the change is small and easily reviewable.
Would you mind updating the last minor issue?

/lgtm

Bobgy · 2020-06-17T02:42:03Z

frontend/server/handlers/pod-logs.ts

+          err?.message &&
+          err.message?.indexOf('Unable to find pod log archive information') > -1
+        ) {
+          res.status(404).send('pod not found');


nit: also append err to make sure we can debug if there are false-positives of 404s?

ohhh, I forgot I approved before.
Feel free to update it in a following up PR

* [UI] only allow troubleshoot link on error banner * [UI] improve use of banners in run view sidepanel * [UI] add info type banner

k8s-ci-robot requested review from Bobgy and neuromage May 26, 2020 11:53

k8s-ci-robot added the size/M label May 26, 2020

k8s-ci-robot added the needs-ok-to-test label May 26, 2020

jonasdebeukelaer force-pushed the fix/log-unavailable-warning branch from 8f75ccc to 88c8729 Compare May 26, 2020 12:15

k8s-ci-robot assigned Bobgy May 27, 2020

k8s-ci-robot added the ok-to-test label May 27, 2020

k8s-ci-robot added lgtm approved and removed needs-ok-to-test labels May 27, 2020

jiezhang reviewed May 27, 2020

View reviewed changes

frontend/src/pages/RunDetails.test.tsx Outdated Show resolved Hide resolved

jonasdebeukelaer force-pushed the fix/log-unavailable-warning branch from 88c8729 to 5bbe7b5 Compare May 30, 2020 17:10

k8s-ci-robot added size/L and removed lgtm size/M labels May 30, 2020

Bobgy reviewed Jun 3, 2020

View reviewed changes

jonasdebeukelaer force-pushed the fix/log-unavailable-warning branch from 5bbe7b5 to 47a372e Compare June 9, 2020 19:15

Bobgy reviewed Jun 12, 2020

View reviewed changes

jonasdebeukelaer force-pushed the fix/log-unavailable-warning branch from 47a372e to c3270dd Compare June 12, 2020 17:08

Bobgy reviewed Jun 15, 2020

View reviewed changes

jonas added 3 commits June 16, 2020 13:54

[UI] only allow troubleshoot link on error banner

a5406c4

[UI] add info type banner

a4ae4e0

[UI] improve use of banners in run view sidepanel

f9627d8

jonasdebeukelaer force-pushed the fix/log-unavailable-warning branch from c3270dd to f9627d8 Compare June 16, 2020 12:54

Bobgy reviewed Jun 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm label Jun 17, 2020

k8s-ci-robot merged commit 040615c into kubeflow:master Jun 17, 2020

RedbackThomson pushed a commit to RedbackThomson/pipelines that referenced this pull request Jun 17, 2020

Fix/log unavailable warning (kubeflow#3848)

e6ce0bd

* [UI] only allow troubleshoot link on error banner * [UI] improve use of banners in run view sidepanel * [UI] add info type banner

Bobgy added the cherrypick-approved area OWNER approves to cherry pick this PR to current active release branch label Jun 19, 2020

Bobgy added the cherrypicked cherry picked to release branch `release-x.y` label Jul 2, 2020

Bobgy pushed a commit that referenced this pull request Jul 2, 2020

Fix/log unavailable warning (#3848)

3c6f6db

* [UI] only allow troubleshoot link on error banner * [UI] improve use of banners in run view sidepanel * [UI] add info type banner

Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020

Fix/log unavailable warning (kubeflow#3848)

956b2f1

* [UI] only allow troubleshoot link on error banner * [UI] improve use of banners in run view sidepanel * [UI] add info type banner

ra312 mentioned this pull request Apr 1, 2022

Warning message is confusing when pod logs cannot be retrieved #3711

Closed

	// TODO: Add integration tests for k8s helper related endpoints
	// describe('/k8s/pod/logs', () => {});

	expect(
	tree
	.findWhere(
	el =>
	el.text() === `Tensorboard is starting, and you may need to wait for a few minutes.`,
	)
	.exists(),
	).toEqual(false);

Fix/log unavailable warning #3848

Fix/log unavailable warning #3848

Conversation

jonasdebeukelaer commented May 26, 2020 • edited by Bobgy Loading

kubeflow-bot commented May 26, 2020

k8s-ci-robot commented May 26, 2020

jonasdebeukelaer commented May 26, 2020 • edited Loading

Bobgy commented May 27, 2020

k8s-ci-robot commented May 27, 2020

Bobgy commented May 27, 2020

Bobgy commented May 27, 2020

Bobgy commented May 27, 2020

Ark-kun commented May 27, 2020

jonasdebeukelaer commented May 30, 2020

jonasdebeukelaer commented May 31, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdebeukelaer commented Jun 8, 2020

jonasdebeukelaer commented Jun 10, 2020

jonasdebeukelaer commented Jun 11, 2020

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bobgy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasdebeukelaer commented May 26, 2020 •

edited by Bobgy

Loading

jonasdebeukelaer commented May 26, 2020 •

edited

Loading

Bobgy Jun 12, 2020 •

edited

Loading