Parse CustomNoUpgrade feature gates #262

bertinatto · 2020-07-09T17:23:31Z

Currently if we add a feature gate to the featuregates.config.openshift.io/cluster object, the controller-manager and the apiserver correctly enable the feature, but the scheduler does not.

This patch adds the missing code that handles this functionality.

bertinatto · 2020-07-09T17:24:08Z

/assign @soltysh

damemi

This change looks fine to me, but does it need to be broken into a new function like you have currently? It looks like there's now 5 helpers all <20 lines that are called in a row to parse the feature gate. That's not your fault, but I don't know if this is helping the readability.

I think this change can be added to the checkForFeatureGates function like so:

func checkForFeatureGates(featureGateLister configlistersv1.FeatureGateLister) map[string]bool {
	featureGateListConfig, err := featureGateLister.Get("cluster")
	var enabledFeatureSets, disabledFeatureSets []string
	var featureGates = make(map[string]bool)
	if err != nil {
		klog.Infof("Error while listing features.config.openshift.io/cluster with %v: so return default feature gates", err.Error())
		if featureSet, ok := v1.FeatureSets[v1.Default]; ok {
			enabledFeatureSets = featureSet.Enabled
			disabledFeatureSets = featureSet.Disabled
		}
		return generateFeatureGates(enabledFeatureSets, disabledFeatureSets, featureGates)
	}

	if featureGateListConfig.Spec.FeatureSet == v1.CustomNoUpgrade {
		if featureGateListConfig.Spec.FeatureGateSelection.CustomNoUpgrade != nil {
			enabledFeatureSets = featureGateListConfig.Spec.FeatureGateSelection.CustomNoUpgrade.Enabled
			disabledFeatureSets = featureGateListConfig.Spec.FeatureGateSelection.CustomNoUpgrade.Disabled
		}
	} else if featureSet, ok := v1.FeatureSets[featureGateListConfig.Spec.FeatureSet]; ok {
		enabledFeatureSets = featureSet.Enabled
		disabledFeatureSets = featureSet.Disabled
	} else {
		klog.Infof("Invalid feature set config found in features.config.openshift.io/cluster %v. Please look at allowed features", featureGateListConfig.Spec.FeatureSet)
	}
	return generateFeatureGates(enabledFeatureSets, disabledFeatureSets, featureGates)
}

Does that look reasonable? The if/elseif/else isn't quite as clean as using straight returns, but I think this would help keep this code more readable. I won't block on this though so I'll approve now, and if you want to push that change then someone can lgtm
/approve

Currently if we add a feature gate to the featuregates.config.openshift.io/cluster object, the controller-manager and the apiserver correctly enable the feature, but the scheduler does not. This patch adds the missing code that handles this functionality.

bertinatto · 2020-07-10T12:34:37Z

Does that look reasonable? The if/elseif/else isn't quite as clean as using straight returns, but I think this would help keep this code more readable. I won't block on this though so I'll approve now, and if you want to push that change then someone can lgtm

Hi Mike, I'm fine with that. PTAL

bertinatto · 2020-07-10T12:38:02Z

/retest

damemi

/lgtm
Thanks!

openshift-ci-robot · 2020-07-10T13:55:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, damemi

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~~ [damemi]

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

bertinatto · 2020-07-10T13:55:47Z

/retest

openshift-bot · 2020-07-10T16:49:51Z

/retest