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

Fix DownwardMetrics MemoryAllocatedToVirtualServers and add missing ResourceProcessorLimit metrics #9808

Merged
merged 2 commits into from
Jun 5, 2023
Merged

Fix DownwardMetrics MemoryAllocatedToVirtualServers and add missing ResourceProcessorLimit metrics #9808

merged 2 commits into from
Jun 5, 2023

Conversation

jcanocan
Copy link
Contributor

@jcanocan jcanocan commented May 26, 2023 •
edited

What this PR does / why we need it:
As part of the SAP certification effort, the DownwardMetrics should be aligned with https://sourcegraph.com/src.fedoraproject.org/rpms/vhostmd/-/blob/vhostmd.conf.

It changes the current state of the DownwardMetrics from:

<metrics>
...
  <metric type="uint64" context="host" unit="KiB">
    <name>AllocatedToVirtualServers</name>
    <value>3649456</value>
  </metric>
...
</metrics>

to:

<metrics>
...
  <metric type="uint64" context="host" unit="KiB">
    <name>MemoryAllocatedToVirtualServers</name>
    <value>3649456</value>
  </metric>
...
  <metric type="uint64" context="vm">
    <name>ResourceProcessorLimit</name>
    <value>2</value>
  </metric>
...
<metrics>

Please note, this change request has been raised by SAP in: #9673 (comment)

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:

DownwardMetrics: Rename AllocatedToVirtualServers metric to AllocatedToVirtualServers and add ResourceProcessorLimit metric

@germag @jcanocan
DownwardMetrics: Fix metrics name
9c15e50 
vhostmd expects[0] the metric showing the physical memory used by the
virtual machines (_not_ physical memory usable by the virtual machines),
to be called MemoryAllocatedToVirtualServers.

[0] https://sourcegraph.com/src.fedoraproject.org/rpms/vhostmd/-/blob/vhostmd.conf

Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>
Signed-off-by: German Maglione <gmaglione@redhat.com>
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. area/monitoring size/S labels May 26, 2023
@jcanocan
Copy link
Contributor Author

/cc @germag

@kubevirt-bot
Copy link
Contributor

@jcanocan: GitHub didn't allow me to request PR reviews from the following users: germag.

Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @germag

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.

@germag germag mentioned this pull request May 26, 2023
Merged
@jcanocan
Copy link
Contributor Author

/test pull-kubevirt-unit-test

@jcanocan
Copy link
Contributor Author

/retest-required

@jcanocan
Copy link
Contributor Author

Removed commit hack: Add the KubeVirt directory as a safe git directory because it was just a temporary hackaround to prevent CI failures. Please refer to #9806 for further details.

@jcanocan
Copy link
Contributor Author

/retest-required

@kubevirt-bot kubevirt-bot added size/M and removed size/S labels Jun 1, 2023
0xFelix
0xFelix reviewed Jun 2, 2023
View reviewed changes
tests/libvmi/cpu.go Outdated Show resolved Hide resolved
@germag @jcanocan
DownwardMetrics: Add ResourceProcessorLimit metric
f9e39e8 
vhostmd expects[0] to find the `ResourceProcessorLimit` metric, that is
the amount of CPU cores defined for the VM. It's the same information
that can be obtained using:

```
virsh -r dominfo my-domain-name | grep ^CPU
```

[0] https://sourcegraph.com/src.fedoraproject.org/rpms/vhostmd/-/blob/vhostmd.conf

Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>
Signed-off-by: German Maglione <gmaglione@redhat.com>
0xFelix
0xFelix approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 2, 2023
@jcanocan
Copy link
Contributor Author

jcanocan commented Jun 2, 2023

@0xFelix Thanks for your reviews :)

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Jun 2, 2023 •
edited

@jcanocan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-check-tests-for-flakes f9e39e8 link false /test pull-kubevirt-check-tests-for-flakes

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.

@jcanocan
Copy link
Contributor Author

jcanocan commented Jun 5, 2023

/retest-required

@jcanocan
Copy link
Contributor Author

jcanocan commented Jun 5, 2023

/cc @rmohr could you please take a look?

@rmohr
Copy link
Member

rmohr commented Jun 5, 2023

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

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

The pull request process is described 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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2023
@jcanocan
Copy link
Contributor Author

jcanocan commented Jun 5, 2023

@rmohr Many thanks for your time! 😊

@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit 2cfd09e into kubevirt:main Jun 5, 2023
36 of 37 checks passed
@kubevirt-bot kubevirt-bot mentioned this pull request Jul 26, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xFelix 0xFelix 0xFelix approved these changes

@dhiller dhiller Awaiting requested review from dhiller

@enp0s3 enp0s3 Awaiting requested review from enp0s3

Assignees

@0xFelix 0xFelix

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/monitoring dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jcanocan @kubevirt-bot @rmohr @kubevirt-commenter-bot @0xFelix @germag