CLOUDSTACK-9368: Fix for Support configurable NFS version for Secondary Storage mounts by nvazquez · Pull Request #1518 · apache/cloudstack

nvazquez · 2016-04-25T12:40:27Z

Description

JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9368
This pull request address a problem introduced in #1361 in which NFS version couldn't be changed after hosts resources were configured on startup (for hosts using VmwareResource), and as host parameters didn't include nfs.version key, it was set null.

Proposed solution

In this proposed solution nfsVersion would be passed in NfsTO through CopyCommand to VmwareResource, who will check if NFS version is still configured or not. If not, it will use the one sent in the command and will set it to its storage processor and storage handler. After those setups, it will proceed executing command.

wido · 2016-04-26T13:06:14Z

A version is a number, eg 3 or 4. Shouldn't the version be passed as a integer through the code instead of a String?

nvazquez · 2016-04-26T15:16:16Z

You're right, I just wanted to avoid parsing but I can change it to an Integer

wido · 2016-04-26T15:23:30Z

@nvazquez If you know it's a number, pass it as an int. That way you will never have a garbage string ending up somewhere. We have types for a reason :)

nvazquez · 2016-04-26T16:44:43Z

Cool, I changed it and pushed again

wido · 2016-04-26T19:11:20Z

Thanks! I'm not familiar with the VMWare code in CloudStack, so I won't give a LGTM. But passing a version as a Int seems a lot better.

nvazquez · 2016-04-26T19:46:22Z

Thanks @wido!

rafaelweingartner · 2016-04-26T19:59:51Z

@nvazquez , what about extracting that “private Integer nfsVersion” to a common superclass to all of those command class? I have created a hierarchy to help you. The classes with an * are the ones that you did not touch directly, they are there just to build the hierarchy. I found a little odd that the “ListTemplateCommand” received those values too, are they being used there?

Maybe you cannot write a single super class to attend all of those cases, but you could easily write on to be the super class for (CopyVolumeCommand, SecStorageSetupCommand, GetStorageStatsCommand, SnapshotCommand, BackupSnapshotCommand , CreatePrivateTemplateFromSnapshotCommand, CreatePrivateTemplateFromVolumeCommand, CreateVolumeFromSnapshotCommand)

Here is the hierachy:

- TemplateOrVolumePostUploadCommand
- Command*
-- StorageCommand*
--- ListTemplateCommand
-- SsCommand*
--- AbstractDownloadCommand*
---- PrimaryStorageDownloadCommand
-- CopyVolumeCommand
-- SecStorageSetupCommand
-- GetStorageStatsCommand
-- SnapshotCommand
--- BackupSnapshotCommand --- CreatePrivateTemplateFromSnapshotCommand
--- CreatePrivateTemplateFromVolumeCommand
--- CreateVolumeFromSnapshotCommand

I missing the test cases for “checkStorageProcessorAndHandlerNfsVersionAttribute”, “examineStorageSubSystemCommandNfsVersion”, “setCurrentNfsVersionInProcessorAndHandler” , “getStorageNfsVersionFromNfsTO” and “reconfigureNfsVersion” methods. They are very good coded, clean, short, well documented. To improve them further the writing of test cases is very welcome.

nvazquez · 2016-04-27T15:14:50Z

Thanks @rafaelweingartner, I like the idea, I think the best way would be leave hierarchy like this:

- Command
-- NEW CLASS
--- StorageCommand
--- SsCommand
--- CopyVolumeCommand
--- SecStorageSetupCommand
--- GetStorageStatsCommand
--- SnapshotCommand
- TemplateOrVolumePostUploadCommand

This way it can be avoided nfsVersion parameter in every class in Command hierarchy, because they won't need it, and just leave TemplateOrVolumePostUploadCommand separate in this hierarchy. Would this be ok?

rafaelweingartner · 2016-04-27T15:16:24Z

@nvazquez I believe so.
This way we can reduce a little bit of code.

nvazquez · 2016-04-29T19:13:11Z

@rafaelweingartner I added test cases and the new hierarchy

rafaelweingartner · 2016-04-29T20:56:12Z

