BUILD-275: add support for build csi volume source

[jkhelil]

https://issues.redhat.com/browse/BUILD-275

[adambkaplan]

@jkhelil generally on the right path here. You can get the build to pass by adding a commit which adds a replace to your go.mod file, referencing your code in openshift/api#1056

(note that your code is currently missing the deep copy functions, and will break as a result.)

[jkhelil]

flaking openshift-e2e-aws-builds-techpreview
/test openshift-e2e-aws-builds-techpreview

[jkhelil]

flaking e2e-gcp-builds
/test e2e-gcp-builds

[gabemontero]

You should put BUILD-275: in the beginning of your PR title. I know you reference it in the PR description, but having it in the title will help us when we have to search PRs in the future.

I do have a question for @adambkaplan in looking at the BUILD-275 acceptance criteria.

It mentions making sure that TechPreviewNoUpgrade feature gate is enabled.

However, the csiVolumesEnabled parameter to setupBuildVolumes is currently on based on only the BuildCSIVolumes feature gate.

See

openshift-controller-manager/pkg/cmd/controller/build.go

Lines 54 to 63 in ebe9867

	fg := ctx.OpenshiftControllerConfig.FeatureGates
	csiVolumesEnabled := false
	if fg != nil {
	for _, v := range fg {
	v = strings.TrimSpace(v)
	if v == "BuildCSIVolumes=true" {
	csiVolumesEnabled = true
	}
	}
	}

@adambkaplan - should that code be changed to check both feature gates? I don't think that BuildCSIVolumes is automatically set to true if tech preview is on, but maybe I am "misremembering" .... and I don't recall exactly where you wired all that feature gate stuff up.

Otherwise @jkhelil you changes here look OK to me wrt wiring in CSI volumes. Though I think getting a review from @coreydaley since he did the introduction of build volumes in general would be good, in case I am missing something.

[jkhelil]

/assign @adambkaplan
/assign @coreydaley

[gabemontero]

You should put [BUILD-275](https://issues.redhat.com/browse/BUILD-275): in the beginning of your PR title. I know you reference it in the PR description, but having it in the title will help us when we have to search PRs in the future.

I do have a question for @adambkaplan in looking at the BUILD-275 acceptance criteria.

It mentions making sure that TechPreviewNoUpgrade feature gate is enabled.

However, the csiVolumesEnabled parameter to setupBuildVolumes is currently on based on only the BuildCSIVolumes feature gate.

See

openshift-controller-manager/pkg/cmd/controller/build.go

Lines 54 to 63 in ebe9867

	fg := ctx.OpenshiftControllerConfig.FeatureGates
	csiVolumesEnabled := false
	if fg != nil {
	for _, v := range fg {
	v = strings.TrimSpace(v)
	if v == "BuildCSIVolumes=true" {
	csiVolumesEnabled = true
	}
	}
	}

@adambkaplan - should that code be changed to check both feature gates? I don't think that BuildCSIVolumes is automatically set to true if tech preview is on, but maybe I am "misremembering" .... and I don't recall exactly where you wired all that feature gate stuff up.

@adambkaplan assisted this old man's memory yesterday (including reminding me of the fact that I actually did some of this stuff, albeit more or less following his instructions ;-) ), and confirmed that only checking for BuildCSIVolumes is sufficient for our means. It s a feature set vs. feature gate distinction, where we can't have BuildCSIVolumes on unless tech preview is on.

So all good on this point.

[adambkaplan]

/approve

Controller changes look good. Once the code for openshift/api is merged then we can use a proper commit to get the updated Build API object.

[adambkaplan]

/hold

Need openshift/api#1056 to merge, as well as an update to openshift/apiserver.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, jkhelil

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [adambkaplan]

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

[jkhelil]

/retest

[jkhelil]

/test verify

[openshift-ci]

@jkhelil: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws	87906e8	link	true	/test e2e-aws
ci/prow/openshift-e2e-aws-builds-techpreview	87906e8	link	false	/test openshift-e2e-aws-builds-techpreview
ci/prow/e2e-aws-builds	87906e8	link	true	/test e2e-aws-builds
ci/prow/e2e-gcp	87906e8	link	true	/test e2e-gcp
ci/prow/e2e-aws-upgrade	87906e8	link	true	/test e2e-aws-upgrade
ci/prow/unit	87906e8	link	true	/test unit
ci/prow/e2e-gcp-builds	87906e8	link	true	/test e2e-gcp-builds
ci/prow/images	87906e8	link	true	/test images
ci/prow/verify	87906e8	link	true	/test verify
ci/prow/e2e-aws-proxy	87906e8	link	false	/test e2e-aws-proxy

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.