Fix metrics stats for VMs not running #5633

joseflauzino · 2021-10-27T11:47:02Z

Description

This PR fixes the behavior of the stats collector with respect to VMs that are not running. The need for this is that if a running VM has a state change for some reason, ACS keeps showing the latest metrics stats collected.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

How Has This Been Tested?

Using ACS UI, I performed the following steps:

I deployed a VM and waited for the complete boot;
I enabled the presentation of metric statistics in the UI;
I waited for ACS to collect and show metrics stats;
And then, I tested two different cases:
- Case 1 - I stopped VM using ACS UI;
- Case 2 - I simulated a VM crash by manually stopping it (directly from the hypervisor);
In both cases, the metrics stats for the VM were successfully cleared. As expected, when I tested Case 1, cleanup was done immediately. In Case 2, I had to wait for ACS to identify that the VM was stopped and the stats collector try to collect new metric stats.

server/src/main/java/com/cloud/server/StatsCollector.java

server/src/test/java/com/cloud/server/StatsCollectorTest.java

joseflauzino · 2021-10-28T11:54:50Z

@sureshanaparti and @DaanHoogland
Done.
Thanks for the suggestions!
Because removeAll() returns a boolean, I had to break into two lines.

DaanHoogland · 2021-11-01T10:06:04Z

@blueorangutan package

blueorangutan · 2021-11-01T10:08:03Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-11-01T11:12:35Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1646

sureshanaparti · 2021-11-01T12:35:34Z

@blueorangutan test

blueorangutan · 2021-11-01T12:36:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-11-01T21:51:05Z

Trillian test result (tid-2502)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31865 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5633-t2502-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2021-11-02T12:15:11Z

server/src/main/java/com/cloud/server/StatsCollector.java

+     */
+    protected void cleanUpVirtualMachineStats() {
+        List<Long> allRunningVmIds = new ArrayList<Long>();
+        for (UserVmVO vm : _userVmDao.listAllRunning()) {


VM stats are captured for running VMs only

cloudstack/server/src/main/java/com/cloud/server/StatsCollector.java

Line 579 in a78af60

List<UserVmVO> vms = _userVmDao.listRunningByHostId(host.getId());

removeVirtualMachineStats() removes the vm stat if the VM is stopped/destroyed after adding to the stats, so no need for this cleanup method.

Thanks for the review @sureshanaparti!
The cleanUpVirtualMachineStats() method is important to maintain consistency of the metric stats shown by ACS even when there are cases of VMs changing state unexpectedly (e.g., when a VM crashes; or when the VM is stopped directly by the hypervisor interface, etc). In these cases the removeVirtualMachineStats() method is not executed, so the cleanUpVirtualMachineStats() method does its job.

joseflauzino · 2021-11-16T13:35:23Z

Any update about this PR?

DaanHoogland · 2021-11-18T09:02:33Z

code looks good, let's retest
@blueorangutan package

blueorangutan · 2021-11-18T09:03:03Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-11-18T13:33:14Z

Packaging result: ✖️ el7 ✖️ el8 ✔️ debian ✖️ suse15. SL-JID 1734

DaanHoogland · 2021-11-18T13:34:50Z

@blueorangutan package

blueorangutan · 2021-11-18T13:36:04Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-11-18T14:43:48Z

Packaging result: ✔️ el7 ✖️ el8 ✔️ debian ✖️ suse15. SL-JID 1739

…metrics-stats-for-vms-not-running

joseflauzino · 2021-11-19T12:35:55Z

@blueorangutan package

blueorangutan · 2021-11-19T12:37:03Z

@joseflauzino a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-11-19T13:39:58Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1749

DaanHoogland · 2021-11-22T09:32:07Z

@blueorangutan test

blueorangutan · 2021-11-22T09:33:02Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-11-22T18:36:20Z

Trillian test result (tid-2572)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31220 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5633-t2572-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

GutoVeronezi

LGTM

@DaanHoogland can we merge this?

DaanHoogland · 2021-12-06T11:33:10Z

@GutoVeronezi yes, I think @sureshanaparti's question was answered to satisfaction as well. let's merge.

sureshanaparti · 2021-12-06T12:52:45Z

@joseflauzino Is this good to go in 4.16.1? If so, please rebase with '4.16'.

joseflauzino · 2021-12-06T14:02:23Z

@sureshanaparti for me we can keep it in the main (4.17).

GutoVeronezi · 2021-12-06T14:06:01Z

Merging this based on 2 LGTMs and package and test results.

* Fix metrics stats for VMs that are not running * Improves the way to get vmIdsToRemoveStats * Improves test Co-authored-by: José Flauzino <jose@scclouds.com.br>

sureshanaparti reviewed Oct 27, 2021

View reviewed changes

server/src/main/java/com/cloud/server/StatsCollector.java Outdated Show resolved Hide resolved

DaanHoogland reviewed Oct 27, 2021

View reviewed changes

server/src/test/java/com/cloud/server/StatsCollectorTest.java Outdated Show resolved Hide resolved

José Flauzino added 3 commits October 28, 2021 09:33

Fix metrics stats for VMs that are not running

6494b09

Improves the way to get vmIdsToRemoveStats

49431fe

Improves test

a78af60

joseflauzino force-pushed the fix-metrics-stats-for-vms-not-running branch from fc07f07 to a78af60 Compare October 28, 2021 12:36

sureshanaparti reviewed Nov 2, 2021

View reviewed changes

GutoVeronezi added this to the 4.17.0.0 milestone Nov 17, 2021

Merge branch 'main' of https://github.com/apache/cloudstack into fix-…

608fc6b

…metrics-stats-for-vms-not-running

DaanHoogland approved these changes Nov 23, 2021

View reviewed changes

GutoVeronezi approved these changes Dec 3, 2021

View reviewed changes

GutoVeronezi merged commit 28385be into apache:main Dec 6, 2021

joseflauzino mentioned this pull request Feb 4, 2022

Persistence of VM stats #5935

Closed

shwstppr mentioned this pull request Sep 15, 2022

UI: Login failed after session timeout #6731

Closed

Fix metrics stats for VMs not running #5633

Fix metrics stats for VMs not running #5633

Uh oh!

Conversation

joseflauzino commented Oct 27, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

Uh oh!

Uh oh!

joseflauzino commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Nov 1, 2021

Uh oh!

blueorangutan commented Nov 1, 2021

Uh oh!

blueorangutan commented Nov 1, 2021

Uh oh!

sureshanaparti commented Nov 1, 2021

Uh oh!

blueorangutan commented Nov 1, 2021

Uh oh!

blueorangutan commented Nov 1, 2021

Uh oh!

sureshanaparti Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

joseflauzino Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

joseflauzino commented Nov 16, 2021

Uh oh!

DaanHoogland commented Nov 18, 2021

Uh oh!

blueorangutan commented Nov 18, 2021

Uh oh!

blueorangutan commented Nov 18, 2021

Uh oh!

DaanHoogland commented Nov 18, 2021

Uh oh!

blueorangutan commented Nov 18, 2021

Uh oh!

blueorangutan commented Nov 18, 2021

Uh oh!

joseflauzino commented Nov 19, 2021

Uh oh!

blueorangutan commented Nov 19, 2021

Uh oh!

blueorangutan commented Nov 19, 2021

Uh oh!

DaanHoogland commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

GutoVeronezi left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Dec 6, 2021

Uh oh!

sureshanaparti commented Dec 6, 2021

Uh oh!

joseflauzino commented Dec 6, 2021

Uh oh!

GutoVeronezi commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joseflauzino commented Oct 28, 2021 •

edited

Loading