Ability to specify NFS mount options while adding a primary storage and modify them on a pre-existing primary storage #8947

abh1sar · 2024-04-19T07:49:18Z

Description

This PR adds the UI and CLI support through which NFS mount options can be specified for a primary storage on KVM hosts.
The mount options tested for this PR are NFS version and nconnect.

Mount options can be given while adding a Primary storage using the add Zone wizard. It can also be given from the Add Primary storage form. A new field NFS mount options is provided in the respective forms.
Mount options can be later modified also from the Primary Storage detail view via the Edit Primary Storage button.

The storage pool needs to be remounted on all the hosts for the new mount option to take affect. So, for a pre-existing storage pool, it has to be in maintenance mode for this functionality to be visible.

NFS mount options are saved in storage_pool_details table and the createStoragePool and updateStoragePool APIs takes the input as a details parameter.

A note on the nconnect option.
(https://documentation.suse.com/sles/15-SP5/html/SLES-all/cha-nfs.html)

This option defines the count of TCP connections that the client makes to the NFS server. All the mounts from the same server on the client will have the same nconnect value and all mounts will share the same number of TCP connections.

The nconnect setting is applied only during the first mount process to the particular NFS server. If the same client executes the mount command to the same NFS server, all already established connections will be shared—no new connection will be established. To change the nconnect setting, you have to unmount all clients connections to the particular NFS server. Then you can define a new value of the nconnect option.

So, from CloudStack’s perspective also, the first storage pool created from a NFS server will set the nconnect setting on the hypervisor host corresponding to the server. Specifying a different nconnect mount option while creating a new storage pool from the same server will not change the nconnect setting on the host.

Similarly if there is only one pre-existing storage pool from a give NFS server mounted on the host, modifying the nconnect mount option via CloudStack will change the nconnect setting on that host.
If there are more than one storage pools from the same server mounted on a host. Changing the nconnect mount option on one of the storage pools via CloudStack will not do anything. To change the nconnect setting on the host, after modifying nconnect mount option on all storage pools, the host needs to be restarted.

This addresses #4482

Doc PR: apache/cloudstack-documentation#396

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):

Primary Storage in Maintenance mode:

Primary Storage not in Maintenance mode:

How Has This Been Tested?

Testcase : Mount failing due to incorrect mount options

Add new Primary Storage fails if incorrect mount option is given.

While updating mount options on a pre-existing pool with incorrect options, cancel maintenance mode will not be able to mount the pool and throw an error.
State of the Storage pool is changed to Up from Maintenance before mounting the pool. So, the storage pool will come out of the maintenance mode even in case of the mount error.

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

…nd modify it later

rohityadavcloud · 2024-04-19T08:05:15Z

@abh1sar given this is a bugfix/improvement related to #4482 changed PR base branch to 4.19

harikrishna-patnala · 2024-04-19T08:06:12Z

@blueorangutan package

blueorangutan · 2024-04-19T08:08:03Z

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

api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java

...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

...rvisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java

...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java

server/src/main/java/com/cloud/api/query/QueryManagerImpl.java

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

server/src/main/java/com/cloud/storage/StoragePoolAutomationImpl.java

test/integration/smoke/test_primary_storage_nfsopts_kvm.py

blueorangutan · 2024-04-19T09:15:31Z

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

harikrishna-patnala

few review comments, please address

harikrishna-patnala · 2024-04-19T09:31:59Z

...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java


