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.

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

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 18.04%. Comparing base (c361409) to head (5785a43).

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              @@
##               main   #12835      +/-   ##
============================================
+ Coverage     18.00%   18.04%   +0.03%     
- Complexity    16466    16499      +33     
============================================
  Files          5977     5977              
  Lines        537777   537816      +39     
  Branches      66037    66041       +4     
============================================
+ Hits          96844    97043     +199     
+ Misses       430011   429844     -167     
- Partials      10922    10929       +7

Flag	Coverage Δ
uitests	`3.52% <ø> (ø)`
unittests	`19.21% <97.72%> (+0.03%)`	⬆️

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.

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

plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/service/NsxApiClientTest.java

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?

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

nvazquez

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

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

Fix existing lbVirtualServer search by lbVirtualServerName

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

DaanHoogland · 2026-04-09T09:04:32Z

@blueorangutan package

blueorangutan · 2026-04-09T09:06:04Z

@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.

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?

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

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

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

Copilot AI reviewed Mar 17, 2026

View reviewed changes

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java Outdated Show resolved Hide resolved

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

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

Copilot AI reviewed Mar 17, 2026

View reviewed changes

winterhazel added this to the 4.23.0 milestone Mar 17, 2026

Copilot started 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 94cabcd Compare March 17, 2026 17:02

ZhyliaievD mentioned this pull request Mar 18, 2026

Fix NSX SDK list handling to fetch all paginated results #12834

Open

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

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java Outdated Show resolved Hide resolved

nvazquez approved these changes Apr 8, 2026

View reviewed changes

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/service/NsxApiClient.java Show resolved Hide resolved

fix: LB Creation avoid 404 API errors due to non-needed patches

5785a43

Fix existing lbVirtualServer search by lbVirtualServerName

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

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

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

codecov bot 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!

Copilot AI 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!

DaanHoogland 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 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!

Reviewers

codecov bot commented Mar 17, 2026 •

edited

Loading

nvazquez left a comment •

edited

Loading