Change the FeatureGates implemantation #1060

nunnatsa · 2021-01-17T11:14:14Z

Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate.

The previose implementation used a map[string]bool to store the feature gates. That way, it was impossible to add documentation for each feature gate.

This PR changes the implementation so the feature gates are now stored in a struct, as a pointer to boolean (to keep them optional). The result is a documentable feature gates as well as strict naming of the feature gates, because their names are not just astring but a field name.

Since HotplugVolumes is not a valid json or yaml tag name (starts with upper case), the gate name was changed to hotplugVolumes (starts with lower case)

As part of this change, the HyperConverged CRD contains the documentation for the hotplugVolumes feature gate, that can be read directly from the CRD, or by calling kubectl explain:

$ kubectl explain HyperConverged.spec.featureGates
KIND:     HyperConverged
VERSION:  hco.kubevirt.io/v1beta1

RESOURCE: featureGates <Object>

DESCRIPTION:
     featureGates HyperConvergedFeatureGates contains a list of feature enabler
     flags. Setting a flag to `true` will enable the feature. Setting `false` or
     removing the feature gate, disables the feature.

FIELDS:
   hotplugVolumes	<boolean>
     Allow attaching a data volume to a running VMI; type is boolean; default is
     false

In addition, this PR adds feature gates documentation to the docs/cluster-configuration.md file with the following usage example:

apiVersion: hco.kubevirt.io/v1beta1
kind: HyperConverged
metadata:
  name: kubevirt-hyperconverged
spec:
  infra: {}
  workloads: {}
  featureGates:
    hotplugVolumes: true

Signed-off-by: Nahshon Unna-Tsameret [email protected]

Release note:

Added CRD documentation to the hotplugVolumes feature gate
Added Feature gates documentation to the cluster-configuration.md document
Change HotplugVolumes to hotplugVolumes

Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

docs/cluster-configuration.md

deploy/olm-catalog/kubevirt-hyperconverged/1.4.0/hco00.crd.yaml

docs/cluster-configuration.md

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

pkg/apis/hco/v1beta1/hyperconverged_types.go

pkg/components/components.go

pkg/controller/operands/kubevirt.go

zcahana · 2021-01-17T14:57:02Z

And another general comment - what is the significance (in terms of API design) of grouping various knobs under a featureGates: field? Is it to indicate the immaturity or the temporariness of the feature gate? Is it a "part" of the HyperConverged CR that we deliberately tell users that nested fields (e.g., .hotplugVolumes) will likely be deprecated soon and later removed (i.e. once the feature graduates, or withdrawn).

pkg/apis/hco/v1beta1/hyperconverged_types.go

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

hco-bot · 2021-01-17T18:41:12Z

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2021-01-17T18:41:16Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

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.

enp0s3 · 2021-01-17T21:02:21Z

And another general comment - what is the significance (in terms of API design) of grouping various knobs under a featureGates: field? Is it to indicate the immaturity or the temporariness of the feature gate? Is it a "part" of the HyperConverged CR that we deliberately tell users that nested fields (e.g., .hotplugVolumes) will likely be deprecated soon and later removed (i.e. once the feature graduates, or withdrawn).

@zcahana Yes. Once a preview/optional feature will grown up into a fully fledged one, we will remove it from the feature gate section and perhaps it will have it's own place in the HCO CR.

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

enp0s3

/lgtm
@nunnatsa I am feeling uncomforatble with using fmt.Print in the code, but if there is a promise to refactor the code to add a logger then let's go forward with this code change.

openshift-ci · 2021-01-18T06:41:28Z

@nunnatsa: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/hco-e2e-image-index-gcp	`2ea7ee7`	link	`/test hco-e2e-image-index-gcp`

Full PR test history. Your PR dashboard.

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.

hco-bot · 2021-01-18T06:56:49Z

hco-e2e-upgrade-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-azure
hco-e2e-image-index-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-azure
hco-e2e-upgrade-prev-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-azure

kubevirt-bot · 2021-01-18T06:56:56Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-azure, ci/prow/hco-e2e-upgrade-azure, ci/prow/hco-e2e-upgrade-prev-azure

In response to this:

hco-e2e-upgrade-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-azure
hco-e2e-image-index-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-azure
hco-e2e-upgrade-prev-aws lane succeeded.
/override ci/prow/hco-e2e-upgrade-prev-azure

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.

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

nunnatsa · 2021-01-18T07:49:23Z

@nunnatsa I am feeling uncomforatble with using fmt.Print in the code, but if there is a promise to refactor the code to add a logger then let's go forward with this code change.

Done.

tiraboschi · 2021-01-18T08:10:07Z

/approve

kubevirt-bot · 2021-01-18T08:10:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tiraboschi

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

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

enp0s3

/lgtm

nunnatsa · 2021-01-18T09:09:47Z

/override-bot

hco-bot · 2021-01-18T09:10:18Z

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2021-01-18T09:10:21Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

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.

nunnatsa · 2021-01-18T09:13:11Z

/cherry-pick release-1.3

kubevirt-bot · 2021-01-18T09:14:11Z

@nunnatsa: #1060 failed to apply on top of branch "release-1.3":

.git/rebase-apply/patch:83: trailing whitespace.
Some configurations are done separately to Infra and Workloads. The CR's Spec object contains the `infra` and the 
.git/rebase-apply/patch:100: trailing whitespace.
The `featureGates` field is an optional set of optional boolean feature enabler. The features in this list are advanced 
.git/rebase-apply/patch:103: trailing whitespace.
To enable a feature, add its name to the `featureGates` list and set it to `true`. Missing or `false` feature gates 
warning: 3 lines add whitespace errors.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
A	deploy/index-image/kubevirt-hyperconverged/1.4.0/hco00.crd.yaml
A	deploy/olm-catalog/kubevirt-hyperconverged/1.4.0/hco00.crd.yaml
M	pkg/apis/hco/v1beta1/hyperconverged_types_test.go
M	pkg/components/components.go
M	pkg/controller/operands/kubevirt.go
M	pkg/controller/operands/kubevirt_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/controller/operands/kubevirt_test.go
CONFLICT (content): Merge conflict in pkg/controller/operands/kubevirt_test.go
Auto-merging pkg/controller/operands/kubevirt.go
CONFLICT (content): Merge conflict in pkg/controller/operands/kubevirt.go
Auto-merging pkg/components/components.go
Auto-merging pkg/apis/hco/v1beta1/hyperconverged_types_test.go
Auto-merging deploy/olm-catalog/kubevirt-hyperconverged/1.3.0/hco00.crd.yaml
Auto-merging deploy/index-image/kubevirt-hyperconverged/1.3.0/hco00.crd.yaml
Patch failed at 0001 Change the FeatureGates implemantation

In response to this:

/cherry-pick release-1.3

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.

* Change the FeatureGates implemantation Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * modify HotplugVolumes casing to hotplugVolumes Signed-off-by: Nahshon Unna-Tsameret <[email protected]> (cherry picked from commit ead55cf) Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate. The previose implementation used a `map[string]bool` to store the feature gates. That way, it was impossible to add documentation for each feature gate. This PR changes the implementation so the feature gates are now stored in a struct, as a pointer to boolean (to keep them optional). The result is a documentable feature gates as well as strict naming of the feature gates, because their names are not just astring but a field name. Since `HotplugVolumes` is not a valid json or yaml tag name (starts with upper case), the gate name was changed to `hotplugVolumes` (starts with lower case) As part of this change, the HyperConverged CRD contains the documentation for the `hotplugVolumes` feature gate, that can be read directly from the CRD, or by calling `kubectl explain`: ``` $ kubectl explain HyperConverged.spec.featureGates KIND: HyperConverged VERSION: hco.kubevirt.io/v1beta1 RESOURCE: featureGates <Object> DESCRIPTION: featureGates HyperConvergedFeatureGates contains a list of feature enabler flags. Setting a flag to `true` will enable the feature. Setting `false` or removing the feature gate, disables the feature. FIELDS: hotplugVolumes <boolean> Allow attaching a data volume to a running VMI; type is boolean; default is false ``` In addition, this PR adds feature gates documentation to the `docs/cluster-configuration.md` file with the following usage example: ```yaml apiVersion: hco.kubevirt.io/v1beta1 kind: HyperConverged metadata: name: kubevirt-hyperconverged spec: infra: {} workloads: {} featureGates: hotplugVolumes: true ``` Signed-off-by: Nahshon Unna-Tsameret <[email protected]> **Release note**: ```release-note Added CRD documentation to the hotplugVolumes feature gate Added Feature gates documentation to the cluster-configuration.md document Change HotplugVolumes to hotplugVolumes ``` Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