+    public LibvirtStoragePoolDef(PoolType type, String poolName, String uuid, String host, String dir, String targetPath, List<String> nfsopts) {
+        this(type, poolName, uuid, host, dir, targetPath);
+        if (nfsopts != null) {


Please use CollectionUtils.isNotEmpty() and in other places as well

harikrishna-patnala · 2024-04-19T09:34:11Z

...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

+        }
+
+        List<String> nfsopts = null;
+        if (type == StoragePoolType.NetworkFilesystem) {


use equals()

harikrishna-patnala · 2024-04-19T09:42:28Z

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

        Map<String, String> details = extractApiParamAsMap(cmd.getDetails());
+        if (details.containsKey("nfsopts")) {
+            Long accountId = cmd.getEntityOwnerId();
+            if (!_accountMgr.isRootAdmin(accountId)) {


Do we need this specific check, createStoragePoolCmd is anyways a root admin command only ?

yes, it is not required here. Removed

harikrishna-patnala · 2024-04-19T09:51:11Z

ui/public/locales/en.json

 "message.action.disable.zone": "Please confirm that you want to disable this zone.",
 "message.action.download.iso": "Please confirm that you want to download this ISO.",
 "message.action.download.template": "Please confirm that you want to download this Template.",
+"message.action.edit.nfs.options": "Please confirm that you want to change NFS mount options.<br>This will cause the storage pool to be remounted on all KVM hosts.",


Do we also need to mention, changes will be in effect after cancelling the maintenance mode ?

Mentioned now. Please check

harikrishna-patnala · 2024-04-19T09:55:10Z

ui/public/locales/en.json

 "message.success.disable.saml.auth": "Successfully disabled SAML authorization",
 "message.success.disable.vpn": "Successfully disabled VPN",
 "message.success.edit.acl": "Successfully edited ACL rule",
+"message.success.edit.nfsopts": "Successfully updated NFS options",


May be here ! mentioning about the effect of changes after cancelling the maintenance mode ?

Have moved Edit NFS mount options to Edit Primary Storage. So this message was removed.

api/src/main/java/org/apache/cloudstack/api/response/StoragePoolResponse.java

...s/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java

sureshanaparti · 2024-04-19T10:16:40Z

...ns/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java

+                        optionsDiffer = true;
+                    } else {
+                        for (String nfsopt : nfsopts) {
+                            if (!poolNfsOptsMap.containsKey(nfsopt)) {


what if value changes for a key (say, nfs version updated)?

The key here contains both the option and the value. For example the key would be "vers=4.1", so if the version is changing optionsDiffer would be true.

Ok, I'm assuming key=vers & value=4.1

sureshanaparti · 2024-04-19T10:20:00Z

server/src/main/java/com/cloud/storage/StorageManagerImpl.java

+            if (optionsMap.containsKey(keyValue[0])) {
+                throw new InvalidParameterValueException("Duplicate NFS option values found for option " + keyValue[0]);
+            }
+            optionsMap.put(keyValue[0], null);


validation of nfs version and nconnect values, supported nfs versions check needed here?

Don't want to perform validations on any mount options as idea is to leave it to the admin if they are using the correct options are not. Cloudstack is only providing the functionality to add options.

We can give an indication that this feature is tested with two mount options (vers and nconnect). But I am thinking that admin can give even more mount options if they desire to do so. That way we don't have to make any changes for supporting more options.

(re-)mount may fail with incorrect options, hope the relevant message is thrown to the operator.

That's a good point. Let me test this and get back.

I have updated the Testing section in PR description with this testcase.
Had to make some changes in cancelMaintain() to note and log the exception passed from libvirt when mount fails.

I can add a few lines in the documentation PR on what happens if incorrect mount options are given if you are ok with the current behaviour.

ui/src/config/section/infra/primaryStorages.js

shwstppr

Good work @abh1sar. I see others have already given feedback so I've just added one.
See if you can add some code coverage and create documentation for the functionality

shwstppr · 2024-04-19T10:27:59Z

ui/src/config/section/infra/primaryStorages.js

+    {
+      api: 'updateStoragePool',
+      icon: 'control-outlined',
+      label: 'label.action.edit.nfs.options',
+      message: 'message.action.edit.nfs.options',
+      dataView: true,
+      popup: true,
+      show: (record) => { return (record.type === 'NetworkFilesystem' && record.hypervisor === 'KVM' && record.state === 'Maintenance') },
+      component: shallowRef(defineAsyncComponent(() => import('@/views/infra/NFSOptsPrimaryStorage.vue')))
+    },


Probably as discussed privately, move the option add NFS mount options in the edit pool form. Show the field only for supported hypervisors/parameters. And show the remount warning when the value is changed. From what I remember, update network form kind of does the same thing for offering change

JoaoJandre

CLGTM, did not test it

sureshanaparti · 2024-06-21T04:44:09Z

@blueorangutan package

blueorangutan · 2024-06-21T04:46: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.

blueorangutan · 2024-06-21T05:41:37Z

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

github-actions · 2024-06-24T04:49:52Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

abh1sar · 2024-06-24T13:44:57Z

@blueorangutan package

blueorangutan · 2024-06-24T13:46:04Z

@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 · 2024-06-24T14:05:43Z

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

abh1sar · 2024-06-24T22:05:01Z

@blueorangutan package

blueorangutan · 2024-06-24T22:06:04Z

@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 · 2024-06-24T23:08:29Z

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

sureshanaparti · 2024-06-25T04:25:11Z

@blueorangutan test

blueorangutan · 2024-06-25T04:26:03Z

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

blueorangutan · 2024-06-25T17:12:17Z

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

Test	Result	Time (s)	Test File

alexandremattioli · 2024-06-25T18:03:34Z

LGTM

…nd modify them on a pre-existing primary storage (apache#8947) * Ability to specify NFS mount options while adding a primary storage and modify it later * Pull 8947: Rename all occurrence of nfsopt to nfsMountOpt and added nfsMountOpts to ApiConstants * Pull 8947: Refactor code - move into separate methods * Pull 8947: CollectionsUtils.isNotEmpty and switch statement in LibvirtStoragePoolDef.java * Pull 8947: UI - cancel maintainenace will remount the storage pool and apply the options * Pull 8947: UI - moved edit NFS mount options to edit Primary Storage form * Pull 8947: UI - moved 'NFS Mount Options' to below 'Type' in dataview * Pull 8947: Fixed message in AddPrimaryStorage.vue * Pull 8947: Convert _nfsmountOpts to Set in libvirtStoragePoolDef * Pull 8947: Throw exception and log error if mount fails due to incorrect mount option * Pull 8947: Added UT and moved integration test to component/maint * Pull 8947: Review comments * Pull 8947: Removed password from integration test * Pull 8947: move details allocation to inside the if loop in getStoragePoolNFSMountOpts * Pull 8947: Fixed a bug in AddPrimaryStorage.vue * Pull 8947: Pool should remain in maintenance mode if mount fails * Pull 8947: Removed password from integration test * Pull 8947: Added UT * Pull 8875: Fixed a bug in CloudStackPrimaryDataStoreLifeCycleImplTest * Pull 8875: Fixed a bug in LibvirtStoragePoolDefTest * Pull 8947: minor code restructuring * Pull 8947 : added some ut for coverage * Fix LibvirtStorageAdapterTest UT

Ability to specify NFS mount options while adding a primary storage a…

4c7712c

…nd modify it later

boring-cyborg bot added component:api component:integration-test component:kvm component:storage component:UI Python Warning... Python code Ahead! labels Apr 19, 2024

abh1sar marked this pull request as draft April 19, 2024 07:51

rohityadavcloud added this to the 4.19.1.0 milestone Apr 19, 2024

rohityadavcloud changed the base branch from main to 4.19 April 19, 2024 08:05

rohityadavcloud requested review from harikrishna-patnala, rohityadavcloud, shwstppr, sureshanaparti, weizhouapache and wido April 19, 2024 08:08

DaanHoogland reviewed Apr 19, 2024

View reviewed changes

harikrishna-patnala reviewed Apr 19, 2024

View reviewed changes