CNTRLPLANE-4014,CNTRLPLANE-4020: Add per-component operator log level configuration - #8878
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@dhgautam99: 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
needs-rebase
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe API adds Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant KASDeploymentController
participant LogLevelToKlogVerbosity
participant KubeAPIServerContainer
HostedControlPlane->>KASDeploymentController: provide operator config or annotation
KASDeploymentController->>LogLevelToKlogVerbosity: map LogLevel to verbosity
LogLevelToKlogVerbosity-->>KASDeploymentController: return verbosity
KASDeploymentController->>KASDeploymentController: parse annotation or use default 2
KASDeploymentController->>KubeAPIServerContainer: set --v argument
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
dhgautam99
changed the title
|
@dhgautam99: This pull request references CNTRLPLANE-3300 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 story 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. |
48f74b4 to
fefb0fd
Compare
openshift-ci
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/hypershift/v1beta1/hostedcluster_types.go (1)
2634-2637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider value type +
omitzeroinstead of pointer forKubeAPIServer.
ComponentLogLevelSpechas only an optionalLogLevelfield with a kubebuilder default (Normal), sonilvs. a zero-value struct carry no distinct meaning here — this is the exact case the API guidelines call out for preferring a non-pointer struct withomitzeroover a pointer withomitempty. Note thatMonitoringSpecin this same file already follows the value+omitzeropattern for a newly-added optional struct field.♻️ Suggested change
- // kubeAPIServer configures the log verbosity of the kube-apiserver component. - // +optional - KubeAPIServer *ComponentLogLevelSpec `json:"kubeAPIServer,omitempty"` + // kubeAPIServer configures the log verbosity of the kube-apiserver component. + // +optional + KubeAPIServer ComponentLogLevelSpec `json:"kubeAPIServer,omitzero"`Note the sibling fields
ClusterVersionOperator,ClusterNetworkOperator, andIngressOperatorin this same struct use the pointer+omitemptypattern already, so this isn't a new inconsistency, but per the API guidelines new additions should prefer the value+omitzeroform.As per coding guidelines, "Use
omitemptyfor scalar fields, slices, and maps, and useomitzerofor struct fields becauseomitemptydoes not omit non-pointer structs" and "Use pointers in CRD types only when the zero value is a valid, distinct user choice; do not use pointers just to model required nested structs."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/hostedcluster_types.go` around lines 2634 - 2637, The KubeAPIServer field is modeled as a pointer with omitempty even though ComponentLogLevelSpec is just an optional struct with a defaulted LogLevel, so nil and an empty struct are not meaningfully different. Update the HostedCluster spec field to use a value type instead of a pointer and switch the JSON tag to omitzero, following the same pattern used by MonitoringSpec. Keep the existing sibling fields like ClusterVersionOperator and IngressOperator unchanged; only adjust KubeAPIServer in hostedcluster_types.go.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/hypershift/v1beta1/hostedcluster_types.go`:
- Around line 2634-2637: The KubeAPIServer field is modeled as a pointer with
omitempty even though ComponentLogLevelSpec is just an optional struct with a
defaulted LogLevel, so nil and an empty struct are not meaningfully different.
Update the HostedCluster spec field to use a value type instead of a pointer and
switch the JSON tag to omitzero, following the same pattern used by
MonitoringSpec. Keep the existing sibling fields like ClusterVersionOperator and
IngressOperator unchanged; only adjust KubeAPIServer in hostedcluster_types.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4fdd3da5-5ded-43a8-8f22-55b1625cddc4
⛔ Files ignored due to path filters (37)
api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterUpdateAcceptRisks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ClusterVersionOperatorConfiguration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDC.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ExternalOIDCWithUpstreamParity.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/GCPPlatform.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HCPEtcdBackup.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/HyperShiftOnlyDynamicResourceAllocation.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/ImageStreamImportMode.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/KMSEncryptionProvider.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/OpenStack.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedcontrolplanes.hypershift.openshift.io/TLSAdherence.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**client/applyconfiguration/hypershift/v1beta1/componentloglevelspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/operatorconfiguration.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.operatorconfiguration.testsuite.yamlis excluded by!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yamlcmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedcontrolplanes-Hypershift-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/**,!cmd/install/assets/**/*.yaml
📒 Files selected for processing (4)
api/hypershift/v1beta1/hostedcluster_types.goapi/hypershift/v1beta1/operator.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment_test.go
💤 Files with no reviewable changes (2)
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment.go
- control-plane-operator/controllers/hostedcontrolplane/v2/kas/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/hypershift/v1beta1/operator.go
Codecov Report❌ Patch coverage is Please upload reports for the commit d01f5a2 to get more accurate results.
Additional details and impacted files@@ Coverage Diff @@
## main #8878 +/- ##
==========================================
- Coverage 46.64% 46.25% -0.40%
==========================================
Files 784 785 +1
Lines 98880 98512 -368
==========================================
- Hits 46123 45563 -560
- Misses 49628 49864 +236
+ Partials 3129 3085 -44
... and 39 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I now have complete understanding of both lint errors. Let me verify my understanding by cross-referencing the exact error messages:
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseTwo 1. The Fix: Change 2. The Fix: Replace Recommendations
Evidence
|
fefb0fd to
d7859b5
Compare
openshift-ci
Bot
added
area/platform/aws
|
/hold Revision 468531a was retested 3 times: holding |
|
/unhold |
|
/hold Revision 468531a was retested 3 times: holding |
|
/unhold |
|
/test e2e-v2-azure-self-managed |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
…og levels Introduce OperatorConfiguration in the HostedCluster API so operators can set per-component log verbosity for the control plane components, replacing hardcoded verbosity flags. - Add ComponentLogLevelSpec and LogLevel (Normal/Debug/Trace/TraceAll) with CEL validation and godocs. etcd log levels are restricted to Normal and Debug. - Wire log-level configuration through kube-apiserver, kube-controller-manager, kube-scheduler, openshift-apiserver, openshift-controller-manager, openshift-oauth-apiserver, oauth-server, and etcd deployments. - For kube-apiserver, fall back to the existing verbosity annotation when the API field is unset; the API field takes precedence when both are set. - Add unit tests covering log-level resolution for all components.
|
/verified by @dhgautam99 as per #8878 comment |
|
@dhgautam99: This PR has been marked as verified by 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. |
|
/lgtm |
|
Scheduling tests matching the |
|
/hold Revision d01f5a2 was retested 3 times: holding |
|
/unhold |
|
/retest |
|
/test verify-deps |
|
/test e2e-v2-azure-self-managed |
|
@dhgautam99: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
13 participants
Summary
Adds per-component operator log level configuration for hosted control plane components, gated behind the
HCPUserFacingOperatorLogsfeature gate (TechPreviewNoUpgrade).LogLevelenum (Normal,Debug,Trace,TraceAll) andComponentLogLevelSpectype in the APIOperatorConfigurationfields for 8 components: kube-apiserver, etcd, kube-controller-manager, kube-scheduler, openshift-controller-manager, openshift-apiserver, openshift-oauth-apiserver, and oauth-serverLogLevelto klog verbosity (--v=) or etcd log level--vflag use the(int, bool)return pattern —--vis only injected when LogLevel is explicitly set, preserving existing defaultsOperatorConfigurationfield takes precedence over the legacyhypershift.openshift.io/kube-api-server-verboseannotationLogLevelis a non-pointer value type withomitempty; "no opinion" is expressed by omitting the field--v=2from kube-scheduler and oauth-apiserver static manifests, replacing with dynamic resolutionComponents
int(always returns fallback 2)--v=2(preserves annotation behavior)!= ""check(int, bool)--vinjected(int, bool)--vinjected(int, bool)--vinjected(int, bool)--vinjected(int, bool)--vinjected(int, bool)--vinjectedTest plan
LogLevelToKlogVerbosityandLogLevelToEtcdLevel(support/util/loglevel_test.go)operatorConfigurationCRD validationmake verifypassesmake test-envtest-ocppasses (K8s 1.35)Which issue(s) this PR fixes
Fixes CNTRLPLANE-3300