Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend the DSL to implement the design of #801 #926

Merged
merged 12 commits into from
Apr 25, 2019
Merged

Extend the DSL to implement the design of #801 #926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0135935
SDK: Create BaseOp class
elikatsis Apr 24, 2019
127ce17
SDK: In preparation for the new feature ResourceOps (#801)
elikatsis Apr 24, 2019
7a8379f
SDK: Simplify the consumption of Volumes by ContainerOps
elikatsis Apr 24, 2019
b1474ac
SDK: Add ResourceOp
elikatsis Apr 24, 2019
67a4875
SDK: Simplify the creation of PersistentVolumeClaim instances
elikatsis Apr 24, 2019
b10e9bf
SDK: Emit a V1Volume as `.volume` from dsl.VolumeOp
elikatsis Apr 24, 2019
6ac42d4
SDK: Add PipelineVolume
elikatsis Apr 24, 2019
6918988
SDK: Simplify the creation of VolumeSnapshot instances
elikatsis Apr 24, 2019
627ecd6
Extend UI for the ResourceOp and Volumes feature of the Compiler
elikatsis Apr 24, 2019
3f81781
Cleaning up the diff (before moving things back)
Ark-kun Apr 24, 2019
4910e2f
Renamed op.deps back to op.dependent_names
Ark-kun Apr 24, 2019
0b9081f
Moved Container, Sidecar and BaseOp classed back to _container_op.py
Ark-kun Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 27 additions & 11 deletions frontend/src/components/StaticNodeDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2018 Google LLC
* Copyright 2018-2019 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more overall comment: Is it possible to implement something that would work even if a pipeline step is used multiple times? In this case, one execution should not override the data written by another.

For example, Ning is planning to add loop support: https://docs.google.com/document/d/12KHoEGe3o-i2WyzaU2JPXp3GQL3_BavGuh-KYBMUdQ8/edit?ts=5c5b2627#heading=h.bhvs46afvzxg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vicaire

One more overall comment: Is it possible to implement something that would work even if a pipeline step is used multiple times? In this case, one execution should not override the data written by another.

For example, Ning is planning to add loop support: https://docs.google.com/document/d/12KHoEGe3o-i2WyzaU2JPXp3GQL3_BavGuh-KYBMUdQ8/edit?ts=5c5b2627#heading=h.bhvs46afvzxg

I will amend the design doc in #801 so we use PipelineParam instances in the K8s resource specs referred to by PipelineVolume and PipelineVolumeSnapshot instances. This should make them usable in loops.

We don't have access to an implementation of loops yet, so we cannot verify our code, but we are confident this will work, based on our use of PipelineParam instances. Let's talk more about it in #801, if you feel there is something that can cause problems with your implementation.

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,7 +20,7 @@ import { classes, stylesheet } from 'typestyle';
import { commonCss, fontsize } from '../Css';
import { SelectedNodeInfo } from '../lib/StaticGraphParser';

export type nodeType = 'container' | 'dag' | 'unknown';
export type nodeType = 'container' | 'resource' | 'dag' | 'unknown';

const css = stylesheet({
fontSizeTitle: {
Expand All @@ -42,19 +42,35 @@ class StaticNodeDetails extends React.Component<StaticNodeDetailsProps> {
const nodeInfo = this.props.nodeInfo;

return <div>
<DetailsTable title='Input parameters' fields={nodeInfo.inputs} />
{(nodeInfo.nodeType === 'container') && (
<div>
<DetailsTable title='Input parameters' fields={nodeInfo.inputs} />

<DetailsTable title='Output parameters' fields={nodeInfo.outputs} />
<DetailsTable title='Output parameters' fields={nodeInfo.outputs} />

<div className={classes(commonCss.header, css.fontSizeTitle)}>Arguments</div>
{nodeInfo.args.map((arg, i) =>
<div key={i} style={{ fontFamily: 'monospace' }}>{arg}</div>)}
<div className={classes(commonCss.header, css.fontSizeTitle)}>Arguments</div>
{nodeInfo.args.map((arg, i) =>
<div key={i} style={{ fontFamily: 'monospace' }}>{arg}</div>)}

<div className={classes(commonCss.header, css.fontSizeTitle)}>Command</div>
{nodeInfo.command.map((c, i) => <div key={i} style={{ fontFamily: 'monospace' }}>{c}</div>)}
<div className={classes(commonCss.header, css.fontSizeTitle)}>Command</div>
{nodeInfo.command.map((c, i) => <div key={i} style={{ fontFamily: 'monospace' }}>{c}</div>)}

<div className={classes(commonCss.header, css.fontSizeTitle)}>Image</div>
<div style={{ fontFamily: 'monospace' }}>{nodeInfo.image}</div>
<div className={classes(commonCss.header, css.fontSizeTitle)}>Image</div>
<div style={{ fontFamily: 'monospace' }}>{nodeInfo.image}</div>

<DetailsTable title='Volume Mounts' fields={nodeInfo.volumeMounts} />
</div>
)}

{(nodeInfo.nodeType === 'resource') && (
<div>
<DetailsTable title='Input parameters' fields={nodeInfo.inputs} />

<DetailsTable title='Output parameters' fields={nodeInfo.outputs} />

<DetailsTable title='Manifest' fields={nodeInfo.resource} />
</div>
)}

{!!nodeInfo.condition && (
<div>
Expand Down
196 changes: 190 additions & 6 deletions frontend/src/lib/StaticGraphParser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2018 Google LLC
* Copyright 2018-2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -76,6 +76,52 @@ describe('StaticGraphParser', () => {
};
}

function newResourceCreatingWorkflow(): any {
return {
spec: {
entrypoint: 'template-1',
templates: [
{
dag: {
tasks: [
{ name: 'create-pvc-task', template: 'create-pvc' },
{
dependencies: ['create-pvc-task'],
name: 'container-1',
template: 'container-1',
},
{
dependencies: ['container-1'],
name: 'create-snapshot-task',
template: 'create-snapshot',
},
]
},
name: 'template-1',
},
{
name: 'create-pvc',
resource: {
action: 'create',
manifest: 'apiVersion: v1\nkind: PersistentVolumeClaim',
},
},
{
container: {},
name: 'container-1',
},
{
name: 'create-snapshot',
resource: {
action: 'create',
manifest: 'apiVersion: snapshot.storage.k8s.io/v1alpha1\nkind: VolumeSnapshot',
},
},
]
}
};
}

describe('createGraph', () => {
it('creates a single node with no edges for a workflow with one step.', () => {
const workflow = newWorkflow();
Expand Down Expand Up @@ -198,6 +244,18 @@ describe('StaticGraphParser', () => {
});
});

it('includes the resource\'s action and manifest itself in the info of resource nodes', () => {
const g = createGraph(newResourceCreatingWorkflow());
g.nodes().forEach((nodeName) => {
const node = g.node(nodeName);
if (nodeName.startsWith('create-pvc')) {
expect(node.info.resource).toEqual([['create', 'apiVersion: v1\nkind: PersistentVolumeClaim']]);
} else if (nodeName.startsWith('create-snapshot')) {
expect(node.info.resource).toEqual([['create', 'apiVersion: snapshot.storage.k8s.io\nkind: VolumeSnapshot']]);
}
});
});

it('renders extremely simple workflows with no steps or DAGs', () => {
const simpleWorkflow = {
spec: {
Expand Down Expand Up @@ -392,12 +450,13 @@ describe('StaticGraphParser', () => {
expect(nodeInfo).toEqual(defaultSelectedNodeInfo);
});

it('returns nodeInfo with empty values for args, command, and/or image if container does not have them', () => {
it('returns nodeInfo of a container with empty values for args, command, image and/or volumeMounts if container does not have them', () => {
const template = {
container: {
// No args
// No command
// No image
// No volumeMounts
},
dag: [],
name: 'template-1',
Expand All @@ -407,6 +466,7 @@ describe('StaticGraphParser', () => {
expect(nodeInfo.args).toEqual([]);
expect(nodeInfo.command).toEqual([]);
expect(nodeInfo.image).toEqual('');
expect(nodeInfo.volumeMounts).toEqual([]);
});


Expand Down Expand Up @@ -449,7 +509,48 @@ describe('StaticGraphParser', () => {
expect(nodeInfo.image).toEqual('some-image');
});

it('returns nodeInfo with empty values if template does not have inputs and/or outputs', () => {
it('returns nodeInfo containing container volumeMounts', () => {
const template = {
container: {
volumeMounts: [{'mountPath': '/some/path', 'name': 'some-vol'}]
},
dag: [],
name: 'template-1',
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('container');
expect(nodeInfo.volumeMounts).toEqual([['/some/path', 'some-vol']]);
});

it('returns nodeInfo of a resource with empty values for action and manifest', () => {
const template = {
dag: [],
name: 'template-1',
resource: {
// No action
// No manifest
},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.resource).toEqual([[]]);
});

it('returns nodeInfo containing resource action and manifest', () => {
const template = {
dag: [],
name: 'template-1',
resource: {
action: 'create',
manifest: 'manifest'
},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.resource).toEqual([['create', 'manifest']]);
});

it('returns nodeInfo of a container with empty values if template does not have inputs and/or outputs', () => {
const template = {
container: {},
dag: [],
Expand All @@ -463,7 +564,7 @@ describe('StaticGraphParser', () => {
expect(nodeInfo.outputs).toEqual([[]]);
});

it('returns nodeInfo containing template inputs params as list of name/value tuples', () => {
it('returns nodeInfo of a container containing template inputs params as list of name/value tuples', () => {
const template = {
container: {},
dag: [],
Expand All @@ -477,7 +578,7 @@ describe('StaticGraphParser', () => {
expect(nodeInfo.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]);
});

it('returns empty strings for inputs with no specified value', () => {
it('returns nodeInfo of a container with empty strings for inputs with no specified value', () => {
const template = {
container: {},
dag: [],
Expand Down Expand Up @@ -516,7 +617,7 @@ describe('StaticGraphParser', () => {
]);
});

it('returns empty strings for outputs with no specified value', () => {
it('returns nodeInfo of a container with empty strings for outputs with no specified value', () => {
const template = {
container: {},
name: 'template-1',
Expand All @@ -532,6 +633,89 @@ describe('StaticGraphParser', () => {
expect(nodeInfo.outputs).toEqual([['param1', ''], ['param2', '']]);
});

it('returns nodeInfo of a resource with empty values if template does not have inputs and/or outputs', () => {
const template = {
dag: [],
// No inputs
// No outputs
name: 'template-1',
resource: {},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.inputs).toEqual([[]]);
expect(nodeInfo.outputs).toEqual([[]]);
});

it('returns nodeInfo of a resource containing template inputs params as list of name/value tuples', () => {
const template = {
dag: [],
inputs: {
parameters: [{ name: 'param1', value: 'val1' }, { name: 'param2', value: 'val2' }]
},
name: 'template-1',
resource: {},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.inputs).toEqual([['param1', 'val1'], ['param2', 'val2']]);
});

it('returns nodeInfo of a resource with empty strings for inputs with no specified value', () => {
const template = {
dag: [],
inputs: {
parameters: [{ name: 'param1' }, { name: 'param2' }]
},
name: 'template-1',
resource: {},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.inputs).toEqual([['param1', ''], ['param2', '']]);
});

it('returns nodeInfo containing resource outputs as list of name/value tuples, pulling from valueFrom if necessary', () => {
const template = {
name: 'template-1',
outputs: {
parameters: [
{ name: 'param1', value: 'val1' },
{ name: 'param2', valueFrom: { jsonPath: 'jsonPath' } },
{ name: 'param3', valueFrom: { path: 'path' } },
{ name: 'param4', valueFrom: { parameter: 'parameterReference' } },
{ name: 'param5', valueFrom: { jqFilter: 'jqFilter' } },
],
},
resource: {},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.outputs).toEqual([
['param1', 'val1'],
['param2', 'jsonPath'],
['param3', 'path'],
['param4', 'parameterReference'],
['param5', 'jqFilter'],
]);
});

it('returns nodeInfo of a resource with empty strings for outputs with no specified value', () => {
const template = {
name: 'template-1',
outputs: {
parameters: [
{ name: 'param1' },
{ name: 'param2' },
],
},
resource: {},
} as any;
const nodeInfo = _populateInfoFromTemplate(new SelectedNodeInfo(), template);
expect(nodeInfo.nodeType).toEqual('resource');
expect(nodeInfo.outputs).toEqual([['param1', ''], ['param2', '']]);
});

});
});

Loading