Fix/log unavailable warning (#3848)

* [UI] only allow troubleshoot link on error banner

* [UI] improve use of banners in run view sidepanel

* [UI] add info type banner

frontend/mock-backend/mock-api-middleware.ts

@@ -589,8 +589,12 @@ export default (app: express.Application) => {
 
   app.get('/k8s/pod/logs', (req, res) => {
     const podName = decodeURIComponent(req.query.podname);
+    if (podName === 'json-12abc') {
+      res.status(404).send('pod not found');
+      return;
+    }
     if (podName === 'coinflip-recursive-q7dqb-3721646052') {
-      res.status(404).send('Failed to retrieve log');
+      res.status(500).send('Failed to retrieve log');
       return;
     }
     const shortLog = fs.readFileSync('./mock-backend/shortlog.txt', 'utf-8');

frontend/server/handlers/pod-logs.ts

@@ -62,7 +62,7 @@ export function getPodLogsHandler(
 
   return async (req, res) => {
     if (!req.query.podname) {
-      res.status(404).send('podname argument is required');
+      res.status(400).send('podname argument is required');
       return;
     }
     const podName = decodeURIComponent(req.query.podname);
@@ -73,7 +73,16 @@ export function getPodLogsHandler(
 
     try {
       const stream = await getPodLogsStream(podName, podNamespace);
-      stream.on('error', err => res.status(500).send('Could not get main container logs: ' + err));
+      stream.on('error', err => {
+        if (
+          err?.message &&
+          err.message?.indexOf('Unable to find pod log archive information') > -1
+        ) {
+          res.status(404).send('pod not found');
+        } else {
+          res.status(500).send('Could not get main container logs: ' + err);
+        }
+      });
       stream.on('end', () => res.end());
       stream.pipe(res);
     } catch (err) {

frontend/src/Css.tsx

@@ -43,6 +43,8 @@ export const color = {
   themeDarker: '#0b59dc',
   warningBg: '#f9f9e1',
   warningText: '#ee8100',
+  infoBg: '#f3f4ff',
+  infoText: '#1a73e8',
   weak: '#9aa0a6',
 };
 

frontend/src/components/Banner.test.tsx

@@ -35,6 +35,11 @@ describe('Banner', () => {
     expect(tree).toMatchSnapshot();
   });
 
+  it('uses info mode when instructed', () => {
+    const tree = shallow(<Banner message={'Some message'} mode={'info'} />);
+    expect(tree).toMatchSnapshot();
+  });
+
   it('shows "Details" button and has dialog when there is additional info', () => {
     const tree = shallow(<Banner message={'Some message'} additionalInfo={'More info'} />);
     expect(tree).toMatchSnapshot();
@@ -52,8 +57,23 @@ describe('Banner', () => {
     expect(tree).toMatchSnapshot();
   });
 
+  it('does not show "Refresh" button if mode is "info"', () => {
+    const tree = shallow(
+      <Banner
+        message={'Some message'}
+        mode={'info'}
+        refresh={() => {
+          /* do nothing */
+        }}
+      />,
+    );
+    expect(tree.findWhere(el => el.text() === 'Refresh').exists()).toEqual(false);
+  });
+
   it('shows troubleshooting link instructed by prop', () => {
-    const tree = shallow(<Banner message='Some message' showTroubleshootingGuideLink={true} />);
+    const tree = shallow(
+      <Banner message='Some message' mode='error' showTroubleshootingGuideLink={true} />,
+    );
     expect(tree).toMatchInlineSnapshot(`
       <div
         className="flex banner mode"
@@ -80,6 +100,13 @@ describe('Banner', () => {
     `);
   });
 
+  it('does not show troubleshooting link if warning', () => {
+    const tree = shallow(
+      <Banner message='Some message' mode='warning' showTroubleshootingGuideLink={true} />,
+    );
+    expect(tree.findWhere(el => el.text() === 'Troubleshooting guide').exists()).toEqual(false);
+  });
+
   it('opens details dialog when button is clicked', () => {
     const tree = shallow(<Banner message='hello' additionalInfo='world' />);
     tree

frontend/src/components/Banner.tsx

@@ -22,11 +22,12 @@ import DialogContent from '@material-ui/core/DialogContent';
 import DialogTitle from '@material-ui/core/DialogTitle';
 import ErrorIcon from '@material-ui/icons/Error';
 import WarningIcon from '@material-ui/icons/Warning';
+import InfoIcon from '@material-ui/icons/Info';
 import { classes, stylesheet } from 'typestyle';
 
 import { color, commonCss, spacing } from '../Css';
 
-export type Mode = 'error' | 'warning';
+export type Mode = 'error' | 'warning' | 'info';
 
 export const css = stylesheet({
   banner: {
@@ -93,6 +94,8 @@ class Banner extends React.Component<BannerProps, BannerState> {
     });
     let bannerIcon = <ErrorIcon className={css.icon} />;
     let dialogTitle = 'An error occurred';
+    let showTroubleshootingGuideLink = false;
+    let showRefreshButton = true;
 
     switch (this.props.mode) {
       case 'error':
@@ -101,6 +104,7 @@ class Banner extends React.Component<BannerProps, BannerState> {
         });
         bannerIcon = <ErrorIcon className={css.icon} />;
         dialogTitle = 'An error occurred';
+        showTroubleshootingGuideLink = this.props.showTroubleshootingGuideLink || false;
         break;
       case 'warning':
         bannerModeCss = stylesheet({
@@ -109,6 +113,14 @@ class Banner extends React.Component<BannerProps, BannerState> {
         bannerIcon = <WarningIcon className={css.icon} />;
         dialogTitle = 'Warning';
         break;
+      case 'info':
+        bannerModeCss = stylesheet({
+          mode: { backgroundColor: color.infoBg, color: color.infoText },
+        });
+        bannerIcon = <InfoIcon className={css.icon} />;
+        dialogTitle = 'Info';
+        showRefreshButton = false;
+        break;
       default:
         // Already set defaults above.
         break;
@@ -121,7 +133,7 @@ class Banner extends React.Component<BannerProps, BannerState> {
           {this.props.message}
         </div>
         <div className={commonCss.flex}>
-          {this.props.showTroubleshootingGuideLink && (
+          {showTroubleshootingGuideLink && (
             <a
               className={css.troubleShootingLink}
               href='https://www.kubeflow.org/docs/pipelines/troubleshooting'
@@ -137,7 +149,7 @@ class Banner extends React.Component<BannerProps, BannerState> {
               Details
             </Button>
           )}
-          {this.props.refresh && (
+          {showRefreshButton && this.props.refresh && (
             <Button
               className={classes(css.button, css.refreshButton)}
               onClick={this._refresh.bind(this)}

frontend/src/components/PodYaml.test.tsx

@@ -84,7 +84,7 @@ describe('PodInfo', () => {
     const { getByText } = render(<PodInfo name='test-pod' namespace='test-ns' />);
     await act(TestUtils.flushPromises);
     getByText(
-      'Warning: failed to retrieve pod info. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration',
+      'Failed to retrieve pod info. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration',
     );
   });
 
@@ -162,7 +162,7 @@ describe('PodEvents', () => {
     const { getByText } = render(<PodEvents name='test-pod' namespace='test-ns' />);
     await act(TestUtils.flushPromises);
     getByText(
-      'Warning: failed to retrieve pod events. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration',
+      'Failed to retrieve pod events. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration',
     );
   });
 });

frontend/src/components/PodYaml.tsx

@@ -14,7 +14,7 @@ export const PodInfo: React.FC<{ name: string; namespace: string }> = ({ name, n
     <PodYaml
       name={name}
       namespace={namespace}
-      errorMessage='Warning: failed to retrieve pod info. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration'
+      errorMessage='Failed to retrieve pod info. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration'
       getYaml={getPodYaml}
     />
   );
@@ -32,7 +32,7 @@ export const PodEvents: React.FC<{
     <PodYaml
       name={name}
       namespace={namespace}
-      errorMessage='Warning: failed to retrieve pod events. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration'
+      errorMessage='Failed to retrieve pod events. Possible reasons include cluster autoscaling, pod preemption or pod cleaned up by time to live configuration'
       getYaml={getPodEventsYaml}
     />
   );

frontend/src/components/__snapshots__/Banner.test.tsx.snap

@@ -107,6 +107,24 @@ exports[`Banner uses error mode when instructed 1`] = `
 </div>
 `;
 
+exports[`Banner uses info mode when instructed 1`] = `
+<div
+  className="flex banner mode"
+>
+  <div
+    className="message"
+  >
+    <pure(InfoIcon)
+      className="icon"
+    />
+    Some message
+  </div>
+  <div
+    className="flex"
+  />
+</div>
+`;
+
 exports[`Banner uses warning mode when instructed 1`] = `
 <div
   className="flex banner mode"

frontend/src/lib/Apis.test.ts

@@ -60,8 +60,8 @@ describe('Apis', () => {
 
   it('getPodLogs', async () => {
     const spy = fetchSpy('http://some/address');
-    expect(await Apis.getPodLogs('some-pod-name')).toEqual('http://some/address');
-    expect(spy).toHaveBeenCalledWith('k8s/pod/logs?podname=some-pod-name', {
+    expect(await Apis.getPodLogs('some-pod-name', 'ns')).toEqual('http://some/address');
+    expect(spy).toHaveBeenCalledWith('k8s/pod/logs?podname=some-pod-name&podnamespace=ns', {
       credentials: 'same-origin',
     });
   });
@@ -87,7 +87,7 @@ describe('Apis', () => {
         text: () => 'bad response',
       }),
     );
-    expect(Apis.getPodLogs('some-pod-name')).rejects.toThrowError('bad response');
+    expect(Apis.getPodLogs('some-pod-name', 'ns')).rejects.toThrowError('bad response');
     expect(Apis.getPodLogs('some-pod-name', 'some-namespace-name')).rejects.toThrowError(
       'bad response',
     );

frontend/src/lib/Apis.ts

@@ -98,7 +98,7 @@ export class Apis {
   /**
    * Get pod logs
    */
-  public static getPodLogs(podName: string, podNamespace?: string): Promise<string> {
+  public static getPodLogs(podName: string, podNamespace: string): Promise<string> {
     let query = `k8s/pod/logs?podname=${encodeURIComponent(podName)}`;
     if (podNamespace) {
       query += `&podnamespace=${encodeURIComponent(podNamespace)}`;