BIZ-794: separte managed and self-managed ACM capacity - #612
Conversation
* Separates the capacity of clusters connected to an ACM hub into "managed" and "self-managed" categories based on the product label. Signed-off-by: Jiri Fiala <jfiala@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jiri-fiala: This pull request references BIZ-794 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. |
📝 WalkthroughWalkthroughAdds two ACM recording rules for connected-cluster worker cores. One rule selects OpenShift and MicroShift clusters. The other selects all remaining products. Both rules sum worker cores by hub Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 97d189b7-d9f5-420c-8b3a-3ce8b29833ca
📒 Files selected for processing (1)
jsonnet/telemeter/rules.libsonnet
| { | ||
| // Sum of ACM managed cluster cores that are considered to be self-managed connected clusters | ||
| // Unlike the above acm_capacity_effective_cpu_cores rule, this goes directly to acm_managed_cluster_worker_cores:max and further filters down based on product names. | ||
| record: acm:managed_cluster_worker_cores:self_managed:sum | ||
| expr: ||| | ||
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product=~"OpenShift|MicroShift"}) | ||
| |||, | ||
| }, | ||
| // Sum of ACM managed cluster cores that are considered to be managed connected clusters (mutually exclusive vs the previous ruke) | ||
| record: acm:managed_cluster_worker_cores:managed:sum | ||
| expr: ||| | ||
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product!~"OpenShift|MicroShift"}) | ||
| |||, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
command -v jsonnetfmt >/dev/null
jsonnetfmt jsonnet/telemeter/rules.libsonnet >/dev/nullRepository: openshift/telemeter
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "jsonnetfmt available:"
command -v jsonnetfmt || true
echo
echo "Relevant file section:"
sed -n '245,272p' jsonnet/telemeter/rules.libsonnet | cat -n
echo
echo "Check Jsonnet parser/fmt behavior on the file:"
jsonnetfmt jsonnet/telemeter/rules.libsonnet || true
echo
echo "Search for similar record field syntax nearby:"
rg -n "record: [a-zA-Z_][^,\n]*:" jsonnet/telemeter/rules.libsonnet jsonnet -g '*.libsonnet' || trueRepository: openshift/telemeter
Length of output: 2697
Fix the Jsonnet syntax before merge.
The added recording-rule objects are malformed and prevent Jsonnet formatting from succeeding.
- Quote the two
recordvalues that contain colons. - Add a comma after both
recordfields. - Add
{before the second rule after the first rule-closing}.
Proposed fix
{
- record: acm:managed_cluster_worker_cores:self_managed:sum
+ record: 'acm:managed_cluster_worker_cores:self_managed:sum',
expr: |||
sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product=~"OpenShift|MicroShift"})
|||,
},
+ {
// Sum of ACM managed cluster cores that are considered to be managed connected clusters (mutually exclusive vs the previous ruke)
- record: acm:managed_cluster_worker_cores:managed:sum
+ record: 'acm:managed_cluster_worker_cores:managed:sum',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| // Sum of ACM managed cluster cores that are considered to be self-managed connected clusters | |
| // Unlike the above acm_capacity_effective_cpu_cores rule, this goes directly to acm_managed_cluster_worker_cores:max and further filters down based on product names. | |
| record: acm:managed_cluster_worker_cores:self_managed:sum | |
| expr: ||| | |
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product=~"OpenShift|MicroShift"}) | |
| |||, | |
| }, | |
| // Sum of ACM managed cluster cores that are considered to be managed connected clusters (mutually exclusive vs the previous ruke) | |
| record: acm:managed_cluster_worker_cores:managed:sum | |
| expr: ||| | |
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product!~"OpenShift|MicroShift"}) | |
| |||, | |
| }, | |
| { | |
| // Sum of ACM managed cluster cores that are considered to be self-managed connected clusters | |
| // Unlike the above acm_capacity_effective_cpu_cores rule, this goes directly to acm_managed_cluster_worker_cores:max and further filters down based on product names. | |
| record: 'acm:managed_cluster_worker_cores:self_managed:sum', | |
| expr: ||| | |
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product=~"OpenShift|MicroShift"}) | |
| |||, | |
| }, | |
| { | |
| // Sum of ACM managed cluster cores that are considered to be managed connected clusters (mutually exclusive vs the previous ruke) | |
| record: 'acm:managed_cluster_worker_cores:managed:sum', | |
| expr: ||| | |
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product!~"OpenShift|MicroShift"}) | |
| |||, | |
| }, |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jiri-fiala 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 |
|
@jiri-fiala: The following tests failed, say
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. |
| // Sum of ACM managed cluster cores that are considered to be managed connected clusters (mutually exclusive vs the previous ruke) | ||
| record: acm:managed_cluster_worker_cores:managed:sum | ||
| expr: ||| | ||
| sum by (_id) (acm_managed_cluster_worker_cores:max and on(hub_cluster_id, managed_cluster_id) acm_managed_cluster_info{product!~"OpenShift|MicroShift"}) |
There was a problem hiding this comment.
Is there a way to write this in such a way that it is looking for a specific product and not just the abscense of OpenShift|MicroShift ?
|
@jiri-fiala just to set expectations: you'll need unit tests to validate the output of the recording rules as in #554 for example. |
4 participants
The newly introduced two rules are meant to separate the vCPU/core capacity reported for capacity connected to an ACM hub based on the product label of those connected cluster to support BIZ-794.