Add experimental volumes and volumeMounts properties to jobs #966

radu-matei · 2019-08-04T13:00:30Z

This PR enables experimental support for adding volumes and volume mounts to jobs.

Note that for the simple use case of sharing a volume across all jobs, storage.enabled should still be used.

A pre-built image for the worker: radumatei/brigade-worker:volume-mounts-check.

cc @krancour - this is a very Kubernetes-specific addition - how would this impact non-Kubernetes targets?

Use cases: #955 (comment) and Mounting volumes for ETL jobs

Example:

    const kind = new Job("kind", "radumatei/golang-kind:1.11-0.4")
    kind.privileged = true;
    kind.volumes = [
        {
            name: "modules",
            hostPath: {
                path: "/lib/modules",
                type: "Directory"
            }
        },
        {
            name: "cgroup",
            hostPath: {
                path: "/sys/fs/cgroup",
                type: "Directory"
            }
        },
        {
            name: "docker-graph-storage",
            emptyDir: {}
        }
    ];

    kind.volumeMounts = [
        {
            name: "modules",
            mountPath: "/lib/modules",
            readOnly: true
        },
        {
            name: "cgroup",
            mountPath: "/sys/fs/cgroup"
        },
        {
            name: "docker-graph-storage",
            mountPath: "/var/lib/docker"
        }
    ];

Signed-off-by: Radu M <[email protected]>

radu-matei · 2019-08-04T13:02:05Z

brigade-worker/src/k8s.ts

+      if (cfg.volume.hostPath != undefined && !project.allowHostMounts) {
+        throw new Error(`allowHostMounts is false in this project, not mounting path ${cfg.volume.hostPath.path}`)
+      }
+      this.runner.spec.volumes.push(cfg.volume);


Is there any scenario where you would need to mount the same volume twice in a single job pod?

The only scenario I can think of is mounting to different paths, although it doesn't seem to be that common - see kubernetes/kubernetes#34453.

I have never got to do it myself. But I see some use with subpath, as mentioned in the issue.

Just curious, but does whether to support mounting the same volume multiple times affect our design/implementation? 🤔

Ah I got it! You've paired a volume and its corresponding mount in each volumeConfig.

I'd rather suggest exposing volumes and volumeMounts without any abstraction first, so that we can provide a standard library feature like this volumeConfig on top of that, or users can build their own.

IMHO for the standard library, something that solves 80% of the use-cases will be enough, that justifies dropping support for mounting the same volume multiple times.

Reading radu-matei/brigadier@94f56c2 - If you agree, implementation-wise, I think adding volumeMounts and volumes that are allowed/used only when volumeConfig doesn't exist would suffice.

That totally makes sense - I initially thought maybe we could simplify and boil things down to a single property, but yeah, you're right, we should keep in line with the Kubernetes abstraction.

Updating.

Actually I like the idea of pairing volumes and mounts.

I even think we can happily merge the volume and the mount as in many cases each volume is mounted only once:

job.volumeConfig = [ { name: "modules", mountPath: "/lib/modules", readOnly: true, hostPath: { path: "/lib/modules", type: "Directory" } }, { name: "cgroup", mountPath: "/sys/fs/cgroup", hostPath: { path: "/sys/fs/cgroup", type: "Directory" } }, { name: "docker-graph-storage", mountPath: "/var/lib/docker", emptyDir: {} } ]