+
+public abstract class StorageNfsVersionCommand extends Command {
+
+ protected StorageNfsVersionCommand(){


are you using this constructor?

Actually, I kept it for the hierarchy. Classes extending StorageNfsVersionCommand will not compile if I remove it, in their constructors the error will be:
Implicit super constructor StorageNfsVersionCommand() is undefined. Must explicitly invoke another constructor
Do you agree about keeping it?

sure I do, I had forgotten that in the child class they were invoking the super (). I am sorry for that.

No problem ;)

nvazquez · 2016-05-02T17:03:16Z

@rafaelweingartner @cristofolini I refactored unit tests based on your comments, I also removed testStorageNfsVersionNotPresent as it is covered by tests checkStorageProcessorAndHandlerNfsVersionAttributeVersionSet and checkStorageProcessorAndHandlerNfsVersionAttributeVersionNotSet

rafaelweingartner · 2016-05-02T19:18:09Z

+ // ---------------------------------------------------------------------------------------------------
+
+ @Test
+ public void checkStorageProcessorAndHandlerNfsVersionAttributeVersionNotSet(){


@nvazquez if the version is not set, should not we check if the method "examineStorageSubSystemCommandNfsVersion" was called?

Sounds good

rafaelweingartner · 2016-05-02T20:07:40Z

@nvazquez, that is great.
As always your PRs are perfect, great job;)
LGTM from me here.

rafaelweingartner · 2016-05-02T20:21:03Z

Just an update,
I forgot to ask, can you squash the commits?
I think this one it is better to have all of the changes into a single commit.

nvazquez · 2016-05-02T20:29:44Z

@rafaelweingartner I squashed them, thanks for your comments and reviews!

serg38 · 2016-05-03T20:09:57Z

All checks passed. @rhtyd, @wido or @GabrielBrascher can you review and give second OK.

serg38 · 2016-05-10T03:48:14Z

@koushik-das Will you be able to review this PR? It has been waiting for 2d LGTM for a while now.

koushik-das · 2016-05-10T10:45:48Z

 private Map<String, TemplateProp> listTemplate(DataStore ssStore) {
- ListTemplateCommand cmd = new ListTemplateCommand(ssStore.getTO(), imageStoreDetailsUtil.getNfsVersion(ssStore.getId()));
+ String nfsVersionParam = imageStoreDetailsUtil.getNfsVersion(ssStore.getId());
+ Integer nfsVersion = (nfsVersionParam != null ? Integer.valueOf(nfsVersionParam) : null);


Please move the nfs version conversion from string to int to a separate method, saw the same pattern at multiple places.

koushik-das · 2016-05-10T11:23:02Z

Looks like nfs version is only supported for VMware. If XS or KVM clusters are added to the zone (having nfs version set on image store) will there be any side effects?
What if this property is set on a image store which is not NFS?

@sateesh-chodapuneedi can you review the VMware code?

nvazquez · 2016-05-13T15:51:40Z

@koushik-das I only use Vmware hypervisor so I couldn't test it for Xen or KVM. However, I reviewed code and found copySnapshotToTemplateFromNfsToNfsXenserver method in NfsSecondaryStorageResource which will attemp mount using nfs version. I'm not very familiar to Xen code in CloudStack but if it uses NfsSecondaryStorage for mounting I think I can succeed, but it should be tested or improved to support those hypervisors

swill · 2016-05-13T16:33:09Z

I will run this through my KVM CI right now...

swill · 2016-05-13T19:24:58Z

I have tried to CI this PR a couple times. I get the following during Deploy DC.

==== Deploy DC Started ====
Exception Occurred :['Traceback (most recent call last):\n', ' File "/data/git/cs1/cloudstack/tools/marvin/marvin/deployDataCenter.py", line 142, in addHosts\n ret = self.__apiClient.addHost(hostcmd)\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 1530, in addHost\n response = self.connection.marvinRequest(command, response_type=response, method=method)\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest\n raise e\n', 'CloudstackAPIException: Execute cmd: addhost failed, due to: errorCode: 530, errorText:Unable to add the host\n']
Exception Occurred :['Traceback (most recent call last):\n', ' File "/data/git/cs1/cloudstack/tools/marvin/marvin/deployDataCenter.py", line 142, in addHosts\n ret = self.__apiClient.addHost(hostcmd)\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 1530, in addHost\n response = self.connection.marvinRequest(command, response_type=response, method=method)\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest\n raise e\n', 'CloudstackAPIException: Execute cmd: addhost failed, due to: errorCode: 530, errorText:Guid is not updated for cluster with specified cluster id; need to wait for hosts in this cluster to come up\n']

I can't find anything in the jetty.log to show why it is failing. I am a bit confused honestly...

serg38 · 2016-05-13T20:43:43Z

This doesn't seem to be related to this PR.Can you post managment-server.log extract around the failure time?

swill · 2016-05-13T21:12:09Z

I am running into this on a couple PRs I need to figure out what is going on...

GabrielBrascher · 2016-05-14T20:00:14Z

+
+ /**
+ * Get storage NFS version from NfsTO
+ * @param nfsTO nfsTO


@nvazquez just a small typo, change "true if NFS version if found" to "true if NFS version was found".
Good job with the code documentation, thanks.

GabrielBrascher · 2016-05-14T20:25:07Z

@nvazquez sorry for the late review,

I could point just 2 typos on Javadoc and some variables with the underscore (would be nice to change those variables names, but they are nothing serious).

Great work, I can't see anything wrong, the code LGTM.

Note: I am just giving my feedback, I don't want to run over the @koushik-das review, it would be nice to check his comment about NFS_VERSION_DETAILS_KEY at the "BaseImageStoreDriverImpl" class. Thanks.

nvazquez · 2016-05-16T16:04:56Z

@GabrielBrascher thanks for your review! I pushed changes fixing issues you've found.
@koushik-das I've replied your comment, sorry for late reply

koushik-das · 2016-05-17T11:35:21Z

@nvazquez I am not too familiar with the VMware specific code, rest of the code looks ok.

Also a comment on #1361 when this feature was initially added. NFS version is stored in image_store_details table and needs to be manually added using some sql script. A better way would be to use ConfigKey to expose the configuration. There will be some work involved to include image store as a scope for specifying configuration. May be you can fix it in subsequent PR.

swill · 2016-05-17T15:26:19Z

I am having a hard time testing this because I can't seem to get the templates to become ready using this PR. My CI ran all night last night and this morning it was still waiting for the templates to be ready. I just did another run and I am stuck at the same place.

http://localhost:8096/client/api?command=listTemplates&templatefilter=all&response=json
Tue May 17 17:17:16 2016 At least template 'tiny linux kvm' is not Ready

I am not seeing any errors in the jetty.log, so I am not sure what is going on here...

nvazquez · 2016-05-19T19:28:36Z

@koushik-das I pushed new changes including an intermediate class as you suggested
@swill hopefully CI passes now

DaanHoogland · 2016-05-19T21:13:57Z

@@ -0,0 +1,30 @@
+package org.apache.cloudstack.storage.image;


license missing

Done, thanks!

koushik-das · 2016-05-20T09:45:50Z

@nvazquez Thanks for the fix. As @DaanHoogland mentioned please add license header to the newly added file.

swill · 2016-05-20T12:37:11Z

CI RESULTS

Tests Run: 83
Skipped: 0
Failed: 0
Errors: 2
Duration: 8h 53m 51s

Summary of the problem(s):

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcRemoteAccessVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 293, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_WU9CS8/results.txt

ERROR: test suite for <class 'integration.smoke.test_vpc_vpn.TestVpcSite2SiteVpn'>
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 209, in run
self.setUp()
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 292, in setUp
self.setupContext(ancestor)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/suite.py", line 315, in setupContext
try_run(context, names)
File "/usr/lib/python2.7/site-packages/nose-1.3.7-py2.7.egg/nose/util.py", line 471, in try_run
return func()
File "/data/git/cs1/cloudstack/test/integration/smoke/test_vpc_vpn.py", line 472, in setUpClass
cls.template.download(cls.apiclient)
File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 1350, in download
elif 'Downloaded' in template.status:
TypeError: argument of type 'NoneType' is not iterable
----------------------------------------------------------------------
Additional details in: /tmp/MarvinLogs/test_network_WU9CS8/results.txt

Associated Uploads

/tmp/MarvinLogs/DeployDataCenter__May_19_2016_22_26_12_OT4DJY:

/tmp/MarvinLogs/test_network_WU9CS8:

/tmp/MarvinLogs/test_vpc_routers_U00WPJ:

Uploads will be available until 2016-07-20 02:00:00 +0200 CEST

Comment created by upr comment.

swill · 2016-05-20T12:41:27Z

This is a passing CI now. @koushik-das do you have anything outstanding with this PR?

@nvazquez can you close and reopen or do a force push to kick off a Jenkins run to see if we can get this green? Thanks for the work on this to get it ready...

nvazquez · 2016-05-20T13:39:42Z

Thanks @swill @koushik-das! Sorry I was missing a license header, I pushed it now.

koushik-das · 2016-05-20T14:07:06Z

@swill nothing outstanding on this

swill · 2016-05-20T14:23:55Z

For some reason Travis is unhappy. Would you mind trying again. Sorry for the inconvenience...

…ry Storage mounts

serg38 · 2016-05-20T16:21:58Z

@swill Travis passed. Looks like it is ready to merge.

CLOUDSTACK-9368: Fix for Support configurable NFS version for Secondary Storage mounts## Description JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9368 This pull request address a problem introduced in #1361 in which NFS version couldn't be changed after hosts resources were configured on startup (for hosts using `VmwareResource`), and as host parameters didn't include `nfs.version` key, it was set `null`. ## Proposed solution In this proposed solution `nfsVersion` would be passed in `NfsTO` through `CopyCommand` to `VmwareResource`, who will check if NFS version is still configured or not. If not, it will use the one sent in the command and will set it to its storage processor and storage handler. After those setups, it will proceed executing command. * pr/1518: CLOUDSTACK-9368: Fix for Support configurable NFS version for Secondary Storage mounts Signed-off-by: Will Stevens <williamstevens@gmail.com>

nvazquez force-pushed the testnfs branch from e139951 to 8f53714Compare April 26, 2016 16:09

rafaelweingartner reviewed Apr 29, 2016
View reviewed changes

rafaelweingartner reviewed May 2, 2016
View reviewed changes

nvazquez force-pushed the testnfs branch from dc627b0 to 5591881Compare May 2, 2016 19:28

nvazquez force-pushed the testnfs branch from 5591881 to c555420Compare May 2, 2016 20:28

nvazquez force-pushed the testnfs branch 3 times, most recently from 7ef25aa to 9e44ab9Compare May 3, 2016 18:17

koushik-das reviewed May 10, 2016
View reviewed changes

GabrielBrascher reviewed May 14, 2016
View reviewed changes

nvazquez force-pushed the testnfs branch from dbe52e6 to 777f98aCompare May 19, 2016 19:25

nvazquez force-pushed the testnfs branch from 777f98a to c667bc5Compare May 19, 2016 19:36

DaanHoogland reviewed May 19, 2016
View reviewed changes

nvazquez force-pushed the testnfs branch from c667bc5 to ba18fc5Compare May 20, 2016 13:35

CLOUDSTACK-9368: Fix for Support configurable NFS version for Seconda…
2d28199
…ry Storage mounts

nvazquez force-pushed the testnfs branch from ba18fc5 to 2d28199Compare May 20, 2016 14:31

asfgit merged commit 2d28199 into apache:masterMay 26, 2016

nvazquez deleted the testnfs branch November 4, 2016 13:25


		public abstract class StorageNfsVersionCommand extends Command {

		protected StorageNfsVersionCommand(){

		@@ -0,0 +1,30 @@
		package org.apache.cloudstack.storage.image;

Uh oh!

Conversation

nvazquez commented Apr 25, 2016

Description

Proposed solution

Uh oh!

wido commented Apr 26, 2016

Uh oh!

nvazquez commented Apr 26, 2016

Uh oh!

wido commented Apr 26, 2016

Uh oh!

nvazquez commented Apr 26, 2016

Uh oh!

wido commented Apr 26, 2016

Uh oh!

nvazquez commented Apr 26, 2016

Uh oh!

rafaelweingartner commented Apr 26, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented Apr 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner commented Apr 27, 2016

Uh oh!

nvazquez commented Apr 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvazquez commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelweingartner commented May 2, 2016

Uh oh!

rafaelweingartner commented May 2, 2016

Uh oh!

nvazquez commented May 2, 2016

Uh oh!

serg38 commented May 3, 2016

Uh oh!

serg38 commented May 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koushik-das commented May 10, 2016

Uh oh!

nvazquez commented May 13, 2016

Uh oh!

swill commented May 13, 2016

Uh oh!

swill commented May 13, 2016

Uh oh!

serg38 commented May 13, 2016

Uh oh!

swill commented May 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented May 14, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented May 16, 2016

Uh oh!

koushik-das commented May 17, 2016

Uh oh!

swill commented May 17, 2016

Uh oh!

nvazquez commented May 19, 2016

rafaelweingartner commented Apr 26, 2016•
edited
Loading

nvazquez commented Apr 27, 2016•
edited
Loading

GabrielBrascher commented May 14, 2016•
edited
Loading