BUILD-281: add feature gate, OCM configuration bit, to enable build csi volume tech preview

[gabemontero]

/assign @adambkaplan
/assign @bparees
(given historical involvement in this effort, please feel free to unassign at your discretion)
/assign @deads2k
(given recent reviews in this space as well as historical involvement in the shared/projected resource space; of course reassign at your discretion)

[gabemontero]

hold .... THIS story is also suppose to do the feature gate a la #982

[gabemontero]

unhold ... feature gate commit included

PTAL

[bparees]

analogous fields in both config/v1 and openshiftcontrolplane/v1

@gabemontero does that mean you confirmed what i said earlier (that we need it in both places because one is the user facing config api, and the other is the internal config struct for the controller process?)

[gabemontero]

analogous fields in both config/v1 and openshiftcontrolplane/v1

@gabemontero does that mean you confirmed what i said earlier (that we need it in both places because one is the user facing config api, and the other is the internal config struct for the controller process?)

not confirmed @bparees .... placeholder / anticipating the answer. Still waiting on @adambkaplan to catch up to this, as he was the one who cited updating openshiftcontrollplane

[gabemontero]

analogous fields in both config/v1 and openshiftcontrolplane/v1

@gabemontero does that mean you confirmed what i said earlier (that we need it in both places because one is the user facing config api, and the other is the internal config struct for the controller process?)

not confirmed @bparees .... placeholder / anticipating the answer. Still waiting on @adambkaplan to catch up to this, as he was the one who cited updating openshiftcontrollplane

actually @bparees @adambkaplan .... if we were to take the discussion thread with @deads2k at https://coreos.slack.com/archives/C014MHHKUSF/p1631216153043900 even further, and mimic the use of library-go for features gates like the kubeapiserver, then we would duplicate for OCM the existing openshift api server https://github.com/openshift/api/blob/master/openshiftcontrolplane/v1/types.go#L48, which is analogous to https://github.com/openshift/api/blob/master/kubecontrolplane/v1/types.go#L64, which is what is currently used by https://github.com/openshift/cluster-kube-apiserver-operator/blob/master/pkg/operator/configobservation/configobservercontroller/observe_config_controller.go#L134

the only fly in the ointment there is those TODO comments above the lines in those first 2 links

[gabemontero]

OK @bparees @adambkaplan @deads2k

this revamped version attempts to capture/implement guidance from various slack conversations with each of you in some fashion, along with some resolution of items previously discussed here

Key points:

• leverage library-go's feature gate observe function
• only need to update the OCM config; hard to reconcile the feature gate observe functionality in library-go with the observe build functionality in OCM-0; plus really only need 1 on/off switch, which goes away once use of csi volumes for builds goes GA
• a bit of back and forth with @deads2k on the differing config approaches between apiserver(s) and OCM, but if I read him correctly, in the end, and with the awareness that feature gates are eventually going away, and with the awareness that our coupling with the cluster storage operator for csi shared resources, and CSO's extensive use of the tech preview feature gate, I've added a "feature gate centric" field to the OCM config

Also, I have WIP PRs for illustrating the end to end flow between library-go, OCM-O, and OCM, and the minimal amount of code delta required, to achieve are means.

Please consider
openshift/library-go#1201
openshift/cluster-openshift-controller-manager-operator#227
openshift/openshift-controller-manager#199

which are all based on the current commit level of this PR at the time of this comment

thanks

[adambkaplan]

First, necessary background:

• https://github.com/openshift/api/blob/master/config/v1/types_build.go#L19-L45 is the build controller configuration that is stored in etcd and configurable by the admin.
• This configuration (and others) gets rendered into the spec.observedConfig field of the operator OpenshiftControllerManager object: https://github.com/openshift/api/blob/master/operator/v1/types_openshiftcontrollermanager.go#L15-L24.
• The schema for OpenshiftControllerManager's observed config is the old 3.x controller manager configuration, which can be expressed as JSON/YAML: https://github.com/openshift/api/blob/master/openshiftcontrolplane/v1/types.go#L164.
• Ultimately openshift-controller-manager-operator merges the default, observed, and unsupported overrides into the OpenshiftControllerManager JSON, and mounts this as a ConfigMap: https://github.com/openshift/cluster-openshift-controller-manager-operator/blob/master/pkg/operator/sync_openshiftcontrollermanager_v311_00.go#L183-L186

