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

Enhancement proposal for supporting feature gates in OLM #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fgiloux
Copy link
Contributor

@fgiloux fgiloux commented May 27, 2022

As part of the optional manifest implementation for OLM it was highlighted that it would be best introduced behind a feature gate. As there is currently no flexible feature gate mechanism in OLM this enhancement proposal details an approach for introducing one. The approach is strongly inspired by what has been done in Kubernetes.

Signed-off-by: Frederic Giloux fgiloux@redhat.com

Signed-off-by: Frederic Giloux <fgiloux@redhat.com>
@fgiloux
Copy link
Contributor Author

fgiloux commented May 27, 2022

cc @joelanford @perdasilva @awgreene


#### Feature gate

The feature gate implementation will leverage existing [Kubernetes libraries](https://pkg.go.dev/k8s.io/component-base/featuregate).
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this imply that all OLM sub-projects need to take a direct dependency on the k8s.io/component-base package?


#### OLMConfig

A new version v2alpha1 of the OLMConfig API will be created and supported along v1. This version will introduce a list named `gates` for activation and deactivation of features:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a field to the olmConfig.status that communicates the status of the enabled/disabled feature?

Copy link
Member

@awgreene awgreene Jun 27, 2022

Choose a reason for hiding this comment

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

An immediate instance where this may be useful is handling the legacy disableCopiedCSVs feature toggle, in which conflicting values between the old and new api can result in an error.

Copy link
Member

Choose a reason for hiding this comment

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

Some features may also take time to configure, so this could communicate feature state.


#### DisableCopiedCSVs

As DisableCopiedCSVs already exists. As a first class field its support needs to be guaranteed. As such the API field will be kept and deprecated but the implementation will convert it to the map key/value pair and will handle it in a way consistent with the other feature gates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we surface this behavior/context in the open api description for this field to help improve local UX?

type: string
value:
description: value is a boolean that specifies whether the feature is activated or not.
type: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

i think api review is going to tell you to use an ENUM with "enabled" and "disabled" as the initial valid values


#### OLMConfig

A new version v2alpha1 of the OLMConfig API will be created and supported along v1. This version will introduce a list named `gates` for activation and deactivation of features:
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a new api version to add these fields?


This enhancement proposal aims at bringing the following benefits to OLM:

- A more phased approach of releasing features.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing in this EP really addresses the implications of a user turning on an alpha/beta/techpreview feature. This is why OCP has the concept of "Techpreviewnoupgrade" among other things, in order to:

  1. make it abundantly clear to the admin that the feature they are turning on is techpreview (you can also achieve this by naming your featuregates appropriately)
  2. make it clear they will not be able to upgrade the cluster (And then enforcing that constraint). It is somewhat up to OLM if they want a similar enforcement, but there are many many subtleties to deal with if you allow upgrades after a techpreview feature has defined some APIs and stored resources using that api. I think it is preferable if you can tie the enablement of TP features to blocking cluster upgrades (of course that is an OCP-specific aspect, so it may not belong in upstream OLM)

One option might be for downstream OLM to override/disable any featuregates that are techpreview, unless the OCP techpreviewnoupgrade featuregate is enabled at the cluster level.

In any case, i'd like to see more discussion of how alpha/beta/techpreview featuregates will be managed/expressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants