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

Add experimental volumes and volumeMounts properties to jobs #966

Merged
merged 6 commits into from
Aug 8, 2019
Merged

Add experimental volumes and volumeMounts properties to jobs #966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
082c15b
Update node_modules
Aug 4, 2019
341f379
Add experimental volumeConfig property to jobs
Aug 4, 2019
a462b20
Update brigadier
Aug 5, 2019
be61038
Split volumeConfig into volumes and volumeMounts
Aug 5, 2019
3e4e9bc
Check volume refernced by volume mount exists
Aug 6, 2019
ef8e1a7
Update brigadier + volume and mounts doc
Aug 7, 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
4 changes: 2 additions & 2 deletions brigade-worker/node_modules/.yarn-integrity
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

222 changes: 201 additions & 21 deletions brigade-worker/node_modules/@brigadecore/brigadier/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 21 additions & 1 deletion brigade-worker/node_modules/@brigadecore/brigadier/out/job.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions brigade-worker/node_modules/@brigadecore/brigadier/out/job.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions brigade-worker/node_modules/@brigadecore/brigadier/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion brigade-worker/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"typescript": "^3.2.2"
},
"dependencies": {
"@brigadecore/brigadier": "^0.5.0",
"@brigadecore/brigadier": "^0.6.1",
"@kubernetes/client-node": "^0.10.1",
"byline": "^5.0.0",
"child-process-promise": "^2.2.1",
Expand Down
32 changes: 32 additions & 0 deletions brigade-worker/src/k8s.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,27 @@ export class JobRunner implements jobs.JobRunner {
}
}

// If the job defines volumes, add them to the pod's volume list.
// If the volume type is `hostPath`, first check if the project allows host mounts
// and throw an error if it it does not.
for (let v of job.volumes) {
if (v.hostPath != undefined && !project.allowHostMounts) {
throw new Error(`allowHostMounts is false in this project, not mounting ${v.hostPath.path}`);
}
this.runner.spec.volumes.push(v);
}

// If the job defines volume mounts, add them to every container's spec.
for (let m of job.volumeMounts) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since volumeMounts and volumes appear to be un-coupled, should we add the same check here to be sure project.allowHostMounts is true?

And/or, in general, should we somehow check for proper API usage? (If, say, configured with a volumeMount setup but no corresponding volume defined, etc. -- or is that allowed?) I suppose as long as we can show the user the k8s library error that should result on mis-configuration, we'd be good.

Copy link
Contributor Author

@radu-matei radu-matei Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Since you need to declare both volume and volume mount, I think checking is unnecessary - it would end up just taking the volume mount name and iterating through the volumes to check if it's mounting a host path - which already do for all volumes.

As for checking the existence of the volume you reference in the volume mount, that is a good idea, I'll add that.

These two checks should cover both of your questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a check for the referenced volume, together with a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed radumatei/brigade-worker:volume-mounts-check if you need to try it on a project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I love the extra validation with volumeExists because that provides us a faster feedback loop 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, thanks Radu!

if (!volumeExists(m, job.volumes)) {
throw new Error(`volume ${m.name} referenced in volume mount is not defined`);
}

for (let i = 0; i < this.runner.spec.containers.length; i++) {
this.runner.spec.containers[i].volumeMounts.push(m);
}
}

if (job.args.length > 0) {
this.runner.spec.containers[0].args = job.args;
}
Expand Down Expand Up @@ -1094,3 +1115,14 @@ export function secretToProject(
}
return p;
}

// helper function to check if the volume referenced by a volume mount is defined by the job
function volumeExists(volumeMount: kubernetes.V1VolumeMount, volumes: kubernetes.V1Volume[]): boolean {
for (let v of volumes) {
if (volumeMount.name === v.name) {
return true;
}
}

return false;
}
Loading