Read featuregates from cluster state #1468

deads2k · 2023-02-14T22:40:25Z

Part 2 of a multi-step plan to make individual featuregates observable via the API. This was previously avoided because when featuregates get promoted from TechPreview to Default, during an upgrade, an old operator may try to enable a TechPreview variant of a feature.

This can be addressed by keying the featuregates by version. Let's see how ugly that will be in practice now that we have a decent build-out of library-go.

Unit tests are still owed. Proof is still owed. cluster-config-operator controller is still owed.

/hold

This is proving the concept, not to be merged until it shows as working end-to-end.

Other steps include

cluster-config-operator (upgrades first) writing the new featuregate
enhancements write up describing how it works end to end.

/assign @JoelSpeed

JoelSpeed · 2023-02-17T11:06:14Z

pkg/operator/configobserver/featuregates/simple_featuregate_reader.go

+	c.lock.Lock()
+	defer c.lock.Unlock()
+
+	c.observationCount++


Does this need to be incremented every time? I guess since this isn't going to happen often you'd expect this number to be small, but I was wondering if the initialFeatureGatesObserved could be the signal, seems there's a duplication right now

if c.observationCount == 0 { return nil, nil, fmt.Errorf("featureGates not yet observed") }

This bit for example would be the same as checking if the initial feature gates have been observed.

Does this need to be incremented every time?

Yeah, it might be possible to rewrite on the channel. Let's be sure I get this unified before we merge anything.

JoelSpeed · 2023-02-17T11:07:25Z

pkg/operator/configobserver/featuregates/simple_featuregate_reader.go

+	initialFeatureGatesObserved               chan struct{}
+	featureGatesHaveChangedSinceFirstObserved chan struct{}


Just trying to clarify why we use channels, IIUC these are being used as signals to other threads, the motivation for these being channels over bools is that you don't need a RWMutex around the values if they are channels?

Just trying to clarify why we use channels, IIUC these are being used as signals to other threads, the motivation for these being channels over bools is that you don't need a RWMutex around the values if they are channels?

Yes it is for signals to other threads. Using channels allows for waiting, which is not as easy with bools. For example, this is easy to make with channels, hard to make reactive with bools.

select{ case <- featureGateAccessor.InitialFeatureGatesObserved(): enabled, disabled, _ := featureGateAccessor.CurrentFeatureGates() klog.Infof("FeatureGates initialized: enabled=%v disabled=%v", enabled, disabled) case <- time.After(1*time.Minute): klog.Errorf("timed out waiting for FeatureGate detection") return fmt.Errorf("timed out waiting for FeatureGate detection") }

JoelSpeed · 2023-02-17T11:10:44Z

vendor/github.com/openshift/api/config/v1/types_feature.go

+	// possible (probable?) future additions include
+	// 1. support level (Stable, ServiceDeliveryOnly, TechPreview, DevPreview)
+	// 2. description


I like the idea of a support level in here, would give a point for users to observe what features are now on that were before, diffing the historic versions.

Something that is in the list that is Stable would only need to be included for 1 release right? As once we go to stable we can remove the gating logic once we've gone through the upgrade?

We are going to need some detailed documentation about process and expectations for our engineers around this process I think

deads2k · 2023-03-08T21:19:02Z

pkg/operator/configobserver/featuregates/observe_featuregates.go

-	return newConfigValue, nil
-}
-
-func FeaturesGatesFromFeatureSets(fg *configv1.FeatureGate) ([]string, []string, error) {


note to self, you still want this for operators that have items on the bootstrap host.

note to self, you still want this for operators that have items on the bootstrap host.

no you won't. They'll update to read the manifest from the installer.

deads2k · 2023-03-20T20:18:06Z

comments addressed, code simplified, unit test added. other than the fake bump, this should be ready.

JoelSpeed

Other than some additional documentation, I think this looks ready to go

JoelSpeed · 2023-04-10T22:38:32Z

pkg/operator/configobserver/featuregates/simple_featuregate_reader.go

This file is a little light on godoc for the functions, structs and interfaces, I think given we hope many people adopt this, some inline documentation would be helpful. Anything exported should have something so that pkg.go.dev is useful

…gate detection mechanism

deads2k · 2023-04-24T19:23:02Z

rebased and comments added. Will open a new PR with proof once openshift/cluster-config-operator#288 merges

openshift-ci · 2023-04-24T19:28:03Z

@deads2k: all tests passed!

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.

JoelSpeed · 2023-04-25T11:16:10Z

/lgtm

JoelSpeed · 2023-04-25T11:16:29Z

/hold

Will check the proof PR before merge

openshift-ci · 2023-04-25T11:17:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed

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:

~~pkg/operator/OWNERS~~ [deads2k]

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

JoelSpeed · 2023-04-25T13:16:43Z

/hold cancel

Proof PR got a successful install and can see logs indicating success in CCO and KCMO

openshift-ci bot assigned JoelSpeed Feb 14, 2023

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 14, 2023

openshift-ci bot requested review from mfojtik and sttts February 14, 2023 22:40

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2023

JoelSpeed reviewed Feb 17, 2023

View reviewed changes

deads2k commented Mar 8, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2023

deads2k force-pushed the read-featuregates branch from 36a15c9 to 654236c Compare March 20, 2023 20:17

deads2k force-pushed the read-featuregates branch from 654236c to 7cca762 Compare March 20, 2023 20:23

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2023

deads2k changed the title ~~[wip] Read featuregates from cluster state~~ Read featuregates from cluster state Mar 28, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2023

deads2k mentioned this pull request Apr 10, 2023

provide new interface with old method of reading featuregates #1497

Merged

JoelSpeed reviewed Apr 10, 2023

View reviewed changes

deads2k force-pushed the read-featuregates branch from 7cca762 to d4a93d4 Compare April 24, 2023 19:17

add featuregate convenience functions for the new techpreview feature…

19d1252

…gate detection mechanism

deads2k force-pushed the read-featuregates branch from d4a93d4 to 19d1252 Compare April 24, 2023 19:22

deads2k mentioned this pull request Apr 24, 2023

use new featuregate API openshift/cluster-kube-controller-manager-operator#730

Merged

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2023

openshift-merge-robot merged commit 00923e0 into openshift:master Apr 25, 2023

		initialFeatureGatesObserved chan struct{}
		featureGatesHaveChangedSinceFirstObserved chan struct{}

Read featuregates from cluster state #1468

Read featuregates from cluster state #1468

Uh oh!

Conversation

deads2k commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 20, 2023

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 24, 2023

Uh oh!

openshift-ci bot commented Apr 24, 2023

Uh oh!

JoelSpeed commented Apr 25, 2023

Uh oh!

JoelSpeed commented Apr 25, 2023

Uh oh!

openshift-ci bot commented Apr 25, 2023

Uh oh!

JoelSpeed commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

deads2k commented Feb 14, 2023 •

edited

Loading