VEP 104: Allow disabling feature gates

[iholder101]

VEP Metadata

Tracking issue: #104
SIG label: /sig compute

What this PR does

Allows to explicitly disable feature gates.

Special notes for your reviewer

VEP in a human readable format: https://github.com/iholder101/kubevirt-enhancements/blob/vep104/initial-vep-pr/veps/sig-compute/104_allow_disabling_feature_gates/vep.md.

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign xpivarc for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[iholder101]

/cc @nunnatsa

[iholder101]

An option question: Do we need a feature gate for this change?

I think it makes sense to not have a feature gate here for these reasons:

1. Logically, this change is very straight-forward. The main reason for having this VEP from my side is to have a thorough discussion regarding APIs.
2. It will unblock enabling beta FGs by default, which is a HUGE gain that we need to apply ASAP.

[iholder101]

I vote in favor of this approach BTW.

[nunnatsa]

Me too

[Barakmor1]

Thank you for taking the initiative to move this forward! Since we currently don’t have a way to disable feature gates, we cannot enable them by default. This affects the feature gate lifecycle and makes it harder to test beta feature gates at scale. That is why I believe this proposal is very important and should be a high priority.

Regarding the proposed solutions, I am fine with approach 1, and I am also okay with approach 4, even though it adds some parsing overhead that needs careful handling. Maintaining an extra API field for something as central as feature gates has a high cost, so this trade-off is worth considering.

[Barakmor1]

I wonder if we should make this string follow best practice conventions, even though the enabled field is only expected to have true or false values. What do you think?

[iholder101]

I wonder if we should make this string follow best practice conventions

By "best practices" you mean to define it as a struct instead of a bool? This is interesting.

On the hand this would make Enabled extendible, which is always nice. On the other hand, this makes a bit less human friendly syntax.

Let's see what others think, but I'm open to this approach 👍

[lyarwood]

Thanks for pushing this forward as a VEP @iholder101, hopefully something we can agree on and land for v1.8.0.

[nunnatsa]

Question about validation:

Should the webhook validate the FG list? I can think about the following cases - should we address them?

• user adds a deprecated FG
• user adds a GA FG
• user is trying to disable a GA FG
• user adds unknown FG
• user enable FG that is enabled by default (beta) - this could be a real use case, when the FG was enabled before an upgrade from a version where the FG was disabled by default (alpha).
• user disables FG that id disabled by default (alpha) - this could be a real use case, when the user disables an unwanted feature befor upgrade to a version, when it becomes beta, and is enabled by default.

Note: anyway, even if we want to validate FGs, IMHO, the result must be a warning, rather than an error.

[iholder101]

Question about validation:

Should the webhook validate the FG list? I can think about the following cases - should we address them?

• user adds a deprecated FG
• user adds a GA FG
• user is trying to disable a GA FG
• user adds unknown FG
• user enable FG that is enabled by default (beta) - this could be a real use case, when the FG was enabled before an upgrade from a version where the FG was disabled by default (alpha).
• user disables FG that id disabled by default (alpha) - this could be a real use case, when the user disables an unwanted feature befor upgrade to a version, when it becomes beta, and is enabled by default.
  Note: anyway, even if we want to validate FGs, IMHO, the result must be a warning, rather than an error.

Adding validations, or enabling beta features by default, is out-of-scope for this VEP and should be addressed elsewhere.
Let's talk about these interesting cases elsewhere or offline :)

[iholder101]

I've now added the following to the VEP:

• Expressed that it's out-of-scope to enable beta features by default.
• Expressed that I wish to make an exception to jump straight to GA with this one.