vpc: optimize createMonitorServiceCommand() execution by phsm · Pull Request #8385 · apache/cloudstack

phsm · 2023-12-20T13:12:07Z

Description

This PR moves an expensive getRouterHealthChecksConfig() upper the execution stack during VPC restart. So this way it is only executed once instead of tens or hundreds times (depending on VPC tiers amount).

This is done by introducing a Map<String, String> routerHealthCheckConfig argument to createMonitorServiceCommand() method that accepts pre-computed health check config.

Fixes#8055

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)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Prepared a test VPC:

10 tiers
redundant offering (2 VRs)
each tier has 1 running VM in it.

Performed VPC restart with clean up. Took 61 minutes to fully restart in my case
Applied these changes
Performed VPC restart with clean up. Took 9 minutes to fully restart.

How did you try to break this feature and the system with this change?

boring-cyborg · 2023-12-20T13:12:12Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

DaanHoogland · 2023-12-20T13:46:08Z

@blueorangutan package

codecov · 2023-12-20T13:53:15Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e47a910) 13.16% compared to head (3c5fc9d) 30.74%.

Files	Patch %	Lines
...ork/router/VirtualNetworkApplianceManagerImpl.java	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.18 #8385 +/- ##
=============================================
+ Coverage 13.16% 30.74% +17.58% - Complexity 9203 33064 +23861 
=============================================
Files 2724 5353 +2629 Lines 258120 374609 +116489 Branches 40231 54635 +14404 =============================================
+ Hits 33989 115188 +81199 - Misses 219823 244149 +24326 - Partials 4308 15272 +10964

