[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1615


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r81019103
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -42,12 +43,19 @@
 class TestSSVMs(cloudstackTestCase):
 
 def setUp(self):
+   test_case = super(TestSSVMs, self)
 self.apiclient = self.testClient.getApiClient()
 self.hypervisor = self.testClient.getHypervisorInfo()
 self.cleanup = []
+   self.config = test_case.getClsConfig()
 self.services = self.testClient.getParsedTestDataConfig()
 self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
 
+self.logger = logging.getLogger('TestSSVMs')
+self.stream_handler = logging.StreamHandler()
+self.logger.setLevel(logging.DEBUG)
+self.logger.addHandler(self.stream_handler)
+
 # Default sleep is set to 90 seconds, which is too long if the 
SSVM takes up to 2min to start.
 # Second sleep in the loop will waste test time.
 self.services["sleep"] = 30
--- End diff --

Thanks for the information.  Good to know to know that it has been 
researched and that it is as low as possible.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80986927
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -42,12 +43,19 @@
 class TestSSVMs(cloudstackTestCase):
 
 def setUp(self):
+   test_case = super(TestSSVMs, self)
 self.apiclient = self.testClient.getApiClient()
 self.hypervisor = self.testClient.getHypervisorInfo()
 self.cleanup = []
+   self.config = test_case.getClsConfig()
 self.services = self.testClient.getParsedTestDataConfig()
 self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
 
+self.logger = logging.getLogger('TestSSVMs')
+self.stream_handler = logging.StreamHandler()
+self.logger.setLevel(logging.DEBUG)
+self.logger.addHandler(self.stream_handler)
+
 # Default sleep is set to 90 seconds, which is too long if the 
SSVM takes up to 2min to start.
 # Second sleep in the loop will waste test time.
 self.services["sleep"] = 30
--- End diff --

We tested changing it to 5 secs and caused much tests to fail, at least in 
out environment


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80951612
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
+if isinstance(list_ssvm_response, list):
+if list_ssvm_response[0].state == 'Running':
+break
+if timeout == 0:
+raise Exception("List SSVM call failed!")
+
+time.sleep(self.services["sleep"])
+timeout = timeout - 1
+
+   self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+ssvm = list_ssvm_response[0]
+self.debug("SSVM state after debug: %s" % ssvm.state)
+self.assertEqual(
 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80951528
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
+if isinstance(list_ssvm_response, list):
+if list_ssvm_response[0].state == 'Running':
+break
+if timeout == 0:
+raise Exception("List SSVM call failed!")
+
+time.sleep(self.services["sleep"])
+timeout = timeout - 1
--- End diff --

Done, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80951509
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
+if isinstance(list_ssvm_response, list):
+if list_ssvm_response[0].state == 'Running':
+break
+if timeout == 0:
+raise Exception("List SSVM call failed!")
+
+time.sleep(self.services["sleep"])
+timeout = timeout - 1
+
+   self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+ssvm = list_ssvm_response[0]
+self.debug("SSVM state after debug: %s" % ssvm.state)
+self.assertEqual(
 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80937979
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
--- End diff --

Done, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80701705
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
--- End diff --

Relying on the result having only one space seems brittle.  Please consider 
splitting on "on" and trimming the results or using a regular expression.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80702685
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
--- End diff --

What is this `for` accomplishing?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80704271
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
+if isinstance(list_ssvm_response, list):
+if list_ssvm_response[0].state == 'Running':
+break
+if timeout == 0:
+raise Exception("List SSVM call failed!")
+
+time.sleep(self.services["sleep"])
+timeout = timeout - 1
+
+   self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+ssvm = list_ssvm_response[0]
+self.debug("SSVM state after debug: %s" % ssvm.state)
+self.assertEqual(
 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80701833
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
--- End diff --

Please add an assertion that `list_ssvm_response` is not `None`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80702827
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
--- End diff --

Please add an assertions to check the following: 

* `list_ssvm_response` is not `None`
* `list_ssvm_response` is an instance of `list_ssvm_response`
* `list_ssvm_response` contains one element with an ID equal to `ssvm.id`

Also, why is `list_ssvm_response` being re-used in multiple contexts across 
this method?  Please consider splitting into multiple variables to more clearly 
express the intent of how each result should be used and avoid overwriting 
values in a manner that causes inconsistent behavior.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80700964
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
+cmd = stopSystemVm.stopSystemVmCmd()
+cmd.id = ssvm.id
+self.apiclient.stopSystemVm(cmd)
+
+timeout = self.services["timeout"]
+while True:
+list_ssvm_response = list_ssvms(
+self.apiclient,
+id=ssvm.id
+)
+if isinstance(list_ssvm_response, list):
+if list_ssvm_response[0].state == 'Running':
+break
+if timeout == 0:
+raise Exception("List SSVM call failed!")
+
+time.sleep(self.services["sleep"])
+timeout = timeout - 1
--- End diff --

Please consider replacing this `while` loop (lines 1299-1311) with 
`utils.waitUntil` as it provides a Pythonic/tested implementation of waiting 
for a condition to complete.  It would also clarify the intent of this section.


---
If your project is set up for it, you can reply to this email 

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80701499
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
+def test_11_ss_nfs_version_on_ssvm(self):
+"""Test NFS Version on Secondary Storage mounted properly on SSVM
+"""
+
+   # 1) List SSVM in zone
+   # 2) Get id and url from mounted nfs store
+   # 3) Update NFS version for previous image store
+   # 4) Stop SSVM
+   # 5) Check NFS version of mounted nfs store after SSVM starts 
+
+   nfs_version = self.config.nfsVersion
+   if nfs_version == None:
+   self.skipTest('No NFS version provided in test data')
+
+   #List SSVM for zone id
+list_ssvm_response = list_ssvms(
+self.apiclient,
+systemvmtype='secondarystoragevm',
+state='Running',
+zoneid=self.zone.id
+)
+self.assertEqual(
+isinstance(list_ssvm_response, list),
+True,
+"Check list response returns a valid list"
+)
+self.assertEqual(
+len(list_ssvm_response),
+1,
+"Check list System VMs response"
+)
+
+ssvm = list_ssvm_response[0]
+image_stores_response = 
ImageStore.list(self.apiclient,zoneid=self.zone.id)
+
+   if self.hypervisor.lower() in ('vmware', 'hyperv'):
+# SSH into SSVMs is done via management server for Vmware and 
Hyper-V
+result = get_process_status(
+self.apiclient.connection.mgtSvr,
+22,
+self.apiclient.connection.user,
+self.apiclient.connection.passwd,
+ssvm.privateip,
+"mount | grep 'type nfs'",
+hypervisor=self.hypervisor)
+
+   for res in result:
+split_res = res.split(" on ")
+mounted_img_store_url = split_res[0]
+for img_store in image_stores_response:
+   img_store_url = str(img_store.url)
+   if img_store_url.startswith("nfs://"):
+   img_store_url = img_store_url[6:]
+#Add colon after ip address to match output from mount 
command
+first_slash = img_store_url.find('/')
+img_store_url = img_store_url[0:first_slash] + ':' + 
img_store_url[first_slash:]
+if img_store_url == mounted_img_store_url:
+img_store_id = img_store.id
+break
+
+self.assertNotEqual(
+img_store_id,
+None,
+"Check image store id mounted on SSVM"
+)
+
+   #Update NFS version for image store mounted on SSVM
+updateConfigurationCmd = 
updateConfiguration.updateConfigurationCmd()
+updateConfigurationCmd.name = "secstorage.nfs.version"
+updateConfigurationCmd.value = nfs_version
+updateConfigurationCmd.imagestoreuuid = img_store_id
+
+updateConfigurationResponse = 
self.apiclient.updateConfiguration(updateConfigurationCmd)
+self.logger.debug("updated the parameter %s with value 
%s"%(updateConfigurationResponse.name, updateConfigurationResponse.value))
+
+   #Stop SSVM
+   self.debug("Stopping SSVM: %s" % ssvm.id)
--- End diff --

Please consider changing the log level to `INFO` as this information seems 
like it would be useful for monitoring the progress of the test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r80702139
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -42,12 +43,19 @@
 class TestSSVMs(cloudstackTestCase):
 
 def setUp(self):
+   test_case = super(TestSSVMs, self)
 self.apiclient = self.testClient.getApiClient()
 self.hypervisor = self.testClient.getHypervisorInfo()
 self.cleanup = []
+   self.config = test_case.getClsConfig()
 self.services = self.testClient.getParsedTestDataConfig()
 self.zone = get_zone(self.apiclient, 
self.testClient.getZoneForTests())
 
+self.logger = logging.getLogger('TestSSVMs')
+self.stream_handler = logging.StreamHandler()
+self.logger.setLevel(logging.DEBUG)
+self.logger.addHandler(self.stream_handler)
+
 # Default sleep is set to 90 seconds, which is too long if the 
SSVM takes up to 2min to start.
 # Second sleep in the loop will waste test time.
 self.services["sleep"] = 30
--- End diff --

I recognize that this value is out of the scope of this change.  However, 
would it make sense to reduce this value to 5 seconds?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r79419754
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="true")
--- End diff --

Done, thanks @koushik-das 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r79389208
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="false")
--- End diff --

Done, thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r79357165
  
--- Diff: test/integration/smoke/test_ssvm.py ---
@@ -1197,3 +1205,148 @@ def test_10_destroy_cpvm(self):
 # Call to verify cloud process is running
 self.test_04_cpvm_internals()
 return
+
+@attr(
+tags=[
+"advanced",
+"advancedns",
+"smoke",
+"basic",
+"sg"],
+required_hardware="false")
--- End diff --

Is it possible to run this test on Simulator? If not please change 
required_hardware to true.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r74079012
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
--- End diff --

@nvazquez that seems like a good solution.  Also, please use 
``Preconditions.checkState(globalNfsVersion !=null, "Unable to find global NFS 
version for version key" + CapacityManager.ImageStoreNFSVersion.key())`` to 
help someone understand why it happened in the unlikely event it occurs.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r74068727
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
--- End diff --

@jburwell I didn't consider it because configuration will be added and it 
will be present unless it is explicitly deleted from db. Anyways, I can add 
`Preconditions.checkState(globalNfsVersion != null)` do you agree?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r74067494
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
+return (value != null ? Integer.valueOf(value) : null);
+}
+/**
+ * Obtain NFS protocol version (if provided) for a store id, if not 
use default config value
  * @param storeId image store id
- * @return {@code null} if {@code nfs.version} is not found for 
storeId 
- * {@code X} if {@code nfs.version} is found found for storeId
+ * @return {@code null} if {@code secstorage.nfs.version} is not found 
for storeId 
+ * {@code X} if {@code secstorage.nfs.version} is found found for 
storeId
  */
 public Integer getNfsVersion(long storeId) throws 
NumberFormatException {
-String nfsVersion = null;
+Integer nfsVersion;
 if (imageStoreDetailsDao.getDetails(storeId) != null){
 Map storeDetails = 
imageStoreDetailsDao.getDetails(storeId);
--- End diff --

Done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r74067440
  
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java 
---
@@ -520,6 +528,16 @@ public String updateConfiguration(final long userId, 
final String name, final St
 _accountDetailsDao.update(accountDetailVO.getId(), 
accountDetailVO);
 }
 break;
+
+case ImageStore:
+final ImageStoreVO imgStore = 
_imageStoreDao.findById(resourceId);
+if (imgStore == null) {
+throw new InvalidParameterValueException("unable to 
find image store by id " + resourceId);
--- End diff --

Thanks, I'll take it into account for next time


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73766528
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
+return (value != null ? Integer.valueOf(value) : null);
+}
+/**
+ * Obtain NFS protocol version (if provided) for a store id, if not 
use default config value
  * @param storeId image store id
- * @return {@code null} if {@code nfs.version} is not found for 
storeId 
- * {@code X} if {@code nfs.version} is found found for storeId
+ * @return {@code null} if {@code secstorage.nfs.version} is not found 
for storeId 
+ * {@code X} if {@code secstorage.nfs.version} is found found for 
storeId
  */
 public Integer getNfsVersion(long storeId) throws 
NumberFormatException {
-String nfsVersion = null;
+Integer nfsVersion;
 if (imageStoreDetailsDao.getDetails(storeId) != null){
 Map storeDetails = 
imageStoreDetailsDao.getDetails(storeId);
--- End diff --

I recognize that this code predates this patch, but it is a double API 
call.  Consider the following refactoring:

```
public Integer getNfsVersion(long storeId) throws NumberFormatException {

final Map storeDetails = 
imageStoreDetailsDao.getDetails(storeId);
if (storeDetails != null && 
storeDetails.containsKey(CapacityManager.ImageStoreNFSVersion.key())) {
final String version = 
storeDetails.get(CapacityManager.ImageStoreNFSVersion.key());
return (val != null ? Integer.valueOf(val) : null);
}

return getGlobalDefaultNfsVersion();

}
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73765825
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
+return (value != null ? Integer.valueOf(value) : null);
+}
+/**
+ * Obtain NFS protocol version (if provided) for a store id, if not 
use default config value
  * @param storeId image store id
- * @return {@code null} if {@code nfs.version} is not found for 
storeId 
- * {@code X} if {@code nfs.version} is found found for storeId
+ * @return {@code null} if {@code secstorage.nfs.version} is not found 
for storeId 
+ * {@code X} if {@code secstorage.nfs.version} is found found for 
storeId
  */
 public Integer getNfsVersion(long storeId) throws 
NumberFormatException {
-String nfsVersion = null;
+Integer nfsVersion;
--- End diff --

Variables should always be initialized.  Consider initializing to ``null``.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73765615
  
--- Diff: server/src/com/cloud/storage/ImageStoreDetailsUtil.java ---
@@ -20,48 +20,68 @@
 
 import javax.inject.Inject;
 
+import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
+import org.apache.cloudstack.framework.config.impl.ConfigurationVO;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreDetailsDao;
 import org.apache.cloudstack.storage.datastore.db.ImageStoreVO;
 
+import com.cloud.capacity.CapacityManager;
+
 public class ImageStoreDetailsUtil {
 
 @Inject
 protected ImageStoreDao imageStoreDao;
 @Inject
 protected ImageStoreDetailsDao imageStoreDetailsDao;
+@Inject
+protected ConfigurationDao configurationDao;
 
 /**
- * Obtain NFS protocol version (if provided) for a store id.
- * It can be set by adding an entry in {@code image_store_details} 
table, providing {@code name=nfs.version} and {@code value=X} (e.g. 3)
+ * Retrieve global secondary storage NFS version default value
+ * @return global default value
+ */
+protected Integer getGlobalDefaultNfsVersion(){
+ConfigurationVO globalNfsVersion = 
configurationDao.findByName(CapacityManager.ImageStoreNFSVersion.key());
+String value = globalNfsVersion.getValue();
--- End diff --

What if ``globalNfsVersion`` is null?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73765448
  
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java 
---
@@ -520,6 +528,16 @@ public String updateConfiguration(final long userId, 
final String name, final St
 _accountDetailsDao.update(accountDetailVO.getId(), 
accountDetailVO);
 }
 break;
+
+case ImageStore:
+final ImageStoreVO imgStore = 
_imageStoreDao.findById(resourceId);
+if (imgStore == null) {
+throw new InvalidParameterValueException("unable to 
find image store by id " + resourceId);
--- End diff --

``InvalidParameterValueException`` is used to indicate that an invalid 
parameter was passed by a caller to a method.  Since ``imgStore`` was not 
passed into this method, it does not seem appropriate to use this exception 
type.  ``IllegalStateException`` seems more appropriate.  Also, consider using 
``Preconditions.checkState`` to replace the ``if`` block with a single line of 
code for greater concession.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[jburwell]

Github user jburwell commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73764835
  
--- Diff: 
api/src/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java ---
@@ -145,6 +156,9 @@ public void execute() {
 if (getAccountId() != null) {
 response.setScope("account");
 }
+if (getImageStoreId() != null) {
+response.setScope("imagestore");
+}
--- End diff --

Please remove this check and adding ``validations = PositiveNumber`` to the 
``imageStoreId @Parameter`` annotation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73283060
  
--- Diff: setup/db/db/schema-481to490.sql ---
@@ -545,3 +545,6 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.0', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.1', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.5', 'centos64Guest', 228, now(), 0);
+
+ALTER TABLE `cloud`.`image_store_details` CHANGE COLUMN `value` `value` 
VARCHAR(255) NULL DEFAULT NULL COMMENT 'value of the detail', ADD COLUMN 
`display` tinyint(1) NOT 
--- End diff --

About the display field, I didn't see that in cluster details. So looks 
like detail VOs implementing ResourceDetail have it, others don't. So for this 
PR it is fine but will be good to make them consistent across all detail VOs.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73281219
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/storage/datastore/db/ImageStoreDaoImpl.java
 ---
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.cloudstack.storage.image.db;
--- End diff --

Missed that out previously, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73156984
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/storage/datastore/db/ImageStoreDetailVO.java
 ---
@@ -23,61 +23,61 @@
 import javax.persistence.Id;
 import javax.persistence.Table;
 
-import org.apache.cloudstack.api.InternalIdentity;
+import org.apache.cloudstack.api.ResourceDetail;
 
 @Entity
 @Table(name = "image_store_details")
-public class ImageStoreDetailVO implements InternalIdentity {
+public class ImageStoreDetailVO implements ResourceDetail {
 @Id
 @GeneratedValue(strategy = GenerationType.IDENTITY)
 @Column(name = "id")
 long id;
 
 @Column(name = "store_id")
-long storeId;
+long resourceId;
 
 @Column(name = "name")
 String name;
 
 @Column(name = "value")
 String value;
 
+@Column(name = "display")
--- End diff --

We added it to make it consistent with `ResourceDetail` as we made 
`ImageStoreDetailVO` implement `ResourceDetail`, which has the method 
`isDisplay`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73156809
  
--- Diff: setup/db/db/schema-481to490.sql ---
@@ -545,3 +545,6 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.0', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.1', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.5', 'centos64Guest', 228, now(), 0);
+
+ALTER TABLE `cloud`.`image_store_details` CHANGE COLUMN `value` `value` 
VARCHAR(255) NULL DEFAULT NULL COMMENT 'value of the detail', ADD COLUMN 
`display` tinyint(1) NOT 
--- End diff --

We decided to make the default value as null to not impact deployments 
which didn't specify a NFS version for their secondary storage operations, so 
they work as they were before.

About *display* column, we added it to make it consistent with 
`ResourceDetail` as we made `ImageStoreDetailVO` implement `ResourceDetail`, 
which has the method `isDisplay`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

Github user nvazquez commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73154622
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/storage/datastore/db/ImageStoreDaoImpl.java
 ---
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.cloudstack.storage.image.db;
--- End diff --

Hi @koushik-das, the reason was to avoid a dependency cycle on Maven poms, 
I tried explaining it under *Important* section in this PR's description


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73118447
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/storage/datastore/db/ImageStoreDetailVO.java
 ---
@@ -23,61 +23,61 @@
 import javax.persistence.Id;
 import javax.persistence.Table;
 
-import org.apache.cloudstack.api.InternalIdentity;
+import org.apache.cloudstack.api.ResourceDetail;
 
 @Entity
 @Table(name = "image_store_details")
-public class ImageStoreDetailVO implements InternalIdentity {
+public class ImageStoreDetailVO implements ResourceDetail {
 @Id
 @GeneratedValue(strategy = GenerationType.IDENTITY)
 @Column(name = "id")
 long id;
 
 @Column(name = "store_id")
-long storeId;
+long resourceId;
 
 @Column(name = "name")
 String name;
 
 @Column(name = "value")
 String value;
 
+@Column(name = "display")
--- End diff --

What is the need for this field?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73118335
  
--- Diff: setup/db/db/schema-481to490.sql ---
@@ -545,3 +545,6 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.0', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.1', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.5', 'centos64Guest', 228, now(), 0);
+
+ALTER TABLE `cloud`.`image_store_details` CHANGE COLUMN `value` `value` 
VARCHAR(255) NULL DEFAULT NULL COMMENT 'value of the detail', ADD COLUMN 
`display` tinyint(1) NOT 
--- End diff --

Also what is the purpose of the display field?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73118216
  
--- Diff: setup/db/db/schema-481to490.sql ---
@@ -545,3 +545,6 @@ INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` 
(uuid,hypervisor_type, hypervis
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.0', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.1', 'centos64Guest', 228, now(), 0);
 INSERT IGNORE INTO `cloud`.`guest_os_hypervisor` (uuid,hypervisor_type, 
hypervisor_version, guest_os_name, guest_os_id, created, is_user_defined) 
VALUES (UUID(), 'VMware', '5.5', 'centos64Guest', 228, now(), 0);
+
+ALTER TABLE `cloud`.`image_store_details` CHANGE COLUMN `value` `value` 
VARCHAR(255) NULL DEFAULT NULL COMMENT 'value of the detail', ADD COLUMN 
`display` tinyint(1) NOT 
--- End diff --

Any reason to make the value field nullable?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[koushik-das]

Github user koushik-das commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1615#discussion_r73114913
  
--- Diff: 
engine/schema/src/org/apache/cloudstack/storage/datastore/db/ImageStoreDaoImpl.java
 ---
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.cloudstack.storage.image.db;
--- End diff --

What is the reason for moving this file to a new location?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request #1615: CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Ma...

[nvazquez]

GitHub user nvazquez opened a pull request:

https://github.com/apache/cloudstack/pull/1615

CLOUDSTACK-9438: Fix for CLOUDSTACK-9252 - Make NFS version changeable in UI

JIRA TICKET: https://issues.apache.org/jira/browse/CLOUDSTACK-9438

### Introduction

From #1361 it was possible to configure NFS version for secondary storage 
mount. 
However, changing NFS version requires inserting an new detail on 
`image_store_details` table, with `name = 'nfs.version'` and `value = X` where 
X is desired NFS version, and then restarting management server for changes to 
take effect.

Our improvement aims to make NFS version changeable from UI, instead of 
previously described workflow.

### Proposed solution
Basically, NFS version is defined as an image store ConfigKey, this implied:
* Adding a new Config scope: **ImageStore**
* Make `ImageStoreDetailsDao` class to extend `ResourceDetailsDaoBase` and 
`ImageStoreDetailVO` implement `ResourceDetail`
* Insert `'display'` column on `image_store_details` table
* Extending `ListCfgsCmd` and `UpdateCfgCmd` to support **ImageStore** 
scope, which implied:
** Injecting `ImageStoreDetailsDao` and `ImageStoreDao` on 
`ConfigurationManagerImpl` class, on `cloud-server` module.

### Important
It is important to mention that `ImageStoreDaoImpl` and 
`ImageStoreDetailsDaoImpl` classes were moved from `cloud-engine-storage` to 
`cloud-engine-schema` module in order to Spring find those beans to inject on 
`ConfigurationManagerImpl` in `cloud-server` module.

We had this maven dependencies between modules:
* `cloud-server --> cloud-engine-schema`
* `cloud-engine-storage --> cloud-secondary-storage --> cloud-server`

As `ImageStoreDaoImpl` and `ImageStoreDetailsDao` were defined in 
`cloud-engine-storage`, and they needed in `cloud-server` module, to be 
injected on `ConfigurationManagerImpl`, if we added dependency from 
`cloud-server` to `cloud-engine-storage` we would introduce a dependency cycle. 
To avoid this cycle, we moved those classes to `cloud-engine-schema` module

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/nvazquez/cloudstack nfsConfigKey

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/cloudstack/pull/1615.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1615


commit c35d9e6dfc6a886890ce2c1c864ec201884cb84c
Author: nvazquez 
Date:   2016-07-12T14:41:52Z

CLOUDSTACK-9438: NFS version as ConfigKey

commit 0e44c8571072f67fc53c2851dc06cd0ced2a6755
Author: nvazquez 
Date:   2016-07-19T21:33:25Z

CLOUDSTACK-9438: Move ImageStoreDaoImpl and ImageStoreDetailsDaoImpl to 
engine-schema module

commit 8383a36b703b9d5166cb1269f8400ca193f20ea1
Author: nvazquez 
Date:   2016-07-21T19:14:07Z

CLOUDSTACK-9438: Add display to ImageStoreDetailVO




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---