* Change the FeatureGates implemantation (#1060) * Change the FeatureGates implemantation Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Fix code-review comments Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * modify HotplugVolumes casing to hotplugVolumes Signed-off-by: Nahshon Unna-Tsameret <[email protected]> (cherry picked from commit ead55cf) Signed-off-by: Nahshon Unna-Tsameret <[email protected]> * Change the FeatureGates implemantation (#1060) Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate. The previose implementation used a `map[string]bool` to store the feature gates. That way, it was impossible to add documentation for each feature gate. This PR changes the implementation so the feature gates are now stored in a struct, as a pointer to boolean (to keep them optional). The result is a documentable feature gates as well as strict naming of the feature gates, because their names are not just astring but a field name. Since `HotplugVolumes` is not a valid json or yaml tag name (starts with upper case), the gate name was changed to `hotplugVolumes` (starts with lower case) As part of this change, the HyperConverged CRD contains the documentation for the `hotplugVolumes` feature gate, that can be read directly from the CRD, or by calling `kubectl explain`: ``` $ kubectl explain HyperConverged.spec.featureGates KIND: HyperConverged VERSION: hco.kubevirt.io/v1beta1 RESOURCE: featureGates <Object> DESCRIPTION: featureGates HyperConvergedFeatureGates contains a list of feature enabler flags. Setting a flag to `true` will enable the feature. Setting `false` or removing the feature gate, disables the feature. FIELDS: hotplugVolumes <boolean> Allow attaching a data volume to a running VMI; type is boolean; default is false ``` In addition, this PR adds feature gates documentation to the `docs/cluster-configuration.md` file with the following usage example: ```yaml apiVersion: hco.kubevirt.io/v1beta1 kind: HyperConverged metadata: name: kubevirt-hyperconverged spec: infra: {} workloads: {} featureGates: hotplugVolumes: true ``` Signed-off-by: Nahshon Unna-Tsameret <[email protected]> **Release note**: ```release-note Added CRD documentation to the hotplugVolumes feature gate Added Feature gates documentation to the cluster-configuration.md document Change HotplugVolumes to hotplugVolumes ``` Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

Change the FeatureGates implemantation

229a6c9

Change the FeatureGates implemantation to be strict and to add documentation to the CR for each feature gate Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 17, 2021

kubevirt-bot requested review from andreyod and erkanerol January 17, 2021 11:14

kubevirt-bot added the size/XL label Jan 17, 2021

dankenigsberg reviewed Jan 17, 2021

View reviewed changes

Fix code-review comments

f51dbcd

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

zcahana reviewed Jan 17, 2021

View reviewed changes

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jan 17, 2021

zcahana reviewed Jan 17, 2021

View reviewed changes

pkg/apis/hco/v1beta1/hyperconverged_types.go Outdated Show resolved Hide resolved

zcahana reviewed Jan 17, 2021

View reviewed changes

pkg/apis/hco/v1beta1/hyperconverged_types.go Outdated Show resolved Hide resolved

Fix code-review comments

2ea7ee7

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

Fix code-review comments

043c5fb

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

enp0s3 approved these changes Jan 18, 2021

View reviewed changes

kubevirt-bot assigned enp0s3 Jan 18, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

modify HotplugVolumes casing to hotplugVolumes

25dc64e

Signed-off-by: Nahshon Unna-Tsameret <[email protected]>

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

tiraboschi approved these changes Jan 18, 2021

View reviewed changes

kubevirt-bot assigned tiraboschi Jan 18, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2021

enp0s3 approved these changes Jan 18, 2021

View reviewed changes

kubevirt-bot merged commit ead55cf into kubevirt:master Jan 18, 2021

nunnatsa deleted the strict-fg-api branch January 18, 2021 09:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the FeatureGates implemantation #1060

Change the FeatureGates implemantation #1060

nunnatsa commented Jan 17, 2021 •

edited

Loading

zcahana commented Jan 17, 2021

hco-bot commented Jan 17, 2021

kubevirt-bot commented Jan 17, 2021

enp0s3 commented Jan 17, 2021 •

edited

Loading

enp0s3 left a comment

openshift-ci bot commented Jan 18, 2021 •

edited

Loading

hco-bot commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

nunnatsa commented Jan 18, 2021

tiraboschi commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

enp0s3 left a comment

nunnatsa commented Jan 18, 2021

hco-bot commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

nunnatsa commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

Change the FeatureGates implemantation #1060

Change the FeatureGates implemantation #1060

Conversation

nunnatsa commented Jan 17, 2021 • edited Loading

zcahana commented Jan 17, 2021

hco-bot commented Jan 17, 2021

kubevirt-bot commented Jan 17, 2021

enp0s3 commented Jan 17, 2021 • edited Loading

enp0s3 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 18, 2021 • edited Loading

hco-bot commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

nunnatsa commented Jan 18, 2021

tiraboschi commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

enp0s3 left a comment

Choose a reason for hiding this comment

nunnatsa commented Jan 18, 2021

hco-bot commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

nunnatsa commented Jan 18, 2021

kubevirt-bot commented Jan 18, 2021

nunnatsa commented Jan 17, 2021 •

edited

Loading

enp0s3 commented Jan 17, 2021 •

edited

Loading

openshift-ci bot commented Jan 18, 2021 •

edited

Loading