CORENET-6714: Enable Network Observability on Day 0 #2752
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
| name: "Network" | ||
| crdName: networks.config.openshift.io | ||
| featureGate: NetworkObservabilityInstall | ||
| tests: | ||
| onCreate: | ||
| - name: Should be able to set InstallNetworkObservability | ||
| initial: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: Network | ||
| spec: | ||
| installNetworkObservability: "Enable" | ||
| expected: | | ||
| apiVersion: config.openshift.io/v1 | ||
| kind: Network | ||
| spec: | ||
| installNetworkObservability: "Enable" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -86,6 +86,14 @@ type NetworkSpec struct { | |||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||||||||||
| NetworkDiagnostics NetworkDiagnostics `json:"networkDiagnostics"` | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // installNetworkObservability is an optional field that enables network observability | ||||||||||||||||||||||||||||||||||||||
| // when omitted or set to enable. If the field is set to disable, it does nothing. | ||||||||||||||||||||||||||||||||||||||
| // Valid values are "", "Enable", "Disable". | ||||||||||||||||||||||||||||||||||||||
| // +openshift:enable:FeatureGate=NetworkObservabilityInstall | ||||||||||||||||||||||||||||||||||||||
| // +kubebuilder:validation:Enum:="";Enable;Disable | ||||||||||||||||||||||||||||||||||||||
| // +optional | ||||||||||||||||||||||||||||||||||||||
| InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"` | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+89
to
+96
There was a problem hiding this comment. 🛠️ Refactor suggestion | 🟠 Major New field should be gated by a feature gate. Per reviewer everettraven's comment on this PR, new fields in OpenShift APIs should be introduced behind a feature gate (disabled by default) and go through the standard feature promotion process. This ensures the field can be safely introduced and tested before becoming generally available. The repository has guidance on adding feature gates and gating fields. 🤖 Prompt for AI Agents
Contributor
There was a problem hiding this comment. This is correct and needs to be done. This cannot just go straight to GA. There was a problem hiding this comment. Code Implementation 🛠️Implementation: Update the
Suggested change
See review comment here There was a problem hiding this comment.
Required changes to introduce the feature gate1. Register the new feature gate in Add an exported variable similar to how other gates are registered. A gate declaration in // FeatureGateInstallNetworkObservability enables automatic installation of
// the Network Observability operator during cluster bootstrap.
var FeatureGateInstallNetworkObservability = newFeatureGate("InstallNetworkObservability").
reportProblemsToJiraComponent("Network Observability").
contactPerson("OlivierCazade").
productScope(ocpSpecific).
enhancementPR("https://github.com/openshift/enhancements/pull/1908").
enableIn(configv1.TechPreviewNoUpgrade).
mustRegister()2. Annotate the field in Add the // installNetworkObservability is an optional field that enables network observability
// when omitted or set to True. If the field is set to False, it does nothing.
// Valid values are "", "True", "False".
// +openshift:enable:FeatureGate=InstallNetworkObservability
// +kubebuilder:validation:Enum="";True;False
// +optional
InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"`3. Update 4. Re-run code generation to update:
Good reference PRs for this pattern: There was a problem hiding this comment.
Comment on lines
+90
to
+96
There was a problem hiding this comment. Description does not match enum values. Same issue as the CRD: the comment on lines 90-92 documents 📝 Proposed fix to align comment with enum- // installNetworkObservability is an optional field that enables network observability
- // when omitted or set to enable. If the field is set to disable, it does nothing.
- // Valid values are "", "Enable", "Disable".
+ // installNetworkObservability is an optional field that enables network observability
+ // when omitted or set to True. If the field is set to False, it does nothing.
+ // Valid values are "", "True", "False".
// +kubebuilder:validation:Enum:="";True;False
// +optional
InstallNetworkObservability *string `json:"installNetworkObservability,omitempty"`🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // NetworkStatus is the current network configuration. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Disable" is misleading, because it is not disabling Network Observability; it does nothing. In the Enhancement Proposal, it uses the term "not enable" to mean it doesn't enable Network Observability.
What about using the values "true", "false" and ""? "false" would imply that it would not install Network Observability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We strongly discourage the use of booleans in OpenShift APIs because fields that start as booleans often evolve to need more than
true || falseas allowed values.We prefer the use of enums that have meaningful values to end-users. I agree that
installNetworkObservability: Enable | Disableis not very clear to an end user.I did a quick read of the EP - do you envision there ever needing to be a way for customers to apply any custom configuration for the day 0 network observability enablement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choosing common 'boolean' values like true/false, yes/no, and 0/1 is easier to remember than some arbitrary dual values like enable/disable. If I try to come up with another term besides "disable", they all sound bad like "null" or "no-op". I like yes/no as well, but it will have the same concerns you raised.
For day 0, the answer is no. Customers will likely need to make changes on day 2. Even if that were to change somewhere down the line, we would still want this knob to enable it separately. That's already the case for many features, which typically have an attribute named
enableunder the particular feature.The bottom line is whether the attribute and the value are meaningful to the user, not the just the value itself. enable/disable by itself is no more clearer than true/false. It has to be associated with the attribute
installNetworkObservabilityto determine whether it's clear or not. So ask yourself, "Is it clear whatinstallNetworkObservabilitymeans when it is true or false?" My answer is yes, and that's because the attribute has a verb in there. If the attribute wasNetworkObservability, then the values likeenableis much better thantrue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. The reason I asked is because if we were intending to enable some kind of custom configuration as part of day 0 installation, a simple enablement field may result in a slightly "clunkier" configuration experience in the future by having separate fields for stating installation intent and then custom configuration.
If you were intending to do day 0 custom configuration, I'd have suggested a structure more along the lines of:
(note: naming is just for an example and not necessarily exactly what I would expect).
I also think that having a nested structure now is generally better for future flexibility rather than locking yourself into having multiple co-located
*NetworkObservabilityfields in the future. In my experience, that starts to get confusing for end-users.Sure. So what happens if in the future you'd like to add a different way to specify the installation of the network observability feature beyond pure yes/no semantics?
Something like:
gives you more flexibility in the future to add new values that are descriptive beyond
True | False.All this being said, if you folks feel strongly that true/false semantics provide the best experience for the end-user then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the comments. There are no plans to extend this, because the thinking is that if you want do anything else, it should be done in the Network Observability Operator (NOO) on day 2.
To muddy the waters a bit more, I'm not that thrilled with the attribute name
installNetworkObservability, because it doesn't just install NOO, it also "enables" it by configuring the custom resource (CR). Here are a few more suggestions: