OCPBUGS-20179: Stop defining time series for hosted clusters or node pools which do not exist anymore.

[Nikokolas3270]

What this PR does / why we need it:

hostedcluster and nodepool time series keep being defined long after the hostedcluster or nodepool is gone.

This is problematic because:

1. We plan to add a new hostedcluster metric which will "count" the number of worker cores and which will be used to bill the customers (see OSD-19085).
   This metric must stop to be defined as soon as the cluster is gone or we will continue to bill the customer for a cluster which is gone.
2. This is misleading.
3. This is kind of a leak; time series were defined forever... unless the hypershift operator was restarting which was not coherent at all.
4. Alerts based on hypershift operator metrics may keep firing if not designed carefully while the hostedcluster or nodepool is gone.

This PR uses dedicated collectors for all hostedcluster & nodepool metrics.

Thanks to that change:

• Time series values are computed at scraping time.
• Metrics stop being defined for a given hostedcluster or nodepool as soon as it is gone.
• Code populating the metrics is now fully decorrelated from the code reconciling the hostedclusters and the nodepools (i.e. the controllers)

This PR also:

1. Stops forcing hypershift_cluster_silence_alerts metric to 1 when the hosted cluster is being deleted.
   Indeed checking whether or not a hosted cluster is being deleted can be achieved by checking whether or not metric hypershift_cluster_deleting_duration_seconds is being defined.
2. Defines the _id label for all nodepool metrics
3. Changes all the metrics measuring time:
   • Time is no longer measured at the end of the action!
   • Time is measured while the action is going on... and the metric stops being defined at the end of the action.
     This is much more coherent with designing an alert on an action taking too much time to complete.
     Also for some metrics (like hypershift_cluster_deletion_duration_seconds) the end of the action was also the end of the hostedcluster or nodepool the metrics refers to, so those metrics couldn't start being defined when the action was done (not to mention that if the hypershift operator was restarted just after, those metrics would have been defined for a very brief period of time if not at all)
   • The following metrics have been renamed according above semantic change:
     • hypershift_cluster_deletion_duration_seconds -> hypershift_cluster_deleting_duration_seconds
     • hypershift_cluster_guest_cloud_resources_deletion_duration_seconds -> hypershift_cluster_guest_cloud_resources_deleting_duration_seconds
     • hypershift_cluster_available_duration_seconds -> hypershift_cluster_waiting_initial_avaibility_duration_seconds
     • hypershift_cluster_initial_rollout_duration_seconds -> hypershift_cluster_initial_rolling_out_duration_seconds
     • hypershift_cluster_upgrade_duration_seconds -> hypershift_cluster_upgrading_duration_seconds
     • hypershift_nodepools_deletion_duration_seconds -> hypershift_nodepools_deleting_duration_seconds
     • hypershift_nodepools_initial_rollout_duration_seconds -> hypershift_nodepools_initial_rolling_out_duration_seconds
   • Unit-tests were migrated accordingly to this new semantic.
4. Replaces metric hypershift_cluster_skipped_cloud_resources_deletion with hypershift_cluster_invalid_aws_creds.
   Indeed the old metric is set (and only set) when deleting the cluster (i.e. when hypershift_cluster_deleting_duration_seconds is set) and when the AWS creds are invalid (when hypershift_cluster_invalid_aws_creds is set). The new metric is no more transaction based which allowed the decorrelation between the controller code and the collector code.
5. Moves all but one metrics at root level in the collectors.
   Indeed those metrics (handled by some kind of fake collector) were either concerning hostedclusters or nodepools.
6. Adds a platform label to hypershift_hostedclusters_failure_conditions metric in order to be consistent with hypershift_nodepools_failure_conditions metric.
7. Initialises hypershift_nodepools_failure_conditions metric to 0 (instead of undefined) for all plaforms & failure conditions in order to be consistent with hypershift_hostedclusters_failure_conditions.
8. Initialises hypershift_nodepools metric to 0 (instead of undefined) to be consistent with hypershift_hostedclusters.
9. Adds a _id label to hypershift_hostedcluster_nodepools metric. Also label cluster_name has been renamed to name in order to be consistant with the other hostedcluster metrics (remark that this metric is however still computed while dealing with the nodepools).
   Maybe this metric could be dropped as it is equivalent to sum by (namespace, name, _id, platform) (label_replace(count by (namespace, cluster_name, _id, platform) (hypershift_nodepools_available_replicas), "name", "$0", "cluster_name", ".*"))
