CNTRLPLANE-3526: Add spec.monitoring API for metrics forwarding#8626
CNTRLPLANE-3526: Add spec.monitoring API for metrics forwarding#8626muraee wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@muraee: This pull request explicitly references no jira issue. 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
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughThis PR migrates metrics forwarding configuration from annotation-based to spec-based control. It adds MonitoringSpec, MetricsForwardingSpec, and enums (MetricsForwardingMode, MetricsSet), copies HostedCluster.Spec.Monitoring to HostedControlPlane.Spec.Monitoring with a backward-compatibility shim for the deprecated annotation, computes an effective metrics set for SRE reconciliation, updates controller predicates and gating to use the spec field, and updates unit and e2e tests to drive behavior via the new spec. Sequence Diagram(s)sequenceDiagram
participant User
participant HostedClusterController
participant HostedCluster
participant HostedControlPlane
participant ControlPlaneOperator
participant MetricsForwarder
User->>HostedCluster: set spec.monitoring.metricsForwarding.mode=Enabled
HostedClusterController->>HostedCluster: read Spec.Monitoring
HostedClusterController->>HostedControlPlane: copy Spec.Monitoring (apply deprecated-annotation shim if unset)
ControlPlaneOperator->>HostedControlPlane: read Spec.Monitoring
ControlPlaneOperator->>MetricsForwarder: enable/disable based on MetricsForwarding.Mode and MetricsSet
MetricsForwarder-->>ControlPlaneOperator: status
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
muraee
changed the title
|
@muraee: This pull request references CNTRLPLANE-3526 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: muraee 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 |
openshift-ci
Bot
added
area/api
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@api/hypershift/v1beta1/hosted_controlplane.go`:
- Around line 190-195: The Monitoring field in HostedControlPlane currently has
an inconsistent JSON tag; locate the Monitoring declaration (Monitoring
MonitoringSpec) and replace the tag string that contains both
"omitempty,omitzero" with the single "omitzero" form (i.e.,
json:"monitoring,omitzero") so it matches the style used by HostedClusterSpec
and other fields like AutoNode.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 1189-1192: The code reads
hcp.Spec.Monitoring.MetricsForwarding.MetricsSet without nil checks which can
panic; update the logic around effectiveMetricsSet (and keep r.MetricsSet
fallback) to first verify hcp.Spec.Monitoring != nil and
hcp.Spec.Monitoring.MetricsForwarding != nil before accessing MetricsSet, and
only override effectiveMetricsSet when those pointers are non-nil and MetricsSet
is non-empty (preserve existing behavior of using metrics.MetricsSet(...) when
present).
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.go`:
- Around line 46-49: The predicate function dereferences
cpContext.HCP.Spec.Monitoring.MetricsForwarding.Mode without nil checks which
can panic when Monitoring or MetricsForwarding are nil; update
predicate(cpContext component.WorkloadContext) to first check cpContext.HCP,
cpContext.HCP.Spec, cpContext.HCP.Spec.Monitoring and
cpContext.HCP.Spec.Monitoring.MetricsForwarding for nil before reading Mode and
combine that guarded check with the existing DisableMonitoringServices
annotation check (hyperv1.DisableMonitoringServices) so the function returns
false (no metrics forwarding) when any of the intermediate structs are nil and
only true when Mode == hyperv1.MetricsForwardingModeEnabled and monitoring is
not disabled.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/component.go`:
- Around line 69-72: The predicate function reads
cpContext.HCP.Spec.Monitoring.MetricsForwarding.Mode without nil guards; add
checks in predicate to ensure cpContext.HCP, cpContext.HCP.Spec, and
cpContext.HCP.Spec.Monitoring are non-nil (and that Monitoring.MetricsForwarding
is present) before accessing Mode, and return false (no reconcile) if any are
nil; keep the existing DisableMonitoringServices annotation check
(hyperv1.DisableMonitoringServices) and only evaluate Mode when the monitoring
structs exist to avoid nil-pointer panics.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 2508-2511: The current fallback flips any non-Enabled mode
(including an explicit Disabled) to Enabled when the deprecated annotation
exists; change the condition to only apply the annotation fallback when the HCP
mode is not explicitly set (e.g., empty/unspecified) rather than any mode other
than Enabled. Concretely, update the check around
hcp.Spec.Monitoring.MetricsForwarding.Mode so it only sets
hyperv1.MetricsForwardingModeEnabled from the deprecated
hcluster.Annotations[hyperv1.EnableMetricsForwarding] when the existing mode is
the unset/zero value (not when it equals hyperv1.MetricsForwardingModeDisabled
or any explicit value).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9bad19ca-0e30-40f1-88ee-d8aca6240665
⛔ Files ignored due to path filters (44)
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/hostedclusterspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanespec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/metricsforwardingspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/monitoringspec.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.monitoring.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/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (11)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/component.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gotest/e2e/util/util_metrics_proxy.gotest/e2e/v2/tests/hosted_cluster_metrics_test.go
muraee
force-pushed
the
metrics-forwarding-api
branch
from
996709e to
89814ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 951-952: The code directly reads
hcp.Spec.Monitoring.MetricsForwarding.Mode which can panic when Monitoring or
MetricsForwarding is nil; update the conditional to first nil-check
hcp.Spec.Monitoring and hcp.Spec.Monitoring.MetricsForwarding and only compare
Mode to hyperv1.MetricsForwardingModeEnabled when both are non-nil, otherwise
treat it as not enabled and call return k8sutil.DeleteAllIfNeeded(ctx, r.client,
deployment, cm, servingCA, podMonitor); ensure you reference the same symbols
(hcp, Spec, Monitoring, MetricsForwarding, Mode,
hyperv1.MetricsForwardingModeEnabled) so the branch exactly mirrors the intended
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9bc66ee1-87cc-4ca1-8f51-fda6d9f9d236
⛔ Files ignored due to path filters (44)
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/hostedclusterspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/hostedcontrolplanespec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/metricsforwardingspec.gois excluded by!client/**client/applyconfiguration/hypershift/v1beta1/monitoringspec.gois excluded by!client/**client/applyconfiguration/utils.gois excluded by!client/**cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.monitoring.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/**/*.yamldocs/content/reference/aggregated-docs.mdis excluded by!docs/content/reference/aggregated-docs.mddocs/content/reference/api.mdis excluded by!docs/content/reference/api.mdvendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (11)
api/hypershift/v1beta1/hosted_controlplane.goapi/hypershift/v1beta1/hostedcluster_types.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/component.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gotest/e2e/util/util_metrics_proxy.gotest/e2e/v2/tests/hosted_cluster_metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.go
- test/e2e/v2/tests/hosted_cluster_metrics_test.go
- api/hypershift/v1beta1/hosted_controlplane.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- test/e2e/util/util_metrics_proxy.go
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8626 +/- ##
==========================================
+ Coverage 41.49% 41.56% +0.07%
==========================================
Files 756 758 +2
Lines 93648 93849 +201
==========================================
+ Hits 38855 39007 +152
- Misses 52057 52093 +36
- Partials 2736 2749 +13
... and 14 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:
|
|
/retest |
muraee
force-pushed
the
metrics-forwarding-api
branch
from
89814ff to
94e176b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go (1)
616-690: ⚡ Quick winAdd a backward-compat test for
metricsSet+ deprecated annotation together.You already test
metricsSetalone and annotation-only fallback, but not the combined case. Add one case wheremonitoring.metricsSetis set andhyperv1.EnableMetricsForwardingis present, and assert bothMetricsSetis preserved andModebecomesEnabled.🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go` around lines 616 - 690, Add a new test case in the tests slice that sets monitoring.MetricsSet (e.g. hyperv1.MetricsSetAll) and also includes the deprecated annotation hyperv1.EnableMetricsForwarding: "true"; set expectedMonitoring to preserve MetricsSet and include MetricsForwarding.Mode: hyperv1.MetricsForwardingModeEnabled so the test verifies that MetricsSet is preserved and Mode becomes Enabled when the annotation is present (refer to the tests variable, monitoring, annotations, expectedMonitoring, MetricsSet and MetricsForwarding.Mode symbols).
🤖 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
`@hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go`:
- Around line 616-690: Add a new test case in the tests slice that sets
monitoring.MetricsSet (e.g. hyperv1.MetricsSetAll) and also includes the
deprecated annotation hyperv1.EnableMetricsForwarding: "true"; set
expectedMonitoring to preserve MetricsSet and include MetricsForwarding.Mode:
hyperv1.MetricsForwardingModeEnabled so the test verifies that MetricsSet is
preserved and Mode becomes Enabled when the annotation is present (refer to the
tests variable, monitoring, annotations, expectedMonitoring, MetricsSet and
MetricsForwarding.Mode symbols).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e1f7770b-5abc-49f8-b1c8-d2d155d6fd33
⛔ Files ignored due to path filters (1)
cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/stable.hostedclusters.monitoring.testsuite.yamlis excluded by!cmd/install/assets/**/*.yaml
📒 Files selected for processing (10)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/metrics_proxy/component.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gotest/e2e/util/util_metrics_proxy.gotest/e2e/v2/tests/hosted_cluster_metrics_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/v2/tests/hosted_cluster_metrics_test.go
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component.go
- test/e2e/util/util_metrics_proxy.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
- control-plane-operator/controllers/hostedcontrolplane/v2/endpoint_resolver/component_test.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
|
/retest |
|
/test e2e-aws |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Some general godoc suggestions per openshift/api conventions
muraee
force-pushed
the
metrics-forwarding-api
branch
from
39a06cf to
b839f14
Compare
muraee
force-pushed
the
metrics-forwarding-api
branch
from
b839f14 to
a9ebe53
Compare
| // monitoring configures monitoring for the hosted cluster, including | ||
| // forwarding of control plane metrics to the hosted cluster's monitoring stack. | ||
| // When omitted, metrics forwarding behavior is determined by the | ||
| // hypershift.openshift.io/enable-metrics-forwarding annotation for backward compatibility. | ||
| // If neither is set, metrics forwarding is disabled. | ||
| // | ||
| // +optional | ||
| Monitoring MonitoringSpec `json:"monitoring,omitzero"` |
There was a problem hiding this comment.
In standalone OpenShift, we use feature-gates to ensure that we don't ship new APIs/fields as GA until we are confident that they are working as expected.
I believe HyperShift has a way to generate APIs following the same pattern but requires manually setting the feature gate enablement state in a static feature gates file.
Both the hypershift operator and control plane operator should also have a way to configure feature gates to be checked in their controllers.
It might be worth considering feature gating this functionality and going through a similar promotion process as we use for standalone openshift features to ensure we have sufficient automated regression testing in place and that those tests have consistently passed for some amount of time.
There was a problem hiding this comment.
Thanks for the suggestion. In HyperShift, feature gates are typically reserved for new platforms (OpenStack, GCP), new CRD types (EtcdBackup), or major feature additions (ClusterVersionOperatorConfiguration) — not for small opt-in fields on existing types.
This field is also fully backward-compatible: omitting it preserves existing behavior via the annotation fallback. The feature itself (metrics forwarding) is already delivered and validated through e2e tests, which is the primary validation mechanism in HyperShift. Adding a feature gate here would add friction without meaningful risk reduction.
| // MetricsForwardingMode controls whether metrics forwarding is active for a hosted cluster. | ||
| // | ||
| // +kubebuilder:validation:Enum=Enabled;Disabled | ||
| type MetricsForwardingMode string | ||
|
|
||
| const ( | ||
| // MetricsForwardingModeEnabled indicates metrics forwarding is active. | ||
| MetricsForwardingModeEnabled MetricsForwardingMode = "Enabled" | ||
|
|
||
| // MetricsForwardingModeDisabled indicates metrics forwarding is inactive. | ||
| MetricsForwardingModeDisabled MetricsForwardingMode = "Disabled" | ||
| ) |
There was a problem hiding this comment.
In general, we discourage the usage of the terms Enabled / Disabled because they are often overloaded and can be more unclear to users as the enum values expand over time.
Maybe options like Forward and DoNotForward are more appropriately descriptive here?
There was a problem hiding this comment.
Good point. We'll rename to Forward / None — follows established Kubernetes API conventions (e.g., DNSPolicy: None) and reads naturally under metricsForwarding.mode.
| // metricsForwarding configures forwarding of control plane metrics into | ||
| // the hosted cluster's monitoring stack. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
What does it mean for this field to be omitted?
There was a problem hiding this comment.
Good catch — the godoc on this field didn't explain the omission behavior. Updated it to clarify: when omitted, metrics forwarding falls back to the hypershift.openshift.io/enable-metrics-forwarding annotation for backward compatibility; if neither is set, forwarding is disabled.
| // When not specified, the global METRICS_SET value is used, which defaults to "Telemetry". | ||
| // | ||
| // +optional | ||
| MetricsSet MetricsSet `json:"metricsSet,omitempty"` |
There was a problem hiding this comment.
Can I set this field if metricsForwarding.mode is set to Disabled?
There was a problem hiding this comment.
Yes — metricsSet is intentionally a sibling of metricsForwarding under monitoring, not nested inside it. The intent is that metricsSet applies broadly to monitoring (ServiceMonitors, PodMonitors, etc.) to override the global METRICS_SET env for individual HostedClusters, not just to forwarding. So setting it independently of metricsForwarding.mode is valid.
@csrwng can you confirm this is the expectation?
| MetricsForwarding MetricsForwardingSpec `json:"metricsForwarding,omitzero"` | ||
|
|
||
| // metricsSet specifies which set of metrics to collect and forward. | ||
| // Valid values are "Telemetry", "SRE", and "All". |
There was a problem hiding this comment.
It would be helpful for end-users that read the documentation generated from this GoDoc if you explain what happens when each of these values are set.
It might help explain to an end-user why they may want to configure this field to a specific value, which is currently unclear to me reviewing this change.
There was a problem hiding this comment.
Good point — expanded the godoc for each enum value to explain what it collects and when you'd use it.
Add MonitoringSpec and MetricsForwardingSpec types to HostedCluster and HostedControlPlane specs, replacing the annotation-based EnableMetricsForwarding mechanism with a proper API field. The new API adds: - monitoring.metricsForwarding.mode (Forward/None) to control metrics forwarding per cluster - monitoring.metricsSet (Telemetry/SRE/All) to override the global METRICS_SET environment variable per cluster Signed-off-by: Mulham Raee <mulham.raee@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generated by: make update Signed-off-by: Mulham Raee <mulham.raee@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
muraee
force-pushed
the
metrics-forwarding-api
branch
from
1f6d34e to
a8d1b05
Compare
muraee
force-pushed
the
metrics-forwarding-api
branch
from
a8d1b05 to
d60ff6f
Compare
Update all consumers to check the new spec field: - HC controller copies Monitoring from HC to HCP with backward compat for the deprecated EnableMetricsForwarding annotation (only when mode is unset; explicit Disabled takes precedence) - CPO resolves per-cluster MetricsSet override before SRE config loading and passes it through ControlPlaneContext - metrics-proxy and endpoint-resolver predicates check Mode enum - HCCO reconcileMetricsForwarder checks spec instead of annotation - HO SRE ConfigMap sync supports per-cluster MetricsSet=SRE Signed-off-by: Mulham Raee <mulham.raee@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
muraee
force-pushed
the
metrics-forwarding-api
branch
from
d60ff6f to
148f322
Compare
|
The Codecov bot comment gave me everything needed. Let me verify one detail about the exact coverage numbers from the check run output vs the comment. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe The 16 uncovered lines break down across four files: 1. The new backward-compatibility logic in 2. The 3. The predicate's updated return statement ( 4. One line in the refactored predicate is not reached by the new Recommendations
Implementing just recommendation (1) is likely the minimum fix to pass the gate — it would move coverage from 11/27 (40.74%) to 12/27 (44.4%), above the 41.49% target. All three should be implemented for proper engineering practice. Evidence
|
|
@muraee: 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. |
7 participants
Summary
spec.monitoring.metricsForwardingAPI on HostedCluster and HostedControlPlane, replacing the annotation-basedhypershift.openshift.io/enable-metrics-forwardingmechanismmetricsSetfield (Telemetry/SRE/All) that overrides the globalMETRICS_SETenv var on the HyperShift OperatorTest plan
TestPredicate)TestReconcileMetricsForwarder)make verifypasses (0 lint issues)make api-lint-fixpasses (0 issues)spec.monitoring.metricsForwarding.mode: Enabled🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests