CLOUDSTACK-9252: Support configurable NFS version for Secondary Storage mounts

[nvazquez]

JIRA Ticket: https://issues.apache.org/jira/browse/CLOUDSTACK-9252

Description of the problem

After starting secondary storage VM, secondary storage tries to be mounted but fails with error: Protocol family not supported

It was found out that adding -o vers=X to mount command it would work, where X is the desired NFS version to use.
If it is desired to mount a store with a specific NFS version, it has passed in image_store_details table for a store with id Y as a property:

store_id	name	value
Y	nfs.version	X (e.g. 3)

Where X stands for NFS version

[rafaelweingartner]

Hi @ nvazquez,
Are you sure that the problem is caused because we are not telling the NFS protocol version to the mount command?

If you a look at the doc in [1] and [2], you will see that if we do not provide a NFS protocol version to be used, it (the mount program) will first try version 4, then 3, and version 2 as the last one.

If the default behavior of mount is to try every possible protocol available, I do not see why we need to add some extra code to enable us to specify one.

Wasn’t your problem caused by something else?

[1] http://man7.org/linux/man-pages/man8/mount.8.html
[2] http://man7.org/linux/man-pages/man5/nfs.5.html#MOUNT_OPTIONS

[serg38]

Unfortunately NFS version negotiation doesn't work properly with all storage vendors. Some vendors e.g. Tintri require that vers=3 is supplied in mount command likely due to the fact they don't support negotiations on the server side.
When such NFS secondary storages tries to get mounted in SSVM an exception in mount is generated
'mount.nfs: Protocol family not supported'
The same happens when trying to mount in shell so mount command needs to be told a specific version. If done so mount succeeds in both management server as well as in SSVM. Supporting nfs.version in image_store_details gives the best option to support great variety of NFS storage vendors without bluntly relying on version negotiation in nfs.mount.

[rafaelweingartner]

@serg38, I did not know that, giving your explanation I rest my arguments ;)

@nvazquez, you changed the method “getMountPoint” to receive a NFS protocol version, that method uses the version in another method that is called “mount”, in which you added a conditional to check if the version != null, then you add it to the mount command. I am ok with that code.

I only have a doubt, if the version may be required why aren’t you using it in most of the code? It seems that you mostly call the “getMountPoint” method as “mountService.getMountPoint(secondaryStorageUrl, null)”.

Actually, the only time I see you using the NFS protocol version is when you use the “prepareSecondaryStorageStore” method. As @serg38 nicely explained that there are vendors that do not support version negotiation, wouldn’t that be the case to always use the version that you already have in hand?

[serg38]

I believe the main reason is to provide the most of backward compatibility. image_store_details doesn't have NFS version in existing installations so after the upgrade to this code it will continue working without any additional changes. However if particular NFS secondary datastore requires explicit version users simply populate table and restart SSVM.

[nvazquez]

Hi @rafaelweingartner,

Thanks for your review! I agree with @serg38, it was thought just to extend the functionality by adding an entry on image_store_details table with nfs version, but if the entry was not added we wanted it to continue working as it was.

[rafaelweingartner]

Ok, I understood that you want to maintain compatibility.
What I do not understand is that: If there is a problem with some NFS (they do not support version discovery), why aren’t you always using the “mountService.getMountPoint” passing the NFS version? Most of the times you just send a null, only at the SSVM code you are using the version; wouldn’t this cause problems in some other piece of the code base such as the one you found out?
You already have the method “getNfsVersion” that will return a null if the version is not configured; I do not see a good reason not to use it; this way we would have a more concise code.

Additionally, you have duplicated the code of “getNfsVersion” in two or three places, I believe we should avoid those duplicated code.

I would also add a Java doc to the “getNfsVersion”, stating what it does, what parameters it looks for, what happens if it does not find the parameter and where (in the DB) those parameters are stored.

[nvazquez]

Ok, @rafaelweingartner I'll work on refactoring this.

[nvazquez]

@rafaelweingartner I pushed some changes according to your comments. Is it better like this?

[rafaelweingartner]

It is better now.
I only have to point two things out.
1 – About the class “com.cloud.storage.ImageStoreDetailsUtil” that you created. You are using static methods, and those static methods are using “variables” that you want to set using the spring suite. The way you did will not work; to my knowledge, those attributes will not be set by spring. If you want to set them with spring you should annotate the class with “@component”, then the object will be in the spring life cycle, and those attributes will be set properly. Of course, if you do that, remove the static from those methods (getNfsVersionByUuid, getNfsVersion) and then you would have to inject the "ImageStoreDetailsUtil" object into those classes that want to use the “getNfsVersion” methods.

2 – The java doc you created for “com.cloud.storage.ImageStoreDetailsUtil.getNfsVersion(long)” is pretty nice. I would like to see the same in “com.cloud.storage.ImageStoreDetailsUtil.getNfsVersionByUuid(String)”. Moreover, I think those methods deserve some test case.

[nvazquez]

Hi @rafaelweingartner

Thanks for your comments! I've been working following your indications, however I didn't create any test case.
I wanted to ask you, as this methods use properties from database, which way could be better to test it. I thought on unit tests, mocking daos to retrieve nfs version, would that be enough?

[rafaelweingartner]

Hi @nvazquez,
I see that you have created a spring bean as I said. However, I noticed some things that may not be needed or that seemed out of place to me. Before I start pointing them out, do you know how spring works? It is not a problem not to know in depth this kind of framework, but if you do not I could prepare some explanation in detail, so it facilitates in the future for you.

At the “spring-engine-storage-image-core-context.xml” line 31, I noticed that you added the bean “imageStoreDetailsUtilImpl” as a dependency of the “templateServiceImpl” bean; that is not necessary, if the bean “templateServiceImpl” has a property of type “imageStoreDetailsUtilImpl” annotated with "@Inject/@Autowired", Spring will automatically sort and create the hierarchy of beans dependencies to instantiate and inject. That “depends-on” configuration can be used for some things that are slightly different; if you are curious about that we can chat in off, just call me on slack or shoot me an email.

Still on “spring-engine-storage-image-core-context.xml” at line 40, you declared the bean “imageStoreDetailsUtilImpl”, that is only necessary if you were not using the “@component” annotation (there are others that you could use too, such as @service, @bean and others, each one to mark a different use type of bean); having said that, there is no need to declare the bean in the XML.

If you tried to build and run the ACS with your changes, but ended up with the application not going up because some problem with Spring dependency resolution; that might have happened because of ACS application contexts hierarchy, If that happened I can help you find the best place to declare the bean; otherwise, you can just use the annotation that is fine, no need to re-declare the bean in an XML file. I do not think that problem will happen because the bean will be created at one of the top-level application contexts of ACS.

Now about the bean itself; I noticed that you created an interface to declare the bean's methods. Normally, when we are creating DAO or service classes that is the right way to do things, create an interface and then the implementation, allowing to use object orientation (O.O.) to change the implementation in the future with configurations in an XML file; (an opinion) this is not the same as creating code for the future, but it is preparing the architecture of a system for the future, I dislike the first one and like the second one. However, with the use of annotations, it is not that easy to change implementation as it is when using XML spring beans declaration; it is not possible to inject a different object that implemented the same interface, since annotations make everything pretty straightforward, so I think it is better to lose the interface and work just with a single class that is the component itself.

Additionally, I noticed that in your interface (that I suggest you not to use in this specific case) you extended the interface “Manager” that brings a lot of things that you do not use, I am guessing you did that because you have seen some other classes, and they all do that. Well, guess what, in your case that is not needed. Actually, in most cases that the "Manager" interface is being used that interface is not needed; I find the “Manager” interface hierarchy a real nightmare, but that is a topic for another chat.

In all of the places you injected the” imageStoreDetailsUtil” object, I suggest you removing the “_” from the attribute name and making them private.

Now the problem with poor application architecture planning appears (it is not your fault, you are actually doing a great job). In some “service/manager/others” that should work as singletons, but are not, your “@Inject” will not work.
These classes “VmwareStorageManagerImpl”, “VmwareStorageProcessor”, “VmwareStorageSubsystemCommandHandler” and “DownloadManagerImpl” are manually instantiated (I might have missed some other), which means that they do not pass through the Spring framework lifecycle, which means that @Inject on them will not work. To transform them into Spring managed objects, it would require much more effort than you should be doing (you are already doing applying a great effort on this). What you can do is to get that bean during the object initialization at their constructor; you can use the “com.cloud.utils.component.ComponentContext.getApplicationContext()” to retrieve the Spring application context, and then the getBean method, to retrieve the desired bean; then you are good to set that bean in an attribute of the object and use it as you are now. Just do not forget to remove the @Inject from the aforementioned classes.

I recommend you after applying those changes, you should try to build and start up the application (ACS with your PR) to check if everything is getting injected and is working as expected. If you run into any problems, just call me.

@nvazquez, that is the way to do it, unit tests using mock DAOs, you are on the right track ;)

BTW: I liked very much your code, small methods, with test cases (still to come) and java doc

[nvazquez]

@rafaelweingartner thanks for your reply!

I'm familiar to Spring, I've worked with it, I know how it works but I wouldn't say I know it in depth. If you don't mind I would find your detailed explanation really helpful for this feature and future tasks.

About bean injection in TemplateServiceImpl, I first tryied @Inject annotation, but ACS couldn't start because it couldn't find a bean of type ImageStoreDetailsUtil. I had the same issue with @Autowired annotation. The only solution I could find was adding bean in xml context file.

I agree with you about not extending Manager interface, I saw it was always done like that and just followed the pattern. I will remove that from ImageStoreDetailsUtil

I could start ACS but didn't fully test Vmware* classes. I would try to get the context and then the bean from it as you suggested.

Thanks a lot for your help!

[rafaelweingartner]

@nvazquez, if you have any doubt in some of the spring modules, just shoot me an email (If I have the knowledge I will not hesitate to help you).
Now I see why you declared the bean in the XML file. If that was the case, then we can try to find a better place for it. If we are going to declare the bean in an XML file, you can also remove the @component annotation from it.

Looking at the ACS structure, I believe that one of the best places to put this bean would be the “spring-engine-storage-core-context”, instead of “spring-engine-storage-image-core-context”. I find it better to make the bean you created available to every project/plugin-in that works with storage.

I will be waiting to see the result from your test with a VMware environment.

[nvazquez]

Hi @DaanHoogland can you help us with Jenkins build? A test file SecondaryStorageManagerTest.java is failing, but that file is not in the branch. I could compile without problems but when I push my changes that test fails, making Jenkins fail. Do you know what could be happening?

Thanks

[cristofolini]

@nvazquez That is very strange. As far as I can tell there isn't a file with that name neither on the current master nor on your branch. There is however a file with that name and containing those exact methods (the ones causing the errors) to be added in PR #1360. That PR has not yet been merged though...

[DaanHoogland]

@nvazquez I think the job doesn't clean the prior classes well. I cleared the workspace. Can you push again?

[nvazquez]

Thanks @DaanHoogland, I pushed again

[nvazquez]

This time it failed for timeout

[rafaelweingartner]

Since it timed out, what about squashing those 3 last commits into one, and crossing the fingers hoping that jenkins succeeds.

[nvazquez]

Sure, I'll do it

[kishankavala]

@nvazquez
Apologies for reviewing it late.

1. Since version is fetched from image_store_details, can we send a map with all details for the image_store instead of just the version. This will make the approach more generic. In case more info is required for other vendors in future, too many changes can be avoided
2. Nfsversion param and corresponding getter/setter methods can be moved to a base class (something like BaseImageStoreCommand)

[kishankavala]

One more query:
How is the nfs.version set in image_store_details table?

[rafaelweingartner]

@nvazquez, luckily the Jenkins build finished with success now.

@kishankavala, for now, I believe the simplest solution is the better. When more details are necessary, we change the code to send a map or some other structure, I personally do not like using maps to move data around, I prefer simple and descriptive POJOs.

Additionally, the task to move getters and setters to an upper class is not easily achievable. I thought about that a while, and after inspecting the command classes that would have to be changed, I decided to mark that as a future work; of course, if you have an easy, nice and neat solution, you are welcome to share. @nvazquez had around worked around a lot of problems and had written a pretty nice, well documented and tested code.

@nvazquez, after all emails and messages exchanged, I can give an LGTM to this PR. Great job man ;)

[nvazquez]

Thanks a lot @rafaelweingartner for your words and your help!

[serg38]

Hi committers,
Can someone perform a second review and merge it at the earliest? We would like this to be part of 4.9?

[rafaelweingartner]

@serg38 , @nvazquez, we still need some other LGTM here. You could ask for some help reviewing this PR at Slack or @dev. Even with other LGTM, I think it would be good to have some integration test executed for this PR, just to make sure that those changes did not break something else. I know that you have already worked pretty hard here and have tested in your environment, but still I believe that those tests are needed.

[kishankavala]

@nvazquez
Can you please respond to my earlier question also.

How is the nfs.version set in image_store_details table?

[nvazquez]

Sure @kishankavala, to set NFS version for a store with id STORE_ID you'll need to insert a record in image_store_details which has:

