fix: LB Creation avoid 404 API errors due to non-needed patches by ZhyliaievD · Pull Request #12835 · apache/cloudstack

ZhyliaievD · 2026-03-17T10:21:26Z

Description

Avoid unnecessary NSX load balancer patch calls when the target resource already exists and does not need updating.
Reuse existing LB active monitor profiles instead of patching them again, which prevents avoidable 404 API errors during LB creation.
Skip server pool patching when the current NSX pool already has the same members, and skip virtual server patching when the virtual server already exists.
Fix createAndAddNsxLbVirtualServer() to check for an existing virtual server using the virtual server name, so existing virtual servers are detected correctly and not patched again

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
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Screenshots (if appropriate):

How Has This Been Tested?

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

Copilot

Pull request overview

This PR optimizes NSX load balancer creation by avoiding unnecessary API PATCH calls when resources already exist and don't need updating. It fixes a bug where createAndAddNsxLbVirtualServer() was checking for an existing virtual server using the wrong name (the LB service name instead of the virtual server name), causing it to never detect existing virtual servers.

Changes:

Skip monitor profile patching when the profile already exists in NSX, and skip server pool patching when the pool already has the same members
Fix the virtual server existence check to use the correct virtual server name instead of the LB service name, so existing virtual servers are detected and not re-patched
Add comprehensive unit tests covering all the new skip/patch scenarios

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java`	Add existence checks for monitor profiles, server pools, and virtual servers to avoid unnecessary NSX API patch calls; fix virtual server lookup to use correct name
`plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxApiClientTest.java`	Add tests for monitor profile reuse, pool member comparison skip/patch logic, and virtual server existence detection

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-03-17T10:44:01Z

Codecov Report

❌ Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 17.99%. Comparing base (23f633a) to head (557d483).
⚠️ Report is 8 commits behind head on 4.22.

Files with missing lines	Patch %	Lines
...va/org/apache/cloudstack/service/NsxApiClient.java	97.72%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## 4.22 #12835 +/- ##
============================================
+ Coverage 17.61% 17.99% +0.37% - Complexity 15706 16557 +851 
============================================
Files 5921 6022 +101 Lines 532392 541426 +9034 Branches 65109 66350 +1241 ============================================
+ Hits 93788 97409 +3621 - Misses 428015 433043 +5028 - Partials 10589 10974 +385

Flag	Coverage Δ
uitests	`3.53% <ø> (-0.18%)`	⬇️
unittests	`19.14% <97.72%> (+0.45%)`	⬆️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland · 2026-03-17T13:57:29Z

@blueorangutan package

blueorangutan · 2026-03-17T13:58:05Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

Copilot

Pull request overview

This PR avoids unnecessary NSX API patch calls during load balancer creation by checking if resources already exist before patching, preventing 404 errors.

Changes:

Skip server pool patching when existing pool has identical members; skip virtual server patching when it already exists.
Reuse existing LB active monitor profiles instead of re-patching them, fetching by ID directly rather than listing all profiles.
Fix createAndAddNsxLbVirtualServer() to look up the virtual server by its own name instead of the LB service name.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`NsxApiClient.java`	Added existence checks for pools, monitor profiles, and virtual servers before issuing patch calls; fixed virtual server lookup key.
`NsxApiClientTest.java`	Added tests covering skip/patch scenarios for server pools, monitor profiles, virtual servers, and error handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

blueorangutan · 2026-03-17T14:49:35Z

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

ZhyliaievD · 2026-03-23T08:22:45Z

@blueorangutan test

sureshanaparti · 2026-03-23T09:00:02Z

@blueorangutan test

blueorangutan · 2026-03-23T09:02:03Z

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

blueorangutan · 2026-03-24T02:24:18Z

[SF] Trillian test result (tid-15714)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 59477 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12835-t15714-kvm-ol8.zip
Smoke tests completed. 145 look OK, 6 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_attach_and_distribute_multiple_volumes	`Failure`	88.65	test_attach_multiple_volumes.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	3752.85	test_kubernetes_clusters.py
ContextSuite context=TestListIdsParams>:teardown	`Error`	1.14	test_list_ids_parameter.py
test_01_snapshot_root_disk	`Error`	4.03	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	`Error`	47.07	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	`Error`	47.07	test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:teardown	`Error`	29.85	test_snapshots.py
test_01_snapshot_usage	`Error`	20.89	test_usage.py
test_01_vpn_usage	`Error`	1.11	test_usage.py
test_01_redundant_vpc_site2site_vpn	`Failure`	367.37	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	416.16	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	248.58	test_vpc_vpn.py

ZhyliaievD · 2026-04-06T09:47:38Z

@weizhouapache @winterhazel a kind reminder about this PR 😅
PS: test failures are not related to suggested changes(not related to nsx at all as far as I see)

winterhazel

Code looks good to me. I just left some minor remarks.

@Pearl1594 @nvazquez could you have a look at this one too?

nvazquez

Thanks for the improvement @ZhyliaievD! Code LGTM - some minor remarks raised by @winterhazel, could be great if addressed

ZhyliaievD · 2026-04-09T08:54:09Z

Thanks for the improvement @ZhyliaievD! Code LGTM - some minor remarks raised by @winterhazel, could be great if addressed

Covered all comments, thanks for review

blueorangutan · 2026-04-09T09:51:38Z

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

nvazquez · 2026-04-09T11:29:39Z

@blueorangutan package

blueorangutan · 2026-04-09T11:32:04Z

@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-04-09T12:15:28Z

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

nvazquez · 2026-04-09T13:17:41Z

@blueorangutan test

blueorangutan · 2026-04-09T13:20:05Z

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

blueorangutan · 2026-04-10T03:59:23Z

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

Test	Result	Time (s)	Test File

DaanHoogland · 2026-04-10T08:32:52Z

@nvazquez @Pearl1594 @ZhyliaievD ccan you advise on testing?

ZhyliaievD · 2026-04-15T09:24:49Z

@nvazquez @Pearl1594 @ZhyliaievD ccan you advise on testing?

Tested on 4.22.0.0 base, see #12834 (comment)

DaanHoogland · 2026-04-15T13:43:22Z

same for this one

@nvazquez @Pearl1594 @ZhyliaievD ccan you advise on testing?
Tested on 4.22.0.0 base, see #12834 (comment)

does it then make sense to base ofo of 22 and merge on that?

weizhouapache

code lgtm

better to fix into 4.22.1.0
cc @sureshanaparti @rajujith

sureshanaparti · 2026-04-15T14:12:18Z

code lgtm
better to fix into 4.22.1.0 cc @sureshanaparti @rajujith

@ZhyliaievD can re-target to 4.22?

Fix existing lbVirtualServer search by lbVirtualServerName

ZhyliaievD · 2026-04-15T14:35:04Z

@sureshanaparti Done

nvazquez · 2026-04-15T14:43:11Z

Thanks @ZhyliaievD

@blueorangutan package

blueorangutan · 2026-04-15T14:44:04Z

@nvazquez 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 · 2026-04-15T16:06:53Z

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

nvazquez · 2026-04-15T16:49:18Z

@blueorangutan test

blueorangutan · 2026-04-15T16:50:04Z

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

blueorangutan · 2026-04-16T08:16:00Z

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

Test	Result	Time (s)	Test File

boring-cyborgBot added the component:networking label Mar 17, 2026

DaanHoogland requested review from Pearl1594, Copilot and nvazquez March 17, 2026 10:33

Copilotstarted reviewing on behalf of DaanHoogland March 17, 2026 10:38 View session

CopilotAI reviewed Mar 17, 2026
View reviewed changes

Comment threadplugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java Outdated

ZhyliaievD force-pushed the fix/nsx-lb-404-api-errors branch from 9f079d8 to 4ea0f3dCompare March 17, 2026 13:04

DaanHoogland requested a review from Copilot March 17, 2026 13:57

CopilotAI reviewed Mar 17, 2026
View reviewed changes

winterhazel added this to the 4.23.0 milestone Mar 17, 2026

Copilotstarted reviewing on behalf of DaanHoogland March 17, 2026 14:10 View session

winterhazel added status:needs-testing status:needs-review labels Mar 17, 2026

ZhyliaievD force-pushed the fix/nsx-lb-404-api-errors branch from 4ea0f3d to 94cabcdCompare March 17, 2026 17:02

ZhyliaievD mentioned this pull request Mar 18, 2026
Fix NSX SDK list handling to fetch all paginated results #12834
Merged
14 tasks

winterhazel self-requested a review April 6, 2026 18:52

winterhazel approved these changes Apr 6, 2026
View reviewed changes

winterhazel reviewed Apr 6, 2026
View reviewed changes

Comment threadplugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java Outdated

nvazquez approved these changes Apr 8, 2026
View reviewed changes

Comment threadplugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java

ZhyliaievD force-pushed the fix/nsx-lb-404-api-errors branch from 94cabcd to 5785a43Compare April 9, 2026 08:46

DaanHoogland removed the status:needs-review label Apr 10, 2026

weizhouapache approved these changes Apr 15, 2026
View reviewed changes

fix: LB Creation avoid 404 API errors due to non-needed patches
557d483
Fix existing lbVirtualServer search by lbVirtualServerName

ZhyliaievD force-pushed the fix/nsx-lb-404-api-errors branch from 5785a43 to 557d483Compare April 15, 2026 14:34

ZhyliaievD changed the base branch from main to 4.22April 15, 2026 14:34

sureshanaparti modified the milestones: 4.23.0, 4.22.1 Apr 16, 2026

sureshanaparti added this to Apache CloudStack 4.22.1 Apr 16, 2026

github-project-automationBot moved this to Todo in Apache CloudStack 4.22.1 Apr 16, 2026

sureshanaparti moved this from Todo to In Progress in Apache CloudStack 4.22.1 Apr 16, 2026

sureshanaparti merged commit 05c5963 into apache:4.22Apr 16, 2026
25 of 28 checks passed

github-project-automationBot moved this from In Progress to Done in Apache CloudStack 4.22.1 Apr 16, 2026

Uh oh!

Conversation

ZhyliaievD commented Mar 17, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecovBot commented Mar 17, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Mar 17, 2026

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

ZhyliaievD commented Mar 23, 2026

Uh oh!

sureshanaparti commented Mar 23, 2026

Uh oh!

blueorangutan commented Mar 23, 2026

Uh oh!

blueorangutan commented Mar 24, 2026

Uh oh!

ZhyliaievD commented Apr 6, 2026

Uh oh!

winterhazel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvazquez left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZhyliaievD commented Apr 9, 2026

Uh oh!

blueorangutan commented Apr 9, 2026

Uh oh!

nvazquez commented Apr 9, 2026

Uh oh!

blueorangutan commented Apr 9, 2026

Uh oh!

blueorangutan commented Apr 9, 2026

Uh oh!

nvazquez commented Apr 9, 2026

Uh oh!

blueorangutan commented Apr 9, 2026

Uh oh!

blueorangutan commented Apr 10, 2026

Uh oh!

DaanHoogland commented Apr 10, 2026

Uh oh!

ZhyliaievD commented Apr 15, 2026

Uh oh!

DaanHoogland commented Apr 15, 2026

Uh oh!

codecovBot commented Mar 17, 2026•
edited
Loading

nvazquez left a comment •
edited
Loading