virt._gen_vol_xml: move all esx-specifics outside

In the near future gen_vol_xml will be able to handle many volume types,
not only for ESX volumes. For this, clean up the function from all the
ESX-specifics code and move them to the caller code.

The volume key and target path values have also been removed since those
are read-only elements that should not be provided for volume creation
as per https://libvirt.org/formatstorage.html#StorageVol

salt/modules/virt.py

@@ -665,18 +665,15 @@ def _gen_xml(
     return template.render(**context)
 
 
-def _gen_vol_xml(vmname, diskname, disktype, size, pool):
+def _gen_vol_xml(name, format, size):
     """
     Generate the XML string to define a libvirt storage volume
     """
     size = int(size) * 1024  # MB
     context = {
-        "name": vmname,
-        "filename": "{0}.{1}".format(diskname, disktype),
-        "volname": diskname,
-        "disktype": disktype,
+        "name": name,
+        "format": format,
         "size": six.text_type(size),
-        "pool": pool,
     }
     fn_ = "libvirt_volume.jinja"
     try:
@@ -1011,12 +1008,7 @@ def _disk_profile(profile, hypervisor, disks, vm_name, **kwargs):
     """
     default = [{"system": {"size": 8192}}]
     if hypervisor == "vmware":
-        overlay = {
-            "format": "vmdk",
-            "model": "scsi",
-            "device": "disk",
-            "pool": "[0] ",
-        }
+        overlay = {"format": "vmdk", "model": "scsi", "device": "disk"}
     elif hypervisor in ["qemu", "kvm"]:
         overlay = {"format": "qcow2", "device": "disk", "model": "virtio"}
     elif hypervisor == "xen":
@@ -1091,13 +1083,20 @@ def _fill_disk_filename(vm_name, disk, hypervisor, **kwargs):
                         "or is unsupported".format(disk["name"], base_dir)
                     )
                 base_dir = pool["target_path"]
+        disk["source_file"] = os.path.join(base_dir, disk["filename"])
+
     elif hypervisor == "bhyve" and vm_name:
         disk["filename"] = "{0}.{1}".format(vm_name, disk["name"])
         disk["source_file"] = os.path.join(
             "/dev/zvol", base_dir or "", disk["filename"]
         )
 
-    disk["source_file"] = os.path.join(base_dir, disk["filename"])
+        disk["source_file"] = os.path.join(base_dir, disk["filename"])
+
+    elif hypervisor in ["esxi", "vmware"]:
+        if not base_dir:
+            base_dir = __salt__["config.get"]("virt:storagepool", "[0] ")
+        disk["source_file"] = "{0}{1}".format(base_dir, disk["filename"])
 
 
 def _complete_nics(interfaces, hypervisor):
@@ -1560,10 +1559,10 @@ def init(
             else:
                 # assume libvirt manages disks for us
                 log.debug("Generating libvirt XML for %s", _disk)
-                vol_xml = _gen_vol_xml(
-                    name, _disk["name"], _disk["format"], _disk["size"], _disk["pool"]
-                )
-                define_vol_xml_str(vol_xml)
+                volume_name = "{0}/{1}".format(name, _disk["name"])
+                filename = "{0}.{1}".format(volume_name, _disk["format"])
+                vol_xml = _gen_vol_xml(filename, _disk["format"], _disk["size"])
+                define_vol_xml_str(vol_xml, pool=_disk.get("pool"))
 
         elif virt_hypervisor in ["qemu", "kvm", "xen"]:
             create_overlay = _disk.get("overlay_image", False)

salt/templates/virt/libvirt_volume.jinja

@@ -1,17 +1,10 @@
 <volume>
-  <name>{{ name }}/{{ filename }}</name>
-  <key>{{ name }}/{{ volname }}</key>
+  <name>{{ name }}</name>
   <source>
   </source>
   <capacity unit='KiB'>{{ size }}</capacity>
   <allocation unit='KiB'>0</allocation>
   <target>
-    <path>{{ pool }}{{ name }}/{{ filename }}</path>
-    <format type='{{ disktype }}'/>
-    <permissions>
-      <mode>00</mode>
-      <owner>0</owner>
-      <group>0</group>
-    </permissions>
+    <format type='{{ format }}'/>
   </target>
 </volume>

tests/unit/modules/test_virt.py

@@ -534,18 +534,17 @@ class VirtTestCase(TestCase, LoaderModuleMockMixin):
             self.assertEqual(eth0["source"], "br0")
             self.assertFalse(eth0["model"])
 
-    def test_gen_vol_xml(self):
+    def test_gen_vol_xml_esx(self):
         """
-        Test virt._get_vol_xml()
+        Test virt._get_vol_xml() for the ESX case
         """
-        xml_data = virt._gen_vol_xml(
-            "vmname", "system", "qcow2", 8192, "/path/to/image/"
-        )
+        xml_data = virt._gen_vol_xml("vmname/system.vmdk", "vmdk", 8192)
         root = ET.fromstring(xml_data)
-        self.assertEqual(root.find("name").text, "vmname/system.qcow2")
-        self.assertEqual(root.find("key").text, "vmname/system")
+        self.assertIsNone(root.get("type"))
+        self.assertEqual(root.find("name").text, "vmname/system.vmdk")
         self.assertEqual(root.find("capacity").attrib["unit"], "KiB")
         self.assertEqual(root.find("capacity").text, six.text_type(8192 * 1024))
+        self.assertEqual(root.find("target/format").get("type"), "vmdk")
 
     def test_gen_xml_for_kvm_default_profile(self):
         """