10. Make sure that annotation hypershift.openshift.io/HasBeenAvailable really get properly added on a hostedcluster when needed... this was not working before.
11. Metric hypershift_nodepools_transition_seconds is now really defined. Old code was attempting to set this metric... but as the new status was already set before the call to ObserveConditionTransitionDuration old & new statuses were actually the same!

Which issue(s) this PR fixes

Contributes to OCPBUGS-20179 (also this is the clean fix for OSD-16638).
Note that the PR proposes some renaming for some metrics, please tell if you are okay with those renames, maybe they can be shipped with this PR.

Checklist

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

[netlify]

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	a091d00
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/654bc4f31fc403000838ed37
😎 Deploy Preview	https://deploy-preview-2671--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[enxebre]

I'm not sure I follow the deletion metrics pattern. How is current service dealing with remaining metrics from deleted clusters?

[Nikokolas3270]

I'm not sure I follow the deletion metrics pattern. How is current service dealing with remaining metrics from deleted clusters?

That's the problem, all the metrics for a given deleted hosted cluster or node pool currently continue to exist till the hypershift-operator is eventually restarted.

Aim of this PR is to make sure the metrics (or more accurately the time series) for a given hosted cluster or node pool cease to exists when the hosted cluster or node pool is deleted.

[enxebre]

That's the problem, all the metrics for a given deleted hosted cluster or node pool currently continue to exist till the hypershift-operator is eventually restarted.

Thanks I see what you are trying to solve, my questions is how are we solving the same thing in traditional ROSA? After a cluster is gone what happen to their metrics?

This is effectively removing our ability to track and meet our SLOs for HC deletion. I'd suggest we put a doc together so we can agree on the goals then we revisit the best way to implement them.

[Nikokolas3270]

That's the problem, all the metrics for a given deleted hosted cluster or node pool currently continue to exist till the hypershift-operator is eventually restarted.

Thanks I see what you are trying to solve, my questions is how are we solving the same thing in traditional ROSA? After a cluster is gone what happen to their metrics?

The metrics are ended; exactly as I intend to do here.

In traditional ROSA, the metrics are all produced by (pods in) the cluster itself. When the cluster is deleted those metrics cease to be produced (for the cluster which has been deleted).

With hypershift, metrics are produced by various sources:

1. By some pods in the data plane
2. By some pods in the management clusters running in the HCP namespace for the considered hosted cluster (ex: the control-plane-operator pod)
3. By the Hypershift operator

Source 1 and 2 cease to produce metrics when the hosted cluster is deleted.
Source 3 continues to produce metrics for a hosted cluster which is gone till the Hypershift operator is (eventually) restarted. In this PR we indent to stop producing metrics for a given hosted cluster as soon as it is deleted.
You see, the problem is that the Hypershift operator continues to exist long after a hosted cluster is gone; this is not the case for the other sources; hence a special handling is required in Hypershift operator code to handle time series indexed on the hosted cluster and/or node pools.

This is effectively removing our ability to track and meet our SLOs for HC deletion. I'd suggest we put a doc together so we can agree on the goals then we revisit the best way to implement them.

Is "This" == "This pull request"?
As previously explained: it is not normal to keep a metric for which the hosted cluster or node pool referenced by the label set is gone: this is a leak!
If you really want to prolongate a time series, you can do so as follows but only for some given additional time:

last_over_time{metric[28d]}

(this works for metrics which are counters or behave as counters)
For global SLOs on all hosted clusters, you normally do not need to go deep and analyse metrics for a given hosted cluster.

[Nikokolas3270]

/retest

[simonpasquier]

My 2 cents (irrespective of the discussion whether the metric should be deleted or not): deleting metrics in code is usually a code smell as deleting series when some object is removed is prone to error. It is in general safer to implement the custom collector pattern with Const metrics...

https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#hdr-Custom_Collectors_and_constant_Metrics

[Nikokolas3270]

My 2 cents (irrespective of the discussion whether the metric should be deleted or not): deleting metrics in code is usually a code smell as deleting series when some object is removed is prone to error. It is in general safer to implement the custom collector pattern with Const metrics...

https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#hdr-Custom_Collectors_and_constant_Metrics

Thanks @simonpasquier for the comment; implementing a custom collector is indeed a better & safer way to go (didn't know about that possibility). I will change the code in this PR to do exactly that.

[celebdor]

/jira refresh

[openshift-ci-robot]

@celebdor: This pull request references Jira Issue OCPBUGS-20179, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.15.0) matches configured target version for branch (4.15.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (jiezhao@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

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.

[sjenning]

/test e2e-aws-metrics

[sjenning]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Nikokolas3270, sjenning

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [sjenning]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[enxebre]

why is this patch needed here now vs before?

[Nikokolas3270]

Hello Alberto,
There was a bug in the old code: the hostedcluster object was annotated in memory but the annotation was not saved in DB.
I believe this is due to this code which only saves the status change but nothing else:
https://github.com/openshift/hypershift/blob/main/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go#L322
Here is some other place in the code in which the same kind of call to patch in needed to save annotations:

hypershift/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

Line 3405 in 7fb56f7

if err := r.Patch(ctx, hcp, client.MergeFromWithOptions(original)); err != nil {

[Nikokolas3270]

Rebased

[sjenning]

--- FAIL: TestReportWaitingInitialAvailabilityDuration (0.01s)
    --- FAIL: TestReportWaitingInitialAvailabilityDuration/When_cluster_just_got_created,_metric_is_reported_with_a_value_set_to_0 (0.01s)
panic: cannot handle unexported field at {*io_prometheus_client.MetricFamily}.state:
	"github.com/prometheus/client_model/go".MetricFamily
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported [recovered]
	panic: cannot handle unexported field at {*io_prometheus_client.MetricFamily}.state:
	"github.com/prometheus/client_model/go".MetricFamily
consider using a custom Comparer; if you control the implementation of type, you can also consider using an Exporter, AllowUnexported, or cmpopts.IgnoreUnexported
...
FAIL	github.com/openshift/hypershift/hypershift-operator/controllers/hostedcluster/metrics	0.146s

[openshift-ci]

@Nikokolas3270: 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/e2e-ibmcloud-iks	30a3773	link	false	/test e2e-ibmcloud-iks
ci/prow/e2e-ibmcloud-roks	30a3773	link	false	/test e2e-ibmcloud-roks

Full PR test history. Your PR dashboard.

Details

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.

[enxebre]

thanks!
/lgtm

[openshift-ci-robot]

@Nikokolas3270: Jira Issue OCPBUGS-20179: All pull requests linked via external trackers have merged:

• openshift/hypershift#2671

Jira Issue OCPBUGS-20179 has been moved to the MODIFIED state.

Details

In response to this:

What this PR does / why we need it:

hostedcluster and nodepool time series keep being defined long after the hostedcluster or nodepool is gone.

This is problematic because:

1. We plan to add a new hostedcluster metric which will "count" the number of worker cores and which will be used to bill the customers (see OSD-19085).
   This metric must stop to be defined as soon as the cluster is gone or we will continue to bill the customer for a cluster which is gone.
2. This is misleading.
3. This is kind of a leak; time series were defined forever... unless the hypershift operator was restarting which was not coherent at all.
4. Alerts based on hypershift operator metrics may keep firing if not designed carefully while the hostedcluster or nodepool is gone.

This PR uses dedicated collectors for all hostedcluster & nodepool metrics.

Thanks to that change:

• Time series values are computed at scraping time.
• Metrics stop being defined for a given hostedcluster or nodepool as soon as it is gone.
• Code populating the metrics is now fully decorrelated from the code reconciling the hostedclusters and the nodepools (i.e. the controllers)

This PR also:

1. Stops forcing hypershift_cluster_silence_alerts metric to 1 when the hosted cluster is being deleted.
   Indeed checking whether or not a hosted cluster is being deleted can be achieved by checking whether or not metric hypershift_cluster_deleting_duration_seconds is being defined.
2. Defines the _id label for all nodepool metrics
3. Changes all the metrics measuring time:

• Time is no longer measured at the end of the action!
• Time is measured while the action is going on... and the metric stops being defined at the end of the action.
  This is much more coherent with designing an alert on an action taking too much time to complete.
  Also for some metrics (like hypershift_cluster_deletion_duration_seconds) the end of the action was also the end of the hostedcluster or nodepool the metrics refers to, so those metrics couldn't start being defined when the action was done (not to mention that if the hypershift operator was restarted just after, those metrics would have been defined for a very brief period of time if not at all)
• The following metrics have been renamed according above semantic change:
  • hypershift_cluster_deletion_duration_seconds -> hypershift_cluster_deleting_duration_seconds
  • hypershift_cluster_guest_cloud_resources_deletion_duration_seconds -> hypershift_cluster_guest_cloud_resources_deleting_duration_seconds
  • hypershift_cluster_available_duration_seconds -> hypershift_cluster_waiting_initial_avaibility_duration_seconds
  • hypershift_cluster_initial_rollout_duration_seconds -> hypershift_cluster_initial_rolling_out_duration_seconds
  • hypershift_cluster_upgrade_duration_seconds -> hypershift_cluster_upgrading_duration_seconds
  • hypershift_nodepools_deletion_duration_seconds -> hypershift_nodepools_deleting_duration_seconds
  • hypershift_nodepools_initial_rollout_duration_seconds -> hypershift_nodepools_initial_rolling_out_duration_seconds
• Unit-tests were migrated accordingly to this new semantic.

4. Replaces metric hypershift_cluster_skipped_cloud_resources_deletion with hypershift_cluster_invalid_aws_creds.
   Indeed the old metric is set (and only set) when deleting the cluster (i.e. when hypershift_cluster_deleting_duration_seconds is set) and when the AWS creds are invalid (when hypershift_cluster_invalid_aws_creds is set). The new metric is no more transaction based which allowed the decorrelation between the controller code and the collector code.
5. Moves all but one metrics at root level in the collectors.
   Indeed those metrics (handled by some kind of fake collector) were either concerning hostedclusters or nodepools.
6. Adds a platform label to hypershift_hostedclusters_failure_conditions metric in order to be consistent with hypershift_nodepools_failure_conditions metric.
7. Initialises hypershift_nodepools_failure_conditions metric to 0 (instead of undefined) for all plaforms & failure conditions in order to be consistent with hypershift_hostedclusters_failure_conditions.
8. Initialises hypershift_nodepools metric to 0 (instead of undefined) to be consistent with hypershift_hostedclusters.
9. Adds a _id label to hypershift_hostedcluster_nodepools metric. Also label cluster_name has been renamed to name in order to be consistant with the other hostedcluster metrics (remark that this metric is however still computed while dealing with the nodepools).
   Maybe this metric could be dropped as it is equivalent to sum by (namespace, name, _id, platform) (label_replace(count by (namespace, cluster_name, _id, platform) (hypershift_nodepools_available_replicas), "name", "$0", "cluster_name", ".*"))
10. Make sure that annotation hypershift.openshift.io/HasBeenAvailable really get properly added on a hostedcluster when needed... this was not working before.
11. Metric hypershift_nodepools_transition_seconds is now really defined. Old code was attempting to set this metric... but as the new status was already set before the call to ObserveConditionTransitionDuration old & new statuses were actually the same!

Which issue(s) this PR fixes

Contributes to OCPBUGS-20179 (also this is the clean fix for OSD-16638).
Note that the PR proposes some renaming for some metrics, please tell if you are okay with those renames, maybe they can be shipped with this PR.

Checklist

• Subject and description added to both, commit and PR.
• Relevant issues have been referenced.
• This change includes docs.
• This change includes unit tests.

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.

[openshift-merge-robot]

Fix included in accepted release 4.15.0-0.nightly-2023-11-14-041944