make vlan check conditional on management network for xen by DaanHoogland · Pull Request #11193 · apache/cloudstack

DaanHoogland · 2025-07-14T11:52:58Z

Description

This PR...

Fixes: #10272

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

DaanHoogland · 2025-07-14T11:56:58Z

@blueorangutan package

blueorangutan · 2025-07-14T11:58:07Z

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

codecov · 2025-07-14T12:01:08Z

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 19.41%. Comparing base (61e74e0) to head (7c1bbfb).
⚠️ Report is 1154 commits behind head on main.

Files with missing lines	Patch %	Lines
...rce/wrapper/xenbase/CitrixSetupCommandWrapper.java	0.00%	6 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main   #11193       +/-   ##
=============================================
+ Coverage     16.15%   19.41%    +3.25%     
- Complexity    13273    18458     +5185     
=============================================
  Files          5656     6095      +439     
  Lines        497728   635457   +137729     
  Branches      60360    98447    +38087     
=============================================
+ Hits          80420   123365    +42945     
- Misses       408357   499265    +90908     
- Partials       8951    12827     +3876

Flag	Coverage Δ
uitests	`4.03% <ø> (+0.03%)`	⬆️
unittests	`20.66% <0.00%> (+3.65%)`	⬆️

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.

blueorangutan · 2025-07-14T16:10:54Z

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

DaanHoogland · 2025-07-15T06:32:33Z

@blueorangutan test ol9 xcpng83

blueorangutan · 2025-07-15T06:34:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol9 mgmt + xcpng83) has been kicked to run smoke tests

blueorangutan · 2025-07-16T02:13:20Z

[SF] Trillian test result (tid-13775)
Environment: xcpng83 (x2), Advanced Networking with Mgmt server ol9
Total time taken: 68299 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11193-t13775-xcpng83.zip
Smoke tests completed. 141 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_01_prepare_and_cancel_maintenance	`Error`	0.17	test_ms_maintenance_and_safe_shutdown.py

DaanHoogland · 2025-07-16T06:40:50Z

@UAnton can you test if this satisfies your use-case, please?

UAnton · 2025-07-16T06:46:07Z

@UAnton can you test if this satisfies your use-case, please?

I can't. Too much time has passed

DaanHoogland · 2025-07-16T07:13:02Z

@UAnton can you test if this satisfies your use-case, please?

I can't. Too much time has passed

ok, should we close the issue than?

Copilot

Pull Request Overview

This PR modifies the VLAN validation logic for XenServer management networks to only apply the check for XenServer API versions prior to version 8. The change makes the VLAN configuration check conditional based on the XenServer API version, allowing management networks on VLANs for newer XenServer versions while maintaining the restriction for older versions.

Adds API version check to conditionally enforce VLAN restrictions on management networks

Copilot · 2025-07-23T17:48:47Z

                    final PIF.Record rec = pif.getRecord(conn);
                    if (rec.management) {
-                        if (rec.VLAN != null && rec.VLAN != -1) {
+                        if (host.getAPIVersionMajor(conn) < 8 && rec.VLAN != null && rec.VLAN != -1) {


The API version check should include proper null safety. Consider checking if host.getAPIVersionMajor(conn) returns a valid value before comparison to prevent potential NullPointerException.

Suggested change

if (host.getAPIVersionMajor(conn) < 8 && rec.VLAN != null && rec.VLAN != -1) {

Integer apiVersionMajor = host.getAPIVersionMajor(conn);

if (apiVersionMajor == null) {

final String msg = "Unable to determine API version for host: " + citrixResourceBase.getHost().getUuid();

logger.warn(msg);

return new SetupAnswer(command, msg);

}

if (apiVersionMajor < 8 && rec.VLAN != null && rec.VLAN != -1) {

DaanHoogland · 2025-07-23T20:22:41Z

@sureshanaparti , if there is no ask for this, I am not sure if we should put effort in it. I am not full of confidence on this implementation.

BannerSmash · 2026-05-28T15:47:37Z

Can we reopen this? This is a massive issue for us, we're looking to use XCP-ng hosts specifically for the ease of use with iSCSI block storage.

This documentation shows that as of version 8 for xenserver and xcp management networks are fine on a VLAN and a recommended path, but management PIFS of anything other than 1500 are unsupported
https://docs.xenserver.com/en-us/xenserver/8/networking.html

I have a nic bond that needs to be used for management, vlan and VXLAN traffic, the bond needs an MTU of at least 1600 to support the VXLAN overlay, if management is on the native VLAN it has to be on the bond, the reccomended way around this is to put the management PIF on a VLAN tagged network on the bond such as bond0.20 so that you can reduce the MTU for the management interface to 1500 there while leaving the overall bond at 9000 for other services.

Because of this archaic check we're basically stuck, we cant use VXLAN backed networks as we cant set our bond to anything other than 1500. I'm willing to test how do we get this moved forward in bypassing the check for version 8 and newer or allowing a user configurable bypass to it?

@DaanHoogland

DaanHoogland · 2026-05-28T16:23:11Z

that is fine @BannerSmash , but are you looking into a solution?

BannerSmash · 2026-05-28T16:30:24Z

that is fine @BannerSmash , but are you looking into a solution?

I'm not great as a developer, I saw solutions posted above with modifying the CitrixSetupCommandWrapper.java file with a version check in there, I'm willing to test but I'd need assistance.

make vlan check conditional on management network for xen

7c1bbfb

DaanHoogland added this to the 4.20.2 milestone Jul 14, 2025

boring-cyborg Bot added the component:XenServer label Jul 14, 2025

DaanHoogland mentioned this pull request Jul 14, 2025

XCP-ng Management network on VLAN #10272

Closed

sureshanaparti requested a review from Copilot July 23, 2025 17:48

Copilot AI reviewed Jul 23, 2025

View reviewed changes

weizhouapache modified the milestones: 4.20.2, 4.20.3 Sep 10, 2025

DaanHoogland closed this Oct 17, 2025

DaanHoogland reopened this May 28, 2026

-                        if (host.getAPIVersionMajor(conn) < 8 && rec.VLAN != null && rec.VLAN != -1) {
+                        Integer apiVersionMajor = host.getAPIVersionMajor(conn);
+                        if (apiVersionMajor == null) {
+                            final String msg = "Unable to determine API version for host: " + citrixResourceBase.getHost().getUuid();
+                            logger.warn(msg);
+                            return new SetupAnswer(command, msg);
+                        }
+                        if (apiVersionMajor < 8 && rec.VLAN != null && rec.VLAN != -1) {

Conversation

DaanHoogland commented Jul 14, 2025

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!

DaanHoogland commented Jul 14, 2025

Uh oh!

blueorangutan commented Jul 14, 2025

Uh oh!

codecov Bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Jul 14, 2025

Uh oh!

DaanHoogland commented Jul 15, 2025

Uh oh!

blueorangutan commented Jul 15, 2025

Uh oh!

blueorangutan commented Jul 16, 2025

Uh oh!

DaanHoogland commented Jul 16, 2025

Uh oh!

UAnton commented Jul 16, 2025

Uh oh!

DaanHoogland commented Jul 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 23, 2025

Uh oh!

BannerSmash commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented May 28, 2026

Uh oh!

BannerSmash commented May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Jul 14, 2025 •

edited

Loading

BannerSmash commented May 28, 2026 •

edited

Loading