[UI] improve use of banners in run view sidepanel

kubeflow · May 30, 2020 · 5bbe7b5 · 5bbe7b5
1 parent caf1aae
commit 5bbe7b5
Show file tree

Hide file tree

Showing 7 changed files with 259 additions and 116 deletions.
diff --git a/frontend/src/components/PodYaml.test.tsx b/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',
     );
   });
 });
diff --git a/frontend/src/components/PodYaml.tsx b/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}
     />
   );

diff --git a/frontend/src/lib/Apis.test.ts b/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',
     );

diff --git a/frontend/src/lib/Apis.ts b/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)}`;
@@ -389,6 +389,7 @@ export class Apis {
       Utils.logger.error(
         `Response for path: ${path} was not 'ok'\n\nResponse was: ${responseText}`,
       );
+      // TODO: create/use existing HTTP error class
       throw new Error(responseText);
     }
   }

diff --git a/frontend/src/pages/RunDetails.test.tsx b/frontend/src/pages/RunDetails.test.tsx
@@ -23,7 +23,7 @@ import { Workflow } from 'third_party/argo-ui/argo_template';
 import { ApiResourceType, ApiRunDetail, RunStorageState } from '../apis/run';
 import { QUERY_PARAMS, RoutePage, RouteParams } from '../components/Router';
 import { PlotType } from '../components/viewers/Viewer';
-import { Apis } from '../lib/Apis';
+import { Apis, JSONObject } from '../lib/Apis';
 import { ButtonKeys } from '../lib/Buttons';
 import { OutputArtifactLoader } from '../lib/OutputArtifactLoader';
 import { NodePhase } from '../lib/StatusUtils';
@@ -49,6 +49,7 @@ const STEP_TABS = {
 };
 
 const NODE_DETAILS_SELECTOR = '[data-testid="run-details-node-details"]';
+
 describe('RunDetails', () => {
   let updateBannerSpy: any;
   let updateDialogSpy: any;
@@ -59,6 +60,7 @@ describe('RunDetails', () => {
   let getExperimentSpy: any;
   let isCustomVisualizationsAllowedSpy: any;
   let getPodLogsSpy: any;
+  let getPodInfoSpy: any;
   let pathsParser: any;
   let pathsWithStepsParser: any;
   let loaderSpy: any;
@@ -118,6 +120,7 @@ describe('RunDetails', () => {
     getExperimentSpy = jest.spyOn(Apis.experimentServiceApi, 'getExperiment');
     isCustomVisualizationsAllowedSpy = jest.spyOn(Apis, 'areCustomVisualizationsAllowed');
     getPodLogsSpy = jest.spyOn(Apis, 'getPodLogs');
+    getPodInfoSpy = jest.spyOn(Apis, 'getPodInfo');
     pathsParser = jest.spyOn(WorkflowParser, 'loadNodeOutputPaths');
     pathsWithStepsParser = jest.spyOn(WorkflowParser, 'loadAllOutputPathsWithStepNames');
     loaderSpy = jest.spyOn(OutputArtifactLoader, 'load');
@@ -134,6 +137,7 @@ describe('RunDetails', () => {
     );
     isCustomVisualizationsAllowedSpy.mockImplementation(() => Promise.resolve(false));
     getPodLogsSpy.mockImplementation(() => 'test logs');
+    getPodInfoSpy.mockImplementation(() => ({ data: 'some data' } as JSONObject));
     pathsParser.mockImplementation(() => []);
     pathsWithStepsParser.mockImplementation(() => []);
     loaderSpy.mockImplementation(() => Promise.resolve([]));
@@ -640,7 +644,7 @@ describe('RunDetails', () => {
     expect(tree.find('Banner')).toMatchInlineSnapshot(`
       <Banner
         message="This step is in Succeeded state with this message: some test message"
-        mode="warning"
+        mode="info"
       />
     `);
   });
@@ -954,6 +958,7 @@ describe('RunDetails', () => {
     it('switches to logs tab in side pane', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
         status: { nodes: { node1: { id: 'node1' } } },
+        metadata: { namespace: 'ns' },
       });
       tree = shallow(<RunDetails {...generateProps()} />);
       await getRunSpy;
@@ -969,8 +974,17 @@ describe('RunDetails', () => {
 
     it('loads and shows logs in side pane', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: {
+          nodes: {
+            node1: {
+              id: 'node1',
+              phase: 'Running',
+            },
+          },
+        },
+        metadata: { namespace: 'ns' },
       });
+
       tree = shallow(<RunDetails {...generateProps()} />);
       await getRunSpy;
       await TestUtils.flushPromises();
@@ -981,13 +995,14 @@ describe('RunDetails', () => {
         .simulate('switch', STEP_TABS.LOGS);
       await getPodLogsSpy;
       expect(getPodLogsSpy).toHaveBeenCalledTimes(1);
-      expect(getPodLogsSpy).toHaveBeenLastCalledWith('node1', undefined);
+      expect(getPodLogsSpy).toHaveBeenLastCalledWith('node1', 'ns');
       expect(tree).toMatchSnapshot();
     });
 
     it('shows stackdriver link next to logs in GKE', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Succeeded' } } },
+        metadata: { namespace: 'ns' },
       });
       tree = shallow(
         <RunDetails
@@ -1046,7 +1061,7 @@ describe('RunDetails', () => {
     it("loads logs in run's namespace", async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
         metadata: { namespace: 'username' },
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Succeeded' } } },
       });
       tree = shallow(<RunDetails {...generateProps()} />);
       await getRunSpy;
@@ -1063,9 +1078,11 @@ describe('RunDetails', () => {
 
     it('shows warning banner and link to Stackdriver in logs area if fetching logs failed and cluster is in GKE', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Failed' } } },
+        metadata: { namespace: 'ns' },
       });
       TestUtils.makeErrorResponseOnce(getPodLogsSpy, 'getting logs failed');
+      TestUtils.makeErrorResponseOnce(getPodInfoSpy, 'error: pods "node1" not found');
       tree = shallow(
         <RunDetails
           {...generateProps()}
@@ -1080,6 +1097,7 @@ describe('RunDetails', () => {
         .at(1)
         .simulate('switch', STEP_TABS.LOGS);
       await getPodLogsSpy;
+      await getPodInfoSpy;
       await TestUtils.flushPromises();
       expect(tree.find(NODE_DETAILS_SELECTOR)).toMatchInlineSnapshot(`
         <div
@@ -1090,10 +1108,11 @@ describe('RunDetails', () => {
             className="page"
           >
             <Banner
-              additionalInfo="getting logs failed"
-              message="Warning: failed to retrieve pod logs. Possible reasons include cluster autoscaling or pod preemption"
-              mode="warning"
+              additionalInfo="Possible reasons include cluster autoscaling, pod garbage collection and pod preemption. Error response: getting logs failed"
+              message="Failed to retrieve pod logs. Use Stackdriver Kubernetes Monitoring to view them."
+              mode="info"
               refresh={[Function]}
+              showTroubleshootingGuideLink={false}
             />
             <div
               className=""
@@ -1117,9 +1136,11 @@ describe('RunDetails', () => {
 
     it('shows warning banner without stackdriver link in logs area if fetching logs failed and cluster is not in GKE', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Failed' } } },
+        metadata: { namespace: 'ns' },
       });
       TestUtils.makeErrorResponseOnce(getPodLogsSpy, 'getting logs failed');
+      TestUtils.makeErrorResponseOnce(getPodInfoSpy, 'error: pods "node1" not found');
       tree = shallow(<RunDetails {...generateProps()} gkeMetadata={{}} />);
       await getRunSpy;
       await TestUtils.flushPromises();
@@ -1129,6 +1150,7 @@ describe('RunDetails', () => {
         .at(1)
         .simulate('switch', STEP_TABS.LOGS);
       await getPodLogsSpy;
+      await getPodInfoSpy;
       await TestUtils.flushPromises();
       expect(tree.find('[data-testid="run-details-node-details"]')).toMatchInlineSnapshot(`
         <div
@@ -1139,10 +1161,11 @@ describe('RunDetails', () => {
             className="page"
           >
             <Banner
-              additionalInfo="getting logs failed"
-              message="Warning: failed to retrieve pod logs. Possible reasons include cluster autoscaling or pod preemption"
-              mode="warning"
+              additionalInfo="Possible reasons include cluster autoscaling, pod garbage collection and pod preemption. Error response: getting logs failed"
+              message="Failed to retrieve pod logs."
+              mode="info"
               refresh={[Function]}
+              showTroubleshootingGuideLink={false}
             />
           </div>
         </div>
@@ -1180,7 +1203,8 @@ describe('RunDetails', () => {
 
     it('keeps side pane open and on same tab when logs change after refresh', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Succeeded' } } },
+        metadata: { namespace: 'ns' },
       });
       tree = shallow(<RunDetails {...generateProps()} />);
       await getRunSpy;
@@ -1198,9 +1222,35 @@ describe('RunDetails', () => {
       expect(tree).toMatchSnapshot();
     });
 
+    it('shows error banner if fetching logs failed not because pod has gone away', async () => {
+      testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
+        status: { nodes: { node1: { id: 'node1', phase: 'Succeeded' } } },
+        metadata: { namespace: 'ns' },
+      });
+      TestUtils.makeErrorResponseOnce(getPodLogsSpy, 'getting logs failed');
+      TestUtils.makeErrorResponseOnce(getPodInfoSpy, 'some error');
+      tree = shallow(<RunDetails {...generateProps()} />);
+      await getRunSpy;
+      await TestUtils.flushPromises();
+      clickGraphNode(tree, 'node1');
+      tree
+        .find('MD2Tabs')
+        .at(1)
+        .simulate('switch', STEP_TABS.LOGS);
+      await getPodLogsSpy;
+      await getPodInfoSpy;
+      await TestUtils.flushPromises();
+      expect(tree.state()).toMatchObject({
+        logsBannerAdditionalInfo: 'Error response: getting logs failed',
+        logsBannerMessage: 'Failed to retrieve pod logs.',
+        logsBannerMode: 'error',
+      });
+    });
+
     it('dismisses log failure warning banner when logs can be fetched after refresh', async () => {
       testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
-        status: { nodes: { node1: { id: 'node1' } } },
+        status: { nodes: { node1: { id: 'node1', phase: 'Failed' } } },
+        metadata: { namespace: 'ns' },
       });
       TestUtils.makeErrorResponseOnce(getPodLogsSpy, 'getting logs failed');
       tree = shallow(<RunDetails {...generateProps()} />);
@@ -1212,12 +1262,12 @@ describe('RunDetails', () => {
         .at(1)
         .simulate('switch', STEP_TABS.LOGS);
       await getPodLogsSpy;
+      await getPodInfoSpy;
       await TestUtils.flushPromises();
       expect(tree.state()).toMatchObject({
-        logsBannerAdditionalInfo: 'getting logs failed',
-        logsBannerMessage:
-          'Warning: failed to retrieve pod logs. Possible reasons include cluster autoscaling or pod preemption',
-        logsBannerMode: 'warning',
+        logsBannerAdditionalInfo: 'Error response: getting logs failed',
+        logsBannerMessage: 'Failed to retrieve pod logs.',
+        logsBannerMode: 'error',
       });
 
       testRun.run!.status = 'Failed';
@@ -1229,6 +1279,64 @@ describe('RunDetails', () => {
     });
   });
 
+  describe('pod tab', () => {
+    it('shows pod info', async () => {
+      testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
+        status: { nodes: { node1: { id: 'node1', phase: 'Failed' } } },
+        metadata: { namespace: 'ns' },
+      });
+      tree = shallow(<RunDetails {...generateProps()} />);
+      await getRunSpy;
+      await TestUtils.flushPromises();
+      clickGraphNode(tree, 'node1');
+      tree
+        .find('MD2Tabs')
+        .at(1)
+        .simulate('switch', STEP_TABS.POD);
+      await getPodInfoSpy;
+      await TestUtils.flushPromises();
+
+      expect(tree.find(NODE_DETAILS_SELECTOR)).toMatchInlineSnapshot(`
+        <div
+          className="page"
+          data-testid="run-details-node-details"
+        >
+          <div
+            className="page"
+          >
+            <PodInfo
+              name="node1"
+              namespace="ns"
+            />
+          </div>
+        </div>
+      `);
+    });
+
+    it('does not show pod pane if selected node skipped', async () => {
+      testRun.pipeline_runtime!.workflow_manifest = JSON.stringify({
+        status: { nodes: { node1: { id: 'node1', phase: 'Skipped' } } },
+        metadata: { namespace: 'ns' },
+      });
+      tree = shallow(<RunDetails {...generateProps()} />);
+      await getRunSpy;
+      await TestUtils.flushPromises();
+      clickGraphNode(tree, 'node1');
+      tree
+        .find('MD2Tabs')
+        .at(1)
+        .simulate('switch', STEP_TABS.POD);
+      await TestUtils.flushPromises();
+
+      expect(tree.find(NODE_DETAILS_SELECTOR)).toMatchInlineSnapshot(`
+        <div
+          className="page"
+          data-testid="run-details-node-details"
+        />
+      `);
+    });
+  });
+
   describe('auto refresh', () => {
     beforeEach(() => {
       testRun.run!.status = NodePhase.PENDING;