Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MON-3763: Add cnv_abnormal #2291

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MON-3763: Add cnv_abnormal #2291

wants to merge 1 commit into from

Conversation

avlitman
Copy link

@avlitman avlitman commented Mar 20, 2024 •
edited

Add new recording rule about cnv issues to telemeter, as for now the only issue is memory exceeded.

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

@avlitman
Copy link
Author

avlitman commented Mar 20, 2024 •
edited

/hold blocked by kubevirt/hyperconverged-cluster-operator#2855

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2024
@openshift-ci openshift-ci bot requested review from jan--f and marioferh March 20, 2024 14:37
@avlitman avlitman mentioned this pull request Mar 20, 2024
Closed
@sradco
Copy link

sradco commented Apr 25, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 25, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: avlitman, sradco
Once this PR has been reviewed and has the lgtm label, please assign danielmellado for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 2, 2024

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 2, 2024
@avlitman
Copy link
Author

avlitman commented May 2, 2024

/unhold

Created created https://issues.redhat.com/browse/MON-3763
And the pr adding this metric is merged as well: kubevirt/hyperconverged-cluster-operator#2855

@jan--f Will appreciate your review and approval, also should I cherry-pick this to 4.16? since I see we are already have 4.17. thanks

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 2, 2024
simonpasquier
simonpasquier reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at https://github.com/kubevirt/hyperconverged-cluster-operator/blob/a98cb26b9ad8774cd676f703600bb0fe407875cc/pkg/monitoring/rules/recordingrules/operator.go#L26-L33, I'm not sure why you have a recording rule only renaming the metric name. Why not sending the original metric?

For reference:

manifests/0000_50_cluster-monitoring-operator_04-config.yaml Outdated
@@ -240,6 +240,12 @@ data:
#
# owners: (https://github.com/kubevirt)
#
# cnv_abnormal represents the reason why the operator might have an issue
# and includes the node, namespace, container, reason labels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why include the node name? Is it meaningful?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the node label wasn't mentioned in https://issues.redhat.com/browse/MON-3763

Copy link
Author

@avlitman avlitman May 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need to node label for sure. will mention it in the ticket.

Copy link
Author

@avlitman avlitman May 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the namespace is removed, so removed also from the yaml.

@simonpasquier
Copy link
Contributor

also should I cherry-pick this to 4.16? since I see we are already have 4.17.

For now 4.16 == 4.17 (until the 4.16 branch is officially cut).

@simonpasquier
Copy link
Contributor

you'll need to run make generate and commit the changes too.

@avlitman
Copy link
Author

avlitman commented May 5, 2024

make generate

@simonpasquier done (:

@simonpasquier
Copy link
Contributor

simonpasquier commented May 6, 2024 •
edited by openshift-ci bot

/hold
/retitle MON-3763: Add cnv_abnormal

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2024
@openshift-ci openshift-ci bot changed the title Add cnv_abnormal MON-3763: Add cnv_abnormal May 6, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 6, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 6, 2024 •
edited by openshift-ci bot

@avlitman: This pull request references MON-3763 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 task to target the "4.16.0" version, but no target version was set.

In response to this:

Add new recording rule about cnv issues to telemeter, as for now the only issue is memory exceeded.

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

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.

@avlitman
Add cnv_abnormal
d690f83 
cnv_abnormal holds issues with the pods for each container, e.g. memory
exceeded value.

Signed-off-by: avlitman <alitman@redhat.com>
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 9, 2024

@avlitman: 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/versions d690f83 link false /test versions
ci/prow/generate d690f83 link true /test generate
ci/prow/e2e-aws-ovn-single-node d690f83 link false /test e2e-aws-ovn-single-node

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sradco sradco sradco left review comments

@jan--f jan--f Awaiting requested review from jan--f

@marioferh marioferh Awaiting requested review from marioferh

@simonpasquier simonpasquier Awaiting requested review from simonpasquier

Assignees

@sradco sradco

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@avlitman @sradco @simonpasquier @openshift-ci-robot