SHIP-0022: Build Strategy Volumes

[adambkaplan]

This proposal will allow build strategy authors to declare volumes which
can be shared across build steps and build runs. The build strategy will
declare the mount point for the volume in each build step.
Volumes can either have a fixed volume source, or an
overridable volume source that can be set in a Build or BuildRun.

Volumes declared in a build strategy are translated into the following:

1. Volume mounts for each task step that have a corresponding build step
2. A volume that is added to the TaskRun's pod template

This feature will replace the implict creation of emptyDir volumes in build
strategies. Build strategies will need to explicitly declare emptyDir
volumes moving forward.

[adambkaplan]

/assign @SaschaSchwarze0 @sbose78

[adambkaplan]

/cc @otaviof @HeavyWombat @imjasonh

[SaschaSchwarze0]

Nice ship. Adding my comments. Some as discussed in the community meeting.

[gabemontero]

Cool stuff @adambkaplan

just one comment on top of what @SaschaSchwarze0 has noted

[gabemontero]

WRT this, could we not create a first class CRD like BuildVolume to encapsulate what you currently have for each entry under your volumes array in the strategy spec. Then developers could query just on those.

Seems like labeling to indicate which volumes applied to which strategies could help narrow the query for developers.

The developer just needs to know the name of the strategy.

[adambkaplan]

This approach would make sense if a BuildVolume would be shared across build strategies, which I don't think is the case.

[gabemontero]

Hmm ... interesting, I was thinking the opposite. That the labels would be a way to associate a BuildVolume CRD with a particular build strategy.

i.e. you have 20 BuildVolumes. 2 are used by strategy A, 6 by strategy B. Sure, maybe you could use naming conventions to draw associations. But the value of a given label for a BuildVolume could be the strategy that uses it.

Separate point on the sharing across strategies. I can see the notion of not sharing build volumes across strategies being the more likely landing spot. But I'm not convinced that there will not be a non-trivial amount of image layer caching scenarios that span more than one strategy. For example, I could see more than one maven based build strategy existing, and both want to leverage a volume that houses some very common maven downloaded jar files.

[adambkaplan]

Practically speaking, I don't think we want to design an API that encourages lots of build strategies to be created. Users shouldn't need to create separate build strategies for, say, kaniko because one uses an image layer cache, and another doesn't.

I'm going to add a more concrete example in the design.

[gabemontero]

Hmm ... I suppose there is "encourages" vs. "accounting for the fact that our users will do it regardless of what we encourage".

All that said, the reality is we don't have tangible user feedback in this space both here with shipwright of course, as well as with openshift, given image layer caching as we both know is one of the remaining features we have not gotten to yet prior to calling that function feature complete. So end of the day, any arguments are speculative ones IMO at this point.

Your Example - Buildah with Image Cache is a useful addition to the SHIP/EP. I'll take that as a positive outcome of our thread here. Aside from having no immediate qualms / I'm good with it, in theory, we could "extend" at the k8s volume source level if need be. i.e.

volumeSource:
      type: PersistentVolumeClaim

if user feedback drove us down the path I'm speculating on. Certainly, our recent openshift entitlements forays have educated us on the pros (and cons) of providing CSI volumes and the like :-)

I wouldn't mind @adambkaplan you leaving this discussion thread open and not resolved so it is easier to find in the future if need be, and we revisit the reviews that lead to your SHIP merging, but otherwise, consider me OK with the outcome here.

thanks

[adambkaplan]

I'm adding a brief write-up of this in the "Alternatives" section as well.

[gabemontero]

the brief write is fine with me @adambkaplan thanks

[SaschaSchwarze0]

Looks good. One consistency-related question on implicit-vs-explicit volumeMounts.

Regarding overrides in Builds and BuildRuns. Not sure if you are aware that the following is possible:

kind: ClusterBuildStrategy
spec:
  params:
    - name: additional-secret
      type: string
      description: Secret to do something.
  buildSteps:
    - name: build
      volumeMounts:
        - name: additional-secret
          mountPath: /somepath/to/addition/secret
  volumes:
    - name: additional-secret
      secret:
        secretName: $(params.additional-secret)

That's another way to enable the build users to control the volume source to some degree (and is still possible with your approach if overridable is set to false). The restriction is that the build strategy author defines the volume type and this cannot be changed by the build user. This is both positive (security concern wise) as well as negative (because potentially limiting).

I think especially for your caching use case you want the type to be changable because the default would be an emptyDir and optionally the persistent cache would be hooked up. So, I am good with your approach.

[adambkaplan]

@SaschaSchwarze0 using parameters to allow configuration of volumes is an interesting possibility. I'm intrigued to see how this would be used in practice. We may have to wait and see what end users do with their creativity!

[adambkaplan]

@SaschaSchwarze0 @sbose78 @gabemontero @otaviof updated the proposal with your feedback and squashed commits. PTAL.

[SaschaSchwarze0]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [SaschaSchwarze0]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gabemontero]

/lgtm

/hold

in case @sbose78 or @otaviof want to comment further

if not, either they or @adambkaplan can cancel the hold

[gabemontero]

bump @sbose78 @otaviof ... any additional review cycles you all want to spend on @adambkaplan 's build strategy volumes EP here, or shall I remove the hold and let this merge?

I'll wait until my EOB Thursday and then I'll remove the hold regardless.

thanks

[otaviof]

bump @sbose78 @otaviof ... any additional review cycles you all want to spend on @adambkaplan 's build strategy volumes EP here, or shall I remove the hold and let this merge?

I'll wait until my EOB Thursday and then I'll remove the hold regardless.

thanks

I had a final pass on the document today and I think we have what we need. Please, lets go ahead on making it official 👍🏼

[gabemontero]

/hold cancel