[Kimchi-devel] [PATCH] [Kimchi 1/3] Add support to create netboot templates.

[Paulo Ricardo Paz Vital]

On Apr 18 01:09PM, Aline Manera wrote:
> 
> 
> On 04/15/2016 03:54 PM, pvital at linux.vnet.ibm.com wrote:
> >From: Paulo Vital <pvital at linux.vnet.ibm.com>
> >
> >Changed API.json and model to accept 'netboot' as source media parameter while
> >creating a new template.
> >
> >Now, when creating a new template and specifying 'netboot' as source media
> >parameter, it is assumed the template will use netboot process -
> >PXE/DHCP/TFTP/(NFS/HTTP/FTP).
> >
> >This is part of solution to Issue #372.
> >
> >Signed-off-by: Paulo Vital <pvital at linux.vnet.ibm.com>
> >---
> >  API.json           |  4 ++--
> >  model/templates.py | 11 ++++++++++-
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> >
> >diff --git a/API.json b/API.json
> >index ff505b1..2926620 100644
> >--- a/API.json
> >+++ b/API.json
> >@@ -469,9 +469,9 @@
> >                  },
> >                  "memory": { "$ref": "#/kimchitype/memory" },
> >                  "source_media": {
> >-                    "description": "Path for installation media (ISO, disk, remote ISO)",
> >+                    "description": "Path for installation media (ISO, disk, remote ISO) or netboot",
> >                      "type" : "string",
> >-                    "pattern" : "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$",
> >+                    "pattern" : "^((/)|(http)[s]?:|[t]?(ftp)[s]?:)+.*$|(netboot)",
> 
> What happens if my ISO file and image file is named as 'netboot'?

Code still works in the normal way, just because the regex is clear. Or user
inputs (1) a complete file path (starting with '/') or URL link (starting with
'http(s):' or '(t)ftp(s):'); or (2) the word 'netboot'.

So, even if a ISO file or image files is named as 'netboot', user needs to
provide the full path, matching the beginning of the regex, or it will not work.

See my test here:

$ curl -k -u test -H "Content-Type: application/json" -H "Accept: application/json" 'https://localhost:8001/plugins/kimchi/templates' -X POST -d '{"source_media": "/var/lib/libvirt/images/netboot"}'
Enter host password for user 'test':

{
  "cpu_info":{
    "maxvcpus":1,
    "vcpus":1
  },
  "graphics":{
    "type":"vnc",
    "listen":"127.0.0.1"
  },
  "cdrom":"/var/lib/libvirt/images/netboot",
  "networks":[
    "default"
  ],
  "icon":"plugins/kimchi/images/icon-ubuntu.png",
  "os_distro":"ubuntu",
  "name":"ubuntu14.04.1461002267700",
  "disks":[
    {
      "index":0,
      "format":"qcow2",
      "pool":{
        "type":"dir",
        "name":"/plugins/kimchi/storagepools/default"
      },
      "size":10
    }
  ],
  "invalid":{},
  "os_version":"14.04",
  "memory":{
    "current":1024,
    "maxmemory":1024
  },
  "folder":[]
}
> 
> It is better to do not mix things.
> 
> I suggest to have source_media type changed to a dict with path and type
> values.
> 
> Example:
> 
> source_media: {type: disk/netboot, path: <only applicable for disk type}
> 
> To specify a disk input, use type=disk and require a path value
> To create a netboot template, specify type=netboot and raise an error if any
> path is specified.
> 

I got your point and this also works, but doesn't this idea move back, let's
say half-step, to the point before Ramon's patch to automatically detect 
the type of media? 

I mean, implementing your suggestion I'll be moving most part of the user's
imput check to the model/templates.py files, instead of let JSON regex works.
IMO, the current implementation keeps things simple to read and maintain. 


> >                      "required": true
> >                  },
> >                  "disks": {
> >diff --git a/model/templates.py b/model/templates.py
> >index a02099c..043fe49 100644
> >--- a/model/templates.py
> >+++ b/model/templates.py
> >@@ -65,6 +65,12 @@ class TemplatesModel(object):
> >          # get source_media
> >          path = params.pop("source_media")
> >
> >+        # Check if source_media is 'netboot'
> >+        if path == 'netboot':
> >+            params['os_distro'] = 'unknown'
> >+            params['os_version'] = 'unknown'
> >+            return self.save_template(params)
> >+
> >          # not local image: set as remote ISO
> >          if urlparse.urlparse(path).scheme in ["http", "https", "tftp", "ftp",
> >                                                "ftps"]:
> >@@ -104,7 +110,10 @@ class TemplatesModel(object):
> >      def save_template(self, params):
> >
> >          # Creates the template class with necessary information
> >-        t = LibvirtVMTemplate(params, scan=True, conn=self.conn)
> >+        if 'cdrom' not in params.keys():
> >+            t = LibvirtVMTemplate(params, conn=self.conn)
> >+        else:
> >+            t = LibvirtVMTemplate(params, scan=True, conn=self.conn)
> >
> >          # Validate cpu info
> >          t.cpuinfo_validate()
> 

-- 
Paulo Ricardo Paz Vital
Linux Technology Center, IBM Systems
http://www.ibm.com/linux/ltc/