So the net is that the API in /config is stored in etcd, the API in /openshiftcontrolplane is not.

In this approach, the library-go function used will parse out the feature gates and render an array of strings with the format <FeatureGate>=<true|false>. The FeatureGates field therefore should just be []string if we move forward with this approach. That said, we would then need extra logic in openshift-controller-manager to parse the feature gates, which to me feels like a bit of overkill.

When I scoped the story out, my thought is we could do the following:

• Add a field to BuildControllerConfig indicating if CSI volumes are enabled: https://github.com/openshift/api/blob/master/openshiftcontrolplane/v1/types.go#L198
• Update our existing build config observation function to read the cluster feature gates, check if BuildCSIVolumes is enabled, and if so, set the appropriate value. See https://github.com/openshift/cluster-openshift-controller-manager-operator/blob/master/pkg/operator/configobservation/builds/observe_builds.go#L18

This would avoid the "kube feature gates" -> "controller manager config" translation, which IMO feels brittle. The downside is that we would more or less need to replicate the logic in the cluster-storage-operator - see https://github.com/openshift/cluster-storage-operator/blob/2e76e02cb8479784a582e57a587be8fd5d75cae2/pkg/operator/csidriveroperator/driverstarter.go#L268-L292

[adambkaplan]

I would leave the bits about future deprecation and such out.

Suggested change

	// featureGates captures the current set of OpenShift feature gates that are of interest to
	// components of the OpenShift Controller Manager. NOTE: this field will possibly be deprecated
	// in a future OpenShift release if the feature gate mechanism in OpenShift is removed.
	FeatureGates map[string][]string `json:"featureGates"`
	// featureGates are the set of extra OpenShift feature gates for openshift-controller-manager.
	// These feature gates can be used to enable features that are tech preview or otherwise not available on
	// OpenShift by default.
	FeatureGates map[string][]string `json:"featureGates"`

[adambkaplan]

A few other things

• The library-go function referenced in BUILD-281: add library-go watch/update of new OCM feature gate field cluster-openshift-controller-manager-operator#227 produces a []string output and sets that on whatever nested JSON object you have. I think this field needs to be []string and not map[string][]string.
• The data is of the format <FeatureGate>=<true|false>. So something on ocm needs to parse the feature gates. Are we OK with that?

[gabemontero]

My testing of things on Friday as I recall came to the conclusion that anything set to false did not get included in what ultimately bubbled up. i.e. it got filtered at the library-go level.

I'll double check if we move forward down this path. But if I'm incorrect, then yes, ocm-o/ocm would need to account for it.

In the interest of code reuse and expediency, I at least am OK with it.

[gabemontero]

I'll pull your suggestion in manually @adambkaplan since I'll need to run make update anyway to get the verifications to pass ... save on some CI churn

thanks

[gabemontero]

godoc update pushed @adambkaplan

[gabemontero]

Thanks for catching up to this after your kubecon foray @adambkaplan ... some opnion/responses embedded below.

First, necessary background:

* https://github.com/openshift/api/blob/master/config/v1/types_build.go#L19-L45 is the build controller configuration that is stored in etcd and configurable by the admin.

* This configuration (and others) gets rendered into the `spec.observedConfig` field of the operator OpenshiftControllerManager object: https://github.com/openshift/api/blob/master/operator/v1/types_openshiftcontrollermanager.go#L15-L24.

* The schema for OpenshiftControllerManager's observed config is the old 3.x controller manager configuration, which can be expressed as JSON/YAML: https://github.com/openshift/api/blob/master/openshiftcontrolplane/v1/types.go#L164.

* Ultimately openshift-controller-manager-operator merges the default, observed, and unsupported overrides into the OpenshiftControllerManager JSON, and mounts this as a ConfigMap: https://github.com/openshift/cluster-openshift-controller-manager-operator/blob/master/pkg/operator/sync_openshiftcontrollermanager_v311_00.go#L183-L186