• store_id = STORE_ID
• name = nfs.version
• value = X where X is the desired NFS version e.g. 2, 3, 4

[serg38]

If requirements to use particular NFS version is known beforehand it can be specified during the image store creation using details argument details[0].key=nfs.version&details[0].value=3 e.g.
api?command=addImageStore&response=json&name=test1&provider=NFS&zoneid=0d074f25-ed31-482f-8bc5-44c9314fc417&url=nfs%3A%2F%2F10.1.1.1%2Fnfs&details%5B0%5D.key=nfs.version&details%5B0%5D.value=3"
Unfortunately updateImageStore is not yet implemented so for an existing image store it needs to be inserted to the table manaully.

[nvazquez]

Hi @rafaelweingartner @kishankavala I've run test_ssvm.py, test_secondary_storage.py and test_vm_life_cycle.py integration tests under real hardware. This are my results:

[root@ussarlabcsmgt41 cloudstack]# cat /tmp/MarvinLogs/test_vm_life_cycle_SFRI8J/results.txt
Test system VM start ... === TestName: test_01_sys_vm_start | Status : SUCCESS ===
ok
Test system templates are ready ... === TestName: test_02_sys_template_ready | Status : SUCCESS ===
ok
Test List secondary storage VMs ... === TestName: test_01_list_sec_storage_vm | Status : SUCCESS ===
ok
Test List console proxy VMs ... === TestName: test_02_list_cpvm_vm | Status : SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
ok
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
ok
Tests for basic zone virtual router ... === TestName: test_basicZoneVirtualRouter | Status : SUCCESS ===
ok
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
ok
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
ok
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
ok
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
ok
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
ok
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
ok
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
ok
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
ok
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
ok
Test for attach and detach ISO to virtual machine ... === TestName: test_10_attachAndDetach_iso | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 24 tests in 2128.458s

OK

[GabrielBrascher]

@nvazquez LGTM based on integration tests and code.

[rafaelweingartner]

merged based on testes and reviews

[bvbharatk]

ACS CI BVT Run

Sumarry:
Build Number 116
Hypervisor xenserver
NetworkType Advanced
Passed=105
Failed=13
Skipped=4

The follwing tests have known issues
test_vpc_remote_access_vpn
test_vpc_site2site_vpn
test_01_test_vm_volume_snapshot
test_02_vpc_privategw_static_routes
test_03_rvpc_privategw_static_routes
test_04_change_offering_small
ContextSuite context=TestNiciraContoller>:setup
test_01_primary_storage_iscsi
test02_internallb_haproxy_stats_on_all_interfaces
test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80
ContextSuite context=TestDeployVM>:setup
test_04_extract_template
test_04_extract_Iso
test_07_list_default_iso
test_dedicateGuestVlanRange

Link to logs Folder (search by build_no): https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0

Failed tests:

Skipped tests:
test_vm_nic_adapter_vmxnet3
test_deploy_vgpu_enabled_vm
test_06_copy_template
test_06_copy_iso

Passed test suits:
integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData
integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup
integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire
integration.smoke.test_over_provisioning.TestUpdateOverProvision
integration.smoke.test_global_settings.TestUpdateConfigWithScope
integration.smoke.test_scale_vm.TestScaleVm
integration.smoke.test_service_offerings.TestCreateServiceOffering
integration.smoke.test_loadbalance.TestLoadBalance
integration.smoke.test_routers.TestRouterServices
integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot
integration.smoke.test_snapshots.TestSnapshotRootDisk
integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners
integration.smoke.test_network.TestDeleteAccount
integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork
integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO
integration.smoke.test_public_ip_range.TestDedicatePublicIPRange
integration.smoke.test_multipleips_per_nic.TestDeployVM
integration.smoke.test_regions.TestRegions
integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup
integration.smoke.test_network_acl.TestNetworkACL
integration.smoke.test_pvlan.TestPVLAN
integration.smoke.test_volumes.TestCreateVolume
integration.smoke.test_ssvm.TestSSVMs
integration.smoke.test_nic.TestNic
integration.smoke.test_deploy_vm_root_resize.TestDeployVM
integration.smoke.test_resource_detail.TestResourceDetail
integration.smoke.test_secondary_storage.TestSecStorageServices
integration.smoke.test_vm_life_cycle.TestDeployVM
integration.smoke.test_disk_offerings.TestCreateDiskOffering