Feature OCPSTRAT-3156: Configurable log levels for hosted control plane components#2038
Feature OCPSTRAT-3156: Configurable log levels for hosted control plane components#2038rutvik23 wants to merge 1 commit into
Conversation
Introduces an enhancement proposal for structured, per-component log level configuration (Normal/Debug/Trace/TraceAll) for hosted control plane components managed by CPO via the HostedCluster CR. Covers kube-apiserver (Phase 1), 7 remaining core components (Phase 2), and documents holistic scope for all CPO-managed components (Phase 3). Replaces the existing ad-hoc kube-apiserver-verbosity-level annotation with a validated, structured API using ComponentLogLevelSpec pointer fields in OperatorConfiguration. Tracking: https://issues.redhat.com/browse/OCPSTRAT-3156 RFE: https://issues.redhat.com/browse/RFE-7777
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@rutvik23: This pull request references OCPSTRAT-3156 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| as structured `*ComponentLogLevelSpec` pointers in the existing `OperatorConfiguration` | ||
| type. The design is intentionally generic to extend to all remaining CPO-managed components | ||
| in future phases. This feature ships ungated (no feature gate) as a low-risk operational | ||
| knob that extends an already-GA framework. |
There was a problem hiding this comment.
The existing ClusterVersionOperator field in OperatorConfiguration IS gated behind +openshift:enable:FeatureGate=ClusterVersionOperatorConfiguration. The API review guide states: "New fields in existing APIs should always be gated." Skipping the gate here is inconsistent with the precedent set by the only other log-level field in this same struct. What is the justification for treating these 8 new fields differently from the CVO field that lives right next to them?
There was a problem hiding this comment.
Good point. My understanding is that the ClusterVersionOperator field was was introduced as part of the CVO Log Level API
enhancement, which introduced a whole new CRD, and setting the precedence of operatorConfiguration.
So I can imagine that not gating this field can be justified by:
- This enhancement basically extends that precedence.
- The change is a minimal change both operationally and fills a HyperShift API gap for standalone OCP customers.
I can also see the advocacy for gating it, in case there're concerns about any undesired operational side effect.
@enxebre @JoelSpeed would like to know your thoughts about this.
There was a problem hiding this comment.
Eventually I would like to see all API additions to HyperShift gated, in the same way they are in OpenShift. At the moment however, I don't think we have the same tooling in place to be able to follow the same feature gating process.
We can gate the field, and we can generate the CRDs/only deploy the field during a subset of deployments opted into tech preview
What we don't have is the testing framework in place to verify that a feature is stable and ready to be promoted like we do in OCP.
I think there's still merit in gating the fields and getting into the habit of testing them under techpreview first (esp since it prevents the field shipping in ROSA/ARO), but the "is this ready for promotion" is a much more subjective and manual decisions presently
| // +kubebuilder:default=Normal | ||
| // +kubebuilder:validation:Enum="";Normal;Debug;Trace;TraceAll | ||
| LogLevel LogLevel `json:"logLevel,omitempty"` | ||
| } |
There was a problem hiding this comment.
Several issues with this type:
-
Static default on a configuration API.
+kubebuilder:default=Normalpersists the default at admission time. Per OpenShift API conventions, configuration APIs should default in the controller, not the schema, so the default can evolve over time. IfNormalis intended to be permanent, document that explicitly. Otherwise, remove the marker and default in the CPO, with Godoc: "When omitted, this means the user has no opinion and the platform defaults to Normal, which is subject to change over time." -
Empty string in enum is ambiguous. The enum includes
""alongside+kubebuilder:default=Normal. If a user submitskubeAPIServer: {}, the field defaults to"Normal". If they explicitly submitlogLevel: "", does that differ from omitting it? The OpenShift convention is: "Only allow the zero value when it is semantically important and means something different to being omitted." What is the semantic meaning of""here? -
Single-field wrapper struct. If
ComponentLogLevelSpecwill only ever containlogLevel, consider using*LogLeveldirectly on theOperatorConfigurationfields (e.g.,KubeAPIServerLogLevel *LogLevel). This is simpler for users (kubeAPIServerLogLevel: DebugvskubeAPIServer: {logLevel: Debug}) and avoids the extensibility trap described below.
| // oAuthServer configures the log verbosity of the oauth-server component. | ||
| // +optional | ||
| OAuthServer *ComponentLogLevelSpec `json:"oAuthServer,omitempty"` | ||
| } |
There was a problem hiding this comment.
These field names (kubeAPIServer, etcd, openShiftAPIServer, etc.) are generic, implying they are the full configuration entry point for that component. But the type is *ComponentLogLevelSpec, which only carries a log level.
If any of these 8 components ever needs additional per-component configuration (resource overrides, feature flags, etc.), you cannot change the type from *ComponentLogLevelSpec to a dedicated struct without breaking structured clients (serialization changes are forbidden post-merge per API review policy).
You already acknowledge this problem for CVO/CNO/Ingress in the Phase 3 note (line 444-454), where those components get LogLevel embedded in their existing dedicated typed structs. That is the correct pattern. Consider one of:
- Use dedicated per-component types from the start (
KubeAPIServerConfig,EtcdConfig, etc.) each with alogLevelfield. More boilerplate now, but future-proof. - Or rename the fields to be specific:
kubeAPIServerLogLevel *LogLevel,etcdLogLevel *LogLevel, etc. Simpler and leaves namespace for futurekubeAPIServer *KubeAPIServerConfigfields.
There was a problem hiding this comment.
I concur that ComponentLogLevelSpec isn't the suitable name for this type.
Now whether to put the per-operator log level directly under operatorConfiguration (e.g. operatorConfiguration.kubeAPIServerLogLevel or nested under an operator dedicated field similar to what's being proposed here is definitely a trade-off.
On one aspect, we don't wanna have a single field wrapper struct for no reason, but on another:
- This design follows the already established precedence by the
clusterVersionOperator. - We try now to really optimize for future extensibility as a learned lesson. The HCP API has technical debt in many places in the form of specific knobs added at a coarse grain level which ended up being inconsistent once extra relevant knobs needed to be added and as you know, the API support guarantees are very aggressive.
So having a per operator subfield of operatorConfiguration would be the option I'd go with here
| type OperatorConfiguration struct { | ||
| // ...existing ClusterVersionOperator, ClusterNetworkOperator, IngressOperator fields... | ||
|
|
||
| // kubeAPIServer configures the log verbosity of the kube-apiserver component. |
There was a problem hiding this comment.
The Godoc should describe user-observable behavior per OpenShift API conventions. Currently it says "configures the log verbosity," which is implementation-oriented. Consider adding:
- What happens when the field is set (rolling restart of the component, increased log volume)
- What happens when the field is omitted: "When omitted, this means the user has no opinion and the platform defaults to Normal verbosity."
The existing clusterVersionOperator field's Godoc in OperatorConfiguration follows this pattern with behavioral documentation on operatorLogLevel.
|
|
||
| For managed services (ROSA HCP, ARO HCP), service-level guardrails may be needed to | ||
| restrict `TraceAll` log level, as glog level 8 can dump sensitive data including request | ||
| bodies and secrets in component logs. |
There was a problem hiding this comment.
"May be needed" is too weak for a GA feature. TraceAll at glog level 8 dumps request bodies including secrets. On ROSA HCP, those logs flow to CloudWatch, which is customer-visible. This is a concrete data-leak vector, not a hypothetical.
Before GA, the enhancement should commit to one of:
- Block
TraceAllon managed service clusters (admission webhook or CEL) - Allow it but ensure logs containing secrets are not forwarded to customer-visible sinks
- Accept the risk with explicit documented warnings
Deferring the decision to "be decided with PM" (Open Question 1) is not acceptable for a feature shipping GA without a feature gate.
| **Risk:** `TraceAll` on managed services (ROSA HCP, ARO HCP) could expose secrets in logs | ||
| flowing to CloudWatch or Azure Monitor (customer-visible). | ||
| **Mitigation:** Add admission policy or CEL rule blocking `TraceAll` on managed service | ||
| clusters. Access model (customer self-service vs SRE/CEE only) to be decided with PM. |
There was a problem hiding this comment.
This mitigation is stated in imperative ("Add admission policy") but is not listed in the GA Criteria (lines 583-598) or the Phase 1/Phase 2 deliverables. Is this a commitment or a suggestion? If it's required for GA, add it to the GA Criteria. If it's deferred, say so explicitly and explain why shipping GA without it is acceptable given the security implications.
| **Risk:** Rolling restarts during log level changes could cause brief API unavailability if | ||
| multiple components are changed simultaneously. | ||
| **Mitigation:** The CPO should apply changes sequentially across components to avoid | ||
| simultaneous restarts. |
There was a problem hiding this comment.
"Should" is RFC 2119 advisory, not mandatory. If a user patches all 8 components in one oc patch, will the CPO actually serialize the rollouts? Or will it update all deployments in a single reconciliation pass? This matters for the HA guarantee. If sequential application is required for safety, state it as "must" and describe the implementation mechanism (e.g., reconcile one component per pass, proceed only after rollout completes).
|
|
||
| The existing `hypershift.openshift.io/kube-apiserver-verbosity-level` annotation is | ||
| deprecated on merge of this feature and will be removed in N+2 (two minor releases after | ||
| GA). |
There was a problem hiding this comment.
"N+2" is ambiguous since this EP doesn't specify what "N" is and the Jira target version is not set (the CI bot flagged this). Use concrete OCP versions (e.g., "deprecated in 4.19, removed in 4.21") so readers don't have to cross-reference the target release.
| with elevated verbosity. Administrators and SREs can alert on this to track clusters with | ||
| non-standard log volume. | ||
| - `HostedCluster` condition `NonDefaultLogLevel=False` — all components running at default | ||
| verbosity. |
There was a problem hiding this comment.
The condition is described but missing key details:
-
What
Reasonvalues are expected? Per Kubernetes API conventions,Reasonis required and should be a CamelCase programmatic identifier. Define the expected values (e.g.,ElevatedVerbosity,AllDefault). -
The condition message should enumerate which components have non-default levels.
NonDefaultLogLevel=Truewithout component detail forces SREs to inspect the full spec to find which component is elevated. Include something like "kube-apiserver: Debug, etcd: Trace" in the message.
| > via a shell one-liner (`/bin/sh -c "... /usr/bin/etcd"`). Injecting a CLI flag means | ||
| > string-manipulating the shell command — fragile and breaks if the command format changes. | ||
| > `ETCD_LOG_LEVEL` matches all 15 existing etcd config values and uses the existing | ||
| > `util.UpsertEnvVar()` utility (from `support/util/containers.go`). etcd internally maps |
There was a problem hiding this comment.
Minor: the file path is incorrect. UpsertEnvVar lives in support/podspec/containers.go, not support/util/containers.go. There is no containers.go in support/util/.
devguyio
left a comment
devguyio
left a comment
There was a problem hiding this comment.
I know this enhancement is still a WIP, but I left a few comments.
| as structured `*ComponentLogLevelSpec` pointers in the existing `OperatorConfiguration` | ||
| type. The design is intentionally generic to extend to all remaining CPO-managed components | ||
| in future phases. This feature ships ungated (no feature gate) as a low-risk operational | ||
| knob that extends an already-GA framework. |
There was a problem hiding this comment.
Good point. My understanding is that the ClusterVersionOperator field was was introduced as part of the CVO Log Level API
enhancement, which introduced a whole new CRD, and setting the precedence of operatorConfiguration.
So I can imagine that not gating this field can be justified by:
- This enhancement basically extends that precedence.
- The change is a minimal change both operationally and fills a HyperShift API gap for standalone OCP customers.
I can also see the advocacy for gating it, in case there're concerns about any undesired operational side effect.
@enxebre @JoelSpeed would like to know your thoughts about this.
| 1. **Configuring all ~50 CPO-managed components in Phases 1 and 2.** Phases 1 and 2 cover | ||
| the 8 core control plane components. Phase 3 (future) covers remaining components. | ||
|
|
||
| 2. **The following components are permanently out of scope** due to their logging model not |
There was a problem hiding this comment.
I'm not sure about using the word "permanently" is correct here. It implies that something will never be considered in the future.
| | Trace | 6 | debug | Deep investigation | | ||
| | TraceAll | 8 | debug | Full dumps — perf impact, may expose secrets | | ||
|
|
||
| The chosen API design is **Option B — Structured fields** in `OperatorConfiguration`. This |
There was a problem hiding this comment.
I'm not sure what are you referring to as Option B?
| > **Why etcd uses `ETCD_LOG_LEVEL` env var and not `--log-level` flag:** etcd is started | ||
| > via a shell one-liner (`/bin/sh -c "... /usr/bin/etcd"`). Injecting a CLI flag means | ||
| > string-manipulating the shell command — fragile and breaks if the command format changes. | ||
| > `ETCD_LOG_LEVEL` matches all 15 existing etcd config values and uses the existing | ||
| > `util.UpsertEnvVar()` utility (from `support/util/containers.go`). etcd internally maps | ||
| > `ETCD_*` env vars to their `--*` flag equivalents. | ||
|
|
||
| > **Why Trace and TraceAll both map to `debug` for etcd:** etcd uses zap, which supports: | ||
| > debug, info, warn, error, panic, fatal. There is no finer granularity below debug. klog | ||
| > has numeric levels (2/4/6/8) giving four distinct settings; etcd only gives two useful | ||
| > ones (info and debug). |
There was a problem hiding this comment.
Does this section belong to the enhancement?
| // oAuthServer configures the log verbosity of the oauth-server component. | ||
| // +optional | ||
| OAuthServer *ComponentLogLevelSpec `json:"oAuthServer,omitempty"` | ||
| } |
There was a problem hiding this comment.
I concur that ComponentLogLevelSpec isn't the suitable name for this type.
Now whether to put the per-operator log level directly under operatorConfiguration (e.g. operatorConfiguration.kubeAPIServerLogLevel or nested under an operator dedicated field similar to what's being proposed here is definitely a trade-off.
On one aspect, we don't wanna have a single field wrapper struct for no reason, but on another:
- This design follows the already established precedence by the
clusterVersionOperator. - We try now to really optimize for future extensibility as a learned lesson. The HCP API has technical debt in many places in the form of specific knobs added at a coarse grain level which ended up being inconsistent once extra relevant knobs needed to be added and as you know, the API support guarantees are very aggressive.
So having a per operator subfield of operatorConfiguration would be the option I'd go with here
| |---|---|---|---| | ||
| | **Phase 1** | kube-apiserver | klog (`--v=N`) | 1 | | ||
| | **Phase 2** | kube-controller-manager, kube-scheduler, etcd, openshift-apiserver, openshift-controller-manager, openshift-oauth-apiserver, oauth-server | klog (`--v=N`) / zap (`ETCD_LOG_LEVEL` env var for etcd) | 7 | | ||
| | **Phase 3 — klog-based managed components** | cluster-network-operator, cluster-version-operator, cluster-autoscaler, cluster-image-registry-operator, cluster-storage-operator, cluster-node-tuning-operator, cluster-policy-controller, csi-snapshot-controller-operator, dns-operator, ingress-operator, cloud-credential-operator, capi-provider, capi-manager, cluster-api, kubevirt-cloud-controller-manager, gcp-cloud-controller-manager, powervs-cloud-controller-manager, openstack-cloud-controller-manager, aws-cloud-controller-manager, azure-cloud-controller-manager, kubevirt-csi-controller, machine-approver, karpenter-operator, konnectivity-agent, olm-operator, catalog-operator, packageserver, openshift-route-controller-manager, endpoint-resolver | klog (`--v=N`); same `LogLevelToKlogVerbosity()` utility as Phases 1 & 2 | ~29 | |
There was a problem hiding this comment.
I don't think phase 3 should be a phase in this plan. The enhancement is focused more on the more user-facing operator which already have a lot of demand from customers to manage similar to how they do in standalone OCP.
We can find UX mitigation in the future as long as the design allows it, such as having a default log level that can be overridden per operator.
| // kubeAPIServer configures the log verbosity of the kube-apiserver component. | ||
| // +optional | ||
| KubeAPIServer *ComponentLogLevelSpec `json:"kubeAPIServer,omitempty"` | ||
|
|
There was a problem hiding this comment.
I wonder how we want to model this. OperatorConfiguration is currently a shim for curated opertator.openshift.io knobs.
This breaks the pattern, maybe have a controlPlaneOperator knob that contains the components, e.g. OperatorConfiguration.controlPlaneOperator.KubeAPIServer.logSpec.level.
Another alternative is to not put this in OperatorConfiguration at all.
cc @JoelSpeed @csrwng
There was a problem hiding this comment.
nvm, these are existing operator.openshift.io crds, so that makes sense. The whole OperatorConfiguration should read as "information to configure an operator to manage X"
6 participants
Why
HyperShift administrators and support engineers cannot adjust log
verbosity for hosted control plane components during troubleshooting.
Standard OCP has this via operatorv1.LogLevel, but HyperShift has no
equivalent except an ad-hoc, unvalidated annotation for kube-apiserver.
This was reported as RFE-7777 and accepted as crucial for minimal
troubleshooting.
What
Enhancement proposal introducing structured per-component log level
configuration for CPO-managed components via the HostedCluster CR:
ComponentLogLevelSpectype withNormal/Debug/Trace/TraceAllenum (matching operatorv1.LogLevel)
OperatorConfigurationfor 8 corecontrol plane components
(Phase 2), all remaining CPO-managed components documented
holistically (Phase 3)
annotation
References