Flag	Coverage Δ
simulator-marvin-tests	`24.59% <77.77%> (?)`
uitests	`4.38% <ø> (?)`
unit-tests	`16.43% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

weizhouapache · 2023-12-20T14:58:24Z

@blueorangutan package

blueorangutan · 2023-12-20T15:00:03Z

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-12-20T16:11:01Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8103

harikrishna-patnala · 2023-12-21T07:35:18Z

@blueorangutan test

blueorangutan · 2023-12-21T07:36:03Z

@harikrishna-patnala a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

harikrishna-patnala

results seems to be very interesting @phsm

blueorangutan · 2023-12-21T19:53:11Z

[SF] Trillian test result (tid-8650)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42730 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8385-t8650-kvm-centos7.zip
Smoke tests completed. 127 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_05_list_volumes_isrecursive	`Failure`	0.03	test_list_volumes.py
test_07_list_volumes_listall	`Failure`	0.02	test_list_volumes.py

sureshanaparti

Code LGTM

sureshanaparti · 2023-12-22T10:08:07Z

@blueorangutan package

blueorangutan · 2023-12-22T10:10:03Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

weizhouapache

code lgtm

blueorangutan · 2023-12-22T11:52:34Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8130

sureshanaparti · 2023-12-22T12:38:02Z

@blueorangutan test

blueorangutan · 2023-12-22T12:40:03Z

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

blueorangutan · 2023-12-23T02:42:26Z

[SF] Trillian test result (tid-8666)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49046 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8385-t8666-kvm-centos7.zip
Smoke tests completed. 121 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

phsm · 2024-01-11T13:39:56Z

Is there anything else that I can do for this PR to be merged? I see some tests are failed but my Java knowledge isn't that great to actually fix them so I'd appreciate any help on this.

weizhouapache · 2024-01-11T13:47:56Z

Is there anything else that I can do for this PR to be merged? I see some tests are failed but my Java knowledge isn't that great to actually fix them so I'd appreciate any help on this.

@DaanHoogland @shwstppr can you please review ?

DaanHoogland

clgtm, not sure what we need to test for this (assuming the smoke tests have tested health check functionality)

phsm · 2024-02-02T08:23:29Z

So, can this PR be merged if everyone approves? :)

weizhouapache · 2024-02-02T09:01:33Z

So, can this PR be merged if everyone approves? :)

Not yet.
Needs someone to manually test it

DaanHoogland · 2024-02-15T12:35:45Z

@phsm I think you wanted to rebase your commit onto 4.18, did you?

weizhouapache · 2024-02-15T12:37:38Z

@phsm
please try the following commands

git fetch apache
git checkout apache/4.18
git branch -D main
git checkout -b main
git cherry-pick e34959fe66f6078b259041bd8023a1f73dec0a3c
git cherry-pick b6b7d47089d39f10712c027826ee0de830d6d9e0
git push origin main -f

Avoid an expensive getRouterHealthChecksConfig() execution multiple times during VPC restart. Fixes#8055

phsm · 2024-02-15T12:41:00Z

@phsm please try the following commands

git fetch apache
git checkout apache/4.18
git branch -D main
git checkout -b main
git cherry-pick e34959fe66f6078b259041bd8023a1f73dec0a3c
git cherry-pick b6b7d47089d39f10712c027826ee0de830d6d9e0
git push origin main -f

Thanks, that seems to have worked.

vishesh92 · 2024-02-15T16:32:16Z

@blueorangutan package

blueorangutan · 2024-02-15T16:34:04Z

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-02-15T17:35:38Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8673

vishesh92 · 2024-02-15T19:05:22Z

@blueorangutan test

blueorangutan · 2024-02-15T19:06:03Z

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

blueorangutan · 2024-02-16T12:27:31Z

[SF] Trillian test result (tid-9247)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42810 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8385-t9247-kvm-centos7.zip
Smoke tests completed. 107 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_10_reboot_cpvm_forced	`Error`	6.24	test_ssvm.py
test_08_migrate_vm	`Error`	0.05	test_vm_life_cycle.py
test_01_redundant_vpc_site2site_vpn	`Failure`	590.97	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	469.91	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	287.80	test_vpc_vpn.py

weizhouapache · 2024-02-17T08:33:25Z

@blueorangutan test rocky8 kvm-rocky8

blueorangutan · 2024-02-17T08:35:19Z

@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

vishesh92 · 2024-02-19T07:57:08Z

@blueorangutan test rocky8 kvm-rocky8

blueorangutan · 2024-02-19T07:58:03Z

@vishesh92 a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

vishesh92 · 2024-02-19T09:36:46Z

Tested these changes. I couldn't find any difference in performance for these changes. I followed these steps:

Setup 2 envs (with and without this patch)
Create a VPC with redundant offering
Add 10 networks and launch 1 VM in each
Restarted VPC with cleanup
Check the difference in created, removed in async_job table for VPCRestartCmd

phsm · 2024-02-19T09:44:41Z

Tested these changes. I couldn't find any difference in performance for these changes. I followed these steps:
1. Setup 2 envs (with and without this patch)
2. Create a VPC with redundant offering
3. Add 10 networks and launch 1 VM in each
4. Restarted VPC with cleanup
5. Check the difference in created, removed in async_job table for VPCRestartCmd

Hi,
As I mentioned earlier, this is only reproducible on CS installations "with history", i.e. with a large cloudstack database with a lot of both running and removed services (VMs, networks, VPCs etc).
Only then the getRouterHealthChecksConfig() takes 1-2 seconds to complete, which with combination of hundreds of this method executions adds a significant time to the process.

weizhouapache · 2024-02-19T09:50:57Z

Tested these changes. I couldn't find any difference in performance for these changes. I followed these steps:
Setup 2 envs (with and without this patch)
Create a VPC with redundant offering
Add 10 networks and launch 1 VM in each
Restarted VPC with cleanup
Check the difference in created, removed in async_job table for VPCRestartCmd

@vishesh92
it is probably because our database is too small.
if the platforms have been running for many years (for example, more than 5), it could make big difference.

considering this is an optimization, it looks there is no regression (in manual and smoke tests), can we say it is good to merge ? @vishesh92

vishesh92 · 2024-02-19T09:57:10Z

@weizhouapache @phsm Got it. It is good to merge from my side.

For manual testing, I just did what I mentioned in the above steps and tried to connect VMs I launched.

weizhouapache · 2024-02-19T10:07:32Z

@weizhouapache @phsm Got it. It is good to merge from my side.
For manual testing, I just did what I mentioned in the above steps and tried to connect VMs I launched.

thanks @vishesh92 for the testing

I will merge it then

boring-cyborg · 2024-02-19T10:07:51Z

Awesome work, congrats on your first merged pull request!

blueorangutan · 2024-02-19T20:42:40Z

[SF] Trillian test result (tid-9286)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 43840 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8385-t9286-kvm-rocky8.zip
Smoke tests completed. 110 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

* Optimize createMonitorServiceCommand() execution. Avoid an expensive getRouterHealthChecksConfig() execution multiple times during VPC restart. Fixesapache#8055 * Move getRouterHealthChecksConfig() outside of loop

boring-cyborgBot added component:networking component:vpc labels Dec 20, 2023

harikrishna-patnala reviewed Dec 21, 2023
View reviewed changes

sureshanaparti reviewed Dec 22, 2023
View reviewed changes

Comment threadserver/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java Outdated

sureshanaparti approved these changes Dec 22, 2023
View reviewed changes

weizhouapache approved these changes Dec 22, 2023
View reviewed changes

DaanHoogland approved these changes Jan 11, 2024
View reviewed changes

DaanHoogland added the status:needs-testing label Feb 3, 2024

DaanHoogland added this to the 4.20.0.0 milestone Feb 3, 2024

weizhouapache changed the base branch from main to 4.19February 15, 2024 09:15

weizhouapache removed this from the 4.20.0.0 milestone Feb 15, 2024

DaanHoogland added this to the 20.0.0 milestone Feb 15, 2024

phsm added 2 commits February 15, 2024 13:39

Optimize createMonitorServiceCommand() execution.
d8814b9
Avoid an expensive getRouterHealthChecksConfig() execution multiple times during VPC restart. Fixes#8055

Move getRouterHealthChecksConfig() outside of loop
3c5fc9d

phsm changed the base branch from main to 4.18February 15, 2024 12:40

DaanHoogland modified the milestones: 20.0.0, 4.18.2.0 Feb 15, 2024

weizhouapache merged commit af942e2 into apache:4.18Feb 19, 2024

weizhouapache mentioned this pull request Feb 19, 2024
Backup virtual router of a redundant VPC stays in starting state after restart with clean up. #8055
Closed

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Uh oh!

Conversation

phsm commented Dec 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

boring-cyborgBot commented Dec 20, 2023

Uh oh!

DaanHoogland commented Dec 20, 2023

Uh oh!

codecovBot commented Dec 20, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weizhouapache commented Dec 20, 2023

Uh oh!

blueorangutan commented Dec 20, 2023

Uh oh!

blueorangutan commented Dec 20, 2023

Uh oh!

harikrishna-patnala commented Dec 21, 2023

Uh oh!

blueorangutan commented Dec 21, 2023

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 21, 2023

Uh oh!

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti commented Dec 22, 2023

Uh oh!

blueorangutan commented Dec 22, 2023

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 22, 2023

Uh oh!

sureshanaparti commented Dec 22, 2023

Uh oh!

blueorangutan commented Dec 22, 2023

Uh oh!

blueorangutan commented Dec 23, 2023

Uh oh!

phsm commented Jan 11, 2024

Uh oh!

weizhouapache commented Jan 11, 2024

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

phsm commented Feb 2, 2024

Uh oh!

weizhouapache commented Feb 2, 2024

Uh oh!

DaanHoogland commented Feb 15, 2024

Uh oh!

weizhouapache commented Feb 15, 2024

Uh oh!

phsm commented Feb 15, 2024

Uh oh!

vishesh92 commented Feb 15, 2024

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

blueorangutan commented Feb 15, 2024

Uh oh!

vishesh92 commented Feb 15, 2024

Uh oh!

phsm commented Dec 20, 2023•
edited
Loading

codecovBot commented Dec 20, 2023•
edited
Loading

phsm commented Feb 19, 2024•
edited
Loading