-
Notifications
You must be signed in to change notification settings - Fork 14
Update quota metric #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Update quota metric #169
Conversation
|
@Barakmor1 please review this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1285076 to
4115dbe
Compare
c39ed92 to
cff3f10
Compare
d523c29 to
6dbda3f
Compare
|
Thanks for this work. At first glance, it looks like many different changes are in one commit. It would be much easier to review if this was split into several commits. For example, one commit for docs, one for the linter, one for renaming, and one for the recording rule for backward compatibility. Reviewing everything in one commit is very hard. Also, we already have a metrics docs generator in AAQ: One more note: AAQ uses Prow for automation, not GitHub Actions. I prefer not to mix these two. If needed, we can add a postsubmit job later using: We can also do this as follow-up work later. |
6dbda3f to
55e0b0b
Compare
|
@Barakmor1 Hi, Updated the PR. Please review |
55e0b0b to
24c5410
Compare
|
@Barakmor1 Thank you for the review. I updated the PR based on your review. Please review again. |
24c5410 to
42ecc7a
Compare
Update the use of github.com/machadovilaca/operator-observabilit to github.com/rhobs/operator-observability-toolkit Signed-off-by: Shirly Radco <[email protected]>
Updated kube_application_aware_resourcequota_creation_timestamp to kube_application_aware_resourcequota_creation_timestamp_seconds to comply with the Prometheus naming conventions. Signed-off-by: Shirly Radco <[email protected]>
Add a backwards compatability recording rule, kube_application_aware_resourcequota_creation_timestamp, since the original metric name was updated to kube_application_aware_resourcequota_creation_timestamp_seconds Signed-off-by: Shirly Radco <[email protected]>
Signed-off-by: Shirly Radco <[email protected]>
Signed-off-by: Shirly Radco <[email protected]>
42ecc7a to
35ba0df
Compare
|
@Barakmor1 Hi, Updated the PR. Please review |
| "prometheus": "k8s", | ||
| "role": "alert-rules", | ||
| "aaq.kubevirt.io": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need these labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't "aaq.kubevirt.io": "", be enough?
| // Build PrometheusRule from registered rules (recording rules and alerts) | ||
| // If building rules fails (e.g., none registered), we proceed with the other resources. | ||
| var prometheusRule *promv1.PrometheusRule | ||
| if err := aaqrules.SetupRules(); err == nil { | ||
| if ruleObj, buildErr := aaqrules.BuildPrometheusRule(namespace); buildErr == nil { | ||
| prometheusRule = ruleObj | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this below
prometheusResources := []client.Object{
namespaced.NewPrometheusServiceMonitor(namespace),
namespaced.NewPrometheusRole(namespace),
namespaced.NewPrometheusRoleBinding(namespace),
namespaced.NewPrometheusService(namespace),
}
and include this code on the same block:
prometheusResources = append(prometheusResources, prometheusRule)
something like:
if err := aaqrules.SetupRules(); err == nil {
if ruleObj, buildErr := aaqrules.BuildPrometheusRule(namespace); buildErr == nil {
prometheusResources = append(prometheusResources, ruleObj)
}
}
Barakmor1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
pkg/aaq-operator/prometheus.go
Outdated
| // For PrometheusRule, ensure Spec is reconciled as well (labels/annotations handled by MergeObject) | ||
| if prDesired, ok := desired.(*promv1.PrometheusRule); ok { | ||
| if prCurrent, ok2 := merged.(*promv1.PrometheusRule); ok2 { | ||
| // Copy spec if differs | ||
| if !equality.Semantic.DeepEqual(prCurrent.Spec, prDesired.Spec) { | ||
| prCurrent.Spec = prDesired.Spec | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. No need for this explicit test.
Update the doc-generator based on the other kubevirt operators and add support for metrics and recording rules. Signed-off-by: Shirly Radco <[email protected]>
35ba0df to
35a4818
Compare
|
[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 |
|
/test pull-aaq-generate-verify |
|
@Barakmor1 all tests pass. Please review |
|
Hey, there are some comments that weren’t addressed yet: |
What this PR does / why we need it:
Updated the
kube_application_aware_resourcequota_creation_timestamp metric name to
kube_application_aware_resourcequota_creation_timestamp_seconds to meet the Prometheus naming conventions
and added a recording rule for backwards compatability.
The PR also adds the prometheus unit tests, the metrics naming linter and the docs generator, like we have the the rest of the kubevirt operators.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note: