Set nfsVersion in ssvm agent.properties only if it is not null #12445

abh1sar · 2026-01-16T05:33:35Z

Description

This PR fixes #12415

The agent.properties contains "nfsVersion = null" if the global settings secstorage.nfs.version is not set.
NfsSecondaryStorageResource picks up the nfsVersion as "null" String in CopyCommand and some other commands, and tries to mount the secondary storage with vers=null option leading to mount failures in logs.

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?

codecov · 2026-01-16T05:41:04Z

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (f1f779a) to head (050e0ed).
⚠️ Report is 1 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
.../secondarystorage/SecondaryStorageManagerImpl.java	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12445   +/-   ##
=========================================
  Coverage     16.23%   16.23%           
- Complexity    13381    13382    +1     
=========================================
  Files          5657     5657           
  Lines        499024   499025    +1     
  Branches      60562    60562           
=========================================
+ Hits          81029    81033    +4     
+ Misses       408959   408957    -2     
+ Partials       9036     9035    -1

Flag	Coverage Δ
uitests	`4.03% <ø> (ø)`
unittests	`17.09% <0.00%> (+<0.01%)`	⬆️

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.

abh1sar · 2026-01-16T05:54:09Z

@blueorangutan package

blueorangutan · 2026-01-16T05:54:35Z

@abh1sar 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-01-16T06:48:33Z

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

DaanHoogland

@abh1sar , this looks good, but you mention, "in CopyCommand and some other commands”. Does this mean this construct needs to be applied in several locations? (and hence methodised)

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

abh1sar · 2026-01-16T08:22:52Z

@abh1sar , this looks good, but you mention, "in CopyCommand and some other commands”. Does this mean this construct needs to be applied in several locations? (and hence methodised)

All the commands take the value from the same place.

Copilot

Pull request overview

This PR fixes a bug where the string literal "null" is written to the SSVM agent.properties file when the global setting secstorage.nfs.version is not configured. This causes NFS mount failures in the secondary storage VM because it attempts to mount with vers=null option.

Changes:

Added a null check before appending nfsVersion to boot arguments in finalizeVirtualMachineProfile method

Comments suppressed due to low confidence (1)

services/secondary-storage/controller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java:333

The same issue exists here where nfsVersion could be the string literal 'null' when secstorage.nfs.version is not configured. Consider adding a null check similar to the fix applied in finalizeVirtualMachineProfile (lines 1228-1230) to prevent setting null values on the command. This would be: if (nfsVersion != null) { setupCmd.setNfsVersion(nfsVersion); }

                String nfsVersion = imageStoreDetailsUtil.getNfsVersion(ssStore.getId());
                setupCmd.setNfsVersion(nfsVersion);

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

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

DaanHoogland · 2026-01-16T09:44:25Z

@abh1sar , this looks good, but you mention, "in CopyCommand and some other commands”. Does this mean this construct needs to be applied in several locations? (and hence methodised)

All the commands take the value from the same place.

meaning they all pass through this big ass finalizeVirtualMachineProfile() right? in that case 👍 .

DaanHoogland

clgtm, (of course this method needs chopping up.)

abh1sar · 2026-01-16T09:46:21Z

@abh1sar , this looks good, but you mention, "in CopyCommand and some other commands”. Does this mean this construct needs to be applied in several locations? (and hence methodised)

All the commands take the value from the same place.

meaning they all pass through this big ass finalizeVirtualMachineProfile() right? in that case 👍 .

Yes, they all read the value of nfsVersion from agent.properties which is set via finalizeVirtualMachineProfile()

DaanHoogland · 2026-01-16T09:49:48Z

@blueorangutan package

blueorangutan · 2026-01-16T09:50:35Z

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

blueorangutan · 2026-01-16T10:59:47Z

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

RosiKyu

LGTM

Fixes: #12415 - secstorage.nfs.version = NULL results in vers=null mount option and SSVM NFS mount failure

Pre-Test Configuration

Confirmed secstorage.nfs.version is NULL (the bug trigger condition):

mysql> SELECT name, value FROM configuration WHERE name='secstorage.nfs.version';
+------------------------+-------+
| name                   | value |
+------------------------+-------+
| secstorage.nfs.version | NULL  |
+------------------------+-------+
1 row in set (0.00 sec)

Test 1: Verify nfsVersion Not in SSVM Boot Arguments

root@s-1-VM:~# cat /var/cache/cloud/cmdline | tr ' ' '\n' | grep -i nfs root@s-1-VM:~#

root@s-1-VM:~# grep -i nfsversion /etc/cloudstack/agent/agent.properties grep: /etc/cloudstack/agent/agent.properties: No such file or directory

Result: PASS - nfsVersion parameter is ABSENT from boot arguments (not nfsVersion=null)

Test 2: Verify Existing NFS Mounts Working

root@s-1-VM:~# mount | grep nfs
10.0.32.4:/acs/secondary/ref-trl-10637-k-Mol9-rositsa-kyuchukova/ref-trl-10637-k-Mol9-rositsa-kyuchukova-sec1 on /mnt/SecStorage/db6cac5e-bd12-3be8-ad93-324d1bd6a5a9 type nfs4 (rw,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,acdirmin=0,acdirmax=0,soft,proto=tcp,timeo=133,retrans=2147483647,sec=sys,clientaddr=10.0.37.116,local_lock=none,addr=10.0.32.4)
10.0.32.4:/acs/secondary/ref-trl-10637-k-Mol9-rositsa-kyuchukova/ref-trl-10637-k-Mol9-rositsa-kyuchukova-sec2 on /mnt/SecStorage/41c5fac8-5833-3769-a25b-d326c94bf416 type nfs4 (rw,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,acdirmin=0,acdirmax=0,soft,proto=tcp,timeo=133,retrans=2147483647,sec=sys,clientaddr=10.0.37.116,local_lock=none,addr=10.0.32.4)

Result: PASS - Both sec1 and sec2 mounted successfully with auto-negotiated vers=4.2

Test 3: Verify No vers=null Errors in Logs

root@s-1-VM:~# grep -i "vers=null" /var/log/cloud.log
root@s-1-VM:~#

Result: PASS - No vers=null mount errors found in SSVM logs

Test 4: Add New Secondary Storage (Critical Test)

Added new secondary storage sec3. This is the operation that failed in the bug report.

root@s-1-VM:~# mount | grep sec3
10.0.32.4:/acs/secondary/ref-trl-10637-k-Mol9-rositsa-kyuchukova/ref-trl-10637-k-Mol9-rositsa-kyuchukova-sec3 on /mnt/SecStorage/809fc3bc-3a29-335e-ac0d-eb1acc7551bb type nfs4 (rw,relatime,vers=4.2,rsize=1048576,wsize=1048576,namlen=255,acdirmin=0,acdirmax=0,soft,proto=tcp,timeo=133,retrans=2147483647,sec=sys,clientaddr=10.0.37.116,local_lock=none,addr=10.0.32.4)

Result: PASS - sec3 mounted successfully with auto-negotiated vers=4.2, NO vers=null error

Test 5: Verify Template Copy Operations

[root@mgmt1 ~]# tail -500 /var/log/cloudstack/management/management-server.log | grep -i "sec3"
2026-01-18 07:40:34,984 INFO  [o.a.c.s.i.TemplateServiceImpl] (pool-378-thread-2:[]) (logid:793cfd5b) Copied template [routing-3] to image store [sec3].
2026-01-18 07:40:51,369 INFO  [o.a.c.s.i.TemplateServiceImpl] (pool-378-thread-1:[]) (logid:793cfd5b) Copied template [centos55-x86_64] to image store [sec3].
2026-01-18 07:41:11,790 DEBUG [c.c.s.StatsCollector] (secstorage-1:[ctx-7ae0f021]) (logid:a61f3242) Verifying image storage [ImageStore {"id":3,"name":"sec3","uuid":"60472c35-d821-46cb-a5cc-7c94abbb6da8"}]. Capacity: total=[2.6357 TB], used=[1.5384 TB], threshold=[95.00%].

Result: PASS - Templates copied successfully to new secondary storage

Test 6: Verify SSVM Initialization

