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.

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

Build-273 Volumes support to Build Strategies #1035

Merged
merged 1 commit into from
May 11, 2022

Conversation

alicerum
Copy link
Member

@alicerum alicerum commented Apr 5, 2022

Changes

This PR adds Volumes support to Build Strategies, allowing user to configure volumes and volume mounts used by the future tekton TaskRun object. Allowing eventually to add config maps, secrets and persistent storage to builds and build runs.
Volumes can be overriden by Build and BuildRun objects.

Tests have not been implemented yet, I am working hard to deliver them during the rest of today.

Duscussion topics

In the original SHIP only emptyDir volumes are defined in the BuildStrategy. In this PR any VolumeSource is supported there. In case we want to only implement emptyDirs in the Build Strategies and all the rest in Builds and BuildRins, we could possibly remove emptyDir definition from strategies and rename field as volumeTemplate or something. Clearly stating that this is not a VolumeSource as defined by k8s.

I moved overridable one level higher, to where optional is. Somehow it makes more sense, but I can move it back.

I did not implement the VolumeSource type, because it is not there in the original k8s type, and I figured there are other means of checking the type.

It is claimed in the SHIP that Secret and BuildConfig volume mounts must be read only and BuildRun must fail otherwise. I have never encountered this before, but I implemented it anyways.

Should I fail the BuildRun in case Secret or ConfigMap required by the build does not exist? I have not implemented this yet, and pod hangs indefinitely until resource appears.

Submitter Checklist

  • Includes tests if functionality changed/was added
  • Includes docs if changes are user-facing
  • Set a kind label on this PR
  • Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

action required: Build Strategies can now define `volumes`, which can be mounted in build steps, and overridden by `Build`s and `BuildRun`s. Build strategies which contain volume mounts in their buid steps must also declare the associated volumes in the strategy spec.

@openshift-ci openshift-ci bot added the release-note Label for when a PR has specified a release note label Apr 5, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2022
@alicerum alicerum changed the title Build 273 Volumes support to Build Strategies Build-273 Volumes support to Build Strategies Apr 5, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2022
@adambkaplan adambkaplan changed the title Build-273 Volumes support to Build Strategies [WIP] Build-273 Volumes support to Build Strategies Apr 6, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2022
@SaschaSchwarze0 SaschaSchwarze0 added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Apr 6, 2022
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Thanks for this start @alicerum. I think it uncovers some imprecisions in the ship that I point out in my comments.

pkg/apis/build/v1alpha1/build_types.go Show resolved Hide resolved
pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/taskrun.go Outdated Show resolved Hide resolved
pkg/volumes/volumes.go Outdated Show resolved Hide resolved
pkg/volumes/volumes.go Outdated Show resolved Hide resolved
test/buildstrategy_samples.go Outdated Show resolved Hide resolved
@SaschaSchwarze0 SaschaSchwarze0 added this to the release-v0.10.0 milestone Apr 6, 2022
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

I believe I need to update the original proposal and drop the optional and type fields. Some of the API assumed a more "restrictive" approach to persistent volumes, and yet the "Risks and Mitigations" section clearly implies that we'll allow any volume source, including hostPath. Passing through the core k8s VolumeSource object is the correct approach.

The proposal recommends that admins enable the PodSecurity admission plugin. This is default on k8s 1.23. This PR (or a follow-up) should make this recommendation clear to users/admins.

pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
pkg/volumes/volumes.go Outdated Show resolved Hide resolved
pkg/volumes/volumes.go Outdated Show resolved Hide resolved
pkg/volumes/volumes.go Outdated Show resolved Hide resolved
pkg/volumes/volumes_test.go Outdated Show resolved Hide resolved
pkg/volumes/volumes_test.go Outdated Show resolved Hide resolved
pkg/volumes/volumes_test.go Outdated Show resolved Hide resolved
pkg/volumes/volumes_test.go Outdated Show resolved Hide resolved
pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
pkg/apis/build/v1alpha1/buildstrategy.go Show resolved Hide resolved
@adambkaplan
Copy link
Member

Updated the SHIP here: shipwright-io/community#77

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2022
@alicerum alicerum force-pushed the build-273 branch 2 times, most recently from fd21f00 to 0ec7241 Compare April 21, 2022 10:36
@adambkaplan
Copy link
Member

Re-running the e2e test - it looked like all but one e2e test passed. The buildah test failed, looked like it hanged pulling the base image.

@adambkaplan
Copy link
Member

adambkaplan commented Apr 22, 2022

@alicerum success! I think the buildah issue was a test flake - if it continues to crop up we'll address in a separate PR.

Please update the Volume mounts section of the Build Strategies doc with the new API, then squash your commits.

Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Good progress. :-)

pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/volumes.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/volumes.go Show resolved Hide resolved
pkg/validate/volumes.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/taskrun.go Outdated Show resolved Hide resolved
@alicerum alicerum changed the title [WIP] Build-273 Volumes support to Build Strategies Build-273 Volumes support to Build Strategies Apr 26, 2022
@openshift-ci openshift-ci bot added release-note-action-required and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Label for when a PR has specified a release note labels Apr 26, 2022
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

Three more items, beside the two comments, please also extend the build.md and buildrun.md in the documentation.

pkg/apis/build/v1alpha1/buildstrategy.go Outdated Show resolved Hide resolved
pkg/reconciler/buildrun/resources/volumes.go Outdated Show resolved Hide resolved
docs/build.md Outdated Show resolved Hide resolved
@alicerum alicerum force-pushed the build-273 branch 3 times, most recently from 4546a78 to 5170f35 Compare May 4, 2022 14:43
This commit adds Volumes support to Build Strategies, Builds and
BuildRuns, allowing user to configure volumes and volume mounts
used by the future tekton TaskRun object.
Volumes can be overridden by Build and BuildRun objects.
Copy link
Member

@SaschaSchwarze0 SaschaSchwarze0 left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2022

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2022
@coreydaley
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit 4dbe852 into shipwright-io:main May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note-action-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants