feature-gates: adding initial structure

[beraldoleal]

This commit introduces the FeatureGates struct in the operator's configuration, enabling a flexible and controlled approach to feature rollout. The FeatureGates struct allows for the enabling or disabling of specific features, with a default behaviour based on each feature's maturity level.

Uses Kubebuilder default value annotations for CRD fields to automatically set default behaviours in the Kubernetes API.

I'm trying to use a simple approach here where we could change promote/depromote a feature by just changing the comments and the default values.

Please let know what I'm missing (probably I'm missing a lot) and what you think about this approach.

[snir911]

Added some comments, however, looks like the right direction to me

[snir911]

@beraldoleal thanks! LGTM overall, couple of questions:

1. are we merging it with the dummy features? i.e. timeTrabel, AIhealing etc...?
2. consider filtering the events using Predicate rather than handler, see this reference (IIUC it would filter the events before actually emitted and handled)
3. Should we deploy also the osc-feature-gates CM with the default values so it can be used as a template for the user? IDK if it's even possible...

[beraldoleal]

Hi @snir911 , my comments bellow:

1. are we merging it with the dummy features? i.e. timeTrabel, AIhealing etc...?

Good question. I was planning to keep the default map empty (since is a code block) but keep the config/samples/featuregates.yaml there with some notes, saying those are fake... (since is just an example and until we have our first feature on boarded). Wdyt?

2. consider filtering the events using Predicate rather than handler, see this reference (IIUC it would filter the events before actually emitted and handled)

When considering the implementation, my initial thought was to use Predicates. However, since the other watches were utilizing EventHandlers, I opted to maintain consistency and avoid a mix of approaches. To be honest, I also prefer using Predicates. If you see no issue with incorporating both EventHandlers and Predicates within the same setup, we could explore this option.

3. Should we deploy also the osc-feature-gates CM with the default values so it can be used as a template for the user? IDK if it's even possible...

I believe it's possible, yes, but it might be unnecessary. The default values are intended for use as-is, without the need for user intervention. Introducing another ConfigMap into users' deployments for those adhering to the default values might not be ideal. The config/samples/ directory serves as a source of "templates" for users. So, unless there is something I'm missing, I would say to keep it just in config/samples/.

[snir911]

Hi @snir911 , my comments bellow:

1. are we merging it with the dummy features? i.e. timeTrabel, AIhealing etc...?

Good question. I was planning to keep the default map empty (since is a code block) but keep the config/samples/featuregates.yaml there with some notes, saying those are fake... (since is just an example and until we have our first feature on boarded). Wdyt?

IDK, we can either start with a real feature (maybe peer-pods? but just print a msg instead of actually enabling it ATM?) or maybe to leave only single dummy feature?

2. consider filtering the events using Predicate rather than handler, see this reference (IIUC it would filter the events before actually emitted and handled)

When considering the implementation, my initial thought was to use Predicates. However, since the other watches were utilizing EventHandlers, I opted to maintain consistency and avoid a mix of approaches. To be honest, I also prefer using Predicates. If you see no issue with incorporating both EventHandlers and Predicates within the same setup, we could explore this option.

AFAIK there's no problem to mix (we do it in the credentials controller), the handlers are usually used when there are some ops needed IIUC..

3. Should we deploy also the osc-feature-gates CM with the default values so it can be used as a template for the user? IDK if it's even possible...

I believe it's possible, yes, but it might be unnecessary. The default values are intended for use as-is, without the need for user intervention. Introducing another ConfigMap into users' deployments for those adhering to the default values might not be ideal. The config/samples/ directory serves as a source of "templates" for users. So, unless there is something I'm missing, I would say to keep it just in config/samples/.

Sure, anyway we can do it at any point..

[beraldoleal]

IDK, we can either start with a real feature (maybe peer-pods? but just print a msg instead of actually enabling it ATM?) or maybe to leave only single dummy feature?

Currently peerpods is enabled by another way... adding peer-pods to the feature gates without disabling the other way it will be confuse. Also, if we add a new feature with just a print message (lets say "CoCo") it will give the users the wrong impression. IMO it will be confusing, because users might think this is actually enabling.

If we don´t have a feature to onboard now, Keeping a single dummy feature reinforces this is fake. So it think is safe (at least until we replace it with a real feature).

AFAIK there's no problem to mix (we do it in the credentials controller), the handlers are usually used when there are some ops needed IIUC..

Ok, I will send a new version with the Predicates.

Thanks again, @snir911 .

[beraldoleal]

@snir911 @bpradipt just pushed with the Predicate implementation, let me know if I'm missing something.

[openshift-ci]

@beraldoleal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sandboxed-containers-operator-e2e	7ac6079	link	false	/test sandboxed-containers-operator-e2e
ci/prow/check	7ac6079	link	false	/test check

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.

[bpradipt]

/lgtm
Thanks @beraldoleal

[bpradipt]

/lgtm
Thanks @beraldoleal

[beraldoleal]

@snir911 ?

[snir911]

LGTM, thanks! (assuming it was tested)