root@s-1-VM:~# journalctl -u cloud --no-pager | tail -5
Jan 18 07:40:28 s-1-VM _run.sh[3439]: 07:40:28,384  INFO NfsSecondaryStorageResource:3168 - Determined host 10.0.32.4 corresponds to IP 10.0.32.4
Jan 18 07:40:28 s-1-VM _run.sh[3439]: 07:40:28,685  INFO NfsSecondaryStorageResource:3232 - snapshots directory created/exists on Secondary Storage.
Jan 18 07:40:28 s-1-VM _run.sh[3439]: 07:40:28,689  INFO NfsSecondaryStorageResource:3232 - volumes directory created/exists on Secondary Storage.
Jan 18 07:40:49 s-1-VM _run.sh[3439]: 07:40:49,349  INFO NfsSecondaryStorageResource:3168 - Determined host 10.0.32.4 corresponds to IP 10.0.32.4

Result: PASS - Secondary storage initialized successfully, required directories created

Summary

Test	Result
Existing NFS mounts (sec1, sec2)	PASS - vers=4.2
Add new secondary storage (sec3)	PASS - vers=4.2
Template copy to sec3	PASS
SSVM initialization	PASS

Conclusion

LGTM

The fix correctly omits the nfsVersion parameter from SSVM boot arguments when secstorage.nfs.version is NULL, allowing the OS to auto-negotiate the NFS version instead of failing with mount.nfs: parsing error on 'vers=' option.

abh1sar · 2026-01-18T08:17:22Z

@blueorangutan test

blueorangutan · 2026-01-18T08:18:33Z

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

blueorangutan · 2026-01-18T08:21:48Z

[SF] Trillian Build Failed (tid-15203)

set nfsVersion in ssvm agent.properties only if it is not null

722c2e3

boring-cyborg bot added component:secondary-storage component:ssvm labels Jan 16, 2026

abh1sar added this to the 4.20.3 milestone Jan 16, 2026

Merge branch '4.20' into ssvm-mount

bee935c

abh1sar requested review from vishesh92 and weizhouapache January 16, 2026 08:10

vishesh92 requested a review from Copilot January 16, 2026 08:16

Copilot started reviewing on behalf of vishesh92 January 16, 2026 08:17 View session

DaanHoogland reviewed Jan 16, 2026

View reviewed changes

vishesh92 reviewed Jan 16, 2026

View reviewed changes

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

abh1sar added 2 commits January 16, 2026 13:55

Use isNotBlank instead of null check

9946565

Merge remote-tracking branch 'origin/ssvm-mount' into ssvm-mount

c44bdce

Copilot AI reviewed Jan 16, 2026

View reviewed changes

...roller/src/main/java/org/apache/cloudstack/secondarystorage/SecondaryStorageManagerImpl.java Outdated Show resolved Hide resolved

simplified the nfsVersion assignment

050e0ed

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

DaanHoogland approved these changes Jan 16, 2026

View reviewed changes

DaanHoogland added the status:needs-testing label Jan 16, 2026

abh1sar assigned RosiKyu Jan 16, 2026

RosiKyu approved these changes Jan 18, 2026

View reviewed changes

RosiKyu removed the status:needs-testing label Jan 18, 2026

RosiKyu added the status:ready-for-merge label Jan 18, 2026

Set nfsVersion in ssvm agent.properties only if it is not null #12445

Are you sure you want to change the base?

Set nfsVersion in ssvm agent.properties only if it is not null #12445

Conversation

abh1sar commented Jan 16, 2026 • 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!

codecov bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

abh1sar commented Jan 16, 2026

Uh oh!

blueorangutan commented Jan 16, 2026

Uh oh!

blueorangutan commented Jan 16, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abh1sar commented Jan 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

DaanHoogland commented Jan 16, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

abh1sar commented Jan 16, 2026

Uh oh!

DaanHoogland commented Jan 16, 2026

Uh oh!

blueorangutan commented Jan 16, 2026

Uh oh!

blueorangutan commented Jan 16, 2026

Uh oh!

RosiKyu left a comment

Choose a reason for hiding this comment

Pre-Test Configuration

Test 1: Verify nfsVersion Not in SSVM Boot Arguments

Test 2: Verify Existing NFS Mounts Working

Test 3: Verify No vers=null Errors in Logs

Test 4: Add New Secondary Storage (Critical Test)

Test 5: Verify Template Copy Operations

Test 6: Verify SSVM Initialization

Summary

Conclusion

Uh oh!

abh1sar commented Jan 18, 2026

Uh oh!

blueorangutan commented Jan 18, 2026

Uh oh!

blueorangutan commented Jan 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

abh1sar commented Jan 16, 2026 •

edited

Loading

codecov bot commented Jan 16, 2026 •

edited

Loading