And I believe names can be usually omitted because they can be generated from mount-path(which are guaranteed to not collide:

job.volumeConfig = [ { // name: "lib-modules", mountPath: "/lib/modules", readOnly: true, hostPath: { path: "/lib/modules", type: "Directory" } }, { // name: "sys-fs-cgroup", mountPath: "/sys/fs/cgroup", hostPath: { path: "/sys/fs/cgroup", type: "Directory" } }, { // name: "var-lib-docker", mountPath: "/var/lib/docker", emptyDir: {} } ]

I actually don't think we should keep both volume + volumeMounts and volumeConfig, as it just invites confusion - what do you think?

So I'd vote for starting with only volumes + volumeMounts if I need to choose one.

// But I do believe something like volumeConfig has a lot of potential, and worth it, even if it needs an extensive documentation to not confuse users.

Yeah, I initially thought that something like volumeConfig could be an abstraction over how volumes and volume mounts work in Kubernetes and simplify setting them up for a job.

But I agree, let's start with plain volumes and volumeMounts, then decide whether to add a higher level abstraction, how you suggested.

Thanks a lot, this has been really helpful!

Brigadier change: brigadecore/brigadier@8c53a1c

Updated - could you please give this another look?
Thanks!

netlify · 2019-08-04T13:03:04Z

Deploy preview for brigade-docs ready!

Built with commit ef8e1a7

https://deploy-preview-966--brigade-docs.netlify.com

Signed-off-by: Radu M <[email protected]>

radu-matei · 2019-08-05T11:08:03Z

brigade-worker/package.json

@@ -38,7 +38,7 @@
    "typescript": "^3.2.2"
  },
  "dependencies": {
-    "@brigadecore/brigadier": "^0.5.0",
+    "@brigadecore/brigadier": "radu-matei/brigadier#job-volume-config-out",


After brigadecore/brigadier#22 is merged and a new version is published, update this.

brigade-worker/src/k8s.ts

vdice

Had one question as well as the request to add an example around a basic configuration somewhere in the docs. Otherwise, looks great!

vdice · 2019-08-05T21:04:05Z

brigade-worker/src/k8s.ts

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


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.

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.

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

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

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

Perfect, thanks Radu!

Signed-off-by: Radu M <[email protected]>

mumoshu

LGTM!
I believe we can merge this once brigadecore/brigadier#22 gets merged/published.

radu-matei · 2019-08-07T16:48:54Z

Merged brigadecore/brigadier#22, rebasing, updating, and adding a doc.

krancour · 2019-08-07T21:14:55Z

cc @krancour - this is a very Kubernetes-specific addition - how would this impact non-Kubernetes targets?

Do we imagine being able to one day orchestrate job containers using something other than Kubernetes? It wasn't on my radar, but even if the answer were yes, I think this could be accommodated with other orchestrators.

Signed-off-by: Radu M <[email protected]>

radu-matei · 2019-08-07T21:42:17Z

Rebased and added a simple doc.

@krancour - not really, or at least not the JS worker anyway. Just trying to make sure we're not overlooking something that might be affected by this - thanks!

radu-matei · 2019-08-08T14:06:00Z

Thanks a lot for the help, everyone!

Radu M added 2 commits August 4, 2019 15:26

Update node_modules

082c15b

Signed-off-by: Radu M <[email protected]>

Add experimental volumeConfig property to jobs

341f379

Signed-off-by: Radu M <[email protected]>

radu-matei commented Aug 4, 2019

View reviewed changes

radu-matei mentioned this pull request Aug 4, 2019

ci(brigade.js): add e2e job #955

Merged

Radu M added 2 commits August 5, 2019 14:04

Update brigadier

a462b20

Signed-off-by: Radu M <[email protected]>

Split volumeConfig into volumes and volumeMounts

be61038

Signed-off-by: Radu M <[email protected]>

radu-matei commented Aug 5, 2019

View reviewed changes

brigade-worker/src/k8s.ts Outdated Show resolved Hide resolved

radu-matei changed the title ~~Add experimental volumeConfig property to jobs~~ Add experimental volumes and volumeMounts properties to jobs Aug 5, 2019

radu-matei marked this pull request as ready for review August 5, 2019 13:02

radu-matei requested review from adamreese, lukepatrick, technosophos and vdice as code owners August 5, 2019 13:02

radu-matei added this to the 1.2 milestone Aug 5, 2019

vdice reviewed Aug 5, 2019

View reviewed changes

Check volume refernced by volume mount exists

3e4e9bc

Signed-off-by: Radu M <[email protected]>

mumoshu approved these changes Aug 6, 2019

View reviewed changes

vdice approved these changes Aug 6, 2019

View reviewed changes

Update brigadier + volume and mounts doc

ef8e1a7

Signed-off-by: Radu M <[email protected]>

radu-matei merged commit 8525067 into brigadecore:master Aug 8, 2019

radu-matei mentioned this pull request Aug 19, 2019

Proposal: long-running worker for high frequency events #977

Closed

radu-matei mentioned this pull request Oct 30, 2019

Attach existing PVC for all jobs in build #792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental volumes and volumeMounts properties to jobs #966

Add experimental volumes and volumeMounts properties to jobs #966

radu-matei commented Aug 4, 2019 •

edited

Loading

radu-matei Aug 4, 2019 •

edited

Loading

mumoshu Aug 5, 2019

mumoshu Aug 5, 2019

mumoshu Aug 5, 2019

radu-matei Aug 5, 2019 •

edited

Loading

mumoshu Aug 5, 2019 •

edited

Loading

mumoshu Aug 5, 2019

radu-matei Aug 5, 2019

radu-matei Aug 5, 2019

radu-matei Aug 5, 2019

netlify bot commented Aug 4, 2019 •

edited

Loading

radu-matei Aug 5, 2019

vdice left a comment

vdice Aug 5, 2019

radu-matei Aug 6, 2019 •

edited

Loading

radu-matei Aug 6, 2019

radu-matei Aug 6, 2019

mumoshu Aug 6, 2019

vdice Aug 6, 2019

mumoshu left a comment

radu-matei commented Aug 7, 2019

krancour commented Aug 7, 2019

radu-matei commented Aug 7, 2019

radu-matei commented Aug 8, 2019

Add experimental volumes and volumeMounts properties to jobs #966

Add experimental volumes and volumeMounts properties to jobs #966

Conversation

radu-matei commented Aug 4, 2019 • edited Loading

radu-matei Aug 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radu-matei Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

mumoshu Aug 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Aug 4, 2019 • edited Loading

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radu-matei Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumoshu left a comment

Choose a reason for hiding this comment

radu-matei commented Aug 7, 2019

krancour commented Aug 7, 2019

radu-matei commented Aug 7, 2019

radu-matei commented Aug 8, 2019

radu-matei commented Aug 4, 2019 •

edited

Loading

radu-matei Aug 4, 2019 •

edited

Loading

radu-matei Aug 5, 2019 •

edited

Loading

mumoshu Aug 5, 2019 •

edited

Loading

netlify bot commented Aug 4, 2019 •

edited

Loading

radu-matei Aug 6, 2019 •

edited

Loading