So the net is that the API in /config is stored in etcd, the API in /openshiftcontrolplane is not.

In this approach, the library-go function used will parse out the feature gates and render an array of strings with the format <FeatureGate>=<true|false>. The FeatureGates field therefore should just be []string if we move forward with this approach. That said, we would then need extra logic in openshift-controller-manager to parse the feature gates, which to me feels like a bit of overkill.

When I scoped the story out, my thought is we could do the following:

* Add a field to BuildControllerConfig indicating if CSI volumes are enabled: https://github.com/openshift/api/blob/master/openshiftcontrolplane/v1/types.go#L198

* Update our existing build config observation function to read the cluster feature gates, check if `BuildCSIVolumes` is enabled, and if so, set the appropriate value. See https://github.com/openshift/cluster-openshift-controller-manager-operator/blob/master/pkg/operator/configobservation/builds/observe_builds.go#L18

OK @adambkaplan yes this gets to the crux of the questions @bparees and I had at the beginning of this PR. The intent you outlined above ^^ now does not precisely with what came out of planning for BUILD-281 and what I currently see in the description there. But no big deal wrt that specifically. I think the crux of the decisions we need to make here is what you accurately summarized below.

This would avoid the "kube feature gates" -> "controller manager config" translation, which IMO feels brittle. The downside is that we would more or less need to replicate the logic in the cluster-storage-operator - see https://github.com/openshift/cluster-storage-operator/blob/2e76e02cb8479784a582e57a587be8fd5d75cae2/pkg/operator/csidriveroperator/driverstarter.go#L268-L292

Totally agree with the characterization of things.

My current opinions are:

• tweaking library-go for this would need to take into account working on top of the existing support provided for the flag driven api server consumption.
• feature gates are going away per @deads2k .... so I think any time spent in that space should be minimized
• feature gating build support for csi volumes is a temporary thing, independent of the fate of feature gates ... ultimately, we want this to be support that just exists like any other. Developers can choose to not use it. Administrators can prevent use of CSI volumes for builds at a per namespace level by simply not granting the builder SA permissions to access the shares

Bottom line, I think we can manage any "brittle" translation in the short term while dealing with a tech preview item, and just jettison the whole thing when we graduate to GA.

[adambkaplan]

• tweaking library-go for this would need to take into account working on top of the existing support provided for the flag driven api server consumption.

I think this PR will help: openshift/library-go#1208

[adambkaplan]

Bottom line, I think we can manage any "brittle" translation in the short term while dealing with a tech preview item, and just jettison the whole thing when we graduate to GA.

This is fair. I'm now sold on using FeatureGates in the OCM configuration spec. However I think the type needs to be changed to []string for us to use it properly.

[gabemontero]

type now []string per the cluster bot testing you were able to do per our conversation in slack @adambkaplan

[adambkaplan]

Confirmed - the observe features function in library-go will write a []string to the raw JSON that is embedded in the ConfigMap:

data:
  config.yaml: |
    {"apiVersion":"openshiftcontrolplane.config.openshift.io/v1","build":{"buildDefaults":{"resources":{}},"imageTemplateFormat":{"format":"registry.build01.ci.openshift.org/ci-ln-skv518b/stable@sha256:7e9d76442b6ae3b7c19f7163b65ae0d5334fbbac42dfb50b62f6afc63f239634"}},"deployer":{"imageTemplateFormat":{"format":"registry.build01.ci.openshift.org/ci-ln-skv518b/stable@sha256:b356a5173fd2a2f4cce8b1bd7640cb94e5375a778409b72d12434196c6936eb3"}},"dockerPullSecret":{"internalRegistryHostname":"image-registry.openshift-image-registry.svc:5000"},"featureGates":["BuildCSIVolumes=true"],"ingress":{"ingressIPNetworkCIDR":""},"kind":"OpenShiftControllerManagerConfig"}
...

[adambkaplan]

/lgtm

@deads2k build team is in agreement with this approach.

[deads2k]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, deads2k, gabemontero

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 [deads2k]

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