Skip to content

Commit

Permalink
[UI] Show cached steps (#3602)
Browse files Browse the repository at this point in the history
* [UI] Show cached steps

* Tests for parseNodePhase

* Complete unit tests

* Update StatusUtils.test.tsx
  • Loading branch information
Bobgy authored Apr 24, 2020
1 parent 0b17641 commit 69efa62
Show file tree
Hide file tree
Showing 5 changed files with 510 additions and 4 deletions.
65 changes: 63 additions & 2 deletions frontend/src/lib/StatusUtils.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@ import {
statusBgColors,
statusToBgColor,
checkIfTerminated,
parseNodePhase,
} from './StatusUtils';
import { NodeStatus, S3Artifact, Artifact } from 'third_party/argo-ui/argo_template';

describe('StatusUtils', () => {
describe('hasFinished', () => {
[
NodePhase.ERROR,
NodePhase.FAILED,
NodePhase.SUCCEEDED,
NodePhase.CACHED,
NodePhase.SKIPPED,
NodePhase.TERMINATED,
].forEach(status => {
Expand Down Expand Up @@ -94,8 +97,10 @@ describe('StatusUtils', () => {
});
});

it("returns color 'succeeded' if status is 'Succeeded'", () => {
expect(statusToBgColor(NodePhase.SUCCEEDED)).toEqual(statusBgColors.succeeded);
[NodePhase.SUCCEEDED, NodePhase.CACHED].forEach(status => {
it(`returns color 'succeeded' if status is '${status}'`, () => {
expect(statusToBgColor(status)).toEqual(statusBgColors.succeeded);
});
});
});

Expand Down Expand Up @@ -130,4 +135,60 @@ describe('StatusUtils', () => {
expect(checkIfTerminated(NodePhase.FAILED, 'some random error')).toEqual(NodePhase.FAILED);
});
});

describe('parseNodePhase', () => {
const DEFAULT_NODE_STATUS = ({
phase: 'Succeeded',
id: 'file-passing-pipelines-55slt-2894085459',
outputs: {
artifacts: [
({
s3: {
key:
'artifacts/file-passing-pipelines-55slt/file-passing-pipelines-55slt-2894085459/sum-numbers-output.tgz',
},
} as unknown) as Artifact,
],
},
} as unknown) as NodeStatus;

it('returns node original phase if not successful', () => {
expect(
parseNodePhase({
...DEFAULT_NODE_STATUS,
phase: 'Failed',
}),
).toEqual('Failed');
});

it('returns succeeded phase for a normal node', () => {
expect(
parseNodePhase({
...DEFAULT_NODE_STATUS,
phase: 'Succeeded',
}),
).toEqual('Succeeded');
});

it('returns cached phase for a cached node', () => {
expect(
parseNodePhase({
...DEFAULT_NODE_STATUS,
id: 'file-passing-pipelines-55slt-2894085459',
phase: 'Succeeded', // Cached nodes have phase == 'Succeeded'
outputs: {
artifacts: [
{
s3: {
// HACK: A cached node's artifacts will refer to a path that doesn't match its own id.
key:
'artifacts/file-passing-pipelines-mjpph/file-passing-pipelines-mjpph-1802581193/sum-numbers-output.tgz',
},
} as Artifact,
],
},
}),
).toEqual('Cached');
});
});
});
25 changes: 25 additions & 0 deletions frontend/src/lib/StatusUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,14 @@
*/

import { logger } from '../lib/Utils';
import { NodeStatus } from '../../third_party/argo-ui/argo_template';

export const statusBgColors = {
error: '#fce8e6',
notStarted: '#f7f7f7',
running: '#e8f0fe',
succeeded: '#e6f4ea',
cached: '#e6f4ea',
terminatedOrSkipped: '#f1f3f4',
warning: '#fef7f0',
};
Expand All @@ -32,6 +34,7 @@ export enum NodePhase {
RUNNING = 'Running',
SKIPPED = 'Skipped',
SUCCEEDED = 'Succeeded',
CACHED = 'Cached',
TERMINATING = 'Terminating',
TERMINATED = 'Terminated',
UNKNOWN = 'Unknown',
Expand All @@ -40,6 +43,7 @@ export enum NodePhase {
export function hasFinished(status?: NodePhase): boolean {
switch (status) {
case NodePhase.SUCCEEDED: // Fall through
case NodePhase.CACHED: // Fall through
case NodePhase.FAILED: // Fall through
case NodePhase.ERROR: // Fall through
case NodePhase.SKIPPED: // Fall through
Expand Down Expand Up @@ -70,6 +74,8 @@ export function statusToBgColor(status?: NodePhase, nodeMessage?: string): strin
return statusBgColors.running;
case NodePhase.SUCCEEDED:
return statusBgColors.succeeded;
case NodePhase.CACHED:
return statusBgColors.cached;
case NodePhase.SKIPPED:
// fall through
case NodePhase.TERMINATED:
Expand All @@ -90,3 +96,22 @@ export function checkIfTerminated(status?: NodePhase, nodeMessage?: string): Nod
}
return status;
}

export function parseNodePhase(node: NodeStatus): NodePhase {
if (node.phase !== 'Succeeded') {
return node.phase as NodePhase; // HACK: NodePhase is a string enum that has the same items as node.phase.
}
return wasNodeCached(node) ? NodePhase.CACHED : NodePhase.SUCCEEDED;
}

function wasNodeCached(node: NodeStatus): boolean {
const artifacts = node.outputs?.artifacts;
if (!artifacts || !node.id) {
return false;
}
// HACK: There is a way to detect the skipped pods based on the WorkflowStatus alone.
// All output artifacts have the pod name (same as node ID) in the URI. But for skipped
// pods, the pod name does not match the URIs.
// (And now there are always some output artifacts since we've enabled log archiving).
return artifacts.some(artifact => artifact.s3 && !artifact.s3.key.includes(node.id));
}
4 changes: 2 additions & 2 deletions frontend/src/lib/WorkflowParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { color } from '../Css';
import { statusToIcon } from '../pages/Status';
import { Constants } from './Constants';
import { KeyValue } from './StaticGraphParser';
import { hasFinished, NodePhase, statusToBgColor } from './StatusUtils';
import { hasFinished, NodePhase, statusToBgColor, parseNodePhase } from './StatusUtils';
import { parseTaskDisplayName } from './ParserUtils';

export enum StorageService {
Expand Down Expand Up @@ -102,7 +102,7 @@ export default class WorkflowParser {

g.setNode(node.id, {
height: Constants.NODE_HEIGHT,
icon: statusToIcon(node.phase as NodePhase, node.startedAt, node.finishedAt, node.message),
icon: statusToIcon(parseNodePhase(node), node.startedAt, node.finishedAt, node.message),
label: nodeLabel,
statusColoring: statusToBgColor(node.phase as NodePhase, node.message),
width: Constants.NODE_WIDTH,
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/pages/Status.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import PendingIcon from '@material-ui/icons/Schedule';
import RunningIcon from '../icons/statusRunning';
import SkippedIcon from '@material-ui/icons/SkipNext';
import SuccessIcon from '@material-ui/icons/CheckCircle';
import CachedIcon from '@material-ui/icons/Cached';
import TerminatedIcon from '../icons/statusTerminated';
import Tooltip from '@material-ui/core/Tooltip';
import UnknownIcon from '@material-ui/icons/Help';
Expand Down Expand Up @@ -73,6 +74,11 @@ export function statusToIcon(
iconColor = color.success;
title = 'Executed successfully';
break;
case NodePhase.CACHED: // This is not argo native, only applies to node.
IconComponent = CachedIcon;
iconColor = color.success;
title = 'Execution was skipped and outputs were taken from cache';
break;
case NodePhase.TERMINATED:
IconComponent = TerminatedIcon;
iconColor = color.terminated;
Expand Down
Loading

0 comments on commit 69efa62

Please